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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <aik@ozlabs.ru>
>>>> ---
>>>>   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