Re: [PATCH v7 2/2] hw/acpi: Implement the SRAT GI affinity structure

[Ankit Agrawal]

Thanks Jonathan for reviewing the change.

Comments inline.

>> The structure needs a PCI device handle [2] that consists of the device BDF.
>> The vfio-pci device corresponding to the acpi-generic-initiator object is
>> located to determine the BDF.
>>
>> [1] ACPI Spec 6.3, Section 5.2.16.6
>> [2] ACPI Spec 6.3, Table 5.80
>>
>> Signed-off-by: Ankit Agrawal <ankita@nvidia.com>
>Hi Ankit,
>
> As the code stands the use of a list seems overkill.

Yeah, I will try out your suggestion.

> Otherwise looks good to me.  I need Generic Ports support for CXL
> stuff so will copy your approach for that as it's ended up nice
> and simple.
> 
> Jonathan

Nice, would be good to have uniform implementations.

>> +/*
>> + * Identify Generic Initiator objects and link them into the list which is
>> + * returned to the caller.
>> + *
>> + * Note: it is the caller's responsibility to free the list to avoid
>> + * memory leak.
>> + */
>> +static GSList *acpi_generic_initiator_get_list(void)
>> +{
>> +    GSList *list = NULL;
>> +
>> +    object_child_foreach(object_get_root(),
>> +                         acpi_generic_initiator_list, &list);
>
> I think you can use object_child_foreach_recursive() and skip the manual
> calling above?

Great suggestion, will give that a try.

>> + * ACPI 6.3:
>> + * Table 5-78 Generic Initiator Affinity Structure
>> + */
>> +static void
>> +build_srat_generic_pci_initiator_affinity(GArray *table_data, int node,
>> +                                          PCIDeviceHandle *handle)
>> +{
>> +    uint8_t index;
>> +
>> +    build_append_int_noprefix(table_data, 5, 1);  /* Type */
>> +    build_append_int_noprefix(table_data, 32, 1); /* Length */
>> +    build_append_int_noprefix(table_data, 0, 1);  /* Reserved */
>> +    build_append_int_noprefix(table_data, 1, 1);  /* Device Handle Type: 
>> PCI */
>> +    build_append_int_noprefix(table_data, node, 4);  /* Proximity Domain */
>> +
>> +    /* Device Handle - PCI */
>> +    build_append_int_noprefix(table_data, handle->segment, 2);
>> +    build_append_int_noprefix(table_data, handle->bdf, 2);
>> +    for (index = 0; index < 12; index++) {
>> +        build_append_int_noprefix(table_data, 0, 1);
>> +    }
>> +
>> +    build_append_int_noprefix(table_data, GEN_AFFINITY_ENABLED, 4); /* 
>> Flags */
>> +    build_append_int_noprefix(table_data, 0, 4);     /* Reserved */
>> +}
>> +
>> +void build_srat_generic_pci_initiator(GArray *table_data)
>> +{
>> +    GSList *gi_list, *list = acpi_generic_initiator_get_list();
>
>
> Did you consider just have the functional called in the scan do this?
> Not sure you need anything as a parameter beyond the GArray *table_data
>
> Something like...
>
> static int acpi_generic_initiator_list(Object *obj, void *opaque)
> {
>    uint8_t index;
>    AcpiGenericInitiator *gi;
>    GArray *table_data = opaque;
>    PCIDeviceHandle dev_handle;
>    PCIDevice *pci_dev;
>    Object *o;
>
>    if (!object_dynamic_cast(obj, TYPE_ACPI_GENERIC_INITIATOR)) {
>        return 0;
>    }
>
>    gi = ACPI_GENERIC_INITIATOR(obj);
>    o = object_resolve_path_type(gi->pci_dev, TYPE_PCI_DEVICE, NULL);
>    if (!o) {
>        error_setg(&error_abort, "GI: Specified device must be a PCI 
>device.\n")
>        return 1;
>    }
>    pci_dev = PCI_DEVICE(o);
>
>    dev_handle.segment = 0;
>    dev_handle.bdf = PCI_BUILD_BDF(pci_bus_num(pci_get_bus(pci_dev)),
>                                               pci_dev->devfn);
>    build_srat_generic_pci_initiator_affinity(table_data,
>                                              gi->node, &dev_handle);
> }
>
> + a call to.
>    object_child_foreach_recursive(object_get_root(),
>                                   acpi_generic_srat, table_data);
>

Thanks. That does seem better.

>> +        pci_dev = PCI_DEVICE(o);
>> +
>> +        dev_handle.segment = 0;
>> +        dev_handle.bdf = PCI_BUILD_BDF(pci_bus_num(pci_get_bus(pci_dev)),
>> +                                                   pci_dev->devfn);
>> +        build_srat_generic_pci_initiator_affinity(table_data,
>> +                                                  gi->node, &dev_handle);
> Should we check for consistency of gi->node and
> -numa node,id=X entries?
>
> Maybe just check less than numa_state->num_nodes as that's the variable
> used to walk the other structures when building srat.

Ack, will add a check to compare against numa_state->num_nodes.

>> +/*
>> + * ACPI 6.3:
>> + * Table 5-81 Flags – Generic Initiator Affinity Structure
>> + */
>> +typedef enum {
>> +    GEN_AFFINITY_ENABLED = (1 << 0), /*
>> +                                      * If clear, the OSPM ignores the 
>> contents
>> +                                      * of the Generic Initiator/Port 
>> Affinity
>> +                                      * Structure. This allows system 
>> firmware
>> +                                      * to populate the SRAT with a static
>> +                                      * number of structures, but only 
>> enable
>> +                                      * them as necessary.
>> +                                      */
> I'd put the comment above the definition to avoid wrapping so much!

Yeah, it does look kind of silly. Will fix it.

>> +} GenericAffinityFlags;
>> +