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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 02/23/2016 05:20 PM, David Gibson wrote:
> On Tue, Feb 23, 2016 at 01:11:35PM +1100, Alexey Kardashevskiy wrote:
>> 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;
>
> I wondered about that change, but I'd have to look closer to see if
> the iova field here is expected to be relative to the MR as well.  It
> would be oddly inconsistent if it wasn't.


It is relative and it does not make sense as there is no source MR/AS in 
iotlb (only target AS) so there is no use in such iova.


>
>> 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?...
>
> Hmm.. I think we need a backend specific hook which will be called
> from vfio_listener_region_add() when it sees a new iommu region, but
> before it calls replay.
>
> Basically that notifier is enumerating to the VFIO subsystem where the
> guest has IOMMU windows.  With DDW on the host, we need something to
> mirror the guest side windows into the host (or reject them, if
> they're not compatible.

The host kernel API consists of 2 ioctls basically - create and remove a 
window. There is no "query" which would tell how many windows are there and 
their parameters. When there is a first vfio-pci device, I basically go 
through the list of emulated windows and create hardware windows for every 
active window. But I do this from spapr-phb code where I have the complete 
picture - how many windows, LIOBNs.

This vfio_listener_region_add() thing - it is called per IOMMU MR, i.e. 
once or twice per PHB, in undefined order, not good (however the order 
never changes in the reality). And how exactly do I connect VFIO container 
and spapr PHB (presumably using QOM)? I cannot think of any good interface 
now... Have another memory listener on PHB's AS in spapr_pci?


> Actually.. that hook should replace the test a little bit higher
> testing against container->min_iova and container->max_iova.  With
> host-side DDW the set of allowable IOVAs is no longer a single, simple
> range - and it's now changeable.

I am missing the purpose of container->min_iova/max_iova, on x86 these are 
constant values and on pseries - QEMU chooses the windows parameters when 
creates the windows so this check is more like assert...


-- 
Alexey