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

[Liu, Yi L]

On Mon, May 08, 2017 at 07:20:34PM +0800, Peter Xu wrote:
> On Mon, May 08, 2017 at 10:38:09AM +, Liu, Yi L wrote:
> > On Thu, 27 Apr 2017 18:53:17 +0800
> > Peter Xu  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 
> > > > ---
> > > >  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.
> 
> Ok.
> 
> > 
> > > > +#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?
> 
> I think a more important point is that we need to make sure this value
> is no larger than hardware support? 

Agree. If it is larger, sanity check would fail.

> Since you are also working on the
> vfio interface for virt-svm... would it be possible that we can talk
> to kernel in some way so that we can know the supported pasid size in
> host IOMMU? So that when guest specifies something bigger, we can stop
> the user.

If it is just to stop when the size is not valid, I think we already have
such sanity check in host when trying to bind guest pasid table. Not sure
if it is practical to talk with kernel on the supported pasid size. But
may think about it. It is very likely that we need to do it through VFIO.

> 
> I don't know the practical value for this field, if it's static
> enough, I think it's also okay we make it static here as well. But
> again, I would prefer at least some comment, like:
> 
>   /* Value N indicates PASID field of N+1 bits, here 0xe stands for.. */

yes, at least we need to add such comments. Would add it.

> > 
> > > 
> > > >
> > > >  /* 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.
> 
> I see. IIUC the 

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

[Peter Xu]

On Mon, May 08, 2017 at 10:38:09AM +, Liu, Yi L wrote:
> On Thu, 27 Apr 2017 18:53:17 +0800
> Peter Xu  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 
> > > ---
> > >  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.

Ok.

> 
> > > +#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?

I think a more important point is that we need to make sure this value
is no larger than hardware support? Since you are also working on the
vfio interface for virt-svm... would it be possible that we can talk
to kernel in some way so that we can know the supported pasid size in
host IOMMU? So that when guest specifies something bigger, we can stop
the user.

I don't know the practical value for this field, if it's static
enough, I think it's also okay we make it static here as well. But
again, I would prefer at least some comment, like:

  /* Value N indicates PASID field of N+1 bits, here 0xe stands for.. */

> 
> > 
> > >
> > >  /* 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.

I see. IIUC the point here is we need to deliver these requests to
host IOMMU, and I guess we need to be able to do this in a synchronous
way as well.

Thanks,

-- 
Peter Xu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH 03/20] intel_iommu: add "svm" option

[Liu, Yi L]

On Thu, 27 Apr 2017 18:53:17 +0800
Peter Xu  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 
> > ---
> >  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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Raj, Ashok]

On Thu, May 04, 2017 at 02:28:53PM -0600, Alex Williamson wrote:
> On Thu, 27 Apr 2017 18:53:17 +0800
> Peter Xu  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 
> > > ---
> > >  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).
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d42fde70849c5ba2f00c37a0666305eb507a47b8
> 
> Do we really need to emulate the buggy implementation?  Seems like we
> could just pretend bit28 never happened here and use bit40 instead.
> 

Agree, bit28 can be gone.  
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Alex Williamson]

On Thu, 27 Apr 2017 18:53:17 +0800
Peter Xu  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 
> > ---
> >  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).

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d42fde70849c5ba2f00c37a0666305eb507a47b8

Do we really need to emulate the buggy implementation?  Seems like we
could just pretend bit28 never happened here and use bit40 instead.

> 
> > +#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?
> 
> >  
> >  /* 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?
> 
> Thanks,
> 
> >  
> >  /* 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
> >   
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Peter Xu]

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 
> ---
>  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).

> +#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?

>  
>  /* 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?

Thanks,

>  
>  /* 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 03/20] intel_iommu: add "svm" option

[Liu, Yi L]

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 
---
 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)
+#define VTD_ECAP_PRS(1ULL << 29)
+#define VTD_ECAP_PTS(0xeULL << 35)
 
 /* 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)
 
 /* 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

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu