| Message ID | 1493201210-14357-4-git-send-email-yi.l.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <yi.l.liu@linux.intel.com> > --- > 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 >
On Thu, 27 Apr 2017 18:53:17 +0800 Peter Xu <peterx@redhat.com> 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 <yi.l.liu@linux.intel.com> > > --- > > 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 > > >
On Thu, May 04, 2017 at 02:28:53PM -0600, Alex Williamson wrote: > On Thu, 27 Apr 2017 18:53:17 +0800 > Peter Xu <peterx@redhat.com> 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 <yi.l.liu@linux.intel.com> > > > --- > > > 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.
On Mon, May 08, 2017 at 07:20:34PM +0800, Peter Xu wrote: > On Mon, May 08, 2017 at 10:38:09AM +0000, Liu, Yi L wrote: > > On Thu, 27 Apr 2017 18:53:17 +0800 > > Peter Xu <peterx@redhat.com> 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 <yi.l.liu@linux.intel.com> > > > > --- > > > > 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 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. yes, deliver request to host. For assigned devices, it is ok. BTW. do you think we need to consider it for emulated devices? Thanks, Yi L
On Thu, 27 Apr 2017 18:53:17 +0800 Peter Xu <peterx@redhat.com> 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 <yi.l.liu@linux.intel.com> > > --- > > 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
On Mon, May 08, 2017 at 10:38:09AM +0000, Liu, Yi L wrote: > On Thu, 27 Apr 2017 18:53:17 +0800 > Peter Xu <peterx@redhat.com> 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 <yi.l.liu@linux.intel.com> > > > --- > > > 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,
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) */
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 <yi.l.liu@linux.intel.com> --- 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(+)