Re: [PATCH] xen/arm: Properly clean update to init_ttbr and smp_up_cpu

[Bertrand Marquis]

Hi Julien,

Nice finding :-)

> On 30 Jan 2024, at 18:29, Julien Grall  wrote:
> 
> From: Julien Grall 
> 
> Recent rework to the secondary boot code modified how init_ttbr and
> smp_up_cpu are accessed. Rather than directly accessing them, we
> are using a pointer to them.
> 
> The helper clean_dcache() is expected to take the variable in parameter
> and then clean its content. As we now pass a pointer to the variable,
> we will clean the area storing the address rather than the content itself.
> 
> Switch to use clean_dcache_va_range() to avoid casting the pointer.
> 
> Fixes: a5ed59e62c6f ("arm/mmu: Move init_ttbr to a new section .data.idmap")
> Fixes: 9a5114074b04 ("arm/smpboot: Move smp_up_cpu to a new section 
> .data.idmap)
> 
> Reported-by: Oleksandr Tyshchenko 
> Signed-off-by: Julien Grall 

Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

> ---
> xen/arch/arm/mmu/smpboot.c | 2 +-
> xen/arch/arm/smpboot.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index bc91fdfe3331..4ffc8254a44b 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -88,7 +88,7 @@ static void set_init_ttbr(lpae_t *root)
>  * init_ttbr will be accessed with the MMU off, so ensure the update
>  * is visible by cleaning the cache.
>  */
> -clean_dcache(ptr);
> +clean_dcache_va_range(ptr, sizeof(uint64_t));
> 
> unmap_domain_page(ptr);
> }
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 119bfa3160ad..a84e706d77da 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -449,7 +449,7 @@ static void set_smp_up_cpu(unsigned long mpidr)
>  * smp_up_cpu will be accessed with the MMU off, so ensure the update
>  * is visible by cleaning the cache.
>  */
> -clean_dcache(ptr);
> +clean_dcache_va_range(ptr, sizeof(unsigned long));
> 
> unmap_domain_page(ptr);
> 
> -- 
> 2.40.1
> 

[xen-4.15-testing test] 184529: tolerable FAIL - PUSHED

[osstest service owner]

flight 184529 xen-4.15-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/184529/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 184107
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184107
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184107
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184107
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 184107
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184107
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184107
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184107
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 184107
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184107
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184107
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184107
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-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-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail 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-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-xl-thunderx 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-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  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-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-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-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-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-amd64-amd64-libvirt-vhd 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-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-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass

version targeted for testing:
 xen  dfafb3ec959b20307d4c640a48b3b55a2896ac30
baseline version:
 xen  6400013f07e5c7fec9f68821755aed94683b663c

Last test of basis   184107  2023-12-12 14:06:52 Z   49 days
Testing same since   184529  2024-01-30 14:06:47 Z0 days1 attempts


People who touched revisions under test:
  Roger Pau Monné 

jobs:
 build-amd64-xsm 

[ovmf test] 184533: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 909a9a5ae4b8236c1ca7cad7f214c752a579bd67
baseline version:
 ovmf 98c7cb3be73d0f15151133abe91bc880a4400794

Last test of basis   184522  2024-01-29 22:12:44 Z1 days
Testing same since   184533  2024-01-30 15:12:47 Z0 days1 attempts


People who touched revisions under test:
  Rebecca Cran 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  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/osstest/ovmf.git
   98c7cb3be7..909a9a5ae4  909a9a5ae4b8236c1ca7cad7f214c752a579bd67 -> 
xen-tested-master

[xen-4.16-testing test] 184530: tolerable trouble: fail/pass/starved - PUSHED

[osstest service owner]

flight 184530 xen-4.16-testing real [real]
flight 184536 xen-4.16-testing real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/184530/
http://logs.test-lab.xenproject.org/osstest/logs/184536/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-xsm   13 debian-fixupfail pass in 184536-retest
 test-amd64-amd64-xl   20 guest-localmigrate/x10 fail pass in 184536-retest

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184108
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184108
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 184108
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184108
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184108
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184108
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184108
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 184108
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 184108
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184108
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184108
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184108
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-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
 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-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  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  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-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-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw  3 hosts-allocate   starved  n/a
 test-arm64-arm64-libvirt-xsm  3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-credit1   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-credit2   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-xsm   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-vhd   3 hosts-allocate   starved  n/a
 test-arm64-arm64-xl-thunderx  3 hosts-allocate   starved  n/a

version targeted for testing:
 xen  e481fc9f32339ebf9ddd171a3995a3e44527d148
baseline version:
 xen  e7c3d6ceaf73120098f9213fd12f79fd50e8e588

Last test of basis   184108  2023-12-12 14:07:04 Z   49 days
Testing same since   184530  2024-01-30 14:07:07 Z0 days1 attempts


People who touched revisions under test:
  Roger Pau Monné 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-arm64   

[xen-unstable test] 184527: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 184518
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184518
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184518
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184518
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184518
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184518
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 184518
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 184518
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184518
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184518
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184518
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184518
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-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-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-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 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-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 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-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  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-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 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-libvirt-qcow2 14 migrate-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
 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  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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

version targeted for testing:
 xen  4a7e71aa085170f1a13976507c8e248f8715f116
baseline version:
 xen  40a74677023a5eb20d7bbc09def37884f80919bd

Last test of basis   184518  2024-01-29 15:11:13 Z1 days
Testing same since   184527  2024-01-30 06:02:22 Z0 days1 attempts


People who touched revisions under test:
  Roger Pau Monne 
  Roger Pau Monné 

jobs:
 

Re: [PATCH] xen/*/asm-offset: Fix bad copy from x86

[Andrew Cooper]

On 30/01/2024 11:00 pm, Shawn Anastasio wrote:
> Hi Andrew,
>
> On 1/30/24 4:28 PM, Andrew Cooper wrote:
>> All architectures have copy bad logic from x86.
>>
>> OFFSET() having a trailing semi-colon within the macro expansion can be a
>> problematic pattern.  It's benign in this case, but fix it anyway.
>>
>> Perform style fixes for the other macros, and tame the mess of BLANK()
>> position to be consistent (one BLANK() after each block) so the intermediate
>> form is legible too.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Roger Pau Monné 
>> CC: Wei Liu 
>> CC: Stefano Stabellini 
>> CC: Julien Grall 
>> CC: Volodymyr Babchuk 
>> CC: Bertrand Marquis 
>> CC: Michal Orzel 
>> CC: Bob Eshleman 
>> CC: Alistair Francis 
>> CC: Connor Davis 
>> CC: Shawn Anastasio 
>>
>> Why does PPC have a local copy of ilog2() here?  Especially as it's not used,
>> and there's a perfectly good one in 
> I believe this was a carryover from before we had asm/bitops.h
> implemented on PPC. Now that we have bitops implemented (and we don't
> define any offsets that use log2 anymore anyways), this is fine to
> remove. Will submit a patch to do so.

Ah - good to know.  Thanks.

> As for the actual contents of this patch,
>
> Acked-by: Shawn Anastasio 

and thanks also.

~Andrew

Re: [PATCH] xen/*/asm-offset: Fix bad copy from x86

[Shawn Anastasio]

Hi Andrew,

On 1/30/24 4:28 PM, Andrew Cooper wrote:
> All architectures have copy bad logic from x86.
> 
> OFFSET() having a trailing semi-colon within the macro expansion can be a
> problematic pattern.  It's benign in this case, but fix it anyway.
> 
> Perform style fixes for the other macros, and tame the mess of BLANK()
> position to be consistent (one BLANK() after each block) so the intermediate
> form is legible too.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Roger Pau Monné 
> CC: Wei Liu 
> CC: Stefano Stabellini 
> CC: Julien Grall 
> CC: Volodymyr Babchuk 
> CC: Bertrand Marquis 
> CC: Michal Orzel 
> CC: Bob Eshleman 
> CC: Alistair Francis 
> CC: Connor Davis 
> CC: Shawn Anastasio 
> 
> Why does PPC have a local copy of ilog2() here?  Especially as it's not used,
> and there's a perfectly good one in 

I believe this was a carryover from before we had asm/bitops.h
implemented on PPC. Now that we have bitops implemented (and we don't
define any offsets that use log2 anymore anyways), this is fine to
remove. Will submit a patch to do so.

As for the actual contents of this patch,

Acked-by: Shawn Anastasio 

Thanks,
Shawn

[PATCH] xen/*/asm-offset: Fix bad copy from x86

[Andrew Cooper]

All architectures have copy bad logic from x86.

OFFSET() having a trailing semi-colon within the macro expansion can be a
problematic pattern.  It's benign in this case, but fix it anyway.

Perform style fixes for the other macros, and tame the mess of BLANK()
position to be consistent (one BLANK() after each block) so the intermediate
form is legible too.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
CC: Bertrand Marquis 
CC: Michal Orzel 
CC: Bob Eshleman 
CC: Alistair Francis 
CC: Connor Davis 
CC: Shawn Anastasio 

Why does PPC have a local copy of ilog2() here?  Especially as it's not used,
and there's a perfectly good one in 
---
 xen/arch/arm/arm32/asm-offsets.c | 13 +++--
 xen/arch/arm/arm64/asm-offsets.c | 12 +++-
 xen/arch/ppc/ppc64/asm-offsets.c | 11 +++
 xen/arch/riscv/riscv64/asm-offsets.c |  8 
 xen/arch/x86/x86_64/asm-offsets.c|  7 ---
 5 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index 05c692bb2822..f19523d741af 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -15,11 +15,11 @@
 
 #define DEFINE(_sym, _val) \
 asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
-  : : "i" (_val) )
+  :: "i" (_val))
 #define BLANK()\
-asm volatile ( "\n.ascii\"==><==\"" : : )
+asm volatile ("\n.ascii\"==><==\"")
 #define OFFSET(_sym, _str, _mem)   \
-DEFINE(_sym, offsetof(_str, _mem));
+DEFINE(_sym, offsetof(_str, _mem))
 
 void __dummy__(void)
 {
@@ -62,18 +62,19 @@ void __dummy__(void)
BLANK();
 
DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
+   BLANK();
 
OFFSET(VCPU_arch_saved_context, struct vcpu, arch.saved_context);
-
BLANK();
+
DEFINE(PROCINFO_sizeof, sizeof(struct proc_info_list));
OFFSET(PROCINFO_cpu_val, struct proc_info_list, cpu_val);
OFFSET(PROCINFO_cpu_mask, struct proc_info_list, cpu_mask);
OFFSET(PROCINFO_cpu_init, struct proc_info_list, cpu_init);
-
BLANK();
-   OFFSET(INITINFO_stack, struct init_info, stack);
 
+   OFFSET(INITINFO_stack, struct init_info, stack);
+   BLANK();
 }
 
 /*
diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index 7adb67a1b81a..8112b9243d39 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -15,11 +15,11 @@
 
 #define DEFINE(_sym, _val) \
 asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
-  : : "i" (_val) )
+  :: "i" (_val))
 #define BLANK()\
-asm volatile ( "\n.ascii\"==><==\"" : : )
+asm volatile ("\n.ascii\"==><==\"")
 #define OFFSET(_sym, _str, _mem)   \
-DEFINE(_sym, offsetof(_str, _mem));
+DEFINE(_sym, offsetof(_str, _mem))
 
 void __dummy__(void)
 {
@@ -48,13 +48,14 @@ void __dummy__(void)
 
DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
OFFSET(CPUINFO_flags, struct cpu_info, flags);
+   BLANK();
 
OFFSET(VCPU_arch_saved_context, struct vcpu, arch.saved_context);
-
BLANK();
-   OFFSET(INITINFO_stack, struct init_info, stack);
 
+   OFFSET(INITINFO_stack, struct init_info, stack);
BLANK();
+
OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0);
OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2);
OFFSET(ARM_SMCCC_1_2_REGS_X0_OFFS, struct arm_smccc_1_2_regs, a0);
@@ -66,6 +67,7 @@ void __dummy__(void)
OFFSET(ARM_SMCCC_1_2_REGS_X12_OFFS, struct arm_smccc_1_2_regs, a12);
OFFSET(ARM_SMCCC_1_2_REGS_X14_OFFS, struct arm_smccc_1_2_regs, a14);
OFFSET(ARM_SMCCC_1_2_REGS_X16_OFFS, struct arm_smccc_1_2_regs, a16);
+   BLANK();
 }
 
 /*
diff --git a/xen/arch/ppc/ppc64/asm-offsets.c b/xen/arch/ppc/ppc64/asm-offsets.c
index 634d7260e3a4..24065578cbdb 100644
--- a/xen/arch/ppc/ppc64/asm-offsets.c
+++ b/xen/arch/ppc/ppc64/asm-offsets.c
@@ -9,12 +9,12 @@
 #include 
 
 #define DEFINE(_sym, _val)  \
-asm volatile ( "\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
-   : : "i" (_val) )
+asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\""  \
+  : : "i" (_val))
 #define BLANK() \
-asm volatile ( "\n.ascii\"==><==\"" : : )
+asm volatile ("\n.ascii\"==><==\"")
 #define OFFSET(_sym, _str, _mem)\
-DEFINE(_sym, offsetof(_str, _mem));
+DEFINE(_sym, offsetof(_str, _mem))
 
 /* 

[PATCH] xen: Drop superfluous semi-colons

[Andrew Cooper]

All these cases happen to be benign, but drop them anyway.  This is one step
towards making -Wextra-semi work.

Signed-off-by: Andrew Cooper 
---
CC: George Dunlap 
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Wei Liu 
CC: Julien Grall 
---
 xen/common/sched/private.h   | 2 +-
 xen/drivers/acpi/apei/hest.c | 2 +-
 xen/include/xen/livepatch.h  | 2 +-
 xen/include/xen/serial.h | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index 79e3f56c5a08..26a196f4283b 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -518,7 +518,7 @@ static inline void sched_unit_unpause(const struct 
sched_unit *unit)
 }
 
 #define REGISTER_SCHEDULER(x) static const struct scheduler *x##_entry \
-  __used_section(".data.schedulers") = 
+  __used_section(".data.schedulers") = 
 
 struct cpupool
 {
diff --git a/xen/drivers/acpi/apei/hest.c b/xen/drivers/acpi/apei/hest.c
index 4ec28c3c11ba..34b376bc2d74 100644
--- a/xen/drivers/acpi/apei/hest.c
+++ b/xen/drivers/acpi/apei/hest.c
@@ -86,7 +86,7 @@ static int hest_esrc_len(const struct acpi_hest_header 
*hest_hdr)
BUG_ON(len == -1);
 
return len;
-};
+}
 
 int apei_hest_parse(apei_hest_func_t func, void *data)
 {
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index df339a134e40..45df4bba4f45 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -182,7 +182,7 @@ static inline int livepatch_op(struct 
xen_sysctl_livepatch_op *op)
 return -ENOSYS;
 }
 
-static inline void check_for_livepatch_work(void) { };
+static inline void check_for_livepatch_work(void) {}
 static inline bool is_patch(const void *addr)
 {
 return 0;
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 870ca2f6eb03..12ef24351d0f 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -175,7 +175,7 @@ void ehci_dbgp_init(void);
 #ifdef CONFIG_XHCI
 void xhci_dbc_uart_init(void);
 #else
-static void inline xhci_dbc_uart_init(void) {};
+static void inline xhci_dbc_uart_init(void) {}
 #endif
 
 void arm_uart_init(void);
-- 
2.30.2

[PATCH] xen/sched: Fix UB shift in compat_set_timer_op()

[Andrew Cooper]

Tamas reported this UBSAN failure from fuzzing:

  (XEN) 

  (XEN) UBSAN: Undefined behaviour in common/sched/compat.c:48:37
  (XEN) left shift of negative value -2147425536
  (XEN) [ Xen-4.19-unstable  x86_64  debug=y ubsan=y  Not tainted ]
  ...
  (XEN) Xen call trace:
  (XEN)[] R ubsan.c#ubsan_epilogue+0xa/0xd9
  (XEN)[] F __ubsan_handle_shift_out_of_bounds+0x11a/0x1c5
  (XEN)[] F compat_set_timer_op+0x41/0x43
  (XEN)[] F hvm_do_multicall_call+0x77f/0xa75
  (XEN)[] F arch_do_multicall_call+0xec/0xf1
  (XEN)[] F do_multicall+0x1dc/0xde3
  (XEN)[] F hvm_hypercall+0xa00/0x149a
  (XEN)[] F vmx_vmexit_handler+0x1596/0x279c
  (XEN)[] F vmx_asm_vmexit_handler+0xdb/0x200

Left-shifting any negative value is strictly undefined behaviour in C, and
the two parameters here come straight from the guest.

The fuzzer happened to choose lo 0xf, hi 0x8000e300.

Switch everything to be unsigned values, making the shift well defined.

As GCC documents:

  As an extension to the C language, GCC does not use the latitude given in
  C99 and C11 only to treat certain aspects of signed '<<' as undefined.
  However, -fsanitize=shift (and -fsanitize=undefined) will diagnose such
  cases.

this was deemed not to need an XSA.

Fixes: 2942f45e09fb ("Enable compatibility mode operation for 
HYPERVISOR_sched_op and HYPERVISOR_set_timer_op.")
Reported-by: Tamas K Lengyel 
Signed-off-by: Andrew Cooper 
---
CC: George Dunlap 
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Wei Liu 
CC: Julien Grall 
---
 xen/common/sched/compat.c| 4 ++--
 xen/include/hypercall-defs.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/common/sched/compat.c b/xen/common/sched/compat.c
index d718e450d40b..dd97593630ee 100644
--- a/xen/common/sched/compat.c
+++ b/xen/common/sched/compat.c
@@ -43,9 +43,9 @@ static int compat_poll(struct compat_sched_poll *compat)
 
 #include "core.c"
 
-int compat_set_timer_op(uint32_t lo, int32_t hi)
+int compat_set_timer_op(uint32_t lo, uint32_t hi)
 {
-return do_set_timer_op(((s64)hi << 32) | lo);
+return do_set_timer_op(((uint64_t)hi << 32) | lo);
 }
 
 #endif /* __COMMON_SCHED_COMPAT_C__ */
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 6d361ddfce1b..47c093acc84d 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -134,7 +134,7 @@ xenoprof_op(int op, void *arg)
 
 #ifdef CONFIG_COMPAT
 prefix: compat
