xl/SR-IOV: disposition of VFs when PF disappears?

xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

All,

Intel reports that the sequence

- xl pci-assignable-add <VF>
- briefly run guest using that device [not sure whether that's really a
  necessary step]
- xl pci-assignable-add <PF of VF>

results in both VF and PF being listed as assignable (the fact that as a
result the PF handed to a guest doesn't work is secondary here, as I
think this is a driver issue). Is that really how it should be? Shouldn't
instead all VFs get removed when the PF device (e.g. due to the
PF driver getting unloaded, which is a necessary part of making it
assignable) goes away? Or is it required for the admin to manually
remove the assignable VFs prior to making the PF go away?

Thanks, Jan

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Ian Campbell]

On Mon, 2014-10-27 at 12:36 +0000, Jan Beulich wrote:
> All,
> 
> Intel reports that the sequence
> 
> - xl pci-assignable-add <VF>
> - briefly run guest using that device [not sure whether that's really a
>   necessary step]
> - xl pci-assignable-add <PF of VF>
> 
> results in both VF and PF being listed as assignable (the fact that as a
> result the PF handed to a guest doesn't work is secondary here, as I
> think this is a driver issue). Is that really how it should be? Shouldn't
> instead all VFs get removed when the PF device (e.g. due to the
> PF driver getting unloaded, which is a necessary part of making it
> assignable) goes away? Or is it required for the admin to manually
> remove the assignable VFs prior to making the PF go away?

xl is just controlling/exposing the set of devices which are bound to
pciback here. (pci-assignable-list is literally a readdir loop over the
relevant sysfs dir).

I'm not sure if it should be up to (lib)xl, pciback or the core Linux
pci stuff to handle the creation/destruction of VF devices when the PF
driver is unbound/assigned. In fact I'm not even sure if VF lifetime is
in any way tied to the PF driver state.

I've added Konrad for a kernel-size pciback perspective.

Ian.

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

>>> On 27.10.14 at 13:57, <Ian.Campbell@eu.citrix.com> wrote:
> On Mon, 2014-10-27 at 12:36 +0000, Jan Beulich wrote:
>> All,
>> 
>> Intel reports that the sequence
>> 
>> - xl pci-assignable-add <VF>
>> - briefly run guest using that device [not sure whether that's really a
>>   necessary step]
>> - xl pci-assignable-add <PF of VF>
>> 
>> results in both VF and PF being listed as assignable (the fact that as a
>> result the PF handed to a guest doesn't work is secondary here, as I
>> think this is a driver issue). Is that really how it should be? Shouldn't
>> instead all VFs get removed when the PF device (e.g. due to the
>> PF driver getting unloaded, which is a necessary part of making it
>> assignable) goes away? Or is it required for the admin to manually
>> remove the assignable VFs prior to making the PF go away?
> 
> xl is just controlling/exposing the set of devices which are bound to
> pciback here. (pci-assignable-list is literally a readdir loop over the
> relevant sysfs dir).

Ah, good to know. In that case yes, pciback ought to be honoring
device removal.

> I'm not sure if it should be up to (lib)xl, pciback or the core Linux
> pci stuff to handle the creation/destruction of VF devices when the PF
> driver is unbound/assigned. In fact I'm not even sure if VF lifetime is
> in any way tied to the PF driver state.

Yes, it is (at least in practice on the NICs I've seen, but iirc the spec
also says so) - VFs won't work without a PF driver in place.

Jan

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Konrad Rzeszutek Wilk]

On Mon, Oct 27, 2014 at 12:57:46PM +0000, Ian Campbell wrote:
> On Mon, 2014-10-27 at 12:36 +0000, Jan Beulich wrote:
> > All,
> > 
> > Intel reports that the sequence
> > 
> > - xl pci-assignable-add <VF>
> > - briefly run guest using that device [not sure whether that's really a
> >   necessary step]
> > - xl pci-assignable-add <PF of VF>
> > 
> > results in both VF and PF being listed as assignable (the fact that as a
> > result the PF handed to a guest doesn't work is secondary here, as I
> > think this is a driver issue). Is that really how it should be? Shouldn't
> > instead all VFs get removed when the PF device (e.g. due to the
> > PF driver getting unloaded, which is a necessary part of making it
> > assignable) goes away? Or is it required for the admin to manually
> > remove the assignable VFs prior to making the PF go away?

I am not sure I see the problem. If the user wishes to give the PF and
VF to a guest they should be able to do so?

