Re: [PATCH v3 13/15] s390x/pci: Add a comment explaining S390PCIIOMMU purpose

[Farhan Ali <alifm@linux.ibm.com>]

On 6/19/2026 10:39 AM, Matthew Rosato wrote:
> On 6/4/26 10:17 PM, Konstantin Shkolnyy wrote:
>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> Commit message needs something here -- You don't need to include the
> explanation of the structure (that's in the commit itself) but maybe
> describe the context (e.g. you're adding the comment because you just
> moved most of the contents of the structure out in support of migration
> and are now describing the remaining purpose)
>
>> ---
>>   include/hw/s390x/s390-pci-bus.h | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
>> index e3cbee2695..2fab28e6e0 100644
>> --- a/include/hw/s390x/s390-pci-bus.h
>> +++ b/include/hw/s390x/s390-pci-bus.h
>> @@ -271,6 +271,14 @@ typedef struct S390PCIDMACount {
>>       QTAILQ_ENTRY(S390PCIDMACount) link;
>>   } S390PCIDMACount;
>>   
>> +/*
>> + * This structure holds the PCI device AddressSpace that QEMU needs to link
>> + * into its internal structures before the zPCI and PCI devices are fully
>> + * initialized. It's a QEMU requirement to provide this "root" AddressSpace
>> + * early. The AddressSpace is only actually used for I/O while the PCI
>> + * device is plugged in and configured by the guest, at which time it gets
>> + * additional memory subregions from zPCI device, that can do real work.
>> + */
> To be more clear, it's not that it is required 'early' so much as that
> it is required to be there at the time the PCI device is plugged.
>
> The lifecycle has always been a bit odd here; it is an attribute of the
> zPCI device, but is required by the PCI device in order to function and
> the necessary linkage between the 2 (PCI->zPCI) is established via the
> iommu_table in the S390pciState.  Would be good to work some of that
> into this explanation.

+1 I think we moving few fields from the struct, so having an 
explanation why do we need the remaining fields would be helpful. Also 
can this struct be extended in the future without any issues and 
breaking? If not then maybe adding something about would that would also 
be helpful.

Thanks

Farhan


>>   struct S390PCIIOMMU {
>>       Object parent_obj;
>>       AddressSpace as;