Re: [Xen-devel] PCI passthrough to QEMU traditional stubdom not working when option ROM present

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Hello,

Eric Shelton, on Fri 21 Oct 2016 09:01:43 -0400, wrote:
> ERROR: PCI region size must be pow2 type=0x8, size=0xdf080000

>           u32 u = pci_read_long(d, reg);
>           if (u != 0xffffffff)
> -           d->rom_base_addr = u;
> +            {
> +              d->rom_base_addr = u;
> +              if (flags & PCI_FILL_SIZES)
> +                {
> +                  u32 size;
> +                  pci_write_long(d, reg, ~0);
> +                  d->rom_size = pci_read_long(d, reg);
> +                  pci_write_long(d, reg, u);
> +                }
> +            }
> = = = =
> 
> It looks like there are a few issues going on with this:

Indeed :) I have to say that 8 years have made me forget about the code
:)

> (1) The expansion ROM BAR at 0x30 appears to be read only, so the
> write of ~0 to determine its size is not working.  As a result,
> d->rom_size is getting set to the base address for the expansion ROM.
> I assume 0x30 being read only is a pciback issue, but I don't know if
> changes after 4.4.14 have affected this - I see there have been
> changes to rom_write() and rom_init() in conf_space_header.c

I don't know about this. Probably an issue in pciback indeed.

> (2) Even if that write issue wasn't happening, the above patch does
> not look like the right way to determine the size of the expansion ROM
> anyway.  For example, with the example device above having a 256K
> expansion ROM, I believe a write of 0xffffffff to 0x30 would result in
> a value of 0xfffc0001 (the lowest bit is an address decode
> enable/disable), which we would not want to store in d->rom_size.
> Instead, something like:
>   d->rom_size = pci_size(u, pci_read_long(d, reg), PCI_ROM_ADDRESS_MASK);
> probably should be done instead, in the same way the other BARs are being 
> sized.

Completely agree. I guess the rom_base_addr field should also have a
& PCI_ROM_ADDRESS_MASK like the other base_addr fields in the patch.

Also, that part is not specific to mini-os. It'd be good to submit it
upstream :)

> (3) (minor) When QEMU errors out, it takes a while for xl to time out
> on it.  Perhaps it would make sense for QEMU to set something in
> xenstore on its way out to let xl know it has errored out.

Mmm, I guess the stubdom itself crashes? I'd say xl should be watching
for the stubdom being alive, and perhaps abort the domain if the stubdom
crashed?

Samuel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel