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

2017-01-21 Thread Peter Xu 
On Fri, Jan 20, 2017 at 09:42:25AM -0600, Eric Blake wrote:
> On 01/12/2017 09:06 PM, Peter Xu wrote:
> > From: Aviv Ben-David 
> 
> Long subject line, please try to keep it around 60 or so characters
> (look at 'git shortlog -30' for comparison).  Also, fix the typos:
> s/capility exposoed/capability exposed/

Will fix this and repost this single patch as v4.1 based on v4 series.

Thanks!

> 
> > 
> > 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 
> > Signed-off-by: Peter Xu 
> > ---
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

-- peterx

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

2017-01-20 Thread Eric Blake 
On 01/12/2017 09:06 PM, Peter Xu wrote:
> From: Aviv Ben-David 

Long subject line, please try to keep it around 60 or so characters
(look at 'git shortlog -30' for comparison).  Also, fix the typos:
s/capility exposoed/capability exposed/

> 
> 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 
> Signed-off-by: Peter Xu 
> ---

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2017-01-20 Thread Peter Xu 
On Fri, Jan 20, 2017 at 09:20:01AM +, Tian, Kevin wrote:
> > From: Peter Xu [mailto:pet...@redhat.com]
> > Sent: Friday, January 20, 2017 5:12 PM
> > 
> > On Fri, Jan 20, 2017 at 08:59:01AM +, Tian, Kevin wrote:
> > 
> > [...]
> > 
> > > > > 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...
> > 
> > Is the newly added device a vfio-pci device? If so, we should hit
> > this and VM will stops to work:
> > 
> > if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
> > error_report("We need to set cache_mode=1 for intel-iommu to enable 
> > "
> >  "device assignment with IOMMU protection.");
> > exit(1);
> > }
> 
> sorry I didn't found this code. In which code path is it hit?

It's in patch 14/14 of this series.

> 
> > 
> > I admit this is not user-friendly, and a better way may be that we
> > disallow the hot-plug in that case, telling the user about the error,
> > rather than crashing the VM. But, I think that can be a patch outside
> > this series, considering (again) that this only affects advanced
> > users.
> > 
> 
> Crashing VM is bad but anyway, I'll leave maintainer to decide
> whether they'd like it fixed now or later. :-)

Sure. Thanks,

-- peterx

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

2017-01-20 Thread Tian, Kevin 
> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Friday, January 20, 2017 5:12 PM
> 
> On Fri, Jan 20, 2017 at 08:59:01AM +, Tian, Kevin wrote:
> 
> [...]
> 
> > > > 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...
> 
> Is the newly added device a vfio-pci device? If so, we should hit
> this and VM will stops to work:
> 
> if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
> error_report("We need to set cache_mode=1 for intel-iommu to enable "
>  "device assignment with IOMMU protection.");
> exit(1);
> }

sorry I didn't found this code. In which code path is it hit?

> 
> I admit this is not user-friendly, and a better way may be that we
> disallow the hot-plug in that case, telling the user about the error,
> rather than crashing the VM. But, I think that can be a patch outside
> this series, considering (again) that this only affects advanced
> users.
> 

Crashing VM is bad but anyway, I'll leave maintainer to decide
whether they'd like it fixed now or later. :-)

Thanks
Kevin

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

2017-01-20 Thread Peter Xu 
On Fri, Jan 20, 2017 at 08:59:01AM +, Tian, Kevin wrote:

[...]

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

Is the newly added device a vfio-pci device? If so, we should hit
this and VM will stops to work:

if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
error_report("We need to set cache_mode=1 for intel-iommu to enable "
 "device assignment with IOMMU protection.");
exit(1);
}

I admit this is not user-friendly, and a better way may be that we
disallow the hot-plug in that case, telling the user about the error,
rather than crashing the VM. But, I think that can be a patch outside
this series, considering (again) that this only affects advanced
users.

Thanks,

-- peterx

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

2017-01-20 Thread Tian, Kevin 
> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Friday, January 20, 2017 4:55 PM
> 
> On Fri, Jan 20, 2017 at 08:32:06AM +, Tian, Kevin wrote:
> > > From: Peter Xu [mailto:pet...@redhat.com]
> > > Sent: Friday, January 13, 2017 11:06 AM
> > >
> > > From: Aviv Ben-David 
> > >
> > > 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 
> > > Signed-off-by: Peter Xu 
> > > ---
> > >  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

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

2017-01-20 Thread Peter Xu 
On Fri, Jan 20, 2017 at 08:32:06AM +, Tian, Kevin wrote:
> > From: Peter Xu [mailto:pet...@redhat.com]
> > Sent: Friday, January 13, 2017 11:06 AM
> > 
> > From: Aviv Ben-David 
> > 
> > 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 
> > Signed-off-by: Peter Xu 
> > ---
> >  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?

Thanks,

-- peterx

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

2017-01-20 Thread Tian, Kevin 
> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Friday, January 13, 2017 11:06 AM
> 
> From: Aviv Ben-David 
> 
> 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 
> Signed-off-by: Peter Xu 
> ---
>  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

2) Does it make sense to automatically set this flag if any VFIO device
has been statically assigned when starting Qemu? 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.

>  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 356f188..4104121 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -202,6 +202,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 405c9d1..749eef9 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -257,6 +257,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 */
> --
> 2.7.4