Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed

[Peter Xu]

On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote:

[...]

> > -static void vtd_iommu_notify_started(MemoryRegion *iommu)
> > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
> > +                                          IOMMUNotifierFlag old,
> > +                                          IOMMUNotifierFlag new)
> >  {
> >      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> 
> Shouldn't this have a sanity check that the new flags doesn't include
> MAP actions?

See your r-b for patch 3, thanks! So skipping this one.

[...]

> > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu,
> > +                                          IOMMUNotifierFlag old,
> > +                                          IOMMUNotifierFlag new)
> > +{
> > +    if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) {
> > +        spapr_tce_notify_started(iommu);
> > +    } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) {
> > +        spapr_tce_notify_stopped(iommu);
> > +    }
> 
> This is wrong.  We need to do the notify_start and stop actions if
> *any* bits are set in the new/old flags, not just if all of them are
> set.

Power should need both, right? I can switch all

  "== IOMMU_NOTIFIER_ALL"

into:

  "!= IOMMU_NOTIFIER_NONE"

in the next version if you like, but AFAICT they are totally the same.

> 
> I'd also prefer to see notify_started and notify_stopped folded into
> this function.

I can do that for v5.

Thanks,

-- peterx