* [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, marcel.apfelbaum
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.