[RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option

Liu Yi L posted 22 patches 6 years, 3 months ago
[RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option
Posted by Liu Yi L 6 years, 3 months ago
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 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"]"

 - "legacy": gives support for SL page table
 - "modern": gives support for FL page table, pasid, virtual command
 -  if not configured, means no scalable mode support, if not proper
    configured, will throw error

Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 hw/i386/intel_iommu.c          | 15 +++++++++++++--
 hw/i386/intel_iommu_internal.h |  3 +++
 include/hw/i386/intel_iommu.h  |  2 +-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 771bed2..4a1a07a 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3019,7 +3019,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),
     DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -3581,7 +3581,12 @@ static void vtd_init(IntelIOMMUState *s)
 
     /* TODO: read cap/ecap from host to decide which cap to be exposed. */
     if (s->scalable_mode) {
-        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
+        if (!strcmp(s->scalable_mode, "legacy")) {
+            s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
+        } else if (!strcmp(s->scalable_mode, "modern")) {
+            s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_PASID
+                       | VTD_ECAP_FLTS | VTD_ECAP_PSS;
+        }
     }
 
     vtd_reset_caches(s);
@@ -3700,6 +3705,12 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
         return false;
     }
 
+    if (s->scalable_mode &&
+        (strcmp(s->scalable_mode, "modern") &&
+         strcmp(s->scalable_mode, "legacy"))) {
+            error_setg(errp, "Invalid x-scalable-mode config");
+    }
+
     return true;
 }
 
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index c1235a7..be7b30a 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -190,8 +190,11 @@
 #define VTD_ECAP_PT                 (1ULL << 6)
 #define VTD_ECAP_MHMV               (15ULL << 20)
 #define VTD_ECAP_SRS                (1ULL << 31)
