[v6,3/3] intel_iommu: allow UNMAP notifiers

Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
Before that is achieved, let's open a door for vhost DMAR support, which
only requires cache invalidations (UNMAP operations).

Meanwhile, converting hw_error() to error_report() and exit(1), to make
the error messages clean and obvious (so no CPU registers will be
dumped).

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

On Wed, Sep 21, 2016 at 12:58:56PM +0800, Peter Xu wrote:
> Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
> Before that is achieved, let's open a door for vhost DMAR support, which
> only requires cache invalidations (UNMAP operations).
> 
> Meanwhile, converting hw_error() to error_report() and exit(1), to make
> the error messages clean and obvious (so no CPU registers will be
> dumped).
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Uh.. I didn't send an R-b for this.  In fact I explicitly said I
didn't think it should be applied until notifications have actually
been implemented by the intel viommu.  I still think that, and think
this should just be dropped.

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9d49be7..e4c3681 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1980,10 +1980,14 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
>  {
>      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
>  
> -    hw_error("Device at bus %s addr %02x.%d requires iommu notifier which "
> -             "is currently not supported by intel-iommu emulation",
> -             vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> -             PCI_FUNC(vtd_as->devfn));
> +    if (new & IOMMU_NOTIFIER_MAP) {
> +        error_report("Device at bus %s addr %02x.%d requires iommu "
> +                     "notifier which is currently not supported by "
> +                     "intel-iommu emulation",
> +                     vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> +                     PCI_FUNC(vtd_as->devfn));
> +        exit(1);
> +    }
>  }
>  
>  static const VMStateDescription vtd_vmstate = {

On Thu, Sep 22, 2016 at 03:24:43PM +1000, David Gibson wrote:
> On Wed, Sep 21, 2016 at 12:58:56PM +0800, Peter Xu wrote:
> > Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
> > Before that is achieved, let's open a door for vhost DMAR support, which
> > only requires cache invalidations (UNMAP operations).
> > 
> > Meanwhile, converting hw_error() to error_report() and exit(1), to make
> > the error messages clean and obvious (so no CPU registers will be
> > dumped).
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Uh.. I didn't send an R-b for this.  In fact I explicitly said I
> didn't think it should be applied until notifications have actually
> been implemented by the intel viommu.  I still think that, and think
> this should just be dropped.

Please refer to:

  https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03034.html

So this line is there since v5.

I took it an honor (and also with my thankfulness) to have added your
r-b line here. I assume what you meant before was: the patch content
is okay, but you would suggest to drop this patch in this series, and
merge this until we got a real implementations for the notifiers. IMHO
that does not mean "remove your r-b in this patch". If you meant to
remove this line (I think not?), please confirm and I can remove it.

I posted patch 3 just to make sure everything is coherent, and let
Paolo decide which way to choose (since I still think it's okay
actually... but again both are ok to me). Also it'll be easier for
Jason to track this down as well (so when Jason sees that Paolo
dropped patch 3, he'll naturally pick it up). If you still insist on
dropping this patch, I'll do it in v7.

Thanks.

-- peterx

On 22/09/2016 07:55, Peter Xu wrote:
> I posted patch 3 just to make sure everything is coherent, and let
> Paolo decide which way to choose (since I still think it's okay
> actually... but again both are ok to me). Also it'll be easier for
> Jason to track this down as well (so when Jason sees that Paolo
> dropped patch 3, he'll naturally pick it up). If you still insist on
> dropping this patch, I'll do it in v7.

I think it's better to keep this patch.

Paolo

On Thu, Sep 22, 2016 at 01:55:38PM +0800, Peter Xu wrote:
> On Thu, Sep 22, 2016 at 03:24:43PM +1000, David Gibson wrote:
> > On Wed, Sep 21, 2016 at 12:58:56PM +0800, Peter Xu wrote:
> > > Intel vIOMMU is still lacking of a complete IOMMU notifier mechanism.
> > > Before that is achieved, let's open a door for vhost DMAR support, which
> > > only requires cache invalidations (UNMAP operations).
> > > 
> > > Meanwhile, converting hw_error() to error_report() and exit(1), to make
> > > the error messages clean and obvious (so no CPU registers will be
> > > dumped).
> > > 
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Uh.. I didn't send an R-b for this.  In fact I explicitly said I
> > didn't think it should be applied until notifications have actually
> > been implemented by the intel viommu.  I still think that, and think
> > this should just be dropped.
> 
> Please refer to:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03034.html
> 
> So this line is there since v5.
> 
> I took it an honor (and also with my thankfulness) to have added your
> r-b line here. I assume what you meant before was: the patch content
> is okay, but you would suggest to drop this patch in this series, and
> merge this until we got a real implementations for the notifiers. IMHO
> that does not mean "remove your r-b in this patch". If you meant to
> remove this line (I think not?), please confirm and I can remove it.

Fair enough.  Sorry for my crabbiness.

> I posted patch 3 just to make sure everything is coherent, and let
> Paolo decide which way to choose (since I still think it's okay
> actually... but again both are ok to me). Also it'll be easier for
> Jason to track this down as well (so when Jason sees that Paolo
> dropped patch 3, he'll naturally pick it up). If you still insist on
> dropping this patch, I'll do it in v7.
> 
> Thanks.
> 
> -- peterx
>

On Thu, Sep 22, 2016 at 09:44:10AM +0200, Paolo Bonzini wrote:
> 
> 
> On 22/09/2016 07:55, Peter Xu wrote:
> > I posted patch 3 just to make sure everything is coherent, and let
> > Paolo decide which way to choose (since I still think it's okay
> > actually... but again both are ok to me). Also it'll be easier for
> > Jason to track this down as well (so when Jason sees that Paolo
> > dropped patch 3, he'll naturally pick it up). If you still insist on
> > dropping this patch, I'll do it in v7.
> 
> I think it's better to keep this patch.

Ok.  I disagree, but I won't argue.