[v5,07/12] pci: add a pci_function_is_valid callback to check function if valid

Commit Message

Cao jin March 23, 2016, 10:12 a.m. UTC

From: Chen Fan <[e-mail]>

PCI hotplug requires that function 0 is added last to close the
slot.  Since vfio supporting AER, we require that the VM bus
contains the same set of devices as the host bus to support AER,
we can perform an AER validation test whenever a function 0 in
the VM is hot-added.

Signed-off-by: Chen Fan <[e-mail]>
---
 hw/pci/pci.c         | 32 ++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h |  1 +
 2 files changed, 33 insertions(+)

Comments

Alex Williamson March 24, 2016, 10:54 p.m. UTC | #1

On Wed, 23 Mar 2016 18:12:02 +0800
Cao jin <[e-mail]> wrote:

> From: Chen Fan <[e-mail]>
> 
> PCI hotplug requires that function 0 is added last to close the
> slot.  Since vfio supporting AER, we require that the VM bus
> contains the same set of devices as the host bus to support AER,
> we can perform an AER validation test whenever a function 0 in
> the VM is hot-added.
> 
> Signed-off-by: Chen Fan <[e-mail]>
> ---
>  hw/pci/pci.c         | 32 ++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h |  1 +
>  2 files changed, 33 insertions(+)


This would of course need Michael's ack.

Michael, we can't do this in vfio-pci code because we can't guarantee
that function 0 is a vfio-pci device.  That would allow users to bypass
our configuration requirements by putting any random non-vfio-pci
device at function 0 of a hot-added multifunction device.

> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e67664d..2a5291a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1840,6 +1840,22 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
>      return bus->devices[devfn];
>  }
>  
> +static void pci_function_is_valid(PCIBus *bus, PCIDevice *d, void *opaque)
> +{
> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(d);
> +    Error **errp = opaque;
> +
> +    if (*errp) {
> +        return;
> +    }
> +
> +    if (!pc->is_valid_func) {
> +        return;
> +    }
> +
> +    pc->is_valid_func(d, errp);
> +}
> +
>  static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>  {
>      PCIDevice *pci_dev = (PCIDevice *)qdev;
> @@ -1882,6 +1898,22 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>          return;
>      }
> +
> +    /*
> +     *  If the function number is 0, indicate the closure of the slot.
> +     *  then we get the chance to check all functions on same device
> +     *  if valid.
> +     */
> +    if (DEVICE(pci_dev)->hotplugged &&
> +        pci_get_function_0(pci_dev) == pci_dev) {
> +        pci_for_each_device(bus, pci_bus_num(bus),
> +                            pci_function_is_valid, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> +            return;
> +        }
> +    }
>  }
>  
>  static void pci_default_realize(PCIDevice *dev, Error **errp)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 0be07c8..4a2f7d4 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -190,6 +190,7 @@ typedef struct PCIDeviceClass {
>  
>      void (*realize)(PCIDevice *dev, Error **errp);
>      int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> +    void (*is_valid_func)(PCIDevice *dev, Error **errp);
>      PCIUnregisterFunc *exit;
>      PCIConfigReadFunc *config_read;
>      PCIConfigWriteFunc *config_write;

Michael S. Tsirkin March 27, 2016, 12:19 p.m. UTC | #2

On Thu, Mar 24, 2016 at 04:54:33PM -0600, Alex Williamson wrote:
> On Wed, 23 Mar 2016 18:12:02 +0800
> Cao jin <[e-mail]> wrote:
> 
> > From: Chen Fan <[e-mail]>
> > 
> > PCI hotplug requires that function 0 is added last to close the
> > slot.  Since vfio supporting AER, we require that the VM bus
> > contains the same set of devices as the host bus to support AER,
> > we can perform an AER validation test whenever a function 0 in
> > the VM is hot-added.
> > 
> > Signed-off-by: Chen Fan <[e-mail]>
> > ---
> >  hw/pci/pci.c         | 32 ++++++++++++++++++++++++++++++++
> >  include/hw/pci/pci.h |  1 +
> >  2 files changed, 33 insertions(+)
> 
> 
> This would of course need Michael's ack.
> 
> Michael, we can't do this in vfio-pci code because we can't guarantee
> that function 0 is a vfio-pci device.  That would allow users to bypass
> our configuration requirements by putting any random non-vfio-pci
> device at function 0 of a hot-added multifunction device.

I got that. What I don't understand is how is the non hotplug
variant handled, given that is_valid_func is not called
in that case.

Also, why does it scan all devices on bus, not all functions
of the device?

> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index e67664d..2a5291a 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1840,6 +1840,22 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
> >      return bus->devices[devfn];
> >  }
> >  
> > +static void pci_function_is_valid(PCIBus *bus, PCIDevice *d, void *opaque)
> > +{
> > +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(d);
> > +    Error **errp = opaque;
> > +
> > +    if (*errp) {
> > +        return;
> > +    }
> > +
> > +    if (!pc->is_valid_func) {
> > +        return;
> > +    }
> > +
> > +    pc->is_valid_func(d, errp);
> > +}
> > +
> >  static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >  {
> >      PCIDevice *pci_dev = (PCIDevice *)qdev;
> > @@ -1882,6 +1898,22 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> >          pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> >          return;
> >      }
> > +
> > +    /*
> > +     *  If the function number is 0, indicate the closure of the slot.
> > +     *  then we get the chance to check all functions on same device
> > +     *  if valid.
> > +     */
> > +    if (DEVICE(pci_dev)->hotplugged &&
> > +        pci_get_function_0(pci_dev) == pci_dev) {
> > +        pci_for_each_device(bus, pci_bus_num(bus),
> > +                            pci_function_is_valid, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> > +            return;
> > +        }
> > +    }
> >  }
> >  
> >  static void pci_default_realize(PCIDevice *dev, Error **errp)
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 0be07c8..4a2f7d4 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -190,6 +190,7 @@ typedef struct PCIDeviceClass {
> >  
> >      void (*realize)(PCIDevice *dev, Error **errp);
> >      int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> > +    void (*is_valid_func)(PCIDevice *dev, Error **errp);
> >      PCIUnregisterFunc *exit;
> >      PCIConfigReadFunc *config_read;
> >      PCIConfigWriteFunc *config_write;

Michael S. Tsirkin March 27, 2016, 12:52 p.m. UTC | #3

On Wed, Mar 23, 2016 at 06:12:02PM +0800, Cao jin wrote:
> From: Chen Fan <[e-mail]>
> 
> PCI hotplug requires that function 0 is added last to close the
> slot.  Since vfio supporting AER, we require that the VM bus
> contains the same set of devices as the host bus to support AER,
> we can perform an AER validation test whenever a function 0 in
> the VM is hot-added.
> 
> Signed-off-by: Chen Fan <[e-mail]>

So I sent some comments, but I do not want to hold up this series.
If there's a respin anyway, pls try to address my comments.
Alex, if you want to merge this in this shape, you can include my

Reviewed-by: Michael S. Tsirkin <[e-mail]>

for now, and we can clean up the issues afterwards.


> ---
>  hw/pci/pci.c         | 32 ++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h |  1 +
>  2 files changed, 33 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e67664d..2a5291a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1840,6 +1840,22 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
>      return bus->devices[devfn];
>  }
>  
> +static void pci_function_is_valid(PCIBus *bus, PCIDevice *d, void *opaque)
> +{
> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(d);
> +    Error **errp = opaque;
> +
> +    if (*errp) {
> +        return;
> +    }
> +
> +    if (!pc->is_valid_func) {
> +        return;
> +    }
> +
> +    pc->is_valid_func(d, errp);
> +}
> +
>  static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>  {
>      PCIDevice *pci_dev = (PCIDevice *)qdev;
> @@ -1882,6 +1898,22 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>          return;
>      }
> +
> +    /*
> +     *  If the function number is 0, indicate the closure of the slot.
> +     *  then we get the chance to check all functions on same device
> +     *  if valid.
> +     */
> +    if (DEVICE(pci_dev)->hotplugged &&
> +        pci_get_function_0(pci_dev) == pci_dev) {
> +        pci_for_each_device(bus, pci_bus_num(bus),
> +                            pci_function_is_valid, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> +            return;
> +        }
> +    }
>  }
>  
>  static void pci_default_realize(PCIDevice *dev, Error **errp)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 0be07c8..4a2f7d4 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -190,6 +190,7 @@ typedef struct PCIDeviceClass {
>  
>      void (*realize)(PCIDevice *dev, Error **errp);
>      int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
> +    void (*is_valid_func)(PCIDevice *dev, Error **errp);
>      PCIUnregisterFunc *exit;
>      PCIConfigReadFunc *config_read;
>      PCIConfigWriteFunc *config_write;
> -- 
> 1.9.3
> 
>

Chen Fan March 31, 2016, 7:23 a.m. UTC | #4

On 03/27/2016 08:19 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 24, 2016 at 04:54:33PM -0600, Alex Williamson wrote:
>> On Wed, 23 Mar 2016 18:12:02 +0800
>> Cao jin <[e-mail]> wrote:
>>
>>> From: Chen Fan <[e-mail]>
>>>
>>> PCI hotplug requires that function 0 is added last to close the
>>> slot.  Since vfio supporting AER, we require that the VM bus
>>> contains the same set of devices as the host bus to support AER,
>>> we can perform an AER validation test whenever a function 0 in
>>> the VM is hot-added.
>>>
>>> Signed-off-by: Chen Fan <[e-mail]>
>>> ---
>>>   hw/pci/pci.c         | 32 ++++++++++++++++++++++++++++++++
>>>   include/hw/pci/pci.h |  1 +
>>>   2 files changed, 33 insertions(+)
>>
>> This would of course need Michael's ack.
>>
>> Michael, we can't do this in vfio-pci code because we can't guarantee
>> that function 0 is a vfio-pci device.  That would allow users to bypass
>> our configuration requirements by putting any random non-vfio-pci
>> device at function 0 of a hot-added multifunction device.
> I got that. What I don't understand is how is the non hotplug
> variant handled, given that is_valid_func is not called
> in that case.
for non-hotplug case, we used machine_done_notifier to check
the bus reset functionality. because we can't ensure the function 0
was initialized at last when cold boot up.
>
> Also, why does it scan all devices on bus, not all functions
> of the device?
I can fix this by testing the bridge whether support ARI.

Thanks.
Chen

>
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index e67664d..2a5291a 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -1840,6 +1840,22 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
>>>       return bus->devices[devfn];
>>>   }
>>>   
>>> +static void pci_function_is_valid(PCIBus *bus, PCIDevice *d, void *opaque)
>>> +{
>>> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(d);
>>> +    Error **errp = opaque;
>>> +
>>> +    if (*errp) {
>>> +        return;
>>> +    }
>>> +
>>> +    if (!pc->is_valid_func) {
>>> +        return;
>>> +    }
>>> +
>>> +    pc->is_valid_func(d, errp);
>>> +}
>>> +
>>>   static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>   {
>>>       PCIDevice *pci_dev = (PCIDevice *)qdev;
>>> @@ -1882,6 +1898,22 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>>           pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>>>           return;
>>>       }
>>> +
>>> +    /*
>>> +     *  If the function number is 0, indicate the closure of the slot.
>>> +     *  then we get the chance to check all functions on same device
>>> +     *  if valid.
>>> +     */
>>> +    if (DEVICE(pci_dev)->hotplugged &&
>>> +        pci_get_function_0(pci_dev) == pci_dev) {
>>> +        pci_for_each_device(bus, pci_bus_num(bus),
>>> +                            pci_function_is_valid, &local_err);
>>> +        if (local_err) {
>>> +            error_propagate(errp, local_err);
>>> +            pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>>> +            return;
>>> +        }
>>> +    }
>>>   }
>>>   
>>>   static void pci_default_realize(PCIDevice *dev, Error **errp)
>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>> index 0be07c8..4a2f7d4 100644
>>> --- a/include/hw/pci/pci.h
>>> +++ b/include/hw/pci/pci.h
>>> @@ -190,6 +190,7 @@ typedef struct PCIDeviceClass {
>>>   
>>>       void (*realize)(PCIDevice *dev, Error **errp);
>>>       int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
>>> +    void (*is_valid_func)(PCIDevice *dev, Error **errp);
>>>       PCIUnregisterFunc *exit;
>>>       PCIConfigReadFunc *config_read;
>>>       PCIConfigWriteFunc *config_write;
>
> .
>

Patch

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e67664d..2a5291a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1840,6 +1840,22 @@  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
     return bus->devices[devfn];
 }
 
