Re: [PATCH v4 06/15] intel-iommu: Implement get_attr() method

[Duan, Zhenzhong]

On 6/23/2023 5:48 AM, Joao Martins wrote:
> Implement IOMMU MR get_attr() method and use the dma_translation
> property to report the IOMMU_ATTR_DMA_TRANSLATION attribute.
> Additionally add the necessary get_iommu_attr into the PCIIOMMUOps to
> support pci_device_iommu_get_attr().
>
> The callback in there acts as a IOMMU-specific address space walker
> which will call get_attr in the IOMMUMemoryRegion backing the device to
> fetch the desired attribute.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   hw/i386/intel_iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1606d1b952d0..ed2a46e008df 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3861,6 +3861,29 @@ static void vtd_iommu_replay(IOMMUMemoryRegion 
> *iommu_mr, IOMMUNotifier *n)
>       return;
>   }
>   
> +static int vtd_iommu_get_attr(IOMMUMemoryRegion *iommu_mr,
> +                              enum IOMMUMemoryRegionAttr attr, void *data)
> +{
> +    VTDAddressSpace *vtd_as = container_of(iommu_mr, VTDAddressSpace, iommu);
> +    IntelIOMMUState *s = vtd_as->iommu_state;
> +    int ret = 0;
> +
> +    switch (attr) {
> +    case IOMMU_ATTR_DMA_TRANSLATION:
> +    {
> +        bool *enabled = (bool *)(uintptr_t) data;
> +
> +        *enabled = s->dma_translation;
> +        break;
> +    }
> +    default:
> +        ret = -EINVAL;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
>   /* Do the initialization. It will also be called when reset, so pay
>    * attention when adding new initialization stuff.
>    */
> @@ -4032,8 +4055,24 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, 
> void *opaque, int devfn)
>       return &vtd_as->as;
>   }
>   
> +static int vtd_get_iommu_attr(PCIBus *bus, void *opaque, int32_t devfn,
> +                              enum IOMMUMemoryRegionAttr attr, void *data)
> +{
> +    IntelIOMMUState *s = opaque;
> +    VTDAddressSpace *vtd_as;
> +
> +    assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
> +
> +    vtd_as = vtd_find_add_as(s, bus, devfn, PCI_NO_PASID);
> +    if (!vtd_as)
> +       return -EINVAL;
> +
> +    return memory_region_iommu_get_attr(&vtd_as->iommu, attr, data);
> +}
> +
>   static PCIIOMMUOps vtd_iommu_ops = {
>       .get_address_space = vtd_host_dma_iommu,
> +    .get_iommu_attr = vtd_get_iommu_attr,
>   };
>   
>   static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> @@ -4197,6 +4236,7 @@ static void 
> vtd_iommu_memory_region_class_init(ObjectClass *klass,
>       imrc->translate = vtd_iommu_translate;
>       imrc->notify_flag_changed = vtd_iommu_notify_flag_changed;
>       imrc->replay = vtd_iommu_replay;
> +    imrc->get_attr = vtd_iommu_get_attr;

Do we have other user of imrc->get_attr? If not, what about squashing

vtd_iommu_get_attr into vtd_get_iommu_attr and have imrc->get_attr removed?

Thanks
Zhenzhong

>   }
>   
>   static const TypeInfo vtd_iommu_memory_region_info = {