-set_timer_op(uint32_t lo, int32_t hi)
+set_timer_op(uint32_t lo, uint32_t hi)
 multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls)
 memory_op(unsigned int cmd, void *arg)
 #ifdef CONFIG_IOREQ_SERVER

base-commit: cc6ba68edf6dcd18c3865e7d7c0f1ed822796426
prerequisite-patch-id: de9234b4d0488be5b3be5e2ec23e85789086debc
-- 
2.30.2

Re: [RFC KERNEL PATCH v4 3/3] PCI/sysfs: Add gsi sysfs for pci_dev

[Bjorn Helgaas]

On Tue, Jan 30, 2024 at 10:07:36AM +0100, Roger Pau Monné wrote:
> On Mon, Jan 29, 2024 at 04:01:13PM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 25, 2024 at 07:17:24AM +, Chen, Jiqian wrote:
> > > On 2024/1/24 00:02, Bjorn Helgaas wrote:
> > > > On Tue, Jan 23, 2024 at 10:13:52AM +, Chen, Jiqian wrote:
> > > >> On 2024/1/23 07:37, Bjorn Helgaas wrote:
> > > >>> On Fri, Jan 05, 2024 at 02:22:17PM +0800, Jiqian Chen wrote:
> > >  There is a need for some scenarios to use gsi sysfs.
> > >  For example, when xen passthrough a device to dumU, it will
> > >  use gsi to map pirq, but currently userspace can't get gsi
> > >  number.
> > >  So, add gsi sysfs for that and for other potential scenarios.
> > > >> ...
> > > > 
> > > >>> I don't know enough about Xen to know why it needs the GSI in
> > > >>> userspace.  Is this passthrough brand new functionality that can't be
> > > >>> done today because we don't expose the GSI yet?
> > 
> > I assume this must be new functionality, i.e., this kind of
> > passthrough does not work today, right?
> > 
> > > >> has ACPI support and is responsible for detecting and controlling
> > > >> the hardware, also it performs privileged operations such as the
> > > >> creation of normal (unprivileged) domains DomUs. When we give to a
> > > >> DomU direct access to a device, we need also to route the physical
> > > >> interrupts to the DomU. In order to do so Xen needs to setup and map
> > > >> the interrupts appropriately.
> > > > 
> > > > What kernel interfaces are used for this setup and mapping?
> > >
> > > For passthrough devices, the setup and mapping of routing physical
> > > interrupts to DomU are done on Xen hypervisor side, hypervisor only
> > > need userspace to provide the GSI info, see Xen code:
> > > xc_physdev_map_pirq require GSI and then will call hypercall to pass
> > > GSI into hypervisor and then hypervisor will do the mapping and
> > > routing, kernel doesn't do the setup and mapping.
> > 
> > So we have to expose the GSI to userspace not because userspace itself
> > uses it, but so userspace can turn around and pass it back into the
> > kernel?
> 
> No, the point is to pass it back to Xen, which doesn't know the
> mapping between GSIs and PCI devices because it can't execute the ACPI
> AML resource methods that provide such information.
> 
> The (Linux) kernel is just a proxy that forwards the hypercalls from
> user-space tools into Xen.

But I guess Xen knows how to interpret a GSI even though it doesn't
have access to AML?

> > It seems like it would be better for userspace to pass an identifier
> > of the PCI device itself back into the hypervisor.  Then the interface
> > could be generic and potentially work even on non-ACPI systems where
> > the GSI concept doesn't apply.
> 
> We would still need a way to pass the GSI to PCI device relation to
> the hypervisor, and then cache such data in the hypervisor.
> 
> I don't think we have any preference of where such information should
> be exposed, but given GSIs are an ACPI concept not specific to Xen
> they should be exposed by a non-Xen specific interface.

AFAIK Linux doesn't expose GSIs directly to userspace yet.  The GSI
concept relies on ACPI MADT, _MAT, _PRT, etc.  A GSI is associated
with some device (PCI in this case) and some interrupt controller
entry.  I don't understand how a GSI value is useful without knowing
something about that framework in which GSIs exist.

Obviously I know less than nothing about Xen, so I apologize for
asking all these stupid questions, but it just doesn't all make sense
to me yet.

Bjorn

[libvirt test] 184526: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 184488
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 184488
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 184488
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 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-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-libvirt 15 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-amd64-amd64-libvirt-vhd 14 migrate-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-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass

version targeted for testing:
 libvirt  3a3f73ea9f1925ca5e256fa54c5aa451ddeaa19e
baseline version:
 libvirt  7d9fe3a637795ef23d6c7152b57cd64f9f2894c7

Last test of basis   184488  2024-01-27 04:20:30 Z3 days
Testing same since   184526  2024-01-30 04:18:50 Z0 days1 attempts


People who touched revisions under test:
  Michal Privoznik 

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-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 pass
 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   pass
 test-arm64-arm64-libvirt-raw pass
 test-armhf-armhf-libvirt-raw pass
 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 reports, and of osstest in general, is 

Re: [PATCH v12 03/15] vpci: add hooks for PCI device assign/de-assign

[Stewart Hildebrand]

On 1/9/24 16:51, Stewart Hildebrand wrote:
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 3a973324bca1..a902de6a8693 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1476,6 +1485,10 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>  if ( pdev->broken && d != hardware_domain && d != dom_io )
>  goto done;
>  
> +write_lock(>domain->pci_lock);
> +vpci_deassign_device(pdev);
> +write_unlock(>domain->pci_lock);
> +
>  rc = pdev_msix_assign(d, pdev);
>  if ( rc )
>  goto done;
> @@ -1502,6 +1515,10 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>  pci_to_dev(pdev), flag);
>  }
>  

After rebasing this on the following commit:

  cb4ecb3cc17b ("pci: fail device assignment if phantom functions cannot be 
assigned")

I'll add this here:

if ( rc )
goto done;

I'll plan on retaining Roger's R-b tag and and Jan's A-b tags for v13.

> +write_lock(>pci_lock);
> +rc = vpci_assign_device(pdev);
> +write_unlock(>pci_lock);
> +
>   done:
>  if ( rc )
>  printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",

[PATCH] x86/hvm: Fix UBSAN failure in do_hvm_op() printk

[Andrew Cooper]

Tamas reported this UBSAN failure from fuzzing:

  (XEN) 

  (XEN) UBSAN: Undefined behaviour in common/vsprintf.c:64:19
  (XEN) negation of -9223372036854775808 cannot be represented in type 'long 
long int':
  (XEN) [ Xen-4.19-unstable  x86_64  debug=y ubsan=y  Not tainted ]
  ...
  (XEN) Xen call trace:
  (XEN)[] R ubsan.c#ubsan_epilogue+0xa/0xd9
  (XEN)[] F __ubsan_handle_negate_overflow+0x99/0xce
  (XEN)[] F vsprintf.c#number+0x10a/0x93e
  (XEN)[] F vsnprintf+0x19e2/0x1c56
  (XEN)[] F console.c#vprintk_common+0x76/0x34d
  (XEN)[] F printk+0x4d/0x4f
  (XEN)[] F do_hvm_op+0x288e/0x28f5
  (XEN)[] F hvm_hypercall+0xad2/0x149a
  (XEN)[] F vmx_vmexit_handler+0x1596/0x279c
  (XEN)[] F vmx_asm_vmexit_handler+0xdb/0x200

The problem is an unsigned -> signed converstion because of a bad
formatter (%ld trying to format an unsigned long).

We could fix it by swapping to %lu, but this is a useless printk() even in
debug builds, so just drop it completely.

Reported-by: Tamas K Lengyel 
Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Tamas K Lengyel 

-Wformat-signedness would catch this, but Xen isn't remotely close to being
able to have this warning enabled.
---
 xen/arch/x86/hvm/hvm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e3bd9157d761..e8deeb022216 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5147,12 +5147,9 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 break;
 
 default:
-{
-gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
 rc = -ENOSYS;
 break;
 }
-}
 
 if ( rc == -ERESTART )
 rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",

base-commit: cc6ba68edf6dcd18c3865e7d7c0f1ed822796426
-- 
2.30.2

Re: [PATCH] xen/arm: Properly clean update to init_ttbr and smp_up_cpu

[Oleksandr Tyshchenko]

On 30.01.24 19:29, Julien Grall wrote:

Hello Julien


> From: Julien Grall 
> 
> Recent rework to the secondary boot code modified how init_ttbr and
> smp_up_cpu are accessed. Rather than directly accessing them, we
> are using a pointer to them.
> 
> The helper clean_dcache() is expected to take the variable in parameter
> and then clean its content. As we now pass a pointer to the variable,
> we will clean the area storing the address rather than the content itself.
> 
> Switch to use clean_dcache_va_range() to avoid casting the pointer.
> 
> Fixes: a5ed59e62c6f ("arm/mmu: Move init_ttbr to a new section .data.idmap")
> Fixes: 9a5114074b04 ("arm/smpboot: Move smp_up_cpu to a new section 
> .data.idmap)
> 
> Reported-by: Oleksandr Tyshchenko 
> Signed-off-by: Julien Grall 


[on Renesas R-Car Gen3 SoC with 8 cores (Arm64)]
Tested-by: Oleksandr Tyshchenko 


> ---
>   xen/arch/arm/mmu/smpboot.c | 2 +-
>   xen/arch/arm/smpboot.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index bc91fdfe3331..4ffc8254a44b 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -88,7 +88,7 @@ static void set_init_ttbr(lpae_t *root)
>* init_ttbr will be accessed with the MMU off, so ensure the update
>* is visible by cleaning the cache.
>*/
> -clean_dcache(ptr);
> +clean_dcache_va_range(ptr, sizeof(uint64_t));
>   
>   unmap_domain_page(ptr);
>   }
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 119bfa3160ad..a84e706d77da 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -449,7 +449,7 @@ static void set_smp_up_cpu(unsigned long mpidr)
>* smp_up_cpu will be accessed with the MMU off, so ensure the update
>* is visible by cleaning the cache.
>*/
> -clean_dcache(ptr);
> +clean_dcache_va_range(ptr, sizeof(unsigned long));
>   
>   unmap_domain_page(ptr);
>   

[linux-linus test] 184525: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184517
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184517
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184517
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 184517
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 184517
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 184517
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184517
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184517
 test-amd64-amd64-libvirt-xsm 15 migrate-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-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 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-libvirt-xsm 15 migrate-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-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  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-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  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  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 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-raw 14 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-libvirt-qcow2 14 migrate-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
 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-libvirt 15 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-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass

version targeted for testing:
 linux861c0981648f5b64c86fd028ee622096eb7af05a
baseline version:
 linux41bccc98fb7931d63d03f326a746ac4d429c1dd3

Last test of basis   184517  2024-01-29 12:55:37 Z1 days
Testing same since   184525  2024-01-30 04:10:43 Z0 days1 attempts


People who touched revisions under test:
  Alexander Potapenko 
  Andrew Morton 
  Audra Mitchell 
  Dan Streetman 
  Dave Kleikamp 
  David Hildenbrand 
  Jan Kara 
  Jiri Olsa 
  Jiri Slaby 
  Johannes Weiner 
  Linus Torvalds 
  Lokesh Gidra 
  Marco Elver 
  Masami Hiramatsu (Google) 
  Miaohe Lin 
  Muchun Song 
  Muhammad Usama Anjum 
  Nhat Pham 
  Nico Pache 
  Oleg Nesterov 
  Oliver Sang 
  Petr Vorel 
  Roman Gushchin 
  Ryan Roberts 
  Samuel Holland 
  Seth Jennings 
  Shakeel Butt 
  Sidhartha Kumar 
  Steven Rostedt (Google) 
  Suren Baghdasaryan 
  Yang Shi 
  Yosry Ahmed 
  Zach O'Keefe 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm

[PATCH] xen/arm: Properly clean update to init_ttbr and smp_up_cpu

[Julien Grall]

From: Julien Grall 

Recent rework to the secondary boot code modified how init_ttbr and
smp_up_cpu are accessed. Rather than directly accessing them, we
are using a pointer to them.

The helper clean_dcache() is expected to take the variable in parameter
and then clean its content. As we now pass a pointer to the variable,
we will clean the area storing the address rather than the content itself.

Switch to use clean_dcache_va_range() to avoid casting the pointer.

Fixes: a5ed59e62c6f ("arm/mmu: Move init_ttbr to a new section .data.idmap")
Fixes: 9a5114074b04 ("arm/smpboot: Move smp_up_cpu to a new section .data.idmap)

Reported-by: Oleksandr Tyshchenko 
Signed-off-by: Julien Grall 
---
 xen/arch/arm/mmu/smpboot.c | 2 +-
 xen/arch/arm/smpboot.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
index bc91fdfe3331..4ffc8254a44b 100644
--- a/xen/arch/arm/mmu/smpboot.c
+++ b/xen/arch/arm/mmu/smpboot.c
@@ -88,7 +88,7 @@ static void set_init_ttbr(lpae_t *root)
  * init_ttbr will be accessed with the MMU off, so ensure the update
  * is visible by cleaning the cache.
  */
-clean_dcache(ptr);
+clean_dcache_va_range(ptr, sizeof(uint64_t));
 
 unmap_domain_page(ptr);
 }
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 119bfa3160ad..a84e706d77da 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -449,7 +449,7 @@ static void set_smp_up_cpu(unsigned long mpidr)
  * smp_up_cpu will be accessed with the MMU off, so ensure the update
  * is visible by cleaning the cache.
  */
-clean_dcache(ptr);
+clean_dcache_va_range(ptr, sizeof(unsigned long));
 
 unmap_domain_page(ptr);
 