> 
> xl is just controlling/exposing the set of devices which are bound to
> pciback here. (pci-assignable-list is literally a readdir loop over the
> relevant sysfs dir).
> 
> I'm not sure if it should be up to (lib)xl, pciback or the core Linux
> pci stuff to handle the creation/destruction of VF devices when the PF
> driver is unbound/assigned. In fact I'm not even sure if VF lifetime is
> in any way tied to the PF driver state.

It is. When we detect that the device is a VF we set some flag so that the
PF won't try to de-allocate the VFs.

> 
> I've added Konrad for a kernel-size pciback perspective.
> 
> Ian.
> 

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Anirban Chakraborty]

On 10/27/14, 6:35 AM, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
wrote:

>On Mon, Oct 27, 2014 at 12:57:46PM +0000, Ian Campbell wrote:
>> On Mon, 2014-10-27 at 12:36 +0000, Jan Beulich wrote:
>> > All,
>> > 
>> > Intel reports that the sequence
>> > 
>> > - xl pci-assignable-add <VF>
>> > - briefly run guest using that device [not sure whether that's really
>>a
>> >   necessary step]
>> > - xl pci-assignable-add <PF of VF>
>> > 
>> > results in both VF and PF being listed as assignable (the fact that
>>as a
>> > result the PF handed to a guest doesn't work is secondary here, as I
>> > think this is a driver issue). Is that really how it should be?
>>Shouldn't
>> > instead all VFs get removed when the PF device (e.g. due to the
>> > PF driver getting unloaded, which is a necessary part of making it
>> > assignable) goes away? Or is it required for the admin to manually
>> > remove the assignable VFs prior to making the PF go away?
>
>I am not sure I see the problem. If the user wishes to give the PF and
>VF to a guest they should be able to do so?

Theoretically, yes a guest can have a PF and all its VFs. However, from
security perspective PF having the privilege of resetting the device etc.,
should stay in a privileged domain. Most of the NICs have some sort of
PF-VF communication where the PF driver would ensure that VF drivers are
notified of imminent PF removal so that the VF drivers can prepare for a
graceful halt of IO. Ideally, a PF removal should do a hot unplug of the
VFs from the guests and admin should not have to manually remove them.

Anirban
 
>
>> 
>> xl is just controlling/exposing the set of devices which are bound to
>> pciback here. (pci-assignable-list is literally a readdir loop over the
>> relevant sysfs dir).
>> 
>> I'm not sure if it should be up to (lib)xl, pciback or the core Linux
>> pci stuff to handle the creation/destruction of VF devices when the PF
>> driver is unbound/assigned. In fact I'm not even sure if VF lifetime is
>> in any way tied to the PF driver state.
>
>It is. When we detect that the device is a VF we set some flag so that the
>PF won't try to de-allocate the VFs.
>
>> 
>> I've added Konrad for a kernel-size pciback perspective.
>> 
>> Ian.
>> 
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xen.org
>http://lists.xen.org/xen-devel

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Stefano Stabellini]

On Mon, 27 Oct 2014, Anirban Chakraborty wrote:
> On 10/27/14, 6:35 AM, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
> wrote:
> 
> >On Mon, Oct 27, 2014 at 12:57:46PM +0000, Ian Campbell wrote:
> >> On Mon, 2014-10-27 at 12:36 +0000, Jan Beulich wrote:
> >> > All,
> >> > 
> >> > Intel reports that the sequence
> >> > 
> >> > - xl pci-assignable-add <VF>
> >> > - briefly run guest using that device [not sure whether that's really
> >>a
> >> >   necessary step]
> >> > - xl pci-assignable-add <PF of VF>
> >> > 
> >> > results in both VF and PF being listed as assignable (the fact that
> >>as a
> >> > result the PF handed to a guest doesn't work is secondary here, as I
> >> > think this is a driver issue). Is that really how it should be?
> >>Shouldn't
> >> > instead all VFs get removed when the PF device (e.g. due to the
> >> > PF driver getting unloaded, which is a necessary part of making it
> >> > assignable) goes away? Or is it required for the admin to manually
> >> > remove the assignable VFs prior to making the PF go away?
> >
> >I am not sure I see the problem. If the user wishes to give the PF and
> >VF to a guest they should be able to do so?
> 
> Theoretically, yes a guest can have a PF and all its VFs. However, from
> security perspective PF having the privilege of resetting the device etc.,
> should stay in a privileged domain. Most of the NICs have some sort of
> PF-VF communication where the PF driver would ensure that VF drivers are
> notified of imminent PF removal so that the VF drivers can prepare for a
> graceful halt of IO. Ideally, a PF removal should do a hot unplug of the
> VFs from the guests and admin should not have to manually remove them.

