Re: [PATCH for-5.0 v11 05/20] virtio-iommu: Endpoint and domains structs and helpers

[Jean-Philippe Brucker]

On Thu, Dec 19, 2019 at 07:31:08PM +0100, Auger Eric wrote:
> Hi Jean,
> 
> On 12/10/19 5:37 PM, Jean-Philippe Brucker wrote:
> > On Fri, Nov 22, 2019 at 07:29:28PM +0100, Eric Auger wrote:
> >> +typedef struct viommu_domain {
> >> +    uint32_t id;
> >> +    GTree *mappings;
> >> +    QLIST_HEAD(, viommu_endpoint) endpoint_list;
> >> +} viommu_domain;
> >> +
> >> +typedef struct viommu_endpoint {
> >> +    uint32_t id;
> >> +    viommu_domain *domain;
> >> +    QLIST_ENTRY(viommu_endpoint) next;
> >> +} viommu_endpoint;
> > 
> > There might be a way to merge viommu_endpoint and the IOMMUDevice
> > structure introduced in patch 4, since they both represent one endpoint.
> > Maybe virtio_iommu_find_add_pci_as() could add the IOMMUDevice to
> > s->endpoints, and IOMMUDevice could store the endpoint ID rather than bus
> > and devfn.
> 
> On PCI bus enumeration we locally store the PCI bus hierarchy under the
> form of GHashTable of IOMMUDevice indexed by iommu_pci_bus pointer.
> Those are all the devices attached to the downstream buses. We also use
> an array of iommu pci bus pointers indexed by bus number that is lazily
> populated due to the fact, at enumeration time we do know the bus number
> yet. As you pointed, I haven't used the array of iommu pci bus pointers
> indexed by bus number in this series and I should actually. Currently I
> am not checking on attach that the sid effectively corresponds to a sid
> protected by this iommu. I will add this in my next version. The above
> structures are used in intel_iommu and smmu code as well and I think
> eventually this may be factorized a common base class..
> 
> on the other hand the gtree of viommu_endpoint - soon renamed in
> CamelCase form ;-) - corresponds to the EPs that are actually attached
> to any domain. It is indexed by sid and not by bus pointer. This is more
> adapted to the virtio-iommu case.
> 
> So, despite your suggestion, I am tempted to keep the different
> structures as the first ones are common to all iommu emulation code and
> the last is adapted to the virtio-iommu operations.
> 
> Thoughts?

Makes sense, it seems better to keep them separate. I had missed that the
PCI bus number is resolved later, and started to move the endpoint ID into
IOMMUDevice when adding MMIO support, but I'll need to revisit this.

I'll be off for two weeks, have a nice holiday!

Thanks,
Jean