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

[Auger Eric <eric.auger@redhat.com>]

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?

Eric

> 
>> +typedef struct viommu_interval {
>> +    uint64_t low;
>> +    uint64_t high;
>> +} viommu_interval;
> 
> I guess these should be named in CamelCase? Although if we're allowed to
> choose my vote goes to underscores :)
> 
> Thanks,
> Jean
>