+static void pci_function_is_valid(PCIBus *bus, PCIDevice *d, void *opaque)
+{
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(d);
+    Error **errp = opaque;
+
+    if (*errp) {
+        return;
+    }
+
+    if (!pc->is_valid_func) {
+        return;
+    }
+
+    pc->is_valid_func(d, errp);
+}
+
 static void pci_qdev_realize(DeviceState *qdev, Error **errp)
 {
     PCIDevice *pci_dev = (PCIDevice *)qdev;
@@ -1882,6 +1898,22 @@  static void pci_qdev_realize(DeviceState *qdev, Error **errp)
         pci_qdev_unrealize(DEVICE(pci_dev), NULL);
         return;
     }
+
+    /*
+     *  If the function number is 0, indicate the closure of the slot.
+     *  then we get the chance to check all functions on same device
+     *  if valid.
+     */
+    if (DEVICE(pci_dev)->hotplugged &&
+        pci_get_function_0(pci_dev) == pci_dev) {
+        pci_for_each_device(bus, pci_bus_num(bus),
+                            pci_function_is_valid, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            pci_qdev_unrealize(DEVICE(pci_dev), NULL);
+            return;
+        }
+    }
 }
 
 static void pci_default_realize(PCIDevice *dev, Error **errp)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 0be07c8..4a2f7d4 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -190,6 +190,7 @@  typedef struct PCIDeviceClass {
 
     void (*realize)(PCIDevice *dev, Error **errp);
     int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
+    void (*is_valid_func)(PCIDevice *dev, Error **errp);
     PCIUnregisterFunc *exit;
     PCIConfigReadFunc *config_read;
     PCIConfigWriteFunc *config_write;