-- 
2.40.1

Re: [PATCH 0/3] x86/intel: expose additional SPEC_CTRL MSR controls

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 04:25:40PM +, Andrew Cooper wrote:
> On 30/01/2024 9:13 am, Roger Pau Monne wrote:
> > Roger Pau Monne (3):
> >   x86/intel: expose IPRED_CTRL to guests
> >   x86/intel: expose RRSBA_CTRL to guests
> >   x86/intel: expose BHI_CTRL to guests
> 
> A couple of things.  First,
> 
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index a04a11858045..382bc07785d0 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -309,8 +309,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr,
> uint64_t *val)
>  
>  /*
>   * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
> - * separate CPUID features for this functionality, but only set will be
> - * active.
> + * separate CPUID features for some of this functionality, but only one set
> + * will be active on a single host.
>   */
>  uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
>  {
> 
> 
> There was a typo (missing the one in "but only one set"), but you're
> also adding bits now which are Intel-only and very likely to stay that way.

Oh, didn't realize the existing typo.

> IPRED_CTRL finally gives Intel CPUs a control with a similar security
> property to AMD IBRS;  i.e. I doubt AMD are going to gain support for
> these bits when they already guarantee that property and have done for
> years already.
> 
> 
> Next, I can't say that I particularly love that indentation.  This seems
> marginally less bad
> 
>     return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>     (ssbd   ? SPEC_CTRL_SSBD   : 0) |
>     (psfd   ? SPEC_CTRL_PSFD   : 0) |
>     (cp->feat.ipred_ctrl
>  ? (SPEC_CTRL_IPRED_DIS_U | SPEC_CTRL_IPRED_DIS_S) : 0) |
>     (cp->feat.rrsba_ctrl
>  ? (SPEC_CTRL_RRSBA_DIS_U | SPEC_CTRL_RRSBA_DIS_S) : 0) |
>     (cp->feat.bhi_ctrl ? SPEC_CTRL_BHI_DIS_S : 0) |
>     0);
> 
> insofar as at least it's fewer lines.  Given the length of these new
> constants, I can't think of anything better.

I prefer my indentation, but it adds an extra line which might not be
desirable.

Feel free to adjust on commit.

Regards, Roger.

Re: [PATCH v3 01/29] bulk: Access existing variables initialized to >F when available

[Zhao Liu]

On Mon, Jan 29, 2024 at 05:44:43PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Mon, 29 Jan 2024 17:44:43 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH v3 01/29] bulk: Access existing variables initialized to
>  >F when available
> X-Mailer: git-send-email 2.41.0
> 
> When a variable is initialized to >field, use it
> in place. Rationale: while this makes the code more concise,
> this also helps static analyzers.
> 
> Mechanical change using the following Coccinelle spatch script:
> 
>  @@
>  type S, F;
>  identifier s, m, v;
>  @@
>   S *s;
>   ...
>   F *v = >m;
>   <+...
>  ->m
>  +v
>   ...+>
> 
> Inspired-by: Zhao Liu 

Thanks!

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/display/ati.c  |  2 +-
>  hw/misc/macio/pmu.c   |  2 +-
>  hw/misc/pvpanic-pci.c |  2 +-
>  hw/pci-bridge/cxl_root_port.c |  2 +-
>  hw/ppc/pnv.c  | 20 ++--
>  hw/virtio/vhost-user-gpio.c   |  8 
>  hw/virtio/vhost-user-scmi.c   |  6 +++---
>  hw/virtio/virtio-pci.c|  2 +-
>  hw/xen/xen_pt.c   |  6 +++---
>  migration/multifd-zlib.c  |  2 +-
>  target/arm/cpu.c  |  4 ++--
>  target/arm/kvm.c  |  2 +-
>  target/arm/machine.c  |  6 +++---
>  target/i386/hvf/x86hvf.c  |  2 +-
>  target/m68k/helper.c  |  2 +-
>  target/ppc/kvm.c  |  8 
>  target/riscv/cpu_helper.c |  2 +-
>  17 files changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 569b8f6165..8d2501bd82 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -991,7 +991,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>  }
>  vga_init(vga, OBJECT(s), pci_address_space(dev),
>   pci_address_space_io(dev), true);
> -vga->con = graphic_console_init(DEVICE(s), 0, s->vga.hw_ops, >vga);
> +vga->con = graphic_console_init(DEVICE(s), 0, s->vga.hw_ops, vga);
>  if (s->cursor_guest_mode) {
>  vga->cursor_invalidate = ati_cursor_invalidate;
>  vga->cursor_draw_line = ati_cursor_draw_line;
> diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
> index e9a90da88f..7fe1c4e517 100644
> --- a/hw/misc/macio/pmu.c
> +++ b/hw/misc/macio/pmu.c
> @@ -737,7 +737,7 @@ static void pmu_realize(DeviceState *dev, Error **errp)
>  timer_mod(s->one_sec_timer, s->one_sec_target);
>  
>  if (s->has_adb) {
> -qbus_init(>adb_bus, sizeof(s->adb_bus), TYPE_ADB_BUS,
> +qbus_init(adb_bus, sizeof(s->adb_bus), TYPE_ADB_BUS,
>dev, "adb.0");
>  adb_register_autopoll_callback(adb_bus, pmu_adb_poll, s);
>  }

I just have the similar comment as BALATON here.

But I feel like it's not easy to cover this case by script, and further
manual cleanup for "sizeof(s->adb_bus)" is okay.

Others are also okay for me.

Reviewed-by: Zhao Liu 

> diff --git a/hw/misc/pvpanic-pci.c b/hw/misc/pvpanic-pci.c
> index c01e4ce864..83be95d0d2 100644
> --- a/hw/misc/pvpanic-pci.c
> +++ b/hw/misc/pvpanic-pci.c
> @@ -48,7 +48,7 @@ static void pvpanic_pci_realizefn(PCIDevice *dev, Error 
> **errp)
>  PVPanicPCIState *s = PVPANIC_PCI_DEVICE(dev);
>  PVPanicState *ps = >pvpanic;
>  
> -pvpanic_setup_io(>pvpanic, DEVICE(s), 2);
> +pvpanic_setup_io(ps, DEVICE(s), 2);
>  
>  pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, >mr);
>  }
> diff --git a/hw/pci-bridge/cxl_root_port.c b/hw/pci-bridge/cxl_root_port.c
> index 8f97697631..2cf2f7bf5f 100644
> --- a/hw/pci-bridge/cxl_root_port.c
> +++ b/hw/pci-bridge/cxl_root_port.c
> @@ -175,7 +175,7 @@ static void cxl_rp_realize(DeviceState *dev, Error **errp)
>  
>  cxl_cstate->dvsec_offset = CXL_ROOT_PORT_DVSEC_OFFSET;
>  cxl_cstate->pdev = pci_dev;
> -build_dvsecs(>cxl_cstate);
> +build_dvsecs(cxl_cstate);
>  
>  cxl_component_register_block_init(OBJECT(pci_dev), cxl_cstate,
>TYPE_CXL_ROOT_PORT);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0297871bdd..202a569e27 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1257,11 +1257,11 @@ static void pnv_chip_power8_realize(DeviceState *dev, 
> Error **errp)
>  }
>  
>  /* Processor Service Interface (PSI) Host Bridge */
> -object_property_set_int(OBJECT(>psi), "bar", PNV_PSIHB_BASE(chip),
> +object_property_set_int(OBJECT(psi8), "bar", PNV_PSIHB_BASE(chip),
>  _fatal);
> -object_property_set_link(OBJECT(>psi), ICS_PROP_XICS,
> +object_property_set_link(OBJECT(psi8), ICS_PROP_XICS,
>   OBJECT(chip8->xics), _abort);
> -if (!qdev_realize(DEVICE(>psi), NULL, errp)) {
> +if (!qdev_realize(DEVICE(psi8), NULL, errp)) {
>  return;
>  }
>  pnv_xscom_add_subregion(chip, PNV_XSCOM_PSIHB_BASE,
> @@ -1292,7 +1292,7 @@ static void pnv_chip_power8_realize(DeviceState *dev, 
> Error **errp)
>  }
>  

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

[osstest service owner]

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

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  cc6ba68edf6dcd18c3865e7d7c0f1ed822796426
baseline version:
 xen  4a7e71aa085170f1a13976507c8e248f8715f116

Last test of basis   184520  2024-01-29 18:00:26 Z0 days
Testing same since   184528  2024-01-30 14:03:48 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  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
   4a7e71aa08..cc6ba68edf  cc6ba68edf6dcd18c3865e7d7c0f1ed822796426 -> smoke

Re: [PATCH 0/3] x86/intel: expose additional SPEC_CTRL MSR controls

[Andrew Cooper]

On 30/01/2024 9:13 am, Roger Pau Monne wrote:
> Roger Pau Monne (3):
>   x86/intel: expose IPRED_CTRL to guests
>   x86/intel: expose RRSBA_CTRL to guests
>   x86/intel: expose BHI_CTRL to guests

A couple of things.  First,

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index a04a11858045..382bc07785d0 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -309,8 +309,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr,
uint64_t *val)
 
 /*
  * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
- * separate CPUID features for this functionality, but only set will be
- * active.
+ * separate CPUID features for some of this functionality, but only one set
+ * will be active on a single host.
  */
 uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
 {


There was a typo (missing the one in "but only one set"), but you're
also adding bits now which are Intel-only and very likely to stay that way.

IPRED_CTRL finally gives Intel CPUs a control with a similar security
property to AMD IBRS;  i.e. I doubt AMD are going to gain support for
these bits when they already guarantee that property and have done for
years already.


Next, I can't say that I particularly love that indentation.  This seems
marginally less bad

    return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
    (ssbd   ? SPEC_CTRL_SSBD   : 0) |
    (psfd   ? SPEC_CTRL_PSFD   : 0) |
    (cp->feat.ipred_ctrl
 ? (SPEC_CTRL_IPRED_DIS_U | SPEC_CTRL_IPRED_DIS_S) : 0) |
    (cp->feat.rrsba_ctrl
 ? (SPEC_CTRL_RRSBA_DIS_U | SPEC_CTRL_RRSBA_DIS_S) : 0) |
    (cp->feat.bhi_ctrl ? SPEC_CTRL_BHI_DIS_S : 0) |
    0);

insofar as at least it's fewer lines.  Given the length of these new
constants, I can't think of anything better.

Happy to fix both on commit.

~Andrew

Re: [PATCH v3 14/29] target/i386: Prefer fast cpu_env() over slower CPU QOM cast macro

[Zhao Liu]

On Mon, Jan 29, 2024 at 05:44:56PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Mon, 29 Jan 2024 17:44:56 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH v3 14/29] target/i386: Prefer fast cpu_env() over slower
>  CPU QOM cast macro
> X-Mailer: git-send-email 2.41.0
> 
> Mechanical patch produced running the command documented
> in scripts/coccinelle/cpu_env.cocci_template header.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/i386/hvf/vmx.h| 13 ++---
>  hw/i386/fw_cfg.c |  3 +-
>  hw/i386/vmmouse.c|  6 ++--
>  hw/i386/xen/xen-hvm.c|  3 +-
>  target/i386/arch_dump.c  | 11 ++--
>  target/i386/arch_memory_mapping.c|  3 +-
>  target/i386/cpu-dump.c   |  3 +-
>  target/i386/cpu.c| 37 
>  target/i386/helper.c | 42 
>  target/i386/hvf/hvf.c|  8 ++
>  target/i386/hvf/x86.c|  4 +--
>  target/i386/hvf/x86_emu.c|  6 ++--
>  target/i386/hvf/x86_task.c   | 10 ++-
>  target/i386/hvf/x86hvf.c |  9 ++
>  target/i386/kvm/kvm.c|  6 ++--
>  target/i386/kvm/xen-emu.c| 32 +++--
>  target/i386/tcg/sysemu/bpt_helper.c  |  3 +-
>  target/i386/tcg/sysemu/excp_helper.c |  3 +-
>  target/i386/tcg/tcg-cpu.c| 14 +++---
>  target/i386/tcg/user/excp_helper.c   |  6 ++--
>  target/i386/tcg/user/seg_helper.c|  3 +-
>  21 files changed, 67 insertions(+), 158 deletions(-)

Reviewed-by: Zhao Liu 

> 
> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
> index 0fffcfa46c..1ad042269b 100644
> --- a/target/i386/hvf/vmx.h
> +++ b/target/i386/hvf/vmx.h
> @@ -175,8 +175,7 @@ static inline void macvm_set_cr4(hv_vcpuid_t vcpu, 
> uint64_t cr4)
>  
>  static inline void macvm_set_rip(CPUState *cpu, uint64_t rip)
>  {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -CPUX86State *env = _cpu->env;
> +CPUX86State *env = cpu_env(cpu);
>  uint64_t val;
>  
>  /* BUG, should take considering overlap.. */
> @@ -196,10 +195,7 @@ static inline void macvm_set_rip(CPUState *cpu, uint64_t 
> rip)
>  
>  static inline void vmx_clear_nmi_blocking(CPUState *cpu)
>  {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -CPUX86State *env = _cpu->env;
> -
> -env->hflags2 &= ~HF2_NMI_MASK;
> +cpu_env(cpu)->hflags2 &= ~HF2_NMI_MASK;
>  uint32_t gi = (uint32_t) rvmcs(cpu->accel->fd, 
> VMCS_GUEST_INTERRUPTIBILITY);
>  gi &= ~VMCS_INTERRUPTIBILITY_NMI_BLOCKING;
>  wvmcs(cpu->accel->fd, VMCS_GUEST_INTERRUPTIBILITY, gi);
> @@ -207,10 +203,7 @@ static inline void vmx_clear_nmi_blocking(CPUState *cpu)
>  
>  static inline void vmx_set_nmi_blocking(CPUState *cpu)
>  {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -CPUX86State *env = _cpu->env;
> -
> -env->hflags2 |= HF2_NMI_MASK;
> +cpu_env(cpu)->hflags2 |= HF2_NMI_MASK;
>  uint32_t gi = (uint32_t)rvmcs(cpu->accel->fd, 
> VMCS_GUEST_INTERRUPTIBILITY);
>  gi |= VMCS_INTERRUPTIBILITY_NMI_BLOCKING;
>  wvmcs(cpu->accel->fd, VMCS_GUEST_INTERRUPTIBILITY, gi);
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 7362daa45a..5239cd40fa 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -155,8 +155,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
>  
>  void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg)
>  {
> -X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(ms->possible_cpus->cpus[0].cpu);
>  uint32_t unused, ebx, ecx, edx;
>  uint64_t feature_control_bits = 0;
>  uint64_t *val;
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index a8d014d09a..f292a14a15 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -74,8 +74,7 @@ struct VMMouseState {
>  
>  static void vmmouse_get_data(uint32_t *data)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
>  data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
> @@ -84,8 +83,7 @@ static void vmmouse_get_data(uint32_t *data)
>  
>  static void vmmouse_set_data(const uint32_t *data)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  env->regs[R_EAX] = data[0]; env->regs[R_EBX] = data[1];
>  env->regs[R_ECX] = data[2]; env->regs[R_EDX] = data[3];
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index f42621e674..61e5060117 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -487,8 +487,7 @@ static void regs_to_cpu(vmware_regs_t *vmport_regs, 
> ioreq_t *req)
>  
>  static void regs_from_cpu(vmware_regs_t *vmport_regs)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Andrew Cooper]

On 30/01/2024 12:59 pm, Jan Beulich wrote:
> On 30.01.2024 13:06, Roger Pau Monné wrote:
>> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
>>> On 30.01.2024 10:13, Roger Pau Monne wrote:
 The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls 
 in
 SPEC_CTRL MSR.

 Note that those controls are not used by the hypervisor.
>>> Despite this, ...
>>>
 --- a/xen/arch/x86/msr.c
 +++ b/xen/arch/x86/msr.c
 @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct 
 cpu_policy *cp)
  return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
  (ssbd   ? SPEC_CTRL_SSBD   : 0) |
  (psfd   ? SPEC_CTRL_PSFD   : 0) |
 +(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
 +SPEC_CTRL_IPRED_DIS_S)
 + : 0) |
  0);
  }
