diff mbox series

[v5,18/20] intel_iommu: Introduce a property x-flts for scalable modern mode

Message ID 20241111083457.2090664-19-zhenzhong.duan@intel.com
State New
Headers show
Series intel_iommu: Enable stage-1 translation for emulated device | expand

Commit Message

Duan, Zhenzhong Nov. 11, 2024, 8:34 a.m. UTC 
Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
related to scalable mode translation, thus there are multiple combinations.

This vIOMMU implementation wants to simplify it with a new property "x-flts".
When enabled in scalable mode, first stage translation also known as scalable
modern mode is supported. When enabled in legacy mode, throw out error.

With scalable modern mode exposed to user, also accurate the pasid entry
check in vtd_pe_type_check().

Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu_internal.h |  2 ++
 hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------
 2 files changed, 21 insertions(+), 9 deletions(-)

Comments

CLEMENT MATHIEU--DRIF Nov. 19, 2024, 6:54 a.m. UTC | #1 
Hi zhenzhong,
Just one comment but you can add Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>


On 11/11/2024 09:34, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
> related to scalable mode translation, thus there are multiple combinations.
>
> This vIOMMU implementation wants to simplify it with a new property "x-flts".
> When enabled in scalable mode, first stage translation also known as scalable
> modern mode is supported. When enabled in legacy mode, throw out error.
>
> With scalable modern mode exposed to user, also accurate the pasid entry
> check in vtd_pe_type_check().
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h |  2 ++
>   hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------
>   2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 2c977aa7da..e8b211e8b0 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -195,6 +195,7 @@
>   #define VTD_ECAP_PASID              (1ULL << 40)
>   #define VTD_ECAP_SMTS               (1ULL << 43)
>   #define VTD_ECAP_SLTS               (1ULL << 46)
> +#define VTD_ECAP_FLTS               (1ULL << 47)
>
>   /* CAP_REG */
>   /* (offset >> 4) << 24 */
> @@ -211,6 +212,7 @@
>   #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
>   #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
>   #define VTD_CAP_DRAIN_READ          (1ULL << 55)
> +#define VTD_CAP_FS1GP               (1ULL << 56)
>   #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE)
>   #define VTD_CAP_CM                  (1ULL << 7)
>   #define VTD_PASID_ID_SHIFT          20
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index b921793c3a..a7a81aebee 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -803,16 +803,18 @@ static inline bool vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)
>   }
>
>   /* Return true if check passed, otherwise false */
> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
> -                                     VTDPASIDEntry *pe)
> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)
>   {
>       switch (VTD_PE_GET_TYPE(pe)) {
> -    case VTD_SM_PASID_ENTRY_SLT:
> -        return true;
> -    case VTD_SM_PASID_ENTRY_PT:
> -        return x86_iommu->pt_supported;
>       case VTD_SM_PASID_ENTRY_FLT:
> +        return !!(s->ecap & VTD_ECAP_FLTS);
> +    case VTD_SM_PASID_ENTRY_SLT:
> +        return !!(s->ecap & VTD_ECAP_SLTS);
>       case VTD_SM_PASID_ENTRY_NESTED:
> +        /* Not support NESTED page table type yet */
> +        return false;
> +    case VTD_SM_PASID_ENTRY_PT:
> +        return !!(s->ecap & VTD_ECAP_PT);
>       default:
>           /* Unknown type */
>           return false;
> @@ -861,7 +863,6 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>       uint8_t pgtt;
>       uint32_t index;
>       dma_addr_t entry_size;
> -    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>
>       index = VTD_PASID_TABLE_INDEX(pasid);
>       entry_size = VTD_PASID_ENTRY_SIZE;
> @@ -875,7 +876,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>       }
>
>       /* Do translation type check */
> -    if (!vtd_pe_type_check(x86_iommu, pe)) {
> +    if (!vtd_pe_type_check(s, pe)) {
>           return -VTD_FR_PASID_TABLE_ENTRY_INV;
>       }
>
> @@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {
>                         VTD_HOST_ADDRESS_WIDTH),
>       DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
>       DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> +    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),
>       DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
>       DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
>       DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> @@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)
>       }
>
>       /* TODO: read cap/ecap from host to decide which cap to be exposed. */
> -    if (s->scalable_mode) {
> +    if (s->scalable_modern) {
> +        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;
> +        s->cap |= VTD_CAP_FS1GP;
> +    } else if (s->scalable_mode) {
>           s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
>       }
>
> @@ -4737,6 +4742,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>           }
>       }
>
> +    if (!s->scalable_mode && s->scalable_modern) {
> +        error_setg(errp, "Legacy mode: not support x-flts=on");
The error message should be "x-flts=on not supported in legacy mode" or 
even "x-flts is only available in scalable mode" as there is no FLT in 
legacy mode
> +        return false;
> +    }
> +
>       if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT &&
>           s->aw_bits != VTD_HOST_AW_48BIT) {
>           error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",
> --
> 2.34.1
>
Duan, Zhenzhong Nov. 19, 2024, 7:28 a.m. UTC | #2 
Hi Clement,

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for
>scalable modern mode
>
>Hi zhenzhong,
>Just one comment but you can add Reviewed-by: Clément Mathieu--
>Drif<clement.mathieu--drif@eviden.com>
>
>
>On 11/11/2024 09:34, Zhenzhong Duan wrote:
>> Caution: External email. Do not open attachments or click links, unless this
>email comes from a known sender and you know the content is safe.
>>
>>
>> Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
>> related to scalable mode translation, thus there are multiple combinations.
>>
>> This vIOMMU implementation wants to simplify it with a new property "x-flts".
>> When enabled in scalable mode, first stage translation also known as scalable
>> modern mode is supported. When enabled in legacy mode, throw out error.
>>
>> With scalable modern mode exposed to user, also accurate the pasid entry
>> check in vtd_pe_type_check().
>>
>> Suggested-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  2 ++
>>   hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------
>>   2 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index 2c977aa7da..e8b211e8b0 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -195,6 +195,7 @@
>>   #define VTD_ECAP_PASID              (1ULL << 40)
>>   #define VTD_ECAP_SMTS               (1ULL << 43)
>>   #define VTD_ECAP_SLTS               (1ULL << 46)
>> +#define VTD_ECAP_FLTS               (1ULL << 47)
>>
>>   /* CAP_REG */
>>   /* (offset >> 4) << 24 */
>> @@ -211,6 +212,7 @@
>>   #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
>>   #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
>>   #define VTD_CAP_DRAIN_READ          (1ULL << 55)
>> +#define VTD_CAP_FS1GP               (1ULL << 56)
>>   #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ |
>VTD_CAP_DRAIN_WRITE)
>>   #define VTD_CAP_CM                  (1ULL << 7)
>>   #define VTD_PASID_ID_SHIFT          20
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index b921793c3a..a7a81aebee 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -803,16 +803,18 @@ static inline bool
>vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)
>>   }
>>
>>   /* Return true if check passed, otherwise false */
>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
>> -                                     VTDPASIDEntry *pe)
>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)
>>   {
>>       switch (VTD_PE_GET_TYPE(pe)) {
>> -    case VTD_SM_PASID_ENTRY_SLT:
>> -        return true;
>> -    case VTD_SM_PASID_ENTRY_PT:
>> -        return x86_iommu->pt_supported;
>>       case VTD_SM_PASID_ENTRY_FLT:
>> +        return !!(s->ecap & VTD_ECAP_FLTS);
>> +    case VTD_SM_PASID_ENTRY_SLT:
>> +        return !!(s->ecap & VTD_ECAP_SLTS);
>>       case VTD_SM_PASID_ENTRY_NESTED:
>> +        /* Not support NESTED page table type yet */
>> +        return false;
>> +    case VTD_SM_PASID_ENTRY_PT:
>> +        return !!(s->ecap & VTD_ECAP_PT);
>>       default:
>>           /* Unknown type */
>>           return false;
>> @@ -861,7 +863,6 @@ static int
>vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>       uint8_t pgtt;
>>       uint32_t index;
>>       dma_addr_t entry_size;
>> -    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>>
>>       index = VTD_PASID_TABLE_INDEX(pasid);
>>       entry_size = VTD_PASID_ENTRY_SIZE;
>> @@ -875,7 +876,7 @@ static int
>vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>       }
>>
>>       /* Do translation type check */
>> -    if (!vtd_pe_type_check(x86_iommu, pe)) {
>> +    if (!vtd_pe_type_check(s, pe)) {
>>           return -VTD_FR_PASID_TABLE_ENTRY_INV;
>>       }
>>
>> @@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {
>>                         VTD_HOST_ADDRESS_WIDTH),
>>       DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,
>FALSE),
>>       DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode,
>FALSE),
>> +    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),
>>       DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control,
>false),
>>       DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
>>       DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
>> @@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)
>>       }
>>
>>       /* TODO: read cap/ecap from host to decide which cap to be exposed. */
>> -    if (s->scalable_mode) {
>> +    if (s->scalable_modern) {
>> +        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;
>> +        s->cap |= VTD_CAP_FS1GP;
>> +    } else if (s->scalable_mode) {
>>           s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
>>       }
>>
>> @@ -4737,6 +4742,11 @@ static bool vtd_decide_config(IntelIOMMUState *s,
>Error **errp)
>>           }
>>       }
>>
>> +    if (!s->scalable_mode && s->scalable_modern) {
>> +        error_setg(errp, "Legacy mode: not support x-flts=on");
>The error message should be "x-flts=on not supported in legacy mode" or
>even "x-flts is only available in scalable mode" as there is no FLT in
>legacy mode

OK, will do.
But I'm not quite clear of the difference between
"Legacy mode: not support x-flts=on" and "x-flts=on not supported in legacy mode".
Is it because the later looks more formal or the former has ambiguity?

Thanks
Zhenzhong
CLEMENT MATHIEU--DRIF Nov. 19, 2024, 8:59 a.m. UTC | #3 
On 19/11/2024 08:28, Duan, Zhenzhong wrote:

Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.


Hi Clement,



-----Original Message-----
From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.com>
Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for
scalable modern mode

Hi zhenzhong,
Just one comment but you can add Reviewed-by: Clément Mathieu--
Drif<clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.com>


On 11/11/2024 09:34, Zhenzhong Duan wrote:


Caution: External email. Do not open attachments or click links, unless this


email comes from a known sender and you know the content is safe.




Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
related to scalable mode translation, thus there are multiple combinations.

This vIOMMU implementation wants to simplify it with a new property "x-flts".
When enabled in scalable mode, first stage translation also known as scalable
modern mode is supported. When enabled in legacy mode, throw out error.

With scalable modern mode exposed to user, also accurate the pasid entry
check in vtd_pe_type_check().

Suggested-by: Jason Wang <jasowang@redhat.com><mailto:jasowang@redhat.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com><mailto:yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com><mailto:yi.y.sun@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com><mailto:zhenzhong.duan@intel.com>
---
  hw/i386/intel_iommu_internal.h |  2 ++
  hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------
  2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 2c977aa7da..e8b211e8b0 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -195,6 +195,7 @@
  #define VTD_ECAP_PASID              (1ULL << 40)
  #define VTD_ECAP_SMTS               (1ULL << 43)
  #define VTD_ECAP_SLTS               (1ULL << 46)
+#define VTD_ECAP_FLTS               (1ULL << 47)

  /* CAP_REG */
  /* (offset >> 4) << 24 */
@@ -211,6 +212,7 @@
  #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
  #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
  #define VTD_CAP_DRAIN_READ          (1ULL << 55)
+#define VTD_CAP_FS1GP               (1ULL << 56)
  #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ |


VTD_CAP_DRAIN_WRITE)


  #define VTD_CAP_CM                  (1ULL << 7)
  #define VTD_PASID_ID_SHIFT          20
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b921793c3a..a7a81aebee 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -803,16 +803,18 @@ static inline bool


vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)


  }

  /* Return true if check passed, otherwise false */
-static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
-                                     VTDPASIDEntry *pe)
+static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)
  {
      switch (VTD_PE_GET_TYPE(pe)) {
-    case VTD_SM_PASID_ENTRY_SLT:
-        return true;
-    case VTD_SM_PASID_ENTRY_PT:
-        return x86_iommu->pt_supported;
      case VTD_SM_PASID_ENTRY_FLT:
+        return !!(s->ecap & VTD_ECAP_FLTS);
+    case VTD_SM_PASID_ENTRY_SLT:
+        return !!(s->ecap & VTD_ECAP_SLTS);
      case VTD_SM_PASID_ENTRY_NESTED:
+        /* Not support NESTED page table type yet */
+        return false;
+    case VTD_SM_PASID_ENTRY_PT:
+        return !!(s->ecap & VTD_ECAP_PT);
      default:
          /* Unknown type */
          return false;
@@ -861,7 +863,6 @@ static int


vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,


      uint8_t pgtt;
      uint32_t index;
      dma_addr_t entry_size;
-    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);

      index = VTD_PASID_TABLE_INDEX(pasid);
      entry_size = VTD_PASID_ENTRY_SIZE;
@@ -875,7 +876,7 @@ static int


vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,


      }

      /* Do translation type check */
