On Fri, Dec 30, 2016 at 04:39:49AM +0000, Liu, Yi L wrote: > > -----Original Message----- > > From: Peter Xu [mailto:pet...@redhat.com] > > Sent: Friday, December 30, 2016 11:44 AM > > To: Liu, Yi L <yi.l....@intel.com> > > Cc: Tian, Kevin <kevin.t...@intel.com>; Lan, Tianyu <tianyu....@intel.com>; > > 'qemu- > > de...@nongnu.org' <qemu-devel@nongnu.org>; 'bd.a...@gmail.com' > > <bd.a...@gmail.com> > > 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?
Hmm, I mean whether the replay is needed for the hotplugged device should depend on which iommu domain it joins when it is plugged. I think it should belong to nowhere when plugged but I am not quite sure... IIUC that'll depend on whether the context entry of that plugged device is valid - and I think it should be an invalid one. > > > [...] > > > > > > > > > > > > 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); Right. That code won't work if only with a change in the if() clause - I wasn't showing any workable codes (even it's not pesudo), only to show what I meant. :) -- peterx
