* [Qemu-devel] [PATCH] hw/pci-bridge: check invalid slot number for root and downstream port @ 2018-12-05 9:41 Huan Xiong 2018-12-14 21:12 ` Michael S. Tsirkin 0 siblings, 1 reply; 3+ messages in thread From: Huan Xiong @ 2018-12-05 9:41 UTC (permalink / raw) To: qemu-devel; +Cc: mst, marcel.apfelbaum Since root and downstream port have only one slot, device should be connected to them using slot 0. QEMU doesn't have a check for that and starts up when a non-zero slot is specified, though the device is not seen in guest OS. The change fixes that by adding a check in PCI device "attr" property setter function. The check is performed only if a PCI device has been connected to a bus, otherwise it does nothing. The latter occurs because setter function is also called in object instantiation phase to set property default value. Signed-off-by: Huan Xiong <huan.xiong@hxt-semitech.com> --- hw/core/qdev-properties.c | 5 ++++- hw/pci-bridge/pcie_root_port.c | 2 +- hw/pci-bridge/xio3130_downstream.c | 2 +- hw/pci/pci.c | 13 +++++++++++++ include/hw/pci/pci.h | 1 + 5 files changed, 20 insertions(+), 3 deletions(-) diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 35072de..6e79219 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -654,6 +654,8 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { DeviceState *dev = DEVICE(obj); + BusState *bus = qdev_get_parent_bus(dev); + BusClass *bus_class = bus ? BUS_GET_CLASS(bus) : NULL; Property *prop = opaque; int32_t value, *ptr = qdev_get_prop_ptr(dev, prop); unsigned int slot, fn, n; @@ -687,7 +689,8 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, goto invalid; } } - if (str[n] != '\0' || fn > 7 || slot > 31) { + if (str[n] != '\0' || fn > 7 || slot > 31 || (bus_class && + bus_class->max_dev != 0 && slot >= bus_class->max_dev)) { goto invalid; } *ptr = slot << 3 | fn; diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c index 45f9e8c..ee42411 100644 --- a/hw/pci-bridge/pcie_root_port.c +++ b/hw/pci-bridge/pcie_root_port.c @@ -61,7 +61,7 @@ static void rp_realize(PCIDevice *d, Error **errp) int rc; pci_config_set_interrupt_pin(d->config, 1); - pci_bridge_initfn(d, TYPE_PCIE_BUS); + pci_bridge_initfn(d, TYPE_PCIE_DOWNSTREAM_BUS); pcie_port_init_reg(d); rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index 467bbab..960a90c 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -62,7 +62,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp) PCIESlot *s = PCIE_SLOT(d); int rc; - pci_bridge_initfn(d, TYPE_PCIE_BUS); + pci_bridge_initfn(d, TYPE_PCIE_DOWNSTREAM_BUS); pcie_port_init_reg(d); rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 56b13b3..457736d 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -187,6 +187,18 @@ static const TypeInfo pcie_bus_info = { .parent = TYPE_PCI_BUS, }; +static void pcie_downstream_bus_class_init(ObjectClass *klass, void *data) +{ + BusClass *k = BUS_CLASS(klass); + k->max_dev = 1; +} + +static const TypeInfo pcie_downstream_bus_info = { + .name = TYPE_PCIE_DOWNSTREAM_BUS, + .parent = TYPE_PCIE_BUS, + .class_init = pcie_downstream_bus_class_init, +}; + static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num); static void pci_update_mappings(PCIDevice *d); static void pci_irq_handler(void *opaque, int irq_num, int level); @@ -2681,6 +2693,7 @@ static void pci_register_types(void) { type_register_static(&pci_bus_info); type_register_static(&pcie_bus_info); + type_register_static(&pcie_downstream_bus_info); type_register_static(&conventional_pci_interface_info); type_register_static(&pcie_interface_info); type_register_static(&pci_device_type_info); diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index e6514bb..2253757 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -393,6 +393,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); #define PCI_BUS_CLASS(klass) OBJECT_CLASS_CHECK(PCIBusClass, (klass), TYPE_PCI_BUS) #define PCI_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(PCIBusClass, (obj), TYPE_PCI_BUS) #define TYPE_PCIE_BUS "PCIE" +#define TYPE_PCIE_DOWNSTREAM_BUS "PCIE-downstream" bool pci_bus_is_express(PCIBus *bus); bool pci_bus_is_root(PCIBus *bus); -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/pci-bridge: check invalid slot number for root and downstream port 2018-12-05 9:41 [Qemu-devel] [PATCH] hw/pci-bridge: check invalid slot number for root and downstream port Huan Xiong @ 2018-12-14 21:12 ` Michael S. Tsirkin 2018-12-21 15:24 ` Xiong, Huan 0 siblings, 1 reply; 3+ messages in thread From: Michael S. Tsirkin @ 2018-12-14 21:12 UTC (permalink / raw) To: Huan Xiong; +Cc: qemu-devel, marcel.apfelbaum On Wed, Dec 05, 2018 at 05:41:26PM +0800, Huan Xiong wrote: > Since root and downstream port have only one slot, device should be > connected to them using slot 0. QEMU doesn't have a check for that > and starts up when a non-zero slot is specified, though the device > is not seen in guest OS. > > The change fixes that by adding a check in PCI device "attr" property > setter function. The check is performed only if a PCI device has been > connected to a bus, otherwise it does nothing. The latter occurs because > setter function is also called in object instantiation phase to set > property default value. > > Signed-off-by: Huan Xiong <huan.xiong@hxt-semitech.com> I thought that a non 0 slot is useful for ARI. No? > --- > hw/core/qdev-properties.c | 5 ++++- > hw/pci-bridge/pcie_root_port.c | 2 +- > hw/pci-bridge/xio3130_downstream.c | 2 +- > hw/pci/pci.c | 13 +++++++++++++ > include/hw/pci/pci.h | 1 + > 5 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 35072de..6e79219 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -654,6 +654,8 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > DeviceState *dev = DEVICE(obj); > + BusState *bus = qdev_get_parent_bus(dev); > + BusClass *bus_class = bus ? BUS_GET_CLASS(bus) : NULL; > Property *prop = opaque; > int32_t value, *ptr = qdev_get_prop_ptr(dev, prop); > unsigned int slot, fn, n; > @@ -687,7 +689,8 @@ static void set_pci_devfn(Object *obj, Visitor *v, const char *name, > goto invalid; > } > } > - if (str[n] != '\0' || fn > 7 || slot > 31) { > + if (str[n] != '\0' || fn > 7 || slot > 31 || (bus_class && > + bus_class->max_dev != 0 && slot >= bus_class->max_dev)) { > goto invalid; > } This looks kind of complicated. Isn't there an easier way? > *ptr = slot << 3 | fn; > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > index 45f9e8c..ee42411 100644 > --- a/hw/pci-bridge/pcie_root_port.c > +++ b/hw/pci-bridge/pcie_root_port.c > @@ -61,7 +61,7 @@ static void rp_realize(PCIDevice *d, Error **errp) > int rc; > > pci_config_set_interrupt_pin(d->config, 1); > - pci_bridge_initfn(d, TYPE_PCIE_BUS); > + pci_bridge_initfn(d, TYPE_PCIE_DOWNSTREAM_BUS); > pcie_port_init_reg(d); > > rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index 467bbab..960a90c 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -62,7 +62,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp) > PCIESlot *s = PCIE_SLOT(d); > int rc; > > - pci_bridge_initfn(d, TYPE_PCIE_BUS); > + pci_bridge_initfn(d, TYPE_PCIE_DOWNSTREAM_BUS); > pcie_port_init_reg(d); > > rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 56b13b3..457736d 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -187,6 +187,18 @@ static const TypeInfo pcie_bus_info = { > .parent = TYPE_PCI_BUS, > }; > > +static void pcie_downstream_bus_class_init(ObjectClass *klass, void *data) > +{ > + BusClass *k = BUS_CLASS(klass); > + k->max_dev = 1; > +} > + > +static const TypeInfo pcie_downstream_bus_info = { > + .name = TYPE_PCIE_DOWNSTREAM_BUS, > + .parent = TYPE_PCIE_BUS, > + .class_init = pcie_downstream_bus_class_init, > +}; > + > static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num); > static void pci_update_mappings(PCIDevice *d); > static void pci_irq_handler(void *opaque, int irq_num, int level); > @@ -2681,6 +2693,7 @@ static void pci_register_types(void) > { > type_register_static(&pci_bus_info); > type_register_static(&pcie_bus_info); > + type_register_static(&pcie_downstream_bus_info); > type_register_static(&conventional_pci_interface_info); > type_register_static(&pcie_interface_info); > type_register_static(&pci_device_type_info); > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index e6514bb..2253757 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -393,6 +393,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); > #define PCI_BUS_CLASS(klass) OBJECT_CLASS_CHECK(PCIBusClass, (klass), TYPE_PCI_BUS) > #define PCI_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(PCIBusClass, (obj), TYPE_PCI_BUS) > #define TYPE_PCIE_BUS "PCIE" > +#define TYPE_PCIE_DOWNSTREAM_BUS "PCIE-downstream" > > bool pci_bus_is_express(PCIBus *bus); > bool pci_bus_is_root(PCIBus *bus); > -- > 2.7.4 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/pci-bridge: check invalid slot number for root and downstream port 2018-12-14 21:12 ` Michael S. Tsirkin @ 2018-12-21 15:24 ` Xiong, Huan 0 siblings, 0 replies; 3+ messages in thread From: Xiong, Huan @ 2018-12-21 15:24 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel@nongnu.org, marcel.apfelbaum@gmail.com On Fri, 14 Dec 2018 21:12:34 +0000 (UTC), Michael S. Tsirkin wrote: > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 2018年12月15日 5:13 > To: Xiong, Huan <huan.xiong@hxt-semitech.com> > Cc: qemu-devel@nongnu.org; marcel.apfelbaum@gmail.com > Subject: Re: [PATCH] hw/pci-bridge: check invalid slot number for root and > downstream port > > On Wed, Dec 05, 2018 at 05:41:26PM +0800, Huan Xiong wrote: > > Since root and downstream port have only one slot, device should be > > connected to them using slot 0. QEMU doesn't have a check for that and > > starts up when a non-zero slot is specified, though the device is not > > seen in guest OS. > > > > The change fixes that by adding a check in PCI device "attr" property > > setter function. The check is performed only if a PCI device has been > > connected to a bus, otherwise it does nothing. The latter occurs > > because setter function is also called in object instantiation phase > > to set property default value. > > > > Signed-off-by: Huan Xiong <huan.xiong@hxt-semitech.com> > > I thought that a non 0 slot is useful for ARI. No? Michael, Sorry for the confusion. I didn't know about ARI and misunderstood the issue. Please ignore the patch. The issue I tried to fix was like the following (note qemu-xhci's slot was non-0). With this setup, qemu started up but the xhci controller wasn't seen in guest. -device pcie-root-port,chassis=2,id=rp.2,bus=pcie.0,addr=0x2 \ -device qemu-xhci,id=usb,bus=rp.2,addr=0x1 \ It now seems to me that there are two factors that lead to the issue: 1) QEMU doesn't set ARI capability for emulated PCIe device (pcie_ari_init() isn't called anywhere). 2) Event if ARI capability was set, the command line perhaps still wouldn't work because addr=0x1 would be function 8 of slot 0, which means function 0 of slot 0 is undefined. Thanks, Ray > > > > --- > > hw/core/qdev-properties.c | 5 ++++- > > hw/pci-bridge/pcie_root_port.c | 2 +- > > hw/pci-bridge/xio3130_downstream.c | 2 +- > > hw/pci/pci.c | 13 +++++++++++++ > > include/hw/pci/pci.h | 1 + > > 5 files changed, 20 insertions(+), 3 deletions(-) > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > > index 35072de..6e79219 100644 > > --- a/hw/core/qdev-properties.c > > +++ b/hw/core/qdev-properties.c > > @@ -654,6 +654,8 @@ static void set_pci_devfn(Object *obj, Visitor *v, > const char *name, > > void *opaque, Error **errp) { > > DeviceState *dev = DEVICE(obj); > > + BusState *bus = qdev_get_parent_bus(dev); > > + BusClass *bus_class = bus ? BUS_GET_CLASS(bus) : NULL; > > Property *prop = opaque; > > int32_t value, *ptr = qdev_get_prop_ptr(dev, prop); > > unsigned int slot, fn, n; > > @@ -687,7 +689,8 @@ static void set_pci_devfn(Object *obj, Visitor *v, > const char *name, > > goto invalid; > > } > > } > > - if (str[n] != '\0' || fn > 7 || slot > 31) { > > + if (str[n] != '\0' || fn > 7 || slot > 31 || (bus_class && > > + bus_class->max_dev != 0 && slot >= bus_class->max_dev)) { > > goto invalid; > > } > > This looks kind of complicated. Isn't there an easier way? > > > *ptr = slot << 3 | fn; > > diff --git a/hw/pci-bridge/pcie_root_port.c > > b/hw/pci-bridge/pcie_root_port.c index 45f9e8c..ee42411 100644 > > --- a/hw/pci-bridge/pcie_root_port.c > > +++ b/hw/pci-bridge/pcie_root_port.c > > @@ -61,7 +61,7 @@ static void rp_realize(PCIDevice *d, Error **errp) > > int rc; > > > > pci_config_set_interrupt_pin(d->config, 1); > > - pci_bridge_initfn(d, TYPE_PCIE_BUS); > > + pci_bridge_initfn(d, TYPE_PCIE_DOWNSTREAM_BUS); > > pcie_port_init_reg(d); > > > > rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, > > diff --git a/hw/pci-bridge/xio3130_downstream.c > > b/hw/pci-bridge/xio3130_downstream.c > > index 467bbab..960a90c 100644 > > --- a/hw/pci-bridge/xio3130_downstream.c > > +++ b/hw/pci-bridge/xio3130_downstream.c > > @@ -62,7 +62,7 @@ static void xio3130_downstream_realize(PCIDevice *d, > Error **errp) > > PCIESlot *s = PCIE_SLOT(d); > > int rc; > > > > - pci_bridge_initfn(d, TYPE_PCIE_BUS); > > + pci_bridge_initfn(d, TYPE_PCIE_DOWNSTREAM_BUS); > > pcie_port_init_reg(d); > > > > rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR, diff > > --git a/hw/pci/pci.c b/hw/pci/pci.c index 56b13b3..457736d 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -187,6 +187,18 @@ static const TypeInfo pcie_bus_info = { > > .parent = TYPE_PCI_BUS, > > }; > > > > +static void pcie_downstream_bus_class_init(ObjectClass *klass, void > > +*data) { > > + BusClass *k = BUS_CLASS(klass); > > + k->max_dev = 1; > > +} > > + > > +static const TypeInfo pcie_downstream_bus_info = { > > + .name = TYPE_PCIE_DOWNSTREAM_BUS, > > + .parent = TYPE_PCIE_BUS, > > + .class_init = pcie_downstream_bus_class_init, }; > > + > > static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num); static > > void pci_update_mappings(PCIDevice *d); static void > > pci_irq_handler(void *opaque, int irq_num, int level); @@ -2681,6 > > +2693,7 @@ static void pci_register_types(void) { > > type_register_static(&pci_bus_info); > > type_register_static(&pcie_bus_info); > > + type_register_static(&pcie_downstream_bus_info); > > type_register_static(&conventional_pci_interface_info); > > type_register_static(&pcie_interface_info); > > type_register_static(&pci_device_type_info); > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index > > e6514bb..2253757 100644 > > --- a/include/hw/pci/pci.h > > +++ b/include/hw/pci/pci.h > > @@ -393,6 +393,7 @@ typedef PCIINTxRoute (*pci_route_irq_fn)(void > > *opaque, int pin); #define PCI_BUS_CLASS(klass) > > OBJECT_CLASS_CHECK(PCIBusClass, (klass), TYPE_PCI_BUS) #define > > PCI_BUS_GET_CLASS(obj) OBJECT_GET_CLASS(PCIBusClass, (obj), TYPE_PCI_BUS) > #define TYPE_PCIE_BUS "PCIE" > > +#define TYPE_PCIE_DOWNSTREAM_BUS "PCIE-downstream" > > > > bool pci_bus_is_express(PCIBus *bus); bool pci_bus_is_root(PCIBus > > *bus); > > -- > > 2.7.4 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-21 15:25 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-05 9:41 [Qemu-devel] [PATCH] hw/pci-bridge: check invalid slot number for root and downstream port Huan Xiong 2018-12-14 21:12 ` Michael S. Tsirkin 2018-12-21 15:24 ` Xiong, Huan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).