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

[Joel Granados]

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

signature.asc
Description: PGP signature