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

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

[Alexey Kardashevskiy]

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.

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);
         }
-- 
2.5.0.rc3

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

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.

> 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);
>          }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Alexey Kardashevskiy]

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.


>
>> 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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

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?


> 
> 
> >
> >>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);
> >>          }
> >
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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 <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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4248 bytes --]

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.

> 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.

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Alexey Kardashevskiy]

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

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

[Paolo Bonzini]

On 23/02/2016 10:00, Alexey Kardashevskiy wrote:
>>>
>>>           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.

ret.iova should be relative to the source AS (i.e. even if a 32-bit
IOMMU region translates between 4GB and 8GB, ret.iova should have bits
32-63 set to 0).

So there is a problem in vfio_iommu_map_notify:

        ret = vfio_dma_map(container, iotlb->iova,
                           iotlb->addr_mask + 1, vaddr,
                           !(iotlb->perm & IOMMU_WO) || mr->readonly);

I think that, in vfio_listener_region_add, the iova variable should be
stored in VFIOGuestIOMMU for use in vfio_iommu_map_notify.

ret.translated_addr should be relative to the target AS, which VFIO
assumes to be address_space_memory.

Paolo

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]

On Tue, Feb 23, 2016 at 10:56:57AM +0100, Paolo Bonzini wrote:
> 
> 
> On 23/02/2016 10:00, Alexey Kardashevskiy wrote:
> >>>
> >>>           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.
> 
> ret.iova should be relative to the source AS (i.e. even if a 32-bit
> IOMMU region translates between 4GB and 8GB, ret.iova should have bits
> 32-63 set to 0).

Uh.. relative to the source AS, or the source MR?  At the moment spapr
implements it relative to the MR.. which seems to match your example.

> So there is a problem in vfio_iommu_map_notify:
> 
>         ret = vfio_dma_map(container, iotlb->iova,
>                            iotlb->addr_mask + 1, vaddr,
>                            !(iotlb->perm & IOMMU_WO) || mr->readonly);

Ah.. yeah.. this needs the address relative to the AS, not the MR

> I think that, in vfio_listener_region_add, the iova variable should be
> stored in VFIOGuestIOMMU for use in vfio_iommu_map_notify.

Hmm, seems a bit clunky, though I guess it's safe due to the BQL.

> ret.translated_addr should be relative to the target AS, which VFIO
> assumes to be address_space_memory.

Right, and as far as I can tell that is the case.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Alexey Kardashevskiy]

On 02/23/2016 08:56 PM, Paolo Bonzini wrote:
>
>
> On 23/02/2016 10:00, Alexey Kardashevskiy wrote:
>>>>
>>>>            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.
>
> ret.iova should be relative to the source AS (i.e. even if a 32-bit
> IOMMU region translates between 4GB and 8GB, ret.iova should have bits
> 32-63 set to 0).


In my test branch with 2 DMA windows I have such PHB AS:

address-space: pci@800000020000000
   0000000000000000-ffffffffffffffff (prio 0, RW): 
pci@800000020000000.iommu-root
     0000000000000000-ffffffffffffffff (prio 0, RW): tce-root-80000001
       0800000000000000-08000000ffffffff (prio 0, RW): tce-iommu-80000001
     0000000000000000-ffffffffffffffff (prio 0, RW): tce-root-80000000
       0000000000000000-000000003fffffff (prio 0, RW): tce-iommu-80000000
     0000040000000000-000004000000ffff (prio 0, RW): msi


The source AS is 0..(u64)-1. iotlb.iova from 
spapr_tce_translate_iommu(tce-root-80000001) will be relative to 
0800000000000000 which is not source AS.

What do I miss here?

>
> So there is a problem in vfio_iommu_map_notify:
>
>          ret = vfio_dma_map(container, iotlb->iova,
>                             iotlb->addr_mask + 1, vaddr,
>                             !(iotlb->perm & IOMMU_WO) || mr->readonly);
>
> I think that, in vfio_listener_region_add, the iova variable should be
> stored in VFIOGuestIOMMU for use in vfio_iommu_map_notify.
>
> ret.translated_addr should be relative to the target AS, which VFIO
> assumes to be address_space_memory.

That is perfectly fine - there is iotlb.target_as.



-- 
Alexey