>>> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
>>> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
>>> description it wants clarifying why it is acceptable to run Xen with the
>>> guest chosen settings for at least the DIS_S bit (assuming that it is
>>> okay to do so). Likely (didn't look there yet) also applicable to the
>>> further two patches.
>> "The added feature is made dependent on IBRSB, which ensures it will
>> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
>> ensures the value of SPEC_CTRL will get context switched on exit/entry
>> to guest."
>>
>> Would adding the above to the commit message clarify the intended
>> implementation?
> It would improve things, at least hinting towards there being a connection
> between exposure and updating on entry to Xen. I'd like to ask though to
> avoid "context switch" when talking about entry from guest context. While
> in a way technically correct, our normal meaning of the term is the
> process of switching vCPU-s out/in on a pCPU.

The guests can only see MSR_SPEC_CTRL when Xen knows (and is) context
switching them properly.

The logic is weird for legacy IBRS reasons, but I don't think any
further justification is necessary.

~Andrew

Mirroring git repositories to gitlab

[Yann Dirson]

Hello all,

The official project git repositories on https://xenbits.xen.org/ do not 
let people subscribe to get eg. notifications on push.  A few repos are 
mirrored in https://gitlab.com/xen-project/ but it does not look like 
there are that many of them, aside from xen.git.

I would love to have all repositories publicly mirrored there, maybe in 
a "xenbits-mirror" subgroub.  Could we do that, or would there be any 
problem with it?

Best regards,
-- 
Yann


Yann Dirson | Vates Platform Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v3 13/34] xen/riscv: introduce cmpxchg.h

[Oleksii]

On Tue, 2024-01-30 at 16:05 +0100, Jan Beulich wrote:
> On 30.01.2024 15:57, Oleksii wrote:
> > On Mon, 2024-01-22 at 17:27 +0100, Jan Beulich wrote:
> > > > +#define __xchg_acquire(ptr, new, size) \
> > > > +({ \
> > > > +    __typeof__(ptr) ptr__ = (ptr); \
> > > > +    __typeof__(new) new__ = (new); \
> > > > +    __typeof__(*(ptr)) ret__; \
> > > > +    switch (size) \
> > > > +   { \
> > > > +    case 4: \
> > > > +    asm volatile( \
> > > > +    "  amoswap.w %0, %2, %1\n" \
> > > > +    RISCV_ACQUIRE_BARRIER \
> > > > +    : "=r" (ret__), "+A" (*ptr__) \
> > > > +    : "r" (new__) \
> > > > +    : "memory" ); \
> > > > +    break; \
> > > > +    case 8: \
> > > > +    asm volatile( \
> > > > +    "  amoswap.d %0, %2, %1\n" \
> > > > +    RISCV_ACQUIRE_BARRIER \
> > > > +    : "=r" (ret__), "+A" (*ptr__) \
> > > > +    : "r" (new__) \
> > > > +    : "memory" ); \
> > > > +    break; \
> > > > +    default: \
> > > > +    ASSERT_UNREACHABLE(); \
> > > > +    } \
> > > > +    ret__; \
> > > > +})
> > > 
> > > If I'm not mistaken this differs from __xchg_relaxed() only in
> > > the
> > > use
> > > of RISCV_ACQUIRE_BARRIER, and ...
> > > 
> > > > +#define xchg_acquire(ptr, x) \
> > > > +({ \
> > > > +    __typeof__(*(ptr)) x_ = (x); \
> > > > +    (__typeof__(*(ptr))) __xchg_acquire((ptr), x_,
> > > > sizeof(*(ptr))); \
> > > > +})
> > > > +
> > > > +#define __xchg_release(ptr, new, size) \
> > > > +({ \
> > > > +    __typeof__(ptr) ptr__ = (ptr); \
> > > > +    __typeof__(new) new__ = (new); \
> > > > +    __typeof__(*(ptr)) ret__; \
> > > > +    switch (size) \
> > > > +   { \
> > > > +    case 4: \
> > > > +    asm volatile ( \
> > > > +    RISCV_RELEASE_BARRIER \
> > > > +    "  amoswap.w %0, %2, %1\n" \
> > > > +    : "=r" (ret__), "+A" (*ptr__) \
> > > > +    : "r" (new__) \
> > > > +    : "memory"); \
> > > > +    break; \
> > > > +    case 8: \
> > > > +    asm volatile ( \
> > > > +    RISCV_RELEASE_BARRIER \
> > > > +    "  amoswap.d %0, %2, %1\n" \
> > > > +    : "=r" (ret__), "+A" (*ptr__) \
> > > > +    : "r" (new__) \
> > > > +    : "memory"); \
> > > > +    break; \
> > > > +    default: \
> > > > +    ASSERT_UNREACHABLE(); \
> > > > +    } \
> > > > +    ret__; \
> > > > +})
> > > 
> > > this only in the use of RISCV_RELEASE_BARRIER. If so they likely
> > > want
> > > folding, to limit redundancy and make eventual updating easier.
> > > (Same
> > > for the cmpxchg helper further down, as it seems.)
> > Also the difference is in where to place barrier before or after
> > atomic
> > instruction. I am not sure that we can easily folded this macros.
> 
> The folded macro would have two barrier parameters - on for acquire,
> one
> for release.
Yes, in such case it will work.

Thanks.

~ Oleksii

Re: [PATCH] x86/traps: Annotate {l,c}star_enter() as nocall

[Jan Beulich]

On 30.01.2024 16:08, Andrew Cooper wrote:
> ... as with other declarations which aren't legal to call from C.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Jan Beulich 

Re: [PATCH] x86/boot: Add braces in reloc.c

[Jan Beulich]

On 30.01.2024 15:48, Andrew Cooper wrote:
> 107 lines is an unreasonably large switch statement to live inside a
> brace-less for loop.  Drop the comment that's clumsily trying to cover the
> fact that this logic has wrong-looking indentation.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Jan Beulich 

[PATCH] x86/traps: Annotate {l,c}star_enter() as nocall

[Andrew Cooper]

