date:20220930

flight 173388 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173388/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-i386   broken
 build-i386-pvops broken
 build-i386-xsm   broken
 build-i386-xsm4 host-install(4)broken REGR. vs. 173364
 build-i3864 host-install(4)broken REGR. vs. 173364
 build-i386-pvops  4 host-install(4)broken REGR. vs. 173364

Tests which did not succeed, but are not blocking:
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 173364
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 173364
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 173364
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 173364
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 173364
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 173364
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 173364
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 173364
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass

version targeted for testing:
 linux5a77386984b513ebfb2700e70dac44509fc81aa9
baseline version:
 linux511cce163b75bc3933fa3de769a82bb7e8663f2b

Last test of basis   173364  2022-09-29 15:40:21 Z

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, Sep 30, 2022 at 08:27:09PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 19:12, Demi Marie Obenour wrote:
> > On Fri, Sep 30, 2022 at 06:30:57PM +0200, Ard Biesheuvel wrote:
> > > I know very little about Xen, but based on the context you provided in
> > > this thread, I'd say that the best approach from the Xen side is to
> > > convert all EfiBootServicesData regions that have configuration tables
> > > pointing into them into EfiAcpiReclaimMemory.
> >
> > Should Xen convert the entire region, or should it try to reserve only
> > the memory it needs?  The latter would require it to parse the
> > configuration tables.  Is there a list of configuration tables that can
> > legitimately be in EfiBootServicesData regions?
> >
> 
> Not really, no. So you would have to convert the entire region
> /unless/ Xen knows the GUID, and therefore knows how to derive the
> size of the table, allowing it to reserve memory more conservatively.
> However, I doubt whether this is worth it: splitting entries implies
> rewriting the memory map, which is a thing I'd rather avoid if I were
> in your shoes.

I actually wonder if Xen needs to reserve *all* of EfiBootServicesData.
The reason is that some (probably buggy) firmware may store ACPI tables
there, and Xen does not have an ACPI implementation.  From my
perspective, a much safer approach would be to pass all of
EfiBootServicesData memory directly to dom0, and have dom0 give Xen back
what it doesn’t wind up using.  That allows dom0’s memory reservation
code to work properly, which it currently does not.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

[ovmf test] 173386: trouble: blocked/broken/pass

flight 173386 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173386/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-i386   broken
 build-i386-pvops broken
 build-i386-xsm   broken
 build-i386-xsm4 host-install(4)broken REGR. vs. 173356
 build-i3864 host-install(4)broken REGR. vs. 173356
 build-i386-pvops  4 host-install(4)broken REGR. vs. 173356

Tests which did not succeed, but are not blocking:
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a

version targeted for testing:
 ovmf 7aa06237b856fd6f8187cc1715a3fe08ab4e98ed
baseline version:
 ovmf b7213bbd59833fb0786c83a28df5f8244602ab5e

Last test of basis   173356  2022-09-28 14:40:29 Z2 days
Testing same since   173386  2022-09-30 12:10:28 Z0 days1 attempts


People who touched revisions under test:
  Rodrigo Gonzalez Del Cueto 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   broken  
 build-amd64  pass
 build-i386   broken  
 build-amd64-libvirt  pass
 build-i386-libvirt   blocked 
 build-amd64-pvopspass
 build-i386-pvops broken  
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  blocked 



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary

broken-job build-i386 broken
broken-job build-i386-pvops broken
broken-job build-i386-xsm broken
broken-step build-i386-xsm host-install(4)
broken-step build-i386 host-install(4)
broken-step build-i386-pvops host-install(4)

Not pushing.


commit 7aa06237b856fd6f8187cc1715a3fe08ab4e98ed
Author: Rodrigo Gonzalez Del Cueto 
Date:   Thu Sep 22 15:35:36 2022 +0800

SecurityPkg: Remove enforcement of final GoIdle transition for CRB commands

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4077

Following the design described in the TCG specification,
the driver implementation of the CRB protocol does not require
enforcing completing the transition to goIdle at the end of a command
sequence.

Signed-off-by: Rodrigo Gonzalez Del Cueto 

Cc: Jiewen Yao 
Cc: Jian J Wang 
Reviewed-by: Jian J Wang

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, Sep 30, 2022 at 10:59:49PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
>  wrote:
> >
> > On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > >  wrote:
> > > >
> > > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > >  wrote:
> > > > > >
> > > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > > discover which firmware can be updated by the OS.  Currently, Linux 
> > > > > > does
> > > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is 
> > > > > > not
> > > > > > possible to use fwupd in a Xen dom0, which is a serious problem for 
> > > > > > e.g.
> > > > > > Qubes OS.
> > > > > >
> > > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > > The UEFI specification requires the ESRT to be in 
> > > > > > EfiBootServicesData
> > > > > > memory, which Xen will use for whatever purposes it likes.  
> > > > > > Therefore,
> > > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > > >
> > > > > > Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > > > EfiBootServicesData
> > > > > > or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > > > EfiBootServicesData
> > > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > > reserved.  Such memory is currently of type 
> > > > > > EFI_RUNTIME_SERVICES_DATA,
> > > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > > >
> > > > > > When running as a Xen dom0, use the new
> > > > > > xen_config_table_memory_region_max() function to determine if Xen 
> > > > > > has
> > > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > > containing it.  This allows programs such as fwupd which require the
> > > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS 
> > > > > > possible.
> > > > > >
> > > > > > Signed-off-by: Demi Marie Obenour 
> > > > >
> > > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > > false when patch 1/2 is applied.
> > > >
> > > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > > alternative way to get the end of the memory region containing the ESRT.
> > > > That is what this patch provides.
> > >
> > > OK. I don't think we need that to be honest. When running under Xen,
> > > we should be able to assume that the ESRT does not span multiple
> > > memory regions arbitrarily, so we can just omit this check if
> > > !efi_enabled(EFI_MEMMAP)
> > >
> > > IIRC (and Peter would know), we are trying to filter out descriptors
> > > that are completely bogus here: zero lenght, zero address, etc etc. I
> > > don't think we need that for Xen.
> >
> > Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> > under Xen than on bare hardware.
> 
> That may be true. But if Xen needs dom0 to be able to cross reference
> the EFI memory map, it should provide one (and set EFI_MEMMAP to
> enabled).

I agree, but it is also a significant amount of work compared to this
patch.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, Sep 30, 2022 at 11:24:37PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 22:59, Ard Biesheuvel  wrote:
> >
> > On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
> >  wrote:
> > >
> > > On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > > > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > > >  wrote:
> > > > >
> > > > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > > >  wrote:
> > > > > > >
> > > > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > > > discover which firmware can be updated by the OS.  Currently, 
> > > > > > > Linux does
> > > > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is 
> > > > > > > not
> > > > > > > possible to use fwupd in a Xen dom0, which is a serious problem 
> > > > > > > for e.g.
> > > > > > > Qubes OS.
> > > > > > >
> > > > > > > Before Xen 4.17, this was not fixable due to hypervisor 
> > > > > > > limitations.
> > > > > > > The UEFI specification requires the ESRT to be in 
> > > > > > > EfiBootServicesData
> > > > > > > memory, which Xen will use for whatever purposes it likes.  
> > > > > > > Therefore,
> > > > > > > Linux cannot safely access the ESRT, as Xen may have overwritten 
> > > > > > > it.
> > > > > > >
> > > > > > > Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > > > > EfiBootServicesData
> > > > > > > or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > > > > EfiBootServicesData
> > > > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > > > reserved.  Such memory is currently of type 
> > > > > > > EFI_RUNTIME_SERVICES_DATA,
> > > > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  
> > > > > > > This
> > > > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > > > >
> > > > > > > When running as a Xen dom0, use the new
> > > > > > > xen_config_table_memory_region_max() function to determine if Xen 
> > > > > > > has
> > > > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > > > containing it.  This allows programs such as fwupd which require 
> > > > > > > the
> > > > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS 
> > > > > > > possible.
> > > > > > >
> > > > > > > Signed-off-by: Demi Marie Obenour 
> > > > > >
> > > > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > > > false when patch 1/2 is applied.
> > > > >
> > > > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > > > alternative way to get the end of the memory region containing the 
> > > > > ESRT.
> > > > > That is what this patch provides.
> > > >
> > > > OK. I don't think we need that to be honest. When running under Xen,
> > > > we should be able to assume that the ESRT does not span multiple
> > > > memory regions arbitrarily, so we can just omit this check if
> > > > !efi_enabled(EFI_MEMMAP)
> > > >
> > > > IIRC (and Peter would know), we are trying to filter out descriptors
> > > > that are completely bogus here: zero lenght, zero address, etc etc. I
> > > > don't think we need that for Xen.
> > >
> > > Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> > > under Xen than on bare hardware.
> >
> > That may be true. But if Xen needs dom0 to be able to cross reference
> > the EFI memory map, it should provide one (and set EFI_MEMMAP to
> > enabled).
> 
> Btw the efi_mem_reserve() for the ESRT is also redundant if it is
> guaranteed to be in RT services data or ACPI reclaim memory.

It’s needed on bare hardware.  On Xen it’s unreachable code.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

[xen-unstable test] 173385: regressions - trouble: blocked/broken/fail/pass

flight 173385 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173385/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-i386   broken
 build-i386-prev  broken
 build-i386-pvops broken
 build-i386-xsm   broken
 build-i386-xsm4 host-install(4)broken REGR. vs. 173379
 build-i386-pvops  4 host-install(4)broken REGR. vs. 173379
 build-i386-prev   4 host-install(4)broken REGR. vs. 173379
 build-i3864 host-install(4)broken REGR. vs. 173379
 test-arm64-arm64-xl-xsm   8 xen-boot fail REGR. vs. 173379
 test-arm64-arm64-libvirt-raw 13 guest-start  fail REGR. vs. 173379

Tests which did not succeed, but are not blocking:
 test-amd64-i386-examine-bios  1 build-check(1)   blocked  n/a
 test-amd64-i386-examine-uefi  1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-raw   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-amd64-i386-livepatch 1 build-check(1)   blocked  n/a
 test-amd64-i386-migrupgrade   1 build-check(1)   blocked  n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-qemut-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-qemut-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-xl1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-pvshim 1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemut-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 1 build-check(1) blocked 
n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 1 build-check(1) blocked 
n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-ws16-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-shadow 1 build-check(1)   blocked  n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemut-ws16-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemut-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-coresched-i386-xl  1 build-check(1)   blocked  n/a
 test-amd64-i386-examine   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-vhd1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-xsm1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 173379
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 173379
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 173379
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 173379
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 173379
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 173379
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 173379
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 173379
 test-arm64-arm64-xl-seattle  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, 30 Sept 2022 at 22:59, Ard Biesheuvel  wrote:
>
> On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
>  wrote:
> >
> > On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > >  wrote:
> > > >
> > > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > >  wrote:
> > > > > >
> > > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > > discover which firmware can be updated by the OS.  Currently, Linux 
> > > > > > does
> > > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is 
> > > > > > not
> > > > > > possible to use fwupd in a Xen dom0, which is a serious problem for 
> > > > > > e.g.
> > > > > > Qubes OS.
> > > > > >
> > > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > > The UEFI specification requires the ESRT to be in 
> > > > > > EfiBootServicesData
> > > > > > memory, which Xen will use for whatever purposes it likes.  
> > > > > > Therefore,
> > > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > > >
> > > > > > Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > > > EfiBootServicesData
> > > > > > or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > > > EfiBootServicesData
> > > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > > reserved.  Such memory is currently of type 
> > > > > > EFI_RUNTIME_SERVICES_DATA,
> > > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > > >
> > > > > > When running as a Xen dom0, use the new
> > > > > > xen_config_table_memory_region_max() function to determine if Xen 
> > > > > > has
> > > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > > containing it.  This allows programs such as fwupd which require the
> > > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS 
> > > > > > possible.
> > > > > >
> > > > > > Signed-off-by: Demi Marie Obenour 
> > > > >
> > > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > > false when patch 1/2 is applied.
> > > >
> > > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > > alternative way to get the end of the memory region containing the ESRT.
> > > > That is what this patch provides.
> > >
> > > OK. I don't think we need that to be honest. When running under Xen,
> > > we should be able to assume that the ESRT does not span multiple
> > > memory regions arbitrarily, so we can just omit this check if
> > > !efi_enabled(EFI_MEMMAP)
> > >
> > > IIRC (and Peter would know), we are trying to filter out descriptors
> > > that are completely bogus here: zero lenght, zero address, etc etc. I
> > > don't think we need that for Xen.
> >
> > Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> > under Xen than on bare hardware.
>
> That may be true. But if Xen needs dom0 to be able to cross reference
> the EFI memory map, it should provide one (and set EFI_MEMMAP to
> enabled).

Btw the efi_mem_reserve() for the ESRT is also redundant if it is
guaranteed to be in RT services data or ACPI reclaim memory.

[PATCH] Use EfiACPIReclaimMemory for ESRT

As discussed on xen-devel, using EfiRuntimeServicesData for more than is
absolutely necessary is a bad idea.
---
 xen/common/efi/boot.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 
db0340c8e2628314226c618dda11ede4c62fdf3b..dba23439758d1e842d267dcd19448e0f9113b115
 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -601,11 +601,13 @@ static size_t __init get_esrt_size(const 
EFI_MEMORY_DESCRIPTOR *desc)
 if ( physical_start > esrt || esrt - physical_start >= len )
 return 0;
 /*
- * The specification requires EfiBootServicesData, but accept
- * EfiRuntimeServicesData, which is a more logical choice.
+ * The specification requires EfiBootServicesData, but also accept
+ * EfiRuntimeServicesData (for compatibility) and EfiACPIReclaimMemory
+ * (which will contain the tables after successful kexec).
  */
 if ( (desc->Type != EfiRuntimeServicesData) &&
- (desc->Type != EfiBootServicesData) )
+ (desc->Type != EfiBootServicesData) &&
+ (desc->Type != EfiACPIReclaimMemory) )
 return 0;
 available_len = len - (esrt - physical_start);
 if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
