diff mbox series

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

Message ID 20230622214845.3980-7-joao.m.martins@oracle.com (mailing list archive)
State New, archived
Headers show
Series vfio: VFIO migration support with vIOMMU | expand

Commit Message

Joao Martins June 22, 2023, 9:48 p.m. UTC 
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(+)

Comments

Duan, Zhenzhong Sept. 8, 2023, 6:23 a.m. UTC | #1 
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 = {
Joao Martins Sept. 8, 2023, 10:11 a.m. UTC | #2 
On 08/09/2023 07:23, Duan, Zhenzhong wrote:
> 
> 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?
> 

There's an user, and that's VFIO, but it's later in the series. It felt more
natural for bisection to do things this way.
Cédric Le Goater Oct. 2, 2023, 3:23 p.m. UTC | #3 
On 6/22/23 23:48, 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.

This looks like two patches to me and the previous should be merged with
the one introducing vtd_iommu_get_attr().

Thanks,

C.

> 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;
>   }
>   
>   static const TypeInfo vtd_iommu_memory_region_info = {
Joao Martins Oct. 6, 2023, 8:42 a.m. UTC | #4 
On 02/10/2023 16:23, Cédric Le Goater wrote:
> On 6/22/23 23:48, 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.
> 
> This looks like two patches to me and the previous should be merged with
> the one introducing vtd_iommu_get_attr().
> 

The reason was that one is a bit useless without the other. But I can spread
into two patches. And I can merge the previous to the one introducing
vtd_iommu_get_attr()

> Thanks,
> 
> C.
> 
>> 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;
>>   }
>>     static const TypeInfo vtd_iommu_memory_region_info = {
>
diff mbox series

Patch

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;
 }
 
 static const TypeInfo vtd_iommu_memory_region_info = {