... as with other declarations which aren't legal to call from C.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
---
 xen/arch/x86/x86_64/traps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 668605e5bc67..02fdb3637d09 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -333,8 +333,8 @@ static unsigned int write_stub_trampoline(
 
 DEFINE_PER_CPU(struct stubs, stubs);
 
-void lstar_enter(void);
-void cstar_enter(void);
+void nocall lstar_enter(void);
+void nocall cstar_enter(void);
 
 void subarch_percpu_traps_init(void)
 {

base-commit: 4a7e71aa085170f1a13976507c8e248f8715f116
prerequisite-patch-id: 897f8a9f53b9c95c7961be1793b2685c11dc6e50
-- 
2.30.2

Re: [PATCH v3 13/34] xen/riscv: introduce cmpxchg.h

[Jan Beulich]

On 30.01.2024 15:57, Oleksii wrote:
> On Mon, 2024-01-22 at 17:27 +0100, Jan Beulich wrote:
>>> +#define __xchg_acquire(ptr, new, size) \
>>> +({ \
>>> +    __typeof__(ptr) ptr__ = (ptr); \
>>> +    __typeof__(new) new__ = (new); \
>>> +    __typeof__(*(ptr)) ret__; \
>>> +    switch (size) \
>>> +   { \
>>> +    case 4: \
>>> +    asm volatile( \
>>> +    "  amoswap.w %0, %2, %1\n" \
>>> +    RISCV_ACQUIRE_BARRIER \
>>> +    : "=r" (ret__), "+A" (*ptr__) \
>>> +    : "r" (new__) \
>>> +    : "memory" ); \
>>> +    break; \
>>> +    case 8: \
>>> +    asm volatile( \
>>> +    "  amoswap.d %0, %2, %1\n" \
>>> +    RISCV_ACQUIRE_BARRIER \
>>> +    : "=r" (ret__), "+A" (*ptr__) \
>>> +    : "r" (new__) \
>>> +    : "memory" ); \
>>> +    break; \
>>> +    default: \
>>> +    ASSERT_UNREACHABLE(); \
>>> +    } \
>>> +    ret__; \
>>> +})
>>
>> If I'm not mistaken this differs from __xchg_relaxed() only in the
>> use
>> of RISCV_ACQUIRE_BARRIER, and ...
>>
>>> +#define xchg_acquire(ptr, x) \
>>> +({ \
>>> +    __typeof__(*(ptr)) x_ = (x); \
>>> +    (__typeof__(*(ptr))) __xchg_acquire((ptr), x_,
>>> sizeof(*(ptr))); \
>>> +})
>>> +
>>> +#define __xchg_release(ptr, new, size) \
>>> +({ \
>>> +    __typeof__(ptr) ptr__ = (ptr); \
>>> +    __typeof__(new) new__ = (new); \
>>> +    __typeof__(*(ptr)) ret__; \
>>> +    switch (size) \
>>> +   { \
>>> +    case 4: \
>>> +    asm volatile ( \
>>> +    RISCV_RELEASE_BARRIER \
>>> +    "  amoswap.w %0, %2, %1\n" \
>>> +    : "=r" (ret__), "+A" (*ptr__) \
>>> +    : "r" (new__) \
>>> +    : "memory"); \
>>> +    break; \
>>> +    case 8: \
>>> +    asm volatile ( \
>>> +    RISCV_RELEASE_BARRIER \
>>> +    "  amoswap.d %0, %2, %1\n" \
>>> +    : "=r" (ret__), "+A" (*ptr__) \
>>> +    : "r" (new__) \
>>> +    : "memory"); \
>>> +    break; \
>>> +    default: \
>>> +    ASSERT_UNREACHABLE(); \
>>> +    } \
>>> +    ret__; \
>>> +})
>>
>> this only in the use of RISCV_RELEASE_BARRIER. If so they likely want
>> folding, to limit redundancy and make eventual updating easier. (Same
>> for the cmpxchg helper further down, as it seems.)
> Also the difference is in where to place barrier before or after atomic
> instruction. I am not sure that we can easily folded this macros.

The folded macro would have two barrier parameters - on for acquire, one
for release.

Jan

Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand]

On 1/25/24 07:33, Roger Pau Monné wrote:
> On Thu, Jan 25, 2024 at 12:23:05PM +0100, Jan Beulich wrote:
>> On 25.01.2024 10:05, Roger Pau Monné wrote:
>>> On Thu, Jan 25, 2024 at 08:43:05AM +0100, Jan Beulich wrote:
 On 24.01.2024 18:51, Roger Pau Monné wrote:
> On Wed, Jan 24, 2024 at 12:34:10PM +0100, Jan Beulich wrote:
>> On 24.01.2024 10:24, Roger Pau Monné wrote:
>>> On Wed, Jan 24, 2024 at 09:48:35AM +0100, Jan Beulich wrote:
 On 23.01.2024 16:07, Roger Pau Monné wrote:
> On Tue, Jan 23, 2024 at 03:32:12PM +0100, Jan Beulich wrote:
>> On 15.01.2024 20:43, Stewart Hildebrand wrote:
>>> @@ -2888,6 +2888,8 @@ int allocate_and_map_msi_pirq(struct domain 
>>> *d, int index, int *pirq_p,
>>>  {
>>>  int irq, pirq, ret;
>>>  
>>> +ASSERT(pcidevs_locked() || rw_is_locked(>pci_lock));
>>
>> If either lock is sufficient to hold here, ...
>>
>>> --- a/xen/arch/x86/physdev.c
>>> +++ b/xen/arch/x86/physdev.c
>>> @@ -123,7 +123,9 @@ int physdev_map_pirq(domid_t domid, int type, 
>>> int *index, int *pirq_p,
>>>  
>>>  case MAP_PIRQ_TYPE_MSI:
>>>  case MAP_PIRQ_TYPE_MULTI_MSI:
>>> +pcidevs_lock();
>>>  ret = allocate_and_map_msi_pirq(d, *index, pirq_p, type, 
>>> msi);
>>> +pcidevs_unlock();
>>>  break;
>>
>
> IIRC (Stewart can further comment) this is done holding the pcidevs
> lock to keep the path unmodified, as there's no need to hold the
> per-domain rwlock.

 Yet why would we prefer to acquire a global lock when a per-domain one
 suffices?
>>>
>>> I was hoping to introduce less changes, specially if they are not
>>> strictly required, as it's less risk.  I'm always quite worry of
>>> locking changes.
>>
>> In which case more description / code commenting is needed. The pattern
>> of the assertions looks dangerous.
>
> Is such dangerousness perception because you fear some of the pcidevs
> lock usage might be there not just for preventing the pdev from going
> away, but also to guarantee exclusive access to certain state?

 Indeed. In my view the main purpose of locks is to guard state. Their
 use here to guard against devices here is imo rather an abuse; as
 mentioned before this should instead be achieved e.g via refcounting.
 And it's bad enough already that pcidevs_lock() alone has been abused
 this way, without proper marking (leaving us to guess in many places).
 It gets worse when a second lock can now also serve this same
 purpose.
>>>
>>> The new lock is taken in read mode in most contexts, and hence can't
>>> be used to indirectly gain exclusive access to domain related
>>> structures in a safe way.
>>
>> Oh, right - I keep being misled by rw_is_locked(). This is a fair
>> argument. Irrespective it would feel better to me if an abstraction
>> construct was introduced; but seeing you don't like the idea I guess
>> I won't insist.
> 
> TBH I'm not going to argue against it if you and Stewart think it's
> clearer, but I also won't request the addition of such wrapper myself.
> 
> Thanks, Roger.

Overall, I think there are two sources of confusion:

1. This patch is using the odd-looking ASSERTs to verify that it is safe to 
*read* d->pdev_list, and/or ensure a pdev does not go away or get reassigned. 
The purpose of these ASSERTs is not immediately obvious due to inadequate 
description.

2. At first glance, the patch appears to be doing two things: using 
d->pci_lock for d->pdev_list/pdev protection, and using d->pci_lock for 
pdev->vpci protection.

Regarding #1, while the review experience could have been improved by 
introducing a wrapper construct, I think it would also (more importantly) be 
valuable to have such a wrapper for the sake of code readability. I think it is 
important to get this right and hopefully avoid/reduce potential future 
confusion. I'll add something like this in v13, e.g. in sched.h:

/* Ensure pdevs do not go away or get assigned to other domains. */
#define pdev_list_is_read_locked(d) ({   \

struct domain *d_ = (d); \

pcidevs_locked() || (d_ && rw_is_locked(_->pci_lock)); \

})

Example use:

ASSERT(pdev_list_is_read_locked(d));

Regarding #2, the patch description primarily talks about protecting the 
pdev->vpci field, and the d->pdev_list read / pdev reassignment protection 
seems an afterthought. However, the use of pcidevs_lock() for pdev protection 
is pre-existing. Now that vPCI callers are going to use d->pci_lock (instead of 
pcidevs_lock()), we are simultaneously changing the allowable mechanism for 
protecting d->pdev_list reads and pdevs going away or getting reassigned. I 
briefly 

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Andrew Cooper]

On 30/01/2024 3:02 pm, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 01:55:56PM +0100, Jan Beulich wrote:
>> On 30.01.2024 12:46, Roger Pau Monné wrote:
>>> On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
 On 30.01.2024 11:27, Roger Pau Monne wrote:
> Dummy set/clear tests for additional spec_ctrl bits.
> ---
>  docs/all-tests.dox  |   2 +
>  tests/test/Makefile |   9 
>  tests/test/main.c   | 100 
>  3 files changed, 111 insertions(+)
>  create mode 100644 tests/test/Makefile
>  create mode 100644 tests/test/main.c
 I'm puzzled: Why "test"? That doesn't describe in any way what this test
 is about.
>>> That's just my place holder for random XTF stuff.  I don't intend this
>>> to be committed.
>> Could have been said then one way or another.
> Yes, realized later when speaking with Andrew that I had forgot to send
> the test I've used, and then didn't adjust the message when sending to
> note this wasn't supposed to be applied.

I've got a local test with some of this in, which I'll extend.

But as with many other things, it's waiting on fixing the test-revision
build infrastructure so the OSSTest Bisector doesn't break when adding
new content to an existing test.

~Andrew

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 01:55:56PM +0100, Jan Beulich wrote:
> On 30.01.2024 12:46, Roger Pau Monné wrote:
> > On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
> >> On 30.01.2024 11:27, Roger Pau Monne wrote:
> >>> Dummy set/clear tests for additional spec_ctrl bits.
> >>> ---
> >>>  docs/all-tests.dox  |   2 +
> >>>  tests/test/Makefile |   9 
> >>>  tests/test/main.c   | 100 
> >>>  3 files changed, 111 insertions(+)
> >>>  create mode 100644 tests/test/Makefile
> >>>  create mode 100644 tests/test/main.c
> >>
> >> I'm puzzled: Why "test"? That doesn't describe in any way what this test
> >> is about.
> > 
> > That's just my place holder for random XTF stuff.  I don't intend this
> > to be committed.
> 
> Could have been said then one way or another.

Yes, realized later when speaking with Andrew that I had forgot to send
the test I've used, and then didn't adjust the message when sending to
note this wasn't supposed to be applied.

Regards, Roger.

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 03:47:37PM +0100, Jan Beulich wrote:
> On 30.01.2024 15:35, Roger Pau Monné wrote:
> > On Tue, Jan 30, 2024 at 01:59:14PM +0100, Jan Beulich wrote:
> >> On 30.01.2024 13:06, Roger Pau Monné wrote:
> >>> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
>  On 30.01.2024 10:13, Roger Pau Monne wrote:
> > The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} 
> > controls in
> > SPEC_CTRL MSR.
> >
> > Note that those controls are not used by the hypervisor.
> 
>  Despite this, ...
> 
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct 
> > cpu_policy *cp)
> >  return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> >  (ssbd   ? SPEC_CTRL_SSBD   : 0) |
> >  (psfd   ? SPEC_CTRL_PSFD   : 0) |
> > +(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> > +SPEC_CTRL_IPRED_DIS_S)
> > + : 0) |
> >  0);
> >  }
> 
>  ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
>  of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
>  description it wants clarifying why it is acceptable to run Xen with the
>  guest chosen settings for at least the DIS_S bit (assuming that it is
>  okay to do so). Likely (didn't look there yet) also applicable to the
>  further two patches.
> >>>
> >>> "The added feature is made dependent on IBRSB, which ensures it will
> >>> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> >>> ensures the value of SPEC_CTRL will get context switched on exit/entry
> >>> to guest."
> >>>
> >>> Would adding the above to the commit message clarify the intended
> >>> implementation?
> >>
> >> It would improve things, at least hinting towards there being a connection
> >> between exposure and updating on entry to Xen. I'd like to ask though to
> >> avoid "context switch" when talking about entry from guest context. While
> >> in a way technically correct, our normal meaning of the term is the
> >> process of switching vCPU-s out/in on a pCPU.
> > 
> > "The added feature is made dependent on IBRSB, which ensures it will
> > only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> > ensures the value of SPEC_CTRL will get toggled between guest and Xen
> > values on exit/entry to guest."
> > 
> > But I wonder, we already allow guests the play with other SPEC_CTRL
> > bits, and Xen toggles the SPEC_CTRL values as required on entry/exit
> > to Xen, so I'm unsure why adding more bits needs so much
> > justification.
> 
> Well, yes, I'm sorry, it was me forgetting the open-coded effect
> SC_MSR_{PV,HVM} has on exposing of the MSR. I guess I'd be happy with
> extending the last sentence a little, maybe "Note that those controls
> are not used by the hypervisor, and they're cleared on entry to Xen."
> If you're okay with that, I'd be happy to adjust while committing

Sure.

> (and assuming no other concerns are raised):
> Reviewed-by: Jan Beulich 
> for all three patches.

Thanks.

Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure

[Stewart Hildebrand]

On 1/24/24 00:00, Stewart Hildebrand wrote:
> On 1/23/24 10:07, Roger Pau Monné wrote:
>> On Tue, Jan 23, 2024 at 03:32:12PM +0100, Jan Beulich wrote:
>>> On 15.01.2024 20:43, Stewart Hildebrand wrote:
 @@ -2888,6 +2888,8 @@ int allocate_and_map_msi_pirq(struct domain *d, int 
 index, int *pirq_p,
  {
  int irq, pirq, ret;
  
 +ASSERT(pcidevs_locked() || rw_is_locked(>pci_lock));
>>>
>>> If either lock is sufficient to hold here, ...
>>>
 --- a/xen/arch/x86/physdev.c
 +++ b/xen/arch/x86/physdev.c
 @@ -123,7 +123,9 @@ int physdev_map_pirq(domid_t domid, int type, int 
 *index, int *pirq_p,
  
  case MAP_PIRQ_TYPE_MSI:
  case MAP_PIRQ_TYPE_MULTI_MSI:
 +pcidevs_lock();
  ret = allocate_and_map_msi_pirq(d, *index, pirq_p, type, msi);
 +pcidevs_unlock();
  break;
>>>
>>> ... why is it the global lock that's being acquired here?
>>>
>>
>> IIRC (Stewart can further comment) this is done holding the pcidevs
>> lock to keep the path unmodified, as there's no need to hold the
>> per-domain rwlock.
>>
> 
> Although allocate_and_map_msi_pirq() was itself acquiring the global 
> pcidevs_lock() before this patch, we could just as well use 
> read_lock(>pci_lock) here instead now. It seems like a good optimization 
> to make, so if there aren't any objections I'll change it to 
> read_lock(>pci_lock).
> 

Actually, I take this back. As mentioned in the cover letter of this series, 
and has been reiterated in recent discussions, the goal with this is to keep 
existing (non-vPCI) code paths as unmodified as possible. So I'll keep it as 
pcidevs_lock() here.

Re: [PATCH v3 13/34] xen/riscv: introduce cmpxchg.h

[Oleksii]

On Mon, 2024-01-22 at 17:27 +0100, Jan Beulich wrote:
> > +#define __xchg_acquire(ptr, new, size) \
> > +({ \
> > +    __typeof__(ptr) ptr__ = (ptr); \
> > +    __typeof__(new) new__ = (new); \
> > +    __typeof__(*(ptr)) ret__; \
> > +    switch (size) \
> > +   { \
> > +    case 4: \
> > +    asm volatile( \
> > +    "  amoswap.w %0, %2, %1\n" \
> > +    RISCV_ACQUIRE_BARRIER \
> > +    : "=r" (ret__), "+A" (*ptr__) \
> > +    : "r" (new__) \
> > +    : "memory" ); \
> > +    break; \
> > +    case 8: \
> > +    asm volatile( \
> > +    "  amoswap.d %0, %2, %1\n" \
> > +    RISCV_ACQUIRE_BARRIER \
> > +    : "=r" (ret__), "+A" (*ptr__) \
> > +    : "r" (new__) \
> > +    : "memory" ); \
> > +    break; \
> > +    default: \
> > +    ASSERT_UNREACHABLE(); \
> > +    } \
> > +    ret__; \
> > +})
> 
> If I'm not mistaken this differs from __xchg_relaxed() only in the
> use
> of RISCV_ACQUIRE_BARRIER, and ...
> 
> > +#define xchg_acquire(ptr, x) \
> > +({ \
> > +    __typeof__(*(ptr)) x_ = (x); \
> > +    (__typeof__(*(ptr))) __xchg_acquire((ptr), x_,
> > sizeof(*(ptr))); \
> > +})
> > +
> > +#define __xchg_release(ptr, new, size) \
> > +({ \
> > +    __typeof__(ptr) ptr__ = (ptr); \
> > +    __typeof__(new) new__ = (new); \
> > +    __typeof__(*(ptr)) ret__; \
> > +    switch (size) \
> > +   { \
> > +    case 4: \
> > +    asm volatile ( \
> > +    RISCV_RELEASE_BARRIER \
> > +    "  amoswap.w %0, %2, %1\n" \
> > +    : "=r" (ret__), "+A" (*ptr__) \
> > +    : "r" (new__) \
> > +    : "memory"); \
> > +    break; \
> > +    case 8: \
> > +    asm volatile ( \
> > +    RISCV_RELEASE_BARRIER \
> > +    "  amoswap.d %0, %2, %1\n" \
> > +    : "=r" (ret__), "+A" (*ptr__) \
> > +    : "r" (new__) \
> > +    : "memory"); \
> > +    break; \
> > +    default: \
> > +    ASSERT_UNREACHABLE(); \
> > +    } \
> > +    ret__; \
> > +})
> 
> this only in the use of RISCV_RELEASE_BARRIER. If so they likely want
> folding, to limit redundancy and make eventual updating easier. (Same
> for the cmpxchg helper further down, as it seems.)
Also the difference is in where to place barrier before or after atomic
instruction. I am not sure that we can easily folded this macros.

~ Oleksii

[ANNOUNCE] Agenda for next community call

[Kelly Choi]

Hi all,

Please add your proposed agenda items below.

https://cryptpad.fr/pad/#/2/pad/edit/mc3pbD9Wghw16Gq1MGyn7mbe/

If any action items are missing or have been resolved, please add/remove
them from the sheet.

*CALL LINK: https://meet.jit.si/XenProjectCommunityCall
*

*DATE: Thursday 1st February 2024*

*TIME: 1600 UTC (4 pm UK time)*
Note the following administrative conventions for the call:

* To allow time to switch between meetings, we plan on starting the
agenda at 16:05 UTC sharp.  Aim to join by 16:03 UTC if possible to allocate
time to sort out technical difficulties.

* If you want to be CC'ed please add or remove yourself from the
sign-up-sheet at
https://cryptpad.fr/pad/#/2/pad/edit/D9vGzihPxxAOe6RFPz0sRCf+/

== Dial-in Information ==
## Meeting time
16:00 - 17:00 British time
Further International meeting times:
https://www.timeanddate.com/worldclock/meetingdetails.html?year=2024=2=1=16=0=0=1234=37=224=179


## Dial in details
https://meet.jit.si/static/dialInInfo.html?room=XenProjectCommunityCall

Many thanks,
Kelly Choi

Community Manager
Xen Project

[PATCH] x86/boot: Add braces in reloc.c

[Andrew Cooper]

107 lines is an unreasonably large switch statement to live inside a
brace-less for loop.  Drop the comment that's clumsily trying to cover the
fact that this logic has wrong-looking indentation.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
---
 xen/arch/x86/boot/reloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 77fdb2be0573..4033557481e8 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -230,6 +230,7 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, 
uint32_t video_out)
 /* Put all needed data into mbi_out. */
 for ( tag = _p(ptr); (u32)tag - mbi_in < mbi_fix->total_size;
   tag = _p(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
+{
 switch ( tag->type )
 {
 case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
@@ -332,11 +333,12 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, 
uint32_t video_out)
 #endif /* CONFIG_VIDEO */
 
 case MULTIBOOT2_TAG_TYPE_END:
-goto end; /* Cannot "break;" here. */
+goto end;
 
 default:
 break;
 }
+}
 
  end:
 

base-commit: 4a7e71aa085170f1a13976507c8e248f8715f116
-- 
2.30.2

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Jan Beulich]

On 30.01.2024 15:35, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 01:59:14PM +0100, Jan Beulich wrote:
>> On 30.01.2024 13:06, Roger Pau Monné wrote:
>>> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
 On 30.01.2024 10:13, Roger Pau Monne wrote:
> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} 
> controls in
> SPEC_CTRL MSR.
>
> Note that those controls are not used by the hypervisor.

 Despite this, ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct 
> cpu_policy *cp)
>  return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>  (ssbd   ? SPEC_CTRL_SSBD   : 0) |
>  (psfd   ? SPEC_CTRL_PSFD   : 0) |
> +(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> +SPEC_CTRL_IPRED_DIS_S)
> + : 0) |
>  0);
>  }

 ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
 of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
 description it wants clarifying why it is acceptable to run Xen with the
 guest chosen settings for at least the DIS_S bit (assuming that it is
 okay to do so). Likely (didn't look there yet) also applicable to the
 further two patches.
>>>
>>> "The added feature is made dependent on IBRSB, which ensures it will
>>> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
>>> ensures the value of SPEC_CTRL will get context switched on exit/entry
>>> to guest."
>>>
>>> Would adding the above to the commit message clarify the intended
>>> implementation?
>>
>> It would improve things, at least hinting towards there being a connection
>> between exposure and updating on entry to Xen. I'd like to ask though to
>> avoid "context switch" when talking about entry from guest context. While
>> in a way technically correct, our normal meaning of the term is the
>> process of switching vCPU-s out/in on a pCPU.
> 
> "The added feature is made dependent on IBRSB, which ensures it will
> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> ensures the value of SPEC_CTRL will get toggled between guest and Xen
> values on exit/entry to guest."
> 
> But I wonder, we already allow guests the play with other SPEC_CTRL
> bits, and Xen toggles the SPEC_CTRL values as required on entry/exit
> to Xen, so I'm unsure why adding more bits needs so much
> justification.

Well, yes, I'm sorry, it was me forgetting the open-coded effect
SC_MSR_{PV,HVM} has on exposing of the MSR. I guess I'd be happy with
extending the last sentence a little, maybe "Note that those controls
are not used by the hypervisor, and they're cleared on entry to Xen."
If you're okay with that, I'd be happy to adjust while committing
(and assuming no other concerns are raised):
Reviewed-by: Jan Beulich 
for all three patches.

Jan

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 01:59:14PM +0100, Jan Beulich wrote:
> On 30.01.2024 13:06, Roger Pau Monné wrote:
> > On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
> >> On 30.01.2024 10:13, Roger Pau Monne wrote:
> >>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} 
> >>> controls in
> >>> SPEC_CTRL MSR.
> >>>
> >>> Note that those controls are not used by the hypervisor.
> >>
> >> Despite this, ...
> >>
> >>> --- a/xen/arch/x86/msr.c
> >>> +++ b/xen/arch/x86/msr.c
> >>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct 
> >>> cpu_policy *cp)
> >>>  return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> >>>  (ssbd   ? SPEC_CTRL_SSBD   : 0) |
> >>>  (psfd   ? SPEC_CTRL_PSFD   : 0) |
> >>> +(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> >>> +SPEC_CTRL_IPRED_DIS_S)
> >>> + : 0) |
> >>>  0);
> >>>  }
> >>
> >> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
> >> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
> >> description it wants clarifying why it is acceptable to run Xen with the
> >> guest chosen settings for at least the DIS_S bit (assuming that it is
> >> okay to do so). Likely (didn't look there yet) also applicable to the
> >> further two patches.
> > 
> > "The added feature is made dependent on IBRSB, which ensures it will
> > only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> > ensures the value of SPEC_CTRL will get context switched on exit/entry
> > to guest."
> > 
> > Would adding the above to the commit message clarify the intended
> > implementation?
> 
> It would improve things, at least hinting towards there being a connection
> between exposure and updating on entry to Xen. I'd like to ask though to
> avoid "context switch" when talking about entry from guest context. While
> in a way technically correct, our normal meaning of the term is the
> process of switching vCPU-s out/in on a pCPU.

"The added feature is made dependent on IBRSB, which ensures it will
only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
ensures the value of SPEC_CTRL will get toggled between guest and Xen
values on exit/entry to guest."

But I wonder, we already allow guests the play with other SPEC_CTRL
bits, and Xen toggles the SPEC_CTRL values as required on entry/exit
to Xen, so I'm unsure why adding more bits needs so much
justification.

Thanks, Roger.

Rust Xen Guest Agent 0.4.0

[Yann Dirson]

A new pre-release of our guest agent prototype written in Rust is
available, numbered 0.3.0 [1].  Identified issues and work to be done
are tracked in Gitlab issue tracker [2].

As always, feedback will be greatly appreciated!

Highlights:

### new features

* can be linked statically with libxenstore to distribute a more
   standalone binary (`-F static`).  Used for official Linux binary.
   Notably useful for guests running a RHEL-derived Linux distro.

### bugfixes

* stale network information in xenstore is now removed on startup

### other noteworthy changes

* CI pipelines stopped producing binaries for EOL'd FreeBSD 12.4,
   switched to 13.2
* CI now produces an (unofficial) binary for FreeBSD with Netlink
   support


[1] https://gitlab.com/xen-project/xen-guest-agent/-/releases/0.4.0
[2] https://gitlab.com/xen-project/xen-guest-agent/-/issues


Yann Dirson | Vates Platform Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Xen Security Advisory 449 v2 (CVE-2023-46839) - pci: phantom functions assigned to incorrect contexts

[Xen . org security team]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Xen Security Advisory CVE-2023-46839 / XSA-449
   version 2

 pci: phantom functions assigned to incorrect contexts

UPDATES IN VERSION 2


Public release.

ISSUE DESCRIPTION
=

PCI devices can make use of a functionality called phantom functions,
that when enabled allows the device to generate requests using the IDs
of functions that are otherwise unpopulated.  This allows a device to
extend the number of outstanding requests.

Such phantom functions need an IOMMU context setup, but failure to
setup the context is not fatal when the device is assigned.  Not
failing device assignment when such failure happens can lead to the
primary device being assigned to a guest, while some of the phantom
functions are assigned to a different domain.

IMPACT
==

Under certain circumstances a malicious guest assigned a PCI device
with phantom functions may be able to access memory from a previous
owner of the device.

VULNERABLE SYSTEMS
==

Systems running all version of Xen are affected.

Only x86 systems are vulnerable.  Arm systems are not vulnerable.

Only systems using PCI passthrough of devices with phantom functions
are affected.

MITIGATION
==

There is no mitigation (other than not passing through PCI devices
with phantom functions to guests).

CREDITS
===

This issue was discovered by Roger Pau Monné of XenServer.

RESOLUTION
==

Applying the appropriate attached patch resolves this issue.

Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball.  Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.

xsa449.patch   xen-unstable - Xen 4.17.x
xsa449-4.16.patch  Xen 4.16.x - Xen 4.15.x

$ sha256sum xsa449*
f77914aae8f917952f66d863d26314875ff96a0d8178f64c94b95825eabbc8a8  xsa449.patch
8f0302c24535ad4c7379469f33afcfdce08ba6db970e0ca1a1bfdd788af6fc6c  
xsa449-4.16.patch
$

DEPLOYMENT DURING EMBARGO
=

Deployment of the patches described above (or others which are
substantially similar) is permitted during the embargo, even on
public-facing systems with untrusted guest users and administrators.

HOWEVER, deployment of the mitigation is NOT permitted (except where
all the affected systems and VMs are administered and used only by
organisations which are members of the Xen Project Security Issues
Predisclosure List).  Specifically, deployment on public cloud systems
is NOT permitted.

This is because removing/replacing of pass-through devices or their
replacement by emulated devices is a guest visible configuration
change, which may lead to re-discovery of the issue.

Deployment of this mitigation is permitted only AFTER the embargo ends.

AND: Distribution of updated software is prohibited (except to other
members of the predisclosure list).

Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.

(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable.  This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)

For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
  http://www.xenproject.org/security-policy.html
-BEGIN PGP SIGNATURE-

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmW49O0MHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZqVQH/jvY8MptcxkihMhykNkRON6H5aBaY0UQKzbiCVBy
Q0g6FoE59mHIsoIYvPHFFw0BNbxgubWkJRgowRTtwxKay9HWUKo22eKaLpX9I+TX
LUo7KFE02/MRWus6mjGNdaTghC2SzGghqAcwhQcPzuaE1qS31S/iWXTe9u0hITHv
M/zswSWuZK0UaejBy55hd/+L554yZ976coSFGyjqqIuSHvkR6+NFCzTSLp3GHsue
5CI3ouW0fR2aQ/Gu3pXBPgG464rQ9rQptsFW11uZ1Ahw9T4ZYQis9cRNNsM5I+f8
paGiJO2+y9oYoMkKRrkHXVwkhmZJbFzvpq0e4VkgHwZxbIc=
=L484
-END PGP SIGNATURE-


xsa449.patch
Description: Binary data


xsa449-4.16.patch
Description: Binary data

Xen Security Advisory 450 v2 (CVE-2023-46840) - VT-d: Failure to quarantine devices in !HVM builds

[Xen . org security team]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Xen Security Advisory CVE-2023-46840 / XSA-450
   version 2

   VT-d: Failure to quarantine devices in !HVM builds

UPDATES IN VERSION 2


Public release.

ISSUE DESCRIPTION
=

Incorrect placement of a preprocessor directive in source code results
in logic that doesn't operate as intended when support for HVM guests is
compiled out of Xen.

IMPACT
==

When a device is removed from a domain, it is not properly quarantined
and retains its access to the domain to which it was previously
assigned.

VULNERABLE SYSTEMS
==

Xen 4.17 and onwards are vulnerable.  Xen 4.16 and older are not
vulnerable.

Only Xen running on x86 platforms with an Intel-compatible VT-d IOMMU is
vulnerable.  Platforms from other manufacturers, or platforms without a
VT-d IOMMU are not vulnerable.

Only systems where PCI devices are passed through to untrusted or
semi-trusted guests are vulnerable.  Systems which do not assign PCI
devices to untrusted guests are not vulnerable.

Xen is only vulnerable when CONFIG_HVM is disabled at build time.  Most
deployments of Xen are expected to have CONFIG_HVM enabled at build
time, and would therefore not be vulnerable.

MITIGATION
==

There is no mitigation.

CREDITS
===

This issue was discovered by Teddy Astie of Vates

RESOLUTION
==

Applying the attached patch resolves this issue.

Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball.  Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.

xsa450.patch   xen-unstable - Xen 4.17.x

$ sha256sum xsa450*
738c79b92ab5ea57f446df3daff6564727fea5feebf8fadeb32acd0cf06ff9fb  xsa450.patch
$

DEPLOYMENT DURING EMBARGO
=

Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.

But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).

Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.


