Re: [Qemu-devel] [RFC v1 3/3] intel_iommu: add scalable-mode option to make scalable mode work

[Yi Sun <yi.y.sun@linux.intel.com>]

On 19-02-12 14:46:29, Peter Xu wrote:

[...]

> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 3664a00..447fdf3 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2492,6 +2492,17 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
> >          }
> >          break;
> >  
> > +    /*
> > +     * TODO: the entity of below two cases will be implemented in future series.
> > +     * To make guest (which integrates scalable mode support patch set in
> > +     * iommu driver) work, just return true is enough so far.
> 
> When you repost, could you mention about what tests have you done?  I
> can think of some general usages:
> 
> Device matrix:
> 
> - virtio-net, vhost=on|off
> - device assignment
> 
> Configuration matrix:
> 
> - legacy/scalable
> - passthrough=on|off
> 
> I believe smoke test (like netperf a few seconds, or even ping) would
> be enough, cover some (or all, which would be very nice to have :) of
> above scenarios.
> 
Thanks for the test cases! I have covered some of them but I think I
can do more tests.

> > +     */
> > +    case VTD_INV_DESC_PC:
> > +        break;
> > +
> > +    case VTD_INV_DESC_PIOTLB:
> > +        break;
> > +
> >      case VTD_INV_DESC_WAIT:
> >          trace_vtd_inv_desc("wait", inv_desc.val[1], inv_desc.val[0]);
> >          if (!vtd_process_wait_desc(s, &inv_desc)) {
> > @@ -3051,6 +3062,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("scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
> 
> Let's start with x-scalable-mode?  Less burden for all.
> 
Sure.

> >      DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> > @@ -3583,6 +3595,16 @@ static void vtd_init(IntelIOMMUState *s)
> >          s->cap |= VTD_CAP_CM;
> >      }
> >  
> > +    /* TODO: read cap/ecap from host to decide which cap to be exposed. */
> > +    if (s->scalable_mode) {
> > +        if (!s->caching_mode) {
> > +            error_report("Need to set caching-mode for scalable mode");
> 
> Could I ask why?
> 
My intention is to make guest explicitly send to make sure SLT shadowing
correctly.

For this point, I also have question. Why does legacy mode not check CM?
If CM is not set, may the DMA remapping be wrong because SLT cannot
match guest's latest change?

> > +            exit(1);
> > +        }
> > +        s->cap |= VTD_CAP_DWD | VTD_CAP_DRD;
> 
> Draining is supported now so this line is not needed.
> 
Got it, thanks!

> > +        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
> > +    }
> > +
> >      vtd_reset_caches(s);
> >  
> >      /* Define registers with default values and bit semantics */
> > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > index 2a753c5..b01953a 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -190,7 +190,9 @@
> >  #define VTD_ECAP_EIM                (1ULL << 4)
> >  #define VTD_ECAP_PT                 (1ULL << 6)
> >  #define VTD_ECAP_MHMV               (15ULL << 20)
> > +#define VTD_ECAP_SRS                (1ULL << 31)
> >  #define VTD_ECAP_SMTS               (1ULL << 43)
> > +#define VTD_ECAP_SLTS               (1ULL << 46)
> >  
> >  /* CAP_REG */
> >  /* (offset >> 4) << 24 */
> > @@ -209,6 +211,8 @@
> >  #define VTD_CAP_DRAIN_READ          (1ULL << 55)
> >  #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ | VTD_CAP_DRAIN_WRITE)
> >  #define VTD_CAP_CM                  (1ULL << 7)
> > +#define VTD_CAP_DWD                 (1ULL << 54)
> > +#define VTD_CAP_DRD                 (1ULL << 55)
> 
> These can be dropped too (see VTD_CAP_DRAIN above).
> 
Thanks!

[...]

> > -- 
> > 1.9.1
> > 
> 
> Regards,
> 
> -- 
> Peter Xu