On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote:
> 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"
Yes, that should be right.
> in the next version if you like, but AFAICT they are totally the
> same.
Again, only if you assume things about how the notifiers will be used,
which is not a good look when designing an interface.
>
> >
> > I'd also prefer to see notify_started and notify_stopped folded into
> > this function.
>
> I can do that for v5.
>
> Thanks,
>
> -- peterx
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