(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable.  This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)

For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
  http://www.xenproject.org/security-policy.html
-BEGIN PGP SIGNATURE-

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmW49MwMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZnwcIALs07CQFYSuQmdgWRYeepkjehMSVhPhvJcYBCFWU
p+80oreGP2pC1LN+T9ndN0kDeUHAO8PeT+XqxHSNfT16Q5EOSeLpUQ8m+UfHUFLU
vtPMjR4sMpnvuZfx0OCMJctDDTM+/muw4AH0BO2zxFfDzGkM96zZ6vAokeer+5HQ
/P9usMm/6jphixVq919RBJ78fFZxKpKhil9tEwNuD6HJW3VNMWp1ypGNyFI3iRhw
XpYzWMB0eW6B6rSInohHJiTS7P6KE5zeXeBPZ5yVHy2J3e3c7nXyrQaaONSRCBdm
/Px2xcg1SpH+3UwoT56Z7tj1DhlgjcY4peb5B58oDK68hMU=
=Dp+G
-END PGP SIGNATURE-


xsa450.patch
Description: Binary data

Re: [PATCH v3 14/29] target/i386: Prefer fast cpu_env() over slower CPU QOM cast macro

[Igor Mammedov]

On Mon, 29 Jan 2024 17:44:56 +0100
Philippe Mathieu-Daudé  wrote:

> Mechanical patch produced running the command documented
> in scripts/coccinelle/cpu_env.cocci_template header.


commenting here since, I'm not expert on coccinelle scripts.

On negative side we are permanently loosing type checking in this area.
Is it worth it, what gains do we get with this series?

Side note,
QOM cast expenses you are replacing could be negated by disabling
CONFIG_QOM_CAST_DEBUG without killing type check code when it's enabled.
That way you will speed up not only cpuenv access but also all other casts
across the board.

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
...
>  static inline void vmx_clear_nmi_blocking(CPUState *cpu)
>  {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -CPUX86State *env = _cpu->env;
> -
> -env->hflags2 &= ~HF2_NMI_MASK;

> +cpu_env(cpu)->hflags2 &= ~HF2_NMI_MASK;

this style of de-referencing return value of macro/function
was discouraged in past and preferred way was 'Foo f = CAST(me); f->some_access

(it's just imprint speaking, I don't recall where it comes from)

>  uint32_t gi = (uint32_t) rvmcs(cpu->accel->fd, 
> VMCS_GUEST_INTERRUPTIBILITY);
>  gi &= ~VMCS_INTERRUPTIBILITY_NMI_BLOCKING;
>  wvmcs(cpu->accel->fd, VMCS_GUEST_INTERRUPTIBILITY, gi);
> @@ -207,10 +203,7 @@ static inline void vmx_clear_nmi_blocking(CPUState *cpu)
>  
>  static inline void vmx_set_nmi_blocking(CPUState *cpu)
>  {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -CPUX86State *env = _cpu->env;
> -
> -env->hflags2 |= HF2_NMI_MASK;
> +cpu_env(cpu)->hflags2 |= HF2_NMI_MASK;
>  uint32_t gi = (uint32_t)rvmcs(cpu->accel->fd, 
> VMCS_GUEST_INTERRUPTIBILITY);
>  gi |= VMCS_INTERRUPTIBILITY_NMI_BLOCKING;
>  wvmcs(cpu->accel->fd, VMCS_GUEST_INTERRUPTIBILITY, gi);
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 7362daa45a..5239cd40fa 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -155,8 +155,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
>  
>  void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg)
>  {
> -X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(ms->possible_cpus->cpus[0].cpu);
>  uint32_t unused, ebx, ecx, edx;
>  uint64_t feature_control_bits = 0;
>  uint64_t *val;
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index a8d014d09a..f292a14a15 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -74,8 +74,7 @@ struct VMMouseState {
>  
>  static void vmmouse_get_data(uint32_t *data)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
>  data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
> @@ -84,8 +83,7 @@ static void vmmouse_get_data(uint32_t *data)
>  
>  static void vmmouse_set_data(const uint32_t *data)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  env->regs[R_EAX] = data[0]; env->regs[R_EBX] = data[1];
>  env->regs[R_ECX] = data[2]; env->regs[R_EDX] = data[3];
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index f42621e674..61e5060117 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -487,8 +487,7 @@ static void regs_to_cpu(vmware_regs_t *vmport_regs, 
> ioreq_t *req)
>  
>  static void regs_from_cpu(vmware_regs_t *vmport_regs)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  vmport_regs->ebx = env->regs[R_EBX];
>  vmport_regs->ecx = env->regs[R_ECX];
> diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> index c290910a04..8939ff9fa9 100644
> --- a/target/i386/arch_dump.c
> +++ b/target/i386/arch_dump.c
> @@ -203,7 +203,6 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>  int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>   int cpuid, DumpState *s)
>  {
> -X86CPU *cpu = X86_CPU(cs);
>  x86_elf_prstatus prstatus;
>  Elf32_Nhdr *note;
>  char *buf;
> @@ -211,7 +210,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>  const char *name = "CORE";
>  int ret;
>  
> -x86_fill_elf_prstatus(, >env, cpuid);
> +x86_fill_elf_prstatus(, cpu_env(cs), cpuid);
>  descsz = sizeof(x86_elf_prstatus);
>  note_size = ELF_NOTE_SIZE(sizeof(Elf32_Nhdr), name_size, descsz);
>  note = g_malloc0(note_size);
> @@ -381,17 +380,13 @@ static inline int 
> cpu_write_qemu_note(WriteCoreDumpFunction f,
>  int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cs,
>   DumpState *s)
>  {
> -X86CPU *cpu = X86_CPU(cs);
> -
> -return cpu_write_qemu_note(f, >env, s, 1);
> +

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Jan Beulich]

On 30.01.2024 13:06, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
>> On 30.01.2024 10:13, Roger Pau Monne wrote:
>>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls 
>>> in
>>> SPEC_CTRL MSR.
>>>
>>> Note that those controls are not used by the hypervisor.
>>
>> Despite this, ...
>>
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct 
>>> cpu_policy *cp)
>>>  return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>>>  (ssbd   ? SPEC_CTRL_SSBD   : 0) |
>>>  (psfd   ? SPEC_CTRL_PSFD   : 0) |
>>> +(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
>>> +SPEC_CTRL_IPRED_DIS_S)
>>> + : 0) |
>>>  0);
>>>  }
>>
>> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
>> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
>> description it wants clarifying why it is acceptable to run Xen with the
>> guest chosen settings for at least the DIS_S bit (assuming that it is
>> okay to do so). Likely (didn't look there yet) also applicable to the
>> further two patches.
> 
> "The added feature is made dependent on IBRSB, which ensures it will
> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> ensures the value of SPEC_CTRL will get context switched on exit/entry
> to guest."
> 
> Would adding the above to the commit message clarify the intended
> implementation?

It would improve things, at least hinting towards there being a connection
between exposure and updating on entry to Xen. I'd like to ask though to
avoid "context switch" when talking about entry from guest context. While
in a way technically correct, our normal meaning of the term is the
process of switching vCPU-s out/in on a pCPU.

Jan

[linux-5.4 test] 184523: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-credit1   8 xen-boot fail in 184507 pass in 184523
 test-armhf-armhf-xl-rtds 14 guest-startfail pass in 184507
 test-armhf-armhf-xl-credit2  14 guest-startfail pass in 184507

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 184507 like 
184362
 test-armhf-armhf-xl-vhd  13 guest-start fail in 184507 like 184381
 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeat fail in 184507 like 
184381
 test-armhf-armhf-xl-rtds15 migrate-support-check fail in 184507 never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-check fail in 184507 never pass
 test-armhf-armhf-xl-credit2 15 migrate-support-check fail in 184507 never pass
 test-armhf-armhf-xl-credit2 16 saverestore-support-check fail in 184507 never 
pass
 test-armhf-armhf-xl-credit1  14 guest-start  fail  like 184370
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 184381
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184381
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184381
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184381
 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeatfail like 184381
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184381
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184381
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184381
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184381
 test-arm64-arm64-libvirt-raw 17 guest-start/debian.repeatfail  like 184381
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 184381
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 184381
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184381
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184381
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  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-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 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-amd64-amd64-libvirt 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-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-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-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-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Jan Beulich]

On 30.01.2024 12:46, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
>> On 30.01.2024 11:27, Roger Pau Monne wrote:
>>> Dummy set/clear tests for additional spec_ctrl bits.
>>> ---
>>>  docs/all-tests.dox  |   2 +
>>>  tests/test/Makefile |   9 
>>>  tests/test/main.c   | 100 
>>>  3 files changed, 111 insertions(+)
>>>  create mode 100644 tests/test/Makefile
>>>  create mode 100644 tests/test/main.c
>>
>> I'm puzzled: Why "test"? That doesn't describe in any way what this test
>> is about.
> 
> That's just my place holder for random XTF stuff.  I don't intend this
> to be committed.

Could have been said then one way or another.