@@ -1144,18 +1146,19 @@ static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE 
*SystemTable)
 for ( i = 0; i < info_size; i += mdesc_size )
 {
 /*
- * ESRT needs to be moved to memory of type EfiRuntimeServicesData
+ * ESRT needs to be moved to memory of type EfiACPIReclaimMemory
  * so that the memory it is in will not be used for other purposes.
  */
 void *new_esrt = NULL;
-size_t esrt_size = get_esrt_size(memory_map + i);
+const EFI_MEMORY_DESCRIPTOR *desc = memory_map + i;
+size_t esrt_size = get_esrt_size(desc);
 
 if ( !esrt_size )
 continue;
-if ( ((EFI_MEMORY_DESCRIPTOR *)(memory_map + i))->Type ==
- EfiRuntimeServicesData )
+if ( desc->Type == EfiRuntimeServicesData ||
+ desc->Type == EfiACPIReclaimMemory )
 break; /* ESRT already safe from reuse */
-status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
+status = efi_bs->AllocatePool(EfiACPIReclaimMemory, esrt_size,
   _esrt);
 if ( status == EFI_SUCCESS && new_esrt )
 {
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> >  wrote:
> > >
> > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > >  wrote:
> > > > >
> > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > discover which firmware can be updated by the OS.  Currently, Linux 
> > > > > does
> > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > > possible to use fwupd in a Xen dom0, which is a serious problem for 
> > > > > e.g.
> > > > > Qubes OS.
> > > > >
> > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > >
> > > > > Starting with Xen 4.17, Xen checks if the ESRT is in 
> > > > > EfiBootServicesData
> > > > > or EfiRuntimeServicesData memory.  If the ESRT is in 
> > > > > EfiBootServicesData
> > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > >
> > > > > When running as a Xen dom0, use the new
> > > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > containing it.  This allows programs such as fwupd which require the
> > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS 
> > > > > possible.
> > > > >
> > > > > Signed-off-by: Demi Marie Obenour 
> > > >
> > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > false when patch 1/2 is applied.
> > >
> > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > alternative way to get the end of the memory region containing the ESRT.
> > > That is what this patch provides.
> >
> > OK. I don't think we need that to be honest. When running under Xen,
> > we should be able to assume that the ESRT does not span multiple
> > memory regions arbitrarily, so we can just omit this check if
> > !efi_enabled(EFI_MEMMAP)
> >
> > IIRC (and Peter would know), we are trying to filter out descriptors
> > that are completely bogus here: zero lenght, zero address, etc etc. I
> > don't think we need that for Xen.
>
> Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> under Xen than on bare hardware.

That may be true. But if Xen needs dom0 to be able to cross reference
the EFI memory map, it should provide one (and set EFI_MEMMAP to
enabled).

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
>  wrote:
> >
> > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > >  wrote:
> > > >
> > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > > Qubes OS.
> > > >
> > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > >
> > > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > ensures that the ESRT can safely be accessed by the OS.
> > > >
> > > > When running as a Xen dom0, use the new
> > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > reserved the ESRT and, if so, find the end of the memory region
> > > > containing it.  This allows programs such as fwupd which require the
> > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > > >
> > > > Signed-off-by: Demi Marie Obenour 
> > >
> > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > false when patch 1/2 is applied.
> >
> > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > alternative way to get the end of the memory region containing the ESRT.
> > That is what this patch provides.
> 
> OK. I don't think we need that to be honest. When running under Xen,
> we should be able to assume that the ESRT does not span multiple
> memory regions arbitrarily, so we can just omit this check if
> !efi_enabled(EFI_MEMMAP)
> 
> IIRC (and Peter would know), we are trying to filter out descriptors
> that are completely bogus here: zero lenght, zero address, etc etc. I
> don't think we need that for Xen.

Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
under Xen than on bare hardware.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> >  wrote:
> > >
> > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > Qubes OS.
> > >
> > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > >
> > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > ensures that the ESRT can safely be accessed by the OS.
> > >
> > > When running as a Xen dom0, use the new
> > > xen_config_table_memory_region_max() function to determine if Xen has
> > > reserved the ESRT and, if so, find the end of the memory region
> > > containing it.  This allows programs such as fwupd which require the
> > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > >
> > > Signed-off-by: Demi Marie Obenour 
> >
> > Why do we need this patch? I'd expect esrt_table_exists() to return
> > false when patch 1/2 is applied.
>
> efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> alternative way to get the end of the memory region containing the ESRT.
> That is what this patch provides.

OK. I don't think we need that to be honest. When running under Xen,
we should be able to assume that the ESRT does not span multiple
memory regions arbitrarily, so we can just omit this check if
!efi_enabled(EFI_MEMMAP)

IIRC (and Peter would know), we are trying to filter out descriptors
that are completely bogus here: zero lenght, zero address, etc etc. I
don't think we need that for Xen.

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, Sep 30, 2022 at 08:42:41PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 20:17, Demi Marie Obenour
>  wrote:
> >
> > On Fri, Sep 30, 2022 at 06:25:53PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > >  wrote:
> > > >
> > > > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, 
> > > > EFI_LOADER_DATA,
> > > > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > > > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > > > must not use EFI tables from such memory.  Most of the remaining EFI
> > > > memory types are not suitable for EFI tables, leaving only
> > > > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > > > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > > > use tables that are located in one of these types of memory.
> > > >
> > > > This patch ensures this, and also adds a function
> > > > (xen_config_table_memory_region_max()) that will be used later to
> > > > replace the usage of the EFI memory map in esrt.c when running under
> > > > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > > > but I have not implemented this.
> > > >
> > > > Signed-off-by: Demi Marie Obenour 
> > > > ---
> > > >  drivers/firmware/efi/efi.c |  8 +---
> > > >  drivers/xen/efi.c  | 35 +++
> > > >  include/linux/efi.h|  9 +
> > > >  3 files changed, 49 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > index 
> > > > e4080ad96089abd7f84745dd8461c548bcbb7685..d344f3ff73d1c5ed0c67e3251a9502e66719741d
> > > >  100644
> > > > --- a/drivers/firmware/efi/efi.c
> > > > +++ b/drivers/firmware/efi/efi.c
> > > > @@ -574,7 +574,6 @@ int __init efi_config_parse_tables(const 
> > > > efi_config_table_t *config_tables,
> > > > unsigned long table;
> > > > int i;
> > > >
> > > > -   pr_info("");
> > >
> > > Why are you removing these prints?
> >
> > If I left them, I would need to include a pr_cont("\n") later.
> 
> There should always be one at the end of the loop, no? Or is this
> related to the diagnostic that gets printed in your helper?

My helper emits a diagnostic (at severity KERN_WARNING) if the table is
in memory that Xen has not reserved.

> > Checkpatch recommends against that.  What is the purpose of this print?
> > I assumed that since it prints an empty string it is superfluous.
> >
> 
> It prints the leading [invisible] loglevel marker, and the 'efi: ' prefix.

Okay, that makes sense.

> > > > for (i = 0; i < count; i++) {
> > > > if (!IS_ENABLED(CONFIG_X86)) {
> > > > guid = _tables[i].guid;
> > > > @@ -585,7 +584,6 @@ int __init efi_config_parse_tables(const 
> > > > efi_config_table_t *config_tables,
> > > >
> > > > if (IS_ENABLED(CONFIG_X86_32) &&
> > > > tbl64[i].table > U32_MAX) {
> > > > -   pr_cont("\n");
> > > > pr_err("Table located above 4GB, 
> > > > disabling EFI.\n");
> > > > return -EINVAL;
> > > > }
> > > > @@ -594,10 +592,14 @@ int __init efi_config_parse_tables(const 
> > > > efi_config_table_t *config_tables,
> > > > table = tbl32[i].table;
> > > > }
> > > >
> > > > +#ifdef CONFIG_XEN_EFI
> > >
> > > We tend to prefer IS_ENABLED() for cases such as this one. That way,
> > > the compiler always gets to see the code inside the conditional block,
> > > which gives better build test coverage (even if CONFIG_XEN_EFI is
> > > disabled).
> >
> > Can I count on the compiler eliminating the code as unreachable?  With
> > CONFIG_XEN_EFI disabled xen_config_table_memory_region_max() would be an
> > undefined symbol.
> >
> 
> If you drop the #ifdef in the .h file (as I suggested below) the code
> will compile fine, and the symbol reference will not be emitted into
> the object, so it will link fine even if the Xen objects are not being
> built.
> 
> We rely on this behavior all over the Linux kernel.

Okay, thanks!

> > > > +   if (efi_enabled(EFI_PARAVIRT) && 
> > > > !xen_config_table_memory_region_max(table))
> > >
> > > So the question here is whether Xen thinks the table should be
> > > disregarded or not. So let's define a prototype that reflects that
> > > purpose, and let the implementation reason about how this should be
> > > achieved.
> >
> > xen_config_table_memory_region_max() doesn’t just return whether the
> > table should be disregarded, but also (if the table should not be
> > ignored) the end of the memory region containing it.
> 
> But the calling code never uses that value, right?

The code in this patch does not use that value.  Patch 2 of 2 does use
it.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, Sep 30, 2022 at 08:27:09PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 19:12, Demi Marie Obenour
>  wrote:
> >
> > On Fri, Sep 30, 2022 at 06:30:57PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 08:44, Jan Beulich  wrote:
> > > >
> > > > On 30.09.2022 01:02, Demi Marie Obenour wrote:
> > > > > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, 
> > > > > EFI_LOADER_DATA,
> > > > > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > > > > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > > > > must not use EFI tables from such memory.  Most of the remaining EFI
> > > > > memory types are not suitable for EFI tables, leaving only
> > > > > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > > > > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > > > > use tables that are located in one of these types of memory.
> > > > >
> > > > > This patch ensures this, and also adds a function
> > > > > (xen_config_table_memory_region_max()) that will be used later to
> > > > > replace the usage of the EFI memory map in esrt.c when running under
> > > > > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > > > > but I have not implemented this.
> > > > >
> > > > > Signed-off-by: Demi Marie Obenour 
> > > >
> > > > In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was 
> > > > passed
> > > > "-mapbs". Should we perhaps extend the interface such that Dom0 can then
> > > > also use tables located in such regions, perhaps by faking
> > > > EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?
> > > >
> > >
> > > I know this ship has sailed for x86, but for the sake of other
> > > architectures, I'd strongly recommend leaving the EFI_MEMORY_RUNTIME
> > > bits alone, for the same reasons I gave earlier. (Runtime mappings for
> > > the firmware code itself, page table fragmentation etc etc)
> >
> > Why do you say that it has sailed for x86?
> >
> 
> The x86 EFI code in Linux makes changes to the EFI memory map in many
> different places in the code. On other architectures, we have managed
> to avoid this, so that the EFI memory map is always identical to the
> one provided by the firmware at boot.
> 
> > > I know very little about Xen, but based on the context you provided in
> > > this thread, I'd say that the best approach from the Xen side is to
> > > convert all EfiBootServicesData regions that have configuration tables
> > > pointing into them into EfiAcpiReclaimMemory.
> >
> > Should Xen convert the entire region, or should it try to reserve only
> > the memory it needs?  The latter would require it to parse the
> > configuration tables.  Is there a list of configuration tables that can
> > legitimately be in EfiBootServicesData regions?
> >
> 
> Not really, no.

Is there a list of tables that can be in EfiBootServicesData and which
Linux cares about?

> So you would have to convert the entire region
> /unless/ Xen knows the GUID, and therefore knows how to derive the
> size of the table, allowing it to reserve memory more conservatively.

My worry is that this will wind up being equivalent to mapping all (or
most) of EfiBootServicesData memory.

> However, I doubt whether this is worth it: splitting entries implies
> rewriting the memory map, which is a thing I'd rather avoid if I were
> in your shoes.

Xen actually uses a different approach: instead of rewriting the memory
map, it uses the EFI pool allocator to allocate memory of the desired
type, copies the table to the newly allocated memory, and installs the
new table in place of the old one.  That only works for tables Xen
understands, though.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, 30 Sept 2022 at 20:17, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 06:25:53PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> >  wrote:
> > >
> > > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> > > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > > must not use EFI tables from such memory.  Most of the remaining EFI
> > > memory types are not suitable for EFI tables, leaving only
> > > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > > use tables that are located in one of these types of memory.
> > >
> > > This patch ensures this, and also adds a function
> > > (xen_config_table_memory_region_max()) that will be used later to
> > > replace the usage of the EFI memory map in esrt.c when running under
> > > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > > but I have not implemented this.
> > >
> > > Signed-off-by: Demi Marie Obenour 
> > > ---
> > >  drivers/firmware/efi/efi.c |  8 +---
> > >  drivers/xen/efi.c  | 35 +++
> > >  include/linux/efi.h|  9 +
> > >  3 files changed, 49 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 
> > > e4080ad96089abd7f84745dd8461c548bcbb7685..d344f3ff73d1c5ed0c67e3251a9502e66719741d
> > >  100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -574,7 +574,6 @@ int __init efi_config_parse_tables(const 
> > > efi_config_table_t *config_tables,
> > > unsigned long table;
> > > int i;
> > >
> > > -   pr_info("");
> >
> > Why are you removing these prints?
>
> If I left them, I would need to include a pr_cont("\n") later.

There should always be one at the end of the loop, no? Or is this
related to the diagnostic that gets printed in your helper?

> Checkpatch recommends against that.  What is the purpose of this print?
> I assumed that since it prints an empty string it is superfluous.
>

It prints the leading [invisible] loglevel marker, and the 'efi: ' prefix.

> > > for (i = 0; i < count; i++) {
> > > if (!IS_ENABLED(CONFIG_X86)) {
> > > guid = _tables[i].guid;
> > > @@ -585,7 +584,6 @@ int __init efi_config_parse_tables(const 
> > > efi_config_table_t *config_tables,
> > >
> > > if (IS_ENABLED(CONFIG_X86_32) &&
> > > tbl64[i].table > U32_MAX) {
> > > -   pr_cont("\n");
> > > pr_err("Table located above 4GB, 
> > > disabling EFI.\n");
> > > return -EINVAL;
> > > }
> > > @@ -594,10 +592,14 @@ int __init efi_config_parse_tables(const 
> > > efi_config_table_t *config_tables,
> > > table = tbl32[i].table;
> > > }
> > >
> > > +#ifdef CONFIG_XEN_EFI
> >
> > We tend to prefer IS_ENABLED() for cases such as this one. That way,
> > the compiler always gets to see the code inside the conditional block,
> > which gives better build test coverage (even if CONFIG_XEN_EFI is
> > disabled).
>
> Can I count on the compiler eliminating the code as unreachable?  With
> CONFIG_XEN_EFI disabled xen_config_table_memory_region_max() would be an
> undefined symbol.
>

If you drop the #ifdef in the .h file (as I suggested below) the code
will compile fine, and the symbol reference will not be emitted into
the object, so it will link fine even if the Xen objects are not being
built.

We rely on this behavior all over the Linux kernel.

> > > +   if (efi_enabled(EFI_PARAVIRT) && 
> > > !xen_config_table_memory_region_max(table))
> >
> > So the question here is whether Xen thinks the table should be
> > disregarded or not. So let's define a prototype that reflects that
> > purpose, and let the implementation reason about how this should be
> > achieved.
>
> xen_config_table_memory_region_max() doesn’t just return whether the
> table should be disregarded, but also (if the table should not be
> ignored) the end of the memory region containing it.

But the calling code never uses that value, right?

> I will make
> xen_efi_config_table_valid() a wrapper around
> xen_config_table_memory_region_max(), as the former also needs to print
> a warning if the table is in an invalid location.
>
> > So
> >
> > if (IS_ENABLED(CONFIG_XEN_EFI) &&
> > efi_enabled(EFI_PARAVIRT) &&
> > xen_efi_config_table_valid(guid, table)
> > continue
> >
> > I should note here, though, that EFI_PARAViRT is only set on x86 not
> > on other architectures that enable CONFIG_XEN_EFI so this will not
> > work anywhere else.
>
> What should I use instead?
>
> > > +

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, 30 Sept 2022 at 19:12, Demi Marie Obenour
 wrote:
>
> On Fri, Sep 30, 2022 at 06:30:57PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 08:44, Jan Beulich  wrote:
> > >
> > > On 30.09.2022 01:02, Demi Marie Obenour wrote:
> > > > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, 
> > > > EFI_LOADER_DATA,
> > > > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > > > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > > > must not use EFI tables from such memory.  Most of the remaining EFI
> > > > memory types are not suitable for EFI tables, leaving only
> > > > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > > > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > > > use tables that are located in one of these types of memory.
> > > >
> > > > This patch ensures this, and also adds a function
> > > > (xen_config_table_memory_region_max()) that will be used later to
> > > > replace the usage of the EFI memory map in esrt.c when running under
> > > > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > > > but I have not implemented this.
> > > >
> > > > Signed-off-by: Demi Marie Obenour 
> > >
> > > In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was passed
> > > "-mapbs". Should we perhaps extend the interface such that Dom0 can then
> > > also use tables located in such regions, perhaps by faking
> > > EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?
> > >
> >
> > I know this ship has sailed for x86, but for the sake of other
> > architectures, I'd strongly recommend leaving the EFI_MEMORY_RUNTIME
> > bits alone, for the same reasons I gave earlier. (Runtime mappings for
> > the firmware code itself, page table fragmentation etc etc)
>
> Why do you say that it has sailed for x86?
>

The x86 EFI code in Linux makes changes to the EFI memory map in many
different places in the code. On other architectures, we have managed
to avoid this, so that the EFI memory map is always identical to the
one provided by the firmware at boot.

> > I know very little about Xen, but based on the context you provided in
> > this thread, I'd say that the best approach from the Xen side is to
> > convert all EfiBootServicesData regions that have configuration tables
> > pointing into them into EfiAcpiReclaimMemory.
>
> Should Xen convert the entire region, or should it try to reserve only
> the memory it needs?  The latter would require it to parse the
> configuration tables.  Is there a list of configuration tables that can
> legitimately be in EfiBootServicesData regions?
>

Not really, no. So you would have to convert the entire region
/unless/ Xen knows the GUID, and therefore knows how to derive the
size of the table, allowing it to reserve memory more conservatively.
However, I doubt whether this is worth it: splitting entries implies
rewriting the memory map, which is a thing I'd rather avoid if I were
in your shoes.

> > I take it XEN_FW_EFI_MEM_INFO is an existing interface? If so, you
> > might do the same for the returned type - EfiBootServicesData ->
> > EfiAcpiReclaimMemory, and not muck about with the EFI_MEMORY_RUNTIME
> > attribute.
>
> It is indeed an existing interface, and this is a much better idea than
> what you proposed.

Right.

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
>  wrote:
> >
> > fwupd requires access to the EFI System Resource Table (ESRT) to
> > discover which firmware can be updated by the OS.  Currently, Linux does
> > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > Qubes OS.
> >
> > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> >
> > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > memory, Xen replaces the ESRT with a copy in memory that it has
> > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > ensures that the ESRT can safely be accessed by the OS.
> >
> > When running as a Xen dom0, use the new
> > xen_config_table_memory_region_max() function to determine if Xen has
> > reserved the ESRT and, if so, find the end of the memory region
> > containing it.  This allows programs such as fwupd which require the
> > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> >
> > Signed-off-by: Demi Marie Obenour 
> 
> Why do we need this patch? I'd expect esrt_table_exists() to return
> false when patch 1/2 is applied.

efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
alternative way to get the end of the memory region containing the ESRT.
That is what this patch provides.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, Sep 30, 2022 at 06:25:53PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
>  wrote:
> >
> > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > must not use EFI tables from such memory.  Most of the remaining EFI
> > memory types are not suitable for EFI tables, leaving only
> > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > use tables that are located in one of these types of memory.
> >
> > This patch ensures this, and also adds a function
> > (xen_config_table_memory_region_max()) that will be used later to
> > replace the usage of the EFI memory map in esrt.c when running under
> > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > but I have not implemented this.
> >
> > Signed-off-by: Demi Marie Obenour 
> > ---
> >  drivers/firmware/efi/efi.c |  8 +---
> >  drivers/xen/efi.c  | 35 +++
> >  include/linux/efi.h|  9 +
> >  3 files changed, 49 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 
> > e4080ad96089abd7f84745dd8461c548bcbb7685..d344f3ff73d1c5ed0c67e3251a9502e66719741d
> >  100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -574,7 +574,6 @@ int __init efi_config_parse_tables(const 
> > efi_config_table_t *config_tables,
> > unsigned long table;
> > int i;
> >
> > -   pr_info("");
> 
> Why are you removing these prints?

If I left them, I would need to include a pr_cont("\n") later.
Checkpatch recommends against that.  What is the purpose of this print?
I assumed that since it prints an empty string it is superfluous.

> > for (i = 0; i < count; i++) {
> > if (!IS_ENABLED(CONFIG_X86)) {
> > guid = _tables[i].guid;
> > @@ -585,7 +584,6 @@ int __init efi_config_parse_tables(const 
> > efi_config_table_t *config_tables,
> >
> > if (IS_ENABLED(CONFIG_X86_32) &&
> > tbl64[i].table > U32_MAX) {
> > -   pr_cont("\n");
> > pr_err("Table located above 4GB, disabling 
> > EFI.\n");
> > return -EINVAL;
> > }
> > @@ -594,10 +592,14 @@ int __init efi_config_parse_tables(const 
> > efi_config_table_t *config_tables,
> > table = tbl32[i].table;
> > }
> >
> > +#ifdef CONFIG_XEN_EFI
> 
> We tend to prefer IS_ENABLED() for cases such as this one. That way,
> the compiler always gets to see the code inside the conditional block,
> which gives better build test coverage (even if CONFIG_XEN_EFI is
> disabled).

Can I count on the compiler eliminating the code as unreachable?  With
CONFIG_XEN_EFI disabled xen_config_table_memory_region_max() would be an
undefined symbol.

> > +   if (efi_enabled(EFI_PARAVIRT) && 
> > !xen_config_table_memory_region_max(table))
> 
> So the question here is whether Xen thinks the table should be
> disregarded or not. So let's define a prototype that reflects that
> purpose, and let the implementation reason about how this should be
> achieved.

xen_config_table_memory_region_max() doesn’t just return whether the
table should be disregarded, but also (if the table should not be
ignored) the end of the memory region containing it.  I will make
xen_efi_config_table_valid() a wrapper around
xen_config_table_memory_region_max(), as the former also needs to print
a warning if the table is in an invalid location.

> So
> 
> if (IS_ENABLED(CONFIG_XEN_EFI) &&
> efi_enabled(EFI_PARAVIRT) &&
> xen_efi_config_table_valid(guid, table)
> continue
> 
> I should note here, though, that EFI_PARAViRT is only set on x86 not
> on other architectures that enable CONFIG_XEN_EFI so this will not
> work anywhere else.

What should I use instead?

> > +   continue;
> > +#endif
> > +
> > if (!match_config_table(guid, table, common_tables) && 
> > arch_tables)
> > match_config_table(guid, table, arch_tables);
> > }
> > -   pr_cont("\n");
> > set_bit(EFI_CONFIG_TABLES, );
> >
> > if (efi_rng_seed != EFI_INVALID_TABLE_ADDR) {
> > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> > index 
> > d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..c2274ddfcc63304008ef0fd78fd9fa416f75d073
> >  100644
> > --- a/drivers/xen/efi.c
> > +++ b/drivers/xen/efi.c
> > @@ -28,6 +28,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >
> > @@ -271,6 +272,40 @@ static void xen_efi_reset_system(int reset_type, 
> > efi_status_t status,
>

[xen-unstable-smoke test] 173387: tolerable all pass - PUSHED

flight 173387 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173387/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  1666086b00442b23e4fd70f4971e3bcf1a16b124
baseline version:
 xen  38e1276db4c5457cd6e7811b4e168aa85c8a0b06

Last test of basis   173384  2022-09-30 08:00:25 Z0 days
Testing same since   173387  2022-09-30 14:01:51 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Roger Pau Monné 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   38e1276db4..1666086b00  1666086b00442b23e4fd70f4971e3bcf1a16b124 -> smoke

Re: [PATCH for-4.17?] x86: support data operand independent timing mode

On 30/09/2022 16:41, Marek Marczykowski-Górecki wrote:

On Fri, Sep 30, 2022 at 11:25:12AM +, Andrew Cooper wrote:

On 15/09/2022 11:04, Jan Beulich wrote:

[1] specifies a long list of instructions which are intended to exhibit
timing behavior independent of the data they operate on. On certain
hardware this independence is optional, controlled by a bit in a new
MSR. Provide a command line option to control the mode Xen and its
guests are to operate in, with a build time control over the default.
Longer term we may want to allow guests to control this.

Since Arm64 supposedly also has such a control, put command line option
and Kconfig control in common files.

[1]
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html

Requested-by: Demi Marie Obenour

Signed-off-by: Jan Beulich

This patch should not be taken; at least not in this form. The whole
DOITM infrastructure is currently under argument, for being impossible
to use appropriately.

I understand why Qubes want this blanket set, but it is a steep penalty
to pay; It's only code which is already written trying to be constant
time/cache which gains any security from this.

Based on the bit description, I'd say rather "prevent _breaking_
security of the code already written". It is not setting this bit that
change behaviour on new parts, but it's not setting it that breaks
previous guarantees. It's really bizarre design choice from Intel...

On current parts, using
SSBD has the same behaviour, but this isn't expected to remain true in
the future.

Forcing it on behind the back of a VM is mutually exclusive with
enumerating it for VMs to use at some point in the future when we have
the capability to. i.e. specifically, you are not able to maintain the
ABI/API in this patch in the future.

Regarding the current behavior of the hypervisor (without this patch):
will guest see DOITM present but not set? Or will they not see it at
all?

Documentation clearly state:
For Intel® Core™ family processors based on microarchitectures
before Ice Lake and Intel Atom® family processors based on
microarchitectures before Gracemont that do not enumerate
IA32_UARCH_MISC_CTL, developers may assume that the instructions listed
here operate as if DOITM is enabled.

So, if guests will not see the feature at all, it Xen should set it
unconditionally, to remain compliant with expected hardware behaviour.

If guests will see the thing (and see it not enabled), then indeed it's
less clear what should be done right now (but I'd still like to have an
option to enable it).

Hmm. So yes, lets approach the problem from the other side, as "this bit needs
setting to unbreak crypto code".

On hardware supporting DOITM, where we do not advertise the feature to guests
(all guests right now), the guest kernel would conclude that it is safe, when
in fact it is not.

So Xen should set the bit behind the back of a guest which doesn't have the
DOITM enumeration presented (which is all guests right now).

But I don't think we want any Kconfig about this, or a dedicated cmdline
option. So how about this for a plan which avoids painting ourselves into a
corner.

1) Extend cpuid= with a no-doitm option. I know it's not actually a CPUID
enumeration, but MSR_ARCH_CAPS should have been CPUID data, and this is the
mechanism we have meaning "pretend this feature isn't enumerated".

2) On boot, and S3 resume, if DOITM and availble, set invariant mode.

That should do as a stopgap for now that keeps software safe.

Then, when we've got MSR_ARCH_CAPS working for guests, the internals of
MSR_UARCH_MISC_CTL change to being a context switched thing which, like
MSR_SPEC_CTRL, we have options for bits set behind the guest's back. Then we
set DOITM behind the guests back if levelling causes the feature to be hidden.
We do this for some bits already, and need to do so for more controls too.

~Andrew

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, Sep 30, 2022 at 06:30:57PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 08:44, Jan Beulich  wrote:
> >
> > On 30.09.2022 01:02, Demi Marie Obenour wrote:
> > > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> > > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > > must not use EFI tables from such memory.  Most of the remaining EFI
> > > memory types are not suitable for EFI tables, leaving only
> > > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > > use tables that are located in one of these types of memory.
> > >
> > > This patch ensures this, and also adds a function
> > > (xen_config_table_memory_region_max()) that will be used later to
> > > replace the usage of the EFI memory map in esrt.c when running under
> > > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > > but I have not implemented this.
> > >
> > > Signed-off-by: Demi Marie Obenour 
> >
> > In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was passed
> > "-mapbs". Should we perhaps extend the interface such that Dom0 can then
> > also use tables located in such regions, perhaps by faking
> > EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?
> >
> 
> I know this ship has sailed for x86, but for the sake of other
> architectures, I'd strongly recommend leaving the EFI_MEMORY_RUNTIME
> bits alone, for the same reasons I gave earlier. (Runtime mappings for
> the firmware code itself, page table fragmentation etc etc)

Why do you say that it has sailed for x86?

> I know very little about Xen, but based on the context you provided in
> this thread, I'd say that the best approach from the Xen side is to
> convert all EfiBootServicesData regions that have configuration tables
> pointing into them into EfiAcpiReclaimMemory.

Should Xen convert the entire region, or should it try to reserve only
the memory it needs?  The latter would require it to parse the
configuration tables.  Is there a list of configuration tables that can
legitimately be in EfiBootServicesData regions?

> I take it XEN_FW_EFI_MEM_INFO is an existing interface? If so, you
> might do the same for the returned type - EfiBootServicesData ->
> EfiAcpiReclaimMemory, and not muck about with the EFI_MEMORY_RUNTIME
> attribute.

It is indeed an existing interface, and this is a much better idea than
what you proposed.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, Sep 30, 2022 at 08:44:21AM +0200, Jan Beulich wrote:
> On 30.09.2022 01:02, Demi Marie Obenour wrote:
> > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > must not use EFI tables from such memory.  Most of the remaining EFI
> > memory types are not suitable for EFI tables, leaving only
> > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > use tables that are located in one of these types of memory.
> > 
> > This patch ensures this, and also adds a function
> > (xen_config_table_memory_region_max()) that will be used later to
> > replace the usage of the EFI memory map in esrt.c when running under
> > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > but I have not implemented this.
> > 
> > Signed-off-by: Demi Marie Obenour 
> 
> In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was passed
> "-mapbs". Should we perhaps extend the interface such that Dom0 can then
> also use tables located in such regions, perhaps by faking
> EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?

I can add a check for EFI_MEMORY_RUNTIME, but only if I can require a Xen
version with 
https://lore.kernel.org/xen-devel/cc0fbcb4-5ea3-178c-e691-9acb7cc9a...@suse.com/t/#u.
This is easy in Qubes OS via RPM dependencies, but I am not sure if it
is suitable for upstream without a mechanism for dom0 to verify that the
patch has been included.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab


signature.asc
Description: PGP signature

Re: [PATCH v4 2/2] Support ESRT in Xen dom0

On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
 wrote:
>
> fwupd requires access to the EFI System Resource Table (ESRT) to
> discover which firmware can be updated by the OS.  Currently, Linux does
> not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> Qubes OS.
>
> Before Xen 4.17, this was not fixable due to hypervisor limitations.
> The UEFI specification requires the ESRT to be in EfiBootServicesData
> memory, which Xen will use for whatever purposes it likes.  Therefore,
> Linux cannot safely access the ESRT, as Xen may have overwritten it.
>
> Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> memory, Xen replaces the ESRT with a copy in memory that it has
> reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> ensures that the ESRT can safely be accessed by the OS.
>
> When running as a Xen dom0, use the new
> xen_config_table_memory_region_max() function to determine if Xen has
> reserved the ESRT and, if so, find the end of the memory region
> containing it.  This allows programs such as fwupd which require the
> ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
>
> Signed-off-by: Demi Marie Obenour 

Why do we need this patch? I'd expect esrt_table_exists() to return
false when patch 1/2 is applied.



> ---
>  drivers/firmware/efi/esrt.c | 43 ++---
>  1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 
> 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..a0642bc161b4b1f94f818b8c9f46511fe2424bb2
>  100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -243,27 +243,44 @@ void __init efi_esrt_init(void)
> void *va;
> struct efi_system_resource_table tmpesrt;
> size_t size, max, entry_size, entries_size;
> -   efi_memory_desc_t md;
> -   int rc;
> phys_addr_t end;
> -
> -   if (!efi_enabled(EFI_MEMMAP))
> -   return;
> +   u32 type;
>
> pr_debug("esrt-init: loading.\n");
> if (!esrt_table_exists())
> return;
>
> -   rc = efi_mem_desc_lookup(efi.esrt, );
> -   if (rc < 0 ||
> -   (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> -md.type != EFI_BOOT_SERVICES_DATA &&
> -md.type != EFI_RUNTIME_SERVICES_DATA)) {
> -   pr_warn("ESRT header is not in the memory map.\n");
> +   if (efi_enabled(EFI_MEMMAP)) {
> +   efi_memory_desc_t md;
> +
> +   if (efi_mem_desc_lookup(efi.esrt, ) < 0 ||
> +   (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> +md.type != EFI_BOOT_SERVICES_DATA &&
> +md.type != EFI_RUNTIME_SERVICES_DATA)) {
> +   pr_warn("ESRT header is not in the memory map.\n");
> +   return;
> +   }
> +
> +   type = md.type;
> +   max = efi_mem_desc_end();
> +#ifdef CONFIG_XEN_EFI
> +   } else if (efi_enabled(EFI_PARAVIRT)) {
> +   max = xen_config_table_memory_region_max(efi.esrt);
> +   /*
> +* This might be wrong, but it doesn't matter.
> +* xen_config_table_memory_region_max() checks the type
> +* of the memory region, and if it returns 0, the code
> +* below will fail without looking at the type.  Choose
> +* a value that will not cause * subsequent code to try
> +* to reserve the memory containing the ESRT, as either
> +* Xen or the firmware has done so already.
> +*/
> +   type = EFI_RUNTIME_SERVICES_DATA;
> +#endif
> +   } else {
> return;
> }
>
> -   max = efi_mem_desc_end();
> if (max < efi.esrt) {
> pr_err("EFI memory descriptor is invalid. (esrt: %p max: 
> %p)\n",
>(void *)efi.esrt, (void *)max);
> @@ -333,7 +350,7 @@ void __init efi_esrt_init(void)
>
> end = esrt_data + size;
> pr_info("Reserving ESRT space from %pa to %pa.\n", _data, );
> -   if (md.type == EFI_BOOT_SERVICES_DATA)
> +   if (type == EFI_BOOT_SERVICES_DATA)
> efi_mem_reserve(esrt_data, esrt_data_size);
>
> pr_debug("esrt-init: loaded.\n");
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
>

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, 30 Sept 2022 at 08:44, Jan Beulich  wrote:
>
> On 30.09.2022 01:02, Demi Marie Obenour wrote:
> > Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> > EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> > Xen before Linux gets to start using it.  Therefore, Linux under Xen
> > must not use EFI tables from such memory.  Most of the remaining EFI
> > memory types are not suitable for EFI tables, leaving only
> > EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> > EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> > use tables that are located in one of these types of memory.
> >
> > This patch ensures this, and also adds a function
> > (xen_config_table_memory_region_max()) that will be used later to
> > replace the usage of the EFI memory map in esrt.c when running under
> > Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> > but I have not implemented this.
> >
> > Signed-off-by: Demi Marie Obenour 
>
> In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was passed
> "-mapbs". Should we perhaps extend the interface such that Dom0 can then
> also use tables located in such regions, perhaps by faking
> EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?
>

I know this ship has sailed for x86, but for the sake of other
architectures, I'd strongly recommend leaving the EFI_MEMORY_RUNTIME
bits alone, for the same reasons I gave earlier. (Runtime mappings for
the firmware code itself, page table fragmentation etc etc)

I know very little about Xen, but based on the context you provided in
this thread, I'd say that the best approach from the Xen side is to
convert all EfiBootServicesData regions that have configuration tables
pointing into them into EfiAcpiReclaimMemory.

I take it XEN_FW_EFI_MEM_INFO is an existing interface? If so, you
might do the same for the returned type - EfiBootServicesData ->
EfiAcpiReclaimMemory, and not muck about with the EFI_MEMORY_RUNTIME
attribute.

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
 wrote:
>
> Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> Xen before Linux gets to start using it.  Therefore, Linux under Xen
> must not use EFI tables from such memory.  Most of the remaining EFI
> memory types are not suitable for EFI tables, leaving only
> EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> use tables that are located in one of these types of memory.
>
> This patch ensures this, and also adds a function
> (xen_config_table_memory_region_max()) that will be used later to
> replace the usage of the EFI memory map in esrt.c when running under
> Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> but I have not implemented this.
>
> Signed-off-by: Demi Marie Obenour 
> ---
>  drivers/firmware/efi/efi.c |  8 +---
>  drivers/xen/efi.c  | 35 +++
>  include/linux/efi.h|  9 +
>  3 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 
> e4080ad96089abd7f84745dd8461c548bcbb7685..d344f3ff73d1c5ed0c67e3251a9502e66719741d
>  100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -574,7 +574,6 @@ int __init efi_config_parse_tables(const 
> efi_config_table_t *config_tables,
> unsigned long table;
> int i;
>
> -   pr_info("");

Why are you removing these prints?

> for (i = 0; i < count; i++) {
> if (!IS_ENABLED(CONFIG_X86)) {
> guid = _tables[i].guid;
> @@ -585,7 +584,6 @@ int __init efi_config_parse_tables(const 
> efi_config_table_t *config_tables,
>
> if (IS_ENABLED(CONFIG_X86_32) &&
> tbl64[i].table > U32_MAX) {
> -   pr_cont("\n");
> pr_err("Table located above 4GB, disabling 
> EFI.\n");
> return -EINVAL;
> }
> @@ -594,10 +592,14 @@ int __init efi_config_parse_tables(const 
> efi_config_table_t *config_tables,
> table = tbl32[i].table;
> }
>
> +#ifdef CONFIG_XEN_EFI

We tend to prefer IS_ENABLED() for cases such as this one. That way,
the compiler always gets to see the code inside the conditional block,
which gives better build test coverage (even if CONFIG_XEN_EFI is
disabled).

> +   if (efi_enabled(EFI_PARAVIRT) && 
> !xen_config_table_memory_region_max(table))

So the question here is whether Xen thinks the table should be
disregarded or not. So let's define a prototype that reflects that
purpose, and let the implementation reason about how this should be
achieved.

So

if (IS_ENABLED(CONFIG_XEN_EFI) &&
efi_enabled(EFI_PARAVIRT) &&
xen_efi_config_table_valid(guid, table)
continue

I should note here, though, that EFI_PARAViRT is only set on x86 not
on other architectures that enable CONFIG_XEN_EFI so this will not
work anywhere else.


> +   continue;
> +#endif
> +
> if (!match_config_table(guid, table, common_tables) && 
> arch_tables)
> match_config_table(guid, table, arch_tables);
> }
> -   pr_cont("\n");
> set_bit(EFI_CONFIG_TABLES, );
>
> if (efi_rng_seed != EFI_INVALID_TABLE_ADDR) {
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> index 
> d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..c2274ddfcc63304008ef0fd78fd9fa416f75d073
>  100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>
> @@ -271,6 +272,40 @@ static void xen_efi_reset_system(int reset_type, 
> efi_status_t status,
> }
>  }
>
> +__init u64 xen_config_table_memory_region_max(u64 addr)

It is more idiomatic for Linux to put __init after the return type.
And if we adopt my suggestion above, this becomes

bool __init xen_efi_config_table_valid(const efi_guid_t *guid, u64 table)

Alternatively, you could pass the string identifier of the table
instead of the guid (or both) to print in the diagnostic message.


> +{
> +   static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> + "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");

Is this the only place where this matters? And this never happens on x86, right?

> +   struct xen_platform_op op = {
> +   .cmd = XENPF_firmware_info,
> +   .u.firmware_info = {
> +   .type = XEN_FW_EFI_INFO,
> +   .index = XEN_FW_EFI_MEM_INFO,
> +   .u.efi_info.mem.addr = addr,
> +   .u.efi_info.mem.size = U64_MAX - addr,
> +   }
> +   };
> +   union

[PATCH v4 1/1] uboot-script-gen: Dynamically compute addr and size when loading binaries

2022-09-30 Thread Andrei Cherechesu (OSS)

From: Andrei Cherechesu 

Normally, the Imagebuilder would precompute the sizes of the loaded
binaries and addresses where they are loaded before generating the
script, and the sizes and addresses that needed to be provided to
Xen via /chosen would be hardcoded in the boot script.

Added an option via "-s" parameter to avoid hardcoding any
address in the boot script, and dynamically compute the
loading addresses for binaries. The first loading address is based
on the MEMORY_START parameter and after loading each binary,
the loading address and the size of the binary are stored in
variables with corresponding names. Then, the loading address
for the next binary is computed and aligned to 0x20.

If the "-s" parameter is not used, the normal flow is executed,
where the loading addresses and sizes for each binaries are
precomputed and hardcoded inside the script, but the loading
addresses and sizes for each binary are now also stored for eventual
later use.

Signed-off-by: Andrei Cherechesu 
Signed-off-by: Stefano Stabellini 
---
 scripts/uboot-script-gen | 114 +++
 1 file changed, 80 insertions(+), 34 deletions(-)

diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
index b24dca2..16269f0 100755
--- a/scripts/uboot-script-gen
+++ b/scripts/uboot-script-gen
@@ -4,6 +4,9 @@ offset=$((2*1024*1024))
 filesize=0
 prog_req=(mkimage file fdtput mktemp awk)
 
+padding_mask=`printf "0x%X\n" $(($offset - 1))`
+padding_mask_inv=`printf "0x%X\n" $((~$padding_mask))`
+
 function cleanup_and_return_err()
 {
 rm -f $UBOOT_SOURCE $UBOOT_SCRIPT
@@ -28,6 +31,7 @@ function dt_mknode()
 #   str
 #   str_a
 #   bool
+#   var
 function dt_set()
 {
 local path=$1
@@ -35,11 +39,26 @@ function dt_set()
 local data_type=$3
 local data=$4
 
+if test $data_type = "var"
+then
+eval data_addr_var="$data"_addr
+eval data_addr=\$"$data_addr_var"
+eval data_size_var="$data"_size
+eval data_size=\$"$data_size_var"
+fi
 
 if test "$UBOOT_SOURCE" && test ! "$FIT"
 then
 var=${var/\#/\\#}
-if test $data_type = "hex" || test $data_type = "int"
+if test $data_type = "var"
+then
+if test $dynamic_loading_opt
+then
+echo "fdt set $path $var <0x0 0x\${"$data_addr_var"} 0x0 
0x\${"$data_size_var"}>" >> $UBOOT_SOURCE
+else
+echo "fdt set $path $var <0x0 $data_addr 0x0 $data_size>" >> 
$UBOOT_SOURCE
+fi
+elif test $data_type = "hex" || test $data_type = "int"
 then
 echo "fdt set $path $var <$data>" >> $UBOOT_SOURCE
 elif test $data_type = "str_a"
@@ -63,7 +82,10 @@ function dt_set()
 
 if test $FDTEDIT
 then
-if test $data_type = "hex"
+if test $data_type = "var"
+then
+fdtput $FDTEDIT -p -t x $path $var 0x0 "$data_addr" 0x0 
"$data_size"
+elif test $data_type = "hex"
 then
 fdtput $FDTEDIT -p -t x $path $var $data
 elif test $data_type = "int"
@@ -87,38 +109,35 @@ function dt_set()
 function add_device_tree_kernel()
 {
 local path=$1
-local addr=$2
-local size=$3
-local bootargs=$4
+local varname=$2
+local bootargs=$3
 
-dt_mknode "$path" "module$addr"
-dt_set "$path/module$addr" "compatible" "str_a" "multiboot,kernel 
multiboot,module"
-dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" 
$size)"
-dt_set "$path/module$addr" "bootargs" "str" "$bootargs"
+dt_mknode "$path" "module-$varname"
+dt_set "$path/module-$varname" "compatible" "str_a" "multiboot,kernel 
multiboot,module"
+dt_set "$path/module-$varname" "reg" "var"  "$varname"
+dt_set "$path/module-$varname" "bootargs" "str" "$bootargs"
 }
 
 
 function add_device_tree_ramdisk()
 {
 local path=$1
-local addr=$2
-local size=$3
+local varname=$2
 
-dt_mknode "$path"  "module$addr"
-dt_set "$path/module$addr" "compatible" "str_a" "multiboot,ramdisk 
multiboot,module"
-dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" 
$size)"
+dt_mknode "$path" "module-$varname"
+dt_set "$path/module-$varname" "compatible" "str_a" "multiboot,ramdisk 
multiboot,module"
+dt_set "$path/module-$varname" "reg" "var"  "$varname"
 }
 
 
 function add_device_tree_passthrough()
 {
 local path=$1
-local addr=$2
-local size=$3
+local varname=$2
 
-dt_mknode "$path"  "module$addr"
-dt_set "$path/module$addr" "compatible" "str_a" "multiboot,device-tree 
multiboot,module"
-dt_set "$path/module$addr" "reg" "hex"  "0x0 $addr 0x0 $(printf "0x%x" 
$size)"
+dt_mknode "$path" "module-$varname"
+dt_set "$path/module-$varname" "compatible" "str_a" "multiboot,device-tree 
multiboot,module"
+dt_set "$path/module-$varname" "reg" "var"  "$varname"
 }
 
 function add_device_tree_mem()
@@ -260,7 +279,7 @@ function

[ImageBuilder][PATCH v4 0/1] Imagebuilder dynamic addresses and sizes

2022-09-30 Thread Andrei Cherechesu (OSS)

From: Andrei Cherechesu 

This sent patch adds support for dynamically computing the addresses
and sizes for loaded binaries via the boot script generated by
Imagebuilder.

Compared to the v3 version of the patch, this includes Stefano's
suggestions of not adding as many "if" statements on the
$dynamic_loading_opt parameter added by the patch, along with
keeping compatibility with the FDTEDIT case.

The sent patch extends Stefano's suggestion, by also keeping
the normal flow (without "-s" parameter) mostly unaltered: the addresses
inside the generated script are literals, and the $memaddr variable
is not re-computed after each binary loaded since it's unused. The
only difference in the normal flow is that the binaries' sizes and
addresses are stored after loading each binary. Also, removed the "0x"
prefix of $memaddr set on the first line in the generated script.

These mentioned changes, compared to Stefano's suggestion, need
an additional 2 "if" branches on $dynamic_loading_opt, which I
find a worthy tradeoff for keeping the normal flow mostly unaltered.
Otherwise, I'm also happy to go with Stefano's suggestion, but
the script generated on the normal flow would contain many
unnecessary steps.

Andrei Cherechesu (1):
  uboot-script-gen: Dynamically compute addr and size when loading
binaries


 scripts/uboot-script-gen | 114 +++
 1 file changed, 80 insertions(+), 34 deletions(-)

-- 
2.35.1

Re: [PATCH for-4.17?] x86: support data operand independent timing mode

2022-09-30 Thread Marek Marczykowski-Górecki

On Fri, Sep 30, 2022 at 11:25:12AM +, Andrew Cooper wrote:
> On 15/09/2022 11:04, Jan Beulich wrote:
> > [1] specifies a long list of instructions which are intended to exhibit
> > timing behavior independent of the data they operate on. On certain
> > hardware this independence is optional, controlled by a bit in a new
> > MSR. Provide a command line option to control the mode Xen and its
> > guests are to operate in, with a build time control over the default.
> > Longer term we may want to allow guests to control this.
> >
> > Since Arm64 supposedly also has such a control, put command line option
> > and Kconfig control in common files.
> >
> > [1] 
> > https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html
> >
> > Requested-by: Demi Marie Obenour 
> > Signed-off-by: Jan Beulich 
> 
> This patch should not be taken; at least not in this form.  The whole
> DOITM infrastructure is currently under argument, for being impossible
> to use appropriately.
> 
> I understand why Qubes want this blanket set, but it is a steep penalty
> to pay;  It's only code which is already written trying to be constant
> time/cache which gains any security from this. 

Based on the bit description, I'd say rather "prevent _breaking_
security of the code already written". It is not setting this bit that
change behaviour on new parts, but it's not setting it that breaks
previous guarantees. It's really bizarre design choice from Intel...

>  On current parts, using
> SSBD has the same behaviour, but this isn't expected to remain true in
> the future.
> 
> Forcing it on behind the back of a VM is mutually exclusive with
> enumerating it for VMs to use at some point in the future when we have
> the capability to.  i.e. specifically, you are not able to maintain the
> ABI/API in this patch in the future.

Regarding the current behavior of the hypervisor (without this patch):
will guest see DOITM present but not set? Or will they not see it at
all?

Documentation clearly state:
For Intel® Core™ family processors based on microarchitectures
before Ice Lake and Intel Atom® family processors based on
microarchitectures before Gracemont that do not enumerate
IA32_UARCH_MISC_CTL, developers may assume that the instructions listed
here operate as if DOITM is enabled.

So, if guests will not see the feature at all, it Xen should set it
unconditionally, to remain compliant with expected hardware behaviour.

If guests will see the thing (and see it not enabled), then indeed it's
less clear what should be done right now (but I'd still like to have an
option to enable it).

> If we do move forward with something like this (under the strict
> understanding that the behaviour is going to change in the future), then
> "DIT" is too short of an acronym to use.  Amongst other things, it's not
> "data independent timing"; it's "controls for forcing ..." which is
> important because these are going to be vendor specific, if even needed
> in the first place.
> 
> ~Andrew

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

signature.asc
Description: PGP signature

Re: [PATCH v2 4/5] tools/ocaml/libs/xc: OCaml 5.0 compatibility

2022-09-30 Thread Edwin Torok

> On 30 Sep 2022, at 15:59, Christian Lindig  
> wrote:
> 
> 
> 
>> On 27 Sep 2022, at 17:13, Edwin Torok  wrote:
>> 
>> 
>> See below for a patch for that. I've included this patch in the correct 
>> place (before the patch that breaks it) in the git repository at: 
>> https://github.com/edwintorok/xen/compare/private/edvint/public0
>> 
> 
> 
> Acked-by: Christian Lindig 
> 
> I believe these changes are fine. We are now allocating the event channel 
> dynamically and this requires using a finaliser to free that memory. 

Thanks,

> 
> 
>> -ifneq ($(MAKECMDGOALS),clean)
>> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))
>> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile 
>> $(OCAML_TOPLEVEL)/Makefile.rules
>>  $(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli 
>> $o,MLDEP,)
>> endif
> 
> Is this the right logic? Moving from ifneq to ifeq here.
> 
> I am not so familiar with the Makfile rules. The gist seems to be: we depend 
> on auto-generated Make dependencies that the Makefile in general depends on. 
> But in a “make clean” (or other “*clean” it is wasteful to generate these 
> only to later remove them. However, these kind of subtleties are obvious 
> enough while we are working on this but over time accumulate to Makefiles 
> that are hard to change. So I wonder if this kind of optimisation, while 
> correct, is worth it, but fine going along with it.
> 

Makefile functions can be a bit confusing to read.

"ifneq ($(MAKECMDGOALS), clean)" means $(MAKECMDGOALS) != "clean"
"ifeq (,$(findstring clean,$(MAKECMDGOALS)))" means that "clean" in 
$(MAKECMDGOALS) == "" (the empty string), or i.o.w. "clean" not in 
$(MAKECMDGOALS), which is a bit more generic than the previous one,
since we have all sorts of rules in the Makefile (especially around subdirs) 
where 'clean' is a substring.
This is quite subtle and I had to reread this line many times too to check it 
is correct.

The real solution here would be to have a single non-recursive Makefile (and 
there is some discussion/patches heading in that direction in xen-devel 
particularly from Anthony), and then evaluating the "clean" rules would be a 
lot less expensive, it'd only have to be done once. But there might be a while 
until we get there, and meanwhile these clean rules slow down the OCaml build 
too much (just running the "clean" takes a lot longer than building the entire 
OCaml libraries and oxenstored sequentially).

Although I only need to use 'clean' when using the upstream Makefiles (where 
almost every incremental change requires a 'clean' inbetween because the 
Makefiles express the dependencies incorrectly),
or when switching from upstream Makefile to 'dune' (a one-off event usually).
Since I use 'Dune' for my daily work anyway (and the makefile is used in our 
internal build system) perhaps this Makefile patch is not needed at all, I can 
change 'Makefile.dune' to not call 'make clean' at all, and I'll know to 
remember to run it if things fail anyway (it'll be pretty obvious when Dune 
says you've got a build artifact in the wrong place).

Best regards,
--Edwin

[linux-linus test] 173383: regressions - trouble: broken/fail/pass

flight 173383 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173383/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsmbroken
 build-arm64-pvops 6 kernel-build   fail in 173378 REGR. vs. 173364

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 5 host-install(5) broken pass in 
173378
 test-armhf-armhf-xl-arndale   8 xen-boot   fail pass in 173378

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-credit1   1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-xl-vhd   1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-xl-seattle   1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-libvirt-raw  1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-examine  1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-xl-thunderx  1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked in 173378 n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked in 173378 n/a
 test-armhf-armhf-xl-arndale 15 migrate-support-check fail in 173378 never pass
 test-armhf-armhf-xl-arndale 16 saverestore-support-check fail in 173378 never 
pass
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 173364
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 173364
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 173364
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 173364
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 173364
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 173364
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 173364
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 173364
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt-qcow2 14

Re: [PATCH v2 4/5] tools/ocaml/libs/xc: OCaml 5.0 compatibility

2022-09-30 Thread Christian Lindig

> On 27 Sep 2022, at 17:13, Edwin Torok  wrote:
> 
> 
> See below for a patch for that. I've included this patch in the correct place 
> (before the patch that breaks it) in the git repository at: 
> https://github.com/edwintorok/xen/compare/private/edvint/public0
> 

Acked-by: Christian Lindig 

I believe these changes are fine. We are now allocating the event channel 
dynamically and this requires using a finaliser to free that memory. 

> -ifneq ($(MAKECMDGOALS),clean)
> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))
> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile 
> $(OCAML_TOPLEVEL)/Makefile.rules
>   $(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli 
> $o,MLDEP,)
> endif

Is this the right logic? Moving from ifneq to ifeq here.

I am not so familiar with the Makfile rules. The gist seems to be: we depend on 
auto-generated Make dependencies that the Makefile in general depends on. But 
in a “make clean” (or other “*clean” it is wasteful to generate these only to 
later remove them. However, these kind of subtleties are obvious enough while 
we are working on this but over time accumulate to Makefiles that are hard to 
change. So I wonder if this kind of optimisation, while correct, is worth it, 
but fine going along with it.

— C

Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

On Fri, Sep 30, 2022 at 09:50:40AM +0200, Jan Beulich wrote:
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there and hence that type's handling can be left alone.

What about dom0?  Should it be translated to E820_RESERVED so that
dom0 doesn't try to use it either?  I guess using E820_RESERVED
could also confuse dom0 about ACPI data placement.

Thanks, Roger.

[PATCH for-4.17] x86/efi: don't translate EFI memory map MMIO regions to e820 RESERVED

2022-09-30 Thread Roger Pau Monne

The EFI memory map contains two memory types (EfiMemoryMappedIO and
EfiMemoryMappedIOPortSpace) used to describe IO memory areas of
devices used by EFI.

The current parsing of the EFI memory map was translating
EfiMemoryMappedIO and EfiMemoryMappedIOPortSpace to E820_RESERVED on
x86.  This is an issue because device MMIO regions (BARs) should not
be positioned on reserved regions.  Any BARs positioned on non-hole
areas of the memory map will cause is_memory_hole() to return false,
which would then cause memory decoding to be disabled for such device.
This leads to EFI firmware malfunctions when using runtime services.

The system under which this was observed has:

EFI memory map:
[...]
 0fd00-0fe7f type=11 attr=8000100d
[...]
:00:1f.5 disabled: BAR [0xfe010, 0xfe010] overlaps with memory map

The device behind this BAR is:

00:1f.5 Serial bus controller [0c80]: Intel Corporation Lewisburg SPI 
Controller (rev 09)
Subsystem: Super Micro Computer Inc Device 091c
Flags: fast devsel
Memory at fe01 (32-bit, non-prefetchable) [size=4K]well

For the record, the symptom observed in that machine was a hard freeze
when attempting to set an EFI variable (XEN_EFI_set_variable).

Fix by not adding regions with type EfiMemoryMappedIO or
EfiMemoryMappedIOPortSpace to the e820 memory map, that allows BARs to
be positioned there.

Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably positioned')
Signed-off-by: Roger Pau Monné 
---
I don't understand the definition of EfiMemoryMappedIOPortSpace:

"System memory-mapped IO region that is used to translate memory
cycles to IO cycles by the processor."

But given its name I would assume it's also likely used to mark ranges
in use by PCI device BARs.

It would also be interesting to forward this information to dom0, so
it doesn't attempt to move the BARs of this device(s) around, or else
issues will arise.

And of course the device must not be passed through to domUs, as
disabling memory decoding on it can lead to a host DoS.  Not that it
makes much sense to pass devices like the one above to guests.

It also makes me wonder whether this playing with the decoding bit
that we do in Xen is safe.  It might be more resilient to only disable
memory decoding when the BARs overlap a RAM region, as that would
indeed cause issues.

We should also consider moving away from the e820 and instead using
the EFI memory map for things like is_memory_hole(), but that would
involve translating e820 memory maps into EFI format, which might be
easier and more reliable than the other way around which we currently
do.
---
 xen/arch/x86/efi/efi-boot.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 836e8c2ba1..12ad94cd71 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -169,6 +169,14 @@ static void __init 
efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
 
 switch ( desc->Type )
 {
+case EfiMemoryMappedIO:
+case EfiMemoryMappedIOPortSpace:
+/*
+ * There no suitable e820 memory type to represent a MMIO area
+ * except a hole.
+ */
+continue;
+
 case EfiBootServicesCode:
 case EfiBootServicesData:
 if ( map_bs )
-- 
2.37.3

Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

Hi Andrew,

> On 30 Sep 2022, at 14:53, Andrew Cooper  wrote:
> 
> On 30/09/2022 08:50, Jan Beulich wrote:
>> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
>> higher priority than the type of the range. To avoid accessing memory at
>> runtime which was re-used for other purposes, make
>> efi_arch_process_memory_map() follow suit. While on x86 in theory the
>> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
>> E820_ACPI memory there and hence that type's handling can be left alone.
>> 
>> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
>> Fixes: facac0af87ef ("x86-64: EFI runtime code")
>> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
>> Signed-off-by: Jan Beulich 
> 
> Isn't this also liable to be the root cause of the crash reported on
> IRC, where a read-only page was being inserted into the heap?

No this should not be related at all.

Cheers
Bertrand

> 
> ~Andrew

Re: [PATCH v3 08/10] x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init

2022-09-30 Thread Borislav Petkov

On Fri, Sep 30, 2022 at 03:11:07PM +0200, Juergen Gross wrote:
> Yes, this can be done. It would practically have to be the first one just
> after CPUHP_BRINGUP_CPU.

Right.

> The question is whether we really want to call the MTRR/PAT initialization
> on hotplugged cpus only after enabling interrupts. Note that the callbacks
> are activated only at the end of start_secondary(), while today MTRR/PAT
> initialization is called some time earlier by:
> 
>   start_secondary()
> smp_callin()
>   smp_store_cpu_info()
> identify_secondary_cpu()
>   mtrr_ap_init()
> 
> I don't think this is a real problem, but I wanted to mention it.

Yep, I saw that too but I don't think there will be a problem either.
I mean, it should be early enough as you point out not to need proper
MTRR/PAT settings yet.

But we'll make sure we test this real good too.

> The next question would be, why MTRR/PAT init should be special
> (meaning: why are all the other functions called that early not
> realized via callbacks)?

Well, our init code is crazy. Frankly, I don't see why not more of the
"init stuff on the freshly hotplugged CPU" work is done there...

> Is it just because of the special handling during boot/resume?

... unless this is the case, ofc. Right.

> It might be worth a discussion whether there shouldn't be a special group
> of callbacks activated BEFORE interrupts are being enabled.

That's a good question. /me writes it down to ask tglx when he gets back.

I mean, that early I don't think it matters whether IRQs are enabled
or not. But this'll need to be audited on a case by case basis. As I
said, our boot code is nuts with stuff bolted on everywhere for whatever
reasons.

> Thanks. I'll write a patch for that.

Thanks too.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 08/10] x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init

2022-09-30 Thread Juergen Gross


On 30.09.22 13:55, Borislav Petkov wrote:

On Thu, Sep 29, 2022 at 10:26:59AM +0200, Juergen Gross wrote:

So right now I'm inclined to be better on the safe side by not adding any
cpu hotplug hook, but to use just the same "delayed AP init" flag as today,
just renaming it. This would leave the delayed MTRR/PAT init in place for
resume and kexec cases, but deferring the MTRR/PAT cleanup due to this
potential issue seems not appropriate, as the cleanup isn't changing the
behavior here.


Ok, what's wrong with adding a special hotplug level just for that thing
and running it very early? Practically pretty much where it was in time,
in identify_secondary_cpu()?


Yes, this can be done. It would practically have to be the first one just
after CPUHP_BRINGUP_CPU.

The question is whether we really want to call the MTRR/PAT initialization
on hotplugged cpus only after enabling interrupts. Note that the callbacks
are activated only at the end of start_secondary(), while today MTRR/PAT
initialization is called some time earlier by:

  start_secondary()
smp_callin()
  smp_store_cpu_info()
identify_secondary_cpu()
  mtrr_ap_init()

I don't think this is a real problem, but I wanted to mention it.

The next question would be, why MTRR/PAT init should be special (meaning:
why are all the other functions called that early not realized via
callbacks)? Is it just because of the special handling during boot/resume?

It might be worth a discussion whether there shouldn't be a special group
of callbacks activated BEFORE interrupts are being enabled.


Having a special one is warranted, as you explain, I'd say.


Thanks. I'll write a patch for that.


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

On 30.09.2022 14:53, Andrew Cooper wrote:
> On 30/09/2022 08:50, Jan Beulich wrote:
>> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
>> higher priority than the type of the range. To avoid accessing memory at
>> runtime which was re-used for other purposes, make
>> efi_arch_process_memory_map() follow suit. While on x86 in theory the
>> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
>> E820_ACPI memory there and hence that type's handling can be left alone.
>>
>> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
>> Fixes: facac0af87ef ("x86-64: EFI runtime code")
>> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
>> Signed-off-by: Jan Beulich 
> 
> Isn't this also liable to be the root cause of the crash reported on
> IRC, where a read-only page was being inserted into the heap?

I wouldn't be able to make the connection, I'm afraid. What was asked there
was lacking details, though.

Jan

Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

On 30/09/2022 08:50, Jan Beulich wrote:
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there and hence that type's handling can be left alone.
>
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> Fixes: facac0af87ef ("x86-64: EFI runtime code")
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
> Signed-off-by: Jan Beulich 

Isn't this also liable to be the root cause of the crash reported on
IRC, where a read-only page was being inserted into the heap?

~Andrew

Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

Hi Jan,

> On 30 Sep 2022, at 09:50, Jan Beulich  wrote:
> 
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there and hence that type's handling can be left alone.
> 
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> Fixes: facac0af87ef ("x86-64: EFI runtime code")
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
> Signed-off-by: Jan Beulich 

Reviewed-by: Bertrand Marquis  #arm

Cheers
Bertrand

> ---
> Partly RFC for Arm, for two reasons:
> 
> On Arm I question the conversion of EfiACPIReclaimMemory, in two ways:
> For one like on x86 such ranges would likely better be retained, as Dom0
> may (will?) have a need to look at tables placed there. Plus converting
> such ranges to RAM even if EFI_MEMORY_WB is not set looks suspicious to
> me as well. I'd be inclined to make the latter adjustment right here
> (while the other change probably would better be separate, if there
> aren't actually reasons for the present behavior).
> 
> On Arm efi_init_memory() is compiled out, so adjusting Arm code here is
> perhaps more for consistency (not leaving a trap for someone to later
> fall into) than a strict requirement. I wonder though how Arm has
> managed to get away without at least some parts of efi_init_memory() for
> all the years that ACPI support has been present there. I guess this is
> connected to most of runtime.c also being compiled out, but that
> continuing to be the case is another aspect puzzling me.
> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -183,13 +183,15 @@ static EFI_STATUS __init efi_process_mem
> 
> for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> {
> -if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
> - (desc_ptr->Type == EfiConventionalMemory ||
> -  desc_ptr->Type == EfiLoaderCode ||
> -  desc_ptr->Type == EfiLoaderData ||
> -  (!map_bs &&
> -   (desc_ptr->Type == EfiBootServicesCode ||
> -desc_ptr->Type == EfiBootServicesData))) )
> +if ( desc_ptr->Attribute & EFI_MEMORY_RUNTIME )
> +/* nothing */;
> +else if ( (desc_ptr->Attribute & EFI_MEMORY_WB) &&
> +  (desc_ptr->Type == EfiConventionalMemory ||
> +   desc_ptr->Type == EfiLoaderCode ||
> +   desc_ptr->Type == EfiLoaderData ||
> +   (!map_bs &&
> +(desc_ptr->Type == EfiBootServicesCode ||
> + desc_ptr->Type == EfiBootServicesData))) )
> {
> if ( !meminfo_add_bank(, desc_ptr) )
> {
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -185,7 +185,9 @@ static void __init efi_arch_process_memo
> /* fall through */
> case EfiLoaderCode:
> case EfiLoaderData:
> -if ( desc->Attribute & EFI_MEMORY_WB )
> +if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> +type = E820_RESERVED;
> +else if ( desc->Attribute & EFI_MEMORY_WB )
> type = E820_RAM;
> else
> case EfiUnusableMemory:

Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

2022-09-30 Thread Luca Fancellu



> On 30 Sep 2022, at 08:50, Jan Beulich  wrote:
> 
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there and hence that type's handling can be left alone.
> 
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> Fixes: facac0af87ef ("x86-64: EFI runtime code")
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
> Signed-off-by: Jan Beulich 
> ---

Hi Jan,

For the Arm part:

Reviewed-by: Luca Fancellu 

I’ve also tested the EFI+ACPI boot on two arm boards

Tested-By: Luca Fancellu

Re: [PATCH] x86/NUMA: improve memnode_shift calculation for multi node system

On 30.09.2022 13:54, Andrew Cooper wrote:
> On 27/09/2022 17:20, Jan Beulich wrote:
>> --- a/xen/arch/x86/srat.c
>> +++ b/xen/arch/x86/srat.c
>> @@ -413,14 +414,37 @@ acpi_numa_memory_affinity_init(const str
>> node, pxm, start, end - 1,
>> ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
>>  
>> -node_memblk_range[num_node_memblks].start = start;
>> -node_memblk_range[num_node_memblks].end = end;
>> -memblk_nodeid[num_node_memblks] = node;
>> +/* Keep node_memblk_range[] sorted by address. */
>> +for (i = 0; i < num_node_memblks; ++i)
>> +if (node_memblk_range[i].start > start ||
>> +(node_memblk_range[i].start == start &&
>> + node_memblk_range[i].end > end))
>> +break;
>> +
>> +memmove(_memblk_range[i + 1], _memblk_range[i],
>> +(num_node_memblks - i) * sizeof(*node_memblk_range));
>> +node_memblk_range[i].start = start;
>> +node_memblk_range[i].end = end;
>> +
>> +memmove(_nodeid[i + 1], _nodeid[i],
>> +(num_node_memblks - i) * sizeof(*memblk_nodeid));
>> +memblk_nodeid[i] = node;
> 
> This is now the 4th example we have of logic wanting a sorted array. 
> (two examples in ARM code which want to switch away from using sort(),
> and the VMX MSR lists).
> 
> I was already contemplating doing a small library (static inline, or
> perhaps extern inline now we've started using that) to abstract away the
> insert/find/delete operations and their decidedly non-trivial pointer
> operations.

For using such library routines the data structures here would need
re-organizing first: We're inserting into two arrays and a bitmap at
the same time.

> The secondary purpose was to be able to do some actual unit tests of the
> library, so we can be rather better assured of correctness.
> 
> 
> For this case, and the two ARM cases, the firmware data is supposed to
> be sorted to begin with, so the search-for-insertion loop should look at
> the num_node_memblks entry first because the overwhelming common case is
> that the end is the correct place to put it.  If not, it should binary
> search backwards rather than doing a linear search.

Well, yes, perhaps. Of course we don't expect there to be very many
entries, at which point I guess a linear search can be deemed acceptable.

Jan

Re: [PATCH v3 08/10] x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init

2022-09-30 Thread Borislav Petkov

On Thu, Sep 29, 2022 at 10:26:59AM +0200, Juergen Gross wrote:
> So right now I'm inclined to be better on the safe side by not adding any
> cpu hotplug hook, but to use just the same "delayed AP init" flag as today,
> just renaming it. This would leave the delayed MTRR/PAT init in place for
> resume and kexec cases, but deferring the MTRR/PAT cleanup due to this
> potential issue seems not appropriate, as the cleanup isn't changing the
> behavior here.

Ok, what's wrong with adding a special hotplug level just for that thing
and running it very early? Practically pretty much where it was in time,
in identify_secondary_cpu()?

Having a special one is warranted, as you explain, I'd say.

Thx.

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

Hi Jan,

We will review and test the arm part (even though it is modifying some unused
 code at the moment) but I wanted to answer you on some questions you have ..

> On 30 Sep 2022, at 09:50, Jan Beulich  wrote:
> 
> efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
> higher priority than the type of the range. To avoid accessing memory at
> runtime which was re-used for other purposes, make
> efi_arch_process_memory_map() follow suit. While on x86 in theory the
> same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
> E820_ACPI memory there and hence that type's handling can be left alone.
> 
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> Fixes: facac0af87ef ("x86-64: EFI runtime code")
> Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
> Signed-off-by: Jan Beulich 
> ---
> Partly RFC for Arm, for two reasons:
> 
> On Arm I question the conversion of EfiACPIReclaimMemory, in two ways:
> For one like on x86 such ranges would likely better be retained, as Dom0
> may (will?) have a need to look at tables placed there. Plus converting
> such ranges to RAM even if EFI_MEMORY_WB is not set looks suspicious to
> me as well. I'd be inclined to make the latter adjustment right here
> (while the other change probably would better be separate, if there
> aren't actually reasons for the present behavior).
> 
> On Arm efi_init_memory() is compiled out, so adjusting Arm code here is
> perhaps more for consistency (not leaving a trap for someone to later
> fall into) than a strict requirement. I wonder though how Arm has
> managed to get away without at least some parts of efi_init_memory() for
> all the years that ACPI support has been present there. I guess this is
> connected to most of runtime.c also being compiled out, but that
> continuing to be the case is another aspect puzzling me.

On arm we only use the boot services in Xen and we do not provide
any efi services to dom0. The required info is passed through a simple device
tree.
There was a discussion on that subject some weeks ago and it is still an open
point to be solved.
Also APCI is officially unsupported on arm.

Cheers
Bertrand

> 
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -183,13 +183,15 @@ static EFI_STATUS __init efi_process_mem
> 
> for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
> {
> -if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
> - (desc_ptr->Type == EfiConventionalMemory ||
> -  desc_ptr->Type == EfiLoaderCode ||
> -  desc_ptr->Type == EfiLoaderData ||
> -  (!map_bs &&
> -   (desc_ptr->Type == EfiBootServicesCode ||
> -desc_ptr->Type == EfiBootServicesData))) )
> +if ( desc_ptr->Attribute & EFI_MEMORY_RUNTIME )
> +/* nothing */;
> +else if ( (desc_ptr->Attribute & EFI_MEMORY_WB) &&
> +  (desc_ptr->Type == EfiConventionalMemory ||
> +   desc_ptr->Type == EfiLoaderCode ||
> +   desc_ptr->Type == EfiLoaderData ||
> +   (!map_bs &&
> +(desc_ptr->Type == EfiBootServicesCode ||
> + desc_ptr->Type == EfiBootServicesData))) )
> {
> if ( !meminfo_add_bank(, desc_ptr) )
> {
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -185,7 +185,9 @@ static void __init efi_arch_process_memo
> /* fall through */
> case EfiLoaderCode:
> case EfiLoaderData:
> -if ( desc->Attribute & EFI_MEMORY_WB )
> +if ( desc->Attribute & EFI_MEMORY_RUNTIME )
> +type = E820_RESERVED;
> +else if ( desc->Attribute & EFI_MEMORY_WB )
> type = E820_RAM;
> else
> case EfiUnusableMemory:

Re: [PATCH] x86/NUMA: improve memnode_shift calculation for multi node system

On 27/09/2022 17:20, Jan Beulich wrote:
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -413,14 +414,37 @@ acpi_numa_memory_affinity_init(const str
>  node, pxm, start, end - 1,
>  ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
>  
> - node_memblk_range[num_node_memblks].start = start;
> - node_memblk_range[num_node_memblks].end = end;
> - memblk_nodeid[num_node_memblks] = node;
> + /* Keep node_memblk_range[] sorted by address. */
> + for (i = 0; i < num_node_memblks; ++i)
> + if (node_memblk_range[i].start > start ||
> + (node_memblk_range[i].start == start &&
> +  node_memblk_range[i].end > end))
> + break;
> +
> + memmove(_memblk_range[i + 1], _memblk_range[i],
> + (num_node_memblks - i) * sizeof(*node_memblk_range));
> + node_memblk_range[i].start = start;
> + node_memblk_range[i].end = end;
> +
> + memmove(_nodeid[i + 1], _nodeid[i],
> + (num_node_memblks - i) * sizeof(*memblk_nodeid));
> + memblk_nodeid[i] = node;

This is now the 4th example we have of logic wanting a sorted array. 
(two examples in ARM code which want to switch away from using sort(),
and the VMX MSR lists).

I was already contemplating doing a small library (static inline, or
perhaps extern inline now we've started using that) to abstract away the
insert/find/delete operations and their decidedly non-trivial pointer
operations.

The secondary purpose was to be able to do some actual unit tests of the
library, so we can be rather better assured of correctness.

For this case, and the two ARM cases, the firmware data is supposed to
be sorted to begin with, so the search-for-insertion loop should look at
the num_node_memblks entry first because the overwhelming common case is
that the end is the correct place to put it.  If not, it should binary
search backwards rather than doing a linear search.

Obviously not work for 4.17, but there's a lot of value in such a library.

~Andrew

Re: [PATCH] x86/NUMA: improve memnode_shift calculation for multi node system

On Tue, Sep 27, 2022 at 06:20:35PM +0200, Jan Beulich wrote:
> SRAT may describe individual nodes using multiple ranges. When they're
> adjacent (with or without a gap in between), only the start of the first
> such range actually needs accounting for. Furthermore the very first
> range doesn't need considering of its start address at all, as it's fine
> to associate all lower addresses (with no memory) with that same node.
> For this to work, the array of ranges needs to be sorted by address -
> adjust logic accordingly in acpi_numa_memory_affinity_init().
> 
> Signed-off-by: Jan Beulich 

Acked-by: Roger Pau Monné 

Thanks, Roger.

Re: [PATCH for-4.17?] x86: support data operand independent timing mode

On 15/09/2022 11:04, Jan Beulich wrote:
> [1] specifies a long list of instructions which are intended to exhibit
> timing behavior independent of the data they operate on. On certain
> hardware this independence is optional, controlled by a bit in a new
> MSR. Provide a command line option to control the mode Xen and its
> guests are to operate in, with a build time control over the default.
> Longer term we may want to allow guests to control this.
>
> Since Arm64 supposedly also has such a control, put command line option
> and Kconfig control in common files.
>
> [1] 
> https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html
>
> Requested-by: Demi Marie Obenour 
> Signed-off-by: Jan Beulich 

This patch should not be taken; at least not in this form.  The whole
DOITM infrastructure is currently under argument, for being impossible
to use appropriately.

I understand why Qubes want this blanket set, but it is a steep penalty
to pay;  It's only code which is already written trying to be constant
time/cache which gains any security from this.  On current parts, using
SSBD has the same behaviour, but this isn't expected to remain true in
the future.

Forcing it on behind the back of a VM is mutually exclusive with
enumerating it for VMs to use at some point in the future when we have
the capability to.  i.e. specifically, you are not able to maintain the
ABI/API in this patch in the future.

If we do move forward with something like this (under the strict
understanding that the behaviour is going to change in the future), then
"DIT" is too short of an acronym to use.  Amongst other things, it's not
"data independent timing"; it's "controls for forcing ..." which is
important because these are going to be vendor specific, if even needed
in the first place.

~Andrew

Re: [xen-unstable-smoke test] 173362: regressions - FAIL

On Thu, Sep 29, 2022 at 05:25:29PM +0100, Andrew Cooper wrote:
> On 29/09/2022 17:22, osstest service owner wrote:
> > flight 173362 xen-unstable-smoke real [real]
> > http://logs.test-lab.xenproject.org/osstest/logs/173362/
> >
> > Regressions :-(
> >
> > Tests which did not succeed and are blocking,
> > including tests which could not be run:
> >  build-arm64-xsm   6 xen-buildfail REGR. vs. 
> > 173347
> 
> arch/arm/gic-v3-its.c: In function 'gicv3_its_deny_access':
> arch/arm/gic-v3-its.c:905:32: error: passing argument 1 of
> 'iomem_deny_access' discards 'const' qualifier from pointer target type
> [-Werror=discarded-qualifiers]
>  rc = iomem_deny_access(d, mfn, mfn + nr);
>     ^
> In file included from arch/arm/gic-v3-its.c:24:
> ./include/xen/iocap.h:32:52: note: expected 'struct domain *' but
> argument is of type 'const struct domain *'
>  static inline int iomem_deny_access(struct domain *d, unsigned long s,
>  ~~~^
> cc1: all warnings being treated as errors

Hm, sorry.  I've tested on gitlab but seems like I didn't hit that
combination.

Roger.

[xen-unstable test] 173379: tolerable FAIL

flight 173379 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173379/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail pass in 
173361

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-rtds  20 guest-localmigrate/x10 fail in 173361 like 173358
 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 173361 like 
173358
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail in 173361 never pass
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 173361
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 173361
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 173361
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 173361
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 173361
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 173361
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 173361
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 173361
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 173361
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 173361
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 173361
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 173361
 test-arm64-arm64-xl-seattle  15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-arm64-arm64-xl-seattle  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never

Re: [RFC PATCH v1 0/2] Add a new acquire resource to query vcpu statistics

2022-09-30 Thread Matias Ezequiel Vara Larsen

On Thu, Sep 29, 2022 at 05:55:50PM +0200, Jan Beulich wrote:
> On 24.08.2022 15:27, Matias Ezequiel Vara Larsen wrote:
> > The purpose of this RFC is to get feedback about a new acquire resource that
> > exposes vcpu statistics for a given domain. The current mechanism to get 
> > those
> > statistics is by querying the hypervisor. This mechanism relies on a 
> > hypercall
> > and holds the domctl spinlock during its execution. When a pv tool like 
> > xcp-rrdd
> > periodically samples these counters, it ends up affecting other paths that 
> > share
> > that spinlock. By using acquire resources, the pv tool only requires a few
> > hypercalls to set the shared memory region and samples are got without 
> > issuing
> > any other hypercall. The original idea has been suggested by Andrew Cooper 
> > to
> > which I have been discussing about how to implement the current PoC. You can
> > find the RFC patch series at [1]. The series is rebased on top of 
> > stable-4.15.
> > 
> > I am currently a bit blocked on 1) what to expose and 2) how to expose it. 
> > For
> > 1), I decided to expose what xcp-rrdd is querying, e.g., 
> > XEN_DOMCTL_getvcpuinfo.
> > More precisely, xcp-rrd gets runstate.time[RUNSTATE_running]. This is a 
> > uint64_t
> > counter. However, the time spent in other states may be interesting too.
> > Regarding 2), I am not sure if simply using an array of uint64_t is enough 
> > or if
> > a different interface should be exposed. The remaining question is when to 
> > get
> > new values. For the moment, I am updating this counter during
> > vcpu_runstate_change().
> > 
> > The current series includes a simple pv tool that shows how this new 
> > interface is
> > used. This tool maps the counter and periodically samples it.
> > 
> > Any feedback/help would be appreciated.
> 
> Before looking more closely - was there perhaps kind-of-review feedback
> during the summit, which would make it more reasonable to look through
> this once a v2 has appeared?
> 

Yes, there was. I will submit v2 from feedback during summit. Thanks for point
it.

Matias

[xen-unstable-smoke test] 173384: tolerable FAIL - PUSHED

flight 173384 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173384/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 20 guest-start/debianhvm.repeat fail 
like 173342
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  38e1276db4c5457cd6e7811b4e168aa85c8a0b06
baseline version:
 xen  211d8419ef8d8a237ff914fd8304b8fefc3ff2cc

Last test of basis   173347  2022-09-28 05:07:54 Z2 days
Failing since173362  2022-09-29 13:03:03 Z0 days7 attempts
Testing same since   173384  2022-09-30 08:00:25 Z0 days1 attempts


People who touched revisions under test:
  Anthony PERARD 
  Dmytro Semenets 
  Jan Beulich 
  Michal Orzel 
  Nathan Studer 
  Oleksandr Andrushchenko 
  Roger Pau Monné 
  Stefano Stabellini 
  Stewart Hildebrand 
  Tamas K Lengyel 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64fail
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   211d8419ef..38e1276db4  38e1276db4c5457cd6e7811b4e168aa85c8a0b06 -> smoke

Re: [xen-unstable-smoke test] 173362: regressions - FAIL

On 30.09.2022 12:22, Anthony PERARD wrote:
> On Fri, Sep 30, 2022 at 08:31:20AM +0200, Jan Beulich wrote:
>> On 29.09.2022 18:25, Andrew Cooper wrote:
>>> On 29/09/2022 17:22, osstest service owner wrote:
 flight 173362 xen-unstable-smoke real [real]
 http://logs.test-lab.xenproject.org/osstest/logs/173362/

 Regressions :-(

 Tests which did not succeed and are blocking,
 including tests which could not be run:
  build-arm64-xsm   6 xen-buildfail REGR. vs. 
 173347
>>>
>>> arch/arm/gic-v3-its.c: In function 'gicv3_its_deny_access':
>>> arch/arm/gic-v3-its.c:905:32: error: passing argument 1 of
>>> 'iomem_deny_access' discards 'const' qualifier from pointer target type
>>> [-Werror=discarded-qualifiers]
>>>  rc = iomem_deny_access(d, mfn, mfn + nr);
>>>     ^
>>> In file included from arch/arm/gic-v3-its.c:24:
>>> ./include/xen/iocap.h:32:52: note: expected 'struct domain *' but
>>> argument is of type 'const struct domain *'
>>>  static inline int iomem_deny_access(struct domain *d, unsigned long s,
>>>  ~~~^
>>> cc1: all warnings being treated as errors
>>
>> I've sent a patch, but this raises another question: Why does the smoke
>> test (try to) build an unsupported configuration? HAS_ITS (which is
>> necessary to be set for the issue to surface) has its prompt depend on
>> UNSUPPORTED, and (implicitly) defaults to N.
> 
> According to osstest sources:
> # ITS driver is required to boot the Hardware Domain
> # on Xen. For now (Xen 4.10/4.11 at at least),
> # will be not built by default and gated by expert mode
> echo >>xen/.config CONFIG_HAS_ITS=y

Hmm, that's been quite a number of revisions back, without things having
changed. Arm maintainers - what's the plan here? What use is it to test
an unsupported configuration (for years)?

But there's a more general aspect here: EXPERT is forced to Y here as
well, which is fine by itself. But it implies UNSUPPORTED also getting
enabled. That latter aspect is what I consider wrong for smoke flights
at least. Yet (as said) HAS_ITS depends on it (and its setting to Y by
the script would have no effect if UNSUPPORTED was off).

Jan

Re: [PATCH] x86/NUMA: improve memnode_shift calculation for multi node system

On 30.09.2022 12:03, Roger Pau Monné wrote:
> On Fri, Sep 30, 2022 at 10:36:20AM +0200, Jan Beulich wrote:
>> On 30.09.2022 10:25, Roger Pau Monné wrote:
>>> On Tue, Sep 27, 2022 at 06:20:35PM +0200, Jan Beulich wrote:
 @@ -413,14 +414,37 @@ acpi_numa_memory_affinity_init(const str
   node, pxm, start, end - 1,
   ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
  
 -  node_memblk_range[num_node_memblks].start = start;
 -  node_memblk_range[num_node_memblks].end = end;
 -  memblk_nodeid[num_node_memblks] = node;
 +  /* Keep node_memblk_range[] sorted by address. */
 +  for (i = 0; i < num_node_memblks; ++i)
 +  if (node_memblk_range[i].start > start ||
 +  (node_memblk_range[i].start == start &&
>>>
>>> Maybe I'm confused, but won't .start == start means we have
>>> overlapping ranges?
>>
>> Yes, except when a range is empty.
> 
> Hm, OK.  Won't overlapping ranges be bad if not empty?

They are and ...

> Shouldn't Xen complain if it finds overlapping ranges, or that's taken
> care somewhere else?

... we do, elsewhere. Still I'd like this code to be generic, at the very
least to - as said - deal with empty ranges as well. It didn't seem
indicated to me to special case empty ranges here, when the code is more
clear when written in a more generic manner.

Jan

Re: [PATCH 1/2] xen: Add static event channel in SUPPORT.md on ARM

Hi Rahul,

> On 23 Sep 2022, at 13:02, Rahul Singh  wrote:
> 
> Static event channel support is tech preview, which shall be documented
> in SUPPORT.md
> 
> Signed-off-by: Rahul Singh 

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> SUPPORT.md | 7 +++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 8ebd63ad82..29f74ac506 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -922,6 +922,13 @@ bootscrub=off are passed as Xen command line parameters. 
> (Memory should
> be scrubbed with bootscrub=idle.) No XSAs will be issues due to
> unscrubbed memory.
> 
> +## Static Event Channel
> +
> +Allow to setup the static event channel on dom0less system, enabling domains
> +to send/receive notifications.
> +
> +Status, ARM: Tech Preview
> +
> # Format and definitions
> 
> This file contains prose, and machine-readable fragments.
> -- 
> 2.25.1
> 
>

Re: [PATCH 2/2] xen/arm: fix booting ACPI based system after static evtchn series

Hi,

> On 23 Sep 2022, at 13:02, Rahul Singh  wrote:
> 
> When ACPI is enabled and the system booted with ACPI, BUG() is observed
> after merging the static event channel series. As there is not DT when
> booted with ACPI there will be no chosen node because of that
> "BUG_ON(chosen == NULL)" will be hit.
> 
> (XEN) Xen BUG at arch/arm/domain_build.c:3578
> 
> Move call to alloc_static_evtchn() under acpi_disabled check to fix the
> issue.
> 
> Fixes: 1fe16b3ed78a (xen/arm: introduce xen-evtchn dom0less property)
> Signed-off-by: Rahul Singh 

Reviewed-by: Bertrand Marquis 

This is needed to fix ACPI build issues and was release-acked already.

Cheers
Bertrand

> ---
> xen/arch/arm/setup.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 61b4f258a0..4395640019 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -1166,9 +1166,10 @@ void __init start_xen(unsigned long boot_phys_offset,
> printk(XENLOG_INFO "Xen dom0less mode detected\n");
> 
> if ( acpi_disabled )
> +{
> create_domUs();
> -
> -alloc_static_evtchn();
> +alloc_static_evtchn();
> +}
> 
> /*
>  * This needs to be called **before** heap_init_late() so modules
> -- 
> 2.25.1
>

Re: [xen-unstable-smoke test] 173362: regressions - FAIL

2022-09-30 Thread Anthony PERARD

On Fri, Sep 30, 2022 at 08:31:20AM +0200, Jan Beulich wrote:
> On 29.09.2022 18:25, Andrew Cooper wrote:
> > On 29/09/2022 17:22, osstest service owner wrote:
> >> flight 173362 xen-unstable-smoke real [real]
> >> http://logs.test-lab.xenproject.org/osstest/logs/173362/
> >>
> >> Regressions :-(
> >>
> >> Tests which did not succeed and are blocking,
> >> including tests which could not be run:
> >>  build-arm64-xsm   6 xen-buildfail REGR. vs. 
> >> 173347
> > 
> > arch/arm/gic-v3-its.c: In function 'gicv3_its_deny_access':
> > arch/arm/gic-v3-its.c:905:32: error: passing argument 1 of
> > 'iomem_deny_access' discards 'const' qualifier from pointer target type
> > [-Werror=discarded-qualifiers]
> >  rc = iomem_deny_access(d, mfn, mfn + nr);
> >     ^
> > In file included from arch/arm/gic-v3-its.c:24:
> > ./include/xen/iocap.h:32:52: note: expected 'struct domain *' but
> > argument is of type 'const struct domain *'
> >  static inline int iomem_deny_access(struct domain *d, unsigned long s,
> >  ~~~^
> > cc1: all warnings being treated as errors
> 
> I've sent a patch, but this raises another question: Why does the smoke
> test (try to) build an unsupported configuration? HAS_ITS (which is
> necessary to be set for the issue to surface) has its prompt depend on
> UNSUPPORTED, and (implicitly) defaults to N.

According to osstest sources:
# ITS driver is required to boot the Hardware Domain
# on Xen. For now (Xen 4.10/4.11 at at least),
# will be not built by default and gated by expert mode
echo >>xen/.config CONFIG_HAS_ITS=y

https://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=ts-xen-build;h=c294a51eafc26e53b5417529b943224902870acf;hb=HEAD#l131

> 
> Jan
> 

-- 
Anthony PERARD

Re: [PATCH] x86/NUMA: improve memnode_shift calculation for multi node system

On Fri, Sep 30, 2022 at 10:36:20AM +0200, Jan Beulich wrote:
> On 30.09.2022 10:25, Roger Pau Monné wrote:
> > On Tue, Sep 27, 2022 at 06:20:35PM +0200, Jan Beulich wrote:
> >> SRAT may describe individual nodes using multiple ranges. When they're
> >> adjacent (with or without a gap in between), only the start of the first
> >> such range actually needs accounting for. Furthermore the very first
> >> range doesn't need considering of its start address at all, as it's fine
> >> to associate all lower addresses (with no memory) with that same node.
> >> For this to work, the array of ranges needs to be sorted by address -
> >> adjust logic accordingly in acpi_numa_memory_affinity_init().
> > 
> > Speaking for myself (due to the lack of knowledge of the NUMA stuff) I
> > would benefit from a bit of context on why and how memnode_shift is
> > used.
> 
> It's used solely to shift PDXes right before indexing memnodemap[].
> Hence a larger shift allows for a smaller array (less memory and,
> perhaps more importantly, less cache footprint). Personally I don't
> think such needs mentioning in a patch, but I know others think
> differently (George for example looks to believe that the present
> situation should always be described). In the case here a simple
> grep for memnode_shift would tell you, and even if I described this
> here it wouldn't really help review I think: You'd still want to
> verify then that what I claim actually matches reality.

Right, that's why I like some context with the patches.  Sometimes (and
I'm saying it's the case here) the context analysis done by the patch
submitter is wrong, and hence it's easier to detect and point out if
it's part of the commit message.

IMO it also helps when looking at git history.

> >> @@ -413,14 +414,37 @@ acpi_numa_memory_affinity_init(const str
> >>   node, pxm, start, end - 1,
> >>   ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
> >>  
> >> -  node_memblk_range[num_node_memblks].start = start;
> >> -  node_memblk_range[num_node_memblks].end = end;
> >> -  memblk_nodeid[num_node_memblks] = node;
> >> +  /* Keep node_memblk_range[] sorted by address. */
> >> +  for (i = 0; i < num_node_memblks; ++i)
> >> +  if (node_memblk_range[i].start > start ||
> >> +  (node_memblk_range[i].start == start &&
> > 
> > Maybe I'm confused, but won't .start == start means we have
> > overlapping ranges?
> 
> Yes, except when a range is empty.

Hm, OK.  Won't overlapping ranges be bad if not empty?

Shouldn't Xen complain if it finds overlapping ranges, or that's taken
care somewhere else?

Thanks, Roger.

Re: [PATCH] x86/NUMA: improve memnode_shift calculation for multi node system

On 30.09.2022 10:25, Roger Pau Monné wrote:
> On Tue, Sep 27, 2022 at 06:20:35PM +0200, Jan Beulich wrote:
>> SRAT may describe individual nodes using multiple ranges. When they're
>> adjacent (with or without a gap in between), only the start of the first
>> such range actually needs accounting for. Furthermore the very first
>> range doesn't need considering of its start address at all, as it's fine
>> to associate all lower addresses (with no memory) with that same node.
>> For this to work, the array of ranges needs to be sorted by address -
>> adjust logic accordingly in acpi_numa_memory_affinity_init().
> 
> Speaking for myself (due to the lack of knowledge of the NUMA stuff) I
> would benefit from a bit of context on why and how memnode_shift is
> used.

It's used solely to shift PDXes right before indexing memnodemap[].
Hence a larger shift allows for a smaller array (less memory and,
perhaps more importantly, less cache footprint). Personally I don't
think such needs mentioning in a patch, but I know others think
differently (George for example looks to believe that the present
situation should always be described). In the case here a simple
grep for memnode_shift would tell you, and even if I described this
here it wouldn't really help review I think: You'd still want to
verify then that what I claim actually matches reality.

>> @@ -413,14 +414,37 @@ acpi_numa_memory_affinity_init(const str
>> node, pxm, start, end - 1,
>> ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
>>  
>> -node_memblk_range[num_node_memblks].start = start;
>> -node_memblk_range[num_node_memblks].end = end;
>> -memblk_nodeid[num_node_memblks] = node;
>> +/* Keep node_memblk_range[] sorted by address. */
>> +for (i = 0; i < num_node_memblks; ++i)
>> +if (node_memblk_range[i].start > start ||
>> +(node_memblk_range[i].start == start &&
> 
> Maybe I'm confused, but won't .start == start means we have
> overlapping ranges?

Yes, except when a range is empty.

>> + node_memblk_range[i].end > end))
>> +break;
>> +
>> +memmove(_memblk_range[i + 1], _memblk_range[i],
>> +(num_node_memblks - i) * sizeof(*node_memblk_range));
>> +node_memblk_range[i].start = start;
>> +node_memblk_range[i].end = end;
>> +
>> +memmove(_nodeid[i + 1], _nodeid[i],
>> +(num_node_memblks - i) * sizeof(*memblk_nodeid));
>> +memblk_nodeid[i] = node;
>> +
>>  if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
>> -__set_bit(num_node_memblks, memblk_hotplug);
>> +next = true;
>>  if (end > mem_hotplug)
>>  mem_hotplug = end;
>>  }
>> +for (; i <= num_node_memblks; ++i) {
>> +bool prev = next;
>> +
>> +next = test_bit(i, memblk_hotplug);
>> +if (prev)
>> +__set_bit(i, memblk_hotplug);
>> +else
>> +__clear_bit(i, memblk_hotplug);
> 
> Nit: I think you could avoid doing the clear for the last bit, ie:
> else if (i != num_node_memblks) __clear_bit(...);
> 
> But I'm not sure it's worth adding the logic, just makes it more
> complicated to follow.

Indeed. I did consider both this and using a single __change_bit()
wrapped in a suitable if(). Both looked to me like they would hamper
proving the code is doing what it ought to do.

Jan

[libvirt test] 173381: regressions - trouble: blocked/broken/pass

flight 173381 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173381/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   5 host-build-prep  fail REGR. vs. 173345

Tests which did not succeed, but are not blocking:
 build-armhf-libvirt   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass

version targeted for testing:
 libvirt  85aafea4499b20a43d5c208143fc1582ab3d6c84
baseline version:
 libvirt  8ead926cb46f1892116cb56aa89390d194ce0b71

Last test of basis   173345  2022-09-28 04:20:21 Z2 days
Testing same since   173381  2022-09-30 04:18:50 Z0 days1 attempts


People who touched revisions under test:
  Kristina Hanicova 
  Lin Ma 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  broken  
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  blocked 
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-libvirt-xsm pass
 test-arm64-arm64-libvirt-xsm pass
 test-amd64-i386-libvirt-xsm  pass
 test-amd64-amd64-libvirt pass
 test-arm64-arm64-libvirt pass
 test-armhf-armhf-libvirt blocked 
 test-amd64-i386-libvirt  pass
 test-amd64-amd64-libvirt-pairpass
 test-amd64-i386-libvirt-pair pass
 test-arm64-arm64-libvirt-qcow2   pass
 test-armhf-armhf-libvirt-qcow2   blocked 
 test-arm64-arm64-libvirt-raw pass
 test-armhf-armhf-libvirt-raw blocked 
 test-amd64-i386-libvirt-raw  pass
 test-amd64-amd64-libvirt-vhd pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these

Re: [PATCH] x86/NUMA: improve memnode_shift calculation for multi node system

On Tue, Sep 27, 2022 at 06:20:35PM +0200, Jan Beulich wrote:
> SRAT may describe individual nodes using multiple ranges. When they're
> adjacent (with or without a gap in between), only the start of the first
> such range actually needs accounting for. Furthermore the very first
> range doesn't need considering of its start address at all, as it's fine
> to associate all lower addresses (with no memory) with that same node.
> For this to work, the array of ranges needs to be sorted by address -
> adjust logic accordingly in acpi_numa_memory_affinity_init().

Speaking for myself (due to the lack of knowledge of the NUMA stuff) I
would benefit from a bit of context on why and how memnode_shift is
used.

> Signed-off-by: Jan Beulich 
> ---
> On my Dinar and Rome systems this changes memnodemapsize to a single
> page. Originally they used 9 / 130 pages (with shifts going from 8 / 6
> to 15 / 16) respectively, resulting from lowmem gaps [A0,FF] / [A0,BF].
> 
> This goes on top of "x86/NUMA: correct memnode_shift calculation for
> single node system".
> 
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -127,7 +127,8 @@ static int __init extract_lsb_from_nodes
>  epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
>  if ( spdx >= epdx )
>  continue;
> -bitfield |= spdx;
> +if ( i && (!nodeids || nodeids[i - 1] != nodeids[i]) )
> +bitfield |= spdx;
>  if ( !i || !nodeids || nodeids[i - 1] != nodeids[i] )
>  nodes_used++;
>  if ( epdx > memtop )
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -312,6 +312,7 @@ acpi_numa_memory_affinity_init(const str
>   unsigned pxm;
>   nodeid_t node;
>   unsigned int i;
> + bool next = false;
>  
>   if (srat_disabled())
>   return;
> @@ -413,14 +414,37 @@ acpi_numa_memory_affinity_init(const str
>  node, pxm, start, end - 1,
>  ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
>  
> - node_memblk_range[num_node_memblks].start = start;
> - node_memblk_range[num_node_memblks].end = end;
> - memblk_nodeid[num_node_memblks] = node;
> + /* Keep node_memblk_range[] sorted by address. */
> + for (i = 0; i < num_node_memblks; ++i)
> + if (node_memblk_range[i].start > start ||
> + (node_memblk_range[i].start == start &&

Maybe I'm confused, but won't .start == start means we have
overlapping ranges?

> +  node_memblk_range[i].end > end))
> + break;
> +
> + memmove(_memblk_range[i + 1], _memblk_range[i],
> + (num_node_memblks - i) * sizeof(*node_memblk_range));
> + node_memblk_range[i].start = start;
> + node_memblk_range[i].end = end;
> +
> + memmove(_nodeid[i + 1], _nodeid[i],
> + (num_node_memblks - i) * sizeof(*memblk_nodeid));
> + memblk_nodeid[i] = node;
> +
>   if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> - __set_bit(num_node_memblks, memblk_hotplug);
> + next = true;
>   if (end > mem_hotplug)
>   mem_hotplug = end;
>   }
> + for (; i <= num_node_memblks; ++i) {
> + bool prev = next;
> +
> + next = test_bit(i, memblk_hotplug);
> + if (prev)
> + __set_bit(i, memblk_hotplug);
> + else
> + __clear_bit(i, memblk_hotplug);

Nit: I think you could avoid doing the clear for the last bit, ie:
else if (i != num_node_memblks) __clear_bit(...);

But I'm not sure it's worth adding the logic, just makes it more
complicated to follow.

Thanks, Roger.

[PATCH][4.17] EFI: don't convert memory marked for runtime use to ordinary RAM

efi_init_memory() in both relevant places is treating EFI_MEMORY_RUNTIME
higher priority than the type of the range. To avoid accessing memory at
runtime which was re-used for other purposes, make
efi_arch_process_memory_map() follow suit. While on x86 in theory the
same would apply to EfiACPIReclaimMemory, we don't actually "reclaim"
E820_ACPI memory there and hence that type's handling can be left alone.

Fixes: bf6501a62e80 ("x86-64: EFI boot code")
Fixes: facac0af87ef ("x86-64: EFI runtime code")
Fixes: 6d70ea10d49f ("Add ARM EFI boot support")
Signed-off-by: Jan Beulich 
---
Partly RFC for Arm, for two reasons:

On Arm I question the conversion of EfiACPIReclaimMemory, in two ways:
For one like on x86 such ranges would likely better be retained, as Dom0
may (will?) have a need to look at tables placed there. Plus converting
such ranges to RAM even if EFI_MEMORY_WB is not set looks suspicious to
me as well. I'd be inclined to make the latter adjustment right here
(while the other change probably would better be separate, if there
aren't actually reasons for the present behavior).

On Arm efi_init_memory() is compiled out, so adjusting Arm code here is
perhaps more for consistency (not leaving a trap for someone to later
fall into) than a strict requirement. I wonder though how Arm has
managed to get away without at least some parts of efi_init_memory() for
all the years that ACPI support has been present there. I guess this is
connected to most of runtime.c also being compiled out, but that
continuing to be the case is another aspect puzzling me.

--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -183,13 +183,15 @@ static EFI_STATUS __init efi_process_mem
 
 for ( Index = 0; Index < (mmap_size / desc_size); Index++ )
 {
-if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
- (desc_ptr->Type == EfiConventionalMemory ||
-  desc_ptr->Type == EfiLoaderCode ||
-  desc_ptr->Type == EfiLoaderData ||
-  (!map_bs &&
-   (desc_ptr->Type == EfiBootServicesCode ||
-desc_ptr->Type == EfiBootServicesData))) )
+if ( desc_ptr->Attribute & EFI_MEMORY_RUNTIME )
+/* nothing */;
+else if ( (desc_ptr->Attribute & EFI_MEMORY_WB) &&
+  (desc_ptr->Type == EfiConventionalMemory ||
+   desc_ptr->Type == EfiLoaderCode ||
+   desc_ptr->Type == EfiLoaderData ||
+   (!map_bs &&
+(desc_ptr->Type == EfiBootServicesCode ||
+ desc_ptr->Type == EfiBootServicesData))) )
 {
 if ( !meminfo_add_bank(, desc_ptr) )
 {
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -185,7 +185,9 @@ static void __init efi_arch_process_memo
 /* fall through */
 case EfiLoaderCode:
 case EfiLoaderData:
-if ( desc->Attribute & EFI_MEMORY_WB )
+if ( desc->Attribute & EFI_MEMORY_RUNTIME )
+type = E820_RESERVED;
+else if ( desc->Attribute & EFI_MEMORY_WB )
 type = E820_RAM;
 else
 case EfiUnusableMemory:

Re: [PATCH] Arm/vGIC: adjust gicv3_its_deny_access() to fit other gic*_iomem_deny_access(

Hi Jan,

> On 30 Sep 2022, at 08:27, Jan Beulich  wrote:
> 
> While an oversight in 9982fe275ba4 ("arm/vgic: drop const attribute
> from gic_iomem_deny_access()"), the issue really became apparent only
> when iomem_deny_access() was switched to have a non-const first
> parameter.
> 
> Fixes: c4e5cc2ccc5b ("x86/ept: limit calls to memory_type_changed()")
> Reported-by: Andrew Cooper 
> Signed-off-by: Jan Beulich 
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> 
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -892,7 +892,7 @@ struct pending_irq *gicv3_assign_guest_e
> return pirq;
> }
> 
> -int gicv3_its_deny_access(const struct domain *d)
> +int gicv3_its_deny_access(struct domain *d)
> {
> int rc = 0;
> unsigned long mfn, nr;
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -139,7 +139,7 @@ unsigned long gicv3_its_make_hwdom_madt(
> #endif
> 
> /* Deny iomem access for its */
> -int gicv3_its_deny_access(const struct domain *d);
> +int gicv3_its_deny_access(struct domain *d);
> 
> bool gicv3_its_host_has_its(void);
> 
> @@ -206,7 +206,7 @@ static inline unsigned long gicv3_its_ma
> }
> #endif
> 
> -static inline int gicv3_its_deny_access(const struct domain *d)
> +static inline int gicv3_its_deny_access(struct domain *d)
> {
> return 0;
> }

[linux-linus test] 173378: regressions - FAIL

flight 173378 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173378/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64-pvops 6 kernel-build fail REGR. vs. 173364

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-seattle   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-thunderx  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-vhd   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-arm64-arm64-examine  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-raw  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit1   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 173364
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 173364
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 173364
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 173364
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 173364
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 173364
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 173364
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 173364
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass

version targeted for testing:
 linux987a926c1d8a40e4256953b04771fbdb63bc7938
baseline version:
 linux511cce163b75bc3933fa3de769a82bb7e8663f2b

Last test of basis   173364  2022-09-29 15:40:21 Z0 days
Testing same since   173378  2022-09-30 00:41:06 Z0 days1 attempts


People who touched revisions under test:
  Al Viro 
  Linus Torvalds 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvops

Re: [PATCH] Arm/vGIC: adjust gicv3_its_deny_access() to fit other gic*_iomem_deny_access(

2022-09-30 Thread Michal Orzel




On 30/09/2022 08:27, Jan Beulich wrote:
> 
> 
> While an oversight in 9982fe275ba4 ("arm/vgic: drop const attribute
> from gic_iomem_deny_access()"), the issue really became apparent only
> when iomem_deny_access() was switched to have a non-const first
> parameter.
> 
> Fixes: c4e5cc2ccc5b ("x86/ept: limit calls to memory_type_changed()")
> Reported-by: Andrew Cooper 
> Signed-off-by: Jan Beulich 
Reviewed-by: Michal Orzel 
and
Tested-by: Michal Orzel 

~Michal

[xen-unstable-smoke test] 173382: regressions - trouble: blocked/broken/fail/pass

flight 173382 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/173382/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-arm64-xsm   6 xen-buildfail REGR. vs. 173347
 build-armhf   5 host-build-prep  fail REGR. vs. 173347

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass

version targeted for testing:
 xen  fb7485788fd7db3b95f4e7fc9bfdfe9ef38e383f
baseline version:
 xen  211d8419ef8d8a237ff914fd8304b8fefc3ff2cc

Last test of basis   173347  2022-09-28 05:07:54 Z2 days
Failing since173362  2022-09-29 13:03:03 Z0 days6 attempts
Testing same since   173367  2022-09-29 17:01:55 Z0 days5 attempts


People who touched revisions under test:
  Anthony PERARD 
  Dmytro Semenets 
  Jan Beulich 
  Michal Orzel 
  Nathan Studer 
  Oleksandr Andrushchenko 
  Roger Pau Monné 
  Stefano Stabellini 
  Stewart Hildebrand 

jobs:
 build-arm64-xsm  fail
 build-amd64  pass
 build-armhf  broken  
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  blocked 
 test-arm64-arm64-xl-xsm  blocked 
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary

broken-job build-armhf broken

Not pushing.


commit fb7485788fd7db3b95f4e7fc9bfdfe9ef38e383f
Author: Anthony PERARD 
Date:   Thu Sep 29 10:51:31 2022 +0100

automation: Information about running containers for a different arch

Adding pointer to 'qemu-user-static'.

Signed-off-by: Anthony PERARD 
Reviewed-by: Michal Orzel 
Acked-by: Stefano Stabellini 
Release-acked-by: Henry Wang 

commit a210e94af38a957fcc99db01d2cfcc3039859445
Author: Michal Orzel 
Date:   Mon Sep 19 20:37:37 2022 +0200

xen/arm: domain_build: Always print the static shared memory region

At the moment, the information about allocating static shared memory
region is only printed during the debug build. This information can also
be helpful for the end user (which may not be the same as the person
building the package), so switch to printk(). Also drop XENLOG_INFO to be
consistent with other printk() used to print the domain information.

Signed-off-by: Michal Orzel 
Acked-by: Stefano Stabellini 
Release-acked-by: Henry Wang 

commit b726541d94bd0a80b5864d17a2cd2e6d73a3fe0a
Author: Jan Beulich 
Date:   Thu Sep 29 14:47:45 2022 +0200

x86: wire up VCPUOP_register_vcpu_time_memory_area for 32-bit guests

Forever sinced its introduction VCPUOP_register_vcpu_time_memory_area
was available only to native domains. Linux, for example, would attempt
to use it irrespective of guest bitness (including in its so called
PVHVM mode) as long as it finds XEN_PVCLOCK_TSC_STABLE_BIT set (which we
set only for clocksource=tsc, which in turn needs engaging via command
line option).

Fixes: a5d39947cb89 ("Allow guests to register secondary vcpu_time_info")
Signed-off-by: Jan Beulich 
Acked-by: Roger Pau Monné 
Release-acked-by: Henry Wang 

commit 9214da34a3cb017ff0417900250bd6d18ca89e15
Author: Jan Beulich 
Date:   Thu Sep 29 14:46:50 2022 +0200

x86: re-connect VCPUOP_send_nmi for 32-bit guests

With the "inversion" of VCPUOP handling, processing arch-specific ones
first, the forwarding of this sub-op from the (common) compat handler to
(common) non-compat one did no longer have the intended effect. It now
needs forwarding between the arch-specific handlers.

Fixes: 8a96c0ea7999 ("xen: move do_vcpu_op() to arch specific code")
Signed-off-by: Jan Beulich

Re: [PATCH v4 1/2] Avoid using EFI tables Xen may have clobbered

On 30.09.2022 01:02, Demi Marie Obenour wrote:
> Memory of type EFI_CONVENTIONAL_MEMORY, EFI_LOADER_CODE, EFI_LOADER_DATA,
> EFI_BOOT_SERVICES_CODE, and EFI_BOOT_SERVICES_DATA may be clobbered by
> Xen before Linux gets to start using it.  Therefore, Linux under Xen
> must not use EFI tables from such memory.  Most of the remaining EFI
> memory types are not suitable for EFI tables, leaving only
> EFI_ACPI_RECLAIM_MEMORY, EFI_RUNTIME_SERVICES_DATA, and
> EFI_RUNTIME_SERVICES_CODE.  When running under Xen, Linux should only
> use tables that are located in one of these types of memory.
> 
> This patch ensures this, and also adds a function
> (xen_config_table_memory_region_max()) that will be used later to
> replace the usage of the EFI memory map in esrt.c when running under
> Xen.  This function can also be used in mokvar-table.c and efi-bgrt.c,
> but I have not implemented this.
> 
> Signed-off-by: Demi Marie Obenour 

In Xen we don't clobber EfiBootServices{Code,Data} when xen.efi was passed
"-mapbs". Should we perhaps extend the interface such that Dom0 can then
also use tables located in such regions, perhaps by faking
EFI_MEMORY_RUNTIME in the attributes returned by XEN_FW_EFI_MEM_INFO?

Jan

Re: [xen-unstable-smoke test] 173362: regressions - FAIL

On 29.09.2022 18:25, Andrew Cooper wrote:
> On 29/09/2022 17:22, osstest service owner wrote:
>> flight 173362 xen-unstable-smoke real [real]
>> http://logs.test-lab.xenproject.org/osstest/logs/173362/
>>
>> Regressions :-(
>>
>> Tests which did not succeed and are blocking,
>> including tests which could not be run:
>>  build-arm64-xsm   6 xen-buildfail REGR. vs. 
>> 173347
> 
> arch/arm/gic-v3-its.c: In function 'gicv3_its_deny_access':
> arch/arm/gic-v3-its.c:905:32: error: passing argument 1 of
> 'iomem_deny_access' discards 'const' qualifier from pointer target type
> [-Werror=discarded-qualifiers]
>  rc = iomem_deny_access(d, mfn, mfn + nr);
>     ^
> In file included from arch/arm/gic-v3-its.c:24:
> ./include/xen/iocap.h:32:52: note: expected 'struct domain *' but
> argument is of type 'const struct domain *'
>  static inline int iomem_deny_access(struct domain *d, unsigned long s,
>  ~~~^
> cc1: all warnings being treated as errors

I've sent a patch, but this raises another question: Why does the smoke
test (try to) build an unsupported configuration? HAS_ITS (which is
necessary to be set for the issue to surface) has its prompt depend on
UNSUPPORTED, and (implicitly) defaults to N.

Jan

[PATCH] Arm/vGIC: adjust gicv3_its_deny_access() to fit other gic*_iomem_deny_access(

While an oversight in 9982fe275ba4 ("arm/vgic: drop const attribute
from gic_iomem_deny_access()"), the issue really became apparent only
when iomem_deny_access() was switched to have a non-const first
parameter.

Fixes: c4e5cc2ccc5b ("x86/ept: limit calls to memory_type_changed()")
Reported-by: Andrew Cooper 
Signed-off-by: Jan Beulich 

--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -892,7 +892,7 @@ struct pending_irq *gicv3_assign_guest_e
 return pirq;
 }
 
-int gicv3_its_deny_access(const struct domain *d)
+int gicv3_its_deny_access(struct domain *d)
 {
 int rc = 0;
 unsigned long mfn, nr;
--- a/xen/arch/arm/include/asm/gic_v3_its.h
+++ b/xen/arch/arm/include/asm/gic_v3_its.h
@@ -139,7 +139,7 @@ unsigned long gicv3_its_make_hwdom_madt(
 #endif
 
 /* Deny iomem access for its */
-int gicv3_its_deny_access(const struct domain *d);
+int gicv3_its_deny_access(struct domain *d);
 
 bool gicv3_its_host_has_its(void);
 
@@ -206,7 +206,7 @@ static inline unsigned long gicv3_its_ma
 }
 #endif
 
-static inline int gicv3_its_deny_access(const struct domain *d)
+static inline int gicv3_its_deny_access(struct domain *d)
 {
 return 0;
 }

Re: [PATCH v5 5/6] xen/x86: move NUMA scan nodes codes from x86 to common