[qemu-mainline test] 158989: regressions - trouble: broken/fail/pass

[osstest service owner]

flight 158989 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/158989/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-xl-seattle  broken
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 152631
 test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeat fail REGR. vs. 152631
 test-amd64-amd64-xl-qcow2   21 guest-start/debian.repeat fail REGR. vs. 152631
 test-armhf-armhf-libvirt-raw 13 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-xl-vhd  13 guest-start  fail REGR. vs. 152631

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-seattle   5 host-install(5)   broken blocked in 152631
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152631
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 152631
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 152631
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152631
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 152631
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 152631
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 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-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-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-cubietruck 15 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass

version targeted for testing:
 qemuu99ae0cd90d3e41b424582cf74bcf32498ca81bb9
baseline version:
 qemuu1d806cef0e38b5db8347a8e12f214d543204a314

Last test of basis   152631  2020-08-20 09:07:46 Z  167 days
Failing since152659  2020-08-21 14:07:39 Z  166 days  337 attempts
Testing same since   158989  2021-02-03 18:44:17 Z0 days1 attempts


373 people touched revisions under test,
not listing them all

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  

Re: [PATCH RFC v1 2/6] swiotlb: convert variables to arrays

[Christoph Hellwig]

On Wed, Feb 03, 2021 at 03:37:05PM -0800, Dongli Zhang wrote:
> This patch converts several swiotlb related variables to arrays, in
> order to maintain stat/status for different swiotlb buffers. Here are
> variables involved:
> 
> - io_tlb_start and io_tlb_end
> - io_tlb_nslabs and io_tlb_used
> - io_tlb_list
> - io_tlb_index
> - max_segment
> - io_tlb_orig_addr
> - no_iotlb_memory
> 
> There is no functional change and this is to prepare to enable 64-bit
> swiotlb.

Claire Chang (on Cc) already posted a patch like this a month ago,
which looks much better because it actually uses a struct instead
of all the random variables. 

Re: Question about xen and Rasp 4B

[Jukka Kaartinen]

On 1.2.2021 21.25, Stefano Stabellini wrote:

On Sat, 30 Jan 2021, Julien Grall wrote:

On 27/01/2021 11:47, Jukka Kaartinen wrote:



On Tue, Jan 26, 2021 at 10:22 PM Stefano Stabellini
mailto:sstabell...@kernel.org>> wrote:

     On Tue, 26 Jan 2021, Jukka Kaartinen wrote:
  > On Tue, Jan 26, 2021 at 2:54 AM Stefano Stabellini
     mailto:sstabell...@kernel.org>> wrote:
  >       On Sat, 23 Jan 2021, Jukka Kaartinen wrote:
  >       > Thanks for the response!
  >       >
  >       > On Sat, Jan 23, 2021 at 2:27 AM Stefano Stabellini
     mailto:sstabell...@kernel.org>> wrote:
  >       >       + xen-devel, Roman,
  >       >
  >       >
  >       >       On Fri, 22 Jan 2021, Jukka Kaartinen wrote:
  >       >       > Hi Stefano,
  >       >       > I'm Jukka Kaartinen a SW developer working on
     enabling hypervisors on mobile platforms. One of our HW that we use
on
  >       >       development is
  >       >       > Raspberry Pi 4B. I wonder if you could help me a
     bit :).
  >       >       >
  >       >       > I'm trying to enable the GPU with Xen + Raspberry
     Pi for
  >       >       dom0.
https://www.raspberrypi.org/forums/viewtopic.php?f=63=232323#p1797605

  >       >       >
  >       >       > I got so far that GPU drivers are loaded (v3d &
     vc4) without errors. But now Xen returns error when X is starting:
  >       >       > (XEN) traps.c:1986:d0v1 HSR=0x93880045
     pc=0x7f97b14e70 gva=0x7f7f817000 gpa=0x401315d000
  >       >       >  I tried to debug what causes this and looks
     like find_mmio_handler cannot find handler.
  >       >       > (See more here:
https://www.raspberrypi.org/forums/viewtopic.php?f=63=232323=25#p1801691

     )
  >       >       >
  >       >       > Any ideas why the handler is not found?
  >       >
  >       >
  >       >       Hi Jukka,
  >       >
  >       >       I am glad to hear that you are interested in Xen on
     RaspberryPi :-)  I
  >       >       haven't tried the GPU yet, I have been using the
     serial only.
  >       >       Roman, did you ever get the GPU working?
  >       >
  >       >
  >       >       The error is a data abort error: Linux is trying to
     access an address
  >       >       which is not mapped to dom0. The address seems to
     be 0x401315d000. It is
  >       >       a pretty high address; I looked in device tree but
     couldn't spot it.
  >       >
  >       >       >From the HSR (the syndrom register) it looks like
     it is a translation
  >       >       fault at EL1 on stage1. As if the Linux address
     mapping was wrong.
  >       >       Anyone has any ideas how this could happen? Maybe a
     reserved-memory
  >       >       misconfiguration?
  >       >
  >       > I had issues with loading the driver in the first place.
     Apparently swiotlb is used, maybe it can cause this. I also tried to
  >       enable CMA.
  >       > config.txt:
  >       > dtoverlay=vc4-fkms-v3d,cma=320M@0x0-0x4000
  >       > gpu_mem=128
  >
  >       Also looking at your other reply and the implementation of
  >       vc4_bo_create, it looks like this is a CMA problem.
  >
  >       It would be good to run a test with the swiotlb-xen
disabled:
  >
  >       diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
  >       index 467fa225c3d0..2bdd12785d14 100644
  >       --- a/arch/arm/xen/mm.c
  >       +++ b/arch/arm/xen/mm.c
  >       @@ -138,8 +138,7 @@ void
     xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int
order)
  >        static int __init xen_mm_init(void)
  >        {
  >               struct gnttab_cache_flush cflush;
  >       -       if (!xen_initial_domain())
  >       -               return 0;
  >       +       return 0;
  >               xen_swiotlb_init(1, false);
  >
  >               cflush.op = 0;
  >
  > With this change the kernel is not booting up. (btw. I'm using
     USB SSD for my OS.)
  > [    0.071081] bcm2835-dma fe007000.dma: Unable to set DMA mask
  > [    0.076277] bcm2835-dma fe007b00.dma: Unable to set DMA mask
  > (XEN) physdev.c:16:d0v0 PHYSDEVOP cmd=25: not implemented
  > (XEN) physdev.c:16:d0v0 PHYSDEVOP cmd=15: not implemented
  > [    0.592695] pci :00:00.0: Failed to add - passthrough or
     MSI/MSI-X might fail!
  > (XEN) physdev.c:16:d0v0 PHYSDEVOP cmd=15: not implemented
  > [    0.606819] pci :01:00.0: Failed to add - passthrough or
     MSI/MSI-X might fail!
  > [    1.212820] usb 1-1: device descriptor read/64, error 18
  > [    1.452815] usb 1-1: device descriptor read/64, 

Re: Question about xen and Rasp 4B

[Jukka Kaartinen]

On 4.2.2021 0.12, Elliott Mitchell wrote:

On Wed, Feb 03, 2021 at 12:55:40PM -0800, Stefano Stabellini wrote:

On Wed, 3 Feb 2021, Jukka Kaartinen wrote:

On 3.2.2021 2.18, Stefano Stabellini wrote:

How are you configuring and installing the kernel?

make bcm2711_defconfig
make Image.gz
make modules_install

?

The device tree is the one from the rpi-5.9.y build? How are you loading
the kernel and device tree with uboot? Do you have any interesting
changes to config.txt?

I am asking because I cannot get to the point of reproducing what you
are seeing: I can boot my rpi-5.9.y kernel on recent Xen but I cannot
get any graphics output on my screen. (The serial works.) I am using the
default Ubuntu Desktop rpi-install target as rootfs and uboot master.



This is what I do:

make bcm2711_defconfig
cat "xen_additions" >> .config
make Image  modules dtbs

make INSTALL_MOD_PATH=rootfs modules_install
depmod -a

cp arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb boot/
cp arch/arm64/boot/dts/overlays/*.dtbo boot/overlays/


Thanks for the detailed instructions. This helps a lot. I saw below in
boot2.source that you are using ${fdt_addr} as DTB source (instead of
loading one), which means you are using the DTB as provided by U-Boot at
runtime, instead of loading your own file.

With these two copies, I take you meant to update the first partition on
the SD card, the one where config.txt lives, right? So that Xen is
getting the DTB and overlays from the rpi-5.9.y kernel tree but passed
down by the RPi loader and U-Boot?

I think the DTB must be the issue as I wasn't applying any overlays
before. I ran a test to use the DTB and overlay from U-Boot but maybe I
haven't updated them properly because I still don't see any output.


Seeing no graphics output from U-Boot is okay.  If the device-tree files
get sufficiently updated you can end up with no output from U-Boot, but
will get output once the Linux kernel's driver is operational (I've seen
this occur).

The most important part is having a HDMI display plugged in during the
early boot stages.  Unless the bootloader sees the display the output
won't get initialized and the Linux driver doesn't handle that.



dtoverlay=vc4-fkms-v3d,cma-64


This is odd.  My understanding is this is appropriate for RP3, but not
RP4.  For RP4 you can have "dtoverlay=disable-vc4" and still get graphics
output (hmm, I'm starting to think I need to double-check this...).
Without the overlay GPU driver (v3d) was not probed. And you need to use 
the fakekms.

Re: Question about xen and Rasp 4B

[Jukka Kaartinen]

On 3.2.2021 22.55, Stefano Stabellini wrote:

On Wed, 3 Feb 2021, Jukka Kaartinen wrote:

On 3.2.2021 2.18, Stefano Stabellini wrote:

On Tue, 2 Feb 2021, Jukka Kaartinen wrote:

Good catch.
GPU works now and I can start X! Thanks! I was also able to create
domU
that runs Raspian OS.


This is very interesting that you were able to achieve that - congrats!

Now, sorry to be a bit dense -- but since this thread went into all
sorts of interesting
directions all at once -- I just have a very particular question: what
is
exact
combination of versions of Xen, Linux kernel and a set of patches that
went
on top that allowed you to do that? I'd love to obviously see it
productized in Xen
upstream, but for now -- I'd love to make available to Project EVE/Xen
community
since there seems to be a few folks interested in EVE/Xen combo being
able
to
do that.


I have tried Xen Release 4.14.0, 4.14.1 and master (from week 4, 2021).

Kernel rpi-5.9.y and rpi-5.10.y branches from
https://github.com/raspberrypi/linux

and

U-boot (master).

For the GPU to work it was enough to disable swiotlb from the kernel(s) as
suggested in this thread.


How are you configuring and installing the kernel?

make bcm2711_defconfig
make Image.gz
make modules_install

?

The device tree is the one from the rpi-5.9.y build? How are you loading
the kernel and device tree with uboot? Do you have any interesting
changes to config.txt?

I am asking because I cannot get to the point of reproducing what you
are seeing: I can boot my rpi-5.9.y kernel on recent Xen but I cannot
get any graphics output on my screen. (The serial works.) I am using the
default Ubuntu Desktop rpi-install target as rootfs and uboot master.



This is what I do:

make bcm2711_defconfig
cat "xen_additions" >> .config
make Image  modules dtbs

make INSTALL_MOD_PATH=rootfs modules_install
depmod -a

cp arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb boot/
cp arch/arm64/boot/dts/overlays/*.dtbo boot/overlays/


Thanks for the detailed instructions. This helps a lot. I saw below in
boot2.source that you are using ${fdt_addr} as DTB source (instead of
loading one), which means you are using the DTB as provided by U-Boot at
runtime, instead of loading your own file.

With these two copies, I take you meant to update the first partition on
the SD card, the one where config.txt lives, right? So that Xen is
getting the DTB and overlays from the rpi-5.9.y kernel tree but passed
down by the RPi loader and U-Boot?

I think the DTB must be the issue as I wasn't applying any overlays
before. I ran a test to use the DTB and overlay from U-Boot but maybe I
haven't updated them properly because I still don't see any output.
I'm using ${fdt_addr} has the changes that FW will do according to your 
congig.txt for example the overlay is applied. I tried to load and apply 
the overlay in the u-boot but there were strange errors so I decided to 
go the easy way.


I the overlay (vc4-fkms-v3d) is not applied GPU (v3d) driver is not probed.

I always see output from u-boot from the serial port. If not then there 
has been something wrong with the device-tree. And it has been because 
u-boot was not able to read from the SSD.






config.txt:

[pi4]
max_framebuffers=2
enable_uart=1
arm_freq=1500
force_turbo=1

[all]
arm_64bit=1
kernel=u-boot.bin

start_file=start4.elf
fixup_file=fixup4.dat

# Enable the audio output, I2C and SPI interfaces on the GPIO header
dtparam=audio=on
dtparam=i2c_arm=on
dtparam=spi=on

# Enable the FKMS ("Fake" KMS) graphics overlay, enable the camera firmware
# and allocate 128Mb to the GPU memory
dtoverlay=vc4-fkms-v3d,cma-64
gpu_mem=128

# Comment out the following line if the edges of the desktop appear outside
# the edges of your display
disable_overscan=1


boot.source:
setenv serverip 10.42.0.1
setenv ipaddr 10.42.0.231
tftpb 0xC0 boot2.scr
source 0xC0

boot2.source:
tftpb 0xE0 xen
tftpb 0x100 Image
setenv lin_size $filesize

fdt addr ${fdt_addr}
fdt resize 1024

fdt set /chosen xen,xen-bootargs "console=dtuart dtuart=serial0 sync_console
dom0_mem=1024M dom0_max_vcpus=1 bootscrub=0 vwfi=native sched=credit2"

fdt mknod /chosen dom0

# These will break the default framebuffer@3e2fe000 that
# is the same chosen -node.
#fdt set /chosen/dom0 \#address-cells <0x2>
#fdt set /chosen/dom0 \#size-cells <0x2>

fdt set /chosen/dom0 compatible "xen,linux-zimage" "xen,multiboot-module"
fdt set /chosen/dom0 reg <0x100 0x${lin_size}>

fdt set /chosen xen,dom0-bootargs "dwc_otg.lpm_enable=0 console=hvc0
earlycon=xen earlyprintk=xen root=/dev/sda4 elevator=deadline rootwait fixrtc
quiet splash"

setenv fdt_high 0x

fdt print /chosen

#xen
booti 0xE0 - ${fdt_addr}

Re: [PATCH] xen/netback: avoid race in xenvif_rx_ring_slots_available()

[Jürgen Groß]

On 04.02.21 00:48, Jakub Kicinski wrote:

On Tue,  2 Feb 2021 08:09:38 +0100 Juergen Gross wrote:

Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
xenvif_rx_ring_slots_available() is no longer called only from the rx
queue kernel thread, so it needs to access the rx queue with the
associated queue held.

Reported-by: Igor Druzhinin 
Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
Cc: sta...@vger.kernel.org
Signed-off-by: Juergen Gross 


Should we route this change via networking trees? I see the bug did not
go through networking :)



I'm fine with either networking or the Xen tree. It should be included
in 5.11, though. So if you are willing to take it, please do so.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

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

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64-xsm  broken
 build-arm64-xsm   4 host-install(4)broken REGR. vs. 152332
 test-amd64-i386-xl-xsm7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail REGR. 
vs. 152332
 test-amd64-i386-xl-qemuu-ws16-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail REGR. vs. 
152332
 test-amd64-i386-qemut-rhel6hvm-intel  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-intel  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-examine   6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt   7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-pair 10 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   7 xen-install  fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 
152332
 test-amd64-i386-freebsd10-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-raw7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-pvshim 7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  7 xen-installfail REGR. vs. 152332
 test-amd64-i386-xl-qemut-win7-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. 
vs. 152332
 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-installfail REGR. vs. 152332
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-installfail REGR. vs. 152332
 test-arm64-arm64-xl-seattle  12 debian-install   fail REGR. vs. 152332
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 152332
 test-arm64-arm64-xl-thunderx 14 guest-start  fail REGR. vs. 152332
 test-amd64-amd64-amd64-pvgrub 20 guest-stop  fail REGR. vs. 152332
 test-armhf-armhf-xl-multivcpu 14 guest-start fail REGR. vs. 152332
 test-amd64-amd64-i386-pvgrub 20 guest-stop   fail REGR. vs. 152332
 test-armhf-armhf-xl-credit1  14 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-xl  14 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-xl-arndale  14 guest-start  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  7 xen-install  fail REGR. vs. 152332
 test-armhf-armhf-libvirt 14 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-xl-cubietruck 14 guest-startfail REGR. vs. 152332
 test-armhf-armhf-xl-credit2  14 guest-start  fail REGR. vs. 152332
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 152332
 test-armhf-armhf-xl-vhd  13 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-libvirt-raw 13 guest-start  fail REGR. vs. 152332
 test-arm64-arm64-libvirt-xsm  8 xen-boot   fail in 158971 REGR. vs. 152332
 test-arm64-arm64-xl-xsm   8 xen-boot   fail in 158971 REGR. vs. 152332

Tests which are failing intermittently (not blocking):
 test-arm64-arm64-xl-credit2  10 host-ping-check-xenfail pass in 158971

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds 14 guest-start  fail REGR. vs. 152332

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-seattle 11 leak-check/basis(11) fail in 158971 blocked in 
152332
 test-arm64-arm64-xl-credit2 11 leak-check/basis(11) fail 

Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses

[Stefano Stabellini]

On Wed, 3 Feb 2021, Julien Grall wrote:
> On Wed, 3 Feb 2021 at 22:18, Stefano Stabellini  
> wrote:
> > > > But aside from PCIe, let's say that we know of a few nodes for which
> > > > "reg" needs a special treatment. I am not sure it makes sense to proceed
> > > > with parsing those nodes without knowing how to deal with that.
> > >
> > > I believe that most of the time the "special" treatment would be to 
> > > ignore the
> > > property "regs" as it will not be an CPU memory address.
> > >
> > > > So maybe
> > > > we should add those nodes to skip_matches until we know what to do with
> > > > them. At that point, I would imagine we would introduce a special
> > > > handle_device function that knows what to do. In the case of PCIe,
> > > > something like "handle_device_pcie".
> > > Could you outline how "handle_device_pcie()" will differ with 
> > > handle_node()?
> > >
> > > In fact, the problem is not the PCIe node directly. Instead, it is the 
> > > second
> > > level of nodes below it (i.e usb@...).
> > >
> > > The current implementation of dt_number_of_address() only look at the bus 
> > > type
> > > of the parent. As the parent has no bus type and "ranges" then it thinks 
> > > this
> > > is something we can translate to a CPU address.
> > >
> > > However, this is below a PCI bus so the meaning of "reg" is completely
> > > different. In this case, we only need to ignore "reg".
> >
> > I see what you are saying and I agree: if we had to introduce a special
> > case for PCI, then  dt_number_of_address() seems to be a good place.  In
> > fact, we already have special PCI handling, see our
> > __dt_translate_address function and xen/common/device_tree.c:dt_busses.
> >
> > Which brings the question: why is this actually failing?
> 
> I already hinted at the reason in my previous e-mail :). Let me expand
> a bit more.
> 
> >
> > pcie0 {
> >  ranges = <0x0200 0x0 0xc000 0x6 0x 0x0 0x4000>;
> >
> > Which means that PCI addresses 0xc000-0x1 become 
> > 0x6-0x7.
> >
> > The offending DT is:
> >
> >  {
> >  pci@1,0 {
> >  #address-cells = <3>;
> >  #size-cells = <2>;
> >  ranges;
> >
> >  reg = <0 0 0 0 0>;
> >
> >  usb@1,0 {
> >  reg = <0x1 0 0 0 0>;
> >  resets = < 
> > RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
> >  };
> >  };
> > };
> >
> >
> > reg = <0x1 0 0 0 0> means that usb@1,0 is PCI device 01:00.0.
> > However, the rest of the regs cells are left as zero. It shouldn't be an
> > issue because usb@1,0 is a child of pci@1,0 but pci@1,0 is not a bus.
> 
> The property "ranges" is used to define a mapping or translation
> between the address space of the "bus" (here pci@1,0) and the address
> space of the bus node's parent ().
> IOW, it means "reg" in usb@1,0 is an address on the PCI bus (i.e. BDF).
> 
> The problem is dt_number_of_address() will only look at the "bus" type
> of the parent using dt_match_bus(). This will return the default bus
> (see dt_bus_default_match()), because this is a property "ranges" in
> the parent node (i.e. pci@1,0). Therefore...
> 
> > So
> > in theory dt_number_of_address() should already return 0 for it.
> 
> ... dt_number_of_address() will return 1 even if the address is not a
> CPU address. So when Xen will try to translate it, it will fail.
> 
> >
> > Maybe reg = <0 0 0 0 0> is the problem. In that case, we could simply
> > add a check to skip 0 size ranges. Just a hack to explain what I mean:
> 
> The parent of pci@1,0 is a PCI bridge (see the property type), so the
> CPU addresses are found not via "regs" but "assigned-addresses".
> 
> In this situation, "regs" will have a different meaning and therefore
> there is no promise that the size will be 0.

I copy/pasted the following:

   pci@1,0 {
   #address-cells = <3>;
   #size-cells = <2>;
   ranges;

   reg = <0 0 0 0 0>;

   usb@1,0 {
   reg = <0x1 0 0 0 0>;
   resets = <
   RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
   };
   };

under pcie0 in my DTS to see what happens (the node is not there in the
device tree for the rpi-5.9.y kernel.) It results in the expected error:

(XEN) Unable to retrieve address 0 for /scb/pcie@7d50/pci@1,0/usb@1,0
(XEN) Device tree generation failed (-22).

I could verify that pci@1,0 is seen as "default" bus due to the range
property, thus dt_number_of_address() returns 1.


I can see that reg = <0 0 0 0 0> is not a problem because it is ignored
given that the parent is a PCI bus. assigned-addresses is the one that
is read.


But from a device tree perspective I am actually confused by the
presence of the "ranges" property under pci@1,0. Is that correct? It is
stating that addresses of children devices will be translated to the
address 

[linux-5.4 test] 158981: regressions - trouble: broken/fail/pass

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-xl-credit1  broken
 test-arm64-arm64-xl-seattle  broken
 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 158387
 test-amd64-amd64-dom0pvh-xl-amd 14 guest-start   fail REGR. vs. 158387
 test-amd64-amd64-xl-multivcpu 14 guest-start fail REGR. vs. 158387
 test-amd64-amd64-xl-pvhv2-amd 14 guest-start fail REGR. vs. 158387
 test-amd64-coresched-amd64-xl 14 guest-start fail REGR. vs. 158387
 test-amd64-coresched-i386-xl 14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-qemuu-freebsd12-amd64 13 guest-startfail REGR. vs. 158387
 test-amd64-amd64-libvirt 14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-freebsd10-i386 13 guest-startfail REGR. vs. 158387
 test-arm64-arm64-xl  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-pair 25 guest-start/debian   fail REGR. vs. 158387
 test-amd64-i386-qemut-rhel6hvm-amd 12 redhat-install fail REGR. vs. 158387
 test-amd64-i386-qemuu-rhel6hvm-amd 12 redhat-install fail REGR. vs. 158387
 test-amd64-i386-freebsd10-amd64 13 guest-start   fail REGR. vs. 158387
 test-amd64-amd64-xl-pvshim   14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-pvhv2-intel 14 guest-start   fail REGR. vs. 158387
 test-amd64-amd64-xl-xsm  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-libvirt  14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-libvirt-xsm 14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl-shadow14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-credit1  14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-pair25 guest-start/debian   fail REGR. vs. 158387
 test-amd64-amd64-xl  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl   14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-shadow   14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl-xsm   14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-credit2  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-libvirt-xsm  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-libvirt-pair 25 guest-start/debian   fail REGR. vs. 158387
 test-amd64-amd64-libvirt-pair 25 guest-start/debian  fail REGR. vs. 158387
 test-amd64-i386-qemut-rhel6hvm-intel 12 redhat-install   fail REGR. vs. 158387
 test-amd64-amd64-qemuu-nested-amd 12 debian-hvm-install  fail REGR. vs. 158387
 test-amd64-i386-qemuu-rhel6hvm-intel 12 redhat-install   fail REGR. vs. 158387
 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install  fail REGR. vs. 158387
 test-amd64-i386-xl-qemut-win7-amd64 12 windows-install   fail REGR. vs. 158387
 test-amd64-amd64-amd64-pvgrub 12 debian-di-install   fail REGR. vs. 158387
 test-amd64-amd64-pygrub  12 debian-di-installfail REGR. vs. 158387
 test-amd64-amd64-qemuu-nested-intel 12 debian-hvm-install fail REGR. vs. 158387
 test-amd64-amd64-xl-qemut-win7-amd64 12 windows-install  fail REGR. vs. 158387
 test-amd64-amd64-i386-pvgrub 12 debian-di-installfail REGR. vs. 158387
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-amd64-i386-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-arm64-arm64-xl-xsm  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 158387
 test-arm64-arm64-libvirt-xsm 14 guest-start  fail REGR. vs. 158387
 test-arm64-arm64-xl-credit2  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. 
vs. 158387
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 12 debian-hvm-install 
fail REGR. vs. 158387
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-amd64-xl-qemut-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-amd64-i386-xl-qemut-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-amd64-xl-qcow212 debian-di-installfail REGR. vs. 158387
 test-amd64-amd64-libvirt-vhd 12 debian-di-installfail REGR. vs. 158387
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 12 debian-hvm-install 
fail 

[ovmf test] 158985: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf e806bb29cfde1b242bb37e72e77364dd812830e0
baseline version:
 ovmf 618e6a1f21a11eaee0a92e19c753969eb4a1b198

Last test of basis   158975  2021-02-03 07:14:36 Z0 days
Testing same since   158985  2021-02-03 13:10:40 Z0 days1 attempts


People who touched revisions under test:
  Jason Lou 
  Lou, Yun 

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
   618e6a1f21..e806bb29cf  e806bb29cfde1b242bb37e72e77364dd812830e0 -> 
xen-tested-master

Re: [PATCH] xen/netback: avoid race in xenvif_rx_ring_slots_available()

[Jakub Kicinski]

On Tue,  2 Feb 2021 08:09:38 +0100 Juergen Gross wrote:
> Since commit 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> xenvif_rx_ring_slots_available() is no longer called only from the rx
> queue kernel thread, so it needs to access the rx queue with the
> associated queue held.
> 
> Reported-by: Igor Druzhinin 
> Fixes: 23025393dbeb3b8b3 ("xen/netback: use lateeoi irq binding")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Juergen Gross 

Should we route this change via networking trees? I see the bug did not
go through networking :)

[PATCH RFC v1 6/6] xen-swiotlb: enable 64-bit xen-swiotlb

[Dongli Zhang]

This patch is to enable the 64-bit xen-swiotlb buffer.

For Xen PVM DMA address, the 64-bit device will be able to allocate from
64-bit swiotlb buffer.

Cc: Joe Jin 
Signed-off-by: Dongli Zhang 
---
 drivers/xen/swiotlb-xen.c | 117 --
 1 file changed, 74 insertions(+), 43 deletions(-)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index e18cae693cdc..c9ab07809e32 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -108,27 +108,36 @@ static int is_xen_swiotlb_buffer(struct device *dev, 
dma_addr_t dma_addr)
unsigned long bfn = XEN_PFN_DOWN(dma_to_phys(dev, dma_addr));
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
phys_addr_t paddr = (phys_addr_t)xen_pfn << XEN_PAGE_SHIFT;
+   int i;
 
/* If the address is outside our domain, it CAN
 * have the same virtual address as another address
 * in our domain. Therefore _only_ check address within our domain.
 */
-   if (pfn_valid(PFN_DOWN(paddr))) {
-   return paddr >= virt_to_phys(xen_io_tlb_start[SWIOTLB_LO]) &&
-  paddr < virt_to_phys(xen_io_tlb_end[SWIOTLB_LO]);
-   }
+   if (!pfn_valid(PFN_DOWN(paddr)))
+   return 0;
+
+   for (i = 0; i < swiotlb_nr; i++)
+   if (paddr >= virt_to_phys(xen_io_tlb_start[i]) &&
+   paddr < virt_to_phys(xen_io_tlb_end[i]))
+   return 1;
+
return 0;
 }
 
 static int
-xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
+xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs,
+ enum swiotlb_t type)
 {
int i, rc;
int dma_bits;
dma_addr_t dma_handle;
phys_addr_t p = virt_to_phys(buf);
 
-   dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
+   if (type == SWIOTLB_HI)
+   dma_bits = max_dma_bits[SWIOTLB_HI];
+   else
+   dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + 
PAGE_SHIFT;
 
i = 0;
do {
@@ -139,7 +148,7 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long 
nslabs)
p + (i << IO_TLB_SHIFT),
get_order(slabs << IO_TLB_SHIFT),
dma_bits, _handle);
-   } while (rc && dma_bits++ < max_dma_bits[SWIOTLB_LO]);
+   } while (rc && dma_bits++ < max_dma_bits[type]);
if (rc)
return rc;
 
@@ -147,16 +156,17 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long 
nslabs)
} while (i < nslabs);
return 0;
 }
-static unsigned long xen_set_nslabs(unsigned long nr_tbl)
+
+static unsigned long xen_set_nslabs(unsigned long nr_tbl, enum swiotlb_t type)
 {
if (!nr_tbl) {
-   xen_io_tlb_nslabs[SWIOTLB_LO] = (64 * 1024 * 1024 >> 
IO_TLB_SHIFT);
-   xen_io_tlb_nslabs[SWIOTLB_LO] = 
ALIGN(xen_io_tlb_nslabs[SWIOTLB_LO],
+   xen_io_tlb_nslabs[type] = (64 * 1024 * 1024 >> IO_TLB_SHIFT);
+   xen_io_tlb_nslabs[type] = ALIGN(xen_io_tlb_nslabs[type],
  IO_TLB_SEGSIZE);
} else
-   xen_io_tlb_nslabs[SWIOTLB_LO] = nr_tbl;
+   xen_io_tlb_nslabs[type] = nr_tbl;
 
-   return xen_io_tlb_nslabs[SWIOTLB_LO] << IO_TLB_SHIFT;
+   return xen_io_tlb_nslabs[type] << IO_TLB_SHIFT;
 }
 
 enum xen_swiotlb_err {
@@ -180,23 +190,24 @@ static const char *xen_swiotlb_error(enum xen_swiotlb_err 
err)
}
return "";
 }
-int __ref xen_swiotlb_init(int verbose, bool early)
+
+static int xen_swiotlb_init_type(int verbose, bool early, enum swiotlb_t type)
 {
unsigned long bytes, order;
int rc = -ENOMEM;
enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
unsigned int repeat = 3;
 
-   xen_io_tlb_nslabs[SWIOTLB_LO] = swiotlb_nr_tbl(SWIOTLB_LO);
+   xen_io_tlb_nslabs[type] = swiotlb_nr_tbl(type);
 retry:
-   bytes = xen_set_nslabs(xen_io_tlb_nslabs[SWIOTLB_LO]);
-   order = get_order(xen_io_tlb_nslabs[SWIOTLB_LO] << IO_TLB_SHIFT);
+   bytes = xen_set_nslabs(xen_io_tlb_nslabs[type], type);
+   order = get_order(xen_io_tlb_nslabs[type] << IO_TLB_SHIFT);
 
/*
 * IO TLB memory already allocated. Just use it.
 */
-   if (io_tlb_start[SWIOTLB_LO] != 0) {
-   xen_io_tlb_start[SWIOTLB_LO] = 
phys_to_virt(io_tlb_start[SWIOTLB_LO]);
+   if (io_tlb_start[type] != 0) {
+   xen_io_tlb_start[type] = phys_to_virt(io_tlb_start[type]);
goto end;
}
 
@@ -204,81 +215,95 @@ int __ref xen_swiotlb_init(int verbose, bool early)
 * Get IO TLB memory from any location.
 */
if (early) {
-   xen_io_tlb_start[SWIOTLB_LO] = memblock_alloc(PAGE_ALIGN(bytes),
+   xen_io_tlb_start[type] = 

[PATCH RFC v1 5/6] xen-swiotlb: convert variables to arrays

[Dongli Zhang]

This patch converts several xen-swiotlb related variables to arrays, in
order to maintain stat/status for different swiotlb buffers. Here are
variables involved:

- xen_io_tlb_start and xen_io_tlb_end
- xen_io_tlb_nslabs
- MAX_DMA_BITS

There is no functional change and this is to prepare to enable 64-bit
xen-swiotlb.

Cc: Joe Jin 
Signed-off-by: Dongli Zhang 
---
 drivers/xen/swiotlb-xen.c | 75 +--
 1 file changed, 40 insertions(+), 35 deletions(-)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 662638093542..e18cae693cdc 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -39,15 +39,17 @@
 #include 
 
 #include 
-#define MAX_DMA_BITS 32
 /*
  * Used to do a quick range check in swiotlb_tbl_unmap_single and
  * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by 
this
  * API.
  */
 
-static char *xen_io_tlb_start, *xen_io_tlb_end;
-static unsigned long xen_io_tlb_nslabs;
+static char *xen_io_tlb_start[SWIOTLB_MAX], *xen_io_tlb_end[SWIOTLB_MAX];
+static unsigned long xen_io_tlb_nslabs[SWIOTLB_MAX];
+
+static int max_dma_bits[] = {32, 64};
+
 /*
  * Quick lookup value of the bus address of the IOTLB.
  */
@@ -112,8 +114,8 @@ static int is_xen_swiotlb_buffer(struct device *dev, 
dma_addr_t dma_addr)
 * in our domain. Therefore _only_ check address within our domain.
 */
if (pfn_valid(PFN_DOWN(paddr))) {
-   return paddr >= virt_to_phys(xen_io_tlb_start) &&
-  paddr < virt_to_phys(xen_io_tlb_end);
+   return paddr >= virt_to_phys(xen_io_tlb_start[SWIOTLB_LO]) &&
+  paddr < virt_to_phys(xen_io_tlb_end[SWIOTLB_LO]);
}
return 0;
 }
@@ -137,7 +139,7 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long 
nslabs)
p + (i << IO_TLB_SHIFT),
get_order(slabs << IO_TLB_SHIFT),
dma_bits, _handle);
-   } while (rc && dma_bits++ < MAX_DMA_BITS);
+   } while (rc && dma_bits++ < max_dma_bits[SWIOTLB_LO]);
if (rc)
return rc;
 
@@ -148,12 +150,13 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long 
nslabs)
 static unsigned long xen_set_nslabs(unsigned long nr_tbl)
 {
if (!nr_tbl) {
-   xen_io_tlb_nslabs = (64 * 1024 * 1024 >> IO_TLB_SHIFT);
-   xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs, IO_TLB_SEGSIZE);
+   xen_io_tlb_nslabs[SWIOTLB_LO] = (64 * 1024 * 1024 >> 
IO_TLB_SHIFT);
+   xen_io_tlb_nslabs[SWIOTLB_LO] = 
ALIGN(xen_io_tlb_nslabs[SWIOTLB_LO],
+ IO_TLB_SEGSIZE);
} else
-   xen_io_tlb_nslabs = nr_tbl;
+   xen_io_tlb_nslabs[SWIOTLB_LO] = nr_tbl;
 
-   return xen_io_tlb_nslabs << IO_TLB_SHIFT;
+   return xen_io_tlb_nslabs[SWIOTLB_LO] << IO_TLB_SHIFT;
 }
 
 enum xen_swiotlb_err {
@@ -184,16 +187,16 @@ int __ref xen_swiotlb_init(int verbose, bool early)
enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
unsigned int repeat = 3;
 
-   xen_io_tlb_nslabs = swiotlb_nr_tbl(SWIOTLB_LO);
+   xen_io_tlb_nslabs[SWIOTLB_LO] = swiotlb_nr_tbl(SWIOTLB_LO);
 retry:
-   bytes = xen_set_nslabs(xen_io_tlb_nslabs);
-   order = get_order(xen_io_tlb_nslabs << IO_TLB_SHIFT);
+   bytes = xen_set_nslabs(xen_io_tlb_nslabs[SWIOTLB_LO]);
+   order = get_order(xen_io_tlb_nslabs[SWIOTLB_LO] << IO_TLB_SHIFT);
 
/*
 * IO TLB memory already allocated. Just use it.
 */
if (io_tlb_start[SWIOTLB_LO] != 0) {
-   xen_io_tlb_start = phys_to_virt(io_tlb_start[SWIOTLB_LO]);
+   xen_io_tlb_start[SWIOTLB_LO] = 
phys_to_virt(io_tlb_start[SWIOTLB_LO]);
goto end;
}
 
@@ -201,76 +204,78 @@ int __ref xen_swiotlb_init(int verbose, bool early)
 * Get IO TLB memory from any location.
 */
if (early) {
-   xen_io_tlb_start = memblock_alloc(PAGE_ALIGN(bytes),
+   xen_io_tlb_start[SWIOTLB_LO] = memblock_alloc(PAGE_ALIGN(bytes),
  PAGE_SIZE);
-   if (!xen_io_tlb_start)
+   if (!xen_io_tlb_start[SWIOTLB_LO])
panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
  __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
} else {
 #define SLABS_PER_PAGE (1 << (PAGE_SHIFT - IO_TLB_SHIFT))
 #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
while ((SLABS_PER_PAGE << order) > IO_TLB_MIN_SLABS) {
-   xen_io_tlb_start = (void 
*)xen_get_swiotlb_free_pages(order);
-   if (xen_io_tlb_start)
+   xen_io_tlb_start[SWIOTLB_LO] = (void 
*)xen_get_swiotlb_free_pages(order);
+   if 

[PATCH RFC v1 4/6] swiotlb: enable 64-bit swiotlb

[Dongli Zhang]

This patch is to enable the 64-bit swiotlb buffer.

The state of the art swiotlb pre-allocates <=32-bit memory in order to meet
the DMA mask requirement for some 32-bit legacy device. Considering most
devices nowadays support 64-bit DMA and IOMMU is available, the swiotlb is
not used for most of the times, except:

1. The xen PVM domain requires the DMA addresses to both (1) less than the
dma mask, and (2) continuous in machine address. Therefore, the 64-bit
device may still require swiotlb on PVM domain.

2. From source code the AMD SME/SEV will enable SWIOTLB_FORCE. As a result
it is always required to allocate from swiotlb buffer even the device dma
mask is 64-bit.

sme_early_init()
-> if (sev_active())
   swiotlb_force = SWIOTLB_FORCE;

Therefore, this patch introduces the 2nd swiotlb buffer for 64-bit DMA
access. For instance, the swiotlb_tbl_map_single() allocates from the 2nd
64-bit buffer if the device DMA mask is
min_not_zero(*hwdev->dma_mask, hwdev->bus_dma_limit).

The example to configure 64-bit swiotlb is "swiotlb=65536,524288,force"
or "swiotlb=,524288,force", where 524288 is the size of 64-bit buffer.

With the patch, the kernel will be able to allocate >4GB swiotlb buffer.
This patch is only for swiotlb, not including xen-swiotlb.

Cc: Joe Jin 
Signed-off-by: Dongli Zhang 
---
 arch/mips/cavium-octeon/dma-octeon.c |   3 +-
 arch/powerpc/kernel/dma-swiotlb.c|   2 +-
 arch/powerpc/platforms/pseries/svm.c |   2 +-
 arch/x86/kernel/pci-swiotlb.c|   5 +-
 arch/x86/pci/sta2x11-fixup.c |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_internal.c |   4 +-
 drivers/gpu/drm/i915/i915_scatterlist.h  |   2 +-
 drivers/gpu/drm/nouveau/nouveau_ttm.c|   2 +-
 drivers/mmc/host/sdhci.c |   2 +-
 drivers/pci/xen-pcifront.c   |   2 +-
 drivers/xen/swiotlb-xen.c|   9 +-
 include/linux/swiotlb.h  |  28 +-
 kernel/dma/swiotlb.c | 339 +++
 13 files changed, 238 insertions(+), 164 deletions(-)

diff --git a/arch/mips/cavium-octeon/dma-octeon.c 
b/arch/mips/cavium-octeon/dma-octeon.c
index df70308db0e6..3480555d908a 100644
--- a/arch/mips/cavium-octeon/dma-octeon.c
+++ b/arch/mips/cavium-octeon/dma-octeon.c
@@ -245,6 +245,7 @@ void __init plat_swiotlb_setup(void)
panic("%s: Failed to allocate %zu bytes align=%lx\n",
  __func__, swiotlbsize, PAGE_SIZE);
 
-   if (swiotlb_init_with_tbl(octeon_swiotlb, swiotlb_nslabs, 1) == -ENOMEM)
+   if (swiotlb_init_with_tbl(octeon_swiotlb, swiotlb_nslabs,
+ SWIOTLB_LO, 1) == -ENOMEM)
panic("Cannot allocate SWIOTLB buffer");
 }
diff --git a/arch/powerpc/kernel/dma-swiotlb.c 
b/arch/powerpc/kernel/dma-swiotlb.c
index fc7816126a40..88113318c53f 100644
--- a/arch/powerpc/kernel/dma-swiotlb.c
+++ b/arch/powerpc/kernel/dma-swiotlb.c
@@ -20,7 +20,7 @@ void __init swiotlb_detect_4g(void)
 static int __init check_swiotlb_enabled(void)
 {
if (ppc_swiotlb_enable)
-   swiotlb_print_info();
+   swiotlb_print_info(SWIOTLB_LO);
else
swiotlb_exit();
 
diff --git a/arch/powerpc/platforms/pseries/svm.c 
b/arch/powerpc/platforms/pseries/svm.c
index 9f8842d0da1f..77910e4ffad8 100644
--- a/arch/powerpc/platforms/pseries/svm.c
+++ b/arch/powerpc/platforms/pseries/svm.c
@@ -52,7 +52,7 @@ void __init svm_swiotlb_init(void)
bytes = io_tlb_nslabs << IO_TLB_SHIFT;
 
vstart = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
-   if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, false))
+   if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, SWIOTLB_LO, 
false))
return;
 
if (io_tlb_start[SWIOTLB_LO])
diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
index c2cfa5e7c152..950624fd95a4 100644
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -67,12 +67,15 @@ void __init pci_swiotlb_init(void)
 
 void __init pci_swiotlb_late_init(void)
 {
+   int i;
+
/* An IOMMU turned us off. */
if (!swiotlb)
swiotlb_exit();
else {
printk(KERN_INFO "PCI-DMA: "
   "Using software bounce buffering for IO (SWIOTLB)\n");
-   swiotlb_print_info();
+   for (i = 0; i < swiotlb_nr; i++)
+   swiotlb_print_info(i);
}
 }
diff --git a/arch/x86/pci/sta2x11-fixup.c b/arch/x86/pci/sta2x11-fixup.c
index 7d2525691854..c440520b2055 100644
--- a/arch/x86/pci/sta2x11-fixup.c
+++ b/arch/x86/pci/sta2x11-fixup.c
@@ -57,7 +57,7 @@ static void sta2x11_new_instance(struct pci_dev *pdev)
int size = STA2X11_SWIOTLB_SIZE;
/* First instance: register your own swiotlb area */
dev_info(>dev, "Using SWIOTLB (size %i)\n", size);
-   if 

[PATCH RFC v1 2/6] swiotlb: convert variables to arrays

[Dongli Zhang]

This patch converts several swiotlb related variables to arrays, in
order to maintain stat/status for different swiotlb buffers. Here are
variables involved:

- io_tlb_start and io_tlb_end
- io_tlb_nslabs and io_tlb_used
- io_tlb_list
- io_tlb_index
- max_segment
- io_tlb_orig_addr
- no_iotlb_memory

There is no functional change and this is to prepare to enable 64-bit
swiotlb.

Cc: Joe Jin 
Signed-off-by: Dongli Zhang 
---
 arch/powerpc/platforms/pseries/svm.c |   6 +-
 drivers/xen/swiotlb-xen.c|   4 +-
 include/linux/swiotlb.h  |   5 +-
 kernel/dma/swiotlb.c | 257 ++-
 4 files changed, 140 insertions(+), 132 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/svm.c 
b/arch/powerpc/platforms/pseries/svm.c
index 7b739cc7a8a9..9f8842d0da1f 100644
--- a/arch/powerpc/platforms/pseries/svm.c
+++ b/arch/powerpc/platforms/pseries/svm.c
@@ -55,9 +55,9 @@ void __init svm_swiotlb_init(void)
if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, false))
return;
 
-   if (io_tlb_start)
-   memblock_free_early(io_tlb_start,
-   PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
+   if (io_tlb_start[SWIOTLB_LO])
+   memblock_free_early(io_tlb_start[SWIOTLB_LO],
+   PAGE_ALIGN(io_tlb_nslabs[SWIOTLB_LO] << 
IO_TLB_SHIFT));
panic("SVM: Cannot allocate SWIOTLB buffer");
 }
 
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 2b385c1b4a99..3261880ad859 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -192,8 +192,8 @@ int __ref xen_swiotlb_init(int verbose, bool early)
/*
 * IO TLB memory already allocated. Just use it.
 */