+#define VTD_ECAP_PSS                (19ULL << 35)
+#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 */
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 66b931e..6062588 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -231,7 +231,7 @@ struct IntelIOMMUState {
     uint32_t version;
 
     bool caching_mode;              /* RO - is cap CM enabled? */
-    bool scalable_mode;             /* RO - is Scalable Mode supported? */
+    char *scalable_mode;            /* RO - Scalable Mode model */
 
     dma_addr_t root;                /* Current root table pointer */
     bool root_scalable;             /* Type of root table (scalable or not) */
-- 
2.7.4


Re: [RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option
Posted by Peter Xu 6 years, 3 months ago
On Thu, Oct 24, 2019 at 08:34:24AM -0400, Liu Yi L 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.
> While this vIOMMU implementation wants 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"]"
> 
>  - "legacy": gives support for SL page table
>  - "modern": gives support for FL page table, pasid, virtual command
>  -  if not configured, means no scalable mode support, if not proper
>     configured, will throw error
> 
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> ---
>  hw/i386/intel_iommu.c          | 15 +++++++++++++--
>  hw/i386/intel_iommu_internal.h |  3 +++
>  include/hw/i386/intel_iommu.h  |  2 +-
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 771bed2..4a1a07a 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3019,7 +3019,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),
>      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -3581,7 +3581,12 @@ static void vtd_init(IntelIOMMUState *s)
>  
>      /* TODO: read cap/ecap from host to decide which cap to be exposed. */
>      if (s->scalable_mode) {
> -        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
> +        if (!strcmp(s->scalable_mode, "legacy")) {
> +            s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
> +        } else if (!strcmp(s->scalable_mode, "modern")) {
> +            s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_PASID
> +                       | VTD_ECAP_FLTS | VTD_ECAP_PSS;
> +        }

Shall we do this string op only once in vtd_decide_config() then keep
it somewhere?

Something like:

  - s->scalable_mode_str to keep the string
  - s->scalable_mode still as a bool to cache the global enablement
  - s->scalable_modern as a bool to keep the mode

?

These could be used in some MMIO path (I think) and parsing strings
always could be a bit overkill.

>      }
>  
>      vtd_reset_caches(s);
> @@ -3700,6 +3705,12 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>          return false;
>      }
>  
> +    if (s->scalable_mode &&
> +        (strcmp(s->scalable_mode, "modern") &&
> +         strcmp(s->scalable_mode, "legacy"))) {
> +            error_setg(errp, "Invalid x-scalable-mode config");
> +    }
> +
>      return true;
>  }
>  
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index c1235a7..be7b30a 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -190,8 +190,11 @@
>  #define VTD_ECAP_PT                 (1ULL << 6)
>  #define VTD_ECAP_MHMV               (15ULL << 20)
>  #define VTD_ECAP_SRS                (1ULL << 31)
> +#define VTD_ECAP_PSS                (19ULL << 35)
> +#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 */
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 66b931e..6062588 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -231,7 +231,7 @@ struct IntelIOMMUState {
>      uint32_t version;
>  
>      bool caching_mode;              /* RO - is cap CM enabled? */
> -    bool scalable_mode;             /* RO - is Scalable Mode supported? */
> +    char *scalable_mode;            /* RO - Scalable Mode model */
>  
>      dma_addr_t root;                /* Current root table pointer */
>      bool root_scalable;             /* Type of root table (scalable or not) */
> -- 
> 2.7.4
> 

Regards,

-- 
Peter Xu

RE: [RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option
Posted by Liu, Yi L 6 years, 3 months ago
> From: Peter Xu [mailto:peterx@redhat.com]
> Sent: Friday, November 1, 2019 10:58 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option
> 
> On Thu, Oct 24, 2019 at 08:34:24AM -0400, Liu Yi L 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.
> > While this vIOMMU implementation wants 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"]"
> >
> >  - "legacy": gives support for SL page table
> >  - "modern": gives support for FL page table, pasid, virtual command
> >  -  if not configured, means no scalable mode support, if not proper
> >     configured, will throw error
> >
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Cc: Yi Sun <yi.y.sun@linux.intel.com>
> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > ---
> >  hw/i386/intel_iommu.c          | 15 +++++++++++++--
> >  hw/i386/intel_iommu_internal.h |  3 +++
> > include/hw/i386/intel_iommu.h  |  2 +-
> >  3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > 771bed2..4a1a07a 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3019,7 +3019,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),
> >      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> > @@ -3581,7 +3581,12 @@ static void vtd_init(IntelIOMMUState *s)
> >
> >      /* TODO: read cap/ecap from host to decide which cap to be exposed. */
> >      if (s->scalable_mode) {
> > -        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
> > +        if (!strcmp(s->scalable_mode, "legacy")) {
> > +            s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
> > +        } else if (!strcmp(s->scalable_mode, "modern")) {
> > +            s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_PASID
> > +                       | VTD_ECAP_FLTS | VTD_ECAP_PSS;
> > +        }
> 
> Shall we do this string op only once in vtd_decide_config() then keep it somewhere?

Agreed. I'll move it to vtd_decide_config().

> Something like:
> 
>   - s->scalable_mode_str to keep the string
>   - s->scalable_mode still as a bool to cache the global enablement
>   - s->scalable_modern as a bool to keep the mode
> 
> ?

So x-scalable-mode is still a string option, just to have a new field to store it?

> 
> These could be used in some MMIO path (I think) and parsing strings always could be
> a bit overkill.

I think so. Let's just align on the direction above.

Regards,
Yi Liu

Re: [RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option
Posted by Peter Xu 6 years, 3 months ago
On Tue, Nov 05, 2019 at 09:14:08AM +0000, Liu, Yi L wrote:
> > Something like:
> > 
> >   - s->scalable_mode_str to keep the string
> >   - s->scalable_mode still as a bool to cache the global enablement
> >   - s->scalable_modern as a bool to keep the mode
> > 
> > ?
> 
> So x-scalable-mode is still a string option, just to have a new field to store it?

Yep.  I'd say maybe we should start to allow to define some union-ish
properties, but for now I think string is ok.

-- 
Peter Xu

RE: [RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option
Posted by Liu, Yi L 6 years, 3 months ago
> From: Peter Xu [mailto:peterx@redhat.com]
> Sent: Tuesday, November 5, 2019 8:50 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 03/22] intel_iommu: modify x-scalable-mode to be string option
> 
> On Tue, Nov 05, 2019 at 09:14:08AM +0000, Liu, Yi L wrote:
> > > Something like:
> > >
> > >   - s->scalable_mode_str to keep the string
> > >   - s->scalable_mode still as a bool to cache the global enablement
> > >   - s->scalable_modern as a bool to keep the mode
> > >
> > > ?
> >
> > So x-scalable-mode is still a string option, just to have a new field to store it?
> 
> Yep.  I'd say maybe we should start to allow to define some union-ish properties, but
> for now I think string is ok.

ok, let me make it in next version.

Thanks,
Yi Liu