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

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 8/18/26 1:25 PM, Konstantin Shkolnyy wrote:
> Because S390PCIIOMMU has just been cleaned up of fields it didn't have to
> carry, add a comment explaining why it exists in the first place and
> discouraging adding more fields to it in the future.

In following the theme with the other patches, how about a re-word to
something like:

Now that S390PCIIOMMU had been reduced to only the IOMMU container
information needed by the PCI layer for the associated slot, add a
comment explaining the purpose of the structure.

> 
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> ---
>  include/hw/s390x/s390-pci-bus.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
> index e3cbee2695..17ecf3e0da 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -271,6 +271,17 @@ 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. QEMU wants to have this "root" AddressSpace before the PCI
> + * device is plugged in. It's only actually used for I/O while the PCI
> + * device is plugged in and configured by the guest, at which time it has
> + * additional memory subregions from zPCI device, that can do real work.
> + * In other words, S390PCIIOMMU holds fields that logically belong to
> + * S390PCIBusDevice but need to exist before S390PCIBusDevice exists.
> + * Therefore, no other kind of fields should be placed in S390PCIIOMMU.
> + */


Starting from "In other words," your comment gets inaccurate.  We will
actually ensure that a S390PCIBusDevice is either 1) already created
prior to the PCIDevice or 2) will create it on-demand during
s390_pcihost_plug() for TYPE_PCI_DEVICE if it didn't already exist.

This nuance goes back to my comment earlier about the lifecycle being a
bit odd.

For now I would suggest to just drop the last 3 lines starting with "In
other words, ..."

With those 3 lines removed and the commit message re-worded:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>