-   if (io_tlb_start != 0) {
-   xen_io_tlb_start = phys_to_virt(io_tlb_start);
+   if (io_tlb_start[SWIOTLB_LO] != 0) {
+   xen_io_tlb_start = phys_to_virt(io_tlb_start[SWIOTLB_LO]);
goto end;
}
 
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index ca125c1b1281..777046cd4d1b 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -76,11 +76,12 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t phys,
 
 #ifdef CONFIG_SWIOTLB
 extern enum swiotlb_force swiotlb_force;
-extern phys_addr_t io_tlb_start, io_tlb_end;
+extern phys_addr_t io_tlb_start[], io_tlb_end[];
 
 static inline bool is_swiotlb_buffer(phys_addr_t paddr)
 {
-   return paddr >= io_tlb_start && paddr < io_tlb_end;
+   return paddr >= io_tlb_start[SWIOTLB_LO] &&
+  paddr < io_tlb_end[SWIOTLB_LO];
 }
 
 void __init swiotlb_exit(void);
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 7c42df6e6100..1fbb65daa2dd 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -69,38 +69,38 @@ enum swiotlb_force swiotlb_force;
  * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by 
this
  * API.
  */
-phys_addr_t io_tlb_start, io_tlb_end;
+phys_addr_t io_tlb_start[SWIOTLB_MAX], io_tlb_end[SWIOTLB_MAX];
 
 /*
  * The number of IO TLB blocks (in groups of 64) between io_tlb_start and
  * io_tlb_end.  This is command line adjustable via setup_io_tlb_npages.
  */
-static unsigned long io_tlb_nslabs;
+static unsigned long io_tlb_nslabs[SWIOTLB_MAX];
 
 /*
  * The number of used IO TLB block
  */
-static unsigned long io_tlb_used;
+static unsigned long io_tlb_used[SWIOTLB_MAX];
 
 /*
  * This is a free list describing the number of free entries available from
  * each index
  */
-static unsigned int *io_tlb_list;
-static unsigned int io_tlb_index;
+static unsigned int *io_tlb_list[SWIOTLB_MAX];
+static unsigned int io_tlb_index[SWIOTLB_MAX];
 
 /*
  * Max segment that we can provide which (if pages are contingous) will
  * not be bounced (unless SWIOTLB_FORCE is set).
  */
-static unsigned int max_segment;
+static unsigned int max_segment[SWIOTLB_MAX];
 
 /*
  * We need to save away the original address corresponding to a mapped entry
  * for the sync operations.
  */
 #define INVALID_PHYS_ADDR (~(phys_addr_t)0)
-static phys_addr_t *io_tlb_orig_addr;
+static phys_addr_t *io_tlb_orig_addr[SWIOTLB_MAX];
 
 /*
  * Protect the above data structures in the map and unmap calls
@@ -113,9 +113,9 @@ static int __init
 setup_io_tlb_npages(char *str)
 {
if (isdigit(*str)) {
-   io_tlb_nslabs = simple_strtoul(str, , 0);
+   io_tlb_nslabs[SWIOTLB_LO] = simple_strtoul(str, , 0);
/* avoid tail segment of size < IO_TLB_SEGSIZE */
-   io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
+   io_tlb_nslabs[SWIOTLB_LO] = ALIGN(io_tlb_nslabs[SWIOTLB_LO], 
IO_TLB_SEGSIZE);
}
if (*str == ',')
++str;
@@ -123,40 +123,40 @@ setup_io_tlb_npages(char *str)
swiotlb_force = SWIOTLB_FORCE;
} else if (!strcmp(str, "noforce")) {

[PATCH RFC v1 3/6] swiotlb: introduce swiotlb_get_type() to calculate swiotlb buffer type

[Dongli Zhang]

This patch introduces swiotlb_get_type() in order to calculate which
swiotlb buffer the given DMA address is belong to.

This is to prepare to enable 64-bit swiotlb.

Cc: Joe Jin 
Signed-off-by: Dongli Zhang 
---
 include/linux/swiotlb.h | 14 ++
 kernel/dma/swiotlb.c|  2 ++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 777046cd4d1b..3d5980d77810 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -3,6 +3,7 @@
 #define __LINUX_SWIOTLB_H
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -23,6 +24,8 @@ enum swiotlb_t {
SWIOTLB_MAX,
 };
 
+extern int swiotlb_nr;
+
 /*
  * Maximum allowable number of contiguous slabs to map,
  * must be a power of 2.  What is the appropriate value ?
@@ -84,6 +87,17 @@ static inline bool is_swiotlb_buffer(phys_addr_t paddr)
   paddr < io_tlb_end[SWIOTLB_LO];
 }
 
+static inline int swiotlb_get_type(phys_addr_t paddr)
+{
+   int i;
+
+   for (i = 0; i < swiotlb_nr; i++)
+   if (paddr >= io_tlb_start[i] && paddr < io_tlb_end[i])
+   return i;
+
+   return -ENOENT;
+}
+
 void __init swiotlb_exit(void);
 unsigned int swiotlb_max_segment(void);
 size_t swiotlb_max_mapping_size(struct device *dev);
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 1fbb65daa2dd..c91d3d2c3936 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -109,6 +109,8 @@ static DEFINE_SPINLOCK(io_tlb_lock);
 
 static int late_alloc;
 
+int swiotlb_nr = 1;
+
 static int __init
 setup_io_tlb_npages(char *str)
 {
-- 
2.17.1

[PATCH RFC v1 0/6] swiotlb: 64-bit DMA buffer

[Dongli Zhang]

This RFC is to introduce the 2nd swiotlb buffer for 64-bit DMA access.  The
prototype is based on v5.11-rc6.

The state of the art swiotlb pre-allocates <=32-bit memory in order to meet
the DMA mask requirement for some 32-bit legacy device. Considering most
devices nowadays support 64-bit DMA and IOMMU is available, the swiotlb is
not used for most of times, except:

1. The xen PVM domain requires the DMA addresses to both (1) <= the device
dma mask, and (2) continuous in machine address. Therefore, the 64-bit
device may still require swiotlb on PVM domain.

2. From source code the AMD SME/SEV will enable SWIOTLB_FORCE. As a result
it is always required to allocate from swiotlb buffer even the device dma
mask is 64-bit.

sme_early_init()
-> if (sev_active())
   swiotlb_force = SWIOTLB_FORCE;


Therefore, this RFC introduces the 2nd swiotlb buffer for 64-bit DMA
access.  For instance, the swiotlb_tbl_map_single() allocates from the 2nd
64-bit buffer if the device DMA mask min_not_zero(*hwdev->dma_mask,
hwdev->bus_dma_limit) is 64-bit.  With the RFC, the Xen/AMD will be able to
allocate >4GB swiotlb buffer.

With it being 64-bit, you can (not in this patch set but certainly
possible) allocate this at runtime. Meaning the size could change depending
on the device MMIO buffers, etc.


I have tested the patch set on Xen PVM dom0 boot via QEMU. The dom0 is boot
via:

qemu-system-x86_64 -smp 8 -m 20G -enable-kvm -vnc :9 \
-net nic -net user,hostfwd=tcp::5029-:22 \
-hda disk.img \
-device nvme,drive=nvme0,serial=deudbeaf1,max_ioqpairs=16 \
-drive file=test.qcow2,if=none,id=nvme0 \
-serial stdio

The "swiotlb=65536,1048576,force" is to configure 32-bit swiotlb as 128MB
and 64-bit swiotlb as 2048MB. The swiotlb is enforced.

vm# cat /proc/cmdline 
placeholder root=UUID=4e942d60-c228-4caf-b98e-f41c365d9703 ro text
swiotlb=65536,1048576,force quiet splash

[5.119877] Booting paravirtualized kernel on Xen
... ...
[5.190423] software IO TLB: Low Mem mapped [mem 
0x000234e0-0x00023ce0] (128MB)
[6.276161] software IO TLB: High Mem mapped [mem 
0x000166f33000-0x0001e6f33000] (2048MB)

0x000234e0 is mapped to 0x001c (32-bit machine address)
0x00023ce0-1 is mapped to 0x0ff3 (32-bit machine address)
0x000166f33000 is mapped to 0x0004b728 (64-bit machine address)
0x0001e6f33000-1 is mapped to 0x00033a07 (64-bit machine address)


While running fio for emulated-NVMe, the swiotlb is allocating from 64-bit
io_tlb_used-highmem.

vm# cat /sys/kernel/debug/swiotlb/io_tlb_nslabs
65536
vm# cat /sys/kernel/debug/swiotlb/io_tlb_used
258
vm# cat /sys/kernel/debug/swiotlb/io_tlb_nslabs-highmem
1048576
vm# cat /sys/kernel/debug/swiotlb/io_tlb_used-highmem
58880


I also tested virtio-scsi (with "disable-legacy=on,iommu_platform=true") on
VM with AMD SEV enabled.

qemu-system-x86_64 -enable-kvm -machine q35 -smp 36 -m 20G \
-drive if=pflash,format=raw,unit=0,file=OVMF_CODE.pure-efi.fd,readonly \
-drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd \
-hda ol7-uefi.qcow2 -serial stdio -vnc :9 \
-net nic -net user,hostfwd=tcp::5029-:22 \
-cpu EPYC -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \
-machine memory-encryption=sev0 \
-device virtio-scsi-pci,id=scsi,disable-legacy=on,iommu_platform=true \
-device scsi-hd,drive=disk0 \
-drive file=test.qcow2,if=none,id=disk0,format=qcow2

The "swiotlb=65536,1048576" is to configure 32-bit swiotlb as 128MB and
64-bit swiotlb as 2048MB. We do not need to force swiotlb because AMD SEV
will set SWIOTLB_FORCE.

# cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-5.11.0-rc6swiotlb+ root=/dev/mapper/ol-root ro
crashkernel=auto rd.lvm.lv=ol/root rd.lvm.lv=ol/swap rhgb quiet
LANG=en_US.UTF-8 swiotlb=65536,1048576

[0.729790] AMD Memory Encryption Features active: SEV
... ...
[2.113147] software IO TLB: Low Mem mapped [mem 
0x73e1e000-0x7be1e000] (128MB)
[2.113151] software IO TLB: High Mem mapped [mem 
0x0004e840-0x00056840] (2048MB)

While running fio for virtio-scsi, the swiotlb is allocating from 64-bit
io_tlb_used-highmem.

vm# cat /sys/kernel/debug/swiotlb/io_tlb_nslabs
65536
vm# cat /sys/kernel/debug/swiotlb/io_tlb_used
0
vm# cat /sys/kernel/debug/swiotlb/io_tlb_nslabs-highmem
1048576
vm# cat /sys/kernel/debug/swiotlb/io_tlb_used-highmem
64647


Please let me know if there is any feedback for this idea and RFC.


Dongli Zhang (6):
   swiotlb: define new enumerated type
   swiotlb: convert variables to arrays
   swiotlb: introduce swiotlb_get_type() to calculate swiotlb buffer type
   swiotlb: enable 64-bit swiotlb
   xen-swiotlb: convert variables to arrays
   xen-swiotlb: enable 64-bit xen-swiotlb

 arch/mips/cavium-octeon/dma-octeon.c |   3 +-
 arch/powerpc/kernel/dma-swiotlb.c|   2 +-
 arch/powerpc/platforms/pseries/svm.c |   8 +-
 arch/x86/kernel/pci-swiotlb.c|   5 +-
 arch/x86/pci/sta2x11-fixup.c   

[PATCH RFC v1 1/6] swiotlb: define new enumerated type

[Dongli Zhang]

This is just to define new enumerated type without functional change.

The 'SWIOTLB_LO' is to index legacy 32-bit swiotlb buffer, while the
'SWIOTLB_HI' is to index the 64-bit buffer.

This is to prepare to enable 64-bit swiotlb.

Cc: Joe Jin 
Signed-off-by: Dongli Zhang 
---
 include/linux/swiotlb.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index d9c9fc9ca5d2..ca125c1b1281 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -17,6 +17,12 @@ enum swiotlb_force {
SWIOTLB_NO_FORCE,   /* swiotlb=noforce */
 };
 
+enum swiotlb_t {
+   SWIOTLB_LO,
+   SWIOTLB_HI,
+   SWIOTLB_MAX,
+};
+
 /*
  * Maximum allowable number of contiguous slabs to map,
  * must be a power of 2.  What is the appropriate value ?
-- 
2.17.1

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

[osstest service owner]

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

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  d203dbd69f1a02577dd6fe571d72beb980c548a6
baseline version:
 xen  5e7aa904405fa2f268c3af213516bae271de3265

Last test of basis   158950  2021-02-02 11:01:24 Z1 days
Testing same since   158993  2021-02-03 21:00:26 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 

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
   5e7aa90440..d203dbd69f  d203dbd69f1a02577dd6fe571d72beb980c548a6 -> smoke

Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses

[Julien Grall]

On Wed, 3 Feb 2021 at 22:18, Stefano Stabellini  wrote:
> > > But aside from PCIe, let's say that we know of a few nodes for which
> > > "reg" needs a special treatment. I am not sure it makes sense to proceed
> > > with parsing those nodes without knowing how to deal with that.
> >
> > I believe that most of the time the "special" treatment would be to ignore 
> > the
> > property "regs" as it will not be an CPU memory address.
> >
> > > So maybe
> > > we should add those nodes to skip_matches until we know what to do with
> > > them. At that point, I would imagine we would introduce a special
> > > handle_device function that knows what to do. In the case of PCIe,
> > > something like "handle_device_pcie".
> > Could you outline how "handle_device_pcie()" will differ with handle_node()?
> >
> > In fact, the problem is not the PCIe node directly. Instead, it is the 
> > second
> > level of nodes below it (i.e usb@...).
> >
> > The current implementation of dt_number_of_address() only look at the bus 
> > type
> > of the parent. As the parent has no bus type and "ranges" then it thinks 
> > this
> > is something we can translate to a CPU address.
> >
> > However, this is below a PCI bus so the meaning of "reg" is completely
> > different. In this case, we only need to ignore "reg".
>
> I see what you are saying and I agree: if we had to introduce a special
> case for PCI, then  dt_number_of_address() seems to be a good place.  In
> fact, we already have special PCI handling, see our
> __dt_translate_address function and xen/common/device_tree.c:dt_busses.
>
> Which brings the question: why is this actually failing?

I already hinted at the reason in my previous e-mail :). Let me expand
a bit more.

>
> pcie0 {
>  ranges = <0x0200 0x0 0xc000 0x6 0x 0x0 0x4000>;
>
> Which means that PCI addresses 0xc000-0x1 become 
> 0x6-0x7.
>
> The offending DT is:
>
>  {
>  pci@1,0 {
>  #address-cells = <3>;
>  #size-cells = <2>;
>  ranges;
>
>  reg = <0 0 0 0 0>;
>
>  usb@1,0 {
>  reg = <0x1 0 0 0 0>;
>  resets = < RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
>  };
>  };
> };
>
>
> reg = <0x1 0 0 0 0> means that usb@1,0 is PCI device 01:00.0.
> However, the rest of the regs cells are left as zero. It shouldn't be an
> issue because usb@1,0 is a child of pci@1,0 but pci@1,0 is not a bus.

The property "ranges" is used to define a mapping or translation
between the address space of the "bus" (here pci@1,0) and the address
space of the bus node's parent ().
IOW, it means "reg" in usb@1,0 is an address on the PCI bus (i.e. BDF).

The problem is dt_number_of_address() will only look at the "bus" type
of the parent using dt_match_bus(). This will return the default bus
(see dt_bus_default_match()), because this is a property "ranges" in
the parent node (i.e. pci@1,0). Therefore...

> So
> in theory dt_number_of_address() should already return 0 for it.

... dt_number_of_address() will return 1 even if the address is not a
CPU address. So when Xen will try to translate it, it will fail.

>
> Maybe reg = <0 0 0 0 0> is the problem. In that case, we could simply
> add a check to skip 0 size ranges. Just a hack to explain what I mean:

The parent of pci@1,0 is a PCI bridge (see the property type), so the
CPU addresses are found not via "regs" but "assigned-addresses".

In this situation, "regs" will have a different meaning and therefore
there is no promise that the size will be 0.

Cheers,

Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses

[Stefano Stabellini]

On Wed, 3 Feb 2021, Julien Grall wrote:
> On 03/02/2021 00:18, Stefano Stabellini wrote:
> > On Tue, 2 Feb 2021, Julien Grall wrote:
> > > On 02/02/2021 18:12, Julien Grall wrote:
> > > > On 02/02/2021 17:47, Elliott Mitchell wrote:
> > > > > The handle_device() function has been returning failure upon
> > > > > encountering a device address which was invalid.  A device tree which
> > > > > had such an entry has now been seen in the wild.  As it causes no
> > > > > failures to simply ignore the entries, ignore them. >
> > > > > Signed-off-by: Elliott Mitchell 
> > > > > 
> > > > > ---
> > > > > I'm starting to suspect there are an awful lot of places in the
> > > > > various
> > > > > domain_build.c files which should simply ignore errors.  This is now
> > > > > the
> > > > > second place I've encountered in 2 months where ignoring errors was
> > > > > the
> > > > > correct action.
> > > > 
> > > > Right, as a counterpoint, we run Xen on Arm HW for several years now and
> > > > this is the first time I heard about issue parsing the DT. So while I
> > > > appreciate that you are eager to run Xen on the RPI...
> > > > 
> > > > >   I know failing in case of error is an engineer's
> > > > > favorite approach, but there seem an awful lot of harmless failures
> > > > > causing panics.
> > > > > 
> > > > > This started as the thread "[RFC PATCH] xen/arm: domain_build: Ignore
> > > > > empty memory bank".  Now it seems clear the correct approach is to
> > > > > simply
> > > > > ignore these entries.
> > > > 
> > > > ... we first need to fully understand the issues. Here a few questions:
> > > >      1) Can you provide more information why you believe the address is
> > > > invalid?
> > > >      2) How does Linux use the node?
> > > >      3) Is it happening with all the RPI DT? If not, what are the
> > > > differences?
> > > 
> > > So I had another look at the device-tree you provided earlier on. The node
> > > is
> > > the following (copied directly from the DTS):
> > > 
> > >  {
> > >  pci@1,0 {
> > >  #address-cells = <3>;
> > >  #size-cells = <2>;
> > >  ranges;
> > > 
> > >  reg = <0 0 0 0 0>;
> > > 
> > >  usb@1,0 {
> > >  reg = <0x1 0 0 0 0>;
> > >  resets = <
> > > RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
> > >  };
> > >  };
> > > };
> > > 
> > > pcie0: pcie@7d50 {
> > > compatible = "brcm,bcm2711-pcie";
> > > reg = <0x0 0x7d50  0x0 0x9310>;
> > > device_type = "pci";
> > > #address-cells = <3>;
> > > #interrupt-cells = <1>;
> > > #size-cells = <2>;
> > > interrupts = ,
> > >  ;
> > > interrupt-names = "pcie", "msi";
> > > interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> > > interrupt-map = <0 0 0 1  GIC_SPI 143
> > >   
> > > IRQ_TYPE_LEVEL_HIGH>;
> > > msi-controller;
> > > msi-parent = <>;
> > > 
> > > ranges = <0x0200 0x0 0xc000 0x6 0x
> > >   0x0 0x4000>;
> > > /*
> > >  * The wrapper around the PCIe block has a bug
> > >  * preventing it from accessing beyond the first 3GB of
> > >  * memory.
> > >  */
> > > dma-ranges = <0x0200 0x0 0x 0x0 0x
> > >   0x0 0xc000>;
> > > brcm,enable-ssc;
> > > };
> > > 
> > > The interpretation of "reg" depends on the context. In this case, we are
> > > trying to interpret as a memory address from the CPU PoV when it has a
> > > different meaning (I am not exactly sure what).
> > > 
> > > In fact, you are lucky that Xen doesn't manage to interpret it. Xen should
> > > really stop trying to look region to map when it discover a PCI bus. I
> > > wrote a
> > > quick hack patch that should ignore it:
> > 
> > Yes, I think you are right. There are a few instances where "reg" is not
> > a address ready to be remapped. It is not just PCI, although that's the
> > most common.  Maybe we need a list, like skip_matches in handle_node.
> 
> From my understanding, "reg" can be considered as an MMIO region only if all
> the *parents up to the root have the property "ranges" and they are not on a
> different bus (e.g. pci).
> 
> Do you have example where this is not the case?

The famous one is CPUs. These are some other examples I could find with
a quick search:

nvmem_firmware {
compatible = "xlnx,zynqmp-nvmem-fw";
#address-cells = <0x1>;
#size-cells = <0x1>;

soc_revision@0 {
reg = <0x0 0x4>;
};
};

