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

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(-)

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
>

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

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

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

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

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

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
>

On Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
<clement.mathieu--drif@eviden.com> wrote:
>
>
>
> 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?

Better but if we add more features to the scalable modern, we need to
change the error message here.

Thanks

>
> 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
> >

On 09/12/2024 04:13, 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 Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
> <clement.mathieu--drif@eviden.com> wrote:
>>
>>
>>
>> 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?
> 
> Better but if we add more features to the scalable modern, we need to
> change the error message here.

Hi Jason

Maybe the weirdness comes from the fact that x-flts on the command line 
is mapped to scalable_modern in the code?

Thanks
 >cmd

> 
> Thanks
> 
>>
>> 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
>>>
>

On Mon, Dec 9, 2024 at 2:15 PM CLEMENT MATHIEU--DRIF
<clement.mathieu--drif@eviden.com> wrote:
>
>
>
> On 09/12/2024 04:13, 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 Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
> > <clement.mathieu--drif@eviden.com> wrote:
> >>
> >>
> >>
> >> 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?
> >
> > Better but if we add more features to the scalable modern, we need to
> > change the error message here.
>
> Hi Jason
>
> Maybe the weirdness comes from the fact that x-flts on the command line
> is mapped to scalable_modern in the code?

Yes, actually the code checks if scalable mode is enabled if scalable
modern is enabled. But this is inconsistent with the error message
(though x-flts was implied there probably).

Thanks


>
> Thanks
>  >cmd
>
> >
> > Thanks
> >
> >>
> >> 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
> >>>
> >

On 09/12/2024 07:24, 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, Dec 9, 2024 at 2:15 PM CLEMENT MATHIEU--DRIF
> <clement.mathieu--drif@eviden.com> wrote:
>>
>>
>> On 09/12/2024 04:13, 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 Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
>>> <clement.mathieu--drif@eviden.com> wrote:
>>>>
>>>>
>>>> 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?
>>> Better but if we add more features to the scalable modern, we need to
>>> change the error message here.
>> Hi Jason
>>
>> Maybe the weirdness comes from the fact that x-flts on the command line
>> is mapped to scalable_modern in the code?
> Yes, actually the code checks if scalable mode is enabled if scalable
> modern is enabled. But this is inconsistent with the error message
> (though x-flts was implied there probably).

Would you rename s->scalable_modern to s->flts?

>
> Thanks
>
>
>> Thanks
>>   >cmd
>>
>>> Thanks
>>>
>>>> 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
>>>>>

Hi Jason, Clement,

Sorry for late reply, just back from vacation.

>-----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
>
>
>
>
>On 09/12/2024 07:24, 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, Dec 9, 2024 at 2:15 PM CLEMENT MATHIEU--DRIF
>> <clement.mathieu--drif@eviden.com> wrote:
>>>
>>>
>>> On 09/12/2024 04:13, 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 Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
>>>> <clement.mathieu--drif@eviden.com> wrote:
>>>>>
>>>>>
>>>>> 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
...
>>>>>>> @@ -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?
>>>> Better but if we add more features to the scalable modern, we need to
>>>> change the error message here.
>>> Hi Jason
>>>
>>> Maybe the weirdness comes from the fact that x-flts on the command line
>>> is mapped to scalable_modern in the code?
>> Yes, actually the code checks if scalable mode is enabled if scalable
>> modern is enabled. But this is inconsistent with the error message
>> (though x-flts was implied there probably).
>
>Would you rename s->scalable_modern to s->flts?

Starting from v4, we replace x-scalable-mode=modern with flts=on on QEMU cmdline.
Scalable modern mode is an alias of stage-1 page table, so I reuse s->scalable_modern
in code, I'm fine to rename to s->flts if that's preferred. In that case, maybe we should
also drop the concept of 'scalable modern mode' totally?

Thanks
Zhenzhong

On Wed, Dec 11, 2024 at 10:50 AM Duan, Zhenzhong
<zhenzhong.duan@intel.com> wrote:
>
> Hi Jason, Clement,
>
> Sorry for late reply, just back from vacation.
>
> >-----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
> >
> >
> >
> >
> >On 09/12/2024 07:24, 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, Dec 9, 2024 at 2:15 PM CLEMENT MATHIEU--DRIF
> >> <clement.mathieu--drif@eviden.com> wrote:
> >>>
> >>>
> >>> On 09/12/2024 04:13, 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 Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
> >>>> <clement.mathieu--drif@eviden.com> wrote:
> >>>>>
> >>>>>
> >>>>> 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
> ...
> >>>>>>> @@ -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?
> >>>> Better but if we add more features to the scalable modern, we need to
> >>>> change the error message here.
> >>> Hi Jason
> >>>
> >>> Maybe the weirdness comes from the fact that x-flts on the command line
> >>> is mapped to scalable_modern in the code?
> >> Yes, actually the code checks if scalable mode is enabled if scalable
> >> modern is enabled. But this is inconsistent with the error message
> >> (though x-flts was implied there probably).
> >
> >Would you rename s->scalable_modern to s->flts?
>
> Starting from v4, we replace x-scalable-mode=modern with flts=on on QEMU cmdline.
> Scalable modern mode is an alias of stage-1 page table, so I reuse s->scalable_modern
> in code, I'm fine to rename to s->flts if that's preferred. In that case, maybe we should
> also drop the concept of 'scalable modern mode' totally?

I think so, it helps to reduce the confusion.

Thanks

>
> Thanks
> Zhenzhong

On 11/12/2024 04:03, 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 Wed, Dec 11, 2024 at 10:50 AM Duan, Zhenzhong
> <zhenzhong.duan@intel.com> wrote:
>> Hi Jason, Clement,
>>
>> Sorry for late reply, just back from vacation.
>>
>>> -----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
>>>
>>>
>>>
>>>
>>> On 09/12/2024 07:24, 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, Dec 9, 2024 at 2:15 PM CLEMENT MATHIEU--DRIF
>>>> <clement.mathieu--drif@eviden.com> wrote:
>>>>>
>>>>> On 09/12/2024 04:13, 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 Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
>>>>>> <clement.mathieu--drif@eviden.com> wrote:
>>>>>>>
>>>>>>> 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
>> ...
>>>>>>>>> @@ -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?
>>>>>> Better but if we add more features to the scalable modern, we need to
>>>>>> change the error message here.
>>>>> Hi Jason
>>>>>
>>>>> Maybe the weirdness comes from the fact that x-flts on the command line
>>>>> is mapped to scalable_modern in the code?
>>>> Yes, actually the code checks if scalable mode is enabled if scalable
>>>> modern is enabled. But this is inconsistent with the error message
>>>> (though x-flts was implied there probably).
>>> Would you rename s->scalable_modern to s->flts?
>> Starting from v4, we replace x-scalable-mode=modern with flts=on on QEMU cmdline.
>> Scalable modern mode is an alias of stage-1 page table, so I reuse s->scalable_modern
>> in code, I'm fine to rename to s->flts if that's preferred. In that case, maybe we should
>> also drop the concept of 'scalable modern mode' totally?
> I think so, it helps to reduce the confusion.
>
> Thanks

Yep, at this stage dropping mentions to "modern" is clearer.

Thanks
 >cmd
>
>> Thanks
>> Zhenzhong