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

[Ankit Agrawal]

>> +static void build_srat_generic_initiator_affinity(GArray *table_data, int 
>> node,
>> +                                                  PCIDeviceHandle *handle,
>> +                                                  GenericAffinityFlags 
>> flags)
>> +{
>> +    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 
>> */
>> +    build_append_int_noprefix(table_data, node, 4);  /* Proximity Domain */
>> +    build_append_int_noprefix(table_data, handle->segment, 2);
>> +    build_append_int_noprefix(table_data, handle->bdf, 2);
>> +    build_append_int_noprefix(table_data, handle->res0, 4);
>> +    build_append_int_noprefix(table_data, handle->res1, 8);
>
> Why are we storing reserved fields in the PCIDeviceHandle?  This
> function is already specific to building a PCI Device Handle, so we
> could just loop build_append_byte() with a fixed zero value here.

Good point, will make the change.


>> +void build_srat_generic_initiator(GArray *table_data)
>> +{
>> +    GSList *gi_list, *list = acpi_generic_initiator_get_list();
>> +    for (gi_list = list; gi_list; gi_list = gi_list->next) {
>> +        AcpiGenericInitiator *gi = gi_list->data;
>> +        Object *o;
>> +        int count;
>> +
>> +        if (gi->node == MAX_NODES) {
>> +            continue;
>> +        }
>
> Why do we have uninitialized AcpiGenericInitiator objects lingering?

Right, we don't need the check.

>> +
>> +        o = object_resolve_path_type(gi->device, TYPE_VFIO_PCI_NOHOTPLUG, 
>> NULL);
>
> TYPE_PCI_DEVICE?  Maybe you could check hotpluggable from the device
> class, but certainly the generic code should not be dependent on being
> a vfio-pci-nohotplug device.  

Understood.

> The spec also supports an ACPI object
> description, so should this be build_srat_generic_pci_initiator()?

Sure, makes sense.

>> +        if (!o) {
>> +            continue;
>> +        }
>> +
>> +        for (count = 0; count < gi->node_count; count++) {
>> +            PCIDeviceHandle dev_handle = {0};
>> +            PCIDevice *pci_dev = PCI_DEVICE(o);
>> +
>> +            dev_handle.bdf = pci_dev->devfn;
>
> Where does the bus part of the bdf get filled in?

Good catch, should have code to added the bus.

>> +            build_srat_generic_initiator_affinity(table_data,
>> +                                                  gi->node + count, 
>> &dev_handle,
>> +                                                  GEN_AFFINITY_ENABLED);
>
> Seems like the code that built the AcpiGenericInitiator object should
> supply the flags.  In fact the flag GEN_AFFINITY_ENABLED might be a
> better indicator to populate the SRAT with the GI than the node value.

Yeah, sure.