Interesting.
At the same time I don't think we should prevent the administrator from
assigning a PF to a VM if she really wants to. Maybe xl should warn the
user that assigning a PF make the VF usage insecure.



> >> xl is just controlling/exposing the set of devices which are bound to
> >> pciback here. (pci-assignable-list is literally a readdir loop over the
> >> relevant sysfs dir).
> >> 
> >> I'm not sure if it should be up to (lib)xl, pciback or the core Linux
> >> pci stuff to handle the creation/destruction of VF devices when the PF
> >> driver is unbound/assigned. In fact I'm not even sure if VF lifetime is
> >> in any way tied to the PF driver state.
> >
> >It is. When we detect that the device is a VF we set some flag so that the
> >PF won't try to de-allocate the VFs.
> >
> >> 
> >> I've added Konrad for a kernel-size pciback perspective.
> >> 
> >> Ian.

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Konrad Rzeszutek Wilk]

On Mon, Oct 27, 2014 at 05:53:01PM +0000, Anirban Chakraborty wrote:
> 
> 
> On 10/27/14, 6:35 AM, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
> wrote:
> 
> >On Mon, Oct 27, 2014 at 12:57:46PM +0000, Ian Campbell wrote:
> >> On Mon, 2014-10-27 at 12:36 +0000, Jan Beulich wrote:
> >> > All,
> >> > 
> >> > Intel reports that the sequence
> >> > 
> >> > - xl pci-assignable-add <VF>
> >> > - briefly run guest using that device [not sure whether that's really
> >>a
> >> >   necessary step]
> >> > - xl pci-assignable-add <PF of VF>
> >> > 
> >> > results in both VF and PF being listed as assignable (the fact that
> >>as a
> >> > result the PF handed to a guest doesn't work is secondary here, as I
> >> > think this is a driver issue). Is that really how it should be?
> >>Shouldn't
> >> > instead all VFs get removed when the PF device (e.g. due to the
> >> > PF driver getting unloaded, which is a necessary part of making it
> >> > assignable) goes away? Or is it required for the admin to manually
> >> > remove the assignable VFs prior to making the PF go away?
> >
> >I am not sure I see the problem. If the user wishes to give the PF and
> >VF to a guest they should be able to do so?
> 
> Theoretically, yes a guest can have a PF and all its VFs. However, from
> security perspective PF having the privilege of resetting the device etc.,
> should stay in a privileged domain. Most of the NICs have some sort of
> PF-VF communication where the PF driver would ensure that VF drivers are
> notified of imminent PF removal so that the VF drivers can prepare for a
> graceful halt of IO. Ideally, a PF removal should do a hot unplug of the
> VFs from the guests and admin should not have to manually remove them.

We seem to be talking about two different things.

1) Assigning a PF and VF to a guest. While it is stupid it should be
   be possible. We could add an warning to the 'xl pci-assign' command
   if somebody does that, but it should be possible.

2). PF removal. Currently if you try to unload the PF and the VFs
   are in use (pciback owns them), the unloading will not happen. Until
   all of the VFs have been de-assigned.
   Is the "bug" here that the reporter (Intel?) wants the VFs to be
   automatically yanked out of a guest when the system admin wants to
   unload the PF?

> 
> Anirban
>  
> >
> >> 
> >> xl is just controlling/exposing the set of devices which are bound to
> >> pciback here. (pci-assignable-list is literally a readdir loop over the
> >> relevant sysfs dir).
> >> 
> >> I'm not sure if it should be up to (lib)xl, pciback or the core Linux
> >> pci stuff to handle the creation/destruction of VF devices when the PF
> >> driver is unbound/assigned. In fact I'm not even sure if VF lifetime is
> >> in any way tied to the PF driver state.
> >
> >It is. When we detect that the device is a VF we set some flag so that the
> >PF won't try to de-allocate the VFs.
> >
> >> 
> >> I've added Konrad for a kernel-size pciback perspective.
> >> 
> >> Ian.
> >> 
> >
> >_______________________________________________
> >Xen-devel mailing list
> >Xen-devel@lists.xen.org
> >http://lists.xen.org/xen-devel
> 

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Anirban Chakraborty]

