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
