RE: [RFC v3 13/25] intel_iommu: modify x-scalable-mode to be string option

[Liu, Yi L]

> From: Peter Xu <address@hidden>
> Sent: Wednesday, February 12, 2020 3:44 AM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [RFC v3 13/25] intel_iommu: modify x-scalable-mode to be string
> option
> 
> On Wed, Jan 29, 2020 at 04:16:44AM -0800, Liu, Yi L wrote:
> > From: Liu Yi L <address@hidden>
> >
> > 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"]"
> 
> Maybe also "off" when someone wants to explicitly disable it?

emmm, I  think x-scalable-mode should be disabled by default. It is enabled
only when "legacy" or "modern" is configured. I'm fine to add "off" as an
explicit way to turn it off if you think it is necessary. :-)

> >
> >  - "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 <address@hidden>
> > Cc: Jacob Pan <address@hidden>
> > Cc: Peter Xu <address@hidden>
> > Cc: Yi Sun <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Richard Henderson <address@hidden>
> > Cc: Eduardo Habkost <address@hidden>
> > Signed-off-by: Liu Yi L <address@hidden>
> > Signed-off-by: Yi Sun <address@hidden>
> > ---
> >  hw/i386/intel_iommu.c          | 27 +++++++++++++++++++++++++--
> >  hw/i386/intel_iommu_internal.h |  3 +++
> > include/hw/i386/intel_iommu.h  |  2 ++
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > 1c1eb7f..33be40c 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3078,7 +3078,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("dma-drain", IntelIOMMUState, dma_drain, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> > @@ -3708,8 +3708,11 @@ static void vtd_init(IntelIOMMUState *s)
> >      }
> >
> >      /* TODO: read cap/ecap from host to decide which cap to be exposed. */
> > -    if (s->scalable_mode) {
> > +    if (s->scalable_mode && !s->scalable_modern) {
> >          s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
> > +    } else if (s->scalable_mode && s->scalable_modern) {
> > +        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_PASID
> > +                   | VTD_ECAP_FLTS | VTD_ECAP_PSS;
> 
> This patch might be good to be the last one after all the impls are ready.

Oh, yes. Let me reorder it in next version.

> >      }
> >
> >      vtd_reset_caches(s);
> > @@ -3845,6 +3848,26 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> Error **errp)
> >          return false;
> >      }
> >
> > +    if (s->scalable_mode_str &&
> > +        (strcmp(s->scalable_mode_str, "modern") &&
> > +         strcmp(s->scalable_mode_str, "legacy"))) {
> > +        error_setg(errp, "Invalid x-scalable-mode config");
> 
> Maybe "..., Please use 'modern', 'legacy', or 'off'." to show options.

Got it.

Thanks,
Yi Liu