Re: [PATCH 1/3] intel-iommu: fail MAP notifier without caching mode

[Peter Xu]

On Wed, Nov 30, 2022 at 02:23:06PM +0800, Jason Wang wrote:
> On Tue, Nov 29, 2022 at 11:35 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Tue, Nov 29, 2022 at 04:10:35PM +0800, Jason Wang wrote:
> > > Without caching mode, MAP notifier won't work correctly since guest
> > > won't send IOTLB update event when it establishes new mappings in the
> > > I/O page tables. Let's fail the IOMMU notifiers early instead of
> > > misbehaving silently.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/i386/intel_iommu.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index a08ee85edf..9143376677 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3186,6 +3186,13 @@ static int 
> > > vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > >                           "Snoop Control with vhost or VFIO is not 
> > > supported");
> > >          return -ENOTSUP;
> > >      }
> > > +    if (!s->caching_mode && (new & IOMMU_NOTIFIER_MAP)) {
> > > +        error_setg_errno(errp, ENOTSUP,
> > > +                         "device %02x.%02x.%x requires caching mode",
> > > +                         pci_bus_num(vtd_as->bus), 
> > > PCI_SLOT(vtd_as->devfn),
> > > +                         PCI_FUNC(vtd_as->devfn));
> > > +        return -ENOTSUP;
> > > +    }
> >
> > We used to have that but got reverted because it's too late to fail, so we
> > moved it over even though not as clean..
> >
> > https://lore.kernel.org/all/20190916080718.3299-5-peterx@redhat.com/
> 
> One of the difference is that the patch doesn't do exit() here. I
> think it's better to fail instead of misbehving silently, this is what
> other vIOMMU did:
> 
> E.g in smmu we had:
> 
>     if (new & IOMMU_NOTIFIER_MAP) {
>         error_setg(errp,
>                    "device %02x.%02x.%x requires iommu MAP notifier which is "
>                    "not currently supported", pci_bus_num(sdev->bus),
>                    PCI_SLOT(sdev->devfn), PCI_FUNC(sdev->devfn));
>         return -EINVAL;
>     }
> 
> So did for amd iommu.

Yeah that's fine.  Would you mind add the explanation (and also above link,
which might be helpful to pick up the history..) to the commit message?
With that:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu