Re: [PATCH v13 6/9] gfxstream + rutabaga: add initial support for gfxstream

[Gurchetan Singh]

On Wed, Sep 20, 2023 at 5:05 AM Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:

> On 20/09/2023 12:42, Akihiko Odaki wrote:
>
> > On 2023/08/29 9:36, Gurchetan Singh wrote:
> >> This adds initial support for gfxstream and cross-domain.  Both
> >> features rely on virtio-gpu blob resources and context types, which
> >> are also implemented in this patch.
> >>
> >> gfxstream has a long and illustrious history in Android graphics
> >> paravirtualization.  It has been powering graphics in the Android
> >> Studio Emulator for more than a decade, which is the main developer
> >> platform.
> >>
> >> Originally conceived by Jesse Hall, it was first known as "EmuGL" [a].
> >> The key design characteristic was a 1:1 threading model and
> >> auto-generation, which fit nicely with the OpenGLES spec.  It also
> >> allowed easy layering with ANGLE on the host, which provides the GLES
> >> implementations on Windows or MacOS enviroments.
> >>
> >> gfxstream has traditionally been maintained by a single engineer, and
> >> between 2015 to 2021, the goldfish throne passed to Frank Yang.
> >> Historians often remark this glorious reign ("pax gfxstreama" is the
> >> academic term) was comparable to that of Augustus and both Queen
> >> Elizabeths.  Just to name a few accomplishments in a resplendent
> >> panoply: higher versions of GLES, address space graphics, snapshot
> >> support and CTS compliant Vulkan [b].
> >>
> >> One major drawback was the use of out-of-tree goldfish drivers.
> >> Android engineers didn't know much about DRM/KMS and especially TTM so
> >> a simple guest to host pipe was conceived.
> >>
> >> Luckily, virtio-gpu 3D started to emerge in 2016 due to the work of
> >> the Mesa/virglrenderer communities.  In 2018, the initial virtio-gpu
> >> port of gfxstream was done by Cuttlefish enthusiast Alistair Delva.
> >> It was a symbol compatible replacement of virglrenderer [c] and named
> >> "AVDVirglrenderer".  This implementation forms the basis of the
> >> current gfxstream host implementation still in use today.
> >>
> >> cross-domain support follows a similar arc.  Originally conceived by
> >> Wayland aficionado David Reveman and crosvm enjoyer Zach Reizner in
> >> 2018, it initially relied on the downstream "virtio-wl" device.
> >>
> >> In 2020 and 2021, virtio-gpu was extended to include blob resources
> >> and multiple timelines by yours truly, features gfxstream/cross-domain
> >> both require to function correctly.
> >>
> >> Right now, we stand at the precipice of a truly fantastic possibility:
> >> the Android Emulator powered by upstream QEMU and upstream Linux
> >> kernel.  gfxstream will then be packaged properfully, and app
> >> developers can even fix gfxstream bugs on their own if they encounter
> >> them.
> >>
> >> It's been quite the ride, my friends.  Where will gfxstream head next,
> >> nobody really knows.  I wouldn't be surprised if it's around for
> >> another decade, maintained by a new generation of Android graphics
> >> enthusiasts.
> >>
> >> Technical details:
> >>- Very simple initial display integration: just used Pixman
> >>- Largely, 1:1 mapping of virtio-gpu hypercalls to rutabaga function
> >>  calls
> >>
> >> Next steps for Android VMs:
> >>- The next step would be improving display integration and UI
> interfaces
> >>  with the goal of the QEMU upstream graphics being in an emulator
> >>  release [d].
> >>
> >> Next steps for Linux VMs for display virtualization:
> >>- For widespread distribution, someone needs to package Sommelier or
> the
> >>  wayland-proxy-virtwl [e] ideally into Debian main. In addition,
> newer
> >>  versions of the Linux kernel come with DRM_VIRTIO_GPU_KMS option,
> >>  which allows disabling KMS hypercalls.  If anyone cares enough,
> it'll
> >>  probably be possible to build a custom VM variant that uses this
> display
> >>  virtualization strategy.
> >>
> >> [a]
> https://android-review.googlesource.com/c/platform/development/+/34470
> >> [b]
> https://android-review.googlesource.com/q/topic:%22vulkan-hostconnection-start%22
> >> [c]
> https://android-review.googlesource.com/c/device/generic/goldfish-opengl/+/761927
> >> [d] https://developer.android.com/studio/releases/emulator
> >> [e] https://github.com/talex5/wayland-proxy-virtwl
> >>
> >> Signed-off-by: Gurchetan Singh 
> >> Tested-by: Alyssa Ross 
> >> Tested-by: Emmanouil Pitsidianakis 
> >> Tested-by: Akihiko Odaki 
> >> Reviewed-by: Emmanouil Pitsidianakis 
> >> Reviewed-by: Antonio Caggiano 
> >> Reviewed-by: Akihiko Odaki 
> >> ---
> >>   hw/display/virtio-gpu-pci-rutabaga.c |   47 ++
> >>   hw/display/virtio-gpu-rutabaga.c | 1119 ++
> >>   hw/display/virtio-vga-rutabaga.c |   50 ++
> >>   3 files changed, 1216 insertions(+)
> >>   create mode 100644 hw/display/virtio-gpu-pci-rutabaga.c
> >>   create mode 100644 hw/display/virtio-gpu-rutabaga.c
> >>   create mode 100644 hw/display/virtio-vga-rutabaga.c

qemu-system-i386 crashes on i9 coffee lake

[Stefan Kadow]

Hello,

since commit b9ade05c
https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=b9ade05cbba977673d5a08bc7a5940c5fd8add0e

qemu-system-i386 crashes on my Intel i9 Coffee Lake System.

This does not happen when the system is booted with the latest microcode 
update. So I do not know if this is really a regression.


Please, see the discussion on xen-users mailing list:
https://lists.xenproject.org/archives/html/xen-users/2023-08/msg00014.html

--
Thx Stefan

[xen-4.15-testing test] 183108: regressions - trouble: fail/pass/starved

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 182658

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail in 
183095 pass in 183108
 test-amd64-i386-libvirt-pair 11 xen-install/dst_host   fail pass in 183095
 test-amd64-amd64-xl-qcow221 guest-start/debian.repeat  fail pass in 183095

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

version targeted for testing:
 xen  3a9a2901cc8b24f28dbdc6fb63f57006c77a1f47
baseline version:
 xen  0517763e771f2e2582bc492fafa42a86400ab957

Last test of basis   182658  2023-09-06 09:50:17 Z   15 days
Testing same since   183081  2023-09-20 10:06:38 Z1 days3 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

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

Re: [PATCH v13 6/9] gfxstream + rutabaga: add initial support for gfxstream

[Akihiko Odaki]

On 2023/09/22 9:03, Gurchetan Singh wrote:



On Wed, Sep 20, 2023 at 5:05 AM Mark Cave-Ayland 
mailto:mark.cave-ayl...@ilande.co.uk>> 
wrote:


On 20/09/2023 12:42, Akihiko Odaki wrote:

 > On 2023/08/29 9:36, Gurchetan Singh wrote:
 >> This adds initial support for gfxstream and cross-domain.  Both
 >> features rely on virtio-gpu blob resources and context types, which
 >> are also implemented in this patch.
 >>
 >> gfxstream has a long and illustrious history in Android graphics
 >> paravirtualization.  It has been powering graphics in the Android
 >> Studio Emulator for more than a decade, which is the main developer
 >> platform.
 >>
 >> Originally conceived by Jesse Hall, it was first known as
"EmuGL" [a].
 >> The key design characteristic was a 1:1 threading model and
 >> auto-generation, which fit nicely with the OpenGLES spec.  It also
 >> allowed easy layering with ANGLE on the host, which provides the
GLES
 >> implementations on Windows or MacOS enviroments.
 >>
 >> gfxstream has traditionally been maintained by a single
engineer, and
 >> between 2015 to 2021, the goldfish throne passed to Frank Yang.
 >> Historians often remark this glorious reign ("pax gfxstreama" is the
 >> academic term) was comparable to that of Augustus and both Queen
 >> Elizabeths.  Just to name a few accomplishments in a resplendent
 >> panoply: higher versions of GLES, address space graphics, snapshot
 >> support and CTS compliant Vulkan [b].
 >>
 >> One major drawback was the use of out-of-tree goldfish drivers.
 >> Android engineers didn't know much about DRM/KMS and especially
TTM so
 >> a simple guest to host pipe was conceived.
 >>
 >> Luckily, virtio-gpu 3D started to emerge in 2016 due to the work of
 >> the Mesa/virglrenderer communities.  In 2018, the initial virtio-gpu
 >> port of gfxstream was done by Cuttlefish enthusiast Alistair Delva.
 >> It was a symbol compatible replacement of virglrenderer [c] and
named
 >> "AVDVirglrenderer".  This implementation forms the basis of the
 >> current gfxstream host implementation still in use today.
 >>
 >> cross-domain support follows a similar arc.  Originally conceived by
 >> Wayland aficionado David Reveman and crosvm enjoyer Zach Reizner in
 >> 2018, it initially relied on the downstream "virtio-wl" device.
 >>
 >> In 2020 and 2021, virtio-gpu was extended to include blob resources
 >> and multiple timelines by yours truly, features
gfxstream/cross-domain
 >> both require to function correctly.
 >>
 >> Right now, we stand at the precipice of a truly fantastic
possibility:
 >> the Android Emulator powered by upstream QEMU and upstream Linux
 >> kernel.  gfxstream will then be packaged properfully, and app
 >> developers can even fix gfxstream bugs on their own if they
encounter
 >> them.
 >>
 >> It's been quite the ride, my friends.  Where will gfxstream head
next,
 >> nobody really knows.  I wouldn't be surprised if it's around for
 >> another decade, maintained by a new generation of Android graphics
 >> enthusiasts.
 >>
 >> Technical details:
 >>    - Very simple initial display integration: just used Pixman
 >>    - Largely, 1:1 mapping of virtio-gpu hypercalls to rutabaga
function
 >>  calls
 >>
 >> Next steps for Android VMs:
 >>    - The next step would be improving display integration and UI
interfaces
 >>  with the goal of the QEMU upstream graphics being in an
emulator
 >>  release [d].
 >>
 >> Next steps for Linux VMs for display virtualization:
 >>    - For widespread distribution, someone needs to package
Sommelier or the
 >>  wayland-proxy-virtwl [e] ideally into Debian main. In
addition, newer
 >>  versions of the Linux kernel come with DRM_VIRTIO_GPU_KMS
option,
 >>  which allows disabling KMS hypercalls.  If anyone cares
enough, it'll
 >>  probably be possible to build a custom VM variant that uses
this display
 >>  virtualization strategy.
 >>
 >> [a]
https://android-review.googlesource.com/c/platform/development/+/34470 

 >> [b]
https://android-review.googlesource.com/q/topic:%22vulkan-hostconnection-start%22 

 >> [c]
https://android-review.googlesource.com/c/device/generic/goldfish-opengl/+/761927 

 >> [d] https://developer.android.com/studio/releases/emulator

 >> [e] https://github.com/talex5/wayland-proxy-virtwl

Re: [PATCH] x86/paging: Delete update_cr3()'s do_locking parameter

[Henry Wang]

Hi,

> On Sep 21, 2023, at 20:38, Jan Beulich  wrote:
> On 20.09.2023 21:21, Andrew Cooper wrote:
>> Nicola reports that the XSA-438 fix introduced new MISRA violations because 
>> of
>> some incidental tidying it tried to do.  The parameter is useless, so resolve
>> the MISRA regression by removing it.
>> 
>> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
>> it to distinguish internal and external callers and therefore whether the
>> paging lock should be taken.
>> 
>> However, we have paging_lock_recursive() for this purpose, which also avoids
>> the ability for the shadow internal callers to accidentally not hold the 
>> lock.
>> 
>> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow 
>> reference")
>> Reported-by: Nicola Vetrini 
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Roger Pau Monné 
>> CC: Wei Liu 
>> CC: George Dunlap 
>> CC: Tim Deegan 
>> CC: Stefano Stabellini 
>> CC: Nicola Vetrini 
>> 
>> Slightly RFC.  Only compile tested so far.
> 
> With shadow/none.c also suitably edited
> Reviewed-by: Jan Beulich 

Release-acked-by: Henry Wang 

Kind regards,
Henry

Re: [PATCH v2 1/2] credit: Limit load balancing to once per millisecond

[Henry Wang]

Hi George,

> On Sep 21, 2023, at 20:23, George Dunlap  wrote:
> 
> The credit scheduler tries as hard as it can to ensure that it always
> runs scheduling units with positive credit (PRI_TS_UNDER) before
> running those with negative credit (PRI_TS_OVER).  If the next
> runnable scheduling unit is of priority OVER, it will always run the
> load balancer, which will scour the system looking for another
> scheduling unit of the UNDER priority.
> 
> Unfortunately, as the number of cores on a system has grown, the cost
> of the work-stealing algorithm has dramatically increased; a recent
> trace on a system with 128 cores showed this taking over 50
> microseconds.
> 
> Add a parameter, load_balance_ratelimit, to limit the frequency of
> load balance operations on a given pcpu.  Default this to 1
> millisecond.
> 
> Invert the load balancing conditional to make it more clear, and line
> up more closely with the comment above it.
> 
> Overall it might be cleaner to have the last_load_balance checking
> happen inside csched_load_balance(), but that would require either
> passing both now and spc into the function, or looking them up again;
> both of which seemed to be worse than simply checking and setting the
> values before calling it.
> 
> On a system with a vcpu:pcpu ratio of 2:1, running Windows guests
> (which will end up calling YIELD during spinlock contention), this
> patch increased performance significantly.
> 
> Signed-off-by: George Dunlap 

Release-acked-by: Henry Wang 

Kind regards,
Henry

Re: [PATCH v2 2/2] credit: Don't steal vcpus which have yielded

[Henry Wang]

Hi George,

> On Sep 21, 2023, at 20:23, George Dunlap  wrote:
> 
> On large systems with many vcpus yielding due to spinlock priority
> inversion, it's not uncommon for a vcpu to yield its timeslice, only
> to be immediately stolen by another pcpu looking for higher-priority
> work.
> 
> To prevent this:
> 
> * Keep the YIELD flag until a vcpu is removed from a runqueue
> 
> * When looking for work to steal, skip vcpus which have yielded
> 
> NB that this does mean that sometimes a VM is inserted into an empty
> runqueue; handle that case.
> 
> Signed-off-by: George Dunlap 

Release-acked-by: Henry Wang 

Kind regards,
Henry

Re: [PATCH] xen/arm64: head.S: Fix wrong enable_boot_cpu_mm() code movement

[Henry Wang]

Hi Julien,

> On Sep 22, 2023, at 01:16, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 16/09/2023 05:06, Henry Wang wrote:
>> Some addressed comments on enable_boot_cpu_mm() were reintroduced
>> back during the code movement from arm64/head.S to arm64/mmu/head.S.
>> We should drop the unreachable code, move the 'mov lr, x5' closer to
>> 'b remove_identity_mapping' so it is clearer that it will return,
>> and update the in-code comment accordingly.
>> Fixes: 6734327d76be ("xen/arm64: Split and move MMU-specific head.S to 
>> mmu/head.S")
>> Reported-by: Julien Grall 
>> Signed-off-by: Henry Wang 
> 
> Reviewed-by: Julien Grall 

Thanks very much!

> 
> I plan to commit this patch in staging so it is part of 4.18. Please let me 
> know if there are any objection.

Yes, please commit this patch, as it should have been part of the committed 
patch
6734327d76be (again I am sorry for the mess).

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

[libvirt test] 183097: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

Last test of basis   183075  2023-09-20 04:21:45 Z1 days
Testing same since   183097  2023-09-21 04:22:05 Z0 days1 attempts


People who touched revisions under test:
  Michal Privoznik 
  Pavel Hrdina 

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 

Re: [PATCH v1 2/3] ARM: GICv3 ITS: do not invalidate memory while sending a command

[Volodymyr Babchuk]

Hi Mark,

I am writing to you, because you are GICv3 maintainer in Linux. We are
updating ITS driver in Xen and we have a question about cache
maintenance WRT memory shared with ITS. As I can see, the Linux ITS
driver uses gic_flush_dcache_to_poc() all over the code. This boils down
to "dc civac" instruction which does both clean and invalidate. But do
we really need to invalidate a cache when we are sending an ITS command?
In my understanding it is sufficient to clean a cache only and Linux
uses clean just out of convenience. Is this correct?

Below you can find our discussion about this.

Julien Grall  writes:

> On 19/09/2023 15:36, Volodymyr Babchuk wrote:
>> Julien Grall  writes:
>>> On 19/09/2023 12:28, Volodymyr Babchuk wrote:
 There is no need to invalidate cache entry because we just wrote into a
 memory region. Writing itself guarantees that cache entry is valid.
>>>
>>> The goal of invalidate is to remove the line from the cache. So I
>>> don't quite understand the reasoning here.
>>>
>> Well, I may be wrong, but what is the goal in removing line from the
>> cache? As I see this, we want to be sure that ITS sees data written in
>> the memory, so we should flush a cache line. But why do we need to
>> remove it from CPU's cache?
>
> I don't exactly know. From a brief look I agree with you. However, our
> driver is based on Linux where the clean & invalidate is also used. So
> I am a little be cautious to remove it.
>
> The way forward would be to ask the Marc Zyngier (GICv3 maintainer)
> why it was added in Linux. Then we can decide whether this can be
> removed in Xen.
>
> Cheers,


-- 
WBR, Volodymyr

Re: [XEN PATCH v3] xen/hypercalls: address violations of MISRA C:2012 Rule 8.3

[Stefano Stabellini]

On Thu, 21 Sep 2023, Federico Serafini wrote:
> Make function declarations and definitions consistent.
> No functional change.
> 
> Signed-off-by: Federico Serafini 

Reviewed-by: Stefano Stabellini 

Re: [XEN PATCH] xen/numa: address a violation of MISRA C:2012 Rule 8.3

[Stefano Stabellini]

On Thu, 21 Sep 2023, Federico Serafini wrote:
> Make object declarations consistent. No functional change.
> 
> Signed-off-by: Federico Serafini 

Reviewed-by: Stefano Stabellini 


> ---
>  xen/include/xen/numa.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index 29b8c2df89..287e81ff66 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -60,7 +60,7 @@ static inline void clear_node_cpumask(unsigned int cpu)
>  /* Simple perfect hash to map pdx to node numbers */
>  extern unsigned int memnode_shift;
>  extern unsigned long memnodemapsize;
> -extern uint8_t *memnodemap;
> +extern nodeid_t *memnodemap;
>  
>  struct node_data {
>  unsigned long node_start_pfn;
> -- 
> 2.34.1
> 

Re: [PATCH] Revert "EDAC/mce_amd: Do not load edac_mce_amd module on guests"

[Elliott Mitchell]

On Fri, Sep 15, 2023 at 01:56:31PM +0200, Borislav Petkov wrote:
> On Thu, Sep 14, 2023 at 10:02:05AM -0700, Elliott Mitchell wrote:
> > Indeed.  At what point is the lack of information and response long
> > enough to simply commit a revert due to those lacks?
> 
> At no point.
> 
> > Even with the commit message having been rewritten and the link to:
> > https://lkml.kernel.org/r/20210628172740.245689-1-smita.koralahallichannabasa...@amd.com
> > added, this still reads as roughly:
> > 
> > "A hypothetical bug on a hypothetivisor"
> 
> If "Hypervisors likely do not expose the SMCA feature to the guest"
> doesn't explain to you what the problem is this commit is fixing, then
> I can't help you.

Problem is you were objecting to 'probable hypothetical "may"
formulations' in what I wrote, yet the original patch message overtly
uses that word.

In order for the first patch to be correct, it is insufficient for the
condition to be unlikely.  Ideally it should be mathematically proven
impossible.

As such I was writing about known counter-examples from the real world.
Mainly at least one hypervisor (Xen) does tend to allow a particular VM
to access sensitive system registers.  Also it is entirely possible some
hypervisor could proxy access to the registers and thus properly simulate
the events.

Not only that, but in fact this very strategy was already actively
deployed:
https://bugs.debian.org/810964

I'm less than 100% certain this successfully retrieves EDAC events on Xen
right now, so I had been taking a look at the situation only to find
767f4b620eda.

Perhaps everyone should consult with large-scale system administrators
when doing things which effect them?


-- 
(\___(\___(\__  --=> 8-) EHM <=--  __/)___/)___/)
 \BS (| ehem+sig...@m5p.com  PGP 87145445 |)   /
  \_CS\   |  _  -O #include  O-   _  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

[linux-linus test] 183089: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 8 xen-boot fail REGR. 
vs. 182531
 test-amd64-coresched-amd64-xl  8 xen-bootfail REGR. vs. 182531
 test-amd64-amd64-xl-xsm   8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-libvirt-raw  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-libvirt-qcow2  8 xen-boot   fail REGR. vs. 182531
 test-amd64-amd64-xl-vhd   8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-pvshim8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-freebsd12-amd64  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-pvhv2-intel  8 xen-boot  fail REGR. vs. 182531
 test-amd64-amd64-xl-qemuu-ws16-amd64  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-qemut-debianhvm-amd64  8 xen-bootfail REGR. vs. 182531
 test-amd64-amd64-xl-qemuu-win7-amd64  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl   8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-qemut-win7-amd64  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-libvirt-xsm  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-dom0pvh-xl-amd 14 guest-start   fail REGR. vs. 182531
 test-amd64-amd64-xl-shadow8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  8 xen-bootfail REGR. vs. 182531
 test-amd64-amd64-examine-uefi  8 reboot  fail REGR. vs. 182531
 test-amd64-amd64-pair12 xen-boot/src_hostfail REGR. vs. 182531
 test-amd64-amd64-pair13 xen-boot/dst_hostfail REGR. vs. 182531
 test-amd64-amd64-xl-credit1   8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-multivcpu  8 xen-bootfail REGR. vs. 182531
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 8 xen-boot fail REGR. 
vs. 182531
 test-amd64-amd64-qemuu-nested-intel  8 xen-boot  fail REGR. vs. 182531
 test-amd64-amd64-xl-qemut-ws16-amd64  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 8 xen-boot fail REGR. vs. 
182531
 test-amd64-amd64-xl-qemuu-ovmf-amd64  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-pvhv2-amd  8 xen-bootfail REGR. vs. 182531
 test-amd64-amd64-qemuu-nested-amd  8 xen-bootfail REGR. vs. 182531
 test-amd64-amd64-libvirt  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-pygrub   8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-freebsd11-amd64  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 8 xen-boot fail REGR. vs. 
182531
 test-amd64-amd64-xl-credit2   8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm  8 xen-boot fail REGR. vs. 182531
 test-amd64-amd64-examine-bios  8 reboot  fail REGR. vs. 182531
 test-amd64-amd64-examine  8 reboot   fail REGR. vs. 182531
 test-amd64-amd64-libvirt-pair 12 xen-boot/src_host   fail REGR. vs. 182531
 test-amd64-amd64-libvirt-pair 13 xen-boot/dst_host   fail REGR. vs. 182531
 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 182531

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-rtds  8 xen-boot fail REGR. vs. 182531

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 182531
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 182531
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 182531
 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-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  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-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-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 

[PATCH WIP] xen/public: move incomplete type definitions to xen.h

[Elliott Mitchell]

Hypercall wrappers need the incomplete type definitions.  Only when the
actual structure needed.  As such these incomplete definitions should be
in xen.h next to their hypercalls, rather than spread all over.

trap_info_t is particularly notable since even though the hypercall is
x86-only, the wrapper is likely to be visible to generic source code.

Signed-off-by: Elliott Mitchell 
---
trap_info_t and HYPERVISOR_set_trap_table() is something I ran into.
With the incomplete definition, the wrapper is accaptable to an ARM
compiler.  Without the incomplete definition, it fails.

Note, this has been shown to build in my environment.  I'm unsure
whether the incomplete structure plus type definition is acceptable to
all supportted compilers.

I'm wondering about __ASSEMBLY__.  I suspect this could be handled better
by having a macro for all these suspiciously similar type definitions.  I
suspect it would be handy for DEFINE_XEN_GUEST_HANDLE() to be null when
__ASSEMBLY__ is defined.

This seems to suggest all the __HYPERVISOR_* definitions need to move
later in the file.
---
 xen/include/public/arch-x86/xen.h |  2 --
 xen/include/public/platform.h |  2 --
 xen/include/public/sched.h|  2 --
 xen/include/public/xen.h  | 24 
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/xen/include/public/arch-x86/xen.h 
b/xen/include/public/arch-x86/xen.h
index c0f4551247..896440333c 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -143,8 +143,6 @@ struct trap_info {
 uint16_t  cs;  /* code selector */
 unsigned long address; /* code offset   */
 };
-typedef struct trap_info trap_info_t;
-DEFINE_XEN_GUEST_HANDLE(trap_info_t);
 
 typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
 
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 15777b5416..bb7f2dfcb0 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -659,8 +659,6 @@ struct xen_platform_op {
 uint8_t   pad[128];
 } u;
 };
-typedef struct xen_platform_op xen_platform_op_t;
-DEFINE_XEN_GUEST_HANDLE(xen_platform_op_t);
 
 #endif /* __XEN_PUBLIC_PLATFORM_H__ */
 
diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
index b4362c6a1d..2b65c0db8c 100644
--- a/xen/include/public/sched.h
+++ b/xen/include/public/sched.h
@@ -118,8 +118,6 @@
 struct sched_shutdown {
 unsigned int reason; /* SHUTDOWN_* => enum sched_shutdown_reason */
 };
-typedef struct sched_shutdown sched_shutdown_t;
-DEFINE_XEN_GUEST_HANDLE(sched_shutdown_t);
 
 struct sched_poll {
 XEN_GUEST_HANDLE(evtchn_port_t) ports;
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index b812a0a324..32a76afbd4 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -75,13 +75,25 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
  */
 
 #define __HYPERVISOR_set_trap_table0
+#ifndef __ASSEMBLY__
+typedef struct trap_info trap_info_t;
+DEFINE_XEN_GUEST_HANDLE(trap_info_t);
+#endif
 #define __HYPERVISOR_mmu_update1
+#ifndef __ASSEMBLY__
+typedef struct mmu_update mmu_update_t;
+DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
+#endif
 #define __HYPERVISOR_set_gdt   2
 #define __HYPERVISOR_stack_switch  3
 #define __HYPERVISOR_set_callbacks 4
 #define __HYPERVISOR_fpu_taskswitch5
 #define __HYPERVISOR_sched_op_compat   6 /* compat since 0x00030101 */
 #define __HYPERVISOR_platform_op   7
+#ifndef __ASSEMBLY__
+typedef struct xen_platform_op xen_platform_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_platform_op_t);
+#endif
 #define __HYPERVISOR_set_debugreg  8
 #define __HYPERVISOR_get_debugreg  9
 #define __HYPERVISOR_update_descriptor10
@@ -100,9 +112,17 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define __HYPERVISOR_vcpu_op  24
 #define __HYPERVISOR_set_segment_base 25 /* x86/64 only */
 #define __HYPERVISOR_mmuext_op26
+#ifndef __ASSEMBLY__
+typedef struct mmuext_op mmuext_op_t;
+DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
+#endif
 #define __HYPERVISOR_xsm_op   27
 #define __HYPERVISOR_nmi_op   28
 #define __HYPERVISOR_sched_op 29
+#ifndef __ASSEMBLY__
+typedef struct sched_shutdown sched_shutdown_t;
+DEFINE_XEN_GUEST_HANDLE(sched_shutdown_t);
+#endif
 #define __HYPERVISOR_callback_op  30
 #define __HYPERVISOR_xenoprof_op  31
 #define __HYPERVISOR_event_channel_op 32
@@ -449,8 +469,6 @@ struct mmuext_op {
 xen_pfn_t src_mfn;
 } arg2;
 };
-typedef struct mmuext_op mmuext_op_t;
-DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
 #endif
 
 /*
@@ -615,8 +633,6 @@ struct mmu_update {
 uint64_t ptr;   /* Machine address of PTE. */
 uint64_t val;   /* New contents of PTE.*/
 };
-typedef struct mmu_update mmu_update_t;

RE: [PATCH v10 28/38] x86/fred: FRED entry/exit and dispatch code

[Li, Xin3]

> > Since future kernels will support boottime toggling of whether 32bit
> > syscall interface should be enabled or not as per:
> > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=
> > x86/entry=1da5c9bc119d3a749b519596b93f9b2667e93c4a
> >
> > It will make more sense to replace this with ia32_enabled() invocation.
> > I guess this could be done as a follow-up patch based on when this is
> > merged as the ia32_enbaled changes are going to be merged in 6.7.
> 
> The simplest solution is to rebase the series to tip x86/entry and just do it 
> right
> away :)

Just did it for the next iteration.

Re: [PATCH] xen/arm64: head.S: Fix wrong enable_boot_cpu_mm() code movement

[Julien Grall]

Hi Henry,

On 16/09/2023 05:06, Henry Wang wrote:

Some addressed comments on enable_boot_cpu_mm() were reintroduced
back during the code movement from arm64/head.S to arm64/mmu/head.S.
We should drop the unreachable code, move the 'mov lr, x5' closer to
'b remove_identity_mapping' so it is clearer that it will return,
and update the in-code comment accordingly.

Fixes: 6734327d76be ("xen/arm64: Split and move MMU-specific head.S to 
mmu/head.S")
Reported-by: Julien Grall 
Signed-off-by: Henry Wang 


Reviewed-by: Julien Grall 

I plan to commit this patch in staging so it is part of 4.18. Please let 
me know if there are any objection.


Cheers,

--
Julien Grall

[xen-4.17-testing test] 183102: tolerable trouble: fail/pass/starved - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  90c540c58985dc774cf0a1d2dc423473d3f37267
baseline version:
 xen  d31e5b2a9c39816a954d1088d4cfc782f0006f39

Last test of basis   182639  2023-09-05 22:40:32 Z   15 days
Testing same since   183083  2023-09-20 10:06:57 Z1 days2 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

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

Re: [GIT PULL] xen: branch for v6.6-rc3

[pr-tracker-bot]

The pull request you sent on Thu, 21 Sep 2023 10:41:10 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git 
> for-linus-6.6a-rc3-tag

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/88a174a906fe9679a26c0f69fcc022743d2c8e05

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [PATCH v9 12/16] vpci: add initial support for virtual PCI bus topology

[Roger Pau Monné]

On Tue, Aug 29, 2023 at 11:19:46PM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko 
> 
> Assign SBDF to the PCI devices being passed through with bus 0.
> The resulting topology is where PCIe devices reside on the bus 0 of the
> root complex itself (embedded endpoints).
> This implementation is limited to 32 devices which are allowed on
> a single PCI bus.
> 
> Please note, that at the moment only function 0 of a multifunction
> device can be passed through.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---
> Since v9:
> - Lock in add_virtual_device() replaced with ASSERT (thanks, Stewart)
> Since v8:
> - Added write lock in add_virtual_device
> Since v6:
> - re-work wrt new locking scheme
> - OT: add ASSERT(pcidevs_write_locked()); to add_virtual_device()
> Since v5:
> - s/vpci_add_virtual_device/add_virtual_device and make it static
> - call add_virtual_device from vpci_assign_device and do not use
>   REGISTER_VPCI_INIT machinery
> - add pcidevs_locked ASSERT
> - use DECLARE_BITMAP for vpci_dev_assigned_map
> Since v4:
> - moved and re-worked guest sbdf initializers
> - s/set_bit/__set_bit
> - s/clear_bit/__clear_bit
> - minor comment fix s/Virtual/Guest/
> - added VPCI_MAX_VIRT_DEV constant (PCI_SLOT(~0) + 1) which will be used
>   later for counting the number of MMIO handlers required for a guest
>   (Julien)
> Since v3:
>  - make use of VPCI_INIT
>  - moved all new code to vpci.c which belongs to it
>  - changed open-coded 31 to PCI_SLOT(~0)
>  - added comments and code to reject multifunction devices with
>functions other than 0
>  - updated comment about vpci_dev_next and made it unsigned int
>  - implement roll back in case of error while assigning/deassigning devices
>  - s/dom%pd/%pd
> Since v2:
>  - remove casts that are (a) malformed and (b) unnecessary
>  - add new line for better readability
>  - remove CONFIG_HAS_VPCI_GUEST_SUPPORT ifdef's as the relevant vPCI
> functions are now completely gated with this config
>  - gate common code with CONFIG_HAS_VPCI_GUEST_SUPPORT
> New in v2
> ---
>  xen/drivers/vpci/vpci.c | 69 +
>  xen/include/xen/sched.h |  8 +
>  xen/include/xen/vpci.h  | 11 +++
>  3 files changed, 88 insertions(+)
> 
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 412685f41d..b284f95e05 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -36,6 +36,54 @@ extern vpci_register_init_t *const __start_vpci_array[];
>  extern vpci_register_init_t *const __end_vpci_array[];
>  #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +static int add_virtual_device(struct pci_dev *pdev)
> +{
> +struct domain *d = pdev->domain;
> +pci_sbdf_t sbdf = { 0 };
> +unsigned long new_dev_number;
> +
> +if ( is_hardware_domain(d) )
> +return 0;
> +
> +ASSERT(pcidevs_locked() && rw_is_write_locked(>domain->pci_lock));


Do you need to check for pcidevs here?  I would think d->pci_lock
would be enough to protect the virtual allocation device bitmap.

> +
> +/*
> + * Each PCI bus supports 32 devices/slots at max or up to 256 when
> + * there are multi-function ones which are not yet supported.
> + */
> +if ( pdev->info.is_extfn )

I think you are missing a !pdev->info.is_virtfn, as is_extfn &&
is_virtfn mean the PF it's an extended function, but not the VF we are
trying to passthrough.

> +{
> +gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n",
> + >sbdf);
> +return -EOPNOTSUPP;
> +}
> +new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map,
> + VPCI_MAX_VIRT_DEV);
> +if ( new_dev_number >= VPCI_MAX_VIRT_DEV )

The > is not required, as find_first_zero_bit() will return
VPCI_MAX_VIRT_DEV if the bitmap is all set.

> +{
> +write_unlock(>domain->pci_lock);
> +return -ENOSPC;
> +}
> +
> +__set_bit(new_dev_number, >vpci_dev_assigned_map);
> +
> +/*
> + * Both segment and bus number are 0:
> + *  - we emulate a single host bridge for the guest, e.g. segment 0
> + *  - with bus 0 the virtual devices are seen as embedded
> + *endpoints behind the root complex
> + *
> + * TODO: add support for multi-function devices.
> + */
> +sbdf.devfn = PCI_DEVFN(new_dev_number, 0);
> +pdev->vpci->guest_sbdf = sbdf;

You could avoid the local sbdf variable and just use PCI_SBDF(0, 0,
new_dev_number, 0);

> +
> +return 0;
> +}
> +
> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> +
>  void vpci_deassign_device(struct pci_dev *pdev)
>  {
>  unsigned int i;
> @@ -46,6 +94,16 @@ void vpci_deassign_device(struct pci_dev *pdev)
>  return;
>  
>  spin_lock(>vpci->lock);
> +
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +if ( pdev->vpci->guest_sbdf.sbdf != ~0 )
> +{
> +

[XEN PATCH] xen/emul-i8254: address a violation of MISRA C:2012 Rule 8.3

[Federico Serafini]

Make function declaration and definition consistent.
No fuctional change.

Signed-off-by: Federico Serafini 
---
 xen/arch/x86/emul-i8254.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c
index 41ec4a1ef1..c48a3606a2 100644
--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -572,7 +572,7 @@ static uint32_t speaker_ioport_read(
 }
 
 static int cf_check handle_speaker_io(
-int dir, unsigned int port, uint32_t bytes, uint32_t *val)
+int dir, unsigned int port, unsigned int bytes, uint32_t *val)
 {
 struct PITState *vpit = vcpu_vpit(current);
 
-- 
2.34.1

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

[osstest service owner]

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

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  39113a8a23fb932b1de81ac83463c16af98d533d
baseline version:
 xen  932c3c8b4bd5cb7e3a2fe78105a80928307c9858

Last test of basis   183090  2023-09-20 20:02:05 Z0 days
Testing same since   183104  2023-09-21 11:00:26 Z0 days1 attempts


People who touched revisions under test:
  Julien Grall 

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
   932c3c8b4b..39113a8a23  39113a8a23fb932b1de81ac83463c16af98d533d -> smoke

Re: [PATCH v2 2/2] credit: Don't steal vcpus which have yielded

[George Dunlap]

On Thu, Sep 21, 2023 at 1:23 PM George Dunlap  wrote:
>
> On large systems with many vcpus yielding due to spinlock priority
> inversion, it's not uncommon for a vcpu to yield its timeslice, only
> to be immediately stolen by another pcpu looking for higher-priority
> work.
>
> To prevent this:
>
> * Keep the YIELD flag until a vcpu is removed from a runqueue
>
> * When looking for work to steal, skip vcpus which have yielded
>
> NB that this does mean that sometimes a VM is inserted into an empty
> runqueue; handle that case.
>
> Signed-off-by: George Dunlap 

Marcus,

Just wanted to verify my interpretation of the testing you did of this
patch several months ago:

1. On the problematic workload, mean execution time for the task under
heavy load was around 12 seconds
2. With only patch 2 of this series (0004 in your tests), mean
execution time under heavy load was around 5 seconds
3. With only patch 1 of this series (0003 in your tests), mean
execution time under heavy load was around 3 seconds
4. With both patch 1 and patch 2 of this series (0003+0004 in your
tests), mean execution time under heavy load was also around 3 seconds

So both patches independently exhibit an improvement; but the combined
effect is about the same as the first patch.

Assuming those results are accurate, I would argue that we should take
both patches.  Does anyone want to argue we should only take the first
one?

 -George

[XEN PATCH] xen/numa: address a violation of MISRA C:2012 Rule 8.3

[Federico Serafini]

Make object declarations consistent. No functional change.

Signed-off-by: Federico Serafini 
---
 xen/include/xen/numa.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index 29b8c2df89..287e81ff66 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -60,7 +60,7 @@ static inline void clear_node_cpumask(unsigned int cpu)
 /* Simple perfect hash to map pdx to node numbers */
 extern unsigned int memnode_shift;
 extern unsigned long memnodemapsize;
-extern uint8_t *memnodemap;
+extern nodeid_t *memnodemap;
 
 struct node_data {
 unsigned long node_start_pfn;
-- 
2.34.1

[xen-4.15-testing test] 183095: regressions - trouble: fail/pass/starved

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 182658

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-shadow 20 guest-localmigrate/x10 fail in 183081 pass in 
183095
 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail pass 
in 183081

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

Re: [PATCH] xen/arm64: head.S: Fix wrong enable_boot_cpu_mm() code movement

[Ayan Kumar Halder]

On 16/09/2023 05:06, Henry Wang wrote:

CAUTION: This message has originated from an External Source. Please use proper 
judgment and caution when opening attachments, clicking links, or responding to 
this email.


Some addressed comments on enable_boot_cpu_mm() were reintroduced
back during the code movement from arm64/head.S to arm64/mmu/head.S.
We should drop the unreachable code, move the 'mov lr, x5' closer to
'b remove_identity_mapping' so it is clearer that it will return,
and update the in-code comment accordingly.

Fixes: 6734327d76be ("xen/arm64: Split and move MMU-specific head.S to 
mmu/head.S")
Reported-by: Julien Grall 
Signed-off-by: Henry Wang 

Reviewed-by: Ayan Kumar Halder 

---
  xen/arch/arm/arm64/mmu/head.S | 11 +++
  1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
index a5271e3880..88075ef083 100644
--- a/xen/arch/arm/arm64/mmu/head.S
+++ b/xen/arch/arm/arm64/mmu/head.S
@@ -329,7 +329,6 @@ ENTRY(enable_boot_cpu_mm)
  load_paddr x0, boot_pgtable

  blenable_mmu
-mov   lr, x5

  /*
   * The MMU is turned on and we are in the 1:1 mapping. Switch
@@ -338,19 +337,15 @@ ENTRY(enable_boot_cpu_mm)
  ldr   x0, =1f
  brx0
  1:
+mov   lr, x5
  /*
   * The 1:1 map may clash with other parts of the Xen virtual memory
   * layout. As it is not used anymore, remove it completely to
   * avoid having to worry about replacing existing mapping
- * afterwards. Function will return to primary_switched.
+ * afterwards. Function will return to the virtual address requested
+ * by the caller.
   */
  b remove_identity_mapping
-
-/*
- * Below is supposed to be unreachable code, as "ret" in
- * remove_identity_mapping will use the return address in LR in 
advance.
- */
-b fail
  ENDPROC(enable_boot_cpu_mm)

  /*
--
2.25.1

Re: [PATCH v9 11/16] vpci/header: reset the command register when adding devices

[Roger Pau Monné]

On Tue, Aug 29, 2023 at 11:19:45PM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko 
> 
> Reset the command register when assigning a PCI device to a guest:
> according to the PCI spec the PCI_COMMAND register is typically all 0's
> after reset, but this might not be true for the guest as it needs
> to respect host's settings.
> For that reason, do not write 0 to the PCI_COMMAND register directly,
> but go through the corresponding emulation layer (cmd_write), which
> will take care about the actual bits written. Also, honor value of
> PCI_COMMAND_VGA_PALETTE value, which is set by firmware.

I think this is likely dangerous, it would be better IMO to simply
make sure the value presented to the guest is all zeros, and that the
vPCI cached state is consistent with that.

> Signed-off-by: Oleksandr Andrushchenko 
> Signed-off-by: Volodymyr Babchuk 
> ---
> Since v9:
> - Honor PCI_COMMAND_VGA_PALETTE bit
> Since v6:
> - use cmd_write directly without introducing emulate_cmd_reg
> - update commit message with more description on all 0's in PCI_COMMAND
> Since v5:
> - updated commit message
> Since v1:
>  - do not write 0 to the command register, but respect host settings.
> ---
>  xen/drivers/vpci/header.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index e351db4620..1d243eeaf9 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -762,6 +762,12 @@ static int cf_check init_bars(struct pci_dev *pdev)
>  return -EOPNOTSUPP;
>  }
>  
> +/* Reset the command register for guests. We want to preserve only
> + * PCI_COMMAND_VGA_PALETTE as it is configured by firmware */

Wrong comment style, and PCI_COMMAND_VGA_PALETTE is likely to be gone
anyway after we perform a FLR of the device anyway?

> +cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> +if ( !is_hwdom )
> +cmd_write(pdev, PCI_COMMAND, cmd & PCI_COMMAND_VGA_PALETTE, header);

Such cmd_write() call might trigger an attempt to change the guest
physmap if you are toggling the Memory Enabled bit from 1 -> 0, and
that would fail because the guest doesn't have BAR p2m mappings setup
yet, those are done at the end of the function by the call to
modify_bars().

Thanks, Roger.

Re: [PATCH] x86/paging: Delete update_cr3()'s do_locking parameter

[Andrew Cooper]

On 21/09/2023 1:38 pm, Jan Beulich wrote:
> On 20.09.2023 21:21, Andrew Cooper wrote:
>> Nicola reports that the XSA-438 fix introduced new MISRA violations because 
>> of
>> some incidental tidying it tried to do.  The parameter is useless, so resolve
>> the MISRA regression by removing it.
>>
>> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
>> it to distinguish internal and external callers and therefore whether the
>> paging lock should be taken.
>>
>> However, we have paging_lock_recursive() for this purpose, which also avoids
>> the ability for the shadow internal callers to accidentally not hold the 
>> lock.
>>
>> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow 
>> reference")
>> Reported-by: Nicola Vetrini 
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Roger Pau Monné 
>> CC: Wei Liu 
>> CC: George Dunlap 
>> CC: Tim Deegan 
>> CC: Stefano Stabellini 
>> CC: Nicola Vetrini 
>>
>> Slightly RFC.  Only compile tested so far.
> With shadow/none.c also suitably edited
> Reviewed-by: Jan Beulich 

Ah yes - I did forget about none.c.  Thanks.

> I'm a little surprised you introduce new uses of the (kind of odd) recursive 
> lock,
> when previously you voiced your dislike for our use of such. ("Kind of odd" 
> because
> unlike spin_lock_recursive(), only the potentially inner caller needs to use 
> the
> recursive form of the acquire.)

I do very much dislike recursive locks, and I do think that an
alternative universe without them would be better code.  But a stream of
int/bool params are a similarly bad antipattern too.

As paging_lock_recursive() is used for this exact purpose elsewhere,
it's silly not to use fix one of the problems when it doesn't really
make the other problem worse.

~Andrew

Re: [PATCH v9 10/16] vpci/header: emulate PCI_COMMAND register for guests

[Roger Pau Monné]

On Tue, Aug 29, 2023 at 11:19:44PM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko 
> 
> Xen and/or Dom0 may have put values in PCI_COMMAND which they expect
> to remain unaltered. PCI_COMMAND_SERR bit is a good example: while the
> guest's view of this will want to be zero initially, the host having set
> it to 1 may not easily be overwritten with 0, or else we'd effectively
> imply giving the guest control of the bit. Thus, PCI_COMMAND register needs
> proper emulation in order to honor host's settings.
> 
> According to "PCI LOCAL BUS SPECIFICATION, REV. 3.0", section "6.2.2
> Device Control" the reset state of the command register is typically 0,
> so when assigning a PCI device use 0 as the initial state for the guest's view
> of the command register.
> 
> Here is the full list of command register bits with notes about
> emulation:
> 
> PCI_COMMAND_IO - Allow guest to control it.
> PCI_COMMAND_MEMORY - Already handled.
> PCI_COMMAND_MASTER - Allow guest to control it.
> PCI_COMMAND_SPECIAL - Guest can generate special cycles only if it has
> access to host bridge that supports software generation of special
> cycles. In our case guest has no access to host bridges at all. Value
> after reset is 0.
> PCI_COMMAND_INVALIDATE - Allows "Memory Write and Invalidate" commands
> to be generated. It requires additional configuration via Cacheline
> Size register. We are not emulating this register right now and we
> can't expect guest to properly configure it.
> PCI_COMMAND_VGA_PALETTE - Enable VGA palette snooping. This bit is set
> by firmware and we want to leave it as is.
> PCI_COMMAND_PARITY - Controls how device response to parity
> errors. We want this bit to be set by a hardware domain.
> PCI_COMMAND_WAIT - Reserved. Should be 0.
> PCI_COMMAND_SERR - Controls if device can assert SERR.
> The same as for COMMAND_PARITY.
> PCI_COMMAND_FAST_BACK - Optional bit that allows fast back-to-back
> transactions. It is configured by firmware, so we don't want guest to
> control it.
> PCI_COMMAND_INTX_DISABLE - Disables INTx signals. If MSI(X) is
> enabled, device is prohibited from asserting INTx. Value after reset
> is 0. Guest can control it freely.

I'm kind of confused by the text above, does "Guest can control it
freely" imply that the guest is able to modify the bit in the device
command register vs the emulated view that we provide?  If so
INTX_DISABLE should not be allowed direct guest modification.

I'm thinking that we might want to allow guest access to the first 3
bits only, while the rest of the values won't be propagated to
hardware, iow: guest will get a fake view of them.

Have you checked with QEMU how are those bits handled?  That's our
current passthrough reference implementation, and we should aim to
handle those similarly in vPCI.

> 
> Signed-off-by: Oleksandr Andrushchenko 
> Signed-off-by: Volodymyr Babchuk 
> ---
> Since v9:
> - Reworked guest_cmd_read
> - Added handling for more bits
> Since v6:
> - fold guest's logic into cmd_write
> - implement cmd_read, so we can report emulated INTx state to guests
> - introduce header->guest_cmd to hold the emulated state of the
>   PCI_COMMAND register for guests
> Since v5:
> - add additional check for MSI-X enabled while altering INTX bit
> - make sure INTx disabled while guests enable MSI/MSI-X
> Since v3:
> - gate more code on CONFIG_HAS_MSI
> - removed logic for the case when MSI/MSI-X not enabled
> ---
>  xen/drivers/vpci/header.c | 54 ---
>  xen/drivers/vpci/msi.c| 10 
>  xen/drivers/vpci/msix.c   |  4 +++
>  xen/include/xen/vpci.h|  3 +++
>  4 files changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 1e82217200..e351db4620 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -502,14 +502,37 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>  return 0;
>  }
>  
> +/* TODO: Add proper emulation for all bits of the command register. */
>  static void cf_check cmd_write(
>  const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
>  {
>  struct vpci_header *header = data;
>  
> +if ( !is_hardware_domain(pdev->domain) )
> +{
> +if ( IS_ENABLED(CONFIG_HAS_PCI_MSI) )
> +{
> +/* Tell guest that device does not support this */
> +cmd &= ~PCI_COMMAND_FAST_BACK;
> +}
> +
> +header->guest_cmd = cmd;
> +
> +if ( IS_ENABLED(CONFIG_HAS_PCI_MSI) )
> +{
> +/* Do not touch INVALIDATE, PARITY and SERR */
> +const uint16_t excluded = PCI_COMMAND_INVALIDATE |
> +PCI_COMMAND_PARITY | PCI_COMMAND_SERR;
> +
> +cmd &= ~excluded;
> +cmd |= pci_conf_read16(pdev->sbdf, reg) & excluded;
> +}

I'm not following why allowing guest setting of those bits is
conditional on HAS_PCI_MSI being build 

Re: [PATCH v2 1/2] credit: Limit load balancing to once per millisecond

[Juergen Gross]

On 21.09.23 14:23, George Dunlap wrote:

The credit scheduler tries as hard as it can to ensure that it always
runs scheduling units with positive credit (PRI_TS_UNDER) before
running those with negative credit (PRI_TS_OVER).  If the next
runnable scheduling unit is of priority OVER, it will always run the
load balancer, which will scour the system looking for another
scheduling unit of the UNDER priority.

Unfortunately, as the number of cores on a system has grown, the cost
of the work-stealing algorithm has dramatically increased; a recent
trace on a system with 128 cores showed this taking over 50
microseconds.

Add a parameter, load_balance_ratelimit, to limit the frequency of
load balance operations on a given pcpu.  Default this to 1
millisecond.

Invert the load balancing conditional to make it more clear, and line
up more closely with the comment above it.

Overall it might be cleaner to have the last_load_balance checking
happen inside csched_load_balance(), but that would require either
passing both now and spc into the function, or looking them up again;
both of which seemed to be worse than simply checking and setting the
values before calling it.

On a system with a vcpu:pcpu ratio of 2:1, running Windows guests
(which will end up calling YIELD during spinlock contention), this
patch increased performance significantly.

Signed-off-by: George Dunlap 
---
Changes since v1:
- Fix editing mistake in commit message
- Improve documentation
- global var is __ro_after_init
- Remove sysctl, as it's not used.  Define max value in credit.c.
- Fix some style issues
- Move comment tweak to the right patch
- In the event that the commandline-parameter value is too high, clip
   to the maximum value rather than setting to the default.

CC: Dario Faggioli 
CC: Andrew Cooper 
CC: George Dunlap 
CC: Jan Beulich 
CC: Julien Grall 
CC: Stefano Stabellini 
CC: Wei Liu 
---
  docs/misc/xen-command-line.pandoc |  8 ++
  xen/common/sched/credit.c | 47 +--
  2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index f88e6a70ae..9c3c72a7f9 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1884,6 +1884,14 @@ By default, Xen will use the INVPCID instruction for TLB 
management if
  it is available.  This option can be used to cause Xen to fall back to
  older mechanisms, which are generally slower.
  
+### load-balance-ratelimit

+> `= `
+
+The minimum interval between load balancing events on a given pcpu, in
+microseconds.  A value of '0' will disable rate limiting.  Maximum
+value 1 second. At the moment only credit honors this parameter.
+Default 1ms.
+
  ### noirqbalance (x86)
  > `= `
  
diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c

index f2cd3d9da3..5c06f596d2 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -50,6 +50,10 @@
  #define CSCHED_TICKS_PER_TSLICE 3
  /* Default timeslice: 30ms */
  #define CSCHED_DEFAULT_TSLICE_MS30
+/* Default load balancing ratelimit: 1ms */
+#define CSCHED_DEFAULT_LOAD_BALANCE_RATELIMIT_US 1000
+/* Max load balancing ratelimit: 1s */
+#define CSCHED_MAX_LOAD_BALANCE_RATELIMIT_US 100
  #define CSCHED_CREDITS_PER_MSEC 10
  /* Never set a timer shorter than this value. */
  #define CSCHED_MIN_TIMERXEN_SYSCTL_SCHED_RATELIMIT_MIN
@@ -153,6 +157,7 @@ struct csched_pcpu {
  
  unsigned int idle_bias;

  unsigned int nr_runnable;
+s_time_t last_load_balance;
  
  unsigned int tick;

  struct timer ticker;
@@ -218,7 +223,7 @@ struct csched_private {
  
  /* Period of master and tick in milliseconds */

  unsigned int tick_period_us, ticks_per_tslice;
-s_time_t ratelimit, tslice, unit_migr_delay;
+s_time_t ratelimit, tslice, unit_migr_delay, load_balance_ratelimit;
  
  struct list_head active_sdom;

  uint32_t weight;
@@ -612,6 +617,8 @@ init_pdata(struct csched_private *prv, struct csched_pcpu 
*spc, int cpu)
  BUG_ON(!is_idle_unit(curr_on_cpu(cpu)));
  cpumask_set_cpu(cpu, prv->idlers);
  spc->nr_runnable = 0;
+
+spc->last_load_balance = NOW();
  }
  
  static void cf_check

@@ -1676,9 +1683,17 @@ csched_runq_steal(int peer_cpu, int cpu, int pri, int 
balance_step)
  return NULL;
  }
  
+/*

+ * Minimum delay, in microseconds, between load balance operations.
+ * This prevents spending too much time doing load balancing, particularly
+ * when the system has a high number of YIELDs due to spinlock priority 
inversion.
+ */
+static unsigned int __ro_after_init load_balance_ratelimit_us = 
CSCHED_DEFAULT_LOAD_BALANCE_RATELIMIT_US;
+integer_param("load-balance-ratelimit", load_balance_ratelimit_us);
+
  static struct csched_unit *
  csched_load_balance(struct csched_private *prv, int cpu,
-struct csched_unit *snext, bool *stolen)
+struct csched_unit *snext, bool 

Re: [PATCH v2 1/2] credit: Limit load balancing to once per millisecond

[Juergen Gross]

On 21.09.23 14:23, George Dunlap wrote:

The credit scheduler tries as hard as it can to ensure that it always
runs scheduling units with positive credit (PRI_TS_UNDER) before
running those with negative credit (PRI_TS_OVER).  If the next
runnable scheduling unit is of priority OVER, it will always run the
load balancer, which will scour the system looking for another
scheduling unit of the UNDER priority.

Unfortunately, as the number of cores on a system has grown, the cost
of the work-stealing algorithm has dramatically increased; a recent
trace on a system with 128 cores showed this taking over 50
microseconds.

Add a parameter, load_balance_ratelimit, to limit the frequency of
load balance operations on a given pcpu.  Default this to 1
millisecond.

Invert the load balancing conditional to make it more clear, and line
up more closely with the comment above it.

Overall it might be cleaner to have the last_load_balance checking
happen inside csched_load_balance(), but that would require either
passing both now and spc into the function, or looking them up again;
both of which seemed to be worse than simply checking and setting the
values before calling it.

On a system with a vcpu:pcpu ratio of 2:1, running Windows guests
(which will end up calling YIELD during spinlock contention), this
patch increased performance significantly.

Signed-off-by: George Dunlap 


Reviewed-by: Juergen Gross 


Juergen



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic

[Philippe Mathieu-Daudé]

On 21/9/23 14:13, Markus Armbruster wrote:

Variables declared in macros can shadow other variables.  Much of the
time, this is harmless, e.g.:

 #define _FDT(exp)  \
 do {   \
 int ret = (exp);   \
 if (ret < 0) { \
 error_report("error creating device tree: %s: %s",   \
 #exp, fdt_strerror(ret));  \
 exit(1);   \
 }  \
 } while (0)

Harmless shadowing in h_client_architecture_support():

 target_ulong ret;

 [...]

 ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
 if (ret == H_SUCCESS) {
 _FDT((fdt_pack(spapr->fdt_blob)));
 [...]
 }

 return ret;

However, we can get in trouble when the shadowed variable is used in a
macro argument:

 #define QOBJECT(obj) ({ \
 typeof(obj) o = (obj);  \
 o ? container_of(&(o)->base, QObject, base) : NULL; \
  })

QOBJECT(o) expands into

 ({
--->typeof(o) o = (o);
 o ? container_of(&(o)->base, QObject, base) : NULL;
 })

Unintended variable name capture at --->.  We'd be saved by
-Winit-self.  But I could certainly construct more elaborate death
traps that don't trigger it.

To reduce the risk of trapping ourselves, we use variable names in
macros that no sane person would use elsewhere.  Here's our actual
definition of QOBJECT():

 #define QOBJECT(obj) ({ \
 typeof(obj) _obj = (obj);   \
 _obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
 })

Works well enough until we nest macro calls.  For instance, with

 #define qobject_ref(obj) ({ \
 typeof(obj) _obj = (obj);   \
 qobject_ref_impl(QOBJECT(_obj));\
 _obj;   \
 })

the expression qobject_ref(obj) expands into

 ({
 typeof(obj) _obj = (obj);
 qobject_ref_impl(
 ({
--->typeof(_obj) _obj = (_obj);
 _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
 }));
 _obj;
 })

Unintended variable name capture at --->.

The only reliable way to prevent unintended variable name capture is
-Wshadow.

One blocker for enabling it is shadowing hiding in function-like
macros like

  qdict_put(dict, "name", qobject_ref(...))

qdict_put() wraps its last argument in QOBJECT(), and the last
argument here contains another QOBJECT().

Use dark preprocessor sorcery to make the macros that give us this
problem use different variable names on every call.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
  include/qapi/qmp/qobject.h | 10 --
  include/qemu/atomic.h  | 17 -
  include/qemu/compiler.h|  3 +++
  include/qemu/osdep.h   | 27 ---
  4 files changed, 43 insertions(+), 14 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] x86/paging: Delete update_cr3()'s do_locking parameter

[Jan Beulich]

On 20.09.2023 21:21, Andrew Cooper wrote:
> Nicola reports that the XSA-438 fix introduced new MISRA violations because of
> some incidental tidying it tried to do.  The parameter is useless, so resolve
> the MISRA regression by removing it.
> 
> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
> it to distinguish internal and external callers and therefore whether the
> paging lock should be taken.
> 
> However, we have paging_lock_recursive() for this purpose, which also avoids
> the ability for the shadow internal callers to accidentally not hold the lock.
> 
> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow 
> reference")
> Reported-by: Nicola Vetrini 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Roger Pau Monné 
> CC: Wei Liu 
> CC: George Dunlap 
> CC: Tim Deegan 
> CC: Stefano Stabellini 
> CC: Nicola Vetrini 
> 
> Slightly RFC.  Only compile tested so far.

With shadow/none.c also suitably edited
Reviewed-by: Jan Beulich 

I'm a little surprised you introduce new uses of the (kind of odd) recursive 
lock,
when previously you voiced your dislike for our use of such. ("Kind of odd" 
because
unlike spin_lock_recursive(), only the potentially inner caller needs to use the
recursive form of the acquire.)

Jan

Re: [PATCH v10 33/38] x86/entry: Add fred_entry_from_kvm() for VMX to handle IRQ/NMI

[Paolo Bonzini]

On 9/21/23 14:11, Nikolay Borisov wrote:


+SYM_FUNC_START(asm_fred_entry_from_kvm)
+    push %rbp
+    mov %rsp, %rbp


use FRAME_BEGIN/FRAME_END macros to ommit this code if 
CONFIG_FRAME_POINTER is disabled.


No, the previous stack pointer is used below, so the code might as well 
use %rbp for that; but it must do so unconditionally.


Paolo


+
+    UNWIND_HINT_SAVE
+
+    /*
+ * Don't check the FRED stack level, the call stack leading to this
+ * helper is effectively constant and shallow (relatively speaking).
+ *
+ * Emulate the FRED-defined redzone and stack alignment.
+ */
+    sub $(FRED_CONFIG_REDZONE_AMOUNT << 6), %rsp
+    and $FRED_STACK_FRAME_RSP_MASK, %rsp
+
+    /*
+ * Start to push a FRED stack frame, which is always 64 bytes:
+ *
+ * ++-+
+ * | Bytes  | Usage   |
+ * ++-+
+ * | 63:56  | Reserved    |
+ * | 55:48  | Event Data  |
+ * | 47:40  | SS + Event Info |
+ * | 39:32  | RSP |
+ * | 31:24  | RFLAGS  |
+ * | 23:16  | CS + Aux Info   |
+ * |  15:8  | RIP |
+ * |   7:0  | Error Code  |
+ * ++-+
+ */
+    push $0    /* Reserved, must be 0 */
+    push $0    /* Event data, 0 for IRQ/NMI */
+    push %rdi    /* fred_ss handed in by the caller */
+    push %rbp


^^ here

Paolo


+    pushf
+    mov $__KERNEL_CS, %rax
+    push %rax 

[PATCH v2 1/2] credit: Limit load balancing to once per millisecond

[George Dunlap]

The credit scheduler tries as hard as it can to ensure that it always
runs scheduling units with positive credit (PRI_TS_UNDER) before
running those with negative credit (PRI_TS_OVER).  If the next
runnable scheduling unit is of priority OVER, it will always run the
load balancer, which will scour the system looking for another
scheduling unit of the UNDER priority.

Unfortunately, as the number of cores on a system has grown, the cost
of the work-stealing algorithm has dramatically increased; a recent
trace on a system with 128 cores showed this taking over 50
microseconds.

Add a parameter, load_balance_ratelimit, to limit the frequency of
load balance operations on a given pcpu.  Default this to 1
millisecond.

Invert the load balancing conditional to make it more clear, and line
up more closely with the comment above it.

Overall it might be cleaner to have the last_load_balance checking
happen inside csched_load_balance(), but that would require either
passing both now and spc into the function, or looking them up again;
both of which seemed to be worse than simply checking and setting the
values before calling it.

On a system with a vcpu:pcpu ratio of 2:1, running Windows guests
(which will end up calling YIELD during spinlock contention), this
patch increased performance significantly.

Signed-off-by: George Dunlap 
---
Changes since v1:
- Fix editing mistake in commit message
- Improve documentation
- global var is __ro_after_init
- Remove sysctl, as it's not used.  Define max value in credit.c.
- Fix some style issues
- Move comment tweak to the right patch
- In the event that the commandline-parameter value is too high, clip
  to the maximum value rather than setting to the default.

CC: Dario Faggioli 
CC: Andrew Cooper 
CC: George Dunlap 
CC: Jan Beulich 
CC: Julien Grall 
CC: Stefano Stabellini 
CC: Wei Liu 
---
 docs/misc/xen-command-line.pandoc |  8 ++
 xen/common/sched/credit.c | 47 +--
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index f88e6a70ae..9c3c72a7f9 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1884,6 +1884,14 @@ By default, Xen will use the INVPCID instruction for TLB 
management if
 it is available.  This option can be used to cause Xen to fall back to
 older mechanisms, which are generally slower.
 
+### load-balance-ratelimit
+> `= `
+
+The minimum interval between load balancing events on a given pcpu, in
+microseconds.  A value of '0' will disable rate limiting.  Maximum
+value 1 second. At the moment only credit honors this parameter.
+Default 1ms.
+
 ### noirqbalance (x86)
 > `= `
 
diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
index f2cd3d9da3..5c06f596d2 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -50,6 +50,10 @@
 #define CSCHED_TICKS_PER_TSLICE 3
 /* Default timeslice: 30ms */
 #define CSCHED_DEFAULT_TSLICE_MS30
+/* Default load balancing ratelimit: 1ms */
+#define CSCHED_DEFAULT_LOAD_BALANCE_RATELIMIT_US 1000
+/* Max load balancing ratelimit: 1s */
+#define CSCHED_MAX_LOAD_BALANCE_RATELIMIT_US 100
 #define CSCHED_CREDITS_PER_MSEC 10
 /* Never set a timer shorter than this value. */
 #define CSCHED_MIN_TIMERXEN_SYSCTL_SCHED_RATELIMIT_MIN
@@ -153,6 +157,7 @@ struct csched_pcpu {
 
 unsigned int idle_bias;
 unsigned int nr_runnable;
+s_time_t last_load_balance;
 
 unsigned int tick;
 struct timer ticker;
@@ -218,7 +223,7 @@ struct csched_private {
 
 /* Period of master and tick in milliseconds */
 unsigned int tick_period_us, ticks_per_tslice;
-s_time_t ratelimit, tslice, unit_migr_delay;
+s_time_t ratelimit, tslice, unit_migr_delay, load_balance_ratelimit;
 
 struct list_head active_sdom;
 uint32_t weight;
@@ -612,6 +617,8 @@ init_pdata(struct csched_private *prv, struct csched_pcpu 
*spc, int cpu)
 BUG_ON(!is_idle_unit(curr_on_cpu(cpu)));
 cpumask_set_cpu(cpu, prv->idlers);
 spc->nr_runnable = 0;
+
+spc->last_load_balance = NOW();
 }
 
 static void cf_check
@@ -1676,9 +1683,17 @@ csched_runq_steal(int peer_cpu, int cpu, int pri, int 
balance_step)
 return NULL;
 }
 
+/*
+ * Minimum delay, in microseconds, between load balance operations.
+ * This prevents spending too much time doing load balancing, particularly
+ * when the system has a high number of YIELDs due to spinlock priority 
inversion.
+ */
+static unsigned int __ro_after_init load_balance_ratelimit_us = 
CSCHED_DEFAULT_LOAD_BALANCE_RATELIMIT_US;
+integer_param("load-balance-ratelimit", load_balance_ratelimit_us);
+
 static struct csched_unit *
 csched_load_balance(struct csched_private *prv, int cpu,
-struct csched_unit *snext, bool *stolen)
+struct csched_unit *snext, bool *stolen)
 {
 const struct cpupool *c = get_sched_res(cpu)->cpupool;
 struct 

[PATCH v2 2/2] credit: Don't steal vcpus which have yielded

[George Dunlap]

On large systems with many vcpus yielding due to spinlock priority
inversion, it's not uncommon for a vcpu to yield its timeslice, only
to be immediately stolen by another pcpu looking for higher-priority
work.

To prevent this:

* Keep the YIELD flag until a vcpu is removed from a runqueue

* When looking for work to steal, skip vcpus which have yielded

NB that this does mean that sometimes a VM is inserted into an empty
runqueue; handle that case.

Signed-off-by: George Dunlap 
---
Changes since v1:
- Moved a comment tweak to the right patch

CC: Dario Faggioli 
---
 xen/common/sched/credit.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
index 5c06f596d2..38a6f6fa6d 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -298,14 +298,10 @@ __runq_insert(struct csched_unit *svc)
  * runnable unit if we can.  The next runq_sort will bring it forward
  * within 30ms if the queue too long. */
 if ( test_bit(CSCHED_FLAG_UNIT_YIELD, >flags)
- && __runq_elem(iter)->pri > CSCHED_PRI_IDLE )
-{
+ && __runq_elem(iter)->pri > CSCHED_PRI_IDLE
+ && iter->next != runq)
 iter=iter->next;
 
-/* Some sanity checks */
-BUG_ON(iter == runq);
-}
-
 list_add_tail(>runq_elem, iter);
 }
 
@@ -321,6 +317,11 @@ __runq_remove(struct csched_unit *svc)
 {
 BUG_ON( !__unit_on_runq(svc) );
 list_del_init(>runq_elem);
+
+/*
+ * Clear YIELD flag when scheduling back in
+ */
+clear_bit(CSCHED_FLAG_UNIT_YIELD, >flags);
 }
 
 static inline void
@@ -1637,6 +1638,13 @@ csched_runq_steal(int peer_cpu, int cpu, int pri, int 
balance_step)
 if ( speer->pri <= pri )
 break;
 
+/*
+ * Don't steal a UNIT which has yielded; it's waiting for a
+ * reason
+ */
+if (test_bit(CSCHED_FLAG_UNIT_YIELD, >flags))
+continue;
+
 /* Is this UNIT runnable on our PCPU? */
 unit = speer->unit;
 BUG_ON( is_idle_unit(unit) );
@@ -1954,11 +1962,6 @@ static void cf_check csched_schedule(
 dec_nr_runnable(sched_cpu);
 }
 
-/*
- * Clear YIELD flag before scheduling out
- */
-clear_bit(CSCHED_FLAG_UNIT_YIELD, >flags);
-
 do {
 snext = __runq_elem(runq->next);
 
-- 
2.25.1

[PATCH v3 4/7] block/dirty-bitmap: Clean up local variable shadowing

[Markus Armbruster]

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: rename both the pair of parameters and the pair of local
variables.  While there, move the local variables to function scope.

Suggested-by: Kevin Wolf 
Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/monitor/bitmap-qmp-cmds.c | 19 ++-
 block/qcow2-bitmap.c|  3 +--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 55f778f5af..70d01a3776 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -258,37 +258,38 @@ void qmp_block_dirty_bitmap_disable(const char *node, 
const char *name,
 bdrv_disable_dirty_bitmap(bitmap);
 }
 
-BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
+BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *dst_node,
+  const char *dst_bitmap,
   BlockDirtyBitmapOrStrList *bms,
   HBitmap **backup, Error **errp)
 {
 BlockDriverState *bs;
 BdrvDirtyBitmap *dst, *src;
 BlockDirtyBitmapOrStrList *lst;
+const char *src_node, *src_bitmap;
 HBitmap *local_backup = NULL;
 
 GLOBAL_STATE_CODE();
 
-dst = block_dirty_bitmap_lookup(node, target, , errp);
+dst = block_dirty_bitmap_lookup(dst_node, dst_bitmap, , errp);
 if (!dst) {
 return NULL;
 }
 
 for (lst = bms; lst; lst = lst->next) {
 switch (lst->value->type) {
-const char *name, *node;
 case QTYPE_QSTRING:
-name = lst->value->u.local;
-src = bdrv_find_dirty_bitmap(bs, name);
+src_bitmap = lst->value->u.local;
+src = bdrv_find_dirty_bitmap(bs, src_bitmap);
 if (!src) {
-error_setg(errp, "Dirty bitmap '%s' not found", name);
+error_setg(errp, "Dirty bitmap '%s' not found", src_bitmap);
 goto fail;
 }
 break;
 case QTYPE_QDICT:
-node = lst->value->u.external.node;
-name = lst->value->u.external.name;
-src = block_dirty_bitmap_lookup(node, name, NULL, errp);
+src_node = lst->value->u.external.node;
+src_bitmap = lst->value->u.external.name;
+src = block_dirty_bitmap_lookup(src_node, src_bitmap, NULL, errp);
 if (!src) {
 goto fail;
 }
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 037fa2d435..ffd5cd3b23 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1555,7 +1555,6 @@ bool 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 const char *name = bdrv_dirty_bitmap_name(bitmap);
 uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
-Qcow2Bitmap *bm;
 
 if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
 bdrv_dirty_bitmap_inconsistent(bitmap)) {
@@ -1625,7 +1624,7 @@ bool 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 
 /* allocate clusters and store bitmaps */
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
+bitmap = bm->dirty_bitmap;
 
 if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
 continue;
-- 
2.41.0

[PATCH v3 7/7] qobject atomics osdep: Make a few macros more hygienic

[Markus Armbruster]

Variables declared in macros can shadow other variables.  Much of the
time, this is harmless, e.g.:

#define _FDT(exp)  \
do {   \
int ret = (exp);   \
if (ret < 0) { \
error_report("error creating device tree: %s: %s",   \
#exp, fdt_strerror(ret));  \
exit(1);   \
}  \
} while (0)

Harmless shadowing in h_client_architecture_support():

target_ulong ret;

[...]

ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
if (ret == H_SUCCESS) {
_FDT((fdt_pack(spapr->fdt_blob)));
[...]
}

return ret;

However, we can get in trouble when the shadowed variable is used in a
macro argument:

#define QOBJECT(obj) ({ \
typeof(obj) o = (obj);  \
o ? container_of(&(o)->base, QObject, base) : NULL; \
 })

QOBJECT(o) expands into

({
--->typeof(o) o = (o);
o ? container_of(&(o)->base, QObject, base) : NULL;
})

Unintended variable name capture at --->.  We'd be saved by
-Winit-self.  But I could certainly construct more elaborate death
traps that don't trigger it.

To reduce the risk of trapping ourselves, we use variable names in
macros that no sane person would use elsewhere.  Here's our actual
definition of QOBJECT():

#define QOBJECT(obj) ({ \
typeof(obj) _obj = (obj);   \
_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
})

Works well enough until we nest macro calls.  For instance, with

#define qobject_ref(obj) ({ \
typeof(obj) _obj = (obj);   \
qobject_ref_impl(QOBJECT(_obj));\
_obj;   \
})

the expression qobject_ref(obj) expands into

({
typeof(obj) _obj = (obj);
qobject_ref_impl(
({
--->typeof(_obj) _obj = (_obj);
_obj ? container_of(&(_obj)->base, QObject, base) : NULL;
}));
_obj;
})

Unintended variable name capture at --->.

The only reliable way to prevent unintended variable name capture is
-Wshadow.

One blocker for enabling it is shadowing hiding in function-like
macros like

 qdict_put(dict, "name", qobject_ref(...))

qdict_put() wraps its last argument in QOBJECT(), and the last
argument here contains another QOBJECT().

Use dark preprocessor sorcery to make the macros that give us this
problem use different variable names on every call.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 include/qapi/qmp/qobject.h | 10 --
 include/qemu/atomic.h  | 17 -
 include/qemu/compiler.h|  3 +++
 include/qemu/osdep.h   | 27 ---
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 9003b71fd3..89b97d88bc 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -45,10 +45,16 @@ struct QObject {
 struct QObjectBase_ base;
 };
 
-#define QOBJECT(obj) ({ \
+/*
+ * Preprocessor sorcery ahead: use a different identifier for the
+ * local variable in each expansion, so we can nest macro calls
+ * without shadowing variables.
+ */
+#define QOBJECT_INTERNAL(obj, _obj) ({  \
 typeof(obj) _obj = (obj);   \
-_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
+_obj ? container_of(&_obj->base, QObject, base) : NULL; \
 })
+#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))
 
 /* Required for qobject_to() */
 #define QTYPE_CAST_TO_QNull QTYPE_QNULL
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index d95612f7a0..f1d3d1702a 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -157,13 +157,20 @@
 smp_read_barrier_depends();
 #endif
 
