[PATCH rfcv1 23/23] intel_iommu: modify x-scalable-mode to be string option

Zhenzhong Duan posted 23 patches 10 months, 2 weeks ago
[PATCH rfcv1 23/23] intel_iommu: modify x-scalable-mode to be string option
Posted by Zhenzhong Duan 10 months, 2 weeks ago
From: Yi Liu <yi.l.liu@intel.com>

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.
While this vIOMMU implementation wants to simplify it for user by providing
typical combinations. User could config it by "x-scalable-mode" option. The
usage is as below:

"-device intel-iommu,x-scalable-mode=["legacy"|"modern"|"off"]"

 - "legacy": gives support for stage-2 page table
 - "modern": gives support for stage-1 page table
 - "off": no scalable mode support
 -  if not configured, means no scalable mode support, if not proper
    configured, will throw error

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>
---
 include/hw/i386/intel_iommu.h |  1 +
 hw/i386/intel_iommu.c         | 25 +++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index f3e75263b7..9cbd568171 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -320,6 +320,7 @@ struct IntelIOMMUState {
 
     bool caching_mode;              /* RO - is cap CM enabled? */
     bool scalable_mode;             /* RO - is Scalable Mode supported? */
+    char *scalable_mode_str;        /* RO - admin's Scalable Mode config */
     bool scalable_modern;           /* RO - is modern SM supported? */
     bool snoop_control;             /* RO - is SNP filed supported? */
 
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index e418305f6e..b507112069 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -5111,7 +5111,7 @@ static Property vtd_properties[] = {
     DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits,
                       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_STRING("x-scalable-mode", IntelIOMMUState, scalable_mode_str),
     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),
@@ -6122,7 +6122,28 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
         }
     }
 
