| Message ID | 1458727927-15082-8-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 23 Mar 2016 18:12:02 +0800 Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > 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 <chen.fan.fnst@cn.fujitsu.com> > --- > 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;
On Thu, Mar 24, 2016 at 04:54:33PM -0600, Alex Williamson wrote: > On Wed, 23 Mar 2016 18:12:02 +0800 > Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > > > 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 <chen.fan.fnst@cn.fujitsu.com> > > --- > > 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;
On Wed, Mar 23, 2016 at 06:12:02PM +0800, Cao jin wrote: > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > 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 <chen.fan.fnst@cn.fujitsu.com> 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 <mst@redhat.com> 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 > >
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 <caoj.fnst@cn.fujitsu.com> wrote: >> >>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >>> >>> 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 <chen.fan.fnst@cn.fujitsu.com> >>> --- >>> 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; > > . >
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;