Re: [PATCH v8 1/2] qom: new object to associate device to NUMA node

[Ankit Agrawal]

>> -object acpi-generic-initiator,id=gi14,pci-dev=dev1,node=16 \
>> -object acpi-generic-initiator,id=gi15,pci-dev=dev1,node=17 \
>> 
>> The performance benefits can be realized by providing the NUMA node distances
>> appropriately (through libvirt tags or Qemu params). The admin can get the
>> distance among nodes in hardware using `numactl -H`.
>
> That's a lot of description when you could just have claimed you want a normal
> GI node for HMAT and we'd have all believed you ;)

Ack, I'll remove this part and change it to say as such.

>> 
>> Link: https://www.nvidia.com/en-in/technologies/multi-instance-gpu [1]
>> Cc: Jonathan Cameron <qemu-devel@nongnu.org>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Acked-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Ankit Agrawal <ankita@nvidia.com>
>
> Hi Ankit,
>
> Some minor things inline. With the includes tidied up.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

>> diff --git a/include/hw/acpi/acpi_generic_initiator.h 
>> b/include/hw/acpi/acpi_generic_initiator.h
>> new file mode 100644
>> index 0000000000..23d0b591c6
>> --- /dev/null
>> +++ b/include/hw/acpi/acpi_generic_initiator.h
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
>> + */
>> +
>> +#ifndef ACPI_GENERIC_INITIATOR_H
>> +#define ACPI_GENERIC_INITIATOR_H
>> +
>> +#include "hw/mem/pc-dimm.h"
>
> Why?
>
>> +#include "hw/acpi/bios-linker-loader.h"
>> +#include "hw/acpi/aml-build.h"
>> +#include "sysemu/numa.h"
>
> This should only include headers that it uses directly.
> If they are needed down in the c files, then include them there.

Ack, will fix this in the next version.

>> +typedef struct AcpiGenericInitiator {
>> +    /* private */
>> +    Object parent;
>> +
>> +    /* public */
>> +    char *pci_dev;
>> +    uint16_t node;
>> +} AcpiGenericInitiator;
>> +
>> +typedef struct AcpiGenericInitiatorClass {
>> +    ObjectClass parent_class;
>> +} AcpiGenericInitiatorClass;
>
> Trivial, but you could push the class definition down into the c file
> given it's not accessed from anywhere else.

Sure will move the AcpiGenericInitiatorClass typedef to the .c file.

> +
> +#endif