-#define qatomic_rcu_read(ptr)  \
-({ \
+/*
+ * Preprocessor sorcery ahead: use a different identifier for the
+ * local variable in each expansion, so we can nest macro calls
+ * without shadowing variables.
+ */
+#define qatomic_rcu_read_internal(ptr, _val)\
+({  \
 qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
-typeof_strip_qual(*ptr) _val;  \
-qatomic_rcu_read__nocheck(ptr, 

[PATCH v3 0/7] Steps towards enabling -Wshadow=local

[Markus Armbruster]

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Bugs love to hide in such code.
Evidence: PATCH 1.

Enabling -Wshadow would prevent bugs like this one.  But we'd have to
clean up all the offenders first.  We got a lot of them.

Enabling -Wshadow=local should be less work for almost as much gain.
I took a stab at it.  There's a small, exciting part, and a large,
boring part.

The exciting part is dark preprocessor sorcery to let us nest macro
calls without shadowing: PATCH 7.

The boring part is cleaning up all the other warnings.  I did some
[PATCH 2-6], but ran out of steam long before finishing the job.  Some
160 unique warnings remain.

To see them, enable -Wshadow=local like so:

diff --git a/meson.build b/meson.build
index 98e68ef0b1..9fc4c7ac9d 100644
--- a/meson.build
+++ b/meson.build
@@ -466,6 +466,9 @@ warn_flags = [
   '-Wno-tautological-type-limit-compare',
   '-Wno-psabi',
   '-Wno-gnu-variable-sized-type-not-at-end',
+  '-Wshadow=local',
+  '-Wno-error=shadow=local',
+  '-Wno-error=shadow=compatible-local',
 ]
 
 if targetos != 'darwin'

You may want to drop the -Wno-error lines.

v3:
* PATCH 7: Comment typo [Eric], peel off a pair of parenthesis [Eric],
  revert accidental line breaks [Kevin]

v2:
* PATCH 3+6: Mollify checkpatch
* PATCH 4: Redo for clearer code, R-bys dropped [Kevin]
* PATCH 5: Rename tweaked [Kevin]
* PATCH 6: Rename local @tran instead of the parameter [Kevin]
* PATCH 7: Drop PASTE(), use glue() instead [Richard]; pass
  identifiers instead of __COUNTER__ for readability [Eric]; add
  comments

Markus Armbruster (7):
  migration/rdma: Fix save_page method to fail on polling error
  migration: Clean up local variable shadowing
  ui: Clean up local variable shadowing
  block/dirty-bitmap: Clean up local variable shadowing
  block/vdi: Clean up local variable shadowing
  block: Clean up local variable shadowing
  qobject atomics osdep: Make a few macros more hygienic

 include/qapi/qmp/qobject.h  | 10 --
 include/qemu/atomic.h   | 17 +++-
 include/qemu/compiler.h |  3 +++
 include/qemu/osdep.h| 27 ++---
 block.c |  9 +
 block/monitor/bitmap-qmp-cmds.c | 19 +-
 block/qcow2-bitmap.c|  3 +--
 block/rbd.c |  2 +-
 block/stream.c  |  1 -
 block/vdi.c |  7 +++
 block/vvfat.c   | 35 +
 hw/block/xen-block.c|  6 +++---
 migration/block.c   |  4 ++--
 migration/ram.c |  8 +++-
 migration/rdma.c| 14 -
 migration/vmstate.c |  2 +-
 ui/gtk.c| 14 ++---
 ui/spice-display.c  |  9 +
 ui/vnc-palette.c|  2 --
 ui/vnc.c| 12 +--
 ui/vnc-enc-zrle.c.inc   |  9 -
 21 files changed, 121 insertions(+), 92 deletions(-)

-- 
2.41.0

[PATCH v3 1/7] migration/rdma: Fix save_page method to fail on polling error

[Markus Armbruster]

qemu_rdma_save_page() reports polling error with error_report(), then
succeeds anyway.  This is because the variable holding the polling
status *shadows* the variable the function returns.  The latter
remains zero.

Broken since day one, and duplicated more recently.

Fixes: 2da776db4846 (rdma: core logic)
Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid)
Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Reviewed-by: Peter Xu 
Reviewed-by: Li Zhijian 
---
 migration/rdma.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index a2a3db35b1..3915d1d7c9 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3282,7 +3282,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
  */
 while (1) {
 uint64_t wr_id, wr_id_in;
-int ret = qemu_rdma_poll(rdma, rdma->recv_cq, _id_in, NULL);
+ret = qemu_rdma_poll(rdma, rdma->recv_cq, _id_in, NULL);
+
 if (ret < 0) {
 error_report("rdma migration: polling error! %d", ret);
 goto err;
@@ -3297,7 +3298,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
 
 while (1) {
 uint64_t wr_id, wr_id_in;
-int ret = qemu_rdma_poll(rdma, rdma->send_cq, _id_in, NULL);
+ret = qemu_rdma_poll(rdma, rdma->send_cq, _id_in, NULL);
+
 if (ret < 0) {
 error_report("rdma migration: polling error! %d", ret);
 goto err;
-- 
2.41.0

[PATCH v3 2/7] migration: Clean up local variable shadowing

[Markus Armbruster]

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Xu 
Reviewed-by: Li Zhijian 
---
 migration/block.c   | 4 ++--
 migration/ram.c | 8 +++-
 migration/rdma.c| 8 +---
 migration/vmstate.c | 2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 86c2256a2b..eb6aafeb9e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -440,8 +440,8 @@ static int init_blk_migration(QEMUFile *f)
 /* Can only insert new BDSes now because doing so while iterating block
  * devices may end up in a deadlock (iterating the new BDSes, too). */
 for (i = 0; i < num_bs; i++) {
-BlkMigDevState *bmds = bmds_bs[i].bmds;
-BlockDriverState *bs = bmds_bs[i].bs;
+bmds = bmds_bs[i].bmds;
+bs = bmds_bs[i].bs;
 
 if (bmds) {
 ret = blk_insert_bs(bmds->blk, bs, _err);
diff --git a/migration/ram.c b/migration/ram.c
index 9040d66e61..0c202f8109 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3517,8 +3517,6 @@ int colo_init_ram_cache(void)
 * we use the same name 'ram_bitmap' as for migration.
 */
 if (ram_bytes_total()) {
-RAMBlock *block;
-
 RAMBLOCK_FOREACH_NOT_IGNORED(block) {
 unsigned long pages = block->max_length >> TARGET_PAGE_BITS;
 block->bmap = bitmap_new(pages);
@@ -3998,12 +3996,12 @@ static int ram_load_precopy(QEMUFile *f)
 }
 }
 if (migrate_ignore_shared()) {
-hwaddr addr = qemu_get_be64(f);
+hwaddr addr2 = qemu_get_be64(f);
 if (migrate_ram_is_ignored(block) &&
-block->mr->addr != addr) {
+block->mr->addr != addr2) {
 error_report("Mismatched GPAs for block %s "
  "%" PRId64 "!= %" PRId64,
- id, (uint64_t)addr,
+ id, (uint64_t)addr2,
  (uint64_t)block->mr->addr);
 ret = -EINVAL;
 }
diff --git a/migration/rdma.c b/migration/rdma.c
index 3915d1d7c9..c78ddfcb74 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1902,9 +1902,11 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, 
RDMAControlHeader *head,
  * by waiting for a READY message.
  */
 if (rdma->control_ready_expected) {
-RDMAControlHeader resp;
-ret = qemu_rdma_exchange_get_response(rdma,
-, RDMA_CONTROL_READY, 
RDMA_WRID_READY);
+RDMAControlHeader resp_ignored;
+
+ret = qemu_rdma_exchange_get_response(rdma, _ignored,
+  RDMA_CONTROL_READY,
+  RDMA_WRID_READY);
 if (ret < 0) {
 return ret;
 }
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..438ea77cfa 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -97,7 +97,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription 
*vmsd,
 return -EINVAL;
 }
 if (vmsd->pre_load) {
-int ret = vmsd->pre_load(opaque);
+ret = vmsd->pre_load(opaque);
 if (ret) {
 return ret;
 }
-- 
2.41.0

[PATCH v3 3/7] ui: Clean up local variable shadowing

[Markus Armbruster]

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Peter Maydell 
---
 ui/gtk.c  | 14 +++---
 ui/spice-display.c|  9 +
 ui/vnc-palette.c  |  2 --
 ui/vnc.c  | 12 ++--
 ui/vnc-enc-zrle.c.inc |  9 -
 5 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index e09f97a86b..3373427c9b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -930,8 +930,8 @@ static gboolean gd_motion_event(GtkWidget *widget, 
GdkEventMotion *motion,
 GdkMonitor *monitor = gdk_display_get_monitor_at_window(dpy, win);
 GdkRectangle geometry;
 
-int x = (int)motion->x_root;
-int y = (int)motion->y_root;
+int xr = (int)motion->x_root;
+int yr = (int)motion->y_root;
 
 gdk_monitor_get_geometry(monitor, );
 
@@ -942,13 +942,13 @@ static gboolean gd_motion_event(GtkWidget *widget, 
GdkEventMotion *motion,
  * may still be only half way across the screen. Without
  * this warp, the server pointer would thus appear to hit
  * an invisible wall */
-if (x <= geometry.x || x - geometry.x >= geometry.width - 1 ||
-y <= geometry.y || y - geometry.y >= geometry.height - 1) {
+if (xr <= geometry.x || xr - geometry.x >= geometry.width - 1 ||
+yr <= geometry.y || yr - geometry.y >= geometry.height - 1) {
 GdkDevice *dev = gdk_event_get_device((GdkEvent *)motion);
-x = geometry.x + geometry.width / 2;
-y = geometry.y + geometry.height / 2;
+xr = geometry.x + geometry.width / 2;
+yr = geometry.y + geometry.height / 2;
 
-gdk_device_warp(dev, screen, x, y);
+gdk_device_warp(dev, screen, xr, yr);
 s->last_set = FALSE;
 return FALSE;
 }
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 5cc47bd668..6eb98a5a5c 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1081,15 +1081,16 @@ static void qemu_spice_gl_update(DisplayChangeListener 
*dcl,
 }
 
 if (render_cursor) {
-int x, y;
+int ptr_x, ptr_y;
+
 qemu_mutex_lock(>lock);
-x = ssd->ptr_x;
-y = ssd->ptr_y;
+ptr_x = ssd->ptr_x;
+ptr_y = ssd->ptr_y;
 qemu_mutex_unlock(>lock);
 egl_texture_blit(ssd->gls, >blit_fb, >guest_fb,
  !y_0_top);
 egl_texture_blend(ssd->gls, >blit_fb, >cursor_fb,
-  !y_0_top, x, y, 1.0, 1.0);
+  !y_0_top, ptr_x, ptr_y, 1.0, 1.0);
 glFlush();
 }
 
diff --git a/ui/vnc-palette.c b/ui/vnc-palette.c
index dc7c0ba997..4e88c412f0 100644
--- a/ui/vnc-palette.c
+++ b/ui/vnc-palette.c
@@ -86,8 +86,6 @@ int palette_put(VncPalette *palette, uint32_t color)
 return 0;
 }
 if (!entry) {
-VncPaletteEntry *entry;
-
 entry = >pool[palette->size];
 entry->color = color;
 entry->idx = idx;
diff --git a/ui/vnc.c b/ui/vnc.c
index 6fd86996a5..ecb75ff8c8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1584,15 +1584,15 @@ static void vnc_jobs_bh(void *opaque)
  */
 static int vnc_client_read(VncState *vs)
 {
-size_t ret;
+size_t sz;
 
 #ifdef CONFIG_VNC_SASL
 if (vs->sasl.conn && vs->sasl.runSSF)
-ret = vnc_client_read_sasl(vs);
+sz = vnc_client_read_sasl(vs);
 else
 #endif /* CONFIG_VNC_SASL */
-ret = vnc_client_read_plain(vs);
-if (!ret) {
+sz = vnc_client_read_plain(vs);
+if (!sz) {
 if (vs->disconnecting) {
 vnc_disconnect_finish(vs);
 return -1;
@@ -3118,8 +3118,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
 cmp_bytes = MIN(VNC_DIRTY_PIXELS_PER_BIT * VNC_SERVER_FB_BYTES,
 server_stride);
 if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
-int width = pixman_image_get_width(vd->server);
-tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, width);
+int w = pixman_image_get_width(vd->server);
+tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, w);
 } else {
 int guest_bpp =
 PIXMAN_FORMAT_BPP(pixman_image_get_format(vd->guest.fb));
diff --git a/ui/vnc-enc-zrle.c.inc b/ui/vnc-enc-zrle.c.inc
index a8ca37d05e..2ef7501d52 100644
--- a/ui/vnc-enc-zrle.c.inc
+++ b/ui/vnc-enc-zrle.c.inc
@@ -153,11 +153,12 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL 
*data, int w, int h,
 }
 
 if (use_rle) {
-ZRLE_PIXEL *ptr = data;
-ZRLE_PIXEL *end = ptr + w * h;
 ZRLE_PIXEL *run_start;
 ZRLE_PIXEL pix;
 
+ptr = data;
+end = ptr + w * h;
+
 while (ptr < end) {
 int len;
 

[PATCH v3 6/7] block: Clean up local variable shadowing

[Markus Armbruster]

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Stefan Hajnoczi 
Acked-by: Anthony PERARD 
Acked-by: Ilya Dryomov 
Reviewed-by: Kevin Wolf 
---
 block.c  |  9 +
 block/rbd.c  |  2 +-
 block/stream.c   |  1 -
 block/vvfat.c| 35 ++-
 hw/block/xen-block.c |  6 +++---
 5 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 8da89aaa62..bb5dd17e9c 100644
--- a/block.c
+++ b/block.c
@@ -3035,18 +3035,19 @@ static BdrvChild 
*bdrv_attach_child_common(BlockDriverState *child_bs,
   _err);
 
 if (ret < 0 && child_class->change_aio_ctx) {
-Transaction *tran = tran_new();
+Transaction *aio_ctx_tran = tran_new();
 GHashTable *visited = g_hash_table_new(NULL, NULL);
 bool ret_child;
 
 g_hash_table_add(visited, new_child);
 ret_child = child_class->change_aio_ctx(new_child, child_ctx,
-visited, tran, NULL);
+visited, aio_ctx_tran,
+NULL);
 if (ret_child == true) {
 error_free(local_err);
 ret = 0;
 }
-tran_finalize(tran, ret_child == true ? 0 : -1);
+tran_finalize(aio_ctx_tran, ret_child == true ? 0 : -1);
 g_hash_table_destroy(visited);
 }
 
@@ -6077,12 +6078,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const 
char *name),
 QLIST_FOREACH(drv, _drivers, list) {
 if (drv->format_name) {
 bool found = false;
-int i = count;
 
 if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, read_only)) {
 continue;
 }
 
+i = count;
 while (formats && i && !found) {
 found = !strcmp(formats[--i], drv->format_name);
 }
diff --git a/block/rbd.c b/block/rbd.c
index 978671411e..472ca05cba 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1290,7 +1290,7 @@ static int coroutine_fn 
qemu_rbd_start_co(BlockDriverState *bs,
  * operations that exceed the current size.
  */
 if (offset + bytes > s->image_size) {
-int r = qemu_rbd_resize(bs, offset + bytes);
+r = qemu_rbd_resize(bs, offset + bytes);
 if (r < 0) {
 return r;
 }
diff --git a/block/stream.c b/block/stream.c
index e522bbdec5..007253880b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -282,7 +282,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 /* Make sure that the image is opened in read-write mode */
 bs_read_only = bdrv_is_read_only(bs);
 if (bs_read_only) {
-int ret;
 /* Hold the chain during reopen */
 if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
 return;
diff --git a/block/vvfat.c b/block/vvfat.c
index 0ddc91fc09..856b479c91 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -777,7 +777,6 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 while((entry=readdir(dir))) {
 unsigned int length=strlen(dirname)+2+strlen(entry->d_name);
 char* buffer;
-direntry_t* direntry;
 struct stat st;
 int is_dot=!strcmp(entry->d_name,".");
 int is_dotdot=!strcmp(entry->d_name,"..");
@@ -857,7 +856,7 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 
 /* fill with zeroes up to the end of the cluster */
 while(s->directory.next%(0x10*s->sectors_per_cluster)) {
-direntry_t* direntry=array_get_next(&(s->directory));
+direntry = array_get_next(&(s->directory));
 memset(direntry,0,sizeof(direntry_t));
 }
 
@@ -1962,24 +1961,24 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, 
direntry_t* direntry, const ch
  * This is horribly inefficient, but that is okay, since
  * it is rarely executed, if at all.
  */
-int64_t offset = cluster2sector(s, cluster_num);
+int64_t offs = cluster2sector(s, cluster_num);
 
 vvfat_close_current_file(s);
 for (i = 0; i < s->sectors_per_cluster; i++) {
 int res;
 
 res = bdrv_is_allocated(s->qcow->bs,
-(offset + i) * BDRV_SECTOR_SIZE,
+(offs + i) * BDRV_SECTOR_SIZE,
 BDRV_SECTOR_SIZE, NULL);
 if (res < 0) {
 return 

[PATCH v3 5/7] block/vdi: Clean up local variable shadowing

[Markus Armbruster]

Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Kevin Wolf 
---
 block/vdi.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6c35309e04..934e1b849b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -634,7 +634,6 @@ vdi_co_pwritev(BlockDriverState *bs, int64_t offset, 
int64_t bytes,
 bmap_entry = le32_to_cpu(s->bmap[block_index]);
 if (!VDI_IS_ALLOCATED(bmap_entry)) {
 /* Allocate new block and write to it. */
-uint64_t data_offset;
 qemu_co_rwlock_upgrade(>bmap_lock);
 bmap_entry = le32_to_cpu(s->bmap[block_index]);
 if (VDI_IS_ALLOCATED(bmap_entry)) {
@@ -700,7 +699,7 @@ nonallocating_write:
 /* One or more new blocks were allocated. */
 VdiHeader *header;
 uint8_t *base;
-uint64_t offset;
+uint64_t bmap_offset;
 uint32_t n_sectors;
 
 g_free(block);
@@ -723,11 +722,11 @@ nonallocating_write:
 bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
 bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
 n_sectors = bmap_last - bmap_first + 1;
-offset = s->bmap_sector + bmap_first;
+bmap_offset = s->bmap_sector + bmap_first;
 base = ((uint8_t *)>bmap[0]) + bmap_first * SECTOR_SIZE;
 logout("will write %u block map sectors starting from entry %u\n",
n_sectors, bmap_first);
-ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
+ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
  n_sectors * SECTOR_SIZE, base, 0);
 }
 
-- 
2.41.0

[XEN PATCH v3] xen/hypercalls: address violations of MISRA C:2012 Rule 8.3

[Federico Serafini]

Make function declarations and definitions consistent.
No functional change.

Signed-off-by: Federico Serafini 
---
Changes in v3:
- removed changes involving types with fixed/unfixed width;
- uniformed parameter names of compat_memory_op() with {do,hvm}_memory_op().
---
Changes in v2:
- change compat_grant_table_op() definition instead of the declaration;
- use unsigned int for multicall()'s parameter in accordance with XEN
  coding style.
---
---
 xen/common/compat/grant_table.c | 22 +-
 xen/common/compat/memory.c  | 40 -
 2 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index f8177c84c0..e00bc24a34 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -57,7 +57,7 @@ CHECK_gnttab_cache_flush;
 #undef xen_gnttab_cache_flush
 
 int compat_grant_table_op(
-unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) cmp_uop, unsigned int count)
+unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
 {
 int rc = 0;
 unsigned int i, cmd_op;
@@ -71,7 +71,7 @@ int compat_grant_table_op(
 {
 #define CASE(name) \
 case GNTTABOP_##name: \
-if ( unlikely(!guest_handle_okay(guest_handle_cast(cmp_uop, \
+if ( unlikely(!guest_handle_okay(guest_handle_cast(uop, \

gnttab_##name##_compat_t), \
  count)) ) \
 rc = -EFAULT; \
@@ -119,7 +119,7 @@ int compat_grant_table_op(
 
 #undef CASE
 default:
-return do_grant_table_op(cmd, cmp_uop, count);
+return do_grant_table_op(cmd, uop, count);
 }
 
 if ( (int)count < 0 )
@@ -148,7 +148,7 @@ int compat_grant_table_op(
 case GNTTABOP_setup_table:
 if ( unlikely(count > 1) )
 rc = -EINVAL;
-else if ( unlikely(__copy_from_guest(, cmp_uop, 1)) )
+else if ( unlikely(__copy_from_guest(, uop, 1)) )
 rc = -EFAULT;
 else if ( unlikely(!compat_handle_okay(cmp.setup.frame_list, 
cmp.setup.nr_frames)) )
 rc = -EFAULT;
@@ -193,7 +193,7 @@ int compat_grant_table_op(
 } while (0)
 XLAT_gnttab_setup_table(, nat.setup);
 #undef XLAT_gnttab_setup_table_HNDL_frame_list
-if ( unlikely(__copy_to_guest(cmp_uop, , 1)) )
+if ( unlikely(__copy_to_guest(uop, , 1)) )
 rc = -EFAULT;
 else
 i = 1;
@@ -203,7 +203,7 @@ int compat_grant_table_op(
 case GNTTABOP_transfer:
 for ( n = 0; n < COMPAT_ARG_XLAT_SIZE / sizeof(*nat.xfer) && i < 
count && rc == 0; ++i, ++n )
 {
-if ( unlikely(__copy_from_guest_offset(, cmp_uop, i, 
1)) )
+if ( unlikely(__copy_from_guest_offset(, uop, i, 1)) )
 rc = -EFAULT;
 else
 {
@@ -222,7 +222,7 @@ int compat_grant_table_op(
 {
 XEN_GUEST_HANDLE_PARAM(gnttab_transfer_compat_t) xfer;
 
-xfer = guest_handle_cast(cmp_uop, gnttab_transfer_compat_t);
+xfer = guest_handle_cast(uop, gnttab_transfer_compat_t);
 guest_handle_add_offset(xfer, i);
 cnt_uop = guest_handle_cast(xfer, void);
 while ( n-- )
@@ -237,7 +237,7 @@ int compat_grant_table_op(
 case GNTTABOP_copy:
 for ( n = 0; n < COMPAT_ARG_XLAT_SIZE / sizeof(*nat.copy) && i < 
count && rc == 0; ++i, ++n )
 {
-if ( unlikely(__copy_from_guest_offset(, cmp_uop, i, 
1)) )
+if ( unlikely(__copy_from_guest_offset(, uop, i, 1)) )
 rc = -EFAULT;
 else
 {
@@ -267,7 +267,7 @@ int compat_grant_table_op(
 {
 XEN_GUEST_HANDLE_PARAM(gnttab_copy_compat_t) copy;
 
-copy = guest_handle_cast(cmp_uop, gnttab_copy_compat_t);
+copy = guest_handle_cast(uop, gnttab_copy_compat_t);
 guest_handle_add_offset(copy, i);
 cnt_uop = guest_handle_cast(copy, void);
 while ( n-- )
@@ -285,7 +285,7 @@ int compat_grant_table_op(
 rc = -EINVAL;
 break;
 }
-if ( unlikely(__copy_from_guest(_status, cmp_uop, 1) ||
+if ( unlikely(__copy_from_guest(_status, uop, 1) ||
   !compat_handle_okay(cmp.get_status.frame_list,
   cmp.get_status.nr_frames)) )
 {
@@ -303,7 +303,7 @@ int compat_grant_table_op(
 if ( rc >= 0 )
 {
 XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_compat_t) get =
-guest_handle_cast(cmp_uop,
+guest_handle_cast(uop,
   

Re: [PATCH v10 33/38] x86/entry: Add fred_entry_from_kvm() for VMX to handle IRQ/NMI

[Nikolay Borisov]

On 14.09.23 г. 7:48 ч., Xin Li wrote:

In IRQ/NMI induced VM exits, KVM VMX needs to execute the respective
handlers, which requires the software to create a FRED stack frame,
and use it to invoke the handlers. Add fred_irq_entry_from_kvm() for
this job.

Export fred_entry_from_kvm() because VMX can be compiled as a module.

Suggested-by: Sean Christopherson 
Tested-by: Shan Kang 
Signed-off-by: Thomas Gleixner 
Signed-off-by: Xin Li 
---

Changes since v9:
* Shove the whole thing into arch/x86/entry/entry_64_fred.S for invoking
   external_interrupt() and fred_exc_nmi() (Sean Christopherson).
* Correct and improve a few comments (Sean Christopherson).
* Merge the two IRQ/NMI asm entries into one as it's fine to invoke
   noinstr code from regular code (Thomas Gleixner).
* Setup the long mode and NMI flags in the augmented SS field of FRED
   stack frame in C instead of asm (Thomas Gleixner).
* Add UNWIND_HINT_{SAVE,RESTORE} to get rid of the warning: "objtool:
   asm_fred_entry_from_kvm+0x0: unreachable instruction" (Peter Zijlstra).

Changes since v8:
* Add a new macro VMX_DO_FRED_EVENT_IRQOFF for FRED instead of
   refactoring VMX_DO_EVENT_IRQOFF (Sean Christopherson).
* Do NOT use a trampoline, just LEA+PUSH the return RIP, PUSH the error
   code, and jump to the FRED kernel entry point for NMI or call
   external_interrupt() for IRQs (Sean Christopherson).
* Call external_interrupt() only when FRED is enabled, and convert the
   non-FRED handling to external_interrupt() after FRED lands (Sean
   Christopherson).
---
  arch/x86/entry/entry_64_fred.S | 73 ++
  arch/x86/entry/entry_fred.c| 14 +++
  arch/x86/include/asm/fred.h| 18 +
  3 files changed, 105 insertions(+)

diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
index d1c2fc4af8ae..f1088d6f2054 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -4,7 +4,9 @@
   */
  
  #include 

+#include 
  #include 
+#include 
  
  #include "calling.h"
  
@@ -54,3 +56,74 @@ SYM_CODE_START_NOALIGN(asm_fred_entrypoint_kernel)

FRED_EXIT
ERETS
  SYM_CODE_END(asm_fred_entrypoint_kernel)
+
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+SYM_FUNC_START(asm_fred_entry_from_kvm)
+   push %rbp
+   mov %rsp, %rbp


use FRAME_BEGIN/FRAME_END macros to ommit this code if 
CONFIG_FRAME_POINTER is disabled.



+
+   UNWIND_HINT_SAVE
+
+   /*
+* Don't check the FRED stack level, the call stack leading to this
+* helper is effectively constant and shallow (relatively speaking).
+*
+* Emulate the FRED-defined redzone and stack alignment.
+*/
+   sub $(FRED_CONFIG_REDZONE_AMOUNT << 6), %rsp
+   and $FRED_STACK_FRAME_RSP_MASK, %rsp
+
+   /*
+* Start to push a FRED stack frame, which is always 64 bytes:
+*
+* ++-+
+* | Bytes  | Usage   |
+* ++-+
+* | 63:56  | Reserved|
+* | 55:48  | Event Data  |
+* | 47:40  | SS + Event Info |
+* | 39:32  | RSP |
+* | 31:24  | RFLAGS  |
+* | 23:16  | CS + Aux Info   |
+* |  15:8  | RIP |
+* |   7:0  | Error Code  |
+* ++-+
+*/
+   push $0 /* Reserved, must be 0 */
+   push $0 /* Event data, 0 for IRQ/NMI */
+   push %rdi   /* fred_ss handed in by the caller */
+   push %rbp
+   pushf
+   mov $__KERNEL_CS, %rax
+   push %rax
+
+   /*
+* Unlike the IDT event delivery, FRED _always_ pushes an error code
+* after pushing the return RIP, thus the CALL instruction CANNOT be
+* used here to push the return RIP, otherwise there is no chance to
+* push an error code before invoking the IRQ/NMI handler.
+*
+* Use LEA to get the return RIP and push it, then push an error code.
+*/
+   lea 1f(%rip), %rax
+   push %rax   /* Return RIP */
+   push $0 /* Error code, 0 for IRQ/NMI */
+
+   PUSH_AND_CLEAR_REGS clear_bp=0 unwind_hint=0
+   movq %rsp, %rdi /* %rdi -> pt_regs */
+   call __fred_entry_from_kvm  /* Call the C entry point */
+   POP_REGS
+   ERETS
+1:
+   /*
+* Objtool doesn't understand what ERETS does, this hint tells it that
+* yes, we'll reach here and with what stack state. A save/restore pair
+* isn't strictly needed, but it's the simplest form.
+*/
+   UNWIND_HINT_RESTORE
+   pop %rbp


FRAME_END


+   RET
+
+SYM_FUNC_END(asm_fred_entry_from_kvm)
+EXPORT_SYMBOL_GPL(asm_fred_entry_from_kvm);
+#endif

Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic

[Markus Armbruster]

Kevin Wolf  writes:

> Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:

[...]

>> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
>> index 9003b71fd3..d36cc97805 100644
>> --- a/include/qapi/qmp/qobject.h
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -45,10 +45,17 @@ struct QObject {
>>  struct QObjectBase_ base;
>>  };
>>  
>> -#define QOBJECT(obj) ({ \
>> +/*
>> + * Preprocessory sorcery ahead: use a different identifier for the
>> + * local variable in each expansion, so we can nest macro calls
>> + * without shadowing variables.
>> + */
>> +#define QOBJECT_INTERNAL(obj, _obj) ({  \
>>  typeof(obj) _obj = (obj);   \
>> -_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
>> +_obj\
>> +? container_of(&(_obj)->base, QObject, base) : NULL;\
>
> What happened here? The code in this line (or now two lines) seems to be
> unchanged apart from a strange looking newline.

Accident, will fix, thanks!

>>  })
>> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))
>
> Kevin

Re: xen | Failed pipeline for staging-4.15 | 3a9a2901

[Jan Beulich]

On 21.09.2023 10:30, Andrew Cooper wrote:
> On 21/09/2023 8:08 am, Jan Beulich wrote:
>> On 20.09.2023 12:50, GitLab wrote:
>>> Job #5127621916 ( https://gitlab.com/xen-project/xen/-/jobs/5127621916/raw )
>>>
>>> Stage: build
>>> Name: debian-stretch-gcc
>> This one failed with
>>
>> In file included from 
>> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/timer.h:4:0,
>>  from 
>> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/timed-average.h:29,
>>  from 
>> /builds/xen-project/xen/tools/qemu-xen-dir/include/block/accounting.h:28,
>>  from 
>> /builds/xen-project/xen/tools/qemu-xen-dir/include/block/block_int.h:27,
>>  from 
>> /builds/xen-project/xen/tools/qemu-xen-dir/block/file-posix.c:30:
>> /usr/include/linux/swab.h: In function '__swab':
>> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/bitops.h:20:34: 
>> error: "sizeof" is not defined [-Werror=undef]
>>  #define BITS_PER_LONG   (sizeof (unsigned long) * BITS_PER_BYTE)
>>   ^
>> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/bitops.h:20:41: 
>> error: missing binary operator before token "("
>>  #define BITS_PER_LONG   (sizeof (unsigned long) * BITS_PER_BYTE)
>>  ^
>> cc1: all warnings being treated as errors
>>
>> What? How can "sizeof" no be "defined"?
> 
> Because the expression is
> 
> #if (sizeof (unsigned long) * BITS_PER_BYTE) = 64
> 
> combined with -Werror=undef saying that it can't figure out out what
> sizeof is supposed to be here as a preprocessor token.

I still don't get it, I'm afraid. Qemu only #define-s BITS_PER_LONG this
way, while __BITS_PER_LONG is a plain constant. Yet it's the latter that
Linux'es swab.h uses in the #if (from its introduction in 5.6). Hence ...

> This needs a bump to qemu in order to fix, like we did on the newer
> branches.

... I can't see yet how doing so would help.

Jan

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

[osstest service owner]

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

Failures :-/ but no regressions.

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

version targeted for testing:
 xen  9b57c800b79b96769ea3dcd6468578fa664d19f9
baseline version:
 xen  2ea38251eb67639be7aa9d7b64084b1be0230273

Last test of basis   183046  2023-09-19 04:03:13 Z2 days
Testing same since   183074  2023-09-19 22:37:00 Z1 days2 attempts


People who touched revisions under test:
  Andrew Cooper 
  Federico Serafini 
  Jan Beulich 
  

Re: [PATCH v9 09/16] vpci/header: program p2m with guest BAR view

[Roger Pau Monné]

On Tue, Aug 29, 2023 at 11:19:44PM +, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko 
> 
> Take into account guest's BAR view and program its p2m accordingly:
> gfn is guest's view of the BAR and mfn is the physical BAR value.
> This way hardware domain sees physical BAR values and guest sees
> emulated ones.
> 
> Hardware domain continues getting the BARs identity mapped, while for
> domUs the BARs are mapped at the requested guest address without
> modifying the BAR address in the device PCI config space.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> Signed-off-by: Volodymyr Babchuk 
> ---
> Since v9:
> - Extended the commit message
> - Use bar->guest_addr in modify_bars
> - Extended printk error message in map_range
> - Moved map_data initialization so .bar can be initialized during declaration
> Since v5:
> - remove debug print in map_range callback
> - remove "identity" from the debug print
> Since v4:
> - moved start_{gfn|mfn} calculation into map_range
> - pass vpci_bar in the map_data instead of start_{gfn|mfn}
> - s/guest_addr/guest_reg
> Since v3:
> - updated comment (Roger)
> - removed gfn_add(map->start_gfn, rc); which is wrong
> - use v->domain instead of v->vpci.pdev->domain
> - removed odd e.g. in comment
> - s/d%d/%pd in altered code
> - use gdprintk for map/unmap logs
> Since v2:
> - improve readability for data.start_gfn and restructure ?: construct
> Since v1:
>  - s/MSI/MSI-X in comments
> ---
>  xen/drivers/vpci/header.c | 52 ---
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 3cc6a96849..1e82217200 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -33,6 +33,7 @@
>  
>  struct map_data {
>  struct domain *d;
> +const struct vpci_bar *bar;
>  bool map;
>  };
>  
> @@ -44,6 +45,12 @@ static int cf_check map_range(
>  
>  for ( ; ; )
>  {
> +/* Start address of the BAR as seen by the guest. */
> +gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d)
> +? map->bar->addr
> +: map->bar->guest_addr));
> +/* Physical start address of the BAR. */
> +mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr));

Both of those should be declared outside of the loop, as there's no
need to (re)calculate them at each iteration.

Also start_gfn likely wants to be unsigned long?  All the usages of it
in the patch convert it to integer by using gfn_x().

>  unsigned long size = e - s + 1;
>  
>  if ( !iomem_access_permitted(map->d, s, e) )
> @@ -63,6 +70,13 @@ static int cf_check map_range(
>  return rc;
>  }
>  
> +/*
> + * Ranges to be mapped don't always start at the BAR start address, 
> as
> + * there can be holes or partially consumed ranges. Account for the
> + * offset of the current address from the BAR start.
> + */
> +start_mfn = mfn_add(start_mfn, s - gfn_x(start_gfn));

This should then be a local loop variable with a different name.

> +
>  /*
>   * ARM TODOs:
>   * - On ARM whether the memory is prefetchable or not should be 
> passed
> @@ -72,8 +86,8 @@ static int cf_check map_range(
>   * - {un}map_mmio_regions doesn't support preemption.
>   */
>  
> -rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
> -  : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
> +rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, start_mfn)
> +  : unmap_mmio_regions(map->d, _gfn(s), size, start_mfn);
>  if ( rc == 0 )
>  {
>  *c += size;
> @@ -82,8 +96,9 @@ static int cf_check map_range(
>  if ( rc < 0 )
>  {
>  printk(XENLOG_G_WARNING
> -   "Failed to identity %smap [%lx, %lx] for d%d: %d\n",
> -   map->map ? "" : "un", s, e, map->d->domain_id, rc);
> +   "Failed to %smap [%lx (%lx), %lx (%lx)] for %pd: %d\n",

I think we would usually write such mapping messages as:

[start gfn, end gfn] -> [start mfn, end mfn]

So:

"Failed to %smap [%lx, %lx] -> [%lx, %lx] for %pd: %d\n"

> +   map->map ? "" : "un", s,  mfn_x(start_mfn), e,
> +   mfn_x(start_mfn) + size, map->d, rc);
>  break;
>  }
>  ASSERT(rc < size);
> @@ -162,10 +177,6 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>  bool vpci_process_pending(struct vcpu *v)
>  {
>  struct pci_dev *pdev = v->vpci.pdev;
> -struct map_data data = {
> -.d = v->domain,
> -.map = v->vpci.cmd & PCI_COMMAND_MEMORY,
> -};
>  struct vpci_header *header = NULL;
>  unsigned int i;
>  
> @@ -177,6 +188,11 @@ bool vpci_process_pending(struct vcpu *v)
>  for ( i = 

Re: [XEN PATCH v3 4/4] README: remove old note about the build system's python expectation

[Andrew Cooper]

On 21/09/2023 6:59 am, Jan Beulich wrote:
> On 20.09.2023 21:40, Andrew Cooper wrote:
>> On 19/09/2023 7:50 am, Jan Beulich wrote:
>>> On 19.09.2023 08:30, Javi Merino wrote:
 5852ca485263 (build: fix tools/configure in case only python3 exists,
 2019-12-11) changed the configure script to test for the availability
 of python3, python and python2 in that order.  It sets PYTHON to the
 first one found in path.  You don't need to have a symlink to python.

 Remove the outdated note from the README.
>>> But that note also covers the hypervisor build, which isn't affected by
>>> ./configure, and even less so by tools/configure.
>> The hypervisor build equally doesn't python= specifying.
> I'm pretty sure that on at least one of my systems I need PYTHON=python3
> when building the hypervisor.

Not since c8a8645f1efe you haven't.

~Andrew

Re: [PATCH v10 28/38] x86/fred: FRED entry/exit and dispatch code

[Thomas Gleixner]

On Thu, Sep 21 2023 at 12:48, Nikolay Borisov wrote:
> On 14.09.23 г. 7:47 ч., Xin Li wrote:
>> +
>> +/* INT80 */
>> +case IA32_SYSCALL_VECTOR:
>> +if (likely(IS_ENABLED(CONFIG_IA32_EMULATION))) {
>
> Since future kernels will support boottime toggling of whether 32bit 
> syscall interface should be enabled or not as per:
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/entry=1da5c9bc119d3a749b519596b93f9b2667e93c4a
>
> It will make more sense to replace this with ia32_enabled() invocation. 
> I guess this could be done as a follow-up patch based on when this is 
> merged as the ia32_enbaled changes are going to be merged in 6.7.

The simplest solution is to rebase the series to tip x86/entry and just
do it right away :)

Re: [PATCH for-4.18 v2] tools/light: Revoke permissions when a PCI detach for HVM domain

[Julien Grall]

Hi,

On 16/09/2023 01:11, Henry Wang wrote:

On Sep 15, 2023, at 20:52, Julien Grall  wrote:

From: Julien Grall 

Currently, libxl will grant IOMEM, I/O port and IRQ permissions when
a PCI is attached (see pci_add_dm_done()) for all domain types. However,
the permissions are only revoked for non-HVM domain (see do_pci_remove()).

This means that HVM domains will be left with extra permissions. While
this look bad on the paper, the IRQ permissions should be revoked
when the Device Model call xc_physdev_unmap_pirq() and such domain
cannot directly mapped I/O port and IOMEM regions. Instead, this has to
be done by a Device Model.

The Device Model can only run in dom0 or PV stubdomain (upstream libxl
doesn't have support for HVM/PVH stubdomain).

For PV/PVH stubdomain, the permission are properly revoked, so there is
no security concern.

This leaves dom0. There are two cases:
  1) Privileged: Anyone gaining access to the Device Model would already
 have large control on the host.
  2) Deprivileged: PCI passthrough require PHYSDEV operations which
 are not accessible when the Device Model is restricted.

So overall, it is believed that the extra permissions cannot be exploited.

Rework the code so the permissions are all removed for HVM domains.
This needs to happen after the QEMU has detached the device. So
the revocation is now moved to pci_remove_detached().

Also add a comment on top of the error message when the PIRQ cannot
be unbind to explain this could be a spurious error as QEMU may have
already done it.

Signed-off-by: Julien Grall 


As in discussion in v1, it is agreed that this patch should be included in
4.18, although technically my release-ack tag should be effective after
code freeze, I am still providing the tag to avoid possible confusion:

Release-acked-by: Henry Wang 


Thanks. I have committed the patch with Anthony's reviewed-by tag.

Cheers,

--
Julien Grall

Re: [PATCH v10 28/38] x86/fred: FRED entry/exit and dispatch code

[Nikolay Borisov]

On 14.09.23 г. 7:47 ч., Xin Li wrote:

From: "H. Peter Anvin (Intel)" 

The code to actually handle kernel and event entry/exit using
FRED. It is split up into two files thus:

- entry_64_fred.S contains the actual entrypoints and exit code, and
   saves and restores registers.
- entry_fred.c contains the two-level event dispatch code for FRED.
   The first-level dispatch is on the event type, and the second-level
   is on the event vector.

Originally-by: Megha Dey 
Signed-off-by: H. Peter Anvin (Intel) 
Co-developed-by: Xin Li 
Tested-by: Shan Kang 
Signed-off-by: Thomas Gleixner 
Signed-off-by: Xin Li 
---

Changes since v9:
* Don't use jump tables, indirect jumps are expensive (Thomas Gleixner).
* Except NMI/#DB/#MCE, FRED really can share the exception handlers
   with IDT (Thomas Gleixner).
* Avoid the sysvec_* idt_entry muck, do it at a central place, reuse code
   instead of blindly copying it, which breaks the performance optimized
   sysvec entries like reschedule_ipi (Thomas Gleixner).
* Add asm_ prefix to FRED asm entry points (Thomas Gleixner).

Changes since v8:
* Don't do syscall early out in fred_entry_from_user() before there are
   proper performance numbers and justifications (Thomas Gleixner).
* Add the control exception handler to the FRED exception handler table
   (Thomas Gleixner).
* Add ENDBR to the FRED_ENTER asm macro.
* Reflect the FRED spec 5.0 change that ERETS and ERETU add 8 to %rsp
   before popping the return context from the stack.

Changes since v1:
* Initialize a FRED exception handler to fred_bad_event() instead of NULL
   if no FRED handler defined for an exception vector (Peter Zijlstra).
* Push calling irqentry_{enter,exit}() and instrumentation_{begin,end}()
   down into individual FRED exception handlers, instead of in the dispatch
   framework (Peter Zijlstra).
---
  arch/x86/entry/Makefile   |   5 +-
  arch/x86/entry/entry_64_fred.S|  52 ++
  arch/x86/entry/entry_fred.c   | 230 ++
  arch/x86/include/asm/asm-prototypes.h |   1 +
  arch/x86/include/asm/fred.h   |   6 +
  5 files changed, 293 insertions(+), 1 deletion(-)
  create mode 100644 arch/x86/entry/entry_64_fred.S
  create mode 100644 arch/x86/entry/entry_fred.c







+
+static noinstr void fred_intx(struct pt_regs *regs)
+{
+   switch (regs->fred_ss.vector) {
+   /* INT0 */
+   case X86_TRAP_OF:
+   exc_overflow(regs);
+   return;
+
+   /* INT3 */
+   case X86_TRAP_BP:
+   exc_int3(regs);
+   return;
+
+   /* INT80 */
+   case IA32_SYSCALL_VECTOR:
+   if (likely(IS_ENABLED(CONFIG_IA32_EMULATION))) {


Since future kernels will support boottime toggling of whether 32bit 
syscall interface should be enabled or not as per:

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/entry=1da5c9bc119d3a749b519596b93f9b2667e93c4a

It will make more sense to replace this with ia32_enabled() invocation. 
I guess this could be done as a follow-up patch based on when this is 
merged as the ia32_enbaled changes are going to be merged in 6.7.




+   /* Save the syscall number */
+   regs->orig_ax = regs->ax;
+   regs->ax = -ENOSYS;
+   do_int80_syscall_32(regs);
+   return;
+   }
+   fallthrough;
+
+   default:
+   exc_general_protection(regs, 0);
+   return;
+   }
+}
+
+static __always_inline void fred_other(struct pt_regs *regs)
+{
+   /* The compiler can fold these conditions into a single test */
+   if (likely(regs->fred_ss.vector == FRED_SYSCALL && regs->fred_ss.lm)) {
+   regs->orig_ax = regs->ax;
+   regs->ax = -ENOSYS;
+   do_syscall_64(regs, regs->orig_ax);
+   return;
+   } else if (likely(IS_ENABLED(CONFIG_IA32_EMULATION) &&


Ditto


+ regs->fred_ss.vector == FRED_SYSENTER &&
+ !regs->fred_ss.lm)) {
+   regs->orig_ax = regs->ax;
+   regs->ax = -ENOSYS;
+   do_fast_syscall_32(regs);
+   return;
+   } else {
+   exc_invalid_op(regs);
+   return;
+   }
+}
+

Re: [PATCH v9 02/16] vpci: use per-domain PCI lock to protect vpci structure

[Roger Pau Monné]

On Wed, Sep 20, 2023 at 03:16:00PM -0400, Stewart Hildebrand wrote:
> On 9/19/23 11:39, Roger Pau Monné wrote:
> > On Tue, Aug 29, 2023 at 11:19:42PM +, Volodymyr Babchuk wrote:
> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> >> index 1edc7f1e91..545a27796e 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -413,8 +413,6 @@ static int cf_check vmx_pi_update_irte(const struct 
> >> vcpu *v,
> >>
> >>  spin_unlock_irq(>lock);
> >>
> >> -ASSERT(pcidevs_locked());
> >> -
> > 
> > Hm, this removal seems dubious, same with some of the removal below.
> > And I don't see any comment in the log message as to why removing the
> > asserts here and in __pci_enable_msi{,x}(), pci_prepare_msix() is
> > safe.
> > 
> 
> I suspect we may want:
> 
> ASSERT(pcidevs_locked() || rw_is_locked(>pci_lock));
> 
> However, we don't have d. Using v->domain here is tricky because v may be 
> NULL. How about passing struct domain *d as an arg to 
> {hvm,vmx}_pi_update_irte()? Or ensuring that all callers pass a valid v?

I guess there was a reason to expect a path with v == NULL, but would
need to go trough the call paths that lead here.

Another option might be use use:

ASSERT(pcidevs_locked() || (v && rw_is_locked(>domain->pci_lock)));

But we would need some understanding of the call site of
vmx_pi_update_irte().

> 
> >>  return iommu_update_ire_from_msi(msi_desc, _desc->msg);
> >>
> >>   unlock_out:
> >> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> >> index d0bf63df1d..ba2963b7d2 100644
> >> --- a/xen/arch/x86/msi.c
> >> +++ b/xen/arch/x86/msi.c
> >> @@ -613,7 +613,7 @@ static int msi_capability_init(struct pci_dev *dev,
> >>  u8 slot = PCI_SLOT(dev->devfn);
> >>  u8 func = PCI_FUNC(dev->devfn);
> >>
> >> -ASSERT(pcidevs_locked());
> >> +ASSERT(pcidevs_locked() || rw_is_locked(>domain->pci_lock));
> >>  pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
> >>  if ( !pos )
> >>  return -ENODEV;
> >> @@ -783,7 +783,7 @@ static int msix_capability_init(struct pci_dev *dev,
> >>  if ( !pos )
> >>  return -ENODEV;
> >>
> >> -ASSERT(pcidevs_locked());
> >> +ASSERT(pcidevs_locked() || rw_is_locked(>domain->pci_lock));
> >>
> >>  control = pci_conf_read16(dev->sbdf, msix_control_reg(pos));
> >>  /*
> >> @@ -1000,7 +1000,6 @@ static int __pci_enable_msi(struct msi_info *msi, 
> >> struct msi_desc **desc)
> >>  struct pci_dev *pdev;
> >>  struct msi_desc *old_desc;
> >>
> >> -ASSERT(pcidevs_locked());
> >>  pdev = pci_get_pdev(NULL, msi->sbdf);
> >>  if ( !pdev )
> >>  return -ENODEV;
> 
> I think we can move the ASSERT here, after we obtain the pdev. Then we can 
> add the pdev->domain->pci_lock check into the mix:
> 
> ASSERT(pcidevs_locked() || rw_is_locked(>domain->pci_lock));

Hm, it would be better to perform the ASSERT before possibly accessing
the pdev list without holding any locks, but it's just an assert so
that might be the best option.

> 
> >> @@ -1055,7 +1054,6 @@ static int __pci_enable_msix(struct msi_info *msi, 
> >> struct msi_desc **desc)
> >>  struct pci_dev *pdev;
> >>  struct msi_desc *old_desc;
> >>
> >> -ASSERT(pcidevs_locked());
> >>  pdev = pci_get_pdev(NULL, msi->sbdf);
> >>  if ( !pdev || !pdev->msix )
> >>  return -ENODEV;
> 
> Same here
> 
> >> @@ -1170,8 +1168,6 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool 
> >> off)
> >>   */
> >>  int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
> >>  {
> >> -ASSERT(pcidevs_locked());
> >> -
> 
> This removal inside pci_enable_msi() may be okay if both __pci_enable_msi() 
> and __pci_enable_msix() have an appropriate ASSERT.

Hm, yes, that's likely fine, but would want a small mention in the
commit message.

> >>  if ( !use_msi )
> >>  return -EPERM;
> >>
> 
> Related: in xen/drivers/passthrough/pci.c:pci_get_pdev() I run into an ASSERT 
> with a PVH dom0:
> 
> (XEN) Assertion 'd || pcidevs_locked()' failed at 
> drivers/passthrough/pci.c:534
> (XEN) [ Xen-4.18-unstable  x86_64  debug=y  Tainted:   C]
> ...
> (XEN) Xen call trace:
> (XEN)[] R pci_get_pdev+0x4c/0xab
> (XEN)[] F arch/x86/msi.c#__pci_enable_msi+0x1d/0xb4
> (XEN)[] F pci_enable_msi+0x20/0x28
> (XEN)[] F map_domain_pirq+0x2b0/0x718
> (XEN)[] F allocate_and_map_msi_pirq+0xff/0x26b
> (XEN)[] F arch/x86/hvm/vmsi.c#vpci_msi_enable+0x53/0x9d
> (XEN)[] F vpci_msi_arch_enable+0x36/0x6c
> (XEN)[] F drivers/vpci/msi.c#control_write+0x71/0x114
> (XEN)[] F 
> drivers/vpci/vpci.c#vpci_write_helper+0x6f/0x7c
> (XEN)[] F vpci_write+0x249/0x2f9
> ...
> 
> With the patch applied, it's valid to call pci_get_pdev() with only 
> d->pci_lock held, so the ASSERT in pci_get_pdev() needs to be reworked too. 
> Inside pci_get_pdev(), d may be null, so we can't easily add || 
> rw_is_locked(>pci_lock) into the 

Re: [PATCH v9 02/16] vpci: use per-domain PCI lock to protect vpci structure

[Roger Pau Monné]

On Thu, Sep 21, 2023 at 09:42:08AM +0200, Jan Beulich wrote:
> On 20.09.2023 15:56, Stewart Hildebrand wrote:
> > On 9/20/23 04:09, Roger Pau Monné wrote:
> >> On Tue, Sep 19, 2023 at 12:20:39PM -0400, Stewart Hildebrand wrote:
> >>> On 9/19/23 11:39, Roger Pau Monné wrote:
>  On Tue, Aug 29, 2023 at 11:19:42PM +, Volodymyr Babchuk wrote:
> > diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> > index 8f2b59e61a..a0733bb2cb 100644
> > --- a/xen/drivers/vpci/msi.c
> > +++ b/xen/drivers/vpci/msi.c
> > @@ -318,15 +321,28 @@ void vpci_dump_msi(void)
> >   * holding the lock.
> >   */
> >  printk("unable to print all MSI-X entries: %d\n", 
> > rc);
> > -process_pending_softirqs();
> > -continue;
> > +goto pdev_done;
> >  }
> >  }
> >
> >  spin_unlock(>vpci->lock);
> > + pdev_done:
> > +/*
> > + * Unlock lock to process pending softirqs. This is
> > + * potentially unsafe, as d->pdev_list can be changed in
> > + * meantime.
> > + */
> > +read_unlock(>pci_lock);
> >  process_pending_softirqs();
> > +if ( !read_trylock(>pci_lock) )
> > +{
> > +printk("unable to access other devices for the 
> > domain\n");
> > +goto domain_done;
> 
>  Shouldn't the domain_done label be after the read_unlock(), so that we
>  can proceed to try to dump the devices for the next domain?  With the
>  proposed code a failure to acquire one of the domains pci_lock
>  terminates the dump.
> 
> > +}
> >  }
> > +read_unlock(>pci_lock);
> >  }
> > + domain_done:
> >  rcu_read_unlock(_read_lock);
> >  }
> >
> >>>
> >>> With the label moved, a no-op expression after the label is needed to 
> >>> make the compiler happy:
> >>>
> >>> }
> >>> }
> >>> read_unlock(>pci_lock);
> >>>  domain_done:
> >>> (void)0;
> >>> }
> >>> rcu_read_unlock(_read_lock);
> >>> }
> >>>
> >>>
> >>> If the no-op is omitted, the compiler may complain (gcc 9.4.0):
> >>>
> >>> drivers/vpci/msi.c: In function ‘vpci_dump_msi’:
> >>> drivers/vpci/msi.c:351:2: error: label at end of compound statement
> >>>   351 |  domain_done:
> >>>   |  ^~~
> >>
> >>
> >> Might be better to place the label at the start of the loop, and
> >> likely rename to next_domain.
> > 
> > That would bypass the loop condition and increment statements.
> 
> Right, such a label would be bogus even without that; instead of "goto"
> the use site then simply should use "continue".

IIRC continue is not suitable because the code would reach the
read_unlock() without having the lock held.

Anyway, I would leave to the submitter to find a suitable way to
continue the domain iteration.

Thanks, Roger.

Re: xen | Failed pipeline for staging | ea36ac0d

[Michal Orzel]

On 19/09/2023 12:21, Jan Beulich wrote:
> 
> 
> On 19.09.2023 11:53, GitLab wrote:
>>
>>
>> Pipeline #1009404353 has failed!
>>
>> Project: xen ( https://gitlab.com/xen-project/xen )
>> Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging )
>>
>> Commit: ea36ac0d ( 
>> https://gitlab.com/xen-project/xen/-/commit/ea36ac0de27c2a7c847a2a52c3e0f97a45864d81
>>  )
>> Commit Message: xen/ppc: Enable full Xen build
>>
>> Bring ppc's Mak...
>> Commit Author: Shawn Anastasio
>> Committed by: Jan Beulich ( https://gitlab.com/jbeulich )
>>
>>
>> Pipeline #1009404353 ( 
>> https://gitlab.com/xen-project/xen/-/pipelines/1009404353 ) triggered by 
>> Ganis ( https://gitlab.com/ganis )
>> had 5 failed jobs.
>>
>> Job #5118269375 ( https://gitlab.com/xen-project/xen/-/jobs/5118269375/raw )
>>
>> Stage: build
>> Name: debian-bullseye-gcc-ppc64le-debug-randconfig
> 
> This and ...
> 
>> Job #5118269256 ( https://gitlab.com/xen-project/xen/-/jobs/5118269256/raw )
>>
>> Stage: analyze
>> Name: eclair-x86_64
>> Job #5118269373 ( https://gitlab.com/xen-project/xen/-/jobs/5118269373/raw )
>>
>> Stage: build
>> Name: debian-bullseye-gcc-ppc64le-randconfig
> 
> ... this imo can't be expected to work. Is it really useful to run randconfig
> tests on ports which are only in the process of being brought up?
> 
>> Job #5118269370 ( https://gitlab.com/xen-project/xen/-/jobs/5118269370/raw )
>>
>> Stage: build
>> Name: debian-bullseye-gcc-ppc64le-debug
>> Job #5118269369 ( https://gitlab.com/xen-project/xen/-/jobs/5118269369/raw )
>>
>> Stage: build
>> Name: debian-bullseye-gcc-ppc64le
> 
> These two, otoh, look to be a result of the tests pre-seeding xen/.config with
> CONFIG_DEBUG settings, followed by making the olddefconfig goal. That, aiui,
> isn't picking up xen/arch/*/configs/*_defconfig, which at this point is
> mandatory for PPC (and likely is going to be so also for RISC-V once the full
> build is enabled there), at least as far as some of the option disables there
> go.
> 
> I think this wants switching to making the defconfig goal, and substituting
> CONFIG_DEBUG in the resulting .config. Due to x86'es and Arm's defconfig-s
> all being empty, this ought to be no change in what exactly is being tested
> there.
Apart from CONFIG_DEBUG there are other options we add using EXTRA_XEN_CONFIG
that might result in new options becoming visible and thus triggering a prompt
without olddefconfig.
So if at all, I think the flow should be:
defconfig
replacements
olddefconfig
make

~Michal

[ovmf test] 183099: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf ea628f28e59849ee7b91e6660c0ecd1a5c6e0884
baseline version:
 ovmf 89dad77cfbffda0303383a11026d854008c1b731

Last test of basis   183087  2023-09-20 15:10:51 Z0 days
Testing same since   183099  2023-09-21 05:10:53 Z0 days1 attempts


People who touched revisions under test:
  Andrei Warkentin 

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
   89dad77cfb..ea628f28e5  ea628f28e59849ee7b91e6660c0ecd1a5c6e0884 -> 
xen-tested-master

Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic

[Kevin Wolf]

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Variables declared in macros can shadow other variables.  Much of the
> time, this is harmless, e.g.:
> 
> #define _FDT(exp)  \
> do {   \
> int ret = (exp);   \
> if (ret < 0) { \
> error_report("error creating device tree: %s: %s",   \
> #exp, fdt_strerror(ret));  \
> exit(1);   \
> }  \
> } while (0)
> 
> Harmless shadowing in h_client_architecture_support():
> 
> target_ulong ret;
> 
> [...]
> 
> ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
> if (ret == H_SUCCESS) {
> _FDT((fdt_pack(spapr->fdt_blob)));
> [...]
> }
> 
> return ret;
> 
> However, we can get in trouble when the shadowed variable is used in a
> macro argument:
> 
> #define QOBJECT(obj) ({ \
> typeof(obj) o = (obj);  \
> o ? container_of(&(o)->base, QObject, base) : NULL; \
>  })
> 
> QOBJECT(o) expands into
> 
> ({
> --->typeof(o) o = (o);
> o ? container_of(&(o)->base, QObject, base) : NULL;
> })
> 
> Unintended variable name capture at --->.  We'd be saved by
> -Winit-self.  But I could certainly construct more elaborate death
> traps that don't trigger it.
> 
> To reduce the risk of trapping ourselves, we use variable names in
> macros that no sane person would use elsewhere.  Here's our actual
> definition of QOBJECT():
> 
> #define QOBJECT(obj) ({ \
> typeof(obj) _obj = (obj);   \
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
> })
> 
> Works well enough until we nest macro calls.  For instance, with
> 
> #define qobject_ref(obj) ({ \
> typeof(obj) _obj = (obj);   \
> qobject_ref_impl(QOBJECT(_obj));\
> _obj;   \
> })
> 
> the expression qobject_ref(obj) expands into
> 
> ({
> typeof(obj) _obj = (obj);
> qobject_ref_impl(
> ({
> --->typeof(_obj) _obj = (_obj);
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
> }));
> _obj;
> })
> 
> Unintended variable name capture at --->.
> 
> The only reliable way to prevent unintended variable name capture is
> -Wshadow.
> 
> One blocker for enabling it is shadowing hiding in function-like
> macros like
> 
>  qdict_put(dict, "name", qobject_ref(...))
> 
> qdict_put() wraps its last argument in QOBJECT(), and the last
> argument here contains another QOBJECT().
> 
> Use dark preprocessor sorcery to make the macros that give us this
> problem use different variable names on every call.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Eric Blake 
> ---
>  include/qapi/qmp/qobject.h | 11 +--
>  include/qemu/atomic.h  | 17 -
>  include/qemu/compiler.h|  3 +++
>  include/qemu/osdep.h   | 31 +++
>  4 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 9003b71fd3..d36cc97805 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -45,10 +45,17 @@ struct QObject {
>  struct QObjectBase_ base;
>  };
>  
> -#define QOBJECT(obj) ({ \
> +/*
> + * Preprocessory sorcery ahead: use a different identifier for the
> + * local variable in each expansion, so we can nest macro calls
> + * without shadowing variables.
> + */
> +#define QOBJECT_INTERNAL(obj, _obj) ({  \
>  typeof(obj) _obj = (obj);   \
> -_obj ? container_of(&(_obj)->base, QObject, base) : NULL;   \
> +_obj\
> +? container_of(&(_obj)->base, QObject, base) : NULL;\

What happened here? The code in this line (or now two lines) seems to be
unchanged apart from a strange looking newline.

>  })
> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))

Kevin

[GIT PULL] xen: branch for v6.6-rc3

[Juergen Gross]

Linus,

Please git pull the following tag:

 git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git 
for-linus-6.6a-rc3-tag

xen: branch for v6.6-rc3

It contains:

- a cleanup patch removing some unused functions in the Xen event
  channel handling

- a 3 patch series fixing a regression (introduced in 6.6 merge window)
  when booting as Xen PV guest

- a small cleanup patch removing another strncpy() instance


Thanks.

Juergen

 arch/arm/xen/enlighten.c  |  2 +-
 arch/x86/entry/common.c   |  2 +-
 arch/x86/include/asm/paravirt_types.h | 15 
 arch/x86/include/asm/xen/hypervisor.h | 37 +++
 arch/x86/kernel/paravirt.c| 67 ---
 arch/x86/xen/efi.c|  2 +-
 arch/x86/xen/enlighten.c  |  2 +-
 arch/x86/xen/enlighten_hvm.c  |  2 +-
 arch/x86/xen/enlighten_pv.c   | 40 +
 arch/x86/xen/mmu_pv.c | 55 ++--
 arch/x86/xen/multicalls.h |  4 +--
 drivers/xen/events/events_base.c  | 21 ++-
 drivers/xen/platform-pci.c|  2 +-
 include/trace/events/xen.h| 12 +++
 include/xen/arm/hypervisor.h  | 12 ---
 include/xen/events.h  |  3 +-
 16 files changed, 123 insertions(+), 155 deletions(-)

Juergen Gross (4):
  xen: simplify evtchn_do_upcall() call maze
  arm/xen: remove lazy mode related definitions
  x86/xen: move paravirt lazy code
  x86/xen: allow nesting of same lazy mode

Justin Stitt (1):
  xen/efi: refactor deprecated strncpy

Re: [PATCH v2 4/7] block/dirty-bitmap: Clean up local variable shadowing

[Kevin Wolf]

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: rename both the pair of parameters and the pair of local
> variables.  While there, move the local variables to function scope.
> 
> Suggested-by: Kevin Wolf 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Kevin Wolf 

Re: [PATCH v2 6/7] block: Clean up local variable shadowing

[Kevin Wolf]

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Stefan Hajnoczi 
> Acked-by: Anthony PERARD 
> Acked-by: Ilya Dryomov 

Reviewed-by: Kevin Wolf 

Re: xen | Failed pipeline for staging-4.15 | 3a9a2901

[Andrew Cooper]

On 21/09/2023 8:08 am, Jan Beulich wrote:
> On 20.09.2023 12:50, GitLab wrote:
>>
>> Pipeline #1010772231 has failed!
>>
>> Project: xen ( https://gitlab.com/xen-project/xen )
>> Branch: staging-4.15 ( 
>> https://gitlab.com/xen-project/xen/-/commits/staging-4.15 )
>>
>> Commit: 3a9a2901 ( 
>> https://gitlab.com/xen-project/xen/-/commit/3a9a2901cc8b24f28dbdc6fb63f57006c77a1f47
>>  )
>> Commit Message: x86/shadow: defer releasing of PV's top-level s...
>> Commit Author: Jan Beulich ( https://gitlab.com/jbeulich )
>> Committed by: Andrew Cooper ( https://gitlab.com/andyhhp )
>>
>>
>> Pipeline #1010772231 ( 
>> https://gitlab.com/xen-project/xen/-/pipelines/1010772231 ) triggered by 
>> Ganis ( https://gitlab.com/ganis )
>> had 19 failed jobs.
> I'm somewhat puzzled here, in two ways:
>
>> Job #5127621914 ( https://gitlab.com/xen-project/xen/-/jobs/5127621914/raw )
>>
>> Stage: build
>> Name: debian-stretch-clang-debug
>> Job #5127621916 ( https://gitlab.com/xen-project/xen/-/jobs/5127621916/raw )
>>
>> Stage: build
>> Name: debian-stretch-gcc
> This one failed with
>
> In file included from 
> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/timer.h:4:0,
>  from 
> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/timed-average.h:29,
>  from 
> /builds/xen-project/xen/tools/qemu-xen-dir/include/block/accounting.h:28,
>  from 
> /builds/xen-project/xen/tools/qemu-xen-dir/include/block/block_int.h:27,
>  from 
> /builds/xen-project/xen/tools/qemu-xen-dir/block/file-posix.c:30:
> /usr/include/linux/swab.h: In function '__swab':
> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/bitops.h:20:34: 
> error: "sizeof" is not defined [-Werror=undef]
>  #define BITS_PER_LONG   (sizeof (unsigned long) * BITS_PER_BYTE)
>   ^
> /builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/bitops.h:20:41: 
> error: missing binary operator before token "("
>  #define BITS_PER_LONG   (sizeof (unsigned long) * BITS_PER_BYTE)
>  ^
> cc1: all warnings being treated as errors
>
> What? How can "sizeof" no be "defined"?

Because the expression is

#if (sizeof (unsigned long) * BITS_PER_BYTE) = 64

combined with -Werror=undef saying that it can't figure out out what
sizeof is supposed to be here as a preprocessor token.

This needs a bump to qemu in order to fix, like we did on the newer
branches.

>
>> Job #5127621965 ( https://gitlab.com/xen-project/xen/-/jobs/5127621965/raw )
>>
>> Stage: build
>> Name: opensuse-leap-clang-debug
>> Job #5127621939 ( https://gitlab.com/xen-project/xen/-/jobs/5127621939/raw )
>>
>> Stage: build
>> Name: debian-unstable-32-gcc-debug
>> Job #5127621913 ( https://gitlab.com/xen-project/xen/-/jobs/5127621913/raw )
>>
>> Stage: build
>> Name: debian-stretch-clang
>> Job #5127621924 ( https://gitlab.com/xen-project/xen/-/jobs/5127621924/raw )
>>
>> Stage: build
>> Name: debian-unstable-clang-debug
>> Job #5127621934 ( https://gitlab.com/xen-project/xen/-/jobs/5127621934/raw )
>>
>> Stage: build
>> Name: debian-unstable-gcc-debug-randconfig
>> Job #5127621930 ( https://gitlab.com/xen-project/xen/-/jobs/5127621930/raw )
>>
>> Stage: build
>> Name: debian-unstable-gcc-debug
>> Job #5127621928 ( https://gitlab.com/xen-project/xen/-/jobs/5127621928/raw )
>>
>> Stage: build
>> Name: debian-unstable-gcc
>> Job #5127621937 ( https://gitlab.com/xen-project/xen/-/jobs/5127621937/raw )
>>
>> Stage: build
>> Name: debian-unstable-32-clang-debug
>> Job #5127621974 ( https://gitlab.com/xen-project/xen/-/jobs/5127621974/raw )
>>
>> Stage: build
>> Name: debian-unstable-gcc-arm64
>> Job #5127621975 ( https://gitlab.com/xen-project/xen/-/jobs/5127621975/raw )
>>
>> Stage: build
>> Name: debian-unstable-gcc-debug-arm64
>> Job #5127621964 ( https://gitlab.com/xen-project/xen/-/jobs/5127621964/raw )
>>
>> Stage: build
>> Name: opensuse-leap-clang
>> Job #5127621892 ( https://gitlab.com/xen-project/xen/-/jobs/5127621892/raw )
>>
>> Stage: build
>> Name: archlinux-gcc-debug
>> Job #5127621889 ( https://gitlab.com/xen-project/xen/-/jobs/5127621889/raw )
>>
>> Stage: build
>> Name: archlinux-gcc
> This one exhibits what e35138a2ffbe ("rombios: Work around GCC issue 99578")
> addresses, yet 4.16, which also doesn't have that backport, succeeded earlier
> on.

I rebuilt the archlinux container yesterday evening for the Py3
changes.  It's just possible that 4.15 ran with a newer container than 4.16

Nevertheless, I should backport the fix to reduce the noise here.

~Andrew

Re: [PATCH v2 5/7] block/vdi: Clean up local variable shadowing

[Kevin Wolf]

Am 20.09.2023 um 20:31 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Stefan Hajnoczi 

Reviewed-by: Kevin Wolf 

[xen-4.17-testing test] 183083: regressions - FAIL

[osstest service owner]

flight 183083 xen-4.17-testing real [real]
flight 183100 xen-4.17-testing real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/183083/
http://logs.test-lab.xenproject.org/osstest/logs/183100/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-libvirt   7 xen-install  fail REGR. vs. 182639
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. 
vs. 182639

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

version targeted for testing:
 xen  90c540c58985dc774cf0a1d2dc423473d3f37267
baseline 

Re: [PATCH v9 02/16] vpci: use per-domain PCI lock to protect vpci structure

[Jan Beulich]

On 20.09.2023 15:56, Stewart Hildebrand wrote:
> On 9/20/23 04:09, Roger Pau Monné wrote:
>> On Tue, Sep 19, 2023 at 12:20:39PM -0400, Stewart Hildebrand wrote:
>>> On 9/19/23 11:39, Roger Pau Monné wrote:
 On Tue, Aug 29, 2023 at 11:19:42PM +, Volodymyr Babchuk wrote:
> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> index 8f2b59e61a..a0733bb2cb 100644
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -318,15 +321,28 @@ void vpci_dump_msi(void)
>   * holding the lock.
>   */
>  printk("unable to print all MSI-X entries: %d\n", 
> rc);
> -process_pending_softirqs();
> -continue;
> +goto pdev_done;
>  }
>  }
>
>  spin_unlock(>vpci->lock);
> + pdev_done:
> +/*
> + * Unlock lock to process pending softirqs. This is
> + * potentially unsafe, as d->pdev_list can be changed in
> + * meantime.
> + */
> +read_unlock(>pci_lock);
>  process_pending_softirqs();
> +if ( !read_trylock(>pci_lock) )
> +{
> +printk("unable to access other devices for the 
> domain\n");
> +goto domain_done;

 Shouldn't the domain_done label be after the read_unlock(), so that we
 can proceed to try to dump the devices for the next domain?  With the
 proposed code a failure to acquire one of the domains pci_lock
 terminates the dump.

> +}
>  }
> +read_unlock(>pci_lock);
>  }
> + domain_done:
>  rcu_read_unlock(_read_lock);
>  }
>
>>>
>>> With the label moved, a no-op expression after the label is needed to make 
>>> the compiler happy:
>>>
>>> }
>>> }
>>> read_unlock(>pci_lock);
>>>  domain_done:
>>> (void)0;
>>> }
>>> rcu_read_unlock(_read_lock);
>>> }
>>>
>>>
>>> If the no-op is omitted, the compiler may complain (gcc 9.4.0):
>>>
>>> drivers/vpci/msi.c: In function ‘vpci_dump_msi’:
>>> drivers/vpci/msi.c:351:2: error: label at end of compound statement
>>>   351 |  domain_done:
>>>   |  ^~~
>>
>>
>> Might be better to place the label at the start of the loop, and
>> likely rename to next_domain.
> 
> That would bypass the loop condition and increment statements.

Right, such a label would be bogus even without that; instead of "goto"
the use site then simply should use "continue".

Jan

Re: [PATCH v6 0/4] ppc: Enable full Xen build

[Jan Beulich]

On 20.09.2023 13:04, George Dunlap wrote:
> On Wed, Sep 20, 2023 at 11:59 AM George Dunlap  
> wrote:
>>
>> On Mon, Sep 18, 2023 at 6:27 PM Shawn Anastasio
>>  wrote:
>>>
>>> On 9/18/23 8:19 AM, Jan Beulich wrote:
 On 14.09.2023 21:03, Shawn Anastasio wrote:
> Shawn Anastasio (4):
>   xen/ppc: Implement bitops.h
>   xen/ppc: Define minimal stub headers required for full build

 Compilation fails after applying this.

>   xen/ppc: Add stub function and symbol definitions

 Continuing nevertheless, linking fails after this.

>   xen/ppc: Enable full Xen build

 Things build okay for me when the full series is applied. Generally we
 wouldn't deliberately break the build between any two patches; doing so
 may be okay here (except I guest CI's build-each-commit would be upset),
 but I'll do so only upon explicit request (and with no-one else objecting).

>>>
>>> Sorry about that. Going forward I'll take more care to ensure that
>>> partially-applied series still build correctly. For this series though,
>>> if you could make an exception it would be appreciated.
>>
>> What would be the reason for the exception?
>>
>> We don't want to follow the rules just for the rules' sake, but the
>> rule is there for a reason: primarily to keep bisection working.  Not
>> sure of osstest is testing the PPC build yet, but if it were, then
>> this sort of thing would make it more difficult for the automatic
>> bisector to find regressions in other parts of the code.  Having
>> non-building patches can also confuse "archaeologists" -- people a few
>> years hence who are trying to understand what the code does.
>>
>> Is there a reason that this series would be particularly difficult to
>> reorganize in a way that would keep it building?  (Haven't looked at
>> it in detail.)
> 
> Sorry, didn't notice that Jan had said it "might be okay here".  Jan,
> don't count this as an objection.

Just to mention it (besides the fact that I did the commits already
before your reply) - at this stage I'm not sure bisection of PPC alone
is particularly important, yet. Hence why I said "might be okay".

Jan

Re: [PATCH v2 0/2] net: Update MemReentrancyGuard for NIC

[Akihiko Odaki]

On 2023/06/01 12:18, Akihiko Odaki wrote:

Recently MemReentrancyGuard was added to DeviceState to record that the
device is engaging in I/O. The network device backend needs to update it
when delivering a packet to a device.

This implementation follows what bottom half does, but it does not add
a tracepoint for the case that the network device backend started
delivering a packet to a device which is already engaging in I/O. This
is because such reentrancy frequently happens for
qemu_flush_queued_packets() and is insignificant.

This series consists of two patches. The first patch makes a bulk change to
add a new parameter to qemu_new_nic() and does not contain behavioral changes.
The second patch actually implements MemReentrancyGuard update.

V1 -> V2: Added the 'Fixes: CVE-2023-3019' tag

Akihiko Odaki (2):
   net: Provide MemReentrancyGuard * to qemu_new_nic()
   net: Update MemReentrancyGuard for NIC

  include/net/net.h |  2 ++
  hw/net/allwinner-sun8i-emac.c |  3 ++-
  hw/net/allwinner_emac.c   |  3 ++-
  hw/net/cadence_gem.c  |  3 ++-
  hw/net/dp8393x.c  |  3 ++-
  hw/net/e1000.c|  3 ++-
  hw/net/e1000e.c   |  2 +-
  hw/net/eepro100.c |  4 +++-
  hw/net/etraxfs_eth.c  |  3 ++-
  hw/net/fsl_etsec/etsec.c  |  3 ++-
  hw/net/ftgmac100.c|  3 ++-
  hw/net/i82596.c   |  2 +-
  hw/net/igb.c  |  2 +-
  hw/net/imx_fec.c  |  2 +-
  hw/net/lan9118.c  |  3 ++-
  hw/net/mcf_fec.c  |  3 ++-
  hw/net/mipsnet.c  |  3 ++-
  hw/net/msf2-emac.c|  3 ++-
  hw/net/mv88w8618_eth.c|  3 ++-
  hw/net/ne2000-isa.c   |  3 ++-
  hw/net/ne2000-pci.c   |  3 ++-
  hw/net/npcm7xx_emc.c  |  3 ++-
  hw/net/opencores_eth.c|  3 ++-
  hw/net/pcnet.c|  3 ++-
  hw/net/rocker/rocker_fp.c |  4 ++--
  hw/net/rtl8139.c  |  3 ++-
  hw/net/smc91c111.c|  3 ++-
  hw/net/spapr_llan.c   |  3 ++-
  hw/net/stellaris_enet.c   |  3 ++-
  hw/net/sungem.c   |  2 +-
  hw/net/sunhme.c   |  3 ++-
  hw/net/tulip.c|  3 ++-
  hw/net/virtio-net.c   |  6 --
  hw/net/vmxnet3.c  |  2 +-
  hw/net/xen_nic.c  |  4 ++--
  hw/net/xgmac.c|  3 ++-
  hw/net/xilinx_axienet.c   |  3 ++-
  hw/net/xilinx_ethlite.c   |  3 ++-
  hw/usb/dev-network.c  |  3 ++-
  net/net.c | 15 +++
  40 files changed, 90 insertions(+), 41 deletions(-)



Hi Jason,

Can you review this series?

Regards,
Akihiko Odaki

Re: xen | Failed pipeline for staging-4.15 | 3a9a2901

[Jan Beulich]

On 20.09.2023 12:50, GitLab wrote:
> 
> 
> Pipeline #1010772231 has failed!
> 
> Project: xen ( https://gitlab.com/xen-project/xen )
> Branch: staging-4.15 ( 
> https://gitlab.com/xen-project/xen/-/commits/staging-4.15 )
> 
> Commit: 3a9a2901 ( 
> https://gitlab.com/xen-project/xen/-/commit/3a9a2901cc8b24f28dbdc6fb63f57006c77a1f47
>  )
> Commit Message: x86/shadow: defer releasing of PV's top-level s...
> Commit Author: Jan Beulich ( https://gitlab.com/jbeulich )
> Committed by: Andrew Cooper ( https://gitlab.com/andyhhp )
> 
> 
> Pipeline #1010772231 ( 
> https://gitlab.com/xen-project/xen/-/pipelines/1010772231 ) triggered by 
> Ganis ( https://gitlab.com/ganis )
> had 19 failed jobs.

I'm somewhat puzzled here, in two ways:

> Job #5127621914 ( https://gitlab.com/xen-project/xen/-/jobs/5127621914/raw )
> 
> Stage: build
> Name: debian-stretch-clang-debug
> Job #5127621916 ( https://gitlab.com/xen-project/xen/-/jobs/5127621916/raw )
> 
> Stage: build
> Name: debian-stretch-gcc

This one failed with

In file included from 
/builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/timer.h:4:0,
 from 
/builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/timed-average.h:29,
 from 
/builds/xen-project/xen/tools/qemu-xen-dir/include/block/accounting.h:28,
 from 
/builds/xen-project/xen/tools/qemu-xen-dir/include/block/block_int.h:27,
 from 
/builds/xen-project/xen/tools/qemu-xen-dir/block/file-posix.c:30:
/usr/include/linux/swab.h: In function '__swab':
/builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/bitops.h:20:34: error: 
"sizeof" is not defined [-Werror=undef]
 #define BITS_PER_LONG   (sizeof (unsigned long) * BITS_PER_BYTE)
  ^
/builds/xen-project/xen/tools/qemu-xen-dir/include/qemu/bitops.h:20:41: error: 
missing binary operator before token "("
 #define BITS_PER_LONG   (sizeof (unsigned long) * BITS_PER_BYTE)
 ^
cc1: all warnings being treated as errors

What? How can "sizeof" no be "defined"?

> Job #5127621965 ( https://gitlab.com/xen-project/xen/-/jobs/5127621965/raw )
> 
> Stage: build
> Name: opensuse-leap-clang-debug
> Job #5127621939 ( https://gitlab.com/xen-project/xen/-/jobs/5127621939/raw )
> 
> Stage: build
> Name: debian-unstable-32-gcc-debug
> Job #5127621913 ( https://gitlab.com/xen-project/xen/-/jobs/5127621913/raw )
> 
> Stage: build
> Name: debian-stretch-clang
> Job #5127621924 ( https://gitlab.com/xen-project/xen/-/jobs/5127621924/raw )
> 
> Stage: build
> Name: debian-unstable-clang-debug
> Job #5127621934 ( https://gitlab.com/xen-project/xen/-/jobs/5127621934/raw )
> 
> Stage: build
> Name: debian-unstable-gcc-debug-randconfig
> Job #5127621930 ( https://gitlab.com/xen-project/xen/-/jobs/5127621930/raw )
> 
> Stage: build
> Name: debian-unstable-gcc-debug
> Job #5127621928 ( https://gitlab.com/xen-project/xen/-/jobs/5127621928/raw )
> 
> Stage: build
> Name: debian-unstable-gcc
> Job #5127621937 ( https://gitlab.com/xen-project/xen/-/jobs/5127621937/raw )
> 
> Stage: build
> Name: debian-unstable-32-clang-debug
> Job #5127621974 ( https://gitlab.com/xen-project/xen/-/jobs/5127621974/raw )
> 
> Stage: build
> Name: debian-unstable-gcc-arm64
> Job #5127621975 ( https://gitlab.com/xen-project/xen/-/jobs/5127621975/raw )
> 
> Stage: build
> Name: debian-unstable-gcc-debug-arm64
> Job #5127621964 ( https://gitlab.com/xen-project/xen/-/jobs/5127621964/raw )
> 
> Stage: build
> Name: opensuse-leap-clang
> Job #5127621892 ( https://gitlab.com/xen-project/xen/-/jobs/5127621892/raw )
> 
> Stage: build
> Name: archlinux-gcc-debug
> Job #5127621889 ( https://gitlab.com/xen-project/xen/-/jobs/5127621889/raw )
> 
> Stage: build
> Name: archlinux-gcc

This one exhibits what e35138a2ffbe ("rombios: Work around GCC issue 99578")
addresses, yet 4.16, which also doesn't have that backport, succeeded earlier
on.

Jan

RE: [PATCH v10 16/38] x86/ptrace: Add FRED additional information to the pt_regs structure

[Li, Xin3]

> > I guess you have FRED 3.0 spec, no?
> Doh you are right, I was looking at the wrong version of the document  
> sorry for
> the noise.

Actually I appreciate your review so much!

Re: [PATCH v10 16/38] x86/ptrace: Add FRED additional information to the pt_regs structure

[Nikolay Borisov]

On 20.09.23 г. 20:23 ч., Li, Xin3 wrote:

+struct fred_ss {
+   u64 ss  : 16,   // SS selector


Is this structure conformant to the return state as described in FRED 5.0?

— The stack segment of the interrupted context, 64 bits formatted as follows:

• Bits 15:0 contain the SS selector. < - WE HAVE THIS

• Bits 31:16 are not currently defined and will be zero until they are.


Where did you download the FRED 5.0 spec from?

Mine says bit 16 is sti, bit 17 for sw initiated events and bit 18 is NMI.

I guess you have FRED 3.0 spec, no?
Doh you are right, I was looking at the wrong version of the document 
 sorry for the noise.



  < - MISSING > hole?


+   sti :  1,   // STI state < -
+   swevent :  1,   // Set if syscall, sysenter or INT n
+   nmi :  1,   // Event is NMI type
+   : 13,
  

Re: [PATCH v2 2/7] migration: Clean up local variable shadowing

[Zhijian Li (Fujitsu)]

On 21/09/2023 02:31, Markus Armbruster wrote:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Peter Xu 

Reviewed-by: Li Zhijian 

Re: [XEN PATCH v3 4/4] README: remove old note about the build system's python expectation

[Jan Beulich]

On 20.09.2023 21:40, Andrew Cooper wrote:
> On 19/09/2023 7:50 am, Jan Beulich wrote:
>> On 19.09.2023 08:30, Javi Merino wrote:
>>> 5852ca485263 (build: fix tools/configure in case only python3 exists,
>>> 2019-12-11) changed the configure script to test for the availability
>>> of python3, python and python2 in that order.  It sets PYTHON to the
>>> first one found in path.  You don't need to have a symlink to python.
>>>
>>> Remove the outdated note from the README.
>> But that note also covers the hypervisor build, which isn't affected by
>> ./configure, and even less so by tools/configure.
> 
> The hypervisor build equally doesn't python= specifying.

I'm pretty sure that on at least one of my systems I need PYTHON=python3
when building the hypervisor.

Jan