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 <ank...@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;
>> +