-    if (!vtd_pe_type_check(x86_iommu, pe)) {
+    if (!vtd_pe_type_check(s, pe)) {
          return -VTD_FR_PASID_TABLE_ENTRY_INV;
      }

@@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {
                        VTD_HOST_ADDRESS_WIDTH),
      DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,


FALSE),


      DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode,


FALSE),


+    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),
      DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control,


false),


      DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
@@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)
      }

      /* TODO: read cap/ecap from host to decide which cap to be exposed. */
-    if (s->scalable_mode) {
+    if (s->scalable_modern) {
+        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;
+        s->cap |= VTD_CAP_FS1GP;
+    } else if (s->scalable_mode) {
          s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
      }

@@ -4737,6 +4742,11 @@ static bool vtd_decide_config(IntelIOMMUState *s,


Error **errp)


          }
      }

+    if (!s->scalable_mode && s->scalable_modern) {
+        error_setg(errp, "Legacy mode: not support x-flts=on");


The error message should be "x-flts=on not supported in legacy mode" or
even "x-flts is only available in scalable mode" as there is no FLT in
legacy mode



OK, will do.
But I'm not quite clear of the difference between
"Legacy mode: not support x-flts=on" and "x-flts=on not supported in legacy mode".
Is it because the later looks more formal or the former has ambiguity?

It's just because the former looks more natural.
But I think the most appropriate would be : "x-flts only available in scalable mode" because the issue is about "availability", not "support" Thanks >cmd



Thanks
Zhenzhong
Duan, Zhenzhong Nov. 19, 2024, 9:25 a.m. UTC | #4 
Clear, will use "x-flts is only available in scalable mode". Thanks Clement.

From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
Sent: Tuesday, November 19, 2024 5:00 PM
To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-devel@nongnu.org
Cc: alex.williamson@redhat.com; clg@redhat.com; eric.auger@redhat.com; mst@redhat.com; peterx@redhat.com; jasowang@redhat.com; jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P <chao.p.peng@intel.com>; Yi Sun <yi.y.sun@linux.intel.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net>
Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for scalable modern mode



On 19/11/2024 08:28, Duan, Zhenzhong wrote:

Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.





Hi Clement,



-----Original Message-----

From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.com>

Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for

scalable modern mode



Hi zhenzhong,

Just one comment but you can add Reviewed-by: Clément Mathieu--

Drif<clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.com>





On 11/11/2024 09:34, Zhenzhong Duan wrote:

Caution: External email. Do not open attachments or click links, unless this

email comes from a known sender and you know the content is safe.





Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities

related to scalable mode translation, thus there are multiple combinations.



This vIOMMU implementation wants to simplify it with a new property "x-flts".

When enabled in scalable mode, first stage translation also known as scalable

modern mode is supported. When enabled in legacy mode, throw out error.



With scalable modern mode exposed to user, also accurate the pasid entry

check in vtd_pe_type_check().



Suggested-by: Jason Wang <jasowang@redhat.com><mailto:jasowang@redhat.com>

Signed-off-by: Yi Liu <yi.l.liu@intel.com><mailto:yi.l.liu@intel.com>

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com><mailto:yi.y.sun@linux.intel.com>

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com><mailto:zhenzhong.duan@intel.com>

---

  hw/i386/intel_iommu_internal.h |  2 ++

  hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------

  2 files changed, 21 insertions(+), 9 deletions(-)



diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h

index 2c977aa7da..e8b211e8b0 100644

--- a/hw/i386/intel_iommu_internal.h

+++ b/hw/i386/intel_iommu_internal.h

@@ -195,6 +195,7 @@

  #define VTD_ECAP_PASID              (1ULL << 40)

  #define VTD_ECAP_SMTS               (1ULL << 43)

  #define VTD_ECAP_SLTS               (1ULL << 46)

+#define VTD_ECAP_FLTS               (1ULL << 47)



  /* CAP_REG */

  /* (offset >> 4) << 24 */

@@ -211,6 +212,7 @@

  #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))

  #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)

  #define VTD_CAP_DRAIN_READ          (1ULL << 55)

+#define VTD_CAP_FS1GP               (1ULL << 56)

  #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ |

VTD_CAP_DRAIN_WRITE)

  #define VTD_CAP_CM                  (1ULL << 7)

  #define VTD_PASID_ID_SHIFT          20

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c

