Re: [Qemu-devel] [RFC PATCH 6/8] memory: introduce AddressSpaceOps

[Liu, Yi L]

Hi David,

I added some update on your question. See if it make any sense.

> -----Original Message-----
> From: David Gibson [mailto:address@hidden
> Sent: Friday, May 26, 2017 1:30 PM
> To: Peter Xu <address@hidden>
> On Thu, May 25, 2017 at 03:24:30PM +0800, Peter Xu wrote:

[...]

> > > > > > >
> > > > > > > I don't believe we have any existing platforms with both an
> > > > > > > IOMMU and a direct mapped window in a device's address
> > > > > > > space.  But it seems to be just too plausible a setup to not
> > > > > > > plan for it. [1]
> > > > > > >
> > > > > > > > This patch derived from a requirement in virt-svm project (on 
> > > > > > > > x86).
> > > > > > > > Virt-svm needs some notification mechanism for each IOMMU
> > > > > > > > (or say, the IOMMU that managers the SVM-enabled device).
> > > > > > > > For now, all IOMMU notifiers are per-memory-region not
> > > > > > > > per-iommu, and that's imho not what virt-svm wants. Any 
> > > > > > > > suggestions?
> > > > > > >
> > > > > > > I don't know SVM, so I can't really make sense of that.
> > > > > > > What format does this identifier need?  What does "for one
> > > > > > > IOMMU" mean in this context - i.e. what guest observable
> > > > > > > properties require the IDs to be the same or to be different.
> > > > > >
> > > > > > Virt-svm should need to trap the content of a register
> > > > > > (actually the data is in the memory, but, let's assume it's a
> > > > > > mmio operation for simplicity, considering it is finally
> > > > > > delivered via invalidation requests), then pass that info down
> > > > > > to kernel. So the listened element is per-iommu not per-mr
> > > > > > this time. When the content changed, vfio will need to be notified, 
> > > > > > then
> pass this info down.
> > > > >
> > > > > I don't entirely follow what you're saying.  When the virtual
> > > > > hardware gets an invalidate request, it looks up the unit to
> > > > > invalidate in memory?  Which component gets to decide that ID?
> > > > > How is it advertised to the guest OS?
> > > > >
> > > > > If your ID is tied to the AS now, you could just iterate through
> > > > > the AS and invalidate any IOMMU MRs that are present within it.
> > > > >
> > > > > Alternatively, if the ID is tied to something more concrete,
> > > > > like a specific PCI host bridge (which incorporates the IOMMU
> > > > > logic), then that device probably already has a handle on the
> > > > > right IOMMU MR to invalidate it.
> > > >
> > > > Sorry to be unclear on the requirement. I don't know what's the ID
> > > > you mentioned above... Anyway, let me try to further simplify the
> > > > use case.
> > >
> > > Right, the ID was me guessing badly at what's going on here, so I
> > > think it confused rather than clarifying.
> > >
> > > > Just assume we have such a requirement: when one register of
> > > > vIOMMU changes, we need to pass this register data to the hardware
> > > > IOMMU by some way. And, let's assume this is a notification
> > > > mechanism, so that every device in the system can listen to this
> > > > register change, then capture what has changed to what. Here the
> > > > point is, in all cases this event is not related to memory region
> > > > at all. So imho we need some other way to do it besides memory region
> IOMMU notifiers.
> > >
> > > Ok.  So is this right?
> > >   * You have a single bank of vIOMMU registers
> > >   * Which control two (or more) IOMMU regions in in the guest's
> > >           address space
> > >   * Assuming the host also has an AMD IOMMU, those will be
> > >           backed by a single IOMMU on the host ("single" meaning
> > >           controlled by a single bank of host registers)
> > >
> > > I'm assuming the guest IOMMU code must know which IOMMU regions it
> > > is managing, so getting from the guest registers to the set of IOMMU
> > > MRs should be easy.
> > >
> > > What's the operation that needs to happen on the host IOMMU, in
> > > terms of the VFIO IOMMU interface?
> >
> > (Sorry to respond so late...)
> >
> > It'll pass the captured data downward to host IOMMU in some way.
> >
> > IMHO if we are discussing the notifier thing only, we don't really
> > need to know what would it do after it gets the data. The point is how
> > we should define this kind if notifies, which differs from current
> > memory region based notifiers.
> 
> I'm trying to understand how it differs - I still don't have a clear picture. 
>  That's why
> I'm asking what needs to be passed to the host MMU, so I can see why you need 
> this
> different notifier.

It differs in the registration. For the MemoryRegion based notifier(MAP/UNMAP), 
it
is registered when IOMMU MemoryRegion is used. On VT-d, it is the time switch
system memory address space to vtd address space. This is introduced by the 
patch
below:

[Qemu-devel] [PATCH v9 8/9] intel_iommu: allow dynamic switch of IOMMU region

For virt-SVM, it wants to add two more notfiers. If we add the notifiers at the 
same
time with the MAP/UNMAP notifier, then virt-SVM cannot work if there is no IOMMU
region switch. This scenario happens when passthru-mode is used in the vIOMMU.

So in order to solve the scenario above, I discussed with Peter. My initial 
plan is to move
the registration of new notifiers to be in vfio_realize(). The logic is if 
vIOMMU is exposed
to guest, then the new notifiers would be registered. This approach is proposed 
in my
RFC patch for virt-SVM.

https://www.spinics.net/lists/kvm/msg148803.html

However, Peter mentioned the approach in the RFC patch may not be compatible 
with
sPAPR since there may be multiple MemoryRegion for sPARR. It would result in 
multiple
notifier registration on sPARR.

Due to it, Peter tries to introduce the IOMMUObject. It would help the 
detection in
vfio_realize() to check if vIOMMU is exposed to guest. Also it moves the iommu 
specific
notifiers to be tied on the exposure of vIOMMU instead of IOMMU Region 
switching.

Any further comments, pls let me know.

Thanks,
Yi L