[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 1/3] qom: new object to associate device to numa node
|
From: |
Vikram Sethi |
|
Subject: |
RE: [PATCH v2 1/3] qom: new object to associate device to numa node |
|
Date: |
Wed, 11 Oct 2023 17:37:11 +0000 |
Hi Jonathan,
> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Monday, October 9, 2023 7:27 AM
> To: Ankit Agrawal <ankita@nvidia.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>; alex.williamson@redhat.com;
> clg@redhat.com; shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> ani@anisinha.ca; berrange@redhat.com; eduardo@habkost.net;
> imammedo@redhat.com; mst@redhat.com; eblake@redhat.com;
> armbru@redhat.com; david@redhat.com; gshan@redhat.com; Aniket
> Agashe <aniketa@nvidia.com>; Neo Jia <cjia@nvidia.com>; Kirti Wankhede
> <kwankhede@nvidia.com>; Tarun Gupta (SW-GPU) <targupta@nvidia.com>;
> Vikram Sethi <vsethi@nvidia.com>; Andy Currid <acurrid@nvidia.com>;
> Dheeraj Nigam <dnigam@nvidia.com>; Uday Dhoke <udhoke@nvidia.com>;
> qemu-arm@nongnu.org; qemu-devel@nongnu.org; Dave Jiang
> <dave.jiang@intel.com>
> Subject: Re: [PATCH v2 1/3] qom: new object to associate device to numa
> node
>
>
> On Sun, 8 Oct 2023 01:47:38 +0530
> <ankita@nvidia.com> wrote:
>
> > From: Ankit Agrawal <ankita@nvidia.com>
> >
> > The CPU cache coherent device memory can be added as NUMA nodes
> > distinct from the system memory nodes. These nodes are associated with
> > the device and Qemu needs a way to maintain this link.
>
> Hi Ankit,
>
> I'm not sure I'm convinced of the approach to creating nodes for memory
> usage (or whether that works in Linux on all NUMA ACPI archs), but I am
> keen to see Generic Initiator support in QEMU. I'd also like to see it done
> in a
> way that naturally extends to Generic Ports which are very similar (but don't
> hang memory off them! :) Dave Jiang posted a PoC a while back for generic
> ports.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fqemu-
> devel%2F168185633821.899932.322047053764766056.stgit%40djiang5-
> mobl3%2F&data=05%7C01%7Cvsethi%40nvidia.com%7C846b19f87bc5424d
> c33608dbc8c3015d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7
> C638324512146712954%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&sdata=v318MXognoITHyv7AFqZAfvUi2hLy2ZUVnLvyQ2IAfY%3D&res
> erved=0
>
> My concern with this approach is that it is using a side effect of a Linux
> implementation detail that the infra structure to bring up coherent memory
> is all present even for a GI only node (if it is which I can't recall) I'm
> also fairly
> sure we never tidied up the detail of going from the GI to the device in Linux
> (because it's harder than a _PXM entry for the device). It requires stashing
> a
> better description than the BDF before potentially doing reenumeration so
> that we can rebuild the association after that is done.
>
I'm not sure I understood the concern. Are you suggesting that the ACPI
specification
somehow prohibits memory associated with a GI node in the same PXM? i.e whether
the GI is memory-less
or with memory isn't mandated by the spec IIRC. Certainly seems perfectly normal
for an accelerator with memory to have a GI with memory and that memory be able
to be associated with the same PXM.
So what about this patch is using a Linux implementation detail? Even if Linux
wasn't currently supporting
that use case, it is something that would have been reasonable to add IMO. What
am I missing?
> >
> > Introduce a new acpi-generic-initiator object to allow host admin
> > provide the device and the corresponding NUMA node. Qemu maintain
> this
> > association and use this object to build the requisite GI Affinity
> > Structure.
> >
> > The admin provides the id of the device and the NUMA node id such as
> > in the following example.
> > -device
> > vfio-pci-nohotplug,host=<bdf>,bus=pcie.0,addr=04.0,rombar=0,id=dev0 \
> > -object acpi-generic-initiator,id=gi0,device=dev0,node=2 \
>
> This seems more different to existing numa configuration than necessary.
> The corner you have here of multiple GIs per PCI device is I hope the corner
> case so I'd like to see something that is really simple for single device,
> single
> node. Note that, whilst we don't do CXL 1.1 etc in QEMU yet, all CXL
> accelerators before CXL 2.0 are pretty much expected to present a GI SRAT
> entry + SRAT memory entry if appropriate.
> For CXL 2.0 and later everything can be left to be discovered by the OS but
> those Generic Ports I mentioned are an important part of that.
>
> Why not something like
> -numa node,gi=dev0 \
> -numa node,gi=dev0 \
> etc or maybe even the slightly weird
> -numa node,gi=dev0,gi=dev0,gi=dev0...
>
> ?
>
> >
> > Signed-off-by: Ankit Agrawal <ankita@nvidia.com>
> > ---
> > hw/acpi/acpi-generic-initiator.c | 74 ++++++++++++++++++++++++
> > hw/acpi/meson.build | 1 +
> > include/hw/acpi/acpi-generic-initiator.h | 30 ++++++++++
> > qapi/qom.json | 20 ++++++-
> > 4 files changed, 123 insertions(+), 2 deletions(-) create mode
> > 100644 hw/acpi/acpi-generic-initiator.c create mode 100644
> > include/hw/acpi/acpi-generic-initiator.h
> >
> > diff --git a/hw/acpi/acpi-generic-initiator.c
> > b/hw/acpi/acpi-generic-initiator.c
> > new file mode 100644
> > index 0000000000..6406736090
> > --- /dev/null
> > +++ b/hw/acpi/acpi-generic-initiator.c
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights
> > +reserved */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/qdev-properties.h"
> > +#include "qapi/error.h"
> > +#include "qapi/visitor.h"
> > +#include "qom/object_interfaces.h"
> > +#include "qom/object.h"
> > +#include "hw/qdev-core.h"
> > +#include "hw/vfio/vfio-common.h"
> > +#include "hw/vfio/pci.h"
> > +#include "hw/pci/pci_device.h"
> > +#include "sysemu/numa.h"
> > +#include "hw/acpi/acpi-generic-initiator.h"
> > +
> > +OBJECT_DEFINE_TYPE_WITH_INTERFACES(AcpiGenericInitiator,
> acpi_generic_initiator,
> > + ACPI_GENERIC_INITIATOR, OBJECT,
> > + { TYPE_USER_CREATABLE },
> > + { NULL })
> > +
> > +OBJECT_DECLARE_SIMPLE_TYPE(AcpiGenericInitiator,
> > +ACPI_GENERIC_INITIATOR)
> > +
> > +static void acpi_generic_initiator_init(Object *obj) {
> > + AcpiGenericInitiator *gi = ACPI_GENERIC_INITIATOR(obj);
> > + gi->device = NULL;
> > + gi->node = MAX_NODES;
> > + gi->node_count = 1;
> > +}
> > +
> > +static void acpi_generic_initiator_finalize(Object *obj) {
> > + AcpiGenericInitiator *gi = ACPI_GENERIC_INITIATOR(obj);
> > +
> > + g_free(gi->device);
> > +}
> > +
> > +static void acpi_generic_initiator_set_device(Object *obj, const char
> *value,
> > + Error **errp) {
> > + AcpiGenericInitiator *gi = ACPI_GENERIC_INITIATOR(obj);
> > +
> > + gi->device = g_strdup(value);
> > +}
> > +
> > +static void acpi_generic_initiator_set_node(Object *obj, Visitor *v,
> > + const char *name, void *opaque,
> > + Error **errp) {
> > + AcpiGenericInitiator *gi = ACPI_GENERIC_INITIATOR(obj);
> > + uint32_t value;
> > +
> > + if (!visit_type_uint32(v, name, &value, errp)) {
> > + return;
> > + }
> > +
> > + if (value >= MAX_NODES) {
> > + return;
> > + }
> > +
> > + gi->node = value;
> > +}
> > +
> > +static void acpi_generic_initiator_class_init(ObjectClass *oc, void
> > +*data) {
> > + object_class_property_add_str(oc,
> ACPI_GENERIC_INITIATOR_DEVICE_PROP, NULL,
> > + acpi_generic_initiator_set_device);
> > + object_class_property_add(oc,
> ACPI_GENERIC_INITIATOR_NODE_PROP, "uint32",
> > + NULL, acpi_generic_initiator_set_node, NULL,
> > + NULL);
> > +}
> > diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build index
> > fc1b952379..22252836ed 100644
> > --- a/hw/acpi/meson.build
> > +++ b/hw/acpi/meson.build
> > @@ -5,6 +5,7 @@ acpi_ss.add(files(
> > 'bios-linker-loader.c',
> > 'core.c',
> > 'utils.c',
> > + 'acpi-generic-initiator.c',
> > ))
> > acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c',
> > 'cpu_hotplug.c'))
> > acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_false:
> > files('acpi-cpu-hotplug-stub.c')) diff --git
> > a/include/hw/acpi/acpi-generic-initiator.h
> > b/include/hw/acpi/acpi-generic-initiator.h
> > new file mode 100644
> > index 0000000000..e67e6e23b1
> > --- /dev/null
> > +++ b/include/hw/acpi/acpi-generic-initiator.h
> > @@ -0,0 +1,30 @@
> > +#ifndef ACPI_GENERIC_INITIATOR_H
> > +#define ACPI_GENERIC_INITIATOR_H
> > +
> > +#include "hw/mem/pc-dimm.h"
> > +#include "hw/acpi/bios-linker-loader.h"
> > +#include "qemu/uuid.h"
> > +#include "hw/acpi/aml-build.h"
> > +#include "qom/object.h"
> > +#include "qom/object_interfaces.h"
> > +
> > +#define TYPE_ACPI_GENERIC_INITIATOR "acpi-generic-initiator"
> > +
> > +#define ACPI_GENERIC_INITIATOR_DEVICE_PROP "device"
> > +#define ACPI_GENERIC_INITIATOR_NODE_PROP "node"
> > +
> > +typedef struct AcpiGenericInitiator {
> > + /* private */
> > + Object parent;
> > +
> > + /* public */
> > + char *device;
> > + uint32_t node;
> > + uint32_t node_count;
> > +} AcpiGenericInitiator;
> > +
> > +typedef struct AcpiGenericInitiatorClass {
> > + ObjectClass parent_class;
> > +} AcpiGenericInitiatorClass;
> > +
> > +#endif
> > diff --git a/qapi/qom.json b/qapi/qom.json index
> > fa3e88c8e6..86c87a161c 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -779,6 +779,20 @@
> > { 'struct': 'VfioUserServerProperties',
> > 'data': { 'socket': 'SocketAddress', 'device': 'str' } }
> >
> > +##
> > +# @AcpiGenericInitiatorProperties:
> > +#
> > +# Properties for acpi-generic-initiator objects.
> > +#
> > +# @device: the ID of the device to be associated with the node # #
> > +@node: the ID of the numa node # # Since: 8.0 ## { 'struct':
> > +'AcpiGenericInitiatorProperties',
> > + 'data': { 'device': 'str', 'node': 'uint32' } }
> > +
> > ##
> > # @RngProperties:
> > #
> > @@ -947,7 +961,8 @@
> > 'tls-creds-x509',
> > 'tls-cipher-suites',
> > { 'name': 'x-remote-object', 'features': [ 'unstable' ] },
> > - { 'name': 'x-vfio-user-server', 'features': [ 'unstable' ] }
> > + { 'name': 'x-vfio-user-server', 'features': [ 'unstable' ] },
> > + 'acpi-generic-initiator'
> > ] }
> >
> > ##
> > @@ -1014,7 +1029,8 @@
> > 'tls-creds-x509': 'TlsCredsX509Properties',
> > 'tls-cipher-suites': 'TlsCredsProperties',
> > 'x-remote-object': 'RemoteObjectProperties',
> > - 'x-vfio-user-server': 'VfioUserServerProperties'
> > + 'x-vfio-user-server': 'VfioUserServerProperties',
> > + 'acpi-generic-initiator': 'AcpiGenericInitiatorProperties'
> > } }
> >
> > ##