On 10/27/14, 11:07 AM, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
wrote:

>On Mon, Oct 27, 2014 at 05:53:01PM +0000, Anirban Chakraborty wrote:
>> 
>> 
>> On 10/27/14, 6:35 AM, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
>> wrote:
>> 
>> >On Mon, Oct 27, 2014 at 12:57:46PM +0000, Ian Campbell wrote:
>> >> On Mon, 2014-10-27 at 12:36 +0000, Jan Beulich wrote:
>> >> > All,
>> >> > 
>> >> > Intel reports that the sequence
>> >> > 
>> >> > - xl pci-assignable-add <VF>
>> >> > - briefly run guest using that device [not sure whether that's
>>really
>> >>a
>> >> >   necessary step]
>> >> > - xl pci-assignable-add <PF of VF>
>> >> > 
>> >> > results in both VF and PF being listed as assignable (the fact that
>> >>as a
>> >> > result the PF handed to a guest doesn't work is secondary here, as
>>I
>> >> > think this is a driver issue). Is that really how it should be?
>> >>Shouldn't
>> >> > instead all VFs get removed when the PF device (e.g. due to the
>> >> > PF driver getting unloaded, which is a necessary part of making it
>> >> > assignable) goes away? Or is it required for the admin to manually
>> >> > remove the assignable VFs prior to making the PF go away?
>> >
>> >I am not sure I see the problem. If the user wishes to give the PF and
>> >VF to a guest they should be able to do so?
>> 
>> Theoretically, yes a guest can have a PF and all its VFs. However, from
>> security perspective PF having the privilege of resetting the device
>>etc.,
>> should stay in a privileged domain. Most of the NICs have some sort of
>> PF-VF communication where the PF driver would ensure that VF drivers are
>> notified of imminent PF removal so that the VF drivers can prepare for a
>> graceful halt of IO. Ideally, a PF removal should do a hot unplug of the
>> VFs from the guests and admin should not have to manually remove them.
>
>We seem to be talking about two different things.
>
>1) Assigning a PF and VF to a guest. While it is stupid it should be
>   be possible. We could add an warning to the 'xl pci-assign' command
>   if somebody does that, but it should be possible.

Agreed.

>
>2). PF removal. Currently if you try to unload the PF and the VFs
>   are in use (pciback owns them), the unloading will not happen. Until
>   all of the VFs have been de-assigned.
>   Is the "bug" here that the reporter (Intel?) wants the VFs to be
>   automatically yanked out of a guest when the system admin wants to
>   unload the PF?
>
>> 
>> Anirban
>>  
>> >
>> >> 
>> >> xl is just controlling/exposing the set of devices which are bound to
>> >> pciback here. (pci-assignable-list is literally a readdir loop over
>>the
>> >> relevant sysfs dir).
>> >> 
>> >> I'm not sure if it should be up to (lib)xl, pciback or the core Linux
>> >> pci stuff to handle the creation/destruction of VF devices when the
>>PF
>> >> driver is unbound/assigned. In fact I'm not even sure if VF lifetime
>>is
>> >> in any way tied to the PF driver state.
>> >
>> >It is. When we detect that the device is a VF we set some flag so that
>>the
>> >PF won't try to de-allocate the VFs.
>> >
>> >> 
>> >> I've added Konrad for a kernel-size pciback perspective.
>> >> 
>> >> Ian.
>> >> 
>> >
>> >_______________________________________________
>> >Xen-devel mailing list
>> >Xen-devel@lists.xen.org
>> >http://lists.xen.org/xen-devel
>> 

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

>>> On 27.10.14 at 19:07, <konrad.wilk@oracle.com> wrote:
> 2). PF removal. Currently if you try to unload the PF and the VFs
>    are in use (pciback owns them), the unloading will not happen. Until
>    all of the VFs have been de-assigned.
>    Is the "bug" here that the reporter (Intel?) wants the VFs to be
>    automatically yanked out of a guest when the system admin wants to
>    unload the PF?

No. They see the VF still listed as assignable after the guest terminated
and the PF got made assignable. The PF driver unloading, however,
should trigger all VFs (which are about to disappear) to get removed
from pciback. I hope to find some time later today to look into why (if
at all) this isn't happening right now.

Jan

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Konrad Rzeszutek Wilk]

