Re: [Qemu-devel] [PATCH RFC v3 01/14] IOMMU: add option to enable VTD_CAP_CM to vIOMMU capility exposoed to guest

[Tian, Kevin]

> From: Peter Xu [mailto:address@hidden
> Sent: Friday, January 20, 2017 4:55 PM
> 
> On Fri, Jan 20, 2017 at 08:32:06AM +0000, Tian, Kevin wrote:
> > > From: Peter Xu [mailto:address@hidden
> > > Sent: Friday, January 13, 2017 11:06 AM
> > >
> > > 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>
> > > Signed-off-by: Peter Xu <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(+)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index ec62239..2868e37 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -2107,6 +2107,7 @@ static Property vtd_properties[] = {
> > >      DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
> > >                              ON_OFF_AUTO_AUTO),
> > >      DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false),
> > > +    DEFINE_PROP_BOOL("cache-mode", IntelIOMMUState, cache_mode_enabled,
> > > FALSE),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >
> > > @@ -2488,6 +2489,10 @@ static void vtd_init(IntelIOMMUState *s)
> > >          s->ecap |= VTD_ECAP_DT;
> > >      }
> > >
> > > +    if (s->cache_mode_enabled) {
> > > +        s->cap |= VTD_CAP_CM;
> > > +    }
> > > +
> >
> > I think some of my old comments are not answered:
> >
> > 1) Better to use caching_mode to follow spec
> 
> Sure.
> 
> >
> > 2) Does it make sense to automatically set this flag if any VFIO device
> > has been statically assigned when starting Qemu?
> 
> I'm okay with both, considering that people using this flag will be
> possibly advanced users. So I would like to hear others' opinion.
> 
> > Also for hot-add
> > device path, some check of caching mode is required. If not set,
> > should we fail hot-add operation? I don't think we have such physical
> > platform with some devices behind IOMMU while others not.
> 
> Could you explain in what case will we fail a hot plug?
> 

user enables intel-iommu, but don't set caching mode.

Then later user hot-add a PCI device to the VM. Guest will assume
newly assigned device also behind the default vIOMMU, and thus
needs to setup IOVA mappings, which is then broken...

Thanks
Kevin