Re: [Qemu-devel] [RFC PATCH 11/13] intel_iommu: provide its own replay() callback

[Liu, Yi L]

> -----Original Message-----
> From: Peter Xu [mailto:address@hidden
> Sent: Thursday, December 29, 2016 5:56 PM
> To: Liu, Yi L <address@hidden>
> Cc: Tian, Kevin <address@hidden>; Lan, Tianyu <address@hidden>; 'qemu-
> address@hidden' <address@hidden>; 'address@hidden'
> <address@hidden>
> Subject: Re: [Qemu-devel] [RFC PATCH 11/13] intel_iommu: provide its own 
> replay()
> callback
> 
> On Thu, Dec 29, 2016 at 07:38:20AM +0000, Liu, Yi L wrote:
> 
> [...]
> 
> > > +static void vtd_iommu_replay(MemoryRegion *mr, IOMMUNotifier *n)
> > > +{
> > > +    VTDAddressSpace *vtd_as = container_of(mr, VTDAddressSpace, iommu);
> > > +    IntelIOMMUState *s = vtd_as->iommu_state;
> > > +    uint8_t bus_n = pci_bus_num(vtd_as->bus);
> > > +    VTDContextEntry ce;
> > > +
> > > +    if (vtd_dev_to_context_entry(s, bus_n, vtd_as->devfn, &ce) == 0) {
> >
> > Hi Peter,
> >
> > Again I'd like to give my cheers on this patch set.
> 
> Thanks.
> 
> >
> > Hereby, I have a minor concern on this part.
> > The replay would be performed in vfio_listener_region_add() when device is
> > assigned. Since guest is just started, so there will be no valid context 
> > entry for the
> > assigned device. So there will be no vtd_page_walk. Thus no change to SL 
> > page
> > table in host. The SL page table would still be a IOVA->HPA mapping. It's 
> > true
> > that the gIOVA->HPA mapping can be built by page map/unmap when guest
> > trying to issue dma. So it is just ok without relpay in the beginning. Hot 
> > plug should
> > be all the same. I guess it is the design here?
> 
> I am not sure whether I get your point here - I think it's by design.
> We don't really need replay if we are sure that the IOMMU address
> space has totally no mapping (i.e., when the guest init). Replay is
> only useful when there are existing mappings in the IOMMU address
> space.
yes, if we are sure no existing mapping, it is ok. There is another case which 
may
complain it. If a device is hotplug in, then there should be existing mapping. 
Just
looked code again. If corresponding context entry is not present, the
vtd_page_walk should be bypassed.

> 
> Another thing to mention: IMHO IOVA -> HPA mapping should be built by
> page map/unmap when guest issues IOTLB invalidations, not DMA (guest
> will first issue IOTLB invalidations, only after that the iova address
> would be a valid DMA address).
Exactly. thx for the correction.

> 
> >
> > However, it may be incompatible with a future work in which we expose an
> > vIOMMU to guest but guest disable IOVA usage. In this scenario, the SL page
> > table in host would be a GPA->HPA instead of a gIOVA->HPA mapping. It would
> > rely on memory_replay to build a GPA->HPA mapping. If I'm correct, I think 
> > it
> > would be expected to have a replay in the time device is assigned.
> 
> If you need a GPA -> HPA mapping (AFAICT you mean virtualized SVM and
> the so-called first level translation), IMHO the default mapping
> behavior of vfio_listener_region_add() suites here (when
> memory_region_is_iommu(section->mr) == false), which maps the whole
> guest physical address space, just like the case when we use vfio
> devices without vIOMMU.
Yes, I mean SVM virtualizaion. To virtualize SVM, an vIOMMU would be exposed to
guest. So memory_region_is_iommu(section->mr) would be true. Then, the original
mapping the whole guest physical address space would not run. Thus no GPA->HPA
mapping would be built. This is what I really concern here.

Thanks,
Yi L