On Tue, Oct 28, 2014 at 10:25:37AM +0000, Jan Beulich wrote:
> >>> On 27.10.14 at 19:07, <konrad.wilk@oracle.com> wrote:
> > 2). PF removal. Currently if you try to unload the PF and the VFs
> >    are in use (pciback owns them), the unloading will not happen. Until
> >    all of the VFs have been de-assigned.
> >    Is the "bug" here that the reporter (Intel?) wants the VFs to be
> >    automatically yanked out of a guest when the system admin wants to
> >    unload the PF?
> 
> No. They see the VF still listed as assignable after the guest terminated
> and the PF got made assignable. The PF driver unloading, however,
> should trigger all VFs (which are about to disappear) to get removed
> from pciback. I hope to find some time later today to look into why (if
> at all) this isn't happening right now.

That is due to PCI_DEV_FLAGS_ASSIGNED being set on the PF device which
inhibits it from unloading until that flag has been cleared.

That is odd that Intel is reporting it to you as Intel added that
flag as part of the SR-IOV NICs to inhibit the PF unloading when VFs
are still in-use.

> 
> Jan
> 

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

>>> On 28.10.14 at 15:00, <konrad.wilk@oracle.com> wrote:
> On Tue, Oct 28, 2014 at 10:25:37AM +0000, Jan Beulich wrote:
>> >>> On 27.10.14 at 19:07, <konrad.wilk@oracle.com> wrote:
>> > 2). PF removal. Currently if you try to unload the PF and the VFs
>> >    are in use (pciback owns them), the unloading will not happen. Until
>> >    all of the VFs have been de-assigned.
>> >    Is the "bug" here that the reporter (Intel?) wants the VFs to be
>> >    automatically yanked out of a guest when the system admin wants to
>> >    unload the PF?
>> 
>> No. They see the VF still listed as assignable after the guest terminated
>> and the PF got made assignable. The PF driver unloading, however,
>> should trigger all VFs (which are about to disappear) to get removed
>> from pciback. I hope to find some time later today to look into why (if
>> at all) this isn't happening right now.
> 
> That is due to PCI_DEV_FLAGS_ASSIGNED being set on the PF device which
> inhibits it from unloading until that flag has been cleared.
> 
> That is odd that Intel is reporting it to you as Intel added that
> flag as part of the SR-IOV NICs to inhibit the PF unloading when VFs
> are still in-use.

