>> + 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.