-    /* Currently only address widths supported are 39 and 48 bits */
+    if (s->scalable_mode_str &&
+        (strcmp(s->scalable_mode_str, "off") &&
+         strcmp(s->scalable_mode_str, "modern") &&
+         strcmp(s->scalable_mode_str, "legacy"))) {
+        error_setg(errp, "Invalid x-scalable-mode config,"
+                         "Please use \"modern\", \"legacy\" or \"off\"");
+        return false;
+    }
+
+    if (s->scalable_mode_str &&
+        !strcmp(s->scalable_mode_str, "legacy")) {
+        s->scalable_mode = true;
+        s->scalable_modern = false;
+    } else if (s->scalable_mode_str &&
+        !strcmp(s->scalable_mode_str, "modern")) {
+        s->scalable_mode = true;
+        s->scalable_modern = true;
+    } else {
+        s->scalable_mode = false;
+        s->scalable_modern = false;
+    }
+
     if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
         (s->aw_bits != VTD_HOST_AW_48BIT) &&
         !s->scalable_modern) {
-- 
2.34.1
Re: [PATCH rfcv1 23/23] intel_iommu: modify x-scalable-mode to be string option
Posted by Joel Granados 10 months ago
On Mon, Jan 15, 2024 at 06:37:35PM +0800, Zhenzhong Duan wrote:
> From: Yi Liu <yi.l.liu@intel.com>
> 
> 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.
> While this vIOMMU implementation wants to simplify it for user by providing
> typical combinations. User could config it by "x-scalable-mode" option. The
> usage is as below:
> 
> "-device intel-iommu,x-scalable-mode=["legacy"|"modern"|"off"]"
> 
>  - "legacy": gives support for stage-2 page table
>  - "modern": gives support for stage-1 page table
>  - "off": no scalable mode support
>  -  if not configured, means no scalable mode support, if not proper
>     configured, will throw error
> 
> 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>
> ---
>  include/hw/i386/intel_iommu.h |  1 +
>  hw/i386/intel_iommu.c         | 25 +++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index f3e75263b7..9cbd568171 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -320,6 +320,7 @@ struct IntelIOMMUState {
>  
>      bool caching_mode;              /* RO - is cap CM enabled? */
>      bool scalable_mode;             /* RO - is Scalable Mode supported? */
> +    char *scalable_mode_str;        /* RO - admin's Scalable Mode config */
>      bool scalable_modern;           /* RO - is modern SM supported? */
>      bool snoop_control;             /* RO - is SNP filed supported? */
>  
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index e418305f6e..b507112069 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -5111,7 +5111,7 @@ static Property vtd_properties[] = {
>      DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits,
>                        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_STRING("x-scalable-mode", IntelIOMMUState, scalable_mode_str),
>      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),
> @@ -6122,7 +6122,28 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>          }
>      }
>  
> -    /* Currently only address widths supported are 39 and 48 bits */
> +    if (s->scalable_mode_str &&
> +        (strcmp(s->scalable_mode_str, "off") &&
> +         strcmp(s->scalable_mode_str, "modern") &&
> +         strcmp(s->scalable_mode_str, "legacy"))) {
> +        error_setg(errp, "Invalid x-scalable-mode config,"
> +                         "Please use \"modern\", \"legacy\" or \"off\"");
> +        return false;
> +    }
> +
> +    if (s->scalable_mode_str &&
> +        !strcmp(s->scalable_mode_str, "legacy")) {
> +        s->scalable_mode = true;
> +        s->scalable_modern = false;
> +    } else if (s->scalable_mode_str &&
> +        !strcmp(s->scalable_mode_str, "modern")) {
> +        s->scalable_mode = true;
> +        s->scalable_modern = true;
> +    } else {
> +        s->scalable_mode = false;
> +        s->scalable_modern = false;
> +    }
> +
>      if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>          (s->aw_bits != VTD_HOST_AW_48BIT) &&
>          !s->scalable_modern) {
> -- 
> 2.34.1
> 
> 

I noticed that this patch changes quite a bit from the previous version
that you had. I Specifically noticed that you dropped VTD_ECAP_PRS from
intel_iommu_internal.h. I was under the impression that this set the
Page Request Servicves capability in the IOMMU effectively enabling PRI
in the iommu.

Why did you remove it from your original patch?

Thx in advance

Best

-- 

Joel Granados
Re: [PATCH rfcv1 23/23] intel_iommu: modify x-scalable-mode to be string option
Posted by Yi Liu 10 months ago
On 2024/1/31 22:40, Joel Granados wrote:
> On Mon, Jan 15, 2024 at 06:37:35PM +0800, Zhenzhong Duan wrote:
>> From: Yi Liu <yi.l.liu@intel.com>
>>
>> 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.
>> While this vIOMMU implementation wants to simplify it for user by providing
>> typical combinations. User could config it by "x-scalable-mode" option. The
>> usage is as below:
>>
>> "-device intel-iommu,x-scalable-mode=["legacy"|"modern"|"off"]"
>>
>>   - "legacy": gives support for stage-2 page table
>>   - "modern": gives support for stage-1 page table
>>   - "off": no scalable mode support
>>   -  if not configured, means no scalable mode support, if not proper
>>      configured, will throw error
>>
>> 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>
>> ---
>>   include/hw/i386/intel_iommu.h |  1 +
>>   hw/i386/intel_iommu.c         | 25 +++++++++++++++++++++++--
>>   2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
>> index f3e75263b7..9cbd568171 100644
>> --- a/include/hw/i386/intel_iommu.h
>> +++ b/include/hw/i386/intel_iommu.h
>> @@ -320,6 +320,7 @@ struct IntelIOMMUState {
>>   
>>       bool caching_mode;              /* RO - is cap CM enabled? */
>>       bool scalable_mode;             /* RO - is Scalable Mode supported? */
>> +    char *scalable_mode_str;        /* RO - admin's Scalable Mode config */
>>       bool scalable_modern;           /* RO - is modern SM supported? */
>>       bool snoop_control;             /* RO - is SNP filed supported? */
>>   
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index e418305f6e..b507112069 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -5111,7 +5111,7 @@ static Property vtd_properties[] = {
>>       DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits,
>>                         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_STRING("x-scalable-mode", IntelIOMMUState, scalable_mode_str),
>>       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),
>> @@ -6122,7 +6122,28 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>           }
>>       }
>>   
>> -    /* Currently only address widths supported are 39 and 48 bits */
>> +    if (s->scalable_mode_str &&
>> +        (strcmp(s->scalable_mode_str, "off") &&
>> +         strcmp(s->scalable_mode_str, "modern") &&
>> +         strcmp(s->scalable_mode_str, "legacy"))) {
>> +        error_setg(errp, "Invalid x-scalable-mode config,"
>> +                         "Please use \"modern\", \"legacy\" or \"off\"");
>> +        return false;
>> +    }
>> +
>> +    if (s->scalable_mode_str &&
>> +        !strcmp(s->scalable_mode_str, "legacy")) {
>> +        s->scalable_mode = true;
>> +        s->scalable_modern = false;
>> +    } else if (s->scalable_mode_str &&
>> +        !strcmp(s->scalable_mode_str, "modern")) {
>> +        s->scalable_mode = true;
>> +        s->scalable_modern = true;
>> +    } else {
>> +        s->scalable_mode = false;
>> +        s->scalable_modern = false;
>> +    }
>> +
>>       if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
>>           (s->aw_bits != VTD_HOST_AW_48BIT) &&
>>           !s->scalable_modern) {
>> -- 
>> 2.34.1
>>
>>
> 
> I noticed that this patch changes quite a bit from the previous version
> that you had. I Specifically noticed that you dropped VTD_ECAP_PRS from
> intel_iommu_internal.h. I was under the impression that this set the
> Page Request Servicves capability in the IOMMU effectively enabling PRI
> in the iommu.
> 
> Why did you remove it from your original patch?

It is because this series does not cover the PRS support. It should come
in a future series.

Regards,
Yi Liu
Re: [PATCH rfcv1 23/23] intel_iommu: modify x-scalable-mode to be string option
Posted by Joel Granados 9 months, 3 weeks ago
On Wed, Jan 31, 2024 at 11:24:18PM +0800, Yi Liu wrote:
> On 2024/1/31 22:40, Joel Granados wrote:
> > On Mon, Jan 15, 2024 at 06:37:35PM +0800, Zhenzhong Duan wrote:
> >> From: Yi Liu <yi.l.liu@intel.com>
> >>
> >> 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.
> >> While this vIOMMU implementation wants to simplify it for user by providing
> >> typical combinations. User could config it by "x-scalable-mode" option. The
> >> usage is as below:
> >>
> >> "-device intel-iommu,x-scalable-mode=["legacy"|"modern"|"off"]"
> >>
> >>   - "legacy": gives support for stage-2 page table
> >>   - "modern": gives support for stage-1 page table
> >>   - "off": no scalable mode support
> >>   -  if not configured, means no scalable mode support, if not proper
> >>      configured, will throw error
> >>
> >> 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>
> >> ---
> >>   include/hw/i386/intel_iommu.h |  1 +
> >>   hw/i386/intel_iommu.c         | 25 +++++++++++++++++++++++--
> >>   2 files changed, 24 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> >> index f3e75263b7..9cbd568171 100644
> >> --- a/include/hw/i386/intel_iommu.h
> >> +++ b/include/hw/i386/intel_iommu.h
> >> @@ -320,6 +320,7 @@ struct IntelIOMMUState {
> >>   
> >>       bool caching_mode;              /* RO - is cap CM enabled? */
> >>       bool scalable_mode;             /* RO - is Scalable Mode supported? */
> >> +    char *scalable_mode_str;        /* RO - admin's Scalable Mode config */
> >>       bool scalable_modern;           /* RO - is modern SM supported? */
> >>       bool snoop_control;             /* RO - is SNP filed supported? */
> >>   
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index e418305f6e..b507112069 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -5111,7 +5111,7 @@ static Property vtd_properties[] = {
> >>       DEFINE_PROP_UINT8("aw-bits", IntelIOMMUState, aw_bits,
> >>                         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_STRING("x-scalable-mode", IntelIOMMUState, scalable_mode_str),
> >>       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),
> >> @@ -6122,7 +6122,28 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> >>           }
> >>       }
> >>   
> >> -    /* Currently only address widths supported are 39 and 48 bits */
> >> +    if (s->scalable_mode_str &&
> >> +        (strcmp(s->scalable_mode_str, "off") &&
> >> +         strcmp(s->scalable_mode_str, "modern") &&
> >> +         strcmp(s->scalable_mode_str, "legacy"))) {
> >> +        error_setg(errp, "Invalid x-scalable-mode config,"
> >> +                         "Please use \"modern\", \"legacy\" or \"off\"");
> >> +        return false;
> >> +    }
> >> +
> >> +    if (s->scalable_mode_str &&
> >> +        !strcmp(s->scalable_mode_str, "legacy")) {
> >> +        s->scalable_mode = true;
> >> +        s->scalable_modern = false;
> >> +    } else if (s->scalable_mode_str &&
> >> +        !strcmp(s->scalable_mode_str, "modern")) {
> >> +        s->scalable_mode = true;
> >> +        s->scalable_modern = true;
> >> +    } else {
> >> +        s->scalable_mode = false;
> >> +        s->scalable_modern = false;
> >> +    }
> >> +
> >>       if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
> >>           (s->aw_bits != VTD_HOST_AW_48BIT) &&
> >>           !s->scalable_modern) {
> >> -- 
> >> 2.34.1
> >>
> >>
> > 
> > I noticed that this patch changes quite a bit from the previous version
> > that you had. I Specifically noticed that you dropped VTD_ECAP_PRS from
> > intel_iommu_internal.h. I was under the impression that this set the
> > Page Request Servicves capability in the IOMMU effectively enabling PRI
> > in the iommu.
> > 
> > Why did you remove it from your original patch?
> 
> It is because this series does not cover the PRS support. It should come
> in a future series.

ok. Thx for getting back to me.

Best
> 
> Regards,
> Yi Liu

-- 

Joel Granados