index b921793c3a..a7a81aebee 100644

--- a/hw/i386/intel_iommu.c

+++ b/hw/i386/intel_iommu.c

@@ -803,16 +803,18 @@ static inline bool

vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)

  }



  /* Return true if check passed, otherwise false */

-static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,

-                                     VTDPASIDEntry *pe)

+static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)

  {

      switch (VTD_PE_GET_TYPE(pe)) {

-    case VTD_SM_PASID_ENTRY_SLT:

-        return true;

-    case VTD_SM_PASID_ENTRY_PT:

-        return x86_iommu->pt_supported;

      case VTD_SM_PASID_ENTRY_FLT:

+        return !!(s->ecap & VTD_ECAP_FLTS);

+    case VTD_SM_PASID_ENTRY_SLT:

+        return !!(s->ecap & VTD_ECAP_SLTS);

      case VTD_SM_PASID_ENTRY_NESTED:

+        /* Not support NESTED page table type yet */

+        return false;

+    case VTD_SM_PASID_ENTRY_PT:

+        return !!(s->ecap & VTD_ECAP_PT);

      default:

          /* Unknown type */

          return false;

@@ -861,7 +863,6 @@ static int

vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,

      uint8_t pgtt;

      uint32_t index;

      dma_addr_t entry_size;

-    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);



      index = VTD_PASID_TABLE_INDEX(pasid);

      entry_size = VTD_PASID_ENTRY_SIZE;

@@ -875,7 +876,7 @@ static int

vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,

      }



      /* Do translation type check */

-    if (!vtd_pe_type_check(x86_iommu, pe)) {

+    if (!vtd_pe_type_check(s, pe)) {

          return -VTD_FR_PASID_TABLE_ENTRY_INV;

      }



@@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {

                        VTD_HOST_ADDRESS_WIDTH),

      DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,

FALSE),

      DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode,

FALSE),

+    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),

      DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control,

false),

      DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),

      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),

@@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)

      }



      /* TODO: read cap/ecap from host to decide which cap to be exposed. */

-    if (s->scalable_mode) {

+    if (s->scalable_modern) {

+        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;

+        s->cap |= VTD_CAP_FS1GP;

+    } else if (s->scalable_mode) {

          s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;

      }



@@ -4737,6 +4742,11 @@ static bool vtd_decide_config(IntelIOMMUState *s,

Error **errp)

          }

      }



