diff mbox

[v4,RESEND,1/3] IOMMU: add option to enable VTD_CAP_CM to vIOMMU capility exposoed to guest

Message ID 1476719064-9242-2-git-send-email-bd.aviv@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aviv B.D. Oct. 17, 2016, 3:44 p.m. UTC 
From: "Aviv Ben-David" <bd.aviv@gmail.com>

This capability asks the guest to invalidate cache before each map operation.
We can use this invalidation to trap map operations in the hypervisor.

Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com>
---
 hw/i386/intel_iommu.c          | 5 +++++
 hw/i386/intel_iommu_internal.h | 1 +
 include/hw/i386/intel_iommu.h  | 2 ++
 3 files changed, 8 insertions(+)

Comments

Jason Wang Oct. 21, 2016, 7:14 a.m. UTC | #1 
On 2016年10月17日 23:44, Aviv B.D wrote:
> From: "Aviv Ben-David" <bd.aviv@gmail.com>
>
> This capability asks the guest to invalidate cache before each map operation.
> We can use this invalidation to trap map operations in the hypervisor.
>
> Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com>
> ---
>   hw/i386/intel_iommu.c          | 5 +++++
>   hw/i386/intel_iommu_internal.h | 1 +
>   include/hw/i386/intel_iommu.h  | 2 ++
>   3 files changed, 8 insertions(+)

As I asked in previous version, this may not be sufficient.

CM requires to cache fault translations which is not implemented in this 
patch. Guest can easily notice this kind of spec violation.

Thanks

