Re: [Qemu-devel] [PATCH qemu] memory: Fix IOMMU replay base address

[Alexey Kardashevskiy]

On 02/22/2016 11:12 PM, David Gibson wrote:
> On Mon, Feb 22, 2016 at 10:36:19PM +1100, Alexey Kardashevskiy wrote:
> > On 02/22/2016 05:26 PM, David Gibson wrote:
> > > On Mon, Feb 22, 2016 at 05:09:39PM +1100, Alexey Kardashevskiy wrote:
> > > > Since a788f227 "memory: Allow replay of IOMMU mapping notifications"
> > > > when new VFIO listener is added, all existing IOMMU mappings are replayed.
> > > > However there is a problem that the base address of an IOMMU memory region
> > > > (IOMMU MR) is ignored which is not a problem for the existing user (which is
> > > > pseries) with its default 32bit DMA window starting at 0 but it is if there is
> > > > another DMA window.
> > > >
> > > > This adjusts the replaying address by mr->addr.
> > >
> > > Uh.. this doesn't look right to me.  AFAICT from the existing
> > > implementations the 'addr' parameter to the translate function is an
> > > offset within the memory region, which would make the original version
> > > correct.
> >
> > Ok, then spapr_tce_translate_iommu() needs to be fixed. Or I am missing
> > something here? The @addr field is definitely ignored now.
>
> I think at least one of us is missing something.  In the normal AS
> translation path, IIUC, it will subtract the mr->addr value to give
> offsets which are passed to translate.  It uses those offsets to find
> the TCE and returns a translated address.  Where else the @addr be
> used?

Ok, this patch is definitely wrong.

The actual problem I am debugging is this:
1. run guest with an emulated XHCI to have 2 DMA windows;
2. hotplug vfio-pci device.
3. observe failing vfio_dma_map().

This triggers memory_region_register_iommu_notifier() + 
memory_region_iommu_replay() which calls vfio_iommu_map_notify() in a loop. 
For the second window (huge one, 0x800000000000000 bus address) it fails as 
iotlb.iova's returned by spapr_tce_translate_iommu() are offsets within 
IOMMU MR.

But vfio_dma_map() (which is eventually called by vfio_iommu_map_notify()) 
needs an absolute iotlb->iova address.

imho this should be correct:

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 2e02dc6..0d6b926 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -126,7 +126,7 @@ static IOMMUTLBEntry 
spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
         hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift);

         tce = tcet->table[addr >> tcet->page_shift];
-        ret.iova = addr & page_mask;
+        ret.iova = (addr + iommu->addr) & page_mask;
         ret.translated_addr = tce & page_mask;


However this does not help either.

Because at the moment I create hardware windows from a helper which is 
called from spapr_phb_hot_plug_child() (HotplugHandlerClass::plug() 
callback). And this is too late as pci_qdev_realize("vfio-pci") calls:

vfio_connect_container -> ... -> vfio_listener_region_add -> .. -> 
memory_region_iommu_replay
and the latter fails - there is no huge window in the host kernel yet.
What is the right way of fixing this?...

This memory_region_iommu_replay() thing really creates problems for me :-/



> > > > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > > > ---
> > > >   memory.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/memory.c b/memory.c
> > > > index 09041ed..377269b 100644
> > > > --- a/memory.c
> > > > +++ b/memory.c
> > > > @@ -1436,7 +1436,7 @@ void memory_region_iommu_replay(MemoryRegion *mr, 
> > > > Notifier *n,
> > > >       IOMMUTLBEntry iotlb;
> > > >
> > > >       for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
> > > > -        iotlb = mr->iommu_ops->translate(mr, addr, is_write);
> > > > +        iotlb = mr->iommu_ops->translate(mr, mr->addr + addr, is_write);
> > > >           if (iotlb.perm != IOMMU_NONE) {
> > > >               n->notify(n, &iotlb);
> > > >           }


--
Alexey