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: Friday, December 30, 2016 11:44 AM
> 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 10:23:21AM +0000, Liu, Yi L wrote:
> 
> [...]
> 
> > > >
> > > > 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.
> 
> Yes, if the context entry is not present, the replay will be bypassed.
> And if it exists, then the replay will work (along with the page walk
> process). Shouldn't that the behavior we want?
> 
> Please clarify if I misunderstood your question.
I was just assuming a replay should be performed when a new device is plug in.
However, according to your statements, it is not required. Is this the 
conclusion in
the previous discussion?

> [...]
> 
> > > >
> > > > 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.
> 
> What I meant above was, we might be able to leverage existing
> no-viommu logic to build the SLPT for virtualized SVM. For example, we
> can switch the if block from:
> 
>   if (memory_region_is_iommu(section->mr))
> 
> into something similiar like:
> 
>   if (memory_region_is_iommu(section->mr) &&
>       !FIRST_LEVEL_TRANSLATION_ENABLED(section->mr))
> 
> Just a quick thought, not sure whether that would be an applicable
> idea. Thanks,
If I remember correctly, with vIOMMU exposed, section->mr here should
not be a ram, the logic would fail in the following code. I agree with you
that map the whole guest physical address space should work with SVM
virtualization. May need to think more on how to make it happen.

hw/i386/common.c, a snippet:
    /* Here we assume that memory_region_is_ram(section->mr)==true */

    vaddr = memory_region_get_ram_ptr(section->mr) +
            section->offset_within_region +
            (iova - section->offset_within_address_space);

Thanks,
Yi L