+    if (!s->scalable_mode && s->scalable_modern) {

+        error_setg(errp, "Legacy mode: not support x-flts=on");

The error message should be "x-flts=on not supported in legacy mode" or

even "x-flts is only available in scalable mode" as there is no FLT in

legacy mode



OK, will do.

But I'm not quite clear of the difference between

"Legacy mode: not support x-flts=on" and "x-flts=on not supported in legacy mode".

Is it because the later looks more formal or the former has ambiguity?

It's just because the former looks more natural.
But I think the most appropriate would be : "x-flts only available in scalable mode" because the issue is about "availability", not "support" Thanks >cmd





Thanks

Zhenzhong
CLEMENT MATHIEU--DRIF Nov. 20, 2024, 6:11 a.m. UTC | #5 
ok, feel free to add my RB if this is the only change

Thanks
cmd


On 19/11/2024 10:25, Duan, Zhenzhong wrote:

Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.


Clear, will use "x-flts is only available in scalable mode". Thanks Clement.

From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.com>
Sent: Tuesday, November 19, 2024 5:00 PM
To: Duan, Zhenzhong <zhenzhong.duan@intel.com><mailto:zhenzhong.duan@intel.com>; qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org>
Cc: alex.williamson@redhat.com<mailto:alex.williamson@redhat.com>; clg@redhat.com<mailto:clg@redhat.com>; eric.auger@redhat.com<mailto:eric.auger@redhat.com>; mst@redhat.com<mailto:mst@redhat.com>; peterx@redhat.com<mailto:peterx@redhat.com>; jasowang@redhat.com<mailto:jasowang@redhat.com>; jgg@nvidia.com<mailto:jgg@nvidia.com>; nicolinc@nvidia.com<mailto:nicolinc@nvidia.com>; joao.m.martins@oracle.com<mailto:joao.m.martins@oracle.com>; Tian, Kevin <kevin.tian@intel.com><mailto:kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com><mailto:yi.l.liu@intel.com>; Peng, Chao P <chao.p.peng@intel.com><mailto:chao.p.peng@intel.com>; Yi Sun <yi.y.sun@linux.intel.com><mailto:yi.y.sun@linux.intel.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com><mailto:marcel.apfelbaum@gmail.com>; Paolo Bonzini <pbonzini@redhat.com><mailto:pbonzini@redhat.com>; Richard Henderson <richard.henderson@linaro.org><mailto:richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net><mailto:eduardo@habkost.net>
Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for scalable modern mode



On 19/11/2024 08:28, Duan, Zhenzhong wrote:

Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.





Hi Clement,



-----Original Message-----

From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.com>

Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for

scalable modern mode



Hi zhenzhong,

Just one comment but you can add Reviewed-by: Clément Mathieu--

Drif<clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.com>





On 11/11/2024 09:34, Zhenzhong Duan wrote:

Caution: External email. Do not open attachments or click links, unless this

email comes from a known sender and you know the content is safe.





Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities

related to scalable mode translation, thus there are multiple combinations.



This vIOMMU implementation wants to simplify it with a new property "x-flts".

When enabled in scalable mode, first stage translation also known as scalable

modern mode is supported. When enabled in legacy mode, throw out error.



With scalable modern mode exposed to user, also accurate the pasid entry

check in vtd_pe_type_check().



Suggested-by: Jason Wang <jasowang@redhat.com><mailto:jasowang@redhat.com>

Signed-off-by: Yi Liu <yi.l.liu@intel.com><mailto:yi.l.liu@intel.com>

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com><mailto:yi.y.sun@linux.intel.com>

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com><mailto:zhenzhong.duan@intel.com>

---

  hw/i386/intel_iommu_internal.h |  2 ++

  hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------

  2 files changed, 21 insertions(+), 9 deletions(-)



diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h

index 2c977aa7da..e8b211e8b0 100644

--- a/hw/i386/intel_iommu_internal.h

+++ b/hw/i386/intel_iommu_internal.h

@@ -195,6 +195,7 @@

  #define VTD_ECAP_PASID              (1ULL << 40)

  #define VTD_ECAP_SMTS               (1ULL << 43)

  #define VTD_ECAP_SLTS               (1ULL << 46)

+#define VTD_ECAP_FLTS               (1ULL << 47)



  /* CAP_REG */

  /* (offset >> 4) << 24 */

@@ -211,6 +212,7 @@

  #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))

  #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)

  #define VTD_CAP_DRAIN_READ          (1ULL << 55)

+#define VTD_CAP_FS1GP               (1ULL << 56)

  #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ |

VTD_CAP_DRAIN_WRITE)

  #define VTD_CAP_CM                  (1ULL << 7)

  #define VTD_PASID_ID_SHIFT          20

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c

index b921793c3a..a7a81aebee 100644

--- a/hw/i386/intel_iommu.c

+++ b/hw/i386/intel_iommu.c

