Re: PCI passthrough to QEMU traditional stubdom not working when option ROM present

[Eric Shelton <eshelton@pobox.com>]

On Mon, Oct 24, 2016 at 6:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 21.10.16 at 15:23, <samuel.thibault@ens-lyon.org> 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

However, the changes we discussed would not change what is being
written to config 0x30 - they simply change how the value read back is
being processed.

As best as I can tell, the current code already writes out the proper
value for sizing ("pci_write_long(d, reg, ~0)" in the above code
snippet).  The value being read back (in the "d->rom_size =
pci_read_long(d, reg)" in the following line) suggests the sizing
write is not having the intended effect, since the base address
(0xdf080000) is being read back after writing 0xffffffff.  In no
scenario that the sizing write is being handled correctly should the
top nibble be 'd', and, as I mentioned below, I would have expected
the first 14 bits to be set (a value of 0xfffc0000 or 0xfffc0001,
depending on how pciback emulates the lowest address decode enable
bit).

Eric

>>> (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@lists.xen.org
https://lists.xen.org/xen-devel