Jan

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
> On 30.01.2024 10:13, Roger Pau Monne wrote:
> > The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls 
> > in
> > SPEC_CTRL MSR.
> > 
> > Note that those controls are not used by the hypervisor.
> 
> Despite this, ...
> 
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct 
> > cpu_policy *cp)
> >  return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> >  (ssbd   ? SPEC_CTRL_SSBD   : 0) |
> >  (psfd   ? SPEC_CTRL_PSFD   : 0) |
> > +(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> > +SPEC_CTRL_IPRED_DIS_S)
> > + : 0) |
> >  0);
> >  }
> 
> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
> description it wants clarifying why it is acceptable to run Xen with the
> guest chosen settings for at least the DIS_S bit (assuming that it is
> okay to do so). Likely (didn't look there yet) also applicable to the
> further two patches.

"The added feature is made dependent on IBRSB, which ensures it will
only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
ensures the value of SPEC_CTRL will get context switched on exit/entry
to guest."

Would adding the above to the commit message clarify the intended
implementation?

Thanks, Roger.

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
> On 30.01.2024 11:27, Roger Pau Monne wrote:
> > Dummy set/clear tests for additional spec_ctrl bits.
> > ---
> >  docs/all-tests.dox  |   2 +
> >  tests/test/Makefile |   9 
> >  tests/test/main.c   | 100 
> >  3 files changed, 111 insertions(+)
> >  create mode 100644 tests/test/Makefile
> >  create mode 100644 tests/test/main.c
> 
> I'm puzzled: Why "test"? That doesn't describe in any way what this test
> is about.

That's just my place holder for random XTF stuff.  I don't intend this
to be committed.

> > --- /dev/null
> > +++ b/tests/test/Makefile
> > @@ -0,0 +1,9 @@
> > +include $(ROOT)/build/common.mk
> > +
> > +NAME  := test
> > +CATEGORY  := utility
> > +TEST-ENVS := hvm32 pv64
> 
> Any reason for this limitation?

Just wanted a PV and an HVM context.

> > --- /dev/null
> > +++ b/tests/test/main.c
> > @@ -0,0 +1,100 @@
> > +/**
> > + * @file tests/test/main.c
> > + * @ref test-test
> > + *
> > + * @page test-test test
> > + *
> > + * @todo Docs for test-test
> > + *
> > + * @see tests/test/main.c
> > + */
> > +#include 
> > +
> > +#define MSR_SPEC_CTRL   0x0048
> > +#define  SPEC_CTRL_IPRED_DIS_U  (_AC(1, ULL) <<  3)
> > +#define  SPEC_CTRL_IPRED_DIS_S  (_AC(1, ULL) <<  4)
> > +#define  SPEC_CTRL_RRSBA_DIS_U  (_AC(1, ULL) <<  5)
> > +#define  SPEC_CTRL_RRSBA_DIS_S  (_AC(1, ULL) <<  6)
> > +#define  SPEC_CTRL_DDP_DIS_U(_AC(1, ULL) <<  8)
> > +#define  SPEC_CTRL_BHI_DIS_S(_AC(1, ULL) << 10)
> > +
> > +const char test_title[] = "SPEC_CTRL";
> > +
> > +static void update_spec_ctrl(uint64_t mask, bool set)
> > +{
> > +uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> > +
> > +if ( set )
> > +spec_ctrl |= mask;
> > +else
> > +spec_ctrl &= ~mask;
> > +
> > +wrmsr(MSR_SPEC_CTRL, spec_ctrl);
> > +}
> > +
> > +static void assert_spec_ctrl(uint64_t mask, bool set)
> > +{
> > +uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> > +
> > +if ( (spec_ctrl & mask) != (set ? mask : 0) )
> > +{
> > +xtf_failure("SPEC_CTRL expected: %#" PRIx64 " got: %#" PRIx64 "\n",
> > +set ? (spec_ctrl | mask) : (spec_ctrl & ~mask),
> > +spec_ctrl);
> > +xtf_exit();
> > +}
> > +}
> > +
> > +static void test_loop(uint64_t mask)
> > +{
> > +update_spec_ctrl(mask, true);
> > +assert_spec_ctrl(mask, true);
> > +/* Ensure context switch to Xen. */
> > +hypercall_yield();
> 
> I'm afraid yielding doesn't guarantee context switching in Xen,

It will ensure a vmexit/trap, which is what I was after here.  Maybe the
comment should be "Trap into Xen." or some such.  It wasn't about
ensuring VM context switching.

Thanks, Roger.

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Jan Beulich]

On 30.01.2024 10:13, Roger Pau Monne wrote:
> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
> SPEC_CTRL MSR.
> 
> Note that those controls are not used by the hypervisor.

Despite this, ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy 
> *cp)
>  return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>  (ssbd   ? SPEC_CTRL_SSBD   : 0) |
>  (psfd   ? SPEC_CTRL_PSFD   : 0) |
> +(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> +SPEC_CTRL_IPRED_DIS_S)
> + : 0) |
>  0);
>  }

... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
description it wants clarifying why it is acceptable to run Xen with the
guest chosen settings for at least the DIS_S bit (assuming that it is
okay to do so). Likely (didn't look there yet) also applicable to the
further two patches.

Jan

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Jan Beulich]

On 30.01.2024 11:27, Roger Pau Monne wrote:
> Dummy set/clear tests for additional spec_ctrl bits.
> ---
>  docs/all-tests.dox  |   2 +
>  tests/test/Makefile |   9 
>  tests/test/main.c   | 100 
>  3 files changed, 111 insertions(+)
>  create mode 100644 tests/test/Makefile
>  create mode 100644 tests/test/main.c

I'm puzzled: Why "test"? That doesn't describe in any way what this test
is about.

> --- /dev/null
> +++ b/tests/test/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME  := test
> +CATEGORY  := utility
> +TEST-ENVS := hvm32 pv64

Any reason for this limitation?

> --- /dev/null
> +++ b/tests/test/main.c
> @@ -0,0 +1,100 @@
> +/**
> + * @file tests/test/main.c
> + * @ref test-test
> + *
> + * @page test-test test
> + *
> + * @todo Docs for test-test
> + *
> + * @see tests/test/main.c
> + */
> +#include 
> +
> +#define MSR_SPEC_CTRL   0x0048
> +#define  SPEC_CTRL_IPRED_DIS_U  (_AC(1, ULL) <<  3)
> +#define  SPEC_CTRL_IPRED_DIS_S  (_AC(1, ULL) <<  4)
> +#define  SPEC_CTRL_RRSBA_DIS_U  (_AC(1, ULL) <<  5)
> +#define  SPEC_CTRL_RRSBA_DIS_S  (_AC(1, ULL) <<  6)
> +#define  SPEC_CTRL_DDP_DIS_U(_AC(1, ULL) <<  8)
> +#define  SPEC_CTRL_BHI_DIS_S(_AC(1, ULL) << 10)
> +
> +const char test_title[] = "SPEC_CTRL";
> +
> +static void update_spec_ctrl(uint64_t mask, bool set)
> +{
> +uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> +
> +if ( set )
> +spec_ctrl |= mask;
> +else
> +spec_ctrl &= ~mask;
> +
> +wrmsr(MSR_SPEC_CTRL, spec_ctrl);
> +}
> +
> +static void assert_spec_ctrl(uint64_t mask, bool set)
> +{
> +uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> +
> +if ( (spec_ctrl & mask) != (set ? mask : 0) )
> +{
> +xtf_failure("SPEC_CTRL expected: %#" PRIx64 " got: %#" PRIx64 "\n",
> +set ? (spec_ctrl | mask) : (spec_ctrl & ~mask),
> +spec_ctrl);
> +xtf_exit();
> +}
> +}
> +
> +static void test_loop(uint64_t mask)
> +{
> +update_spec_ctrl(mask, true);
> +assert_spec_ctrl(mask, true);
> +/* Ensure context switch to Xen. */
> +hypercall_yield();

I'm afraid yielding doesn't guarantee context switching in Xen, if the
system (or even just the one CPU) is otherwise idle. Hence at the very
least please don't say "ensure" in the comment. But perhaps more
reliable to e.g. use "poll" with a timeout. While I didn't post that
addition, I've used such for testing my vCPU-area-registration work:

struct sched_poll poll = { .timeout = s + SECONDS(1) };
rc = hypercall_sched_op(SCHEDOP_poll, );
if ( rc )
xtf_error("Could not poll (%d)\n", rc);

(there also to ensure enough time passes for the time area to be
updated).

I actually found this to have another neat side effect: The guest then
can't go away so quickly that "xl console" doesn't manage to attach to
the guest (which otherwise I observe to work only about every other
time).

Jan

[PATCH] XTF: tests SPEC_CTRL added bits

[Roger Pau Monne]

Dummy set/clear tests for additional spec_ctrl bits.
---
 docs/all-tests.dox  |   2 +
 tests/test/Makefile |   9 
 tests/test/main.c   | 100 
 3 files changed, 111 insertions(+)
 create mode 100644 tests/test/Makefile
 create mode 100644 tests/test/main.c

diff --git a/docs/all-tests.dox b/docs/all-tests.dox
index 892a9e474743..5a66ac252ea5 100644
--- a/docs/all-tests.dox
+++ b/docs/all-tests.dox
@@ -187,3 +187,5 @@ states.
 
 @subpage test-nested-vmx - Nested VT-x tests.
 */
+# Placeholder: Merge into the appropriate location above
+@subpage test-test - @todo title
diff --git a/tests/test/Makefile b/tests/test/Makefile
new file mode 100644
index ..19bc4b6a4639
--- /dev/null
+++ b/tests/test/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME  := test
+CATEGORY  := utility
+TEST-ENVS := hvm32 pv64
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/test/main.c b/tests/test/main.c
new file mode 100644
index ..9a25e95d91b7
--- /dev/null
+++ b/tests/test/main.c
@@ -0,0 +1,100 @@
+/**
+ * @file tests/test/main.c
+ * @ref test-test
+ *
+ * @page test-test test
+ *
+ * @todo Docs for test-test
+ *
+ * @see tests/test/main.c
+ */
+#include 
+
+#define MSR_SPEC_CTRL   0x0048
+#define  SPEC_CTRL_IPRED_DIS_U  (_AC(1, ULL) <<  3)
+#define  SPEC_CTRL_IPRED_DIS_S  (_AC(1, ULL) <<  4)
+#define  SPEC_CTRL_RRSBA_DIS_U  (_AC(1, ULL) <<  5)
+#define  SPEC_CTRL_RRSBA_DIS_S  (_AC(1, ULL) <<  6)
+#define  SPEC_CTRL_DDP_DIS_U(_AC(1, ULL) <<  8)
+#define  SPEC_CTRL_BHI_DIS_S(_AC(1, ULL) << 10)
+
+const char test_title[] = "SPEC_CTRL";
+
+static void update_spec_ctrl(uint64_t mask, bool set)
+{
+uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
+
+if ( set )
+spec_ctrl |= mask;
+else
+spec_ctrl &= ~mask;
+
+wrmsr(MSR_SPEC_CTRL, spec_ctrl);
+}
+
+static void assert_spec_ctrl(uint64_t mask, bool set)
+{
+uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
+
+if ( (spec_ctrl & mask) != (set ? mask : 0) )
+{
+xtf_failure("SPEC_CTRL expected: %#" PRIx64 " got: %#" PRIx64 "\n",
+set ? (spec_ctrl | mask) : (spec_ctrl & ~mask),
+spec_ctrl);
+xtf_exit();
+}
+}
+
+static void test_loop(uint64_t mask)
+{
+update_spec_ctrl(mask, true);
+assert_spec_ctrl(mask, true);
+/* Ensure context switch to Xen. */
+hypercall_yield();
+assert_spec_ctrl(mask, true);
+
+update_spec_ctrl(mask, false);
+assert_spec_ctrl(mask, false);
+/* Ensure context switch to Xen. */
+hypercall_yield();
+assert_spec_ctrl(mask, false);
+}
+
+void test_main(void)
+{
+static const struct {
+const char *name;
+unsigned int feat;
+uint64_t mask;
+} tests[] = {
+{ "IPRED CTRL", 1, SPEC_CTRL_IPRED_DIS_U | SPEC_CTRL_IPRED_DIS_S },
+{ "RRSBA CTRL", 2, SPEC_CTRL_RRSBA_DIS_U | SPEC_CTRL_RRSBA_DIS_S },
+{ "DDP DIS", 3, SPEC_CTRL_DDP_DIS_U },
+{ "BHI DIS", 4, SPEC_CTRL_BHI_DIS_S },
+};
+unsigned int i;
+uint32_t regs[4];
+
+cpuid_count(7, 2, [0], [1], [2], [3]);
+
+for ( i = 0; i < ARRAY_SIZE(tests); i++ )
+{
+if ( !test_bit(tests[i].feat, [3]) )
+continue;
+
+printk("Testing %s\n", tests[i].name);
+test_loop(tests[i].mask);
+}
+
+xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.43.0

Re: [PATCH v3 6/6] hw/xen: convert stderr prints to error/warn reports

[Alex Bennée]

Manos Pitsidianakis  writes:

> According to the QEMU Coding Style document:
>
>> Do not use printf(), fprintf() or monitor_printf(). Instead, use
>> error_report() or error_vreport() from error-report.h. This ensures the
>> error is reported in the right place (current monitor or stderr), and in
>> a uniform format.
>> Use error_printf() & friends to print additional information.
>
> This commit changes fprintfs that report warnings and errors to the
> appropriate report functions.
>
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Manos Pitsidianakis 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 5/6] hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints

[Alex Bennée]

Manos Pitsidianakis  writes:

> Tracing DPRINTFs to stderr might not be desired. A developer that relies
> on tracepoints should be able to opt-in to each tracepoint and rely on
> QEMU's log redirection, instead of stderr by default.
>
> This commit converts DPRINTFs in this file that are used for tracing
> into tracepoints.
>
> Signed-off-by: Manos Pitsidianakis 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 4/6] hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints

[Alex Bennée]

Manos Pitsidianakis  writes:

> Tracing DPRINTFs to stderr might not be desired. A developer that relies
> on tracepoints should be able to opt-in to each tracepoint and rely on
> QEMU's log redirection, instead of stderr by default.
>
> This commit converts DPRINTFs in this file that are used for tracing
> into tracepoints.
>
> Signed-off-by: Manos Pitsidianakis 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 2/2] xenpm: Print message for disabled commands

[Jan Beulich]

On 25.01.2024 19:14, Jason Andryuk wrote:
> xenpm get-cpufreq-states currently just prints no output when cpufreq is
> disabled or HWP is running.  Have it print an appropriate message.  The
> cpufreq disabled one mirrors the cpuidle disabled one.
> 
> cpufreq disabled:
> $ xenpm get-cpufreq-states
> Either Xen cpufreq is disabled or no valid information is registered!
> 
> Under HWP:
> $ xenpm get-cpufreq-states
> P-State information not supported.  Try 'get-cpufreq-average' or 'start'.
> 
> Also allow xenpm to handle EOPNOTSUPP from the pmstat hypercalls.
> EOPNOTSUPP is returned when HWP is active in some cases and allows the
> differentiation from cpufreq being disabled.
> 
> Signed-off-by: Jason Andryuk 

Acked-by: Jan Beulich 
on the assumption that really xenpm also ought to be listed in "X86
ARCHITECTURE" in ./MAINTAINERS (and be CONFIG_X86-only in the respective
Makefile).

Jan

Re: [PATCH 1/2] x86: Remove gdbstub

[Jan Beulich]

On 26.01.2024 21:54, Andrew Cooper wrote:
> In 13y of working on Xen, I've never seen seen it used.  The implementation
> was introduced (commit b69f92f3012e, Jul 28 2004) with known issues such as:
> 
>   /* Resuming after we've stopped used to work, but more through luck
>  than any actual intention.  It doesn't at the moment. */

As mentioned elsewhere, this alone might constitute a valid use of gdb.
Hence I'm a little hesitant here, but given no indication of even this
narrow use case having been employed by anyone at any time ...

> which appear to have gone unfixed for the 20 years since.
> 
> Nowadays there are more robust ways of inspecting crashed state, such as a
> kexec crash kernel, or running Xen in a VM.
> 
> This will allow us to clean up some hooks around the codebase which are
> proving awkward for other tasks.
> 
> Signed-off-by: Andrew Cooper 

Acked-by: Jan Beulich 

Jan

Re: [PATCH 2/2] xen: Remove debugger.h

[Jan Beulich]

On 26.01.2024 21:54, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/bug.h
> +++ b/xen/arch/x86/include/asm/bug.h
> @@ -1,28 +1,8 @@
>  #ifndef __X86_BUG_H__
>  #define __X86_BUG_H__
>  
> -/*
> - * Please do not include in the header any header that might
> - * use BUG/ASSERT/etc maros asthey will be defined later after
> - * the return to  from the current header:
> - * 
> - * :
> - *  ...
> - *   :
> - * ...
> - * 
> - * ...
> - *  ...
> - *  #define BUG() ...
> - *  ...
> - *  #define ASSERT() ...
> - *  ...
> - */
> -

Leaving aside the clumsiness of the initial text, other than in common/bug.c
I don't see why this comment is to go away (and, if at all, in this very
patch). With it retained or the removal suitably explained
Acked-by: Jan Beulich 

Jan

>  #ifndef __ASSEMBLY__
>  
> -#define BUG_DEBUGGER_TRAP_FATAL(regs) debugger_trap_fatal(X86_EXC_GP,regs)
> -
>  #define BUG_INSTR   "ud2"
>  #define BUG_ASM_CONST   "c"
>  

[PATCH 0/3] x86/intel: expose additional SPEC_CTRL MSR controls

[Roger Pau Monne]

Hello,

Introduce support for exposing {IPRED,RRSBA,BHI}_CTRL feature bits and
allow setting the corresponding SPEC_CTRL MSR fields.

The bits are documented in:

https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/branch-history-injection.html

Xen doesn't use the bits itself.

Thanks, Roger.

Roger Pau Monne (3):
  x86/intel: expose IPRED_CTRL to guests
  x86/intel: expose RRSBA_CTRL to guests
  x86/intel: expose BHI_CTRL to guests

 xen/arch/x86/msr.c  | 7 +++
 xen/include/public/arch-x86/cpufeatureset.h | 6 +++---
 xen/tools/gen-cpuid.py  | 3 ++-
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monne]

The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
SPEC_CTRL MSR.

Note that those controls are not used by the hypervisor.

Signed-off-by: Roger Pau Monné 
---
 xen/arch/x86/msr.c  | 3 +++
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 xen/tools/gen-cpuid.py  | 3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index c33dc78cd8f6..d500f87a5fd1 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy 
*cp)
 return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
 (ssbd   ? SPEC_CTRL_SSBD   : 0) |
 (psfd   ? SPEC_CTRL_PSFD   : 0) |
+(cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
+SPEC_CTRL_IPRED_DIS_S)
+ : 0) |
 0);
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index c897c2040136..e586e141c329 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -302,7 +302,7 @@ XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /*   Protected 
Processor Inventory
 
 /* Intel-defined CPU features, CPUID level 0x0007:2.edx, word 13 */
 XEN_CPUFEATURE(INTEL_PSFD, 13*32+ 0) /*A  MSR_SPEC_CTRL.PSFD */
-XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /*   MSR_SPEC_CTRL.IPRED_DIS_* */
+XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /*A  MSR_SPEC_CTRL.IPRED_DIS_* */
 XEN_CPUFEATURE(RRSBA_CTRL, 13*32+ 2) /*   MSR_SPEC_CTRL.RRSBA_DIS_* */
 XEN_CPUFEATURE(DDP_CTRL,   13*32+ 3) /*   MSR_SPEC_CTRL.DDP_DIS_U */
 XEN_CPUFEATURE(BHI_CTRL,   13*32+ 4) /*   MSR_SPEC_CTRL.BHI_DIS_S */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index df5222a3cdd0..45fab5e75d1c 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -319,7 +319,8 @@ def crunch_numbers(state):
 # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
 # as dependent features simplifies Xen's logic, and prevents the guest
 # from seeing implausible configurations.
-IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],
+IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS,
+IPRED_CTRL],
 IBRS: [AMD_STIBP, AMD_SSBD, PSFD, AUTO_IBRS,
IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
 IBPB: [IBPB_RET, SBPB, IBPB_BRTYPE],
-- 
2.43.0

[PATCH 3/3] x86/intel: expose BHI_CTRL to guests

[Roger Pau Monne]

The CPUID feature bit signals the presence of the BHI_DIS_S control in
SPEC_CTRL MSR.

Note that those controls are not used by the hypervisor.

Signed-off-by: Roger Pau Monné 
---
 xen/arch/x86/msr.c  | 1 +
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 xen/tools/gen-cpuid.py  | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index b3b4f75c021a..e0d57bce40ec 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -330,6 +330,7 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy 
*cp)
 (cp->feat.rrsba_ctrl ? (SPEC_CTRL_RRSBA_DIS_U |
 SPEC_CTRL_RRSBA_DIS_S)
  : 0) |
+(cp->feat.bhi_ctrl   ? SPEC_CTRL_BHI_DIS_S : 0) |
 0);
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index bade4edab30c..be5c1b748e27 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -305,7 +305,7 @@ XEN_CPUFEATURE(INTEL_PSFD, 13*32+ 0) /*A  
MSR_SPEC_CTRL.PSFD */
 XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /*A  MSR_SPEC_CTRL.IPRED_DIS_* */
 XEN_CPUFEATURE(RRSBA_CTRL, 13*32+ 2) /*A  MSR_SPEC_CTRL.RRSBA_DIS_* */
 XEN_CPUFEATURE(DDP_CTRL,   13*32+ 3) /*   MSR_SPEC_CTRL.DDP_DIS_U */
-XEN_CPUFEATURE(BHI_CTRL,   13*32+ 4) /*   MSR_SPEC_CTRL.BHI_DIS_S */
+XEN_CPUFEATURE(BHI_CTRL,   13*32+ 4) /*A  MSR_SPEC_CTRL.BHI_DIS_S */
 XEN_CPUFEATURE(MCDT_NO,13*32+ 5) /*A  MCDT_NO */
 
 /* Intel-defined CPU features, CPUID level 0x0007:1.ecx, word 14 */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 1c6d76244177..25d329ce486f 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -320,7 +320,7 @@ def crunch_numbers(state):
 # as dependent features simplifies Xen's logic, and prevents the guest
 # from seeing implausible configurations.
 IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS,
-IPRED_CTRL, RRSBA_CTRL],
+IPRED_CTRL, RRSBA_CTRL, BHI_CTRL],
 IBRS: [AMD_STIBP, AMD_SSBD, PSFD, AUTO_IBRS,
IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
 IBPB: [IBPB_RET, SBPB, IBPB_BRTYPE],
-- 
2.43.0

[PATCH 2/3] x86/intel: expose RRSBA_CTRL to guests

[Roger Pau Monne]

The CPUID feature bit signals the presence of the RRSBA_DIS_{U,S} controls in
SPEC_CTRL MSR.

Note that those controls are not used by the hypervisor.

Signed-off-by: Roger Pau Monné 
---
 xen/arch/x86/msr.c  | 3 +++
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 xen/tools/gen-cpuid.py  | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index d500f87a5fd1..b3b4f75c021a 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -327,6 +327,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy 
*cp)
 (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
 SPEC_CTRL_IPRED_DIS_S)
  : 0) |
+(cp->feat.rrsba_ctrl ? (SPEC_CTRL_RRSBA_DIS_U |
+SPEC_CTRL_RRSBA_DIS_S)
+ : 0) |
 0);
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index e586e141c329..bade4edab30c 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -303,7 +303,7 @@ XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /*   Protected 
Processor Inventory
 /* Intel-defined CPU features, CPUID level 0x0007:2.edx, word 13 */
 XEN_CPUFEATURE(INTEL_PSFD, 13*32+ 0) /*A  MSR_SPEC_CTRL.PSFD */
 XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /*A  MSR_SPEC_CTRL.IPRED_DIS_* */
-XEN_CPUFEATURE(RRSBA_CTRL, 13*32+ 2) /*   MSR_SPEC_CTRL.RRSBA_DIS_* */
+XEN_CPUFEATURE(RRSBA_CTRL, 13*32+ 2) /*A  MSR_SPEC_CTRL.RRSBA_DIS_* */
 XEN_CPUFEATURE(DDP_CTRL,   13*32+ 3) /*   MSR_SPEC_CTRL.DDP_DIS_U */
 XEN_CPUFEATURE(BHI_CTRL,   13*32+ 4) /*   MSR_SPEC_CTRL.BHI_DIS_S */
 XEN_CPUFEATURE(MCDT_NO,13*32+ 5) /*A  MCDT_NO */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 45fab5e75d1c..1c6d76244177 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -320,7 +320,7 @@ def crunch_numbers(state):
 # as dependent features simplifies Xen's logic, and prevents the guest
 # from seeing implausible configurations.
 IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS,
-IPRED_CTRL],
+IPRED_CTRL, RRSBA_CTRL],
 IBRS: [AMD_STIBP, AMD_SSBD, PSFD, AUTO_IBRS,
IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
 IBPB: [IBPB_RET, SBPB, IBPB_BRTYPE],
-- 
2.43.0

Re: [PATCH v6 00/15] Arm cache coloring

[Michal Orzel]

Hi Carlo,

On 29/01/2024 18:17, Carlo Nonato wrote:
> 
> 
> Shared caches in multi-core CPU architectures represent a problem for
> predictability of memory access latency. This jeopardizes applicability
> of many Arm platform in real-time critical and mixed-criticality
> scenarios. We introduce support for cache partitioning with page
> coloring, a transparent software technique that enables isolation
> between domains and Xen, and thus avoids cache interference.
> 
> When creating a domain, a simple syntax (e.g. `0-3` or `4-11`) allows
> the user to define assignments of cache partitions ids, called colors,
> where assigning different colors guarantees no mutual eviction on cache
> will ever happen. This instructs the Xen memory allocator to provide
> the i-th color assignee only with pages that maps to color i, i.e. that
> are indexed in the i-th cache partition.
> 
> The proposed implementation supports the dom0less feature.
> The proposed implementation doesn't support the static-mem feature.
> The solution has been tested in several scenarios, including Xilinx Zynq
> MPSoCs.
> 
> Open points:
> - Michal found some problem here
> https://patchew.org/Xen/20230123154735.74832-1-carlo.non...@minervasys.tech/20230123154735.74832-4-carlo.non...@minervasys.tech/#a7a06a26-ae79-402c-96a4-a1ebfe8b5...@amd.com
>   but I havent fully understood it. In the meantime I want to advance with v6,
>   so I hope we can continue the discussion here.
The problem is that when LLC coloring is enabled, you use allocate_memory() for 
hwdom, just like for any
other domain, so it will get assigned a VA range from a typical Xen guest 
memory map (i.e. GUEST_RAM{0,1}_{BASE,SIZE}).
This can result in memory conflicts given that the HW resources are mapped 1:1 
to it (MMIO, reserved memory regions).
Instead, for hwdom we should use the host memory layout to prevent these 
conflicts. A good example is find_unallocated_memory().
You need to:
 - fetch available RAM,
 - remove reserved-memory regions,
 - report ranges (+aligning the base and skipping banks that are not reasonable 
big)
This will give you a list of memory regions you can then use to pass to 
allocate_bank_memory().
The problem, as always, is to determine the size of the first region so that is 
is sufficiently
large to keep kernel+dtb+initrd in relatively close proximity.

~Michal

Re: xen | Failed pipeline for staging | 40a74677

[Jan Beulich]

On 30.01.2024 10:01, GitLab wrote:
> 
> 
> Pipeline #1155726092 has failed!
> 
> Project: xen ( https://gitlab.com/xen-project/xen )
> Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging )
> 
> Commit: 40a74677 ( 
> https://gitlab.com/xen-project/xen/-/commit/40a74677023a5eb20d7bbc09def37884f80919bd
>  )
> Commit Message: x86: purge NMI_IO_APIC
> 
> Even going back to 3.2 ...
> Commit Author: Jan Beulich ( https://gitlab.com/jbeulich )
> 
> 
> Pipeline #1155726092 ( 
> https://gitlab.com/xen-project/xen/-/pipelines/1155726092 ) triggered by 
> Ganis ( https://gitlab.com/ganis )
> had 6 failed jobs.
> 
> Job #6040704964 ( https://gitlab.com/xen-project/xen/-/jobs/6040704964/raw )
> 
> Stage: test
> Name: zen3p-pci-hvm-x86-64-gcc-debug

While this one shows an odd DHCP failure in the guest (can't spot any reason
in the log), ...

> Job #6040704957 ( https://gitlab.com/xen-project/xen/-/jobs/6040704957/raw )
> 
> Stage: test
> Name: adl-pci-hvm-x86-64-gcc-debug
> Job #6040704949 ( https://gitlab.com/xen-project/xen/-/jobs/6040704949/raw )
> 
> Stage: test
> Name: adl-suspend-x86-64-gcc-debug
> Job #6040704943 ( https://gitlab.com/xen-project/xen/-/jobs/6040704943/raw )
> 
> Stage: test
> Name: adl-smoke-x86-64-gcc-debug
> Job #6040704945 ( https://gitlab.com/xen-project/xen/-/jobs/6040704945/raw )
> 
> Stage: test
> Name: adl-smoke-x86-64-dom0pvh-gcc-debug
> Job #6040704953 ( https://gitlab.com/xen-project/xen/-/jobs/6040704953/raw )
> 
> Stage: test
> Name: adl-pci-pv-x86-64-gcc-debug

... all of these look to have timed out. Will ping Marek ...

Jan

Re: [RFC KERNEL PATCH v4 3/3] PCI/sysfs: Add gsi sysfs for pci_dev

[Roger Pau Monné]

On Mon, Jan 29, 2024 at 04:01:13PM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 25, 2024 at 07:17:24AM +, Chen, Jiqian wrote:
> > On 2024/1/24 00:02, Bjorn Helgaas wrote:
> > > On Tue, Jan 23, 2024 at 10:13:52AM +, Chen, Jiqian wrote:
> > >> On 2024/1/23 07:37, Bjorn Helgaas wrote:
> > >>> On Fri, Jan 05, 2024 at 02:22:17PM +0800, Jiqian Chen wrote:
> >  There is a need for some scenarios to use gsi sysfs.
> >  For example, when xen passthrough a device to dumU, it will
> >  use gsi to map pirq, but currently userspace can't get gsi
> >  number.
> >  So, add gsi sysfs for that and for other potential scenarios.
> > >> ...
> > > 
> > >>> I don't know enough about Xen to know why it needs the GSI in
> > >>> userspace.  Is this passthrough brand new functionality that can't be
> > >>> done today because we don't expose the GSI yet?
> 
> I assume this must be new functionality, i.e., this kind of
> passthrough does not work today, right?
> 
> > >> has ACPI support and is responsible for detecting and controlling
> > >> the hardware, also it performs privileged operations such as the
> > >> creation of normal (unprivileged) domains DomUs. When we give to a
> > >> DomU direct access to a device, we need also to route the physical
> > >> interrupts to the DomU. In order to do so Xen needs to setup and map
> > >> the interrupts appropriately.
> > > 
> > > What kernel interfaces are used for this setup and mapping?
> >
> > For passthrough devices, the setup and mapping of routing physical
> > interrupts to DomU are done on Xen hypervisor side, hypervisor only
> > need userspace to provide the GSI info, see Xen code:
> > xc_physdev_map_pirq require GSI and then will call hypercall to pass
> > GSI into hypervisor and then hypervisor will do the mapping and
> > routing, kernel doesn't do the setup and mapping.
> 
> So we have to expose the GSI to userspace not because userspace itself
> uses it, but so userspace can turn around and pass it back into the
> kernel?

No, the point is to pass it back to Xen, which doesn't know the
mapping between GSIs and PCI devices because it can't execute the ACPI
AML resource methods that provide such information.

The (Linux) kernel is just a proxy that forwards the hypercalls from
user-space tools into Xen.

> It seems like it would be better for userspace to pass an identifier
> of the PCI device itself back into the hypervisor.  Then the interface
> could be generic and potentially work even on non-ACPI systems where
> the GSI concept doesn't apply.

We would still need a way to pass the GSI to PCI device relation to
the hypervisor, and then cache such data in the hypervisor.

I don't think we have any preference of where such information should
be exposed, but given GSIs are an ACPI concept not specific to Xen
they should be exposed by a non-Xen specific interface.

Thanks, Roger.