@@ -803,16 +803,18 @@ static inline bool

vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)

  }



  /* Return true if check passed, otherwise false */

-static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,

-                                     VTDPASIDEntry *pe)

+static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)

  {

      switch (VTD_PE_GET_TYPE(pe)) {

-    case VTD_SM_PASID_ENTRY_SLT:

-        return true;

-    case VTD_SM_PASID_ENTRY_PT:

-        return x86_iommu->pt_supported;

      case VTD_SM_PASID_ENTRY_FLT:

+        return !!(s->ecap & VTD_ECAP_FLTS);

+    case VTD_SM_PASID_ENTRY_SLT:

+        return !!(s->ecap & VTD_ECAP_SLTS);

      case VTD_SM_PASID_ENTRY_NESTED:

+        /* Not support NESTED page table type yet */

+        return false;

+    case VTD_SM_PASID_ENTRY_PT:

+        return !!(s->ecap & VTD_ECAP_PT);

      default:

          /* Unknown type */

          return false;

@@ -861,7 +863,6 @@ static int

vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,

      uint8_t pgtt;

      uint32_t index;

      dma_addr_t entry_size;

-    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);



      index = VTD_PASID_TABLE_INDEX(pasid);

      entry_size = VTD_PASID_ENTRY_SIZE;

@@ -875,7 +876,7 @@ static int

vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,

      }



      /* Do translation type check */

-    if (!vtd_pe_type_check(x86_iommu, pe)) {

+    if (!vtd_pe_type_check(s, pe)) {

          return -VTD_FR_PASID_TABLE_ENTRY_INV;

      }



@@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {

                        VTD_HOST_ADDRESS_WIDTH),

      DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,

FALSE),

      DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode,

FALSE),

+    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),

      DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control,

false),

      DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),

      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),

@@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)

      }



      /* TODO: read cap/ecap from host to decide which cap to be exposed. */

-    if (s->scalable_mode) {

+    if (s->scalable_modern) {

+        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;

+        s->cap |= VTD_CAP_FS1GP;

+    } else if (s->scalable_mode) {

          s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;

      }



@@ -4737,6 +4742,11 @@ static bool vtd_decide_config(IntelIOMMUState *s,

Error **errp)

          }

      }