That flag doesn't inhibit the PF driver unloading. It only suppresses
it freeing the VFs in that process (and afaics nothing will ever allow
them to be released subsequently in that case). I'm about to test a
change to pciback (it's building right now).

Jan

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

>>> On 28.10.14 at 16:18, <JBeulich@suse.com> wrote:
> That flag doesn't inhibit the PF driver unloading. It only suppresses
> it freeing the VFs in that process (and afaics nothing will ever allow
> them to be released subsequently in that case). I'm about to test a
> change to pciback (it's building right now).

Attached a raw patch for you or anyone else to comment on from
a conceptual perspective. The main thing probably is to determine
whether any further serialization needs to be added.

Jan


[-- Attachment #2: 901839.patch --]
[-- Type: text/plain, Size: 1394 bytes --]

--- sle12.orig/drivers/xen/xen-pciback/pci_stub.c
+++ sle12/drivers/xen/xen-pciback/pci_stub.c
@@ -1502,6 +1502,44 @@ parse_error:
 fs_initcall(pcistub_init);
 #endif
 
+#ifdef CONFIG_PCI_IOV
+static int pci_stub_notifier(struct notifier_block *nb,
+			     unsigned long action, void *data)
+{
+	struct device *dev = data;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	switch (action) {
+	case BUS_NOTIFY_UNBIND_DRIVER:
+		for (;;) {
+			struct pcistub_device *psdev;
+			unsigned long flags;
+			bool found = false;
+
+			spin_lock_irqsave(&pcistub_devices_lock, flags);
+
+			list_for_each_entry(psdev, &pcistub_devices, dev_list)
+				if (!psdev->pdev && psdev->dev != pdev
+				    && pci_physfn(psdev->dev) == pdev) {
+					found = true;
+					break;
+				}
+			spin_unlock_irqrestore(&pcistub_devices_lock, flags);
+			if (!found)
+				break;
+			device_release_driver(&psdev->dev->dev);
+		}
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block pci_stub_nb = {
+	.notifier_call = pci_stub_notifier,
+};
+#endif
+
 static int __init xen_pcibk_init(void)
 {
 	int err;
@@ -1523,6 +1561,10 @@ static int __init xen_pcibk_init(void)
 	err = xen_pcibk_xenbus_register();
 	if (err)
 		pcistub_exit();
+#ifdef CONFIG_PCI_IOV
+	else
+		bus_register_notifier(&pci_bus_type, &pci_stub_nb);
+#endif
 
 	return err;
 }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

>>> On 28.10.14 at 17:25, <JBeulich@suse.com> wrote:
>>>> On 28.10.14 at 16:18, <JBeulich@suse.com> wrote:
>> That flag doesn't inhibit the PF driver unloading. It only suppresses
>> it freeing the VFs in that process (and afaics nothing will ever allow
>> them to be released subsequently in that case). I'm about to test a
>> change to pciback (it's building right now).
> 
> Attached a raw patch for you or anyone else to comment on from
> a conceptual perspective. The main thing probably is to determine
> whether any further serialization needs to be added.

And right after sending I noticed I forgot to unregister the notifier
in xen_pcibk_cleanup(). Consider this present when looking at the
patch.

Jan

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Konrad Rzeszutek Wilk]

On Tue, Oct 28, 2014 at 04:25:16PM +0000, Jan Beulich wrote:
> >>> On 28.10.14 at 16:18, <JBeulich@suse.com> wrote:
> > That flag doesn't inhibit the PF driver unloading. It only suppresses
> > it freeing the VFs in that process (and afaics nothing will ever allow
> > them to be released subsequently in that case). I'm about to test a
> > change to pciback (it's building right now).
> 
> Attached a raw patch for you or anyone else to comment on from
> a conceptual perspective. The main thing probably is to determine
> whether any further serialization needs to be added.
> 
> Jan
> 

> --- sle12.orig/drivers/xen/xen-pciback/pci_stub.c
> +++ sle12/drivers/xen/xen-pciback/pci_stub.c
> @@ -1502,6 +1502,44 @@ parse_error:
>  fs_initcall(pcistub_init);
>  #endif
>  
> +#ifdef CONFIG_PCI_IOV
> +static int pci_stub_notifier(struct notifier_block *nb,
> +			     unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	switch (action) {
> +	case BUS_NOTIFY_UNBIND_DRIVER:
> +		for (;;) {
> +			struct pcistub_device *psdev;
> +			unsigned long flags;
> +			bool found = false;
> +
> +			spin_lock_irqsave(&pcistub_devices_lock, flags);
> +
> +			list_for_each_entry(psdev, &pcistub_devices, dev_list)
> +				if (!psdev->pdev && psdev->dev != pdev
> +				    && pci_physfn(psdev->dev) == pdev) {
> +					found = true;
> +					break;
> +				}
> +			spin_unlock_irqrestore(&pcistub_devices_lock, flags);

There are existing functions in the PCI back driver to find an device based
on the 'struct pci_dev'.

That should make this piece of code much simpler by just looking for it
and if found call the 'device_release_driver'.

However I am not entirely sure what ought to be done if the VF is
in _use_ ? Should we still do it?

> +			if (!found)
> +				break;
> +			device_release_driver(&psdev->dev->dev);
> +		}
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block pci_stub_nb = {
> +	.notifier_call = pci_stub_notifier,
> +};
> +#endif
> +
>  static int __init xen_pcibk_init(void)
>  {
>  	int err;
> @@ -1523,6 +1561,10 @@ static int __init xen_pcibk_init(void)
>  	err = xen_pcibk_xenbus_register();
>  	if (err)
>  		pcistub_exit();
> +#ifdef CONFIG_PCI_IOV
> +	else
> +		bus_register_notifier(&pci_bus_type, &pci_stub_nb);
> +#endif
>  
>  	return err;
>  }

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

>>> On 28.10.14 at 17:56, <konrad.wilk@oracle.com> wrote:
> On Tue, Oct 28, 2014 at 04:25:16PM +0000, Jan Beulich wrote:
>> --- sle12.orig/drivers/xen/xen-pciback/pci_stub.c
>> +++ sle12/drivers/xen/xen-pciback/pci_stub.c
>> @@ -1502,6 +1502,44 @@ parse_error:
>>  fs_initcall(pcistub_init);
>>  #endif
>>  
>> +#ifdef CONFIG_PCI_IOV
>> +static int pci_stub_notifier(struct notifier_block *nb,
>> +			     unsigned long action, void *data)
>> +{
>> +	struct device *dev = data;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	switch (action) {
>> +	case BUS_NOTIFY_UNBIND_DRIVER:
>> +		for (;;) {
>> +			struct pcistub_device *psdev;
>> +			unsigned long flags;
>> +			bool found = false;
>> +
>> +			spin_lock_irqsave(&pcistub_devices_lock, flags);
>> +
>> +			list_for_each_entry(psdev, &pcistub_devices, dev_list)
>> +				if (!psdev->pdev && psdev->dev != pdev
>> +				    && pci_physfn(psdev->dev) == pdev) {
>> +					found = true;
>> +					break;
>> +				}
>> +			spin_unlock_irqrestore(&pcistub_devices_lock, flags);
> 
> There are existing functions in the PCI back driver to find an device based
> on the 'struct pci_dev'.
> 
> That should make this piece of code much simpler by just looking for it
> and if found call the 'device_release_driver'.

But we're not after the psdev matching the incoming pdev.

> However I am not entirely sure what ought to be done if the VF is
> in _use_ ? Should we still do it?

Of course not (as being insecure) - that's what the !psdev->pdev
check is there for.

Jan

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Andrew Cooper]

On 27/10/14 12:36, Jan Beulich wrote:
> All,
>
> Intel reports that the sequence
>
> - xl pci-assignable-add <VF>
> - briefly run guest using that device [not sure whether that's really a
>   necessary step]
> - xl pci-assignable-add <PF of VF>
>
> results in both VF and PF being listed as assignable (the fact that as a
> result the PF handed to a guest doesn't work is secondary here, as I
> think this is a driver issue). Is that really how it should be? Shouldn't
> instead all VFs get removed when the PF device (e.g. due to the
> PF driver getting unloaded, which is a necessary part of making it
> assignable) goes away? Or is it required for the admin to manually
> remove the assignable VFs prior to making the PF go away?

Which type of devices are these?

>From my recollection using igb and ixgbe with Netscalar, the VFs are
created after the PF driver has bound and started up.

Moving the binding from the real PF driver to pciback should remove the
VFs, although I would not be surprised if this has been overlooked, or
is expected to work but currently buggy.

As a logical consequence of the above, a PF with VFs must strictly not
be assignable, for safety reasons.

There are security considerations involved with passing a PF to a
domain, as that domU can still cause VFs to appear in dom0.  A PF should
either stay in dom0, be given to a trusted device driver domain.

~Andrew

Re: xl/SR-IOV: disposition of VFs when PF disappears?

[Jan Beulich]

>>> On 27.10.14 at 14:03, <andrew.cooper3@citrix.com> wrote:
> On 27/10/14 12:36, Jan Beulich wrote:
>> All,
>>
>> Intel reports that the sequence
>>
>> - xl pci-assignable-add <VF>
>> - briefly run guest using that device [not sure whether that's really a
>>   necessary step]
>> - xl pci-assignable-add <PF of VF>
>>
>> results in both VF and PF being listed as assignable (the fact that as a
>> result the PF handed to a guest doesn't work is secondary here, as I
>> think this is a driver issue). Is that really how it should be? Shouldn't
>> instead all VFs get removed when the PF device (e.g. due to the
>> PF driver getting unloaded, which is a necessary part of making it
>> assignable) goes away? Or is it required for the admin to manually
>> remove the assignable VFs prior to making the PF go away?
> 
> Which type of devices are these?

Intel having reported that, it's Intel NICs (not sure which driver,
but I also don't think this is relevant).

> From my recollection using igb and ixgbe with Netscalar, the VFs are
> created after the PF driver has bound and started up.

Yes, they're being created as the PF driver loads.

> Moving the binding from the real PF driver to pciback should remove the
> VFs, although I would not be surprised if this has been overlooked, or
> is expected to work but currently buggy.

Right - as a side effect of the PF getting unbound from its driver.

> As a logical consequence of the above, a PF with VFs must strictly not
> be assignable, for safety reasons.

I don't think this should be enforced, but rather left to the discretion
of the host admin.

> There are security considerations involved with passing a PF to a
> domain, as that domU can still cause VFs to appear in dom0.  A PF should
> either stay in dom0, be given to a trusted device driver domain.

How would a PF driver in a DomU cause VFs to appear in Dom0?
These aren't being found by scanning the bus, but rather get
created via software means afaik.

Jan