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

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

[Eric Shelton]

I am running Xen 4.6.3 (with Marek's recent patches at
https://lists.xen.org/archives/html/xen-devel/2016-10/msg01122.html)
and Linux 4.4.14.

qemu-traditional (running in a stubdom) exits due to an error when I
try doing passthrough of a PCI device that includes an
option/expansion ROM.  Specifically, the console log for the dm domain
shows:

= = = =
pcifront_watches: waiting for backend to get into the right state
/local/domain/0/backend/pci/19/0

******************* PCIFRONT for device/pci/0 **********


backend at /local/domain/0/backend/pci/19/0
**************************
pcifront_watches: waiting for backend events
/local/domain/0/backend/pci/19/0/state
xs_read_watch() -> device-model/18/command dm-command
dm-command: hot insert pass-through pci dev
warning: pt_iomul not supported in stubdom 05:00.0
ERROR: PCI region size must be pow2 type=0x8, size=0xdf080000
close(0)
= = = =

The size shown in the error message is actually the base address for
the expansion ROM:

05:00.0 Ethernet controller: Intel Corporation 82574L Gigabit Network Connection
    Subsystem: Intel Corporation Gigabit CT Desktop Adapter
    Flags: fast devsel, IRQ 19
    Memory at df0c0000 (32-bit, non-prefetchable) [disabled] [size=128K]
    Memory at df000000 (32-bit, non-prefetchable) [disabled] [size=512K]
    I/O ports at e000 [disabled] [size=32]
    Memory at df0e0000 (32-bit, non-prefetchable) [disabled] [size=16K]
    Expansion ROM at df080000 [disabled] [size=256K]

05:00.0 Ethernet controller: Intel Corporation 82574L Gigabit Network Connection
00: 86 80 d3 10 00 00 10 00 00 00 00 02 00 00 00 00
10: 00 00 0c df 00 00 00 df 01 e0 00 00 00 00 0e df
20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 1f a0
30: 00 00 08 df c8 00 00 00 00 00 00 00 0b 01 00 00

The expansion ROM size appears to be getting set by this portion of
stubdom/pciutils.patch:
= = = =
          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:
(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
(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.
(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.

Can someone provide comments on items 1 and 2 above?  In particular, I
am not sure what the right answer is for 1.

Thanks,
Eric

PS: While looking into this, it looks like the patch discussed at
https://lists.xen.org/archives/html/xen-devel/2014-05/msg01465.html
(Prevent QEMU from mapping PCI option ROM at address 0) for
qemu-traditional never ended up getting resolved.  It seems like
keeping anything - particularly a reflashable option ROM - from
getting mapped to the NULL page would be a good idea.

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

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

[Samuel Thibault]

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

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

[Jan Beulich]

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

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

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

[Eric Shelton]

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

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

[Jan Beulich]

>>> On 24.10.16 at 14:30, <eshelton@pobox.com> wrote:
> 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).

No - for sizing a ROM BAR, only the address portion is supposed to
be written with all 1s as per the PCI spec. In particular it should be
quite clear that writing the enable bit with 1 is a bad thing when the
address portion is also written with all 1s.

I'd like to note though that as of commit d2bd05d88d ("xen-pciback:
return proper values during BAR sizing") the handling there is more
relaxed - previously writes of 0xFFFFFFFE were the only ones
considered to be sizing ones.

Jan

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

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

[Eric Shelton]

On Mon, Oct 24, 2016 at 8:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 24.10.16 at 14:30, <eshelton@pobox.com> wrote:
>> 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).
>
> No - for sizing a ROM BAR, only the address portion is supposed to
> be written with all 1s as per the PCI spec. In particular it should be
> quite clear that writing the enable bit with 1 is a bad thing when the
> address portion is also written with all 1s.
>
> I'd like to note though that as of commit d2bd05d88d ("xen-pciback:
> return proper values during BAR sizing") the handling there is more
> relaxed - previously writes of 0xFFFFFFFE were the only ones
> considered to be sizing ones.
>
> Jan

Rev 2.2 of the PCI spec, at 6.2.5.2, states that "configuration
software can determine how much address space the device requires by
writing a value of all 1' to the address portion of the register."
Does the "address portion" refer to the upper 21 bits (meaning we
should write 0xFFFFF800) or the upper 31 bits (meaning we should write
0xFFFFFFFE)?  4.4.14 pciback code appears to expect the latter, and
with the newer more relaxed pciback code it doesn't matter.  For
purposes of fixing this bug in Xen, which do you consider to be the
"correct" sizing write value for QEMU to be writing out when it sizes
the expansion ROM?

Eric

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

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

[Jan Beulich]

>>> On 24.10.16 at 15:01, <eshelton@pobox.com> wrote:
> On Mon, Oct 24, 2016 at 8:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 24.10.16 at 14:30, <eshelton@pobox.com> wrote:
>>> 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).
>>
>> No - for sizing a ROM BAR, only the address portion is supposed to
>> be written with all 1s as per the PCI spec. In particular it should be
>> quite clear that writing the enable bit with 1 is a bad thing when the
>> address portion is also written with all 1s.
>>
>> I'd like to note though that as of commit d2bd05d88d ("xen-pciback:
>> return proper values during BAR sizing") the handling there is more
>> relaxed - previously writes of 0xFFFFFFFE were the only ones
>> considered to be sizing ones.
> 
> Rev 2.2 of the PCI spec, at 6.2.5.2, states that "configuration
> software can determine how much address space the device requires by
> writing a value of all 1' to the address portion of the register."
> Does the "address portion" refer to the upper 21 bits (meaning we
> should write 0xFFFFF800) or the upper 31 bits (meaning we should write
> 0xFFFFFFFE)?

Clearly the upper 21 bits.

>  4.4.14 pciback code appears to expect the latter, and
> with the newer more relaxed pciback code it doesn't matter.  For
> purposes of fixing this bug in Xen, which do you consider to be the
> "correct" sizing write value for QEMU to be writing out when it sizes
> the expansion ROM?

0xFFFFF800 | (<read-val> & 0x7FE)

Which then obviously won't work with older pciback (as much as ~0
didn't work, so that's no regression). It may be worth considering to
make another attempt with 0xFFFFFFFE if after the first write the
originally read value gets read back again.

And btw - is it really qemu that does this? I didn't think it would be
a user space app doing any such sizing; I thought this would get
done elsewhere in stubdom/mini-os.

Jan

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

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

[Samuel Thibault]

Jan Beulich, on Mon 24 Oct 2016 07:12:27 -0600, wrote:
> And btw - is it really qemu that does this?

It's pciaccess which does it.

> I didn't think it would be a user space app doing any such sizing; I
> thought this would get done elsewhere in stubdom/mini-os.

mini-os just lets libpciaccess use its generic support instead of
implementing its own PCI driver.

Samuel

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

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

[Eric Shelton]

On Mon, Oct 24, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 24.10.16 at 15:01, <eshelton@pobox.com> wrote:
>> On Mon, Oct 24, 2016 at 8:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 24.10.16 at 14:30, <eshelton@pobox.com> wrote:
>>>> 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).
>>>
>>> No - for sizing a ROM BAR, only the address portion is supposed to
>>> be written with all 1s as per the PCI spec. In particular it should be
>>> quite clear that writing the enable bit with 1 is a bad thing when the
>>> address portion is also written with all 1s.
>>>
>>> I'd like to note though that as of commit d2bd05d88d ("xen-pciback:
>>> return proper values during BAR sizing") the handling there is more
>>> relaxed - previously writes of 0xFFFFFFFE were the only ones
>>> considered to be sizing ones.
>>
>> Rev 2.2 of the PCI spec, at 6.2.5.2, states that "configuration
>> software can determine how much address space the device requires by
>> writing a value of all 1' to the address portion of the register."
>> Does the "address portion" refer to the upper 21 bits (meaning we
>> should write 0xFFFFF800) or the upper 31 bits (meaning we should write
>> 0xFFFFFFFE)?
>
> Clearly the upper 21 bits.

It's perhaps not all that clear, given that earlier pciback code
expected 0xFFFFFFFE.

>>  4.4.14 pciback code appears to expect the latter, and
>> with the newer more relaxed pciback code it doesn't matter.  For
>> purposes of fixing this bug in Xen, which do you consider to be the
>> "correct" sizing write value for QEMU to be writing out when it sizes
>> the expansion ROM?
>
> 0xFFFFF800 | (<read-val> & 0x7FE)

Excuse me for being a little dense, but I don't understand what is
going on with the "| (<read-val> & 0x7FE)" portion above.  Should the
fixed code be:

pci_write_long(d, reg, ~PCI_ROM_ADDRESS_MASK);

Or something else?

Thanks,
Eric

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

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

[Jan Beulich]

>>> On 24.10.16 at 15:56, <eshelton@pobox.com> wrote:
> On Mon, Oct 24, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 24.10.16 at 15:01, <eshelton@pobox.com> wrote:
>>> On Mon, Oct 24, 2016 at 8:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 24.10.16 at 14:30, <eshelton@pobox.com> wrote:
>>>>> 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).
>>>>
>>>> No - for sizing a ROM BAR, only the address portion is supposed to
>>>> be written with all 1s as per the PCI spec. In particular it should be
>>>> quite clear that writing the enable bit with 1 is a bad thing when the
>>>> address portion is also written with all 1s.
>>>>
>>>> I'd like to note though that as of commit d2bd05d88d ("xen-pciback:
>>>> return proper values during BAR sizing") the handling there is more
>>>> relaxed - previously writes of 0xFFFFFFFE were the only ones
>>>> considered to be sizing ones.
>>>
>>> Rev 2.2 of the PCI spec, at 6.2.5.2, states that "configuration
>>> software can determine how much address space the device requires by
>>> writing a value of all 1' to the address portion of the register."
>>> Does the "address portion" refer to the upper 21 bits (meaning we
>>> should write 0xFFFFF800) or the upper 31 bits (meaning we should write
>>> 0xFFFFFFFE)?
>>
>> Clearly the upper 21 bits.
> 
> It's perhaps not all that clear, given that earlier pciback code
> expected 0xFFFFFFFE.

Well, the "clearly" referred to the PCI spec.

>>>  4.4.14 pciback code appears to expect the latter, and
>>> with the newer more relaxed pciback code it doesn't matter.  For
>>> purposes of fixing this bug in Xen, which do you consider to be the
>>> "correct" sizing write value for QEMU to be writing out when it sizes
>>> the expansion ROM?
>>
>> 0xFFFFF800 | (<read-val> & 0x7FE)
> 
> Excuse me for being a little dense, but I don't understand what is
> going on with the "| (<read-val> & 0x7FE)" portion above.  Should the
> fixed code be:
> 
> pci_write_long(d, reg, ~PCI_ROM_ADDRESS_MASK);
> 
> Or something else?

You first read the BAR, mask out the enable bit, or in the address
mask, and write out the resulting value.

Jan

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