Re: [Qemu-devel] [RFC PATCH 03/20] intel_iommu: add "svm" option

[Liu, Yi L]

On Thu, 27 Apr 2017 18:53:17 +0800
Peter Xu <address@hidden> wrote:

> On Wed, Apr 26, 2017 at 06:06:33PM +0800, Liu, Yi L wrote:
> > Expose "Shared Virtual Memory" to guest by using "svm" option.
> > Also use "svm" to expose SVM related capabilities to guest.
> > e.g. "-device intel-iommu, svm=on"
> >
> > Signed-off-by: Liu, Yi L <address@hidden>
> > ---
> >  hw/i386/intel_iommu.c          | 10 ++++++++++
> >  hw/i386/intel_iommu_internal.h |  5 +++++
> > include/hw/i386/intel_iommu.h  |  1 +
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > bf98fa5..ba1e7eb 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2453,6 +2453,7 @@ static Property vtd_properties[] = {
> >      DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false),
> >      DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,
> FALSE),
> >      DEFINE_PROP_BOOL("ecs", IntelIOMMUState, ecs, FALSE),
> > +    DEFINE_PROP_BOOL("svm", IntelIOMMUState, svm, FALSE),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > @@ -2973,6 +2974,15 @@ static void vtd_init(IntelIOMMUState *s)
> >          s->ecap |= VTD_ECAP_ECS;
> >      }
> >
> > +    if (s->svm) {
> > +        if (!s->ecs || !x86_iommu->pt_supported || !s->caching_mode) {
> > +            error_report("Need to set ecs, pt, caching-mode for svm");
> > +            exit(1);
> > +        }
> > +        s->cap |= VTD_CAP_DWD | VTD_CAP_DRD;
> > +        s->ecap |= VTD_ECAP_PRS | VTD_ECAP_PTS | VTD_ECAP_PASID28;
> > +    }
> > +
> >      if (s->caching_mode) {
> >          s->cap |= VTD_CAP_CM;
> >      }
> > diff --git a/hw/i386/intel_iommu_internal.h
> > b/hw/i386/intel_iommu_internal.h index 71a1c1e..f2a7d12 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -191,6 +191,9 @@
> >  #define VTD_ECAP_PT                 (1ULL << 6)
> >  #define VTD_ECAP_MHMV               (15ULL << 20)
> >  #define VTD_ECAP_ECS                (1ULL << 24)
> > +#define VTD_ECAP_PASID28            (1ULL << 28)
> 
> Could I ask what's this bit? On my spec, it says this bit is reserved and 
> defunct (spec
> version: June 2016).

As Ashok confirmed, yes it should be bit 40. would update it.

> > +#define VTD_ECAP_PRS                (1ULL << 29)
> > +#define VTD_ECAP_PTS                (0xeULL << 35)
> 
> Would it better we avoid using 0xe here, or at least add some comment?

For this value, it must be no more than the bits host supports. So it may be
better to have a default value and meanwhile expose an option to let user
set it. how about your opinion?

> 
> >
> >  /* CAP_REG */
> >  /* (offset >> 4) << 24 */
> > @@ -207,6 +210,8 @@
> >  #define VTD_CAP_PSI                 (1ULL << 39)
> >  #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
> >  #define VTD_CAP_CM                  (1ULL << 7)
> > +#define VTD_CAP_DWD                 (1ULL << 54)
> > +#define VTD_CAP_DRD                 (1ULL << 55)
> 
> Just to confirm: after this series, we should support drain read/write then, 
> right?

I haven’t done special process against it in IOMMU emulator. It's set to keep
consistence with VT-d spec since DWD and DRW is required capability when
PASID it reported as Set. However, I think it should be fine if guest issue QI
with drain read/write set in the descriptor. Host should be able to process it.

Thanks,
Yi L
> >
> >  /* Supported Adjusted Guest Address Widths */
> >  #define VTD_CAP_SAGAW_SHIFT         8
> > diff --git a/include/hw/i386/intel_iommu.h
> > b/include/hw/i386/intel_iommu.h index ae21fe5..8981615 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -267,6 +267,7 @@ struct IntelIOMMUState {
> >
> >      bool caching_mode;          /* RO - is cap CM enabled? */
> >      bool ecs;                       /* Extended Context Support */
> > +    bool svm;                       /* Shared Virtual Memory */
> >
> >      dma_addr_t root;                /* Current root table pointer */
> >      bool root_extended;             /* Type of root table (extended or 
> > not) */
> > --
> > 1.9.1
> >
> 
> --
> Peter Xu