cci@fd6e {
compatible = "arm,cci-400";
reg = <0x0 0xfd6e 0x0 0x9000>;

Re: Question about xen and Rasp 4B

[Elliott Mitchell]

On Wed, Feb 03, 2021 at 12:55:40PM -0800, Stefano Stabellini wrote:
> On Wed, 3 Feb 2021, Jukka Kaartinen wrote:
> > On 3.2.2021 2.18, Stefano Stabellini wrote:
> > > How are you configuring and installing the kernel?
> > > 
> > > make bcm2711_defconfig
> > > make Image.gz
> > > make modules_install
> > > 
> > > ?
> > > 
> > > The device tree is the one from the rpi-5.9.y build? How are you loading
> > > the kernel and device tree with uboot? Do you have any interesting
> > > changes to config.txt?
> > > 
> > > I am asking because I cannot get to the point of reproducing what you
> > > are seeing: I can boot my rpi-5.9.y kernel on recent Xen but I cannot
> > > get any graphics output on my screen. (The serial works.) I am using the
> > > default Ubuntu Desktop rpi-install target as rootfs and uboot master.
> > > 
> > 
> > This is what I do:
> > 
> > make bcm2711_defconfig
> > cat "xen_additions" >> .config
> > make Image  modules dtbs
> > 
> > make INSTALL_MOD_PATH=rootfs modules_install
> > depmod -a
> > 
> > cp arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb boot/
> > cp arch/arm64/boot/dts/overlays/*.dtbo boot/overlays/
> 
> Thanks for the detailed instructions. This helps a lot. I saw below in
> boot2.source that you are using ${fdt_addr} as DTB source (instead of
> loading one), which means you are using the DTB as provided by U-Boot at
> runtime, instead of loading your own file.
> 
> With these two copies, I take you meant to update the first partition on
> the SD card, the one where config.txt lives, right? So that Xen is
> getting the DTB and overlays from the rpi-5.9.y kernel tree but passed
> down by the RPi loader and U-Boot?
> 
> I think the DTB must be the issue as I wasn't applying any overlays
> before. I ran a test to use the DTB and overlay from U-Boot but maybe I
> haven't updated them properly because I still don't see any output.

Seeing no graphics output from U-Boot is okay.  If the device-tree files
get sufficiently updated you can end up with no output from U-Boot, but
will get output once the Linux kernel's driver is operational (I've seen
this occur).

The most important part is having a HDMI display plugged in during the
early boot stages.  Unless the bootloader sees the display the output
won't get initialized and the Linux driver doesn't handle that.


> > dtoverlay=vc4-fkms-v3d,cma-64

This is odd.  My understanding is this is appropriate for RP3, but not
RP4.  For RP4 you can have "dtoverlay=disable-vc4" and still get graphics
output (hmm, I'm starting to think I need to double-check this...).


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

Re: Linux 5.11-rc6 compile error

[Shuah Khan]

On 2/3/21 1:06 PM, Linus Torvalds wrote:

On Wed, Feb 3, 2021 at 11:58 AM Shuah Khan  wrote:


ld: arch/x86/built-in.a: member arch/x86/kernel/pci-swiotlb.o in archive
is not an object
make: *** [Makefile:1170: vmlinux] Error 1


That honestly sounds like something went wrong earlier - things like
doing a system upgrade in the middle of the build, or perhaps running
out of disk space or similar.

I've not seen any other reports of the same, and google doesn't find
anything like that either.

Does it keep happening if you do a "git clean -dqfx" to make sure you
have no old corrupt object files sound and re-do the whole build?

 Linus



My bad. I was playing with two test systems this morning and totally
lost track. All is well after a "make clean" and make.

Sorry for the noise.

thanks,
-- Shuah

[xen-unstable test] 158977: trouble: broken/fail/pass

[osstest service owner]

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

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-xl-credit1  broken
 test-arm64-arm64-xl-seattle  broken
 test-arm64-arm64-xl-xsm  broken

Tests which are failing intermittently (not blocking):
 test-arm64-arm64-xl-seattle   5 host-install(5)  broken pass in 158957
 test-arm64-arm64-xl-xsm   5 host-install(5)  broken pass in 158957
 test-arm64-arm64-xl-credit1   5 host-install(5)  broken pass in 158957
 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 158957 pass in 
158977
 test-amd64-amd64-qemuu-nested-intel 20 debian-hvm-install/l1/l2 fail pass in 
158957

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

version targeted for testing:
 xen  5e7aa904405fa2f268c3af213516bae271de3265
baseline version:
 xen  

Re: Linux 5.11-rc6 compile error

[Randy Dunlap]

On 2/3/21 11:58 AM, Shuah Khan wrote:
> I am seeing the following compile error on Linux 5.11-rc6.
> No issues on 5.11.0-rc5 with the same config.
> 
> ld: arch/x86/built-in.a: member arch/x86/kernel/pci-swiotlb.o in archive is 
> not an object
> make: *** [Makefile:1170: vmlinux] Error 1
> 
> CONFIG_SWIOTLB_XEN=y
> CONFIG_SWIOTLB=y

Those config settings in allmodconfig builds for me.


> I can debug further later on today. Checking to see if there are any
> known problems.


-- 
~Randy

Re: Question about xen and Rasp 4B

[Stefano Stabellini]

On Wed, 3 Feb 2021, Jukka Kaartinen wrote:
> On 3.2.2021 2.18, Stefano Stabellini wrote:
> > On Tue, 2 Feb 2021, Jukka Kaartinen wrote:
> > > > > Good catch.
> > > > > GPU works now and I can start X! Thanks! I was also able to create
> > > > > domU
> > > > > that runs Raspian OS.
> > > > 
> > > > This is very interesting that you were able to achieve that - congrats!
> > > > 
> > > > Now, sorry to be a bit dense -- but since this thread went into all
> > > > sorts of interesting
> > > > directions all at once -- I just have a very particular question: what
> > > > is
> > > > exact
> > > > combination of versions of Xen, Linux kernel and a set of patches that
> > > > went
> > > > on top that allowed you to do that? I'd love to obviously see it
> > > > productized in Xen
> > > > upstream, but for now -- I'd love to make available to Project EVE/Xen
> > > > community
> > > > since there seems to be a few folks interested in EVE/Xen combo being
> > > > able
> > > > to
> > > > do that.
> > > 
> > > I have tried Xen Release 4.14.0, 4.14.1 and master (from week 4, 2021).
> > > 
> > > Kernel rpi-5.9.y and rpi-5.10.y branches from
> > > https://github.com/raspberrypi/linux
> > > 
> > > and
> > > 
> > > U-boot (master).
> > > 
> > > For the GPU to work it was enough to disable swiotlb from the kernel(s) as
> > > suggested in this thread.
> > 
> > How are you configuring and installing the kernel?
> > 
> > make bcm2711_defconfig
> > make Image.gz
> > make modules_install
> > 
> > ?
> > 
> > The device tree is the one from the rpi-5.9.y build? How are you loading
> > the kernel and device tree with uboot? Do you have any interesting
> > changes to config.txt?
> > 
> > I am asking because I cannot get to the point of reproducing what you
> > are seeing: I can boot my rpi-5.9.y kernel on recent Xen but I cannot
> > get any graphics output on my screen. (The serial works.) I am using the
> > default Ubuntu Desktop rpi-install target as rootfs and uboot master.
> > 
> 
> This is what I do:
> 
> make bcm2711_defconfig
> cat "xen_additions" >> .config
> make Image  modules dtbs
> 
> make INSTALL_MOD_PATH=rootfs modules_install
> depmod -a
> 
> cp arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb boot/
> cp arch/arm64/boot/dts/overlays/*.dtbo boot/overlays/

Thanks for the detailed instructions. This helps a lot. I saw below in
boot2.source that you are using ${fdt_addr} as DTB source (instead of
loading one), which means you are using the DTB as provided by U-Boot at
runtime, instead of loading your own file.

With these two copies, I take you meant to update the first partition on
the SD card, the one where config.txt lives, right? So that Xen is
getting the DTB and overlays from the rpi-5.9.y kernel tree but passed
down by the RPi loader and U-Boot?

I think the DTB must be the issue as I wasn't applying any overlays
before. I ran a test to use the DTB and overlay from U-Boot but maybe I
haven't updated them properly because I still don't see any output.


> config.txt:
> 
> [pi4]
> max_framebuffers=2
> enable_uart=1
> arm_freq=1500
> force_turbo=1
> 
> [all]
> arm_64bit=1
> kernel=u-boot.bin
> 
> start_file=start4.elf
> fixup_file=fixup4.dat
> 
> # Enable the audio output, I2C and SPI interfaces on the GPIO header
> dtparam=audio=on
> dtparam=i2c_arm=on
> dtparam=spi=on
> 
> # Enable the FKMS ("Fake" KMS) graphics overlay, enable the camera firmware
> # and allocate 128Mb to the GPU memory
> dtoverlay=vc4-fkms-v3d,cma-64
> gpu_mem=128
> 
> # Comment out the following line if the edges of the desktop appear outside
> # the edges of your display
> disable_overscan=1
> 
> 
> boot.source:
> setenv serverip 10.42.0.1
> setenv ipaddr 10.42.0.231
> tftpb 0xC0 boot2.scr
> source 0xC0
> 
> boot2.source:
> tftpb 0xE0 xen
> tftpb 0x100 Image
> setenv lin_size $filesize
> 
> fdt addr ${fdt_addr}
> fdt resize 1024
> 
> fdt set /chosen xen,xen-bootargs "console=dtuart dtuart=serial0 sync_console
> dom0_mem=1024M dom0_max_vcpus=1 bootscrub=0 vwfi=native sched=credit2"
> 
> fdt mknod /chosen dom0
> 
> # These will break the default framebuffer@3e2fe000 that
> # is the same chosen -node.
> #fdt set /chosen/dom0 \#address-cells <0x2>
> #fdt set /chosen/dom0 \#size-cells <0x2>
> 
> fdt set /chosen/dom0 compatible "xen,linux-zimage" "xen,multiboot-module"
> fdt set /chosen/dom0 reg <0x100 0x${lin_size}>
> 
> fdt set /chosen xen,dom0-bootargs "dwc_otg.lpm_enable=0 console=hvc0
> earlycon=xen earlyprintk=xen root=/dev/sda4 elevator=deadline rootwait fixrtc
> quiet splash"
> 
> setenv fdt_high 0x
> 
> fdt print /chosen
> 
> #xen
> booti 0xE0 - ${fdt_addr}
> 

Re: XSA-332 kernel patch - huge network performance on pfSense VMs

[Andrew Cooper]

On 26/01/2021 15:04, Samuel Verschelde wrote:
> Le 18/01/2021 à 11:03, Roger Pau Monné a écrit :
>> On Fri, Jan 15, 2021 at 03:03:26PM +, Samuel Verschelde wrote:
>>> Hi list,
>>>
>>> Another "popular" thread on XCP-ng forum [1], started in october 2020,
>>> allowed us to detect that patch 12 from the XSA-332 advisory [2] had
>>> a very
>>> significant impact on network performance in the case of pfSense VMs.
>>>
>>> We reproduced the issue internally (well, we reproduced "something".
>>> The
>>> user setups in this thread are diverse) and our findings seem to
>>> confirm
>>> what the users reported. Running iperf3 from the pfSense VM to a
>>> debian VM
>>> gives results around 5 times slower than before. Reverting this
>>> single patch
>>> brings the performance back. On the debian to pfSense direction, the
>>> drop is
>>> about 25%.
>>
>> pfSense is based on FreeBSD, so I would bet that whatever performance
>> degradation you are seeing would also happen with plain FreeBSD. I
>> would assume netfront in FreeBSD is triggering the ratelimit on Linux,
>> and hence it gets throttled.
>>
>> Do you think you have the bandwidth to look into the FreeBSD side and
>> try to provide a fix? I'm happy to review and commit in upstream
>> FreeBSD, but would be nice to have someone else also in the loop as
>> ATM I'm the only one doing FreeBSD/Xen development AFAIK.
>>
>> Thanks, Roger.
>>
>
> (sorry about the previous email, looks like my mail client hates me)
>
> I would personnally not be able to hack into either Xen, the linux
> kernel or FreeBSD in any efficient way. My role here is limited to
> packaging, testing and acting as a relay between users and developers.
> We currently don't have anyone at Vates who would be able to hack into
> FreeBSD either.
>
> What currently put FreeBSD on our radar is the large amount of users
> who use FreeNAS/TrueNAS or pfSense VMs, and the recent bugs they
> detected (XSA-360 and this performance drop).
>
> Additionnally, regarding this performance issue, some users report an
> impact of that same patch 12 on the network performance of their
> non-BSD VMs [1][2], so I think the FreeBSD case might be helpful to
> help identify what in that patch caused throttling (if that's what
> happens), because it's easier to reproduce, but I'm not sure fixes
> would only need to be made in FreeBSD.
>
> Best regards,
>
> Samuel Verschelde
>
> [1] https://xcp-ng.org/forum/post/35521 mentions debian based Untangle
> OS and inter-VLAN traffic
> [2] https://xcp-ng.org/forum/post/35476 general slowdown affecting all
> VMs (VM to workstation traffic), from the first user who identified
> patch 12 as the cause.

Further to this, XenServer testing has also observed a ~5x drop in
intrahost VM->VM network performance between PV VMs running under PV-Shim

As one specific case has been bisected to patch 11, its obvious that
FreeBSD's netfront is hitting dom0's new spurious-event detection and
mitigation.  It is also reasonable to presume that the other ~5x hits
are related, which means it isn't behaviour unique to the FreeBSD netfront.

The next step is to figure out whether the event is genuinely spurious
(i.e. the frontend really is sending too many notifications), or whether
dom0's judgement of spuriosity is wrong.

~Andrew

Linux 5.11-rc6 compile error

[Shuah Khan]

I am seeing the following compile error on Linux 5.11-rc6.
No issues on 5.11.0-rc5 with the same config.

ld: arch/x86/built-in.a: member arch/x86/kernel/pci-swiotlb.o in archive 
is not an object

make: *** [Makefile:1170: vmlinux] Error 1

CONFIG_SWIOTLB_XEN=y
CONFIG_SWIOTLB=y

I can debug further later on today. Checking to see if there are any
known problems.

thanks,
-- Shuah

[PATCH v2 2/2] tools/libxl: only set viridian flags on new domains

[Igor Druzhinin]

Domains migrating or restoring should have viridian HVM param key in
the migration stream already and setting that twice results in Xen
returing -EEXIST on the second attempt later (during migration stream parsing)
in case the values don't match. That causes migration/restore operation
to fail at destination side.

That issue is now resurfaced by the latest commits (983524671 and 7e5cffcd1e)
extending default viridian feature set making the values from the previous
migration streams and those set at domain construction different.

Suggested-by: Andrew Cooper 
Signed-off-by: Igor Druzhinin 
---
 tools/libs/light/libxl_x86.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 91169d1..58187ed 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -468,7 +468,10 @@ int libxl__arch_domain_create(libxl__gc *gc,
 (ret = hvm_set_conf_params(gc, domid, info)) != 0)
 goto out;
 
-if (info->type == LIBXL_DOMAIN_TYPE_HVM &&
+/* Viridian flags are already a part of the migration stream so set
+ * them here only for brand new domains. */
+if (!state->restore &&
+info->type == LIBXL_DOMAIN_TYPE_HVM &&
 (ret = hvm_set_viridian_features(gc, domid, info)) != 0)
 goto out;
 
-- 
2.7.4

[PATCH v2 1/2] tools/libxl: pass libxl__domain_build_state to libxl__arch_domain_create

[Igor Druzhinin]

No functional change.

Signed-off-by: Igor Druzhinin 
---
New patch in v2 as requested.
---
 tools/libs/light/libxl_arch.h | 6 --
 tools/libs/light/libxl_arm.c  | 4 +++-
 tools/libs/light/libxl_dom.c  | 2 +-
 tools/libs/light/libxl_x86.c  | 6 --
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
index 6a91775..c305d70 100644
--- a/tools/libs/light/libxl_arch.h
+++ b/tools/libs/light/libxl_arch.h
@@ -30,8 +30,10 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
 
 /* arch specific internal domain creation function */
 _hidden
-int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
-   uint32_t domid);
+int libxl__arch_domain_create(libxl__gc *gc,
+  libxl_domain_config *d_config,
+  libxl__domain_build_state *state,
+  uint32_t domid);
 
 /* setup arch specific hardware description, i.e. DTB on ARM */
 _hidden
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 66e8a06..8c4eda3 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -126,7 +126,9 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
 return 0;
 }
 
-int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
+int libxl__arch_domain_create(libxl__gc *gc,
+  libxl_domain_config *d_config,
+  ibxl__domain_build_state *state,
   uint32_t domid)
 {
 return 0;
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 1916857..842a51c 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -378,7 +378,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
 state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, 
state->store_domid);
 state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, 
state->console_domid);
 
-rc = libxl__arch_domain_create(gc, d_config, domid);
+rc = libxl__arch_domain_create(gc, d_config, state, domid);
 
 /* Construct a CPUID policy, but only for brand new domains.  Domains
  * being migrated-in/restored have CPUID handled during the
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 91a9fc7..91169d1 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -453,8 +453,10 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t 
domid,
 return ret;
 }
 
-int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
-uint32_t domid)
+int libxl__arch_domain_create(libxl__gc *gc,
+  libxl_domain_config *d_config,
+  libxl__domain_build_state *state,
+  uint32_t domid)
 {
 const libxl_domain_build_info *info = _config->b_info;
 int ret = 0;
-- 
2.7.4

Re: Linux 5.11-rc6 compile error

[Linus Torvalds]

On Wed, Feb 3, 2021 at 11:58 AM Shuah Khan  wrote:
>
> ld: arch/x86/built-in.a: member arch/x86/kernel/pci-swiotlb.o in archive
> is not an object
> make: *** [Makefile:1170: vmlinux] Error 1

That honestly sounds like something went wrong earlier - things like
doing a system upgrade in the middle of the build, or perhaps running
out of disk space or similar.

I've not seen any other reports of the same, and google doesn't find
anything like that either.

Does it keep happening if you do a "git clean -dqfx" to make sure you
have no old corrupt object files sound and re-do the whole build?

Linus

Re: [PATCH for-4.15] x86/efi: enable MS ABI attribute on clang

[Andrew Cooper]

On 03/02/2021 17:58, Roger Pau Monne wrote:
> Or else the EFI service calls will use the wrong calling convention.
>
> The __ms_abi__ attribute is available on all supported versions of
> clang.
>
> Signed-off-by: Roger Pau Monné 

Acked-by: Andrew Cooper 

> ---
> Cc: Ian Jackson 
>
> Without this a Xen built with clang won't be able to correctly use the
> EFI services, leading to weird messages from the firmware and crashes.
> The impact of this fix for GCC users is exactly 0, and will fix the
> build on clang.

DYM "fix the compiled binary"?

The build on clang isn't broken atm, but it provably has the wrong ABI.

~Andrew

[qemu-mainline test] 158969: regressions - FAIL

[osstest service owner]

flight 158969 qemu-mainline real [real]
flight 158986 qemu-mainline real-retest [real]
http://logs.test-lab.xenproject.org/osstest/logs/158969/
http://logs.test-lab.xenproject.org/osstest/logs/158986/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeat fail REGR. vs. 152631
 test-amd64-amd64-xl-qcow2   21 guest-start/debian.repeat fail REGR. vs. 152631
 test-armhf-armhf-libvirt-raw 13 guest-start  fail REGR. vs. 152631
 test-armhf-armhf-xl-vhd  13 guest-start  fail REGR. vs. 152631

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

version targeted for testing:
 qemuu77f3804ab7ed94b471a14acb260e5aeacf26193f
baseline version:
 qemuu1d806cef0e38b5db8347a8e12f214d543204a314

Last test of basis   152631  2020-08-20 09:07:46 Z  167 days
Failing since152659  2020-08-21 14:07:39 Z  166 days  336 attempts
Testing same since   158969  2021-02-03 00:38:03 Z0 days1 attempts


372 people touched revisions under test,
not listing them all

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

Re: [PATCH v3] NetBSD: use system-provided headers

[Roger Pau Monné]

On Wed, Feb 03, 2021 at 05:26:44PM +, Ian Jackson wrote:
> Manuel Bouyer writes ("[PATCH v3] NetBSD: use system-provided headers"):
> > +#ifdef __NetBSD__
> > +#include 
> > +#else
> >  #include 
> > +#endif
> >  #include 
> >  #include 
> Maneul, thanks.  I think this is a bugfix and ought in principle to go
> in but I think we probably want to do this with configure rather than
> ad-hoc ifdefs.
> 
> Roger, what do you think ?  Were you going to add a configure test for
> the #ifdef that we put in earlier ?

Yes, sorry, I owe you that patch. Will try to do tomorrow so that we
can have a model for other headers. AFAICT I will have to build
something around AC_CHECK_HEADER so that we can get a define that
contains a path that can be used with an #include directive.

Thanks, Roger.

[linux-linus test] 158971: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-xsm7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail REGR. 
vs. 152332
 test-amd64-i386-xl-qemuu-ws16-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail REGR. vs. 
152332
 test-amd64-i386-qemut-rhel6hvm-intel  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-intel  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-examine   6 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt   7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332
 test-amd64-coresched-i386-xl  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-pair 10 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-ws16-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-libvirt-xsm   7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 
152332
 test-amd64-i386-freebsd10-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-raw7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-pvshim 7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332
 test-amd64-i386-xl-shadow 7 xen-install  fail REGR. vs. 152332
 test-amd64-i386-freebsd10-i386  7 xen-installfail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-win7-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-win7-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-install   fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. 
vs. 152332
 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 152332
 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail REGR. vs. 152332
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-installfail REGR. vs. 152332
 test-arm64-arm64-examine  8 reboot   fail REGR. vs. 152332
 test-arm64-arm64-libvirt-xsm  8 xen-boot fail REGR. vs. 152332
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-installfail REGR. vs. 152332
 test-arm64-arm64-xl-credit1   8 xen-boot fail REGR. vs. 152332
 test-amd64-amd64-amd64-pvgrub 20 guest-stop  fail REGR. vs. 152332
 test-arm64-arm64-xl-thunderx 14 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-xl-multivcpu 14 guest-start fail REGR. vs. 152332
 test-amd64-amd64-i386-pvgrub 20 guest-stop   fail REGR. vs. 152332
 test-armhf-armhf-xl-credit1  14 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-xl-arndale  14 guest-start  fail REGR. vs. 152332
 test-amd64-i386-xl-qemut-debianhvm-amd64  7 xen-install  fail REGR. vs. 152332
 test-armhf-armhf-libvirt 14 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-xl-cubietruck 14 guest-startfail REGR. vs. 152332
 test-armhf-armhf-xl-credit2  14 guest-start  fail REGR. vs. 152332
 test-arm64-arm64-xl   8 xen-boot fail REGR. vs. 152332
 test-arm64-arm64-xl-xsm   8 xen-boot fail REGR. vs. 152332
 test-armhf-armhf-xl-vhd  13 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-xl  14 guest-start  fail REGR. vs. 152332
 test-armhf-armhf-libvirt-raw 13 guest-start  fail REGR. vs. 152332

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds 14 guest-start  fail REGR. vs. 152332

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-xl-seattle  11 leak-check/basis(11)fail blocked in 152332
 test-arm64-arm64-xl-credit2  11 leak-check/basis(11)fail blocked in 152332
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 152332
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152332
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 152332
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152332
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 152332
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 

[PATCH for-4.15] x86/efi: enable MS ABI attribute on clang

[Roger Pau Monne]

Or else the EFI service calls will use the wrong calling convention.

The __ms_abi__ attribute is available on all supported versions of
clang.

Signed-off-by: Roger Pau Monné 
---
Cc: Ian Jackson 

Without this a Xen built with clang won't be able to correctly use the
EFI services, leading to weird messages from the firmware and crashes.
The impact of this fix for GCC users is exactly 0, and will fix the
build on clang.

The biggest fallout from this could be using the attribute on a
compiler that doesn't support it, which would translate into a build
failure, but the gitlab tests have shown no issues.
---
 xen/include/asm-x86/x86_64/efibind.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/x86_64/efibind.h 
b/xen/include/asm-x86/x86_64/efibind.h
index b013db175d..ddcfae07ec 100644
--- a/xen/include/asm-x86/x86_64/efibind.h
+++ b/xen/include/asm-x86/x86_64/efibind.h
@@ -172,7 +172,7 @@ typedef uint64_t   UINTN;
 #ifndef EFIAPI  // Forces EFI calling conventions reguardless 
of compiler options
 #ifdef _MSC_EXTENSIONS
 #define EFIAPI __cdecl  // Force C calling convention for Microsoft C 
compiler
-#elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
+#elif __clang__ || __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 4)
 #define EFIAPI __attribute__((__ms_abi__))  // Force Microsoft ABI
 #else
 #define EFIAPI  // Substitute expresion to force C calling 
convention
-- 
2.29.2

Re: [PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource

[Andrew Cooper]

On 03/02/2021 17:51, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify 
> errno handling for map_resource"):
>> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI
>> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel
>> support.
>>
>> The Linux logic was contorted in what appears to be a deliberate attempt to
>> skip the now-deleted logic for the EOPNOTSUPP case.  Simplify it.
> Release-Acked-by: Ian Jackson 
>
> Sorry for my earlier confusion.  I had lost the context between the
> two patches.  I will explain my reasoning for the R-A:
>
> For the first two hunks (freebsd.c): these are consequential cleanup

FreeBSD and Linux.

> from patch 1/2 of this series.  Splitting this up made this easier to
> review and we don't want to leave the rather unfortunate constructs
> which arise from some hunks of 1/1.  IOW, the combination of 1/1 plus
> the first two hunks here is definitely release-worthy and the split
> has helped review.
>
> The final hunk is a straightforward bugfix.
>
> This combination of two completely different kinds of thing is a bit
> confusing but now that I have explained it to myself I'm satisfied.

Thanks,

~Andrew

Re: [PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno 
handling for map_resource"):
> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI
> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel
> support.
> 
> The Linux logic was contorted in what appears to be a deliberate attempt to
> skip the now-deleted logic for the EOPNOTSUPP case.  Simplify it.

Release-Acked-by: Ian Jackson 

Sorry for my earlier confusion.  I had lost the context between the
two patches.  I will explain my reasoning for the R-A:

For the first two hunks (freebsd.c): these are consequential cleanup
from patch 1/2 of this series.  Splitting this up made this easier to
review and we don't want to leave the rather unfortunate constructs
which arise from some hunks of 1/1.  IOW, the combination of 1/1 plus
the first two hunks here is definitely release-worthy and the split
has helped review.

The final hunk is a straightforward bugfix.

This combination of two completely different kinds of thing is a bit
confusing but now that I have explained it to myself I'm satisfied.

Ian.

Re: [PATCH] xenstored: close socket connections on error

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 05:38:59PM +, Ian Jackson wrote:
> > [...]
> > broken on Linux too ?
> 
> Andy pointed me to the recent thread "xenstored file descriptor leak"
> which answers all these questions.  I think it would have been nice if
> some tools maintainer(s) had been CC'd on that :-).

I did use add_maintainers.pl against it (or at last it was my intent)

> 
> Juergen, I guess I will get a formal R-b from you ?
> 
> Release-Acked-by: Ian Jackson 
> 
> 
> Manuel, in response to this:
> 
> > When I started, I looked at the wiki for instructions about
> > patches, but didn't find any ...
> 
> Earlier I offered you help with git, in private email.  I agree that
> git is confusing and sometimes impenetrable.  But it seems that what
> you are doing now is worse!  Please take me up on my offer of help.

I didn't forget. It's just that I don't even know what to ask to start
with.  It seems that StGit will help a lot though.

> 
> Our wiki doesn't give instructions on how to use git to maintain a
> patch series.  Those instructions would not be Xen-specific.  Perhaps
> we could have a pointer or two, but everyone has their own pet methods
> and tooling so the result would perhaps be more confusing than
> helpful.

a howto is alwaus helpfull. Even if it's not the one and only way
to do, at last it gives a start point.

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses

[Julien Grall]

On 03/02/2021 00:18, Stefano Stabellini wrote:

On Tue, 2 Feb 2021, Julien Grall wrote:

On 02/02/2021 18:12, Julien Grall wrote:

On 02/02/2021 17:47, Elliott Mitchell wrote:

The handle_device() function has been returning failure upon
encountering a device address which was invalid.  A device tree which
had such an entry has now been seen in the wild.  As it causes no
failures to simply ignore the entries, ignore them. >
Signed-off-by: Elliott Mitchell 

---
I'm starting to suspect there are an awful lot of places in the various
domain_build.c files which should simply ignore errors.  This is now the
second place I've encountered in 2 months where ignoring errors was the
correct action.


Right, as a counterpoint, we run Xen on Arm HW for several years now and
this is the first time I heard about issue parsing the DT. So while I
appreciate that you are eager to run Xen on the RPI...


  I know failing in case of error is an engineer's
favorite approach, but there seem an awful lot of harmless failures
causing panics.

This started as the thread "[RFC PATCH] xen/arm: domain_build: Ignore
empty memory bank".  Now it seems clear the correct approach is to simply
ignore these entries.


... we first need to fully understand the issues. Here a few questions:
     1) Can you provide more information why you believe the address is
invalid?
     2) How does Linux use the node?
     3) Is it happening with all the RPI DT? If not, what are the
differences?


So I had another look at the device-tree you provided earlier on. The node is
the following (copied directly from the DTS):

 {
 pci@1,0 {
 #address-cells = <3>;
 #size-cells = <2>;
 ranges;

 reg = <0 0 0 0 0>;

 usb@1,0 {
 reg = <0x1 0 0 0 0>;
 resets = < RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
 };
 };
};

pcie0: pcie@7d50 {
compatible = "brcm,bcm2711-pcie";
reg = <0x0 0x7d50  0x0 0x9310>;
device_type = "pci";
#address-cells = <3>;
#interrupt-cells = <1>;
#size-cells = <2>;
interrupts = ,
 ;
interrupt-names = "pcie", "msi";
interrupt-map-mask = <0x0 0x0 0x0 0x7>;
interrupt-map = <0 0 0 1  GIC_SPI 143
  IRQ_TYPE_LEVEL_HIGH>;
msi-controller;
msi-parent = <>;

ranges = <0x0200 0x0 0xc000 0x6 0x
  0x0 0x4000>;
/*
 * The wrapper around the PCIe block has a bug
 * preventing it from accessing beyond the first 3GB of
 * memory.
 */