+    if (!s->scalable_mode && s->scalable_modern) {

+        error_setg(errp, "Legacy mode: not support x-flts=on");

The error message should be "x-flts=on not supported in legacy mode" or

even "x-flts is only available in scalable mode" as there is no FLT in

legacy mode



OK, will do.

But I'm not quite clear of the difference between

"Legacy mode: not support x-flts=on" and "x-flts=on not supported in legacy mode".

Is it because the later looks more formal or the former has ambiguity?

It's just because the former looks more natural.
But I think the most appropriate would be : "x-flts only available in scalable mode" because the issue is about "availability", not "support" Thanks >cmd





Thanks

Zhenzhong
Jason Wang Dec. 4, 2024, 3:34 a.m. UTC | #6 
On Mon, Nov 11, 2024 at 4:39 PM Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:
>
> Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
> related to scalable mode translation, thus there are multiple combinations.
>
> This vIOMMU implementation wants to simplify it with a new property "x-flts".
> When enabled in scalable mode, first stage translation also known as scalable
> modern mode is supported. When enabled in legacy mode, throw out error.
>
> With scalable modern mode exposed to user, also accurate the pasid entry
> check in vtd_pe_type_check().
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/i386/intel_iommu_internal.h |  2 ++
>  hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 2c977aa7da..e8b211e8b0 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -195,6 +195,7 @@
>  #define VTD_ECAP_PASID              (1ULL << 40)
>  #define VTD_ECAP_SMTS               (1ULL << 43)
>  #define VTD_ECAP_SLTS               (1ULL << 46)
> +#define VTD_ECAP_FLTS               (1ULL << 47)
>
>  /* CAP_REG */
>  /* (offset >> 4) << 24 */
> @@ -211,6 +212,7 @@
>  #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
>  #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
>  #define VTD_CAP_DRAIN_READ          (1ULL << 55)
> +#define VTD_CAP_FS1GP               (1ULL << 56)
>  #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE)
>  #define VTD_CAP_CM                  (1ULL << 7)
>  #define VTD_PASID_ID_SHIFT          20
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index b921793c3a..a7a81aebee 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -803,16 +803,18 @@ static inline bool vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)
>  }
>
>  /* Return true if check passed, otherwise false */
> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
> -                                     VTDPASIDEntry *pe)
> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)
>  {
>      switch (VTD_PE_GET_TYPE(pe)) {
> -    case VTD_SM_PASID_ENTRY_SLT:
> -        return true;
> -    case VTD_SM_PASID_ENTRY_PT:
> -        return x86_iommu->pt_supported;
>      case VTD_SM_PASID_ENTRY_FLT:
> +        return !!(s->ecap & VTD_ECAP_FLTS);
> +    case VTD_SM_PASID_ENTRY_SLT:
> +        return !!(s->ecap & VTD_ECAP_SLTS);
>      case VTD_SM_PASID_ENTRY_NESTED:
> +        /* Not support NESTED page table type yet */
> +        return false;
> +    case VTD_SM_PASID_ENTRY_PT:
> +        return !!(s->ecap & VTD_ECAP_PT);
>      default:
>          /* Unknown type */
>          return false;
> @@ -861,7 +863,6 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>      uint8_t pgtt;
>      uint32_t index;
>      dma_addr_t entry_size;
> -    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>
>      index = VTD_PASID_TABLE_INDEX(pasid);
>      entry_size = VTD_PASID_ENTRY_SIZE;
> @@ -875,7 +876,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>      }
>
>      /* Do translation type check */
> -    if (!vtd_pe_type_check(x86_iommu, pe)) {
> +    if (!vtd_pe_type_check(s, pe)) {
>          return -VTD_FR_PASID_TABLE_ENTRY_INV;
>      }
>
> @@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {
>                        VTD_HOST_ADDRESS_WIDTH),
>      DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
>      DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> +    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),
>      DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
>      DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
>      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> @@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)
>      }
>
>      /* TODO: read cap/ecap from host to decide which cap to be exposed. */
> -    if (s->scalable_mode) {
> +    if (s->scalable_modern) {
> +        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;
> +        s->cap |= VTD_CAP_FS1GP;
> +    } else if (s->scalable_mode) {
>          s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
>      }
>
> @@ -4737,6 +4742,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>          }
>      }
>
> +    if (!s->scalable_mode && s->scalable_modern) {
> +        error_setg(errp, "Legacy mode: not support x-flts=on");

This seems to be wired, should we say "scalable mode is needed for
scalable modern mode"?

> +        return false;
> +    }
> +
>      if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT &&
>          s->aw_bits != VTD_HOST_AW_48BIT) {
>          error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",
> --
> 2.34.1
>

Thanks
CLEMENT MATHIEU--DRIF Dec. 4, 2024, 6:14 a.m. UTC | #7 
On 04/12/2024 04:34, Jason Wang wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> 
> 
> On Mon, Nov 11, 2024 at 4:39 PM Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:
>>
>> Intel VT-d 3.0 introduces scalable mode, and it has a bunch of capabilities
>> related to scalable mode translation, thus there are multiple combinations.
>>
>> This vIOMMU implementation wants to simplify it with a new property "x-flts".
>> When enabled in scalable mode, first stage translation also known as scalable
>> modern mode is supported. When enabled in legacy mode, throw out error.
>>
>> With scalable modern mode exposed to user, also accurate the pasid entry
>> check in vtd_pe_type_check().
>>
>> Suggested-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  2 ++
>>   hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------
>>   2 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index 2c977aa7da..e8b211e8b0 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -195,6 +195,7 @@
>>   #define VTD_ECAP_PASID              (1ULL << 40)
>>   #define VTD_ECAP_SMTS               (1ULL << 43)
>>   #define VTD_ECAP_SLTS               (1ULL << 46)
>> +#define VTD_ECAP_FLTS               (1ULL << 47)
>>
>>   /* CAP_REG */
>>   /* (offset >> 4) << 24 */
>> @@ -211,6 +212,7 @@
>>   #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
>>   #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
>>   #define VTD_CAP_DRAIN_READ          (1ULL << 55)
>> +#define VTD_CAP_FS1GP               (1ULL << 56)
>>   #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE)
>>   #define VTD_CAP_CM                  (1ULL << 7)
>>   #define VTD_PASID_ID_SHIFT          20
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index b921793c3a..a7a81aebee 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -803,16 +803,18 @@ static inline bool vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)
>>   }
>>
>>   /* Return true if check passed, otherwise false */
>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
>> -                                     VTDPASIDEntry *pe)
>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)
>>   {
>>       switch (VTD_PE_GET_TYPE(pe)) {
>> -    case VTD_SM_PASID_ENTRY_SLT:
>> -        return true;
>> -    case VTD_SM_PASID_ENTRY_PT:
>> -        return x86_iommu->pt_supported;
>>       case VTD_SM_PASID_ENTRY_FLT:
>> +        return !!(s->ecap & VTD_ECAP_FLTS);
>> +    case VTD_SM_PASID_ENTRY_SLT:
>> +        return !!(s->ecap & VTD_ECAP_SLTS);
>>       case VTD_SM_PASID_ENTRY_NESTED:
>> +        /* Not support NESTED page table type yet */
>> +        return false;
>> +    case VTD_SM_PASID_ENTRY_PT:
>> +        return !!(s->ecap & VTD_ECAP_PT);
>>       default:
>>           /* Unknown type */
>>           return false;
>> @@ -861,7 +863,6 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>       uint8_t pgtt;
>>       uint32_t index;
>>       dma_addr_t entry_size;
>> -    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>>
>>       index = VTD_PASID_TABLE_INDEX(pasid);
>>       entry_size = VTD_PASID_ENTRY_SIZE;
>> @@ -875,7 +876,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>       }
>>
>>       /* Do translation type check */
>> -    if (!vtd_pe_type_check(x86_iommu, pe)) {
>> +    if (!vtd_pe_type_check(s, pe)) {
>>           return -VTD_FR_PASID_TABLE_ENTRY_INV;
>>       }
>>
>> @@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {
>>                         VTD_HOST_ADDRESS_WIDTH),
>>       DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
>>       DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
>> +    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),
>>       DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
>>       DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
>>       DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
>> @@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)
>>       }
>>
>>       /* TODO: read cap/ecap from host to decide which cap to be exposed. */
>> -    if (s->scalable_mode) {
>> +    if (s->scalable_modern) {
>> +        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;
>> +        s->cap |= VTD_CAP_FS1GP;
>> +    } else if (s->scalable_mode) {
>>           s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
>>       }
>>
>> @@ -4737,6 +4742,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>           }
>>       }
>>
>> +    if (!s->scalable_mode && s->scalable_modern) {
>> +        error_setg(errp, "Legacy mode: not support x-flts=on");
> 
> This seems to be wired, should we say "scalable mode is needed for
> scalable modern mode"?

Hi Jason,

We agreed to use the following sentence: "x-flts is only available in 
scalable mode"

Does it look goot to you?

Thanks
cmd

> 
>> +        return false;
>> +    }
>> +
>>       if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT &&
>>           s->aw_bits != VTD_HOST_AW_48BIT) {
>>           error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",
>> --
>> 2.34.1
>>
> 
> Thanks
>
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 2c977aa7da..e8b211e8b0 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -195,6 +195,7 @@ 
 #define VTD_ECAP_PASID              (1ULL << 40)
 #define VTD_ECAP_SMTS               (1ULL << 43)
 #define VTD_ECAP_SLTS               (1ULL << 46)
+#define VTD_ECAP_FLTS               (1ULL << 47)
 
 /* CAP_REG */
 /* (offset >> 4) << 24 */
@@ -211,6 +212,7 @@ 
 #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
 #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
 #define VTD_CAP_DRAIN_READ          (1ULL << 55)
+#define VTD_CAP_FS1GP               (1ULL << 56)
 #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE)
 #define VTD_CAP_CM                  (1ULL << 7)
 #define VTD_PASID_ID_SHIFT          20
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b921793c3a..a7a81aebee 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -803,16 +803,18 @@  static inline bool vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)
 }
 
 /* Return true if check passed, otherwise false */
-static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
-                                     VTDPASIDEntry *pe)
+static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)
 {
     switch (VTD_PE_GET_TYPE(pe)) {
-    case VTD_SM_PASID_ENTRY_SLT:
-        return true;
-    case VTD_SM_PASID_ENTRY_PT:
-        return x86_iommu->pt_supported;
     case VTD_SM_PASID_ENTRY_FLT:
+        return !!(s->ecap & VTD_ECAP_FLTS);
+    case VTD_SM_PASID_ENTRY_SLT:
+        return !!(s->ecap & VTD_ECAP_SLTS);
     case VTD_SM_PASID_ENTRY_NESTED:
+        /* Not support NESTED page table type yet */
+        return false;
+    case VTD_SM_PASID_ENTRY_PT:
+        return !!(s->ecap & VTD_ECAP_PT);
     default:
         /* Unknown type */
         return false;
@@ -861,7 +863,6 @@  static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
     uint8_t pgtt;
     uint32_t index;
     dma_addr_t entry_size;
-    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
 
     index = VTD_PASID_TABLE_INDEX(pasid);
     entry_size = VTD_PASID_ENTRY_SIZE;
@@ -875,7 +876,7 @@  static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
     }
 
     /* Do translation type check */
-    if (!vtd_pe_type_check(x86_iommu, pe)) {
+    if (!vtd_pe_type_check(s, pe)) {
         return -VTD_FR_PASID_TABLE_ENTRY_INV;
     }
 
@@ -3827,6 +3828,7 @@  static Property vtd_properties[] = {
                       VTD_HOST_ADDRESS_WIDTH),
     DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
     DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
+    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),
     DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
     DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
     DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
@@ -4558,7 +4560,10 @@  static void vtd_cap_init(IntelIOMMUState *s)
     }
 
     /* TODO: read cap/ecap from host to decide which cap to be exposed. */
-    if (s->scalable_mode) {
+    if (s->scalable_modern) {
+        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;
+        s->cap |= VTD_CAP_FS1GP;
+    } else if (s->scalable_mode) {
         s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
     }
 
@@ -4737,6 +4742,11 @@  static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
         }
     }
 
+    if (!s->scalable_mode && s->scalable_modern) {
+        error_setg(errp, "Legacy mode: not support x-flts=on");
+        return false;
+    }
+
     if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT &&
         s->aw_bits != VTD_HOST_AW_48BIT) {
         error_setg(errp, "%s mode: supported values for aw-bits are: %d, %d",