| Message ID | 20240702131428.664859-6-Jonathan.Cameron@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: NUMA nodes for CXL HB as GP + complex NUMA test | expand |
On Tue, 2 Jul 2024 14:14:10 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > Using a property allows us to hide the internal details of the PCI device > from the code to build a SRAT Generic Initiator Affinity Structure with > PCI Device Handle. > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > V4: Avoid confusion with device creation parameter bus but renaming to > busnr > --- > hw/acpi/acpi_generic_initiator.c | 11 ++++++----- > hw/pci/pci.c | 14 ++++++++++++++ > 2 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c > index 73bafaaaea..f2711c91ef 100644 > --- a/hw/acpi/acpi_generic_initiator.c > +++ b/hw/acpi/acpi_generic_initiator.c > @@ -9,6 +9,7 @@ > #include "hw/boards.h" > #include "hw/pci/pci_device.h" > #include "qemu/error-report.h" > +#include "qapi/error.h" > > typedef struct AcpiGenericInitiatorClass { > ObjectClass parent_class; > @@ -79,7 +80,7 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) > MachineState *ms = MACHINE(qdev_get_machine()); > AcpiGenericInitiator *gi; > GArray *table_data = opaque; > - PCIDevice *pci_dev; > + uint8_t bus, devfn; > Object *o; > > if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_INITIATOR)) { > @@ -100,10 +101,10 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) > exit(1); > } > > - pci_dev = PCI_DEVICE(o); > - build_srat_pci_generic_initiator(table_data, gi->node, 0, > - pci_bus_num(pci_get_bus(pci_dev)), > - pci_dev->devfn); > + bus = object_property_get_uint(o, "busnr", &error_fatal); > + devfn = object_property_get_uint(o, "addr", &error_fatal); devfn in PCI code is 32bit, while here it's declared as unit8_t, which seems wrong. It likely would work in case of PCIe root ports/switches where slot is 0, but should quickly break elsewhere as soon as slot is more than 0. If it's intentional, there should be fat comment here about why it this way and an assert to catch silent cropping of the value. > + > + build_srat_pci_generic_initiator(table_data, gi->node, 0, bus, devfn); > > return 0; > } > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 50b86d5790..29d4852c21 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -67,6 +67,19 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev); > static void pcibus_reset_hold(Object *obj, ResetType type); > static bool pcie_has_upstream_port(PCIDevice *dev); > > +static void prop_pci_busnr_get(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + uint8_t busnr = pci_dev_bus_num(PCI_DEVICE(obj)); > + > + visit_type_uint8(v, name, &busnr, errp); > +} > + > +static const PropertyInfo prop_pci_busnr = { > + .name = "busnr", > + .get = prop_pci_busnr_get, > +}; > + > static Property pci_props[] = { > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > @@ -85,6 +98,7 @@ static Property pci_props[] = { > QEMU_PCIE_ERR_UNC_MASK_BITNR, true), > DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, > QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), > + { .name = "busnr", .info = &prop_pci_busnr }, > DEFINE_PROP_END_OF_LIST() > }; >
On Thu, 11 Jul 2024 13:53:31 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Tue, 2 Jul 2024 14:14:10 +0100 > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > Using a property allows us to hide the internal details of the PCI device > > from the code to build a SRAT Generic Initiator Affinity Structure with > > PCI Device Handle. > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > --- > > V4: Avoid confusion with device creation parameter bus but renaming to > > busnr > > --- > > hw/acpi/acpi_generic_initiator.c | 11 ++++++----- > > hw/pci/pci.c | 14 ++++++++++++++ > > 2 files changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c > > index 73bafaaaea..f2711c91ef 100644 > > --- a/hw/acpi/acpi_generic_initiator.c > > +++ b/hw/acpi/acpi_generic_initiator.c > > @@ -9,6 +9,7 @@ > > #include "hw/boards.h" > > #include "hw/pci/pci_device.h" > > #include "qemu/error-report.h" > > +#include "qapi/error.h" > > > > typedef struct AcpiGenericInitiatorClass { > > ObjectClass parent_class; > > @@ -79,7 +80,7 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) > > MachineState *ms = MACHINE(qdev_get_machine()); > > AcpiGenericInitiator *gi; > > GArray *table_data = opaque; > > - PCIDevice *pci_dev; > > + uint8_t bus, devfn; > > Object *o; > > > > if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_INITIATOR)) { > > @@ -100,10 +101,10 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) > > exit(1); > > } > > > > - pci_dev = PCI_DEVICE(o); > > - build_srat_pci_generic_initiator(table_data, gi->node, 0, > > - pci_bus_num(pci_get_bus(pci_dev)), > > - pci_dev->devfn); > > + bus = object_property_get_uint(o, "busnr", &error_fatal); > > + devfn = object_property_get_uint(o, "addr", &error_fatal); > > devfn in PCI code is 32bit, while here it's declared as unit8_t, > which seems wrong. > It likely would work in case of PCIe root ports/switches where slot is 0, > but should quickly break elsewhere as soon as slot is more than 0. > > If it's intentional, there should be fat comment here about why it this way > and an assert to catch silent cropping of the value. Ignore that, obviously the rest of the QEMU does not care about this downcast. Maybe add assert anyways to catch too big devfn returned, which unlikely to happen ever. anyways: Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > + > > + build_srat_pci_generic_initiator(table_data, gi->node, 0, bus, devfn); > > > > return 0; > > } > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 50b86d5790..29d4852c21 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -67,6 +67,19 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev); > > static void pcibus_reset_hold(Object *obj, ResetType type); > > static bool pcie_has_upstream_port(PCIDevice *dev); > > > > +static void prop_pci_busnr_get(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + uint8_t busnr = pci_dev_bus_num(PCI_DEVICE(obj)); > > + > > + visit_type_uint8(v, name, &busnr, errp); > > +} > > + > > +static const PropertyInfo prop_pci_busnr = { > > + .name = "busnr", > > + .get = prop_pci_busnr_get, > > +}; > > + > > static Property pci_props[] = { > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > > @@ -85,6 +98,7 @@ static Property pci_props[] = { > > QEMU_PCIE_ERR_UNC_MASK_BITNR, true), > > DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, > > QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), > > + { .name = "busnr", .info = &prop_pci_busnr }, > > DEFINE_PROP_END_OF_LIST() > > }; > > >
On Thu, 11 Jul 2024 14:23:16 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 11 Jul 2024 13:53:31 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Tue, 2 Jul 2024 14:14:10 +0100 > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > > > Using a property allows us to hide the internal details of the PCI device > > > from the code to build a SRAT Generic Initiator Affinity Structure with > > > PCI Device Handle. > > > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > --- > > > V4: Avoid confusion with device creation parameter bus but renaming to > > > busnr > > > --- > > > hw/acpi/acpi_generic_initiator.c | 11 ++++++----- > > > hw/pci/pci.c | 14 ++++++++++++++ > > > 2 files changed, 20 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c > > > index 73bafaaaea..f2711c91ef 100644 > > > --- a/hw/acpi/acpi_generic_initiator.c > > > +++ b/hw/acpi/acpi_generic_initiator.c > > > @@ -9,6 +9,7 @@ > > > #include "hw/boards.h" > > > #include "hw/pci/pci_device.h" > > > #include "qemu/error-report.h" > > > +#include "qapi/error.h" > > > > > > typedef struct AcpiGenericInitiatorClass { > > > ObjectClass parent_class; > > > @@ -79,7 +80,7 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) > > > MachineState *ms = MACHINE(qdev_get_machine()); > > > AcpiGenericInitiator *gi; > > > GArray *table_data = opaque; > > > - PCIDevice *pci_dev; > > > + uint8_t bus, devfn; > > > Object *o; > > > > > > if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_INITIATOR)) { > > > @@ -100,10 +101,10 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) > > > exit(1); > > > } > > > > > > - pci_dev = PCI_DEVICE(o); > > > - build_srat_pci_generic_initiator(table_data, gi->node, 0, > > > - pci_bus_num(pci_get_bus(pci_dev)), > > > - pci_dev->devfn); > > > + bus = object_property_get_uint(o, "busnr", &error_fatal); > > > + devfn = object_property_get_uint(o, "addr", &error_fatal); > > > > devfn in PCI code is 32bit, while here it's declared as unit8_t, > > which seems wrong. > > It likely would work in case of PCIe root ports/switches where slot is 0, > > but should quickly break elsewhere as soon as slot is more than 0. > > > > If it's intentional, there should be fat comment here about why it this way > > and an assert to catch silent cropping of the value. > > Ignore that, obviously the rest of the QEMU does not care about this downcast. It's indeed odd that the storage is 32 bits. > > Maybe add assert anyways to catch too big devfn returned, > which unlikely to happen ever. Will do. assert(devfn >= 0 && devfn < PCI_DEVFN_MAX); with devfn locally as an int32_t and object_property_get_int() to match with the type. > > anyways: > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > > > + > > > + build_srat_pci_generic_initiator(table_data, gi->node, 0, bus, devfn); > > > > > > return 0; > > > } > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > index 50b86d5790..29d4852c21 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -67,6 +67,19 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev); > > > static void pcibus_reset_hold(Object *obj, ResetType type); > > > static bool pcie_has_upstream_port(PCIDevice *dev); > > > > > > +static void prop_pci_busnr_get(Object *obj, Visitor *v, const char *name, > > > + void *opaque, Error **errp) > > > +{ > > > + uint8_t busnr = pci_dev_bus_num(PCI_DEVICE(obj)); > > > + > > > + visit_type_uint8(v, name, &busnr, errp); > > > +} > > > + > > > +static const PropertyInfo prop_pci_busnr = { > > > + .name = "busnr", > > > + .get = prop_pci_busnr_get, > > > +}; > > > + > > > static Property pci_props[] = { > > > DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), > > > DEFINE_PROP_STRING("romfile", PCIDevice, romfile), > > > @@ -85,6 +98,7 @@ static Property pci_props[] = { > > > QEMU_PCIE_ERR_UNC_MASK_BITNR, true), > > > DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, > > > QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), > > > + { .name = "busnr", .info = &prop_pci_busnr }, > > > DEFINE_PROP_END_OF_LIST() > > > }; > > > > > > >
diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c index 73bafaaaea..f2711c91ef 100644 --- a/hw/acpi/acpi_generic_initiator.c +++ b/hw/acpi/acpi_generic_initiator.c @@ -9,6 +9,7 @@ #include "hw/boards.h" #include "hw/pci/pci_device.h" #include "qemu/error-report.h" +#include "qapi/error.h" typedef struct AcpiGenericInitiatorClass { ObjectClass parent_class; @@ -79,7 +80,7 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) MachineState *ms = MACHINE(qdev_get_machine()); AcpiGenericInitiator *gi; GArray *table_data = opaque; - PCIDevice *pci_dev; + uint8_t bus, devfn; Object *o; if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_INITIATOR)) { @@ -100,10 +101,10 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) exit(1); } - pci_dev = PCI_DEVICE(o); - build_srat_pci_generic_initiator(table_data, gi->node, 0, - pci_bus_num(pci_get_bus(pci_dev)), - pci_dev->devfn); + bus = object_property_get_uint(o, "busnr", &error_fatal); + devfn = object_property_get_uint(o, "addr", &error_fatal); + + build_srat_pci_generic_initiator(table_data, gi->node, 0, bus, devfn); return 0; } diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 50b86d5790..29d4852c21 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -67,6 +67,19 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev); static void pcibus_reset_hold(Object *obj, ResetType type); static bool pcie_has_upstream_port(PCIDevice *dev); +static void prop_pci_busnr_get(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + uint8_t busnr = pci_dev_bus_num(PCI_DEVICE(obj)); + + visit_type_uint8(v, name, &busnr, errp); +} + +static const PropertyInfo prop_pci_busnr = { + .name = "busnr", + .get = prop_pci_busnr_get, +}; + static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), DEFINE_PROP_STRING("romfile", PCIDevice, romfile), @@ -85,6 +98,7 @@ static Property pci_props[] = { QEMU_PCIE_ERR_UNC_MASK_BITNR, true), DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), + { .name = "busnr", .info = &prop_pci_busnr }, DEFINE_PROP_END_OF_LIST() };
Using a property allows us to hide the internal details of the PCI device from the code to build a SRAT Generic Initiator Affinity Structure with PCI Device Handle. Suggested-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- V4: Avoid confusion with device creation parameter bus but renaming to busnr --- hw/acpi/acpi_generic_initiator.c | 11 ++++++----- hw/pci/pci.c | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-)