dma-ranges = <0x0200 0x0 0x 0x0 0x
  0x0 0xc000>;
brcm,enable-ssc;
};

The interpretation of "reg" depends on the context. In this case, we are
trying to interpret as a memory address from the CPU PoV when it has a
different meaning (I am not exactly sure what).

In fact, you are lucky that Xen doesn't manage to interpret it. Xen should
really stop trying to look region to map when it discover a PCI bus. I wrote a
quick hack patch that should ignore it:


Yes, I think you are right. There are a few instances where "reg" is not
a address ready to be remapped. It is not just PCI, although that's the
most common.  Maybe we need a list, like skip_matches in handle_node.


From my understanding, "reg" can be considered as an MMIO region only 
if all the *parents up to the root have the property "ranges" and they 
are not on a different bus (e.g. pci).


Do you have example where this is not the case?

Whether Xen does it correctly is another question :).





diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 374bf655ee34..937fd1e387b7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1426,7 +1426,7 @@ static int __init handle_device(struct domain *d, struct
dt_device_node *dev,

  static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
struct dt_device_node *node,
-  p2m_type_t p2mt)
+  p2m_type_t p2mt, bool pci_bus)
  {
  static const struct dt_device_match skip_matches[] __initconst =
  {
@@ -1532,9 +1532,14 @@ static int __init handle_node(struct domain *d, struct
kernel_info *kinfo,
 "WARNING: Path %s is reserved, skip the node as we may re-use
the path.\n",
 path);

-res = handle_device(d, node, p2mt);
-if ( res)
-return res;
+if ( !pci_bus )
+{
+res = handle_device(d, node, p2mt);
+if ( res)
+   return res;
+
+pci_bus = dt_device_type_is_equal(node, "pci");
+}

  /*
   * The property "name" is used to have a different name on older FDT
@@ -1554,7 +1559,7 @@ static int __init handle_node(struct domain *d, struct
kernel_info *kinfo,

  for ( child = node->child; child != NULL; child 

Re: [PATCH for-4.15 0/3] tools/oxenstored bugfixes

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.15 0/3] tools/oxenstored bugfixes"):
> All of these been posted before, but were tangled in other content which is
> not appropriate for 4.15 any more.  As a consequence, I didn't get around to
> committing them before the code freeze.
> 
> They were all found with unit testing, specifically fuzzing the
> serialising/deserialising logic introduced for restartiblity, asserting that
> the tree before and after was identical.
> 
> The unit testing/fuzzing content isn't suitable for 4.15, but these bugfixes
> want backporting to all releases, and should therefore be considered for 4.15
> at this point.

I just gave my

Release-Acked-by: Ian Jackson 

in that other mail.  FTAOD that applies to all three.

Christian, would you be able to do a maintainer review ?

Thanks,
Ian.

Re: [PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST

[Ian Jackson]

Andrew Cooper writes ("[PATCH 1/3] tools/oxenstored: Fix quota calculation for 
mkdir EEXIST"):
> From: Edwin Török 
> 
> We increment the domain's quota on mkdir even when the node already exists.
> This results in a quota inconsistency after live update, where reconstructing
> the tree from scratch results in a different quota.
> 
> Not a security issue because the domain uses up quota faster, so it will only
> get a Quota error sooner than it should.
> 
> Found by the structured fuzzer.

Thanks for these.  They look like straightforward bugfixes, so they
don't need a release ack, but FTR

Release-Acked-by: Ian Jackson 

I don't feel qualified to give a maintainer-ack...

Ian.

Re: [PATCH] xenstored: close socket connections on error

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] xenstored: close socket connections on error"):
> Manuel Bouyer writes ("[PATCH] xenstored: close socket connections on error"):
> > On error, don't keep socket connection in ignored state but close them.
> > When the remote end of a socket is closed, xenstored will flag it as an
> > error and switch the connection to ignored. But on some OSes (e.g.
> > NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
> > state will stay open forever in xenstored (and it will loop with CPU 100%
> > busy).
> 
> Juergen, I think you probably know this code the best.  Would you be
> able to review this ?
> 
> I'm not sure I understand what the specific behaviour on NetBSD is
> that is upsetting xenstored.  Or rather, what it is that xenstored is
> using to tell when the socket should be closed.
> 
> I grepped for POLLERR and nothing came up.
> 
> Or to put it another way, is this commit
> 
> > Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
> 
> broken on Linux too ?

Andy pointed me to the recent thread "xenstored file descriptor leak"
which answers all these questions.  I think it would have been nice if
some tools maintainer(s) had been CC'd on that :-).

Juergen, I guess I will get a formal R-b from you ?

Release-Acked-by: Ian Jackson 


Manuel, in response to this:

> When I started, I looked at the wiki for instructions about
> patches, but didn't find any ...

Earlier I offered you help with git, in private email.  I agree that
git is confusing and sometimes impenetrable.  But it seems that what
you are doing now is worse!  Please take me up on my offer of help.

Our wiki doesn't give instructions on how to use git to maintain a
patch series.  Those instructions would not be Xen-specific.  Perhaps
we could have a pointer or two, but everyone has their own pet methods
and tooling so the result would perhaps be more confusing than
helpful.

Regards,
Ian.

[PATCH for-4.15 0/3] tools/oxenstored bugfixes

[Andrew Cooper]

All of these been posted before, but were tangled in other content which is
not appropriate for 4.15 any more.  As a consequence, I didn't get around to
committing them before the code freeze.

They were all found with unit testing, specifically fuzzing the
serialising/deserialising logic introduced for restartiblity, asserting that
the tree before and after was identical.

The unit testing/fuzzing content isn't suitable for 4.15, but these bugfixes
want backporting to all releases, and should therefore be considered for 4.15
at this point.

Edwin Török (3):
  tools/oxenstored: Fix quota calculation for mkdir EEXIST
  tools/oxenstored: Reject invalid watch paths early
  tools/oxenstored: mkdir conflicts were sometimes missed

 tools/ocaml/xenstored/connection.ml  | 5 ++---
 tools/ocaml/xenstored/connections.ml | 4 +++-
 tools/ocaml/xenstored/store.ml   | 1 +
 tools/ocaml/xenstored/transaction.ml | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH 2/3] tools/oxenstored: Reject invalid watch paths early

[Andrew Cooper]

From: Edwin Török 

Watches on invalid paths were accepted, but they would never trigger.  The
client also got no notification that its watch is bad and would never trigger.

Found again by the structured fuzzer, due to an error on live update reload:
the invalid watch paths would get rejected during live update and the list of
watches would be different pre/post live update.

The testcase is watch on `//`, which is an invalid path.

Signed-off-by: Edwin Török 
---
CC: Christian Lindig 
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/ocaml/xenstored/connection.ml  | 5 ++---
 tools/ocaml/xenstored/connections.ml | 4 +++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/ocaml/xenstored/connection.ml 
b/tools/ocaml/xenstored/connection.ml
index d09a0fa405..65f99ea6f2 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -158,18 +158,17 @@ let get_children_watches con path =
 let is_dom0 con =
Perms.Connection.is_dom0 (get_perm con)
 
-let add_watch con path token =
+let add_watch con (path, apath) token =
if !Quota.activate && !Define.maxwatch > 0 &&
   not (is_dom0 con) && con.nb_watches > !Define.maxwatch then
raise Quota.Limit_reached;
-   let apath = get_watch_path con path in
let l = get_watches con apath in
if List.exists (fun w -> w.token = token) l then
raise Define.Already_exist;
let watch = watch_create ~con ~token ~path in
Hashtbl.replace con.watches apath (watch :: l);
con.nb_watches <- con.nb_watches + 1;
-   apath, watch
+   watch
 
 let del_watch con path token =
let apath = get_watch_path con path in
diff --git a/tools/ocaml/xenstored/connections.ml 
b/tools/ocaml/xenstored/connections.ml
index 8a66eeec3a..3c7429fe7f 100644
--- a/tools/ocaml/xenstored/connections.ml
+++ b/tools/ocaml/xenstored/connections.ml
@@ -114,8 +114,10 @@ let key_of_path path =
"" :: Store.Path.to_string_list path
 
 let add_watch cons con path token =
-   let apath, watch = Connection.add_watch con path token in
+   let apath = Connection.get_watch_path con path in
+   (* fail on invalid paths early by calling key_of_str before adding 
watch *)
let key = key_of_str apath in
+   let watch = Connection.add_watch con (path, apath) token in
let watches =
if Trie.mem cons.watches key
then Trie.find cons.watches key
-- 
2.11.0

[PATCH 1/3] tools/oxenstored: Fix quota calculation for mkdir EEXIST

[Andrew Cooper]

From: Edwin Török 

We increment the domain's quota on mkdir even when the node already exists.
This results in a quota inconsistency after live update, where reconstructing
the tree from scratch results in a different quota.

Not a security issue because the domain uses up quota faster, so it will only
get a Quota error sooner than it should.

Found by the structured fuzzer.

Signed-off-by: Edwin Török 
---
CC: Christian Lindig 
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/ocaml/xenstored/store.ml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml
index 1bd0c81f6f..20e67b1427 100644
--- a/tools/ocaml/xenstored/store.ml
+++ b/tools/ocaml/xenstored/store.ml
@@ -419,6 +419,7 @@ let mkdir store perm path =
(* It's upt to the mkdir logic to decide what to do with existing path 
*)
if not (existing || (Perms.Connection.is_dom0 perm)) then Quota.check 
store.quota owner 0;
store.root <- path_mkdir store perm path;
+   if not existing then
Quota.add_entry store.quota owner
 
 let rm store perm path =
-- 
2.11.0

[PATCH 3/3] tools/oxenstored: mkdir conflicts were sometimes missed

[Andrew Cooper]

From: Edwin Török 

Due to how set_write_lowpath was used here it didn't detect create/delete
conflicts.  When we create an entry we must mark our parent as modified
(this is what creating a new node via write does).

Otherwise we can have 2 transactions one creating, and another deleting a node
both succeeding depending on timing.  Or one transaction reading an entry,
concluding it doesn't exist, do some other work based on that information and
successfully commit even if another transaction creates the node via mkdir
meanwhile.

Signed-off-by: Edwin Török 
---
CC: Christian Lindig 
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/ocaml/xenstored/transaction.ml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/ocaml/xenstored/transaction.ml 
b/tools/ocaml/xenstored/transaction.ml
index 25bc8c3b4a..17b1bdf2ea 100644
--- a/tools/ocaml/xenstored/transaction.ml
+++ b/tools/ocaml/xenstored/transaction.ml
@@ -165,7 +165,7 @@ let write t perm path value =
 
 let mkdir ?(with_watch=true) t perm path =
Store.mkdir t.store perm path;
-   set_write_lowpath t path;
+   set_write_lowpath t (Store.Path.get_parent path);
if with_watch then
add_wop t Xenbus.Xb.Op.Mkdir path
 
-- 
2.11.0

Re: [PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource

[Andrew Cooper]

On 03/02/2021 17:18, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify 
> errno handling for map_resource"):
>> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI
>> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel
>> support.
>>
>> The Linux logic was contorted in what appears to be a deliberate attempt to
>> skip the now-deleted logic for the EOPNOTSUPP case.  Simplify it.
> AFAICT this is a mixture of cleanup/refactoring, and bugfix.  Is that
> correct ?
>
>> diff --git a/tools/libs/foreignmemory/netbsd.c 
>> b/tools/libs/foreignmemory/netbsd.c
>> index 565682e064..c0b1b8f79d 100644
>> --- a/tools/libs/foreignmemory/netbsd.c
>> +++ b/tools/libs/foreignmemory/netbsd.c
>> @@ -147,6 +147,9 @@ int osdep_xenforeignmemory_map_resource(
>>  rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, );
>>  if ( rc )
>>  {
>> +if ( errno == ENOSYS )
>> +errno = EOPNOTSUPP;
>> +
>>  if ( fres->addr )
>>  {
>>  int saved_errno = errno;
> Specifically, I guess this is the bugfix ?

It is a bugfix on NetBSD (for brand new functionality), and cleanup on
FreeBSD/Linux specifically split out of the previous patch.

~Andrew

Re: [PATCH v3] NetBSD: use system-provided headers

[Ian Jackson]

Manuel Bouyer writes ("[PATCH v3] NetBSD: use system-provided headers"):
> +#ifdef __NetBSD__
> +#include 
> +#else
>  #include 
> +#endif
>  #include 
>  #include 
Maneul, thanks.  I think this is a bugfix and ought in principle to go
in but I think we probably want to do this with configure rather than
ad-hoc ifdefs.

Roger, what do you think ?  Were you going to add a configure test for
the #ifdef that we put in earlier ?

Thanks,
Ian.

Re: [PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging

[Andrew Cooper]

On 03/02/2021 17:16, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.15 1/2] libs/foreignmem: Drop useless 
> and/or misleading logging"):
>> These log lines are all in response to single system calls, and do not 
>> provide
>> any information which the immediate caller can't determine themselves.  It is
>> however exceedinly rude to put junk like this onto stderr, especially as
>> system call failures are not even error conditions in certain circumstances.
>>
>> The FreeBSD logging has stale function names in, and solaris shouldn't have
>> passed code review to start with.
>>
>> No functional change.
> Thanks.
>
> Reviewed-by: Ian Jackson 
> Release-Acked-by: Ian Jackson 

Thanks,

>
>>  int saved_errno = errno;
>> -PERROR("");
>> +
> That's particularly wtf...

My thoughts exactly.

~Andrew

Re: [PATCH] xenstored: close socket connections on error

[Ian Jackson]

Manuel Bouyer writes ("[PATCH] xenstored: close socket connections on error"):
> On error, don't keep socket connection in ignored state but close them.
> When the remote end of a socket is closed, xenstored will flag it as an
> error and switch the connection to ignored. But on some OSes (e.g.
> NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
> state will stay open forever in xenstored (and it will loop with CPU 100%
> busy).

Juergen, I think you probably know this code the best.  Would you be
able to review this ?

I'm not sure I understand what the specific behaviour on NetBSD is
that is upsetting xenstored.  Or rather, what it is that xenstored is
using to tell when the socket should be closed.

I grepped for POLLERR and nothing came up.

Or to put it another way, is this commit

> Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083

broken on Linux too ?

Ian.

Re: [PATCH] add a qemu-ifup script on NetBSD

[Ian Jackson]

Manuel Bouyer writes ("[PATCH] add a qemu-ifup script on NetBSD"):
> On NetBSD, qemu-xen will use a qemu-ifup script to setup the tap interfaces
> (as qemu-xen-traditional used to). Copy the script from qemu-xen-traditional,
> and install it on NetBSD. While there document parameters and environnement
> variables.

Release-Acked-by: Ian Jackson 

> +++ b/tools/hotplug/NetBSD/qemu-ifup
> @@ -0,0 +1,9 @@
> +#!/bin/sh
> +
> +#called by qemu when a HVM domU is started.
> +# first parameter is tap interface, second is the bridge name
> +# environement variable $XEN_DOMAIN_ID contains the domU's ID,
> +# which can be used to retrieve extra parameters from the xenstore.
> +
> +ifconfig $1 up
> +exec /sbin/brconfig $2 add $1

Acked-by: Ian Jackson 

Ian.

Re: [PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno 
handling for map_resource"):
> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI
> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel
> support.
> 
> The Linux logic was contorted in what appears to be a deliberate attempt to
> skip the now-deleted logic for the EOPNOTSUPP case.  Simplify it.

AFAICT this is a mixture of cleanup/refactoring, and bugfix.  Is that
correct ?

> diff --git a/tools/libs/foreignmemory/netbsd.c 
> b/tools/libs/foreignmemory/netbsd.c
> index 565682e064..c0b1b8f79d 100644
> --- a/tools/libs/foreignmemory/netbsd.c
> +++ b/tools/libs/foreignmemory/netbsd.c
> @@ -147,6 +147,9 @@ int osdep_xenforeignmemory_map_resource(
>  rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, );
>  if ( rc )
>  {
> +if ( errno == ENOSYS )
> +errno = EOPNOTSUPP;
> +
>  if ( fres->addr )
>  {
>  int saved_errno = errno;

Specifically, I guess this is the bugfix ?

Ian.

Re: [PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.15 1/2] libs/foreignmem: Drop useless 
and/or misleading logging"):
> These log lines are all in response to single system calls, and do not provide
> any information which the immediate caller can't determine themselves.  It is
> however exceedinly rude to put junk like this onto stderr, especially as
> system call failures are not even error conditions in certain circumstances.
> 
> The FreeBSD logging has stale function names in, and solaris shouldn't have
> passed code review to start with.
> 
> No functional change.

Thanks.

Reviewed-by: Ian Jackson 
Release-Acked-by: Ian Jackson 

>  int saved_errno = errno;
> -PERROR("");
> +

That's particularly wtf...

Ian.

Re: [PATCH v3 0/3] Generic SMMU Bindings

[Rahul Singh]

Hello Stefano,

> On 2 Feb 2021, at 5:44 pm, Stefano Stabellini  wrote:
> 
> On Tue, 2 Feb 2021, Rahul Singh wrote:
>> Hello Stefano,
>> 
>>> On 26 Jan 2021, at 10:58 pm, Stefano Stabellini  
>>> wrote:
>>> 
>>> Hi all,
>>> 
>>> This series introduces support for the generic SMMU bindings to
>>> xen/drivers/passthrough/arm/smmu.c.
>>> 
>>> The last version of the series was
>>> https://marc.info/?l=xen-devel=159539053406643
>>> 
>>> I realize that it is late for 4.15 -- I think it is OK if this series
>>> goes in afterwards.
>> 
>> I tested the series on the Juno board it is woking fine.  
>> I found one issue in SMMU driver while testing this series that is not 
>> related to this series but already existing issue in SMMU driver.
>> 
>> If there are more than one device behind SMMU and they share the same 
>> Stream-Id, SMMU driver is creating the new SMR entry without checking the 
>> already configured SMR entry if SMR entry correspond to stream-id is already 
>> configured.  Because of this I observed the stream match conflicts on Juno 
>> board.
>> 
>> (XEN) smmu: /iommu@7fb3: Unexpected global fault, this could be serious
>> (XEN) smmu: /iommu@7fb3: GFSR 0x0004, GFSYNR0 0x0006, 
>> GFSYNR1 0x, GFSYNR2 0x
>> 
>> 
>> Below two patches is required to be ported to Xen to fix the issue from 
>> Linux driver.
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y=1f3d5ca43019bff1105838712d55be087d93c0da
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y=21174240e4f4439bb8ed6c116cdbdc03eba2126e
> 
> 
> Good catch and thanks for the pointers! Do you have any interest in
> backporting these two patches or should I put them on my TODO list?

Yes I am happy to backport these patches to XEN. 
I will send the patch for review once 4.15 release branch out from master.
 
Regards,
Rahul
> 
> Unrelated to who does the job, we should discuss if it makes sense to
> try to fix the bug for 4.15. The patches don't seem trivial so I am
> tempted to say that it might be best to leave the bug unfixed for 4.15
> and fix it later.

[PATCH v3] NetBSD: use system-provided headers

[Manuel Bouyer]

On NetBSD use the system-provided headers for ioctl and related definitions,
they are up to date and have more chances to match the kernel's idea of
the ioctls and structures.
Remove now-unused NetBSD/evtchn.h and NetBSD/privcmd.h.
Don't fail install if xen/sys/*.h are not present.

Signed-off-by: Manuel Bouyer 
---
 tools/debugger/gdbsx/xg/xg_main.c  |   4 +
 tools/include/Makefile |   2 +
 tools/include/xen-sys/NetBSD/evtchn.h  |  86 
 tools/include/xen-sys/NetBSD/privcmd.h | 106 -
 tools/libs/call/private.h  |   4 +
 tools/libs/ctrl/xc_private.h   |   4 +
 tools/libs/foreignmemory/private.h |   6 ++
 7 files changed, 20 insertions(+), 192 deletions(-)
 delete mode 100644 tools/include/xen-sys/NetBSD/evtchn.h
 delete mode 100644 tools/include/xen-sys/NetBSD/privcmd.h

diff --git a/tools/debugger/gdbsx/xg/xg_main.c 
b/tools/debugger/gdbsx/xg/xg_main.c
index 4576c762af..903d60baed 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -49,7 +49,11 @@
 #include "xg_public.h"
 #include 
 #include 
+#ifdef __NetBSD__
+#include 
+#else
 #include 
+#endif
 #include 
 #include 
 
diff --git a/tools/include/Makefile b/tools/include/Makefile
index 4d4ec5f974..65fb67a771 100644
--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -68,7 +68,9 @@ install: all
$(INSTALL_DATA) xen/foreign/*.h $(DESTDIR)$(includedir)/xen/foreign
$(INSTALL_DATA) xen/hvm/*.h $(DESTDIR)$(includedir)/xen/hvm
$(INSTALL_DATA) xen/io/*.h $(DESTDIR)$(includedir)/xen/io
+ifneq ($(wildcard xen/sys/*.h),)
$(INSTALL_DATA) xen/sys/*.h $(DESTDIR)$(includedir)/xen/sys
+endif
$(INSTALL_DATA) xen/xsm/*.h $(DESTDIR)$(includedir)/xen/xsm
 
 .PHONY: uninstall
diff --git a/tools/include/xen-sys/NetBSD/evtchn.h 
b/tools/include/xen-sys/NetBSD/evtchn.h
deleted file mode 100644
index 2d8a1f9164..00
--- a/tools/include/xen-sys/NetBSD/evtchn.h
+++ /dev/null
@@ -1,86 +0,0 @@
-/* $NetBSD: evtchn.h,v 1.1.1.1 2007/06/14 19:39:45 bouyer Exp $ */
-/**
- * evtchn.h
- * 
- * Interface to /dev/xen/evtchn.
- * 
- * Copyright (c) 2003-2005, K A Fraser
- * 
- * This file may be distributed separately from the Linux kernel, or
- * incorporated into other software packages, subject to the following license:
- * 
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this source file (the "Software"), to deal in the Software without
- * restriction, including without limitation the rights to use, copy, modify,
- * merge, publish, distribute, sublicense, and/or sell copies of the Software,
- * and to permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- * 
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- * 
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- */
-
-#ifndef __NetBSD_EVTCHN_H__
-#define __NetBSD_EVTCHN_H__
-
-/*
- * Bind a fresh port to VIRQ @virq.
- */
-#define IOCTL_EVTCHN_BIND_VIRQ \
-   _IOWR('E', 4, struct ioctl_evtchn_bind_virq)
-struct ioctl_evtchn_bind_virq {
-   unsigned int virq;
-   unsigned int port;
-};
-
-/*
- * Bind a fresh port to remote <@remote_domain, @remote_port>.
- */
-#define IOCTL_EVTCHN_BIND_INTERDOMAIN  \
-   _IOWR('E', 5, struct ioctl_evtchn_bind_interdomain)
-struct ioctl_evtchn_bind_interdomain {
-   unsigned int remote_domain, remote_port;
-   unsigned int port;
-};
-
-/*
- * Allocate a fresh port for binding to @remote_domain.
- */
-#define IOCTL_EVTCHN_BIND_UNBOUND_PORT \
-   _IOWR('E', 6, struct ioctl_evtchn_bind_unbound_port)
-struct ioctl_evtchn_bind_unbound_port {
-   unsigned int remote_domain;
-   unsigned int port;
-};
-
-/*
- * Unbind previously allocated @port.
- */
-#define IOCTL_EVTCHN_UNBIND\
-   _IOW('E', 7, struct ioctl_evtchn_unbind)
-struct ioctl_evtchn_unbind {
-   unsigned int port;
-};
-
-/*
- * Send event to previously allocated @port.
- */
-#define IOCTL_EVTCHN_NOTIFY\
-   _IOW('E', 8, struct ioctl_evtchn_notify)
-struct ioctl_evtchn_notify {
-   unsigned int port;
-};
-
-/* Clear and reinitialise the event buffer. Clear error condition. */
-#define IOCTL_EVTCHN_RESET \
-  

[PATCH v3] Document qemu-ifup on NetBSD

[Manuel Bouyer]

Document that on NetBSD, the tap interface will be configured by the
qemu-ifup script.

Signed-off-by: Manuel Bouyer 
Release-Acked-by: Ian Jackson 
---
 docs/man/xl-network-configuration.5.pod | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/docs/man/xl-network-configuration.5.pod 
b/docs/man/xl-network-configuration.5.pod
index af058d4d3c..8e5fd909fa 100644
--- a/docs/man/xl-network-configuration.5.pod
+++ b/docs/man/xl-network-configuration.5.pod
@@ -172,6 +172,9 @@ add it to the relevant bridge). Defaults to
 C but can be set to any script. Some example
 scripts are installed in C.
 
+Note on NetBSD HVM guests will ignore the script option for tap
+(emulated) interfaces and always use
+C to configure the interface in bridged mode.
 
 =head2 ip
 
-- 
2.29.2

[PATCH] add a qemu-ifup script on NetBSD

[Manuel Bouyer]

On NetBSD, qemu-xen will use a qemu-ifup script to setup the tap interfaces
(as qemu-xen-traditional used to). Copy the script from qemu-xen-traditional,
and install it on NetBSD. While there document parameters and environnement
variables.

Signed-off-by: Manuel Bouyer 
---
 tools/hotplug/NetBSD/Makefile  | 1 +
 tools/hotplug/NetBSD/qemu-ifup | 9 +
 2 files changed, 10 insertions(+)
 create mode 100644 tools/hotplug/NetBSD/qemu-ifup

diff --git a/tools/hotplug/NetBSD/Makefile b/tools/hotplug/NetBSD/Makefile
index 114b223207..f909ffa367 100644
--- a/tools/hotplug/NetBSD/Makefile
+++ b/tools/hotplug/NetBSD/Makefile
@@ -7,6 +7,7 @@ XEN_SCRIPTS += locking.sh
 XEN_SCRIPTS += block
 XEN_SCRIPTS += vif-bridge
 XEN_SCRIPTS += vif-ip
+XEN_SCRIPTS += qemu-ifup
 
 XEN_SCRIPT_DATA =
 XEN_RCD_PROG = rc.d/xencommons rc.d/xendomains rc.d/xen-watchdog 
rc.d/xendriverdomain
diff --git a/tools/hotplug/NetBSD/qemu-ifup b/tools/hotplug/NetBSD/qemu-ifup
new file mode 100644
index 00..4305419f44
--- /dev/null
+++ b/tools/hotplug/NetBSD/qemu-ifup
@@ -0,0 +1,9 @@
+#!/bin/sh
+
+#called by qemu when a HVM domU is started.
+# first parameter is tap interface, second is the bridge name
+# environement variable $XEN_DOMAIN_ID contains the domU's ID,
+# which can be used to retrieve extra parameters from the xenstore.
+
+ifconfig $1 up
+exec /sbin/brconfig $2 add $1
-- 
2.29.2

[PATCH] xenstored: close socket connections on error

[Manuel Bouyer]

On error, don't keep socket connection in ignored state but close them.
When the remote end of a socket is closed, xenstored will flag it as an
error and switch the connection to ignored. But on some OSes (e.g.
NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
state will stay open forever in xenstored (and it will loop with CPU 100%
busy).

Signed-off-by: Manuel Bouyer 
Fixes: d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
---
 tools/xenstore/xenstored_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 1ab6f162cb..0fea598352 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1440,6 +1440,9 @@ static void ignore_connection(struct connection *conn)
 
talloc_free(conn->in);
conn->in = NULL;
+   /* if this is a socket connection, drop it now */
+   if (conn->fd >= 0)
+   talloc_free(conn);
 }
 
 static const char *sockmsg_string(enum xsd_sockmsg_type type)
-- 
2.29.2

[PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging

[Andrew Cooper]

These log lines are all in response to single system calls, and do not provide
any information which the immediate caller can't determine themselves.  It is
however exceedinly rude to put junk like this onto stderr, especially as
system call failures are not even error conditions in certain circumstances.

The FreeBSD logging has stale function names in, and solaris shouldn't have
passed code review to start with.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Manuel Bouyer 

The errno constructs in osdep_xenforeignmemory_map_resource() are addressed in
the following patch, to avoid adding complexity to this one.

This reduces the quantity of noise from unit tests, where certain syscall
failures are definitely not errors.
---
 tools/libs/foreignmemory/freebsd.c | 7 ++-
 tools/libs/foreignmemory/linux.c   | 6 +-
 tools/libs/foreignmemory/netbsd.c  | 2 --
 tools/libs/foreignmemory/solaris.c | 2 +-
 4 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index 9a2796f0b7..04bfa806b0 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -65,10 +65,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 
 addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
 if ( addr == MAP_FAILED )
-{
-PERROR("xc_map_foreign_bulk: mmap failed");
 return NULL;
-}
 
 ioctlx.num = num;
 ioctlx.dom = dom;
@@ -80,7 +77,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 if ( rc < 0 )
 {
 int saved_errno = errno;
-PERROR("xc_map_foreign_bulk: ioctl failed");
+
 (void)munmap(addr, num << PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
@@ -137,7 +134,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 int saved_errno;
 
 if ( errno != ENOSYS )
-PERROR("mmap resource ioctl failed");
+;
 else
 errno = EOPNOTSUPP;
 
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index d0eead1196..050b9ed3a5 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -171,10 +171,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED,
 fd, 0);
 if ( addr == MAP_FAILED )
-{
-PERROR("mmap failed");
 return NULL;
-}
 
 ioctlx.num = num;
 ioctlx.dom = dom;
@@ -273,7 +270,6 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-PERROR("ioctl failed");
 (void)munmap(addr, num << PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
@@ -330,7 +326,7 @@ int osdep_xenforeignmemory_map_resource(
 int saved_errno;
 
 if ( errno != fmem->unimpl_errno && errno != EOPNOTSUPP )
-PERROR("ioctl failed");
+;
 else
 errno = EOPNOTSUPP;
 
diff --git a/tools/libs/foreignmemory/netbsd.c 
b/tools/libs/foreignmemory/netbsd.c
index 4ae60aafdd..565682e064 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -147,8 +147,6 @@ int osdep_xenforeignmemory_map_resource(
 rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, );
 if ( rc )
 {
-PERROR("ioctl failed");
-
 if ( fres->addr )
 {
 int saved_errno = errno;
diff --git a/tools/libs/foreignmemory/solaris.c 
b/tools/libs/foreignmemory/solaris.c
index ee8aae4fbd..958fb01f6d 100644
--- a/tools/libs/foreignmemory/solaris.c
+++ b/tools/libs/foreignmemory/solaris.c
@@ -83,7 +83,7 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, 
uint32_t dom,
 if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, ) < 0 )
 {
 int saved_errno = errno;
-PERROR("");
+
 (void)munmap(addr, num*XC_PAGE_SIZE);
 errno = saved_errno;
 return NULL;
-- 
2.11.0

[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource

[Andrew Cooper]

Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI
appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel
support.

The Linux logic was contorted in what appears to be a deliberate attempt to
skip the now-deleted logic for the EOPNOTSUPP case.  Simplify it.

Signed-off-by: Andrew Cooper 
---
CC: Ian Jackson 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Manuel Bouyer 
---
 tools/libs/foreignmemory/freebsd.c | 4 +---
 tools/libs/foreignmemory/linux.c   | 4 +---
 tools/libs/foreignmemory/netbsd.c  | 3 +++
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index 04bfa806b0..d94ea07862 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -133,9 +133,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 {
 int saved_errno;
 
-if ( errno != ENOSYS )
-;
-else
+if ( errno == ENOSYS )
 errno = EOPNOTSUPP;
 
 if ( fres->addr )
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 050b9ed3a5..c1f35e2db7 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -325,9 +325,7 @@ int osdep_xenforeignmemory_map_resource(
 {
 int saved_errno;
 
-if ( errno != fmem->unimpl_errno && errno != EOPNOTSUPP )
-;
-else
+if ( errno == fmem->unimpl_errno )
 errno = EOPNOTSUPP;
 
 if ( fres->addr )
diff --git a/tools/libs/foreignmemory/netbsd.c 
b/tools/libs/foreignmemory/netbsd.c
index 565682e064..c0b1b8f79d 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -147,6 +147,9 @@ int osdep_xenforeignmemory_map_resource(
 rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, );
 if ( rc )
 {
+if ( errno == ENOSYS )
+errno = EOPNOTSUPP;
+
 if ( fres->addr )
 {
 int saved_errno = errno;
-- 
2.11.0

Re: [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter

[Andrew Cooper]

On 02/02/2021 09:04, Jan Beulich wrote:
> On 02.02.2021 00:26, Andrew Cooper wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -132,6 +132,56 @@ static void vcpu_info_reset(struct vcpu *v)
>>  v->vcpu_info_mfn = INVALID_MFN;
>>  }
>>  
>> +static void vmtrace_free_buffer(struct vcpu *v)
>> +{
>> +const struct domain *d = v->domain;
>> +struct page_info *pg = v->vmtrace.pg;
>> +unsigned int i;
>> +
>> +if ( !pg )
>> +return;
>> +
>> +v->vmtrace.pg = NULL;
>> +
>> +for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>> +{
>> +put_page_alloc_ref([i]);
>> +put_page_and_type([i]);
>> +}
>> +}
>> +
>> +static int vmtrace_alloc_buffer(struct vcpu *v)
>> +{
>> +struct domain *d = v->domain;
>> +struct page_info *pg;
>> +unsigned int i;
>> +
>> +if ( !d->vmtrace_size )
>> +return 0;
>> +
>> +pg = alloc_domheap_pages(d, get_order_from_bytes(d->vmtrace_size),
>> + MEMF_no_refcount);
>> +if ( !pg )
>> +return -ENOMEM;
>> +
>> +for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>> +if ( unlikely(!get_page_and_type([i], d, PGT_writable_page)) )
>> +goto refcnt_err;
>> +
>> +/*
>> + * We must only let vmtrace_free_buffer() take any action in the success
>> + * case when we've taken all the refs it intends to drop.
>> + */
>> +v->vmtrace.pg = pg;
>> +return 0;
>> +
>> + refcnt_err:
>> +while ( i-- )
>> +put_page_and_type([i]);
>> +
>> +return -ENODATA;
> Would you mind at least logging how many pages may be leaked
> here? I also don't understand why you don't call
> put_page_alloc_ref() in the loop - that's fine to do prior to
> the put_page_and_type(), and will at least limit the leak.
> The buffer size here typically isn't insignificant, and it
> may be helpful to not unnecessarily defer the freeing to
> relinquish_resources() (assuming we will make that one also
> traverse the list of "extra" pages, but I understand that's
> not going to happen for 4.15 anymore anyway).
>
> Additionally, while I understand you're not in favor of the
> comments we have on all three similar code paths, I think
> replicating their comments here would help easily spotting
> (by grep-ing e.g. for "fishy") all instances that will need
> adjusting once we have figured a better overall solution.

How is:

    for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
    if ( unlikely(!get_page_and_type([i], d, PGT_writable_page)) )
    /*
 * The domain can't possibly know about this page yet, so
failure
 * here is a clear indication of something fishy going on.
 */
    goto refcnt_err;

    /*
 * We must only let vmtrace_free_buffer() take any action in the success
 * case when we've taken all the refs it intends to drop.
 */
    v->vmtrace.pg = pg;
    return 0;

 refcnt_err:
    /*
 * We can theoretically reach this point if someone has taken 2^43
refs on
 * the frames in the time the above loop takes to execute, or
someone has
 * made a blind decrease reservation hypercall and managed to pick the
 * right mfn.  Free the memory we safely can, and leak the rest.
 */
    while ( i-- )
    {
    put_page_alloc_ref([i]);
    put_page_and_type([i]);
    }

    return -ENODATA;

this?

~Andrew

RE: VIRIDIAN CRASH: 3b c0000096 75b12c5 9e7f1580 0

[Paul Durrant]

> -Original Message-
> From: Jan Beulich 
> Sent: 03 February 2021 15:43
> To: p...@xen.org
> Cc: xen-devel@lists.xenproject.org; 'James Dingwall' 
> 
> Subject: Re: VIRIDIAN CRASH: 3b c096 75b12c5 9e7f1580 0
> 
> On 03.02.2021 16:04, Paul Durrant wrote:
> >> From: Xen-devel  On Behalf Of Jan 
> >> Beulich
> >> Sent: 03 February 2021 14:55
> >>
> >> On 01.02.2021 16:26, James Dingwall wrote:
> >>> 21244@1612191983.282480:xen_platform_log xen platform: XEN|BUGCHECK: 
> >>> EXCEPTION (A824848948C2):
> >>> 21244@1612191983.282617:xen_platform_log xen platform: XEN|BUGCHECK: 
> >>> CONTEXT (D0014343D580):
> >>> 21244@1612191983.282717:xen_platform_log xen platform: XEN|BUGCHECK: - GS 
> >>> = 002B
> >>> 21244@1612191983.282816:xen_platform_log xen platform: XEN|BUGCHECK: - FS 
> >>> = 0053
> >>> 21244@1612191983.282914:xen_platform_log xen platform: XEN|BUGCHECK: - ES 
> >>> = 002B
> >>> 21244@1612191983.283011:xen_platform_log xen platform: XEN|BUGCHECK: - DS 
> >>> = 002B
> >>> 21244@1612191983.283127:xen_platform_log xen platform: XEN|BUGCHECK: - SS 
> >>> = 0018
> >>> 21244@1612191983.283226:xen_platform_log xen platform: XEN|BUGCHECK: - CS 
> >>> = 0010
> >>> 21244@1612191983.283332:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> EFLAGS = 0202
> >>> 21244@1612191983.283444:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RDI = F64D5C20
> >>> 21244@1612191983.283555:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RSI = F6367280
> >>> 21244@1612191983.283666:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RBX = 8011E060
> >>> 21244@1612191983.283810:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RDX = F64D5C20
> >>> 21244@1612191983.283972:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RCX = 0199
> >>> 21244@1612191983.284350:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RAX = 0004
> >>> 21244@1612191983.284523:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RBP = 4343E891
> >>> 21244@1612191983.284658:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RIP = A43C72C5
> >>> 21244@1612191983.284842:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> RSP = 4343DFA0
> >>> 21244@1612191983.284959:xen_platform_log xen platform: XEN|BUGCHECK: - R8 
> >>> = 0008
> >>> 21244@1612191983.285073:xen_platform_log xen platform: XEN|BUGCHECK: - R9 
> >>> = 000E
> >>> 21244@1612191983.285188:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> R10 = 0002
> >>> 21244@1612191983.285304:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> R11 = 4343E808
> >>> 21244@1612191983.285420:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> R12 = 
> >>> 21244@1612191983.285564:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> R13 = F7964E50
> >>> 21244@1612191983.285680:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> R14 = F64D5C20
> >>> 21244@1612191983.285796:xen_platform_log xen platform: XEN|BUGCHECK: - 
> >>> R15 = F7964E50
> >>
> >> I'm also confused by this - the pointer given for CONTEXT suggests this
> >> is a 64-bit kernel, yet none of the registers - including RIP and RSP -
> >> have non-zero upper 32 bits. Or is qemu truncating these values?
> >
> > The logging is coming from the PV drivers (in
> https://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/xen/bug_check.c).
>  The
> truncated values may just be due to a 32-bit user process I guess.
> 
> Since you pointed me at the code and truncation inside a string
> not likely being due to some user process, I went and looked:
> The driver uses %016X, instead of e.g. converting to (PVOID)
> and using %p like code elsewhere in the file does (presumably
> because there's no really convenient way to print 64-bit values
> in Windows, short of using their custom "%016I64X" format
> specifier, and the absence of a uniform specifier allowing to
> format pointer-sized integers independent of architecture).

Oh yes, good point... Other places in the code use the %p trick. It should be 
changed.

  Paul

> 
> Jan

Re: VIRIDIAN CRASH: 3b c0000096 75b12c5 9e7f1580 0

[Jan Beulich]

On 03.02.2021 16:04, Paul Durrant wrote:
>> From: Xen-devel  On Behalf Of Jan 
>> Beulich
>> Sent: 03 February 2021 14:55
>>
>> On 01.02.2021 16:26, James Dingwall wrote:
>>> 21244@1612191983.282480:xen_platform_log xen platform: XEN|BUGCHECK: 
>>> EXCEPTION (A824848948C2):
>>> 21244@1612191983.282617:xen_platform_log xen platform: XEN|BUGCHECK: 
>>> CONTEXT (D0014343D580):
>>> 21244@1612191983.282717:xen_platform_log xen platform: XEN|BUGCHECK: - GS = 
>>> 002B
>>> 21244@1612191983.282816:xen_platform_log xen platform: XEN|BUGCHECK: - FS = 
>>> 0053
>>> 21244@1612191983.282914:xen_platform_log xen platform: XEN|BUGCHECK: - ES = 
>>> 002B
>>> 21244@1612191983.283011:xen_platform_log xen platform: XEN|BUGCHECK: - DS = 
>>> 002B
>>> 21244@1612191983.283127:xen_platform_log xen platform: XEN|BUGCHECK: - SS = 
>>> 0018
>>> 21244@1612191983.283226:xen_platform_log xen platform: XEN|BUGCHECK: - CS = 
>>> 0010
>>> 21244@1612191983.283332:xen_platform_log xen platform: XEN|BUGCHECK: - 
>>> EFLAGS = 0202
>>> 21244@1612191983.283444:xen_platform_log xen platform: XEN|BUGCHECK: - RDI 
>>> = F64D5C20
>>> 21244@1612191983.283555:xen_platform_log xen platform: XEN|BUGCHECK: - RSI 
>>> = F6367280
>>> 21244@1612191983.283666:xen_platform_log xen platform: XEN|BUGCHECK: - RBX 
>>> = 8011E060
>>> 21244@1612191983.283810:xen_platform_log xen platform: XEN|BUGCHECK: - RDX 
>>> = F64D5C20
>>> 21244@1612191983.283972:xen_platform_log xen platform: XEN|BUGCHECK: - RCX 
>>> = 0199
>>> 21244@1612191983.284350:xen_platform_log xen platform: XEN|BUGCHECK: - RAX 
>>> = 0004
>>> 21244@1612191983.284523:xen_platform_log xen platform: XEN|BUGCHECK: - RBP 
>>> = 4343E891
>>> 21244@1612191983.284658:xen_platform_log xen platform: XEN|BUGCHECK: - RIP 
>>> = A43C72C5
>>> 21244@1612191983.284842:xen_platform_log xen platform: XEN|BUGCHECK: - RSP 
>>> = 4343DFA0
>>> 21244@1612191983.284959:xen_platform_log xen platform: XEN|BUGCHECK: - R8 = 
>>> 0008
>>> 21244@1612191983.285073:xen_platform_log xen platform: XEN|BUGCHECK: - R9 = 
>>> 000E
>>> 21244@1612191983.285188:xen_platform_log xen platform: XEN|BUGCHECK: - R10 
>>> = 0002
>>> 21244@1612191983.285304:xen_platform_log xen platform: XEN|BUGCHECK: - R11 
>>> = 4343E808
>>> 21244@1612191983.285420:xen_platform_log xen platform: XEN|BUGCHECK: - R12 
>>> = 
>>> 21244@1612191983.285564:xen_platform_log xen platform: XEN|BUGCHECK: - R13 
>>> = F7964E50
>>> 21244@1612191983.285680:xen_platform_log xen platform: XEN|BUGCHECK: - R14 
>>> = F64D5C20
>>> 21244@1612191983.285796:xen_platform_log xen platform: XEN|BUGCHECK: - R15 
>>> = F7964E50
>>
>> I'm also confused by this - the pointer given for CONTEXT suggests this
>> is a 64-bit kernel, yet none of the registers - including RIP and RSP -
>> have non-zero upper 32 bits. Or is qemu truncating these values?
> 
> The logging is coming from the PV drivers (in 
> https://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/xen/bug_check.c).
>  The truncated values may just be due to a 32-bit user process I guess.

Since you pointed me at the code and truncation inside a string
not likely being due to some user process, I went and looked:
The driver uses %016X, instead of e.g. converting to (PVOID)
and using %p like code elsewhere in the file does (presumably
because there's no really convenient way to print 64-bit values
in Windows, short of using their custom "%016I64X" format
specifier, and the absence of a uniform specifier allowing to
format pointer-sized integers independent of architecture).

Jan

Re: [PATCH] x86/string: correct memmove()'s forwarding to memcpy()

[Andrew Cooper]

On 03/02/2021 15:31, Jan Beulich wrote:
> On 03.02.2021 16:01, Andrew Cooper wrote:
>> On 03/02/2021 14:22, Jan Beulich wrote:
>>> With memcpy() expanding to the compiler builtin, we may not hand it
>>> overlapping source and destination. We strictly mean to forward to our
>>> own implementation (a few lines up in the same source file).
>>>
>>> Fixes: 78825e1c60fa ("x86/string: Clean up x86/string.h")
>>> Signed-off-by: Jan Beulich 
>> I agree that the current logic is buggy, but I'm not sure this is an
>> improvement.
>>
>> You've switched from relying on GCC's builtin to operate forwards, to
>> relying on Xen's implementation operating forwards.
> Is there such a guarantee for the compiler builtin? If so - no
> need for this patch indeed. But I couldn't find any doc saying
> so.

I've never seen it emit anything which isn't a forwards operation (i.e.
I think the compiled result tended to be safe in practice), but C's
flexibility does explicitly permit a backwards implementation.

>
>> At the very least, can we get a code comment stating something like
>> "depends on Xen's implementation operating forwards" ?
> No problem at all.

In which case Acked-by: Andrew Cooper  to
avoid a round trip.

>
>>> ---
>>> An alternative would be to "#undef memcpy" near the top of the file. But
>>> I think the way it's done now is more explicit to the reader. An #undef
>>> would be the only way if the macro was an object-like one.
>> I chose not to use undef's to avoid impacting the optimisation of other
>> functions in this file.  I can't remember if this made a difference in
>> practice.
>>
>>> At least with gcc10 this does alter generated code: The builtin gets
>>> expanded into a tail call, while after this change our memcpy() gets
>>> inlined into memmove(). This would change again once we separate the 3
>>> functions here into their own CUs for placing them in an archive.
>> As (perhaps) a tangent, how do we plan to provide x86-optimised versions
>> in combination with the library work?
> By specifying the per-arch lib.a first.

Ok - good to hear.

>>   We're long overdue needing to
>> refresh our fast-strings support to include fast rep-mov/stosb.
> That's pretty much orthogonal to the code movement though.

Yes, but it does need doing.  We're perpetually playing catchup, and
there are meaningful improvements to be had for logic such as
clear_page() which is fairly poor, optimisation wise atm.

~Andrew

Re: [PATCH] x86/string: correct memmove()'s forwarding to memcpy()

[Jan Beulich]

On 03.02.2021 16:01, Andrew Cooper wrote:
> On 03/02/2021 14:22, Jan Beulich wrote:
>> With memcpy() expanding to the compiler builtin, we may not hand it
>> overlapping source and destination. We strictly mean to forward to our
>> own implementation (a few lines up in the same source file).
>>
>> Fixes: 78825e1c60fa ("x86/string: Clean up x86/string.h")
>> Signed-off-by: Jan Beulich 
> 
> I agree that the current logic is buggy, but I'm not sure this is an
> improvement.
> 
> You've switched from relying on GCC's builtin to operate forwards, to
> relying on Xen's implementation operating forwards.

Is there such a guarantee for the compiler builtin? If so - no
need for this patch indeed. But I couldn't find any doc saying
so.

> At the very least, can we get a code comment stating something like
> "depends on Xen's implementation operating forwards" ?

No problem at all.

>> ---
>> An alternative would be to "#undef memcpy" near the top of the file. But
>> I think the way it's done now is more explicit to the reader. An #undef
>> would be the only way if the macro was an object-like one.
> 
> I chose not to use undef's to avoid impacting the optimisation of other
> functions in this file.  I can't remember if this made a difference in
> practice.
> 
>> At least with gcc10 this does alter generated code: The builtin gets
>> expanded into a tail call, while after this change our memcpy() gets
>> inlined into memmove(). This would change again once we separate the 3
>> functions here into their own CUs for placing them in an archive.
> 
> As (perhaps) a tangent, how do we plan to provide x86-optimised versions
> in combination with the library work?

By specifying the per-arch lib.a first.

>  We're long overdue needing to
> refresh our fast-strings support to include fast rep-mov/stosb.

That's pretty much orthogonal to the code movement though.

Jan

RE: VIRIDIAN CRASH: 3b c0000096 75b12c5 9e7f1580 0

[Paul Durrant]

> -Original Message-
> From: Xen-devel  On Behalf Of Jan 
> Beulich
> Sent: 03 February 2021 14:55
> To: James Dingwall 
> Cc: xen-devel@lists.xenproject.org
> Subject: Re: VIRIDIAN CRASH: 3b c096 75b12c5 9e7f1580 0
> 
> On 01.02.2021 16:26, James Dingwall wrote:
> > I am building the xen 4.11 branch at
> > 310ab79875cb705cc2c7daddff412b5a4899f8c9 which includes commit
> > 3b5de119f0399cbe745502cb6ebd5e6633cc139c "86/msr: fix handling of
> > MSR_IA32_PERF_{STATUS/CTL}".  I think this should address this error
> > recorded in xen's dmesg:
> >
> > (XEN) d11v0 VIRIDIAN CRASH: 3b c096 75b12c5 9e7f1580 0
> 
> It seems to me that you imply some information here which might
> better be spelled out. As it stands I do not see the immediate
> connection between the cited commit and the crash. C096 is
> STATUS_PRIVILEGED_INSTRUCTION, which to me ought to be impossible
> for code running in ring 0. Of course I may simply not know enough
> about modern Windows' internals to understand the connection.
> 
> > I have removed `viridian = [..]` from the xen config nut still get this
> > reliably when launching PassMark Performance Test and it is collecting
> > CPU information.
> >
> > This is recorded in the domain qemu-dm log:
> >
> > 21244@1612191983.279616:xen_platform_log xen platform: XEN|BUGCHECK: >
> > 21244@1612191983.279819:xen_platform_log xen platform: XEN|BUGCHECK: 
> > SYSTEM_SERVICE_EXCEPTION:
> C096 F800A43C72C5 D0014343D580 
> > 21244@1612191983.279959:xen_platform_log xen platform: XEN|BUGCHECK: 
> > EXCEPTION (F800A43C72C5):
> > 21244@1612191983.280075:xen_platform_log xen platform: XEN|BUGCHECK: - Code 
> > = C148320F
> > 21244@1612191983.280205:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Flags = 0B4820E2
> > 21244@1612191983.280346:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Address = A824948D4800
> > 21244@1612191983.280504:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[0] =
> 8B0769850F07
> > 21244@1612191983.280633:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[1] =
> 46B70F4024448906
> > 21244@1612191983.280754:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[2] =
> 0F2444896604
> > 21244@1612191983.280876:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[3] =
> E983C88B410646B6
> > 21244@1612191983.281012:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[4] =
> 0D7401E9831E7401
> > 21244@1612191983.281172:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[5] =
> 54B70F217502F983
> > 21244@1612191983.281304:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[6] =
> 54B70F15EBED4024
> > 21244@1612191983.281426:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[7] =
> EBC0B70FED664024
> > 21244@1612191983.281547:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[8] =
> 0FEC402454B70F09
> > 21244@1612191983.281668:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[9] =
> 448B42244489C0B6
> > 21244@1612191983.281809:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[10] =
> 2444B70F06894024
> > 21244@1612191983.281932:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[11] =
> 4688440446896644
> > 21244@1612191983.282052:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[12] =
> 073846C74906
> > 21244@1612191983.282185:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[13] =
> F883070AE900
> > 21244@1612191983.282340:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > Parameter[14] =
> 8B06F9850F07
> > 21244@1612191983.282480:xen_platform_log xen platform: XEN|BUGCHECK: 
> > EXCEPTION (A824848948C2):
> > 21244@1612191983.282617:xen_platform_log xen platform: XEN|BUGCHECK: 
> > CONTEXT (D0014343D580):
> > 21244@1612191983.282717:xen_platform_log xen platform: XEN|BUGCHECK: - GS = 
> > 002B
> > 21244@1612191983.282816:xen_platform_log xen platform: XEN|BUGCHECK: - FS = 
> > 0053
> > 21244@1612191983.282914:xen_platform_log xen platform: XEN|BUGCHECK: - ES = 
> > 002B
> > 21244@1612191983.283011:xen_platform_log xen platform: XEN|BUGCHECK: - DS = 
> > 002B
> > 21244@1612191983.283127:xen_platform_log xen platform: XEN|BUGCHECK: - SS = 
> > 0018
> > 21244@1612191983.283226:xen_platform_log xen platform: XEN|BUGCHECK: - CS = 
> > 0010
> > 21244@1612191983.283332:xen_platform_log xen platform: XEN|BUGCHECK: - 
> > EFLAGS = 0202
> > 21244@1612191983.283444:xen_platform_log xen platform: XEN|BUGCHECK: - RDI 
> > = F64D5C20
> > 21244@1612191983.283555:xen_platform_log xen platform: XEN|BUGCHECK: - RSI 
> > = F6367280
> > 21244@1612191983.283666:xen_platform_log xen platform: XEN|BUGCHECK: - RBX 
> > = 8011E060
> > 21244@1612191983.283810:xen_platform_log xen platform: XEN|BUGCHECK: - RDX 
> > = F64D5C20
> > 21244@1612191983.283972:xen_platform_log xen platform: XEN|BUGCHECK: - RCX 
> > = 0199
> 

Re: [PATCH] x86/string: correct memmove()'s forwarding to memcpy()

[Andrew Cooper]

On 03/02/2021 14:22, Jan Beulich wrote:
> With memcpy() expanding to the compiler builtin, we may not hand it
> overlapping source and destination. We strictly mean to forward to our
> own implementation (a few lines up in the same source file).
>
> Fixes: 78825e1c60fa ("x86/string: Clean up x86/string.h")
> Signed-off-by: Jan Beulich 

I agree that the current logic is buggy, but I'm not sure this is an
improvement.

You've switched from relying on GCC's builtin to operate forwards, to
relying on Xen's implementation operating forwards.

At the very least, can we get a code comment stating something like
"depends on Xen's implementation operating forwards" ?

> ---
> An alternative would be to "#undef memcpy" near the top of the file. But
> I think the way it's done now is more explicit to the reader. An #undef
> would be the only way if the macro was an object-like one.

I chose not to use undef's to avoid impacting the optimisation of other
functions in this file.  I can't remember if this made a difference in
practice.

> At least with gcc10 this does alter generated code: The builtin gets
> expanded into a tail call, while after this change our memcpy() gets
> inlined into memmove(). This would change again once we separate the 3
> functions here into their own CUs for placing them in an archive.

As (perhaps) a tangent, how do we plan to provide x86-optimised versions
in combination with the library work?  We're long overdue needing to
refresh our fast-strings support to include fast rep-mov/stosb.

~Andrew

>
> --- a/xen/arch/x86/string.c
> +++ b/xen/arch/x86/string.c
> @@ -43,7 +43,7 @@ void *(memmove)(void *dest, const void *
>  return dest;
>  
>  if ( dest < src )
> -return memcpy(dest, src, n);
> +return (memcpy)(dest, src, n);
>  
>  asm volatile (
>  "   std ; "

Re: VIRIDIAN CRASH: 3b c0000096 75b12c5 9e7f1580 0

[Jan Beulich]

On 01.02.2021 16:26, James Dingwall wrote:
> I am building the xen 4.11 branch at 
> 310ab79875cb705cc2c7daddff412b5a4899f8c9 which includes commit 
> 3b5de119f0399cbe745502cb6ebd5e6633cc139c "86/msr: fix handling of 
> MSR_IA32_PERF_{STATUS/CTL}".  I think this should address this error 
> recorded in xen's dmesg:
> 
> (XEN) d11v0 VIRIDIAN CRASH: 3b c096 75b12c5 9e7f1580 0

It seems to me that you imply some information here which might
better be spelled out. As it stands I do not see the immediate
connection between the cited commit and the crash. C096 is
STATUS_PRIVILEGED_INSTRUCTION, which to me ought to be impossible
for code running in ring 0. Of course I may simply not know enough
about modern Windows' internals to understand the connection.

> I have removed `viridian = [..]` from the xen config nut still get this 
> reliably when launching PassMark Performance Test and it is collecting 
> CPU information.
> 
> This is recorded in the domain qemu-dm log:
> 
> 21244@1612191983.279616:xen_platform_log xen platform: XEN|BUGCHECK: >
> 21244@1612191983.279819:xen_platform_log xen platform: XEN|BUGCHECK: 
> SYSTEM_SERVICE_EXCEPTION: C096 F800A43C72C5 D0014343D580 
> 
> 21244@1612191983.279959:xen_platform_log xen platform: XEN|BUGCHECK: 
> EXCEPTION (F800A43C72C5):
> 21244@1612191983.280075:xen_platform_log xen platform: XEN|BUGCHECK: - Code = 
> C148320F
> 21244@1612191983.280205:xen_platform_log xen platform: XEN|BUGCHECK: - Flags 
> = 0B4820E2
> 21244@1612191983.280346:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Address = A824948D4800
> 21244@1612191983.280504:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[0] = 8B0769850F07
> 21244@1612191983.280633:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[1] = 46B70F4024448906
> 21244@1612191983.280754:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[2] = 0F2444896604
> 21244@1612191983.280876:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[3] = E983C88B410646B6
> 21244@1612191983.281012:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[4] = 0D7401E9831E7401
> 21244@1612191983.281172:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[5] = 54B70F217502F983
> 21244@1612191983.281304:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[6] = 54B70F15EBED4024
> 21244@1612191983.281426:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[7] = EBC0B70FED664024
> 21244@1612191983.281547:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[8] = 0FEC402454B70F09
> 21244@1612191983.281668:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[9] = 448B42244489C0B6
> 21244@1612191983.281809:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[10] = 2444B70F06894024
> 21244@1612191983.281932:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[11] = 4688440446896644
> 21244@1612191983.282052:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[12] = 073846C74906
> 21244@1612191983.282185:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[13] = F883070AE900
> 21244@1612191983.282340:xen_platform_log xen platform: XEN|BUGCHECK: - 
> Parameter[14] = 8B06F9850F07
> 21244@1612191983.282480:xen_platform_log xen platform: XEN|BUGCHECK: 
> EXCEPTION (A824848948C2):
> 21244@1612191983.282617:xen_platform_log xen platform: XEN|BUGCHECK: CONTEXT 
> (D0014343D580):
> 21244@1612191983.282717:xen_platform_log xen platform: XEN|BUGCHECK: - GS = 
> 002B
> 21244@1612191983.282816:xen_platform_log xen platform: XEN|BUGCHECK: - FS = 
> 0053
> 21244@1612191983.282914:xen_platform_log xen platform: XEN|BUGCHECK: - ES = 
> 002B
> 21244@1612191983.283011:xen_platform_log xen platform: XEN|BUGCHECK: - DS = 
> 002B
> 21244@1612191983.283127:xen_platform_log xen platform: XEN|BUGCHECK: - SS = 
> 0018
> 21244@1612191983.283226:xen_platform_log xen platform: XEN|BUGCHECK: - CS = 
> 0010
> 21244@1612191983.283332:xen_platform_log xen platform: XEN|BUGCHECK: - EFLAGS 
> = 0202
> 21244@1612191983.283444:xen_platform_log xen platform: XEN|BUGCHECK: - RDI = 
> F64D5C20
> 21244@1612191983.283555:xen_platform_log xen platform: XEN|BUGCHECK: - RSI = 
> F6367280
> 21244@1612191983.283666:xen_platform_log xen platform: XEN|BUGCHECK: - RBX = 
> 8011E060
> 21244@1612191983.283810:xen_platform_log xen platform: XEN|BUGCHECK: - RDX = 
> F64D5C20
> 21244@1612191983.283972:xen_platform_log xen platform: XEN|BUGCHECK: - RCX = 
> 0199
> 21244@1612191983.284350:xen_platform_log xen platform: XEN|BUGCHECK: - RAX = 
> 0004
> 21244@1612191983.284523:xen_platform_log xen platform: XEN|BUGCHECK: - RBP = 
> 4343E891
> 21244@1612191983.284658:xen_platform_log xen platform: XEN|BUGCHECK: - RIP = 
> A43C72C5
> 21244@1612191983.284842:xen_platform_log xen platform: XEN|BUGCHECK: - RSP = 
> 4343DFA0
> 

Re: [PATCH] tools/libxl: only set viridian flags on new domains

[Ian Jackson]

Igor Druzhinin writes ("[PATCH] tools/libxl: only set viridian flags on new 
domains"):
> Domains migrating or restoring should have viridian HVM param key in
> the migration stream already and setting that twice results in Xen
> returing -EEXIST on the second attempt later (during migration stream parsing)
> in case the values don't match. That causes migration/restore operation
> to fail at destination side.
> 
> That issue is now resurfaced by the latest commits (983524671 and 7e5cffcd1e)
> extending default viridian feature set making the values from the previous
> migration streams and those set at domain construction different.

I am OK with this in principle but I would have preferred the prep
work of passing libxl__domain_build_state* through to more places to
be split out into its own patch.

As it is it's not easy to see the wood for the trees.  If we weren't
in the freeze I would probably just shrug and ack it but I think a bit
more care is needed at this stage.

Would you mind splitting the patch in two, the first of which is "no
intended functional change" ?

Thanks,
Ian.

[PATCH] x86/string: correct memmove()'s forwarding to memcpy()

[Jan Beulich]

With memcpy() expanding to the compiler builtin, we may not hand it
overlapping source and destination. We strictly mean to forward to our
own implementation (a few lines up in the same source file).

Fixes: 78825e1c60fa ("x86/string: Clean up x86/string.h")
Signed-off-by: Jan Beulich 
---
An alternative would be to "#undef memcpy" near the top of the file. But
I think the way it's done now is more explicit to the reader. An #undef
would be the only way if the macro was an object-like one.

At least with gcc10 this does alter generated code: The builtin gets
expanded into a tail call, while after this change our memcpy() gets
inlined into memmove(). This would change again once we separate the 3
functions here into their own CUs for placing them in an archive.

--- a/xen/arch/x86/string.c
+++ b/xen/arch/x86/string.c
@@ -43,7 +43,7 @@ void *(memmove)(void *dest, const void *
 return dest;
 
 if ( dest < src )
-return memcpy(dest, src, n);
+return (memcpy)(dest, src, n);
 
 asm volatile (
 "   std ; "

Re: [PATCH] tools/libxl: only set viridian flags on new domains

[Andrew Cooper]

On 03/02/2021 04:01, Igor Druzhinin wrote:
> Domains migrating or restoring should have viridian HVM param key in
> the migration stream already and setting that twice results in Xen
> returing -EEXIST on the second attempt later (during migration stream parsing)
> in case the values don't match. That causes migration/restore operation
> to fail at destination side.
>
> That issue is now resurfaced by the latest commits (983524671 and 7e5cffcd1e)
> extending default viridian feature set making the values from the previous
> migration streams and those set at domain construction different.
>
> Signed-off-by: Igor Druzhinin 

Acked-by: Andrew Cooper 

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 02:11:43PM +0100, Jürgen Groß wrote:
> Not using git on a daily basis I really suggest to use stgit as an
> add.on:
> 
> https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_StGit
> 
> It makes handling multiple iterations of a patch rather easy.

thanks. When I started, I looked at the wiki for instructions about
patches, but didn't find any ...

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 14:03, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 01:58:19PM +0100, Jürgen Groß wrote:

On 03.02.21 13:47, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote:

Uh, this is no regular patch.


I though by regular patch, you meants a plain diff -u


You've sent correct patches before,


Yes, and it's very time-consuming. This is why I want to have it right the
first time and not go through sevreral iterations with this protocol.



Its not that hard if you have a proper git tree...


git is the problem actually. I'm not used to it ...



Not using git on a daily basis I really suggest to use stgit as an
add.on:

https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_StGit

It makes handling multiple iterations of a patch rather easy.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 01:58:19PM +0100, Jürgen Groß wrote:
> On 03.02.21 13:47, Manuel Bouyer wrote:
> > On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote:
> > > Uh, this is no regular patch.
> > 
> > I though by regular patch, you meants a plain diff -u
> > 
> > > You've sent correct patches before,
> > 
> > Yes, and it's very time-consuming. This is why I want to have it right the
> > first time and not go through sevreral iterations with this protocol.
> > 
> 
> Its not that hard if you have a proper git tree...

git is the problem actually. I'm not used to it ...

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 13:47, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote:

Uh, this is no regular patch.


I though by regular patch, you meants a plain diff -u


You've sent correct patches before,


Yes, and it's very time-consuming. This is why I want to have it right the
first time and not go through sevreral iterations with this protocol.



Its not that hard if you have a proper git tree...


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

[ovmf test] 158975: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 618e6a1f21a11eaee0a92e19c753969eb4a1b198
baseline version:
 ovmf 3f90ac3ec03512e2374cd2968c047a7e856a8965

Last test of basis   158959  2021-02-02 16:40:59 Z0 days
Testing same since   158975  2021-02-03 07:14:36 Z0 days1 attempts


People who touched revisions under test:
  gechao 
  Marc Moisson-Franckhauser 
  Sami Mujawar 
  Vijayenthiran Subramaniam 

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
   3f90ac3ec0..618e6a1f21  618e6a1f21a11eaee0a92e19c753969eb4a1b198 -> 
xen-tested-master

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote:
> Uh, this is no regular patch.

I though by regular patch, you meants a plain diff -u

> You've sent correct patches before,

Yes, and it's very time-consuming. This is why I want to have it right the
first time and not go through sevreral iterations with this protocol.

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 13:33, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 01:21:59PM +0100, Jürgen Groß wrote:

Will do

In the patch I'm going to submit, may I add

Reviewed-by: Jürgen Groß 
?



Let me see the patch (including commit message) first, please.

So just send out as a regular patch, and I'll respond accordingly. :-)


Attached is the new patch.

As commit message I suggest:

xenstored: close socket connections on error

On error, don't keep socket connection in ignored state but close them.
When the remote end of a socket is closed, xenstored will flag it as an
error and switch the connection to ignored. But on some OSes (e.g.
NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
state will stay open forever in xenstored (and it will loop with CPU 100%
busy).



Uh, this is no regular patch. You've sent correct patches before,
haven't you? The patch should have a Signed-off-by: and in this case a
Fixes: tag as well (hint: the patch breaking your use case was
commit d2fa370d3ef9cbe22d7256c608671cdcdf6e0083). See

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 01:21:59PM +0100, Jürgen Groß wrote:
> > Will do
> > 
> > In the patch I'm going to submit, may I add
> > 
> > Reviewed-by: Jürgen Groß 
> > ?
> > 
> 
> Let me see the patch (including commit message) first, please.
> 
> So just send out as a regular patch, and I'll respond accordingly. :-)

Attached is the new patch.

As commit message I suggest:

xenstored: close socket connections on error

On error, don't keep socket connection in ignored state but close them.
When the remote end of a socket is closed, xenstored will flag it as an
error and switch the connection to ignored. But on some OSes (e.g.
NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored
state will stay open forever in xenstored (and it will loop with CPU 100%
busy).

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--
--- xenstored_core.c.orig   2021-02-03 13:25:38.568308454 +0100
+++ xenstored_core.c2021-02-03 13:25:44.282429872 +0100
@@ -1440,6 +1440,9 @@
 
talloc_free(conn->in);
conn->in = NULL;
+   /* if this is a socket connection, drop it now */
+   if (conn->fd >= 0)
+   talloc_free(conn);
 }
 
 static const char *sockmsg_string(enum xsd_sockmsg_type type)

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 13:17, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 01:13:53PM +0100, Jürgen Groß wrote:

On 03.02.21 13:03, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote:

On 03.02.21 12:48, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote:

[...]
This shouldn't happen in case we are closing the socket actively.

In the end we should just do a talloc_free(conn) in
ignore_connection() if it is a socket based one. This should revert
the critical modification of the XSA-115 fixes for sockets while
keeping the desired effect for domain connections.


Hello
here's an updated patch which works for me. Does anyone see a problem
with it ? If not I will submit it for commit.



Do you really need the first hunk? I would have thought just freeing
conn in ignore_connection() is enough.

In case you are seeing problems without the first hunk, please say so
in a comment added to this hunk in order to avoid it being removed
sometimes in the future.


No I don't need it. From your previous comments I though it was a good idea
to keep it, but I can remove it if you think it's better.


Yes, please remove it.


Will do

In the patch I'm going to submit, may I add

Reviewed-by: Jürgen Groß 
?



Let me see the patch (including commit message) first, please.

So just send out as a regular patch, and I'll respond accordingly. :-)


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 01:13:53PM +0100, Jürgen Groß wrote:
> On 03.02.21 13:03, Manuel Bouyer wrote:
> > On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote:
> > > On 03.02.21 12:48, Manuel Bouyer wrote:
> > > > On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote:
> > > > > [...]
> > > > > This shouldn't happen in case we are closing the socket actively.
> > > > > 
> > > > > In the end we should just do a talloc_free(conn) in
> > > > > ignore_connection() if it is a socket based one. This should revert
> > > > > the critical modification of the XSA-115 fixes for sockets while
> > > > > keeping the desired effect for domain connections.
> > > > 
> > > > Hello
> > > > here's an updated patch which works for me. Does anyone see a problem
> > > > with it ? If not I will submit it for commit.
> > > > 
> > > 
> > > Do you really need the first hunk? I would have thought just freeing
> > > conn in ignore_connection() is enough.
> > > 
> > > In case you are seeing problems without the first hunk, please say so
> > > in a comment added to this hunk in order to avoid it being removed
> > > sometimes in the future.
> > 
> > No I don't need it. From your previous comments I though it was a good idea
> > to keep it, but I can remove it if you think it's better.
> 
> Yes, please remove it.

Will do

In the patch I'm going to submit, may I add

Reviewed-by: Jürgen Groß 
?

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 13:03, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote:

On 03.02.21 12:48, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote:

[...]
This shouldn't happen in case we are closing the socket actively.

In the end we should just do a talloc_free(conn) in
ignore_connection() if it is a socket based one. This should revert
the critical modification of the XSA-115 fixes for sockets while
keeping the desired effect for domain connections.


Hello
here's an updated patch which works for me. Does anyone see a problem
with it ? If not I will submit it for commit.



Do you really need the first hunk? I would have thought just freeing
conn in ignore_connection() is enough.

In case you are seeing problems without the first hunk, please say so
in a comment added to this hunk in order to avoid it being removed
sometimes in the future.


No I don't need it. From your previous comments I though it was a good idea
to keep it, but I can remove it if you think it's better.


Yes, please remove it.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote:
> On 03.02.21 12:48, Manuel Bouyer wrote:
> > On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote:
> > > [...]
> > > This shouldn't happen in case we are closing the socket actively.
> > > 
> > > In the end we should just do a talloc_free(conn) in
> > > ignore_connection() if it is a socket based one. This should revert
> > > the critical modification of the XSA-115 fixes for sockets while
> > > keeping the desired effect for domain connections.
> > 
> > Hello
> > here's an updated patch which works for me. Does anyone see a problem
> > with it ? If not I will submit it for commit.
> > 
> 
> Do you really need the first hunk? I would have thought just freeing
> conn in ignore_connection() is enough.
> 
> In case you are seeing problems without the first hunk, please say so
> in a comment added to this hunk in order to avoid it being removed
> sometimes in the future.

No I don't need it. From your previous comments I though it was a good idea
to keep it, but I can remove it if you think it's better.

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Null scheduler and vwfi native problem

[Jürgen Groß]

On 03.02.21 12:20, Julien Grall wrote:

Hi Juergen,

On 03/02/2021 11:00, Jürgen Groß wrote:

On 03.02.21 10:19, Julien Grall wrote:

Hi,

On 03/02/2021 07:31, Dario Faggioli wrote:

On Tue, 2021-02-02 at 15:23 +, Julien Grall wrote:

In reality, it is probably still too early as a pCPU can be
considered
quiesced until a call to rcu_lock*() (such rcu_lock_domain()).


Well, yes, in theory, we could track down which is the first RCU read
side crit. section on this path, and put the call right before that (if
I understood what you mean).


Oh, that's not what I meant. This will indeed be far more complex 
than I originally had in mind.


AFAIU, the RCU uses critical section to protect data. So the 
"entering" could be used as "the pCPU is not quiesced" and "exiting" 
could be used as "the pCPU is quiesced".


The concern with my approach is we would need to make sure that Xen 
correctly uses the rcu helpers. I know Juergen worked on that 
recently, but I don't know whether this is fully complete.


I think it is complete, but I can't be sure, of course.

One bit missing (for catching some wrong uses of the helpers) is this
patch:

https://lists.xen.org/archives/html/xen-devel/2020-03/msg01759.html

I don't remember why it hasn't been taken, but I think there was a
specific reason for that.


Looking at v8 and the patch is suitably reviewed by Jan. So I am a bit 
puzzled to why this wasn't committed... I had to go to v6 and notice the 
following message:


"albeit to be honest I'm not fully convinced we need to go this far."

Was the implication that his reviewed-by was conditional to someone else 
answering to the e-mail?


I have no record for that being the case.

Patches 1-3 of that series were needed for getting rid of
stop_machine_run() in rcu handling and to fix other problems. Patch 4
was adding some additional ASSERT()s for making sure no potential
deadlocks due to wrong rcu usage could creep in again.

Patch 5 was more a "nice to have" addition in order to avoid any
wrong usage of rcu which should have no real negative impact on the
system stability.

So I believe Jan as the committer didn't want to commit it himself, but
was fine with the overall idea and implementation.

I still think for code sanity it would be nice, but I was rather busy
with Xenstore and event channel security work at that time, so I didn't
urge anyone to take this patch.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 12:48, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote:

[...]
This shouldn't happen in case we are closing the socket actively.

In the end we should just do a talloc_free(conn) in
ignore_connection() if it is a socket based one. This should revert
the critical modification of the XSA-115 fixes for sockets while
keeping the desired effect for domain connections.


Hello
here's an updated patch which works for me. Does anyone see a problem
with it ? If not I will submit it for commit.



Do you really need the first hunk? I would have thought just freeing
conn in ignore_connection() is enough.

In case you are seeing problems without the first hunk, please say so
in a comment added to this hunk in order to avoid it being removed
sometimes in the future.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

[linux-5.4 bisection] complete test-arm64-arm64-xl

[osstest service owner]

branch xen-unstable
xenbranch xen-unstable
job test-arm64-arm64-xl
testid guest-start

Tree: linux 
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: seabios git://xenbits.xen.org/osstest/seabios.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

  Bug is in tree:  linux 
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
  Bug introduced:  a09d4e7acdbf276b2096661ee82454ae3dd24d2b
  Bug not present: acc402fa5bf502d471d50e3d495379f093a7f9e4
  Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/158983/


  commit a09d4e7acdbf276b2096661ee82454ae3dd24d2b
  Author: David Woodhouse 
  Date:   Wed Jan 13 13:26:02 2021 +
  
  xen: Fix event channel callback via INTX/GSI
  
  [ Upstream commit 3499ba8198cad47b731792e5e56b9ec2a78a83a2 ]
  
  For a while, event channel notification via the PCI platform device
  has been broken, because we attempt to communicate with xenstore before
  we even have notifications working, with the xs_reset_watches() call
  in xs_init().
  
  We tend to get away with this on Xen versions below 4.0 because we avoid
  calling xs_reset_watches() anyway, because xenstore might not cope with
  reading a non-existent key. And newer Xen *does* have the vector
  callback support, so we rarely fall back to INTX/GSI delivery.
  
  To fix it, clean up a bit of the mess of xs_init() and xenbus_probe()
  startup. Call xs_init() directly from xenbus_init() only in the !XS_HVM
  case, deferring it to be called from xenbus_probe() in the XS_HVM case
  instead.
  
  Then fix up the invocation of xenbus_probe() to happen either from its
  device_initcall if the callback is available early enough, or when the
  callback is finally set up. This means that the hack of calling
  xenbus_probe() from a workqueue after the first interrupt, or directly
  from the PCI platform device setup, is no longer needed.
  
  Signed-off-by: David Woodhouse 
  Reviewed-by: Boris Ostrovsky 
  Link: 
https://lore.kernel.org/r/20210113132606.422794-2-dw...@infradead.org
  Signed-off-by: Juergen Gross 
  Signed-off-by: Sasha Levin 


For bisection revision-tuple graph see:
   
http://logs.test-lab.xenproject.org/osstest/results/bisect/linux-5.4/test-arm64-arm64-xl.guest-start.html
Revision IDs in each graph node refer, respectively, to the Trees above.


Running cs-bisection-step 
--graph-out=/home/logs/results/bisect/linux-5.4/test-arm64-arm64-xl.guest-start 
--summary-out=tmp/158983.bisection-summary --basis-template=158387 
--blessings=real,real-bisect,real-retry linux-5.4 test-arm64-arm64-xl 
guest-start
Searching for failure / basis pass:
 158962 fail [host=laxton1] / 158681 [host=rochester1] 158624 [host=rochester0] 
158616 [host=rochester1] 158609 [host=rochester0] 158603 [host=rochester1] 
158593 [host=laxton0] 158583 ok.
Failure / basis pass flights: 158962 / 158583
Tree: linux 
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: seabios git://xenbits.xen.org/osstest/seabios.git
Tree: xen git://xenbits.xen.org/xen.git
Latest 0fbca6ce4174724f28be5268c5d210f51ed96e31 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3b468095cd3dfcd1aa4ae63bc623f534bc2390d2 
7ea428895af2840d85c524f0bd11a38aac308308 
ef88eeaf052c8a7d28c5f85e790c5e45bcffa45e 
9dc687f155a57216b83b17f9cde55dd43e06b0cd
Basis pass d26b3110041a9fddc6c6e36398f53f7eab8cff82 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
5b4a97bbc39ed8e7eb50038b9cffe2e948e49995 
7ea428895af2840d85c524f0bd11a38aac308308 
ef88eeaf052c8a7d28c5f85e790c5e45bcffa45e 
e8adbf680b56a3f4b9600c7bcc04fec1877a6213
Generating revisions with ./adhoc-revtuple-generator  
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git#d26b3110041a9fddc6c6e36398f53f7eab8cff82-0fbca6ce4174724f28be5268c5d210f51ed96e31
 
git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860
 
git://xenbits.xen.org/osstest/ovmf.git#5b4a97bbc39ed8e7eb50038b9cffe2e948e49995-3b468095cd3dfcd1aa4ae63bc623f534bc2390d2
 git://xenbits.xen.org/qemu-xen.git#7ea4288\
 95af2840d85c524f0bd11a38aac308308-7ea428895af2840d85c524f0bd11a38aac308308 
git://xenbits.xen.org/osstest/seabios.git#ef88eeaf052c8a7d28c5f85e790c5e45bcffa45e-ef88eeaf052c8a7d28c5f85e790c5e45bcffa45e
 
git://xenbits.xen.org/xen.git#e8adbf680b56a3f4b9600c7bcc04fec1877a6213-9dc687f155a57216b83b17f9cde55dd43e06b0cd
Loaded 15001 nodes in revision graph
Searching for test results:
 158563 

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote:
> [...]
> This shouldn't happen in case we are closing the socket actively.
> 
> In the end we should just do a talloc_free(conn) in
> ignore_connection() if it is a socket based one. This should revert
> the critical modification of the XSA-115 fixes for sockets while
> keeping the desired effect for domain connections.

Hello
here's an updated patch which works for me. Does anyone see a problem
with it ? If not I will submit it for commit.

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--
--- xenstored_core.c.orig   2021-02-02 18:06:33.389316841 +0100
+++ xenstored_core.c2021-02-03 12:46:17.204376338 +0100
@@ -397,9 +397,12 @@
 !list_empty(>out_list)))
*ptimeout = 0;
} else {
-   short events = POLLIN|POLLPRI;
-   if (!list_empty(>out_list))
-   events |= POLLOUT;
+   short events = 0;
+   if (!conn->is_ignored) {
+   events |= POLLIN|POLLPRI;
+   if (!list_empty(>out_list))
+   events |= POLLOUT;
+   }
conn->pollfd_idx = set_fd(conn->fd, events);
}
}
@@ -1440,6 +1443,9 @@
 
talloc_free(conn->in);
conn->in = NULL;
+   /* if this is a socket connection, drop it now */
+   if (conn->fd >= 0)
+   talloc_free(conn);
 }
 
 static const char *sockmsg_string(enum xsd_sockmsg_type type)

Re: Null scheduler and vwfi native problem

[Julien Grall]

Hi Juergen,

On 03/02/2021 11:00, Jürgen Groß wrote:

On 03.02.21 10:19, Julien Grall wrote:

Hi,

On 03/02/2021 07:31, Dario Faggioli wrote:

On Tue, 2021-02-02 at 15:23 +, Julien Grall wrote:

In reality, it is probably still too early as a pCPU can be
considered
quiesced until a call to rcu_lock*() (such rcu_lock_domain()).


Well, yes, in theory, we could track down which is the first RCU read
side crit. section on this path, and put the call right before that (if
I understood what you mean).


Oh, that's not what I meant. This will indeed be far more complex than 
I originally had in mind.


AFAIU, the RCU uses critical section to protect data. So the 
"entering" could be used as "the pCPU is not quiesced" and "exiting" 
could be used as "the pCPU is quiesced".


The concern with my approach is we would need to make sure that Xen 
correctly uses the rcu helpers. I know Juergen worked on that 
recently, but I don't know whether this is fully complete.


I think it is complete, but I can't be sure, of course.

One bit missing (for catching some wrong uses of the helpers) is this
patch:

https://lists.xen.org/archives/html/xen-devel/2020-03/msg01759.html

I don't remember why it hasn't been taken, but I think there was a
specific reason for that.


Looking at v8 and the patch is suitably reviewed by Jan. So I am a bit 
puzzled to why this wasn't committed... I had to go to v6 and notice the 
following message:


"albeit to be honest I'm not fully convinced we need to go this far."

Was the implication that his reviewed-by was conditional to someone else 
answering to the e-mail?


Cheers,

--
Julien Grall

Re: Null scheduler and vwfi native problem

[Jürgen Groß]

On 03.02.21 10:19, Julien Grall wrote:

Hi,

On 03/02/2021 07:31, Dario Faggioli wrote:

On Tue, 2021-02-02 at 15:23 +, Julien Grall wrote:

In reality, it is probably still too early as a pCPU can be
considered
quiesced until a call to rcu_lock*() (such rcu_lock_domain()).


Well, yes, in theory, we could track down which is the first RCU read
side crit. section on this path, and put the call right before that (if
I understood what you mean).


Oh, that's not what I meant. This will indeed be far more complex than I 
originally had in mind.


AFAIU, the RCU uses critical section to protect data. So the "entering" 
could be used as "the pCPU is not quiesced" and "exiting" could be used 
as "the pCPU is quiesced".


The concern with my approach is we would need to make sure that Xen 
correctly uses the rcu helpers. I know Juergen worked on that recently, 
but I don't know whether this is fully complete.


I think it is complete, but I can't be sure, of course.

One bit missing (for catching some wrong uses of the helpers) is this
patch:

https://lists.xen.org/archives/html/xen-devel/2020-03/msg01759.html

I don't remember why it hasn't been taken, but I think there was a
specific reason for that.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

[linux-5.4 test] 158962: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 158387
 test-amd64-amd64-dom0pvh-xl-amd 14 guest-start   fail REGR. vs. 158387
 test-amd64-amd64-xl-multivcpu 14 guest-start fail REGR. vs. 158387
 test-amd64-amd64-xl-pvhv2-amd 14 guest-start fail REGR. vs. 158387
 test-amd64-coresched-amd64-xl 14 guest-start fail REGR. vs. 158387
 test-amd64-amd64-qemuu-freebsd12-amd64 13 guest-startfail REGR. vs. 158387
 test-amd64-coresched-i386-xl 14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-libvirt 14 guest-start  fail REGR. vs. 158387
 test-arm64-arm64-xl  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-qemut-rhel6hvm-amd 12 redhat-install fail REGR. vs. 158387
 test-amd64-i386-qemuu-rhel6hvm-amd 12 redhat-install fail REGR. vs. 158387
 test-amd64-i386-freebsd10-amd64 13 guest-start   fail REGR. vs. 158387
 test-amd64-amd64-xl-pvshim   14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-pvhv2-intel 14 guest-start   fail REGR. vs. 158387
 test-amd64-i386-freebsd10-i386 13 guest-startfail REGR. vs. 158387
 test-amd64-amd64-xl-xsm  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-libvirt  14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-libvirt-xsm 14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-credit1  14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-pair25 guest-start/debian   fail REGR. vs. 158387
 test-amd64-amd64-xl  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl   14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-shadow   14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl-xsm   14 guest-start  fail REGR. vs. 158387
 test-amd64-amd64-xl-credit2  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-pair 25 guest-start/debian   fail REGR. vs. 158387
 test-amd64-i386-libvirt-xsm  14 guest-start  fail REGR. vs. 158387
 test-arm64-arm64-xl-seattle  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-libvirt-pair 25 guest-start/debian   fail REGR. vs. 158387
 test-amd64-amd64-libvirt-pair 25 guest-start/debian  fail REGR. vs. 158387
 test-amd64-i386-qemut-rhel6hvm-intel 12 redhat-install   fail REGR. vs. 158387
 test-amd64-amd64-qemuu-nested-amd 12 debian-hvm-install  fail REGR. vs. 158387
 test-amd64-i386-qemuu-rhel6hvm-intel 12 redhat-install   fail REGR. vs. 158387
 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install  fail REGR. vs. 158387
 test-amd64-i386-xl-qemut-win7-amd64 12 windows-install   fail REGR. vs. 158387
 test-amd64-amd64-amd64-pvgrub 12 debian-di-install   fail REGR. vs. 158387
 test-amd64-amd64-pygrub  12 debian-di-installfail REGR. vs. 158387
 test-amd64-amd64-qemuu-nested-intel 12 debian-hvm-install fail REGR. vs. 158387
 test-amd64-amd64-xl-qemut-win7-amd64 12 windows-install  fail REGR. vs. 158387
 test-amd64-amd64-i386-pvgrub 12 debian-di-installfail REGR. vs. 158387
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-amd64-i386-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-arm64-arm64-xl-xsm  14 guest-start  fail REGR. vs. 158387
 test-arm64-arm64-xl-credit1  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 158387
 test-arm64-arm64-xl-credit2  14 guest-start  fail REGR. vs. 158387
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. 
vs. 158387
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 12 debian-hvm-install 
fail REGR. vs. 158387
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-amd64-xl-qemut-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-amd64-xl-qcow212 debian-di-installfail REGR. vs. 158387
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail 
REGR. vs. 158387
 test-amd64-i386-xl-qemut-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 
158387
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 12 debian-hvm-install 
fail REGR. vs. 158387
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 12 debian-hvm-install 
fail REGR. vs. 158387
 test-amd64-amd64-libvirt-vhd 12 debian-di-installfail REGR. vs. 

[xen-unstable-coverity test] 158979: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 xen  5e7aa904405fa2f268c3af213516bae271de3265
baseline version:
 xen  9dc687f155a57216b83b17f9cde55dd43e06b0cd

Last test of basis   158849  2021-01-31 09:18:27 Z3 days
Testing same since   158979  2021-02-03 09:18:36 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Ian Jackson 
  Jan Beulich 
  Julien Grall 
  Manuel Bouyer 
  Roger Pau Monne 
  Roger Pau Monné 

jobs:
 coverity-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/xen.git
   9dc687f155..5e7aa90440  5e7aa904405fa2f268c3af213516bae271de3265 -> 
coverity-tested/smoke

Re: Null scheduler and vwfi native problem

[Julien Grall]

Hi,

On 03/02/2021 07:31, Dario Faggioli wrote:

On Tue, 2021-02-02 at 15:23 +, Julien Grall wrote:

In reality, it is probably still too early as a pCPU can be
considered
quiesced until a call to rcu_lock*() (such rcu_lock_domain()).


Well, yes, in theory, we could track down which is the first RCU read
side crit. section on this path, and put the call right before that (if
I understood what you mean).


Oh, that's not what I meant. This will indeed be far more complex than I 
originally had in mind.


AFAIU, the RCU uses critical section to protect data. So the "entering" 
could be used as "the pCPU is not quiesced" and "exiting" could be used 
as "the pCPU is quiesced".


The concern with my approach is we would need to make sure that Xen 
correctly uses the rcu helpers. I know Juergen worked on that recently, 
but I don't know whether this is fully complete.


Cheers,

--
Julien Grall

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 09:16, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 09:05:27AM +0100, Jürgen Groß wrote:

[...]
Yes, I think this is a good idea.


Well, after some sleep I don't think it is. We should always keep at last
POLLIN or we will never notice a socket close otherwise.


Adding the fd of an ignored socket connection to the list is the real
problem here. Why should that be done?


If we don't do it, we never notice when the socket is closed and the file
descriptor will stay forever. When I tried it, I had about 50 zombie
file descriptors open in xenstored, after starting only 2 domains.


This shouldn't happen in case we are closing the socket actively.

In the end we should just do a talloc_free(conn) in
ignore_connection() if it is a socket based one. This should revert
the critical modification of the XSA-115 fixes for sockets while
keeping the desired effect for domain connections.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: xenstored file descriptor leak

[Manuel Bouyer]

On Wed, Feb 03, 2021 at 09:05:27AM +0100, Jürgen Groß wrote:
> > > [...]
> > > Yes, I think this is a good idea.
> > 
> > Well, after some sleep I don't think it is. We should always keep at last
> > POLLIN or we will never notice a socket close otherwise.
> 
> Adding the fd of an ignored socket connection to the list is the real
> problem here. Why should that be done?

If we don't do it, we never notice when the socket is closed and the file
descriptor will stay forever. When I tried it, I had about 50 zombie
file descriptors open in xenstored, after starting only 2 domains.

> > > > 
> > > > Now I wonder if, on NetBSD at last, a read error or short read shouldn't
> > > > cause the socket to be closed, as with:
> > > > 
> > > > @@ -1561,6 +1565,8 @@
> > > >bad_client:
> > > > ignore_connection(conn);
> > > > +   /* we don't want to keep this connection alive */
> > > > +   talloc_free(conn);
> > > >}
> > > 
> > > This is wrong for non-socket connections, as we want to keep the domain
> > > in question to be known to xenstored.
> > > 
> > > For socket connections this should be okay, though.
> > 
> > What are "non-socket connections" BTW ? I don't think I've seen one
> > in my test.
> 
> Every connection to another domain.
> 
> > Is there a way to know if a connection is socket or non-socket ?
> 
> Active socket connections have conn->fd >= 0.

OK, I'll rework my patch. Thanks 

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

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

[osstest service owner]

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

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds18 guest-start/debian.repeat fail REGR. vs. 158922

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

version targeted for testing:
 xen  5e7aa904405fa2f268c3af213516bae271de3265
baseline version:
 xen  9dc687f155a57216b83b17f9cde55dd43e06b0cd

Last test of basis   158922  2021-02-02 01:51:30 Z1 days
Testing same since   158957  2021-02-02 14:38:10 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Ian Jackson 
  Jan Beulich 
  Julien Grall 
  Manuel Bouyer 
  Roger Pau Monne 
  Roger Pau Monné 

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

Re: xenstored file descriptor leak

[Jürgen Groß]

On 03.02.21 08:57, Manuel Bouyer wrote:

On Wed, Feb 03, 2021 at 07:18:51AM +0100, Jürgen Groß wrote:

On 02.02.21 19:37, Manuel Bouyer wrote:

Hello,
on NetBSD I'm tracking down an issue where xenstored never closes its
file descriptor to /var/run/xenstored/socket and instead loops at 100%
CPU on these descriptors.

xenstored loops because poll(2) returns a POLLIN event for these
descriptors but they are marked is_ignored = true.

I'm seeing this with xen 4.15, 4.13 and has also been reported with 4.11
with latest security patches.
It seems to have started with patches for XSA-115 (A user reported this
for 4.11)

I've tracked it down to a difference in poll(2) implementation; it seems
that linux will return something that is not (POLLIN|POLLOUT) when a
socket if closed; while NetBSD returns POLLIN (this matches the NetBSD's
man page).


Yeah, Linux seems to return POLLHUP additionally.


Overall, it seems that the poll(2) behavior with non-regular files
is highly OS-dependant when it comes to border cases (like a remote
close of a socket)





First I think there may be a security issue here, as, even on linux it should
be possible for a client to cause a socket to go to the is_ignored state,
while still sending data and cause xenstored to go to a 100% CPU loop.


No security issue, as sockets are restricted to dom0 and user root.

In case you are suspecting a security issue, please don't send such
mails to xen-devel in future, but to secur...@lists.xenproject.org.


Yes, sorry. Will do next time.




I think this is needed anyway:

--- xenstored_core.c.orig   2021-02-02 18:06:33.389316841 +0100
+++ xenstored_core.c2021-02-02 19:27:43.761877371 +0100
@@ -397,9 +397,12 @@
 !list_empty(>out_list)))
*ptimeout = 0;
} else {
-   short events = POLLIN|POLLPRI;
-   if (!list_empty(>out_list))
-   events |= POLLOUT;
+   short events = 0;
+   if (!conn->is_ignored) {
+   events |= POLLIN|POLLPRI;
+   if (!list_empty(>out_list))
+   events |= POLLOUT;
+   }
conn->pollfd_idx = set_fd(conn->fd, events);
}
}


Yes, I think this is a good idea.


Well, after some sleep I don't think it is. We should always keep at last
POLLIN or we will never notice a socket close otherwise.


Adding the fd of an ignored socket connection to the list is the real
problem here. Why should that be done?







Now I wonder if, on NetBSD at last, a read error or short read shouldn't
cause the socket to be closed, as with:

@@ -1561,6 +1565,8 @@
   bad_client:
ignore_connection(conn);
+   /* we don't want to keep this connection alive */
+   talloc_free(conn);
   }


This is wrong for non-socket connections, as we want to keep the domain
in question to be known to xenstored.

For socket connections this should be okay, though.


What are "non-socket connections" BTW ? I don't think I've seen one
in my test.


Every connection to another domain.


Is there a way to know if a connection is socket or non-socket ?


Active socket connections have conn->fd >= 0.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature