Xen devel - Sep 2013 - Multi-Function PCI passthrough not implemented?

Hi all,

There are several mentions of these features on the wiki
(http://wiki.xen.org/wiki/VTdHowTo,
http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation). However,
this is definitely not working and I could not see it implemented
anywhere in libxl.

I actually even think there is a bug in the code:

xlu_pci_parse_bdf in libxlu_pci.c accepts inputs of the form
"domain:bus:dev.*" (* is really a star here), which is supposed to
designate all the functions for this PCI device.
In this case, pcidev->func will be set to an uninitiated value by
pcidev_struct_fill().
Later on, libxl__device_pci_add() and libxl_pcidev_assignable()
(libxl_pci.c) are called with pcidev as an argument. And because
pcidev->func is garbage, an error is thrown.

From the user''s point of view, it looks like
this:
> xl pci-attach ovsVM0 ''0000:85:10.*''
libxl: error: libxl_pci.c:1058:libxl__device_pci_add: PCI device
0:85:10.40 is not assignable

(40 is the garbage hexadecimal value)

Antonin

>>> On 13.09.13 at 23:52, Antonin Bas <antoninb@cs.stanford.edu>
wrote:
> There are several mentions of these features on the wiki
> (http://wiki.xen.org/wiki/VTdHowTo,
> http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation). However,
> this is definitely not working and I could not see it implemented
> anywhere in libxl.
> 
> I actually even think there is a bug in the code:
> 
> xlu_pci_parse_bdf in libxlu_pci.c accepts inputs of the form
> "domain:bus:dev.*" (* is really a star here), which is supposed
to
> designate all the functions for this PCI device.
> In this case, pcidev->func will be set to an uninitiated value by
> pcidev_struct_fill().
> Later on, libxl__device_pci_add() and libxl_pcidev_assignable()
> (libxl_pci.c) are called with pcidev as an argument. And because
> pcidev->func is garbage, an error is thrown.
You not mentioning the Xen version I''d assume you talk about
-unstable, yet looking at the code I can''t match things up with
what you say above. In fact it looks to me as if multi-function
support was properly dealt with by libxl{u,}_pci.c...

Jan

Monday, September 16, 2013, 9:27:58 AM, you wrote:
>>>> On 13.09.13 at 23:52, Antonin Bas
<antoninb@cs.stanford.edu> wrote:
>> There are several mentions of these features on the wiki
>> (http://wiki.xen.org/wiki/VTdHowTo,
>> http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation). However,
>> this is definitely not working and I could not see it implemented
>> anywhere in libxl.
>> 
>> I actually even think there is a bug in the code:
>> 
>> xlu_pci_parse_bdf in libxlu_pci.c accepts inputs of the form
>> "domain:bus:dev.*" (* is really a star here), which is
supposed to
>> designate all the functions for this PCI device.
>> In this case, pcidev->func will be set to an uninitiated value by
>> pcidev_struct_fill().
>> Later on, libxl__device_pci_add() and libxl_pcidev_assignable()
>> (libxl_pci.c) are called with pcidev as an argument. And because
>> pcidev->func is garbage, an error is thrown.
> You not mentioning the Xen version I''d assume you talk about
> -unstable, yet looking at the code I can''t match things up with
> what you say above. In fact it looks to me as if multi-function
> support was properly dealt with by libxl{u,}_pci.c...
Hi Jan,

It is easy to trigger with unstable.

libxl__device_pci_add early on calls libxl_pcidev_assignable to verify the
device func is assignable,
but this function isn''t multifunction aware and tries to match the
non-existant function number used
to to the assignable list which only contains the individual reserved device and
function numbers.

So it returns false and xl gives an error like:
libxl: error: libxl_pci.c:1056:libxl__device_pci_add: PCI device 0:7:0.f0 is not
assignable


So libxl_pcidev_assignable should check if it''s multifunction is
specified, and if so check if all 8 func''s for this devices
are assignable.

--
Sander
> Jan

>>> On 16.09.13 at 14:11, Sander Eikelenboom
<linux@eikelenboom.it> wrote:
> Monday, September 16, 2013, 9:27:58 AM, you wrote:
> 
>>>>> On 13.09.13 at 23:52, Antonin Bas
<antoninb@cs.stanford.edu> wrote:
>>> There are several mentions of these features on the wiki
>>> (http://wiki.xen.org/wiki/VTdHowTo,
>>> http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation).
However,
>>> this is definitely not working and I could not see it implemented
>>> anywhere in libxl.
>>> 
>>> I actually even think there is a bug in the code:
>>> 
>>> xlu_pci_parse_bdf in libxlu_pci.c accepts inputs of the form
>>> "domain:bus:dev.*" (* is really a star here), which is
supposed to
>>> designate all the functions for this PCI device.
>>> In this case, pcidev->func will be set to an uninitiated value
by
>>> pcidev_struct_fill().
>>> Later on, libxl__device_pci_add() and libxl_pcidev_assignable()
>>> (libxl_pci.c) are called with pcidev as an argument. And because
>>> pcidev->func is garbage, an error is thrown.
> 
>> You not mentioning the Xen version I''d assume you talk about
>> -unstable, yet looking at the code I can''t match things up
with
>> what you say above. In fact it looks to me as if multi-function
>> support was properly dealt with by libxl{u,}_pci.c...
> 
> Hi Jan,
> 
> It is easy to trigger with unstable.
> 
> libxl__device_pci_add early on calls libxl_pcidev_assignable to verify the 
> device func is assignable,
> but this function isn''t multifunction aware and tries to match the
> non-existant function number used
> to to the assignable list which only contains the individual reserved
device
> and function numbers.
> 
> So it returns false and xl gives an error like:
> libxl: error: libxl_pci.c:1056:libxl__device_pci_add: PCI device 0:7:0.f0
is
> not assignable
> 
> 
> So libxl_pcidev_assignable should check if it''s multifunction is
specified,
> and if so check if all 8 func''s for this devices
> are assignable.
Ah, okay, I needed to check one call hierarchy level deeper...

Anyway - just wanted to get the situation confirmed, I''ll leave
fixing this to our tools experts anyway.

Jan

Monday, September 16, 2013, 2:32:52 PM, you wrote:
>>>> On 16.09.13 at 14:11, Sander Eikelenboom
<linux@eikelenboom.it> wrote:
>> Monday, September 16, 2013, 9:27:58 AM, you wrote:
>> 
>>>>>> On 13.09.13 at 23:52, Antonin Bas
<antoninb@cs.stanford.edu> wrote:
>>>> There are several mentions of these features on the wiki
>>>> (http://wiki.xen.org/wiki/VTdHowTo,
>>>> http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation).
However,
>>>> this is definitely not working and I could not see it
implemented
>>>> anywhere in libxl.
>>>> 
>>>> I actually even think there is a bug in the code:
>>>> 
>>>> xlu_pci_parse_bdf in libxlu_pci.c accepts inputs of the form
>>>> "domain:bus:dev.*" (* is really a star here), which
is supposed to
>>>> designate all the functions for this PCI device.
>>>> In this case, pcidev->func will be set to an uninitiated
value by
>>>> pcidev_struct_fill().
>>>> Later on, libxl__device_pci_add() and libxl_pcidev_assignable()
>>>> (libxl_pci.c) are called with pcidev as an argument. And
because
>>>> pcidev->func is garbage, an error is thrown.
>> 
>>> You not mentioning the Xen version I''d assume you talk
about
>>> -unstable, yet looking at the code I can''t match things up
with
>>> what you say above. In fact it looks to me as if multi-function
>>> support was properly dealt with by libxl{u,}_pci.c...
>> 
>> Hi Jan,
>> 
>> It is easy to trigger with unstable.
>> 
>> libxl__device_pci_add early on calls libxl_pcidev_assignable to verify
the
>> device func is assignable,
>> but this function isn''t multifunction aware and tries to match
the
>> non-existant function number used
>> to to the assignable list which only contains the individual reserved
device
>> and function numbers.
>> 
>> So it returns false and xl gives an error like:
>> libxl: error: libxl_pci.c:1056:libxl__device_pci_add: PCI device
0:7:0.f0 is
>> not assignable
>> 
>> 
>> So libxl_pcidev_assignable should check if it''s multifunction
is specified,
>> and if so check if all 8 func''s for this devices
>> are assignable.
> Ah, okay, I needed to check one call hierarchy level deeper...
> Anyway - just wanted to get the situation confirmed, I''ll leave
> fixing this to our tools experts anyway.
CC''ed IanC and Stefano. (resend because i accidentally removed the old
CC''s)

I tried to see how far i could come with outcommenting the safety checks in
libxl_pcidev_assignable.

It works with qemu-xen-traditional when i specify a vslot for the multifunction
pcidev (f.e. pci="00:03:00.*@7"),
but i can''t see why it is necessary to specify it by hand (unless one
would like it to end up at a particular slot for some reason) ?
Or is there no other way to "group" the individual functions in the
qmp syntax and have them end up on the same vdev ?

With qem-xen-(upstream) this fails with a qmp error since it doesn''t
seem to have no knowledge of the "addr" parameter that gets added by
specifying the vslot.

--
Sander

> Jan

What would be really useful is that the BDF notation extension
described at http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation
actually be supported.
Just supporting the ''*'' notation (with or without giving a
vslot) is
not entirely satisfying. People may want to assign some funcs (e.g.
0-3) to one vslot and the others (4-7) to another vslot, like is
possible with the libvirt multifunction=''on'' attribute (since
libvirt
0.9.7, with QEMU > 0.1.3). The ''*'' notation should also
handle the
case where the device has less than 8 funcs and assign the existing
funcs without crashing.

Best,

Antonin

2013/9/16 Sander Eikelenboom
<linux@eikelenboom.it>:
>
> Monday, September 16, 2013, 2:32:52 PM, you wrote:
>
>>>>> On 16.09.13 at 14:11, Sander Eikelenboom
<linux@eikelenboom.it> wrote:
>
>>> Monday, September 16, 2013, 9:27:58 AM, you wrote:
>>>
>>>>>>> On 13.09.13 at 23:52, Antonin Bas
<antoninb@cs.stanford.edu> wrote:
>>>>> There are several mentions of these features on the wiki
>>>>> (http://wiki.xen.org/wiki/VTdHowTo,
>>>>>
http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation). However,
>>>>> this is definitely not working and I could not see it
implemented
>>>>> anywhere in libxl.
>>>>>
>>>>> I actually even think there is a bug in the code:
>>>>>
>>>>> xlu_pci_parse_bdf in libxlu_pci.c accepts inputs of the
form
>>>>> "domain:bus:dev.*" (* is really a star here),
which is supposed to
>>>>> designate all the functions for this PCI device.
>>>>> In this case, pcidev->func will be set to an uninitiated
value by
>>>>> pcidev_struct_fill().
>>>>> Later on, libxl__device_pci_add() and
libxl_pcidev_assignable()
>>>>> (libxl_pci.c) are called with pcidev as an argument. And
because
>>>>> pcidev->func is garbage, an error is thrown.
>>>
>>>> You not mentioning the Xen version I''d assume you talk
about
>>>> -unstable, yet looking at the code I can''t match
things up with
>>>> what you say above. In fact it looks to me as if multi-function
>>>> support was properly dealt with by libxl{u,}_pci.c...
>>>
>>> Hi Jan,
>>>
>>> It is easy to trigger with unstable.
>>>
>>> libxl__device_pci_add early on calls libxl_pcidev_assignable to
verify the
>>> device func is assignable,
>>> but this function isn''t multifunction aware and tries to
match the
>>> non-existant function number used
>>> to to the assignable list which only contains the individual
reserved device
>>> and function numbers.
>>>
>>> So it returns false and xl gives an error like:
>>> libxl: error: libxl_pci.c:1056:libxl__device_pci_add: PCI device
0:7:0.f0 is
>>> not assignable
>>>
>>>
>>> So libxl_pcidev_assignable should check if it''s
multifunction is specified,
>>> and if so check if all 8 func''s for this devices
>>> are assignable.
>
>> Ah, okay, I needed to check one call hierarchy level deeper...
>
>> Anyway - just wanted to get the situation confirmed, I''ll
leave
>> fixing this to our tools experts anyway.
>
> CC''ed IanC and Stefano. (resend because i accidentally removed the
old CC''s)
>
> I tried to see how far i could come with outcommenting the safety checks in
libxl_pcidev_assignable.
>
> It works with qemu-xen-traditional when i specify a vslot for the
multifunction pcidev (f.e. pci="00:03:00.*@7"),
> but i can''t see why it is necessary to specify it by hand (unless
one would like it to end up at a particular slot for some reason) ?
> Or is there no other way to "group" the individual functions in
the qmp syntax and have them end up on the same vdev ?
>
> With qem-xen-(upstream) this fails with a qmp error since it
doesn''t seem to have no knowledge of the "addr" parameter
that gets added by specifying the vslot.
>
> --
> Sander
>
>
>> Jan
>
>

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Antonin Bas
> Sent: Wednesday, September 18, 2013 1:55 AM
> To: Sander Eikelenboom
> Cc: xen-devel@lists.xen.org; Antonin Bas; Ian Campbell; Jan Beulich;
Stefano
> Stabellini
> Subject: Re: [Xen-devel] Multi-Function PCI passthrough not implemented?
> 
> What would be really useful is that the BDF notation extension
> described at http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation
> actually be supported.
> Just supporting the ''*'' notation (with or without giving
a vslot) is
> not entirely satisfying. People may want to assign some funcs (e.g.
> 0-3) to one vslot and the others (4-7) to another vslot, like is
> possible with the libvirt multifunction=''on'' attribute
(since libvirt
> 0.9.7, with QEMU > 0.1.3). The ''*'' notation should
also handle the
> case where the device has less than 8 funcs and assign the existing
> funcs without crashing.
> 
Seems the current implementation of xl only supports "*" notation, the
following
notations mentioned at
http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation
are not supported:

0000:00:1d.0-2
0000:00:1d.0,3,5,7
0000:00:1d.2=0-0=2 
0000:00:1d.0=3,3=2,5=1,7=0

> Best,
> 
> Antonin
> 
> 2013/9/16 Sander Eikelenboom <linux@eikelenboom.it>:
> >
> > Monday, September 16, 2013, 2:32:52 PM, you wrote:
> >
> >>>>> On 16.09.13 at 14:11, Sander Eikelenboom
<linux@eikelenboom.it>
> wrote:
> >
> >>> Monday, September 16, 2013, 9:27:58 AM, you wrote:
> >>>
> >>>>>>> On 13.09.13 at 23:52, Antonin Bas
<antoninb@cs.stanford.edu>
> wrote:
> >>>>> There are several mentions of these features on the
wiki
> >>>>> (http://wiki.xen.org/wiki/VTdHowTo,
> >>>>>
http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation).
> However,
> >>>>> this is definitely not working and I could not see it
implemented
> >>>>> anywhere in libxl.
> >>>>>
> >>>>> I actually even think there is a bug in the code:
> >>>>>
> >>>>> xlu_pci_parse_bdf in libxlu_pci.c accepts inputs of
the form
> >>>>> "domain:bus:dev.*" (* is really a star
here), which is supposed to
> >>>>> designate all the functions for this PCI device.
> >>>>> In this case, pcidev->func will be set to an
uninitiated value by
> >>>>> pcidev_struct_fill().
> >>>>> Later on, libxl__device_pci_add() and
libxl_pcidev_assignable()
> >>>>> (libxl_pci.c) are called with pcidev as an argument.
And because
> >>>>> pcidev->func is garbage, an error is thrown.
> >>>
> >>>> You not mentioning the Xen version I''d assume you
talk about
> >>>> -unstable, yet looking at the code I can''t match
things up with
> >>>> what you say above. In fact it looks to me as if
multi-function
> >>>> support was properly dealt with by libxl{u,}_pci.c...
> >>>
> >>> Hi Jan,
> >>>
> >>> It is easy to trigger with unstable.
> >>>
> >>> libxl__device_pci_add early on calls libxl_pcidev_assignable
to verify the
> >>> device func is assignable,
> >>> but this function isn''t multifunction aware and tries
to match the
> >>> non-existant function number used
> >>> to to the assignable list which only contains the individual
reserved device
> >>> and function numbers.
> >>>
> >>> So it returns false and xl gives an error like:
> >>> libxl: error: libxl_pci.c:1056:libxl__device_pci_add: PCI
device 0:7:0.f0 is
> >>> not assignable
> >>>
> >>>
> >>> So libxl_pcidev_assignable should check if it''s
multifunction is specified,
> >>> and if so check if all 8 func''s for this devices
> >>> are assignable.
> >
> >> Ah, okay, I needed to check one call hierarchy level deeper...
> >
> >> Anyway - just wanted to get the situation confirmed, I''ll
leave
> >> fixing this to our tools experts anyway.
> >
> > CC''ed IanC and Stefano. (resend because i accidentally
removed the old CC''s)
> >
> > I tried to see how far i could come with outcommenting the safety
checks in
> libxl_pcidev_assignable.
> >
> > It works with qemu-xen-traditional when i specify a vslot for the
multifunction
> pcidev (f.e. pci="00:03:00.*@7"),
> > but i can''t see why it is necessary to specify it by hand
(unless one would like it
> to end up at a particular slot for some reason) ?
> > Or is there no other way to "group" the individual functions
in the qmp syntax
> and have them end up on the same vdev ?
> >
> > With qem-xen-(upstream) this fails with a qmp error since it
doesn''t seem to
> have no knowledge of the "addr" parameter that gets added by
specifying the
> vslot.
> >
> > --
> > Sander
> >
> >
> >> Jan
> >
> >
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Thanks,
Feng

