Re: [Qemu-devel] [PATCH v4 RESEND 1/3] IOMMU: add option to enable VTD_CAP_CM to vIOMMU capility exposoed to guest

[Michael S. Tsirkin]

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" <address@hidden>
> > 
> > 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 <address@hidden>
> > ---
> >   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 */