[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_c
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed |
Date: |
Wed, 14 Sep 2016 17:22:40 +1000 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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
- [Qemu-devel] [PATCH v4 1/3] memory: introduce IOMMUNotifier and its caps, (continued)
[Qemu-devel] [PATCH v4 2/3] memory: introduce IOMMUOps.notify_flag_changed, Peter Xu, 2016/09/08
[Qemu-devel] [PATCH v4 3/3] intel_iommu: allow UNMAP notifiers, Peter Xu, 2016/09/08
Re: [Qemu-devel] [PATCH v4 0/3] Introduce IOMMUNotifier struct, no-reply, 2016/09/09