>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 2efd69b..69730cb 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2012,6 +2012,7 @@ static const MemoryRegionOps vtd_mem_ops = {
>   
>   static Property vtd_properties[] = {
>       DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> +    DEFINE_PROP_BOOL("cache-mode", IntelIOMMUState, cache_mode_enabled, FALSE),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -2385,6 +2386,10 @@ static void vtd_init(IntelIOMMUState *s)
>           s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
>       }
>   
> +    if (s->cache_mode_enabled) {
> +        s->cap |= VTD_CAP_CM;
> +    }
> +
>       vtd_reset_context_cache(s);
>       vtd_reset_iotlb(s);
>   
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 0829a50..35d9f3a 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -201,6 +201,7 @@
>   #define VTD_CAP_MAMV                (VTD_MAMV << 48)
>   #define VTD_CAP_PSI                 (1ULL << 39)
>   #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
> +#define VTD_CAP_CM                  (1ULL << 7)
>   
>   /* 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 a42dbd7..7a94f16 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -258,6 +258,8 @@ struct IntelIOMMUState {
>       uint8_t womask[DMAR_REG_SIZE];  /* WO (write only - read returns 0) */
>       uint32_t version;
>   
> +    bool cache_mode_enabled;        /* RO - is cap CM enabled? */
> +
>       dma_addr_t root;                /* Current root table pointer */
>       bool root_extended;             /* Type of root table (extended or not) */
>       bool dmar_enabled;              /* Set if DMA remapping is enabled */
Michael S. Tsirkin Oct. 21, 2016, 7:47 p.m. UTC | #2 
On Fri, Oct 21, 2016 at 03:14:00PM +0800, Jason Wang wrote:
> 
> 
> On 2016年10月17日 23:44, Aviv B.D wrote:
> > From: "Aviv Ben-David" <bd.aviv@gmail.com>
> > 
> > This capability asks the guest to invalidate cache before each map operation.
> > We can use this invalidation to trap map operations in the hypervisor.
> > 
> > Signed-off-by: Aviv Ben-David <bd.aviv@gmail.com>
> > ---
> >   hw/i386/intel_iommu.c          | 5 +++++
> >   hw/i386/intel_iommu_internal.h | 1 +
> >   include/hw/i386/intel_iommu.h  | 2 ++
> >   3 files changed, 8 insertions(+)
> 
> As I asked in previous version, this may not be sufficient.
> 
> CM requires to cache fault translations which is not implemented in this
> patch.

I'm not sure why would there be a requirement to cache
fault information. Cache can always be invalidated for
any reason, in particular an empty cache is always OK.


> Guest can easily notice this kind of spec violation.

How?

> Thanks
> 
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 2efd69b..69730cb 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2012,6 +2012,7 @@ static const MemoryRegionOps vtd_mem_ops = {
> >   static Property vtd_properties[] = {
> >       DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> > +    DEFINE_PROP_BOOL("cache-mode", IntelIOMMUState, cache_mode_enabled, FALSE),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> > @@ -2385,6 +2386,10 @@ static void vtd_init(IntelIOMMUState *s)
> >           s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
> >       }
> > +    if (s->cache_mode_enabled) {
> > +        s->cap |= VTD_CAP_CM;
> > +    }
> > +
> >       vtd_reset_context_cache(s);
> >       vtd_reset_iotlb(s);
> > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > index 0829a50..35d9f3a 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -201,6 +201,7 @@
> >   #define VTD_CAP_MAMV                (VTD_MAMV << 48)
> >   #define VTD_CAP_PSI                 (1ULL << 39)
> >   #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
> > +#define VTD_CAP_CM                  (1ULL << 7)
> >   /* 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 a42dbd7..7a94f16 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -258,6 +258,8 @@ struct IntelIOMMUState {
> >       uint8_t womask[DMAR_REG_SIZE];  /* WO (write only - read returns 0) */
> >       uint32_t version;
> > +    bool cache_mode_enabled;        /* RO - is cap CM enabled? */
> > +
> >       dma_addr_t root;                /* Current root table pointer */
> >       bool root_extended;             /* Type of root table (extended or not) */
> >       bool dmar_enabled;              /* Set if DMA remapping is enabled */
Jason Wang Oct. 24, 2016, 2:32 a.m. UTC | #3 
On 2016年10月22日 03:47, Michael S. Tsirkin wrote:
> On Fri, Oct 21, 2016 at 03:14:00PM +0800, Jason Wang wrote:
>> >
>> >
>> >On 2016年10月17日 23:44, Aviv B.D wrote:
>>> > >From: "Aviv Ben-David"<bd.aviv@gmail.com>
>>> > >
>>> > >This capability asks the guest to invalidate cache before each map operation.
>>> > >We can use this invalidation to trap map operations in the hypervisor.
>>> > >
>>> > >Signed-off-by: Aviv Ben-David<bd.aviv@gmail.com>
>>> > >---
>>> > >   hw/i386/intel_iommu.c          | 5 +++++
>>> > >   hw/i386/intel_iommu_internal.h | 1 +
>>> > >   include/hw/i386/intel_iommu.h  | 2 ++
>>> > >   3 files changed, 8 insertions(+)
>> >
>> >As I asked in previous version, this may not be sufficient.
>> >
>> >CM requires to cache fault translations which is not implemented in this
>> >patch.
> I'm not sure why would there be a requirement to cache
> fault information. Cache can always be invalidated for
> any reason, in particular an empty cache is always OK.

s/requires/may/. But what did here is "don't". Isn't this an obvious 
violation?

Empty cache only work if we don't implement an real IOTLB but traverse 
the IO page tables each time.

>
>> >Guest can easily notice this kind of spec violation.
> How?
>

I guess this may do the detection:

1) map iova A to be non-present.
2) invalidate iova A
3) access iova A
4) map iova A to addr B
5) access iova A

A correct implemented CM may meet fault in step 5, but with this patch, 
we don't.
diff mbox

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 2efd69b..69730cb 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2012,6 +2012,7 @@  static const MemoryRegionOps vtd_mem_ops = {
 
 static Property vtd_properties[] = {
     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
+    DEFINE_PROP_BOOL("cache-mode", IntelIOMMUState, cache_mode_enabled, FALSE),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2385,6 +2386,10 @@  static void vtd_init(IntelIOMMUState *s)
         s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
     }
 
+    if (s->cache_mode_enabled) {
+        s->cap |= VTD_CAP_CM;
+    }
+
     vtd_reset_context_cache(s);
     vtd_reset_iotlb(s);
 
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 0829a50..35d9f3a 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -201,6 +201,7 @@ 
 #define VTD_CAP_MAMV                (VTD_MAMV << 48)
 #define VTD_CAP_PSI                 (1ULL << 39)
 #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
+#define VTD_CAP_CM                  (1ULL << 7)
 
 /* 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 a42dbd7..7a94f16 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -258,6 +258,8 @@  struct IntelIOMMUState {
     uint8_t womask[DMAR_REG_SIZE];  /* WO (write only - read returns 0) */
     uint32_t version;
 
+    bool cache_mode_enabled;        /* RO - is cap CM enabled? */
+
     dma_addr_t root;                /* Current root table pointer */
     bool root_extended;             /* Type of root table (extended or not) */
     bool dmar_enabled;              /* Set if DMA remapping is enabled */