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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.10.16 at 15:23, <samuel.thibault@xxxxxxxxxxxx> wrote:
> 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.

No - iirc pciback simply disallows bogus writes (but not proper ones
used for sizing), i.e. with (2) addressed this one should disappear
too.

Jan

>> (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