>>> On 18.09.13 at 16:03, "Wu, Feng"
<feng.wu@intel.com> wrote:
> Seems the current implementation of xl only supports "*"
notation, the
> following
> notations mentioned at 
> http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation 
> are not supported:
> 
> 0000:00:1d.0-2
> 0000:00:1d.0,3,5,7
> 0000:00:1d.2=0-0=2 
> 0000:00:1d.0=3,3=2,5=1,7=0
Altering function numbers, especially to or from zero, is clearly not
generally valid: There are devices where the multi-function bits is
set only for function zero, and hence any other function being made
(virtual) function zero would result in all other functions not being
found during the guest OSes bus scan.

Jan

2013/9/18 Jan Beulich <JBeulich@suse.com>:
>>>> On 18.09.13 at 16:03, "Wu, Feng"
<feng.wu@intel.com> wrote:
>> Seems the current implementation of xl only supports "*"
notation, the
>> following
>> notations mentioned at
>> http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation
>> are not supported:
>>
>> 0000:00:1d.0-2
>> 0000:00:1d.0,3,5,7
>> 0000:00:1d.2=0-0=2
>> 0000:00:1d.0=3,3=2,5=1,7=0
>
> Altering function numbers, especially to or from zero, is clearly not
> generally valid: There are devices where the multi-function bits is
> set only for function zero, and hence any other function being made
> (virtual) function zero would result in all other functions not being
> found during the guest OSes bus scan.
I am pretty sure I had it working in KVM with the following config:

<interface type=''hostdev''>
      <source>
        <address type=''pci''
domain=''0x0000'' bus=''0x06''
slot=''0x10''
function=''0x0''/>
      </source>
      <address type=''pci'' domain=''0x0000''
bus=''0x00'' slot=''0x05''
function=''0x0'' multifunction=''on''/>
    </interface>
    <interface type=''hostdev''>
      <source>
        <address type=''pci''
domain=''0x0000'' bus=''0x06''
slot=''0x10''
function=''0x1''/>
      </source>
      <address type=''pci'' domain=''0x0000''
bus=''0x00'' slot=''0x05''
function=''0x1''/>
    </interface>
    <interface type=''hostdev''>
      <source>
        <address type=''pci''
domain=''0x0000'' bus=''0x06''
slot=''0x10''
function=''0x2''/>
      </source>
      <address type=''pci'' domain=''0x0000''
bus=''0x00'' slot=''0x06''
function=''0x0'' multifunction=''on''/>
    </interface>
    <interface type=''hostdev''>
      <source>
        <address type=''pci''
domain=''0x0000'' bus=''0x06''
slot=''0x10''
function=''0x3''/>
      </source>
      <address type=''pci'' domain=''0x0000''
bus=''0x00'' slot=''0x06''
function=''0x1''/>
 </interface>

So this was working with SR-IOV devices.
But maybe you''re right, and it could fail with other PCI devices.
>
> Jan
>
Antonin

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, September 18, 2013 10:08 PM
> To: Wu, Feng
> Cc: Ian Campbell; Antonin Bas; Sander Eikelenboom; StefanoStabellini;
> xen-devel@lists.xen.org
> Subject: RE: [Xen-devel] Multi-Function PCI passthrough not implemented?
> 
> >>> On 18.09.13 at 16:03, "Wu, Feng"
<feng.wu@intel.com> wrote:
> > Seems the current implementation of xl only supports "*"
notation, the
> > following
> > notations mentioned at
> > http://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation
> > are not supported:
> >
> > 0000:00:1d.0-2
> > 0000:00:1d.0,3,5,7
> > 0000:00:1d.2=0-0=2
> > 0000:00:1d.0=3,3=2,5=1,7=0
> 
> Altering function numbers, especially to or from zero, is clearly not
> generally valid: There are devices where the multi-function bits is
> set only for function zero, and hence any other function being made
> (virtual) function zero would result in all other functions not being
> found during the guest OSes bus scan.
We can virtualize the multi-function bit for guests for the multi-function
device. So no matter altering function numbers to or from zero, it is not
a problem for the guest to find the devices when doing the bus scan.
> 
> Jan
Thanks,
Feng