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

[Ankit Agrawal]

>> +        for (l = gi->nodelist; l; l = l->next) {
>> +            PCIDeviceHandle dev_handle = {0};
>> +            PCIDevice *pci_dev = PCI_DEVICE(o);
>> +            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,
>> +                                                      l->value, 
>> &dev_handle);
>> +        }
>> +    }
>
> if you never initialize segment then I don't see why have it.
> It's just the bdf, just pass that as parameter no need for a struct.
>
> I'd explicitly set the segment to zero just to make it more apparent
> that it would need to be addressed when QEMU adds multi-segment
> support.

Okay, so I'll keep the segment id, but set it to 0 explicitly.

>> + * ACPI spec, Revision 6.5
>
> we normally just say ACPI 6.5 even though a couple of places are more
> verbose.
>
> In ACPI we document *earliest* spec version that includes this, not just
> a random one you looked at. I checked 6.3 and it's there.
> Pls find earliest one.

Will make the change.

>> +typedef enum {
>> +    GEN_AFFINITY_NOFLAGS = 0,
>> +    GEN_AFFINITY_ENABLED = (1 << 0),
>> +    GEN_AFFINITY_ARCH_TRANS = (1 << 1),
>> +} GenericAffinityFlags;
>
> Don't add these one-time use flags. They are impossible to match to
> spec without reading and memorizing all of it. The way we do it in ACPI
> code is this:
>
> (1 << 0) /* [text matching ACPI spec verbatim ] */
>
> this also means you will not add a ton of dead code just because it is
> in the spec.

Ack.

>> +typedef struct PCIDeviceHandle {
>> +    uint16_t segment;
>> +    uint16_t bdf;
>> +    uint8_t res[12];
>
> what is this "res" and why do you need to pass it? It's always 0 isn't
> it?

It is 12 bytes reserved field in the "Device Handle - PCI" described in 
ACPI 6.5, Table 5.66. I'll remove it.

>> +
>> +        o = object_resolve_path_type(gi->device, TYPE_VFIO_PCI, NULL);
>
> As per previous comments, this should not be tied to vfio.  This should
> be able to describe an association between any PCI device and various
> proximity domains, even those beyond this current use case.

Sure, will change it to use TYPE_PCI_DEVICE.

> It also looks like this support just silently fails if the device
> string isn't the right type or isn't found.  That's not good.  Should
> the previous patch validate the device where the Error return is more
> readily available rather than only doing a strdup there?  Maybe then we
> should store the object there rather than a char buffer.

AFAIU in a normal flow currently, a qemu -object is (parsed and) created much
earlier that a -device. This complicates the situation as when the
acpi-generic-initiator object is being created, the device is not available for
error check. Maybe I should treat this object specially to create much later?

> Don't we also still need to enforce that the device is not hotpluggable
> since we're tying it to this fixed ACPI object?  That was implicit when
> previously testing for the non-hotpluggable vfio-pci device type, but
> should rely on something like device_get_hotpluggable() now.

I think this will be similarly problematic as above due to the sequence of
object creation.

> Also the ACPI Generic Initiator supports either a PCI or ACPI device
> handle, where we're only adding PCI support here.  What do we want ACPI
> device support to look like?  Is it sufficient that device= only
> accepts a PCI device now and fails on anything else and would later be
> updated to accept an ACPI device or should the object have different
> entry points, ex. pci_dev = vs acpi_dev= where it might later be
> introspected whether ACPI device support exists?

I am fine with either way. If we prefer different entry points, I can make the
change.