| Message ID | 20240712110837.1439736-11-Jonathan.Cameron@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | acpi: NUMA nodes for CXL HB as GP + complex NUMA test | expand |
On Fri, 12 Jul 2024 12:08:14 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > These are very similar to the recently added Generic Initiators > but instead of representing an initiator of memory traffic they > represent an edge point beyond which may lie either targets or > initiators. Here we add these ports such that they may > be targets of hmat_lb records to describe the latency and > bandwidth from host side initiators to the port. A discoverable > mechanism such as UEFI CDAT read from CXL devices and switches > is used to discover the remainder of the path, and the OS can build > up full latency and bandwidth numbers as need for work and data > placement decisions. > > Acked-by: Markus Armbruster <armbru@redhat.com> > Tested-by: "Huang, Ying" <ying.huang@intel.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> ACPI tables generation LGTM As for the rest my review is perfunctory mostly. > --- > v5: Push the definition of TYPE_ACPI_GENERIC_PORT down into the > c file (similar to TYPE_ACPI_GENERIC_INITIATOR in earlier patch) > --- > qapi/qom.json | 34 +++++++++ > include/hw/acpi/aml-build.h | 4 + > include/hw/acpi/pci.h | 2 +- > include/hw/pci/pci_bridge.h | 1 + > hw/acpi/aml-build.c | 40 ++++++++++ > hw/acpi/pci.c | 112 +++++++++++++++++++++++++++- > hw/arm/virt-acpi-build.c | 2 +- > hw/i386/acpi-build.c | 2 +- > hw/pci-bridge/pci_expander_bridge.c | 1 - > 9 files changed, 193 insertions(+), 5 deletions(-) > > diff --git a/qapi/qom.json b/qapi/qom.json > index 8e75a419c3..b97c031b73 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -838,6 +838,38 @@ > 'data': { 'pci-dev': 'str', > 'node': 'uint32' } } > > +## > +# @AcpiGenericPortProperties: > +# > +# Properties for acpi-generic-port objects. > +# > +# @pci-bus: QOM path of the PCI bus of the hostbridge associated with > +# this SRAT Generic Port Affinity Structure. This is the same as > +# the bus parameter for the root ports attached to this host > +# bridge. The resulting SRAT Generic Port Affinity Structure will > +# refer to the ACPI object in DSDT that represents the host bridge > +# (e.g. ACPI0016 for CXL host bridges). See ACPI 6.5 Section > +# 5.2.16.7 for more information. > +# > +# @node: Similar to a NUMA node ID, but instead of providing a > +# reference point used for defining NUMA distances and access > +# characteristics to memory or from an initiator (e.g. CPU), this > +# node defines the boundary point between non-discoverable system > +# buses which must be described by firmware, and a discoverable > +# bus. NUMA distances and access characteristics are defined to > +# and from that point. For system software to establish full > +# initiator to target characteristics this information must be > +# combined with information retrieved from the discoverable part > +# of the path. An example would use CDAT (see UEFI.org) > +# information read from devices and switches in conjunction with > +# link characteristics read from PCIe Configuration space. you lost me here (even reading this several time doesn't help). Perhaps I lack specific domain knowledge, but is there a way to make it more comprehensible for layman? > +# > +# Since: 9.1 > +## > +{ 'struct': 'AcpiGenericPortProperties', > + 'data': { 'pci-bus': 'str', > + 'node': 'uint32' } } > + > ## > # @RngProperties: > # > @@ -1031,6 +1063,7 @@ > { 'enum': 'ObjectType', > 'data': [ > 'acpi-generic-initiator', > + 'acpi-generic-port', > 'authz-list', > 'authz-listfile', > 'authz-pam', > @@ -1106,6 +1139,7 @@ > 'discriminator': 'qom-type', > 'data': { > 'acpi-generic-initiator': 'AcpiGenericInitiatorProperties', > + 'acpi-generic-port': 'AcpiGenericPortProperties', > 'authz-list': 'AuthZListProperties', > 'authz-listfile': 'AuthZListFileProperties', > 'authz-pam': 'AuthZPAMProperties', > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 33eef85791..9e30c735bb 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -490,6 +490,10 @@ void build_srat_pci_generic_initiator(GArray *table_data, int node, > uint16_t segment, uint8_t bus, > uint8_t devfn); > > +void build_srat_acpi_generic_port(GArray *table_data, int node, > + const char *hid, > + uint32_t uid); > + > void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms, > const char *oem_id, const char *oem_table_id); > > diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h > index 3015a8171c..6359d574fd 100644 > --- a/include/hw/acpi/pci.h > +++ b/include/hw/acpi/pci.h > @@ -41,6 +41,6 @@ Aml *aml_pci_device_dsm(void); > void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus); > void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope); > > -void build_srat_generic_pci_initiator(GArray *table_data); > +void build_srat_generic_affinity_structures(GArray *table_data); > > #endif > diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h > index 5cd452115a..5456e24883 100644 > --- a/include/hw/pci/pci_bridge.h > +++ b/include/hw/pci/pci_bridge.h > @@ -102,6 +102,7 @@ typedef struct PXBPCIEDev { > PXBDev parent_obj; > } PXBPCIEDev; > > +#define TYPE_PXB_CXL_BUS "pxb-cxl-bus" > #define TYPE_PXB_DEV "pxb" > OBJECT_DECLARE_SIMPLE_TYPE(PXBDev, PXB_DEV) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index 968b654e58..4067100dd6 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -1955,6 +1955,19 @@ static void build_append_srat_pci_device_handle(GArray *table_data, > build_append_int_noprefix(table_data, 0, 12); > } > > +static void build_append_srat_acpi_device_handle(GArray *table_data, > + const char *hid, > + uint32_t uid) > +{ > + assert(strlen(hid) == 8); > + /* Device Handle - ACPI */ > + for (int i = 0; i < sizeof(hid); i++) { > + build_append_int_noprefix(table_data, hid[i], 1); > + } > + build_append_int_noprefix(table_data, uid, 4); > + build_append_int_noprefix(table_data, 0, 4); > +} > + > /* > * ACPI spec, Revision 6.3 > * 5.2.16.6 Generic Initiator Affinity Structure > @@ -1982,6 +1995,33 @@ void build_srat_pci_generic_initiator(GArray *table_data, int node, > build_append_int_noprefix(table_data, 0, 4); > } > > +/* > + * ACPI spec, Revision 6.5 > + * 5.2.16.7 Generic Port Affinity Structure > + * With ACPI Device Handle. > + */ > +void build_srat_acpi_generic_port(GArray *table_data, int node, shouldn't node be uint32_t? > + const char *hid, > + uint32_t uid) > +{ > + /* Type */ > + build_append_int_noprefix(table_data, 6, 1); > + /* Length */ > + build_append_int_noprefix(table_data, 32, 1); > + /* Reserved */ > + build_append_int_noprefix(table_data, 0, 1); > + /* Device Handle Type: ACPI */ > + build_append_int_noprefix(table_data, 0, 1); > + /* Proximity Domain */ > + build_append_int_noprefix(table_data, node, 4); > + /* Device Handle */ > + build_append_srat_acpi_device_handle(table_data, hid, uid); > + /* Flags - GP Enabled */ > + build_append_int_noprefix(table_data, 1, 4); > + /* Reserved */ > + build_append_int_noprefix(table_data, 0, 4); > +} > + > /* > * ACPI spec 5.2.17 System Locality Distance Information Table > * (Revision 2.0 or later) > diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c > index 3e1db161cc..020ad53c03 100644 > --- a/hw/acpi/pci.c > +++ b/hw/acpi/pci.c > @@ -30,6 +30,7 @@ > #include "hw/boards.h" > #include "hw/acpi/aml-build.h" > #include "hw/acpi/pci.h" > +#include "hw/pci/pci_bridge.h" > #include "hw/pci/pci_device.h" > #include "hw/pci/pcie_host.h" > > @@ -177,9 +178,118 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) > return 0; > } > > -void build_srat_generic_pci_initiator(GArray *table_data) > +typedef struct AcpiGenericPort { > + /* private */ > + Object parent; > + > + /* public */ > + char *pci_bus; > + uint16_t node; ditto > +} AcpiGenericPort; > + > +typedef struct AcpiGenericPortClass { > + ObjectClass parent_class; > +} AcpiGenericPortClass; > + > +#define TYPE_ACPI_GENERIC_PORT "acpi-generic-port" > + > +OBJECT_DEFINE_TYPE_WITH_INTERFACES(AcpiGenericPort, acpi_generic_port, > + ACPI_GENERIC_PORT, OBJECT, > + { TYPE_USER_CREATABLE }, > + { NULL }) > + > +OBJECT_DECLARE_SIMPLE_TYPE(AcpiGenericPort, ACPI_GENERIC_PORT) > + > +static void acpi_generic_port_init(Object *obj) > +{ > + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); > + > + gp->node = MAX_NODES; > + gp->pci_bus = NULL; > +} > + > +static void acpi_generic_port_finalize(Object *obj) > +{ > + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); > + > + g_free(gp->pci_bus); > +} > + > +static void acpi_generic_port_set_pci_bus(Object *obj, const char *val, > + Error **errp) > +{ > + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); > + > + gp->pci_bus = g_strdup(val); > +} > + > +static void acpi_generic_port_set_node(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); > + uint32_t value; > + > + if (!visit_type_uint32(v, name, &value, errp)) { > + return; > + } > + > + if (value >= MAX_NODES) { > + error_printf("%s: Invalid NUMA node specified\n", > + TYPE_ACPI_GENERIC_INITIATOR); > + exit(1); > + } > + > + gp->node = value; as long as gp->node is uint32_t it should be fine. otherwise it's too fragile. > +} > + > +static void acpi_generic_port_class_init(ObjectClass *oc, void *data) > +{ > + object_class_property_add_str(oc, "pci-bus", NULL, > + acpi_generic_port_set_pci_bus); > + object_class_property_add(oc, "node", "int", NULL, > + acpi_generic_port_set_node, NULL, NULL); missing property description calls. > +} > + > +static int build_acpi_generic_port(Object *obj, void *opaque) > +{ > + MachineState *ms = MACHINE(qdev_get_machine()); > + const char *hid = "ACPI0016"; > + GArray *table_data = opaque; > + AcpiGenericPort *gp; > + uint32_t uid; > + Object *o; > + > + if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_PORT)) { > + return 0; > + } > + > + gp = ACPI_GENERIC_PORT(obj); > + > + if (gp->node >= ms->numa_state->num_nodes) { > + error_printf("%s: node %d is invalid.\n", > + TYPE_ACPI_GENERIC_PORT, gp->node); > + exit(1); not sure, maybe use error_fatal instead of using exit(1)? CCing Markus to check if it's ok. > + } > + > + o = object_resolve_path_type(gp->pci_bus, TYPE_PXB_CXL_BUS, NULL); > + if (!o) { > + error_printf("%s: device must be a CXL host bridge.\n", > + TYPE_ACPI_GENERIC_PORT); > + exit(1); > + } ditto > + > + uid = object_property_get_uint(o, "acpi_uid", &error_fatal); > + build_srat_acpi_generic_port(table_data, gp->node, hid, uid); > + > + return 0; > +} > + > +void build_srat_generic_affinity_structures(GArray *table_data) > { > object_child_foreach_recursive(object_get_root(), > build_acpi_generic_initiator, > table_data); > + object_child_foreach_recursive(object_get_root(), build_acpi_generic_port, > + table_data); > } > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index a50b00b7c1..d98651df55 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -510,7 +510,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > } > } > > - build_srat_generic_pci_initiator(table_data); > + build_srat_generic_affinity_structures(table_data); > > if (ms->nvdimms_state->is_enabled) { > nvdimm_build_srat(table_data); > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 2eaa4c9203..7f5ca188c1 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2048,7 +2048,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine) > build_srat_memory(table_data, 0, 0, 0, MEM_AFFINITY_NOFLAGS); > } > > - build_srat_generic_pci_initiator(table_data); > + build_srat_generic_affinity_structures(table_data); > > /* > * Entry is required for Windows to enable memory hotplug in OS > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c > index b94cb85cfb..cd7f2d5423 100644 > --- a/hw/pci-bridge/pci_expander_bridge.c > +++ b/hw/pci-bridge/pci_expander_bridge.c > @@ -38,7 +38,6 @@ DECLARE_INSTANCE_CHECKER(PXBBus, PXB_BUS, > DECLARE_INSTANCE_CHECKER(PXBBus, PXB_PCIE_BUS, > TYPE_PXB_PCIE_BUS) > > -#define TYPE_PXB_CXL_BUS "pxb-cxl-bus" > DECLARE_INSTANCE_CHECKER(PXBBus, PXB_CXL_BUS, > TYPE_PXB_CXL_BUS) >
On Mon, 15 Jul 2024 16:48:41 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 12 Jul 2024 12:08:14 +0100 > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > These are very similar to the recently added Generic Initiators > > but instead of representing an initiator of memory traffic they > > represent an edge point beyond which may lie either targets or > > initiators. Here we add these ports such that they may > > be targets of hmat_lb records to describe the latency and > > bandwidth from host side initiators to the port. A discoverable > > mechanism such as UEFI CDAT read from CXL devices and switches > > is used to discover the remainder of the path, and the OS can build > > up full latency and bandwidth numbers as need for work and data > > placement decisions. > > > > Acked-by: Markus Armbruster <armbru@redhat.com> > > Tested-by: "Huang, Ying" <ying.huang@intel.com> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > ACPI tables generation LGTM > As for the rest my review is perfunctory mostly. The node type points and missing descriptor applying equally to generic initiators. I'll add a couple of patches cleaning that up as well as fixing them up for generic ports. For the exit(1) that was copying other similar locations. I don't mind changing it though if something else is preferred. Given tight timescales (and I was away for a few days which didn't help), I'll send out a v6 with changes as below. Jonathan > > > --- > > v5: Push the definition of TYPE_ACPI_GENERIC_PORT down into the > > c file (similar to TYPE_ACPI_GENERIC_INITIATOR in earlier patch) > > --- > > qapi/qom.json | 34 +++++++++ > > include/hw/acpi/aml-build.h | 4 + > > include/hw/acpi/pci.h | 2 +- > > include/hw/pci/pci_bridge.h | 1 + > > hw/acpi/aml-build.c | 40 ++++++++++ > > hw/acpi/pci.c | 112 +++++++++++++++++++++++++++- > > hw/arm/virt-acpi-build.c | 2 +- > > hw/i386/acpi-build.c | 2 +- > > hw/pci-bridge/pci_expander_bridge.c | 1 - > > 9 files changed, 193 insertions(+), 5 deletions(-) > > > > diff --git a/qapi/qom.json b/qapi/qom.json > > index 8e75a419c3..b97c031b73 100644 > > --- a/qapi/qom.json > > +++ b/qapi/qom.json > > @@ -838,6 +838,38 @@ > > 'data': { 'pci-dev': 'str', > > 'node': 'uint32' } } > > > > +## > > +# @AcpiGenericPortProperties: > > +# > > +# Properties for acpi-generic-port objects. > > +# > > +# @pci-bus: QOM path of the PCI bus of the hostbridge associated with > > +# this SRAT Generic Port Affinity Structure. This is the same as > > +# the bus parameter for the root ports attached to this host > > +# bridge. The resulting SRAT Generic Port Affinity Structure will > > +# refer to the ACPI object in DSDT that represents the host bridge > > +# (e.g. ACPI0016 for CXL host bridges). See ACPI 6.5 Section > > +# 5.2.16.7 for more information. > > +# > > > +# @node: Similar to a NUMA node ID, but instead of providing a > > +# reference point used for defining NUMA distances and access > > +# characteristics to memory or from an initiator (e.g. CPU), this > > +# node defines the boundary point between non-discoverable system > > +# buses which must be described by firmware, and a discoverable > > +# bus. NUMA distances and access characteristics are defined to > > +# and from that point. For system software to establish full > > +# initiator to target characteristics this information must be > > +# combined with information retrieved from the discoverable part > > +# of the path. An example would use CDAT (see UEFI.org) > > +# information read from devices and switches in conjunction with > > +# link characteristics read from PCIe Configuration space. > > you lost me here (even reading this several time doesn't help). > Perhaps I lack specific domain knowledge, but is there a way to make it > more comprehensible for layman? This is far from the first version (which Markus really didn't like ;) It is really easy to draw as a sequence of diagrams and really tricky to put in text! Not so easy to get the kernel code right either as it turns out but that's another story. Perhaps if I add something to the end to say what you do with it that might help? "To get the full path latency, from CPU to CXL attached DRAM on a type 3 CXL device: Add the latency from CPU to Generic Port (from HMAT indexed via the the node ID in this SRAT structure) to that for CXL bus links, the latency across intermediate switches and from the EP port to the actual memory. Bandwidth is more complex as there may be interleaving across multiple devices and shared links in the path." > > > +# > > +# Since: 9.1 > > +## > > +{ 'struct': 'AcpiGenericPortProperties', > > + 'data': { 'pci-bus': 'str', > > + 'node': 'uint32' } } > > +
On Wed, Jul 17, 2024 at 04:02:58PM +0100, Jonathan Cameron wrote: > On Mon, 15 Jul 2024 16:48:41 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Fri, 12 Jul 2024 12:08:14 +0100 > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > > > These are very similar to the recently added Generic Initiators > > > but instead of representing an initiator of memory traffic they > > > represent an edge point beyond which may lie either targets or > > > initiators. Here we add these ports such that they may > > > be targets of hmat_lb records to describe the latency and > > > bandwidth from host side initiators to the port. A discoverable > > > mechanism such as UEFI CDAT read from CXL devices and switches > > > is used to discover the remainder of the path, and the OS can build > > > up full latency and bandwidth numbers as need for work and data > > > placement decisions. > > > > > > Acked-by: Markus Armbruster <armbru@redhat.com> > > > Tested-by: "Huang, Ying" <ying.huang@intel.com> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > ACPI tables generation LGTM > > As for the rest my review is perfunctory mostly. > > The node type points and missing descriptor applying equally to generic > initiators. I'll add a couple of patches cleaning that up as well as > fixing them up for generic ports. > > For the exit(1) that was copying other similar locations. I don't > mind changing it though if something else is preferred. > > Given tight timescales (and I was away for a few days which didn't > help), I'll send out a v6 with changes as below. > > Jonathan > I'm working on a pull and going offline for a week guys, what's not in will be in the next release. Sorry. > > > > > --- > > > v5: Push the definition of TYPE_ACPI_GENERIC_PORT down into the > > > c file (similar to TYPE_ACPI_GENERIC_INITIATOR in earlier patch) > > > --- > > > qapi/qom.json | 34 +++++++++ > > > include/hw/acpi/aml-build.h | 4 + > > > include/hw/acpi/pci.h | 2 +- > > > include/hw/pci/pci_bridge.h | 1 + > > > hw/acpi/aml-build.c | 40 ++++++++++ > > > hw/acpi/pci.c | 112 +++++++++++++++++++++++++++- > > > hw/arm/virt-acpi-build.c | 2 +- > > > hw/i386/acpi-build.c | 2 +- > > > hw/pci-bridge/pci_expander_bridge.c | 1 - > > > 9 files changed, 193 insertions(+), 5 deletions(-) > > > > > > diff --git a/qapi/qom.json b/qapi/qom.json > > > index 8e75a419c3..b97c031b73 100644 > > > --- a/qapi/qom.json > > > +++ b/qapi/qom.json > > > @@ -838,6 +838,38 @@ > > > 'data': { 'pci-dev': 'str', > > > 'node': 'uint32' } } > > > > > > +## > > > +# @AcpiGenericPortProperties: > > > +# > > > +# Properties for acpi-generic-port objects. > > > +# > > > +# @pci-bus: QOM path of the PCI bus of the hostbridge associated with > > > +# this SRAT Generic Port Affinity Structure. This is the same as > > > +# the bus parameter for the root ports attached to this host > > > +# bridge. The resulting SRAT Generic Port Affinity Structure will > > > +# refer to the ACPI object in DSDT that represents the host bridge > > > +# (e.g. ACPI0016 for CXL host bridges). See ACPI 6.5 Section > > > +# 5.2.16.7 for more information. > > > +# > > > > > +# @node: Similar to a NUMA node ID, but instead of providing a > > > +# reference point used for defining NUMA distances and access > > > +# characteristics to memory or from an initiator (e.g. CPU), this > > > +# node defines the boundary point between non-discoverable system > > > +# buses which must be described by firmware, and a discoverable > > > +# bus. NUMA distances and access characteristics are defined to > > > +# and from that point. For system software to establish full > > > +# initiator to target characteristics this information must be > > > +# combined with information retrieved from the discoverable part > > > +# of the path. An example would use CDAT (see UEFI.org) > > > +# information read from devices and switches in conjunction with > > > +# link characteristics read from PCIe Configuration space. > > > > you lost me here (even reading this several time doesn't help). > > Perhaps I lack specific domain knowledge, but is there a way to make it > > more comprehensible for layman? > > This is far from the first version (which Markus really didn't like ;) > It is really easy to draw as a sequence of diagrams and really tricky > to put in text! Not so easy to get the kernel code right either > as it turns out but that's another story. > > Perhaps if I add something to the end to say what you do with it > that might help? > > "To get the full path latency, from CPU to CXL attached DRAM on a type 3 > CXL device: Add the latency from CPU to Generic Port (from HMAT indexed > via the the node ID in this SRAT structure) to that for CXL bus links, the > latency across intermediate switches and from the EP port to the > actual memory. Bandwidth is more complex as there may be interleaving > across multiple devices and shared links in the path." > > > > > > +# > > > +# Since: 9.1 > > > +## > > > +{ 'struct': 'AcpiGenericPortProperties', > > > + 'data': { 'pci-bus': 'str', > > > + 'node': 'uint32' } } > > > + >
On Wed, 17 Jul 2024 11:11:06 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 17, 2024 at 04:02:58PM +0100, Jonathan Cameron wrote: > > On Mon, 15 Jul 2024 16:48:41 +0200 > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > On Fri, 12 Jul 2024 12:08:14 +0100 > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > > > > > These are very similar to the recently added Generic Initiators > > > > but instead of representing an initiator of memory traffic they > > > > represent an edge point beyond which may lie either targets or > > > > initiators. Here we add these ports such that they may > > > > be targets of hmat_lb records to describe the latency and > > > > bandwidth from host side initiators to the port. A discoverable > > > > mechanism such as UEFI CDAT read from CXL devices and switches > > > > is used to discover the remainder of the path, and the OS can build > > > > up full latency and bandwidth numbers as need for work and data > > > > placement decisions. > > > > > > > > Acked-by: Markus Armbruster <armbru@redhat.com> > > > > Tested-by: "Huang, Ying" <ying.huang@intel.com> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > ACPI tables generation LGTM > > > As for the rest my review is perfunctory mostly. > > > > The node type points and missing descriptor applying equally to generic > > initiators. I'll add a couple of patches cleaning that up as well as > > fixing them up for generic ports. > > > > For the exit(1) that was copying other similar locations. I don't > > mind changing it though if something else is preferred. > > > > Given tight timescales (and I was away for a few days which didn't > > help), I'll send out a v6 with changes as below. > > > > Jonathan > > > > I'm working on a pull and going offline for a week guys, what's not in > will be in the next release. Sorry. No problem. Thanks for letting us know! In that case I'll sit on v6 for a while and hopefully we can get it lined up early next cycle without too much bios-tables test churn pain. Jonathan
On Mon, 15 Jul 2024 16:48:41 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 12 Jul 2024 12:08:14 +0100 > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > > These are very similar to the recently added Generic Initiators > > but instead of representing an initiator of memory traffic they > > represent an edge point beyond which may lie either targets or > > initiators. Here we add these ports such that they may > > be targets of hmat_lb records to describe the latency and > > bandwidth from host side initiators to the port. A discoverable > > mechanism such as UEFI CDAT read from CXL devices and switches > > is used to discover the remainder of the path, and the OS can build > > up full latency and bandwidth numbers as need for work and data > > placement decisions. > > > > Acked-by: Markus Armbruster <armbru@redhat.com> > > Tested-by: "Huang, Ying" <ying.huang@intel.com> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > ACPI tables generation LGTM > As for the rest my review is perfunctory mostly. Hi Igor, Given I guess we will soon be in the 9.2 cycle, revisiting this to prepare a v6. Added missing parameter descriptions for properties in here and an additional patch for the ones missing for generic initiators. Another patch fixes the uint32_t fragilty you pointed out for generic initiators (fixed here as well) 2 things remain, the docs and the question you are asked Markus. See inline. > > > --- > > v5: Push the definition of TYPE_ACPI_GENERIC_PORT down into the > > c file (similar to TYPE_ACPI_GENERIC_INITIATOR in earlier patch) > > --- > > qapi/qom.json | 34 +++++++++ > > include/hw/acpi/aml-build.h | 4 + > > include/hw/acpi/pci.h | 2 +- > > include/hw/pci/pci_bridge.h | 1 + > > hw/acpi/aml-build.c | 40 ++++++++++ > > hw/acpi/pci.c | 112 +++++++++++++++++++++++++++- > > hw/arm/virt-acpi-build.c | 2 +- > > hw/i386/acpi-build.c | 2 +- > > hw/pci-bridge/pci_expander_bridge.c | 1 - > > 9 files changed, 193 insertions(+), 5 deletions(-) > > > > diff --git a/qapi/qom.json b/qapi/qom.json > > index 8e75a419c3..b97c031b73 100644 > > --- a/qapi/qom.json > > +++ b/qapi/qom.json > > @@ -838,6 +838,38 @@ > > 'data': { 'pci-dev': 'str', > > 'node': 'uint32' } } > > > > +## > > +# @AcpiGenericPortProperties: > > +# > > +# Properties for acpi-generic-port objects. > > +# > > +# @pci-bus: QOM path of the PCI bus of the hostbridge associated with > > +# this SRAT Generic Port Affinity Structure. This is the same as > > +# the bus parameter for the root ports attached to this host > > +# bridge. The resulting SRAT Generic Port Affinity Structure will > > +# refer to the ACPI object in DSDT that represents the host bridge > > +# (e.g. ACPI0016 for CXL host bridges). See ACPI 6.5 Section > > +# 5.2.16.7 for more information. > > +# > > > +# @node: Similar to a NUMA node ID, but instead of providing a > > +# reference point used for defining NUMA distances and access > > +# characteristics to memory or from an initiator (e.g. CPU), this > > +# node defines the boundary point between non-discoverable system > > +# buses which must be described by firmware, and a discoverable > > +# bus. NUMA distances and access characteristics are defined to > > +# and from that point. For system software to establish full > > +# initiator to target characteristics this information must be > > +# combined with information retrieved from the discoverable part > > +# of the path. An example would use CDAT (see UEFI.org) > > +# information read from devices and switches in conjunction with > > +# link characteristics read from PCIe Configuration space. > > you lost me here (even reading this several time doesn't help). > Perhaps I lack specific domain knowledge, but is there a way to make it > more comprehensible for layman? I've had a few passes and clearly still failing :( Maybe an example is the way to go? Does the following help? (replacing the 'An example sentence' above). # For example, a CXL Memory device M is directly # plugged into a CXL root port P which is part of a # CXL root bridge B. To calculate latency from a CPU in # the host to the memory on M, the latency from that CPU # to the bridge B (and hence port P)* must be added # to the latency due to the CXL Link between P and M # (calculated from link status registers) and that from # the CXL port on device M to the actual memory (read from # CDAT via a mailbox on the device). # The CPU to root bridge latency (*) is provided by ACPI HMAT. # HMAT latency data is indexed with the combination of the # proximity domain for the CPU and that for the root bridge. # This node value is the root bridge part of that index pair. ... > > +static int build_acpi_generic_port(Object *obj, void *opaque) > > +{ > > + MachineState *ms = MACHINE(qdev_get_machine()); > > + const char *hid = "ACPI0016"; > > + GArray *table_data = opaque; > > + AcpiGenericPort *gp; > > + uint32_t uid; > > + Object *o; > > + > > + if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_PORT)) { > > + return 0; > > + } > > + > > + gp = ACPI_GENERIC_PORT(obj); > > + > > + if (gp->node >= ms->numa_state->num_nodes) { > > > + error_printf("%s: node %d is invalid.\n", > > + TYPE_ACPI_GENERIC_PORT, gp->node); > > + exit(1); > > not sure, > maybe use error_fatal instead of using exit(1)? > > CCing Markus to check if it's ok. Markus? > > > > + } > > + > > + o = object_resolve_path_type(gp->pci_bus, TYPE_PXB_CXL_BUS, NULL); > > + if (!o) { > > + error_printf("%s: device must be a CXL host bridge.\n", > > + TYPE_ACPI_GENERIC_PORT); > > + exit(1); > > + } > ditto >
diff --git a/qapi/qom.json b/qapi/qom.json index 8e75a419c3..b97c031b73 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -838,6 +838,38 @@ 'data': { 'pci-dev': 'str', 'node': 'uint32' } } +## +# @AcpiGenericPortProperties: +# +# Properties for acpi-generic-port objects. +# +# @pci-bus: QOM path of the PCI bus of the hostbridge associated with +# this SRAT Generic Port Affinity Structure. This is the same as +# the bus parameter for the root ports attached to this host +# bridge. The resulting SRAT Generic Port Affinity Structure will +# refer to the ACPI object in DSDT that represents the host bridge +# (e.g. ACPI0016 for CXL host bridges). See ACPI 6.5 Section +# 5.2.16.7 for more information. +# +# @node: Similar to a NUMA node ID, but instead of providing a +# reference point used for defining NUMA distances and access +# characteristics to memory or from an initiator (e.g. CPU), this +# node defines the boundary point between non-discoverable system +# buses which must be described by firmware, and a discoverable +# bus. NUMA distances and access characteristics are defined to +# and from that point. For system software to establish full +# initiator to target characteristics this information must be +# combined with information retrieved from the discoverable part +# of the path. An example would use CDAT (see UEFI.org) +# information read from devices and switches in conjunction with +# link characteristics read from PCIe Configuration space. +# +# Since: 9.1 +## +{ 'struct': 'AcpiGenericPortProperties', + 'data': { 'pci-bus': 'str', + 'node': 'uint32' } } + ## # @RngProperties: # @@ -1031,6 +1063,7 @@ { 'enum': 'ObjectType', 'data': [ 'acpi-generic-initiator', + 'acpi-generic-port', 'authz-list', 'authz-listfile', 'authz-pam', @@ -1106,6 +1139,7 @@ 'discriminator': 'qom-type', 'data': { 'acpi-generic-initiator': 'AcpiGenericInitiatorProperties', + 'acpi-generic-port': 'AcpiGenericPortProperties', 'authz-list': 'AuthZListProperties', 'authz-listfile': 'AuthZListFileProperties', 'authz-pam': 'AuthZPAMProperties', diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h index 33eef85791..9e30c735bb 100644 --- a/include/hw/acpi/aml-build.h +++ b/include/hw/acpi/aml-build.h @@ -490,6 +490,10 @@ void build_srat_pci_generic_initiator(GArray *table_data, int node, uint16_t segment, uint8_t bus, uint8_t devfn); +void build_srat_acpi_generic_port(GArray *table_data, int node, + const char *hid, + uint32_t uid); + void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms, const char *oem_id, const char *oem_table_id); diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h index 3015a8171c..6359d574fd 100644 --- a/include/hw/acpi/pci.h +++ b/include/hw/acpi/pci.h @@ -41,6 +41,6 @@ Aml *aml_pci_device_dsm(void); void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus); void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope); -void build_srat_generic_pci_initiator(GArray *table_data); +void build_srat_generic_affinity_structures(GArray *table_data); #endif diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h index 5cd452115a..5456e24883 100644 --- a/include/hw/pci/pci_bridge.h +++ b/include/hw/pci/pci_bridge.h @@ -102,6 +102,7 @@ typedef struct PXBPCIEDev { PXBDev parent_obj; } PXBPCIEDev; +#define TYPE_PXB_CXL_BUS "pxb-cxl-bus" #define TYPE_PXB_DEV "pxb" OBJECT_DECLARE_SIMPLE_TYPE(PXBDev, PXB_DEV) diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index 968b654e58..4067100dd6 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -1955,6 +1955,19 @@ static void build_append_srat_pci_device_handle(GArray *table_data, build_append_int_noprefix(table_data, 0, 12); } +static void build_append_srat_acpi_device_handle(GArray *table_data, + const char *hid, + uint32_t uid) +{ + assert(strlen(hid) == 8); + /* Device Handle - ACPI */ + for (int i = 0; i < sizeof(hid); i++) { + build_append_int_noprefix(table_data, hid[i], 1); + } + build_append_int_noprefix(table_data, uid, 4); + build_append_int_noprefix(table_data, 0, 4); +} + /* * ACPI spec, Revision 6.3 * 5.2.16.6 Generic Initiator Affinity Structure @@ -1982,6 +1995,33 @@ void build_srat_pci_generic_initiator(GArray *table_data, int node, build_append_int_noprefix(table_data, 0, 4); } +/* + * ACPI spec, Revision 6.5 + * 5.2.16.7 Generic Port Affinity Structure + * With ACPI Device Handle. + */ +void build_srat_acpi_generic_port(GArray *table_data, int node, + const char *hid, + uint32_t uid) +{ + /* Type */ + build_append_int_noprefix(table_data, 6, 1); + /* Length */ + build_append_int_noprefix(table_data, 32, 1); + /* Reserved */ + build_append_int_noprefix(table_data, 0, 1); + /* Device Handle Type: ACPI */ + build_append_int_noprefix(table_data, 0, 1); + /* Proximity Domain */ + build_append_int_noprefix(table_data, node, 4); + /* Device Handle */ + build_append_srat_acpi_device_handle(table_data, hid, uid); + /* Flags - GP Enabled */ + build_append_int_noprefix(table_data, 1, 4); + /* Reserved */ + build_append_int_noprefix(table_data, 0, 4); +} + /* * ACPI spec 5.2.17 System Locality Distance Information Table * (Revision 2.0 or later) diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c index 3e1db161cc..020ad53c03 100644 --- a/hw/acpi/pci.c +++ b/hw/acpi/pci.c @@ -30,6 +30,7 @@ #include "hw/boards.h" #include "hw/acpi/aml-build.h" #include "hw/acpi/pci.h" +#include "hw/pci/pci_bridge.h" #include "hw/pci/pci_device.h" #include "hw/pci/pcie_host.h" @@ -177,9 +178,118 @@ static int build_acpi_generic_initiator(Object *obj, void *opaque) return 0; } -void build_srat_generic_pci_initiator(GArray *table_data) +typedef struct AcpiGenericPort { + /* private */ + Object parent; + + /* public */ + char *pci_bus; + uint16_t node; +} AcpiGenericPort; + +typedef struct AcpiGenericPortClass { + ObjectClass parent_class; +} AcpiGenericPortClass; + +#define TYPE_ACPI_GENERIC_PORT "acpi-generic-port" + +OBJECT_DEFINE_TYPE_WITH_INTERFACES(AcpiGenericPort, acpi_generic_port, + ACPI_GENERIC_PORT, OBJECT, + { TYPE_USER_CREATABLE }, + { NULL }) + +OBJECT_DECLARE_SIMPLE_TYPE(AcpiGenericPort, ACPI_GENERIC_PORT) + +static void acpi_generic_port_init(Object *obj) +{ + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); + + gp->node = MAX_NODES; + gp->pci_bus = NULL; +} + +static void acpi_generic_port_finalize(Object *obj) +{ + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); + + g_free(gp->pci_bus); +} + +static void acpi_generic_port_set_pci_bus(Object *obj, const char *val, + Error **errp) +{ + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); + + gp->pci_bus = g_strdup(val); +} + +static void acpi_generic_port_set_node(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + AcpiGenericPort *gp = ACPI_GENERIC_PORT(obj); + uint32_t value; + + if (!visit_type_uint32(v, name, &value, errp)) { + return; + } + + if (value >= MAX_NODES) { + error_printf("%s: Invalid NUMA node specified\n", + TYPE_ACPI_GENERIC_INITIATOR); + exit(1); + } + + gp->node = value; +} + +static void acpi_generic_port_class_init(ObjectClass *oc, void *data) +{ + object_class_property_add_str(oc, "pci-bus", NULL, + acpi_generic_port_set_pci_bus); + object_class_property_add(oc, "node", "int", NULL, + acpi_generic_port_set_node, NULL, NULL); +} + +static int build_acpi_generic_port(Object *obj, void *opaque) +{ + MachineState *ms = MACHINE(qdev_get_machine()); + const char *hid = "ACPI0016"; + GArray *table_data = opaque; + AcpiGenericPort *gp; + uint32_t uid; + Object *o; + + if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_PORT)) { + return 0; + } + + gp = ACPI_GENERIC_PORT(obj); + + if (gp->node >= ms->numa_state->num_nodes) { + error_printf("%s: node %d is invalid.\n", + TYPE_ACPI_GENERIC_PORT, gp->node); + exit(1); + } + + o = object_resolve_path_type(gp->pci_bus, TYPE_PXB_CXL_BUS, NULL); + if (!o) { + error_printf("%s: device must be a CXL host bridge.\n", + TYPE_ACPI_GENERIC_PORT); + exit(1); + } + + uid = object_property_get_uint(o, "acpi_uid", &error_fatal); + build_srat_acpi_generic_port(table_data, gp->node, hid, uid); + + return 0; +} + +void build_srat_generic_affinity_structures(GArray *table_data) { object_child_foreach_recursive(object_get_root(), build_acpi_generic_initiator, table_data); + object_child_foreach_recursive(object_get_root(), build_acpi_generic_port, + table_data); } diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index a50b00b7c1..d98651df55 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -510,7 +510,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) } } - build_srat_generic_pci_initiator(table_data); + build_srat_generic_affinity_structures(table_data); if (ms->nvdimms_state->is_enabled) { nvdimm_build_srat(table_data); diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 2eaa4c9203..7f5ca188c1 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2048,7 +2048,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine) build_srat_memory(table_data, 0, 0, 0, MEM_AFFINITY_NOFLAGS); } - build_srat_generic_pci_initiator(table_data); + build_srat_generic_affinity_structures(table_data); /* * Entry is required for Windows to enable memory hotplug in OS diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c index b94cb85cfb..cd7f2d5423 100644 --- a/hw/pci-bridge/pci_expander_bridge.c +++ b/hw/pci-bridge/pci_expander_bridge.c @@ -38,7 +38,6 @@ DECLARE_INSTANCE_CHECKER(PXBBus, PXB_BUS, DECLARE_INSTANCE_CHECKER(PXBBus, PXB_PCIE_BUS, TYPE_PXB_PCIE_BUS) -#define TYPE_PXB_CXL_BUS "pxb-cxl-bus" DECLARE_INSTANCE_CHECKER(PXBBus, PXB_CXL_BUS, TYPE_PXB_CXL_BUS)