Re: [PATCH for-5.0 v11 04/20] virtio-iommu: Add the iommu regions

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

Hi Jean,

On 12/10/19 5:34 PM, Jean-Philippe Brucker wrote:
> Two small things below, but looks good overall
> 
> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> On Fri, Nov 22, 2019 at 07:29:27PM +0100, Eric Auger wrote:
>> +static AddressSpace *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
>> +                                              int devfn)
>> +{
>> +    VirtIOIOMMU *s = opaque;
>> +    IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>> +    static uint32_t mr_index;
>> +    IOMMUDevice *sdev;
>> +
>> +    if (!sbus) {
>> +        sbus = g_malloc0(sizeof(IOMMUPciBus) +
>> +                         sizeof(IOMMUDevice *) * IOMMU_PCI_DEVFN_MAX);
>> +        sbus->bus = bus;
>> +        g_hash_table_insert(s->as_by_busptr, bus, sbus);
>> +    }
>> +
>> +    sdev = sbus->pbdev[devfn];
>> +    if (!sdev) {
>> +        char *name = g_strdup_printf("%s-%d-%d",
>> +                                     TYPE_VIRTIO_IOMMU_MEMORY_REGION,
>> +                                     mr_index++, devfn);
>> +        sdev = sbus->pbdev[devfn] = g_malloc0(sizeof(IOMMUDevice));
>> +
>> +        sdev->viommu = s;
>> +        sdev->bus = bus;
>> +        sdev->devfn = devfn;
> 
> It might be better to store the endpoint ID in IOMMUDevice, then you could
> get rid of virtio_iommu_get_sid(), and remove a tiny bit of overhead in
> virtio_iommu_translate(). But I doubt it's significant.
virtio_iommu_find_add_as() gets called on PCI bus enumeration. At that
point, the bus number may not be resolved. So I cannot retrieve and set
the bus_number in this function.

When virtio_iommu_get_sid() is called we are sure pci_bus_num(dev->bus)
returns a correct value.
> 
> [...]
>> +static const TypeInfo virtio_iommu_memory_region_info = {
>> +    .parent = TYPE_IOMMU_MEMORY_REGION,
>> +    .name = TYPE_VIRTIO_IOMMU_MEMORY_REGION,
>> +    .class_init = virtio_iommu_memory_region_class_init,
>> +};
>> +
>> +
> 
> nit: newline.
Thanks

Eric
> 
> Thanks,
> Jean 
>