[Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Peter Xu]

The problem is that, address_space_get_iotlb_entry() shares a lot with
address_space_translate(). This patch tries to abstract the
shared elements.

Originally, this work is derived from discussion from VT-d passthrough
series discussions [1]. But for sure we can just see this series as a
standalone cleanup. So I posted it separately here.

Smoke tests are done with general VM boots, IOs, especially with vhost
dmar configurations.

I believe with current series I can throw away the old patch [1],
which may be good. But before that, please kindly review. Thanks.

References:

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg02335.html

Peter Xu (4):
  exec: simplify phys_page_find() params
  exec: rename resolve_subpage
  exec: further use is_mmio
  exec: abstract address_space_do_translate()

 exec.c | 122 ++++++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 76 insertions(+), 46 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] exec: simplify phys_page_find() params

[Peter Xu]

It really only plays with the dispatchers, so the parameter list does
not need that complexity. This helps for readability at least.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 exec.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/exec.c b/exec.c
index eac6085..1d76c63 100644
--- a/exec.c
+++ b/exec.c
@@ -371,10 +371,11 @@ static inline bool section_covers_addr(const MemoryRegionSection *section,
                              int128_getlo(section->size), addr);
 }
 
-static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
-                                           Node *nodes, MemoryRegionSection *sections)
+static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr addr)
 {
-    PhysPageEntry *p;
+    PhysPageEntry lp = d->phys_map, *p;
+    Node *nodes = d->map.nodes;
+    MemoryRegionSection *sections = d->map.sections;
     hwaddr index = addr >> TARGET_PAGE_BITS;
     int i;
 
@@ -412,8 +413,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
         section_covers_addr(section, addr)) {
         update = false;
     } else {
-        section = phys_page_find(d->phys_map, addr, d->map.nodes,
-                                 d->map.sections);
+        section = phys_page_find(d, addr);
         update = true;
     }
     if (resolve_subpage && section->mr->subpage) {
@@ -1246,8 +1246,7 @@ static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *secti
     subpage_t *subpage;
     hwaddr base = section->offset_within_address_space
         & TARGET_PAGE_MASK;
-    MemoryRegionSection *existing = phys_page_find(d->phys_map, base,
-                                                   d->map.nodes, d->map.sections);
+    MemoryRegionSection *existing = phys_page_find(d, base);
     MemoryRegionSection subsection = {
         .offset_within_address_space = base,
         .size = int128_make64(TARGET_PAGE_SIZE),
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/4] exec: simplify phys_page_find() params

[David Gibson]

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

On Mon, May 15, 2017 at 04:50:57PM +0800, Peter Xu wrote:
> It really only plays with the dispatchers, so the parameter list does
> not need that complexity. This helps for readability at least.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  exec.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index eac6085..1d76c63 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -371,10 +371,11 @@ static inline bool section_covers_addr(const MemoryRegionSection *section,
>                               int128_getlo(section->size), addr);
>  }
>  
> -static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
> -                                           Node *nodes, MemoryRegionSection *sections)
> +static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr addr)
>  {
> -    PhysPageEntry *p;
> +    PhysPageEntry lp = d->phys_map, *p;
> +    Node *nodes = d->map.nodes;
> +    MemoryRegionSection *sections = d->map.sections;
>      hwaddr index = addr >> TARGET_PAGE_BITS;
>      int i;
>  
> @@ -412,8 +413,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>          section_covers_addr(section, addr)) {
>          update = false;
>      } else {
> -        section = phys_page_find(d->phys_map, addr, d->map.nodes,
> -                                 d->map.sections);
> +        section = phys_page_find(d, addr);
>          update = true;
>      }
>      if (resolve_subpage && section->mr->subpage) {
> @@ -1246,8 +1246,7 @@ static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *secti
>      subpage_t *subpage;
>      hwaddr base = section->offset_within_address_space
>          & TARGET_PAGE_MASK;
> -    MemoryRegionSection *existing = phys_page_find(d->phys_map, base,
> -                                                   d->map.nodes, d->map.sections);
> +    MemoryRegionSection *existing = phys_page_find(d, base);
>      MemoryRegionSection subsection = {
>          .offset_within_address_space = base,
>          .size = int128_make64(TARGET_PAGE_SIZE),

-- 
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 1/4] exec: simplify phys_page_find() params

[Paolo Bonzini]

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



On 16/05/2017 04:53, David Gibson wrote:
> On Mon, May 15, 2017 at 04:50:57PM +0800, Peter Xu wrote:
>> It really only plays with the dispatchers, so the parameter list does
>> not need that complexity. This helps for readability at least.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Queued, thanks.

Paolo

> 
>> ---
>>  exec.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index eac6085..1d76c63 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -371,10 +371,11 @@ static inline bool section_covers_addr(const MemoryRegionSection *section,
>>                               int128_getlo(section->size), addr);
>>  }
>>  
>> -static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
>> -                                           Node *nodes, MemoryRegionSection *sections)
>> +static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr addr)
>>  {
>> -    PhysPageEntry *p;
>> +    PhysPageEntry lp = d->phys_map, *p;
>> +    Node *nodes = d->map.nodes;
>> +    MemoryRegionSection *sections = d->map.sections;
>>      hwaddr index = addr >> TARGET_PAGE_BITS;
>>      int i;
>>  
>> @@ -412,8 +413,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>>          section_covers_addr(section, addr)) {
>>          update = false;
>>      } else {
>> -        section = phys_page_find(d->phys_map, addr, d->map.nodes,
>> -                                 d->map.sections);
>> +        section = phys_page_find(d, addr);
>>          update = true;
>>      }
>>      if (resolve_subpage && section->mr->subpage) {
>> @@ -1246,8 +1246,7 @@ static void register_subpage(AddressSpaceDispatch *d, MemoryRegionSection *secti
>>      subpage_t *subpage;
>>      hwaddr base = section->offset_within_address_space
>>          & TARGET_PAGE_MASK;
>> -    MemoryRegionSection *existing = phys_page_find(d->phys_map, base,
>> -                                                   d->map.nodes, d->map.sections);
>> +    MemoryRegionSection *existing = phys_page_find(d, base);
>>      MemoryRegionSection subsection = {
>>          .offset_within_address_space = base,
>>          .size = int128_make64(TARGET_PAGE_SIZE),
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 2/4] exec: rename resolve_subpage

[Peter Xu]

It is not easy for people to know "what this parameter does" before
knowing "what is subpage". Let's use "is_mmio" to make it easier to
understand.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 exec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index 1d76c63..0adae94 100644
--- a/exec.c
+++ b/exec.c
@@ -403,7 +403,7 @@ bool memory_region_is_unassigned(MemoryRegion *mr)
 /* Called from RCU critical section */
 static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
                                                         hwaddr addr,
-                                                        bool resolve_subpage)
+                                                        bool is_mmio)
 {
     MemoryRegionSection *section = atomic_read(&d->mru_section);
     subpage_t *subpage;
@@ -416,7 +416,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
         section = phys_page_find(d, addr);
         update = true;
     }
-    if (resolve_subpage && section->mr->subpage) {
+    if (is_mmio && section->mr->subpage) {
         subpage = container_of(section->mr, subpage_t, iomem);
         section = &d->map.sections[subpage->sub_section[SUBPAGE_IDX(addr)]];
     }
@@ -429,13 +429,13 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
 /* Called from RCU critical section */
 static MemoryRegionSection *
 address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *xlat,
-                                 hwaddr *plen, bool resolve_subpage)
+                                 hwaddr *plen, bool is_mmio)
 {
     MemoryRegionSection *section;
     MemoryRegion *mr;
     Int128 diff;
 
-    section = address_space_lookup_region(d, addr, resolve_subpage);
+    section = address_space_lookup_region(d, addr, is_mmio);
     /* Compute offset within MemoryRegionSection */
     addr -= section->offset_within_address_space;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/4] exec: rename resolve_subpage

[Paolo Bonzini]

On 15/05/2017 10:50, Peter Xu wrote:
> It is not easy for people to know "what this parameter does" before
> knowing "what is subpage". Let's use "is_mmio" to make it easier to
> understand.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Maybe invert the direction and call it for_iotlb?

Paolo

> ---
>  exec.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 1d76c63..0adae94 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -403,7 +403,7 @@ bool memory_region_is_unassigned(MemoryRegion *mr)
>  /* Called from RCU critical section */
>  static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>                                                          hwaddr addr,
> -                                                        bool resolve_subpage)
> +                                                        bool is_mmio)
>  {
>      MemoryRegionSection *section = atomic_read(&d->mru_section);
>      subpage_t *subpage;
> @@ -416,7 +416,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>          section = phys_page_find(d, addr);
>          update = true;
>      }
> -    if (resolve_subpage && section->mr->subpage) {
> +    if (is_mmio && section->mr->subpage) {
>          subpage = container_of(section->mr, subpage_t, iomem);
>          section = &d->map.sections[subpage->sub_section[SUBPAGE_IDX(addr)]];
>      }
> @@ -429,13 +429,13 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>  /* Called from RCU critical section */
>  static MemoryRegionSection *
>  address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *xlat,
> -                                 hwaddr *plen, bool resolve_subpage)
> +                                 hwaddr *plen, bool is_mmio)
>  {
>      MemoryRegionSection *section;
>      MemoryRegion *mr;
>      Int128 diff;
>  
> -    section = address_space_lookup_region(d, addr, resolve_subpage);
> +    section = address_space_lookup_region(d, addr, is_mmio);
>      /* Compute offset within MemoryRegionSection */
>      addr -= section->offset_within_address_space;
>  
> 

Re: [Qemu-devel] [PATCH 2/4] exec: rename resolve_subpage

[Peter Xu]

On Mon, May 15, 2017 at 11:03:49AM +0200, Paolo Bonzini wrote:
> 
> 
> On 15/05/2017 10:50, Peter Xu wrote:
> > It is not easy for people to know "what this parameter does" before
> > knowing "what is subpage". Let's use "is_mmio" to make it easier to
> > understand.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Maybe invert the direction and call it for_iotlb?

I wasn't aware that tcg is using resolve_subpage==false always. Then
at least is_mmio does not suite here. I am not sure whether for-iotlb
suites here either... So maybe I will just drop this patch as well,
just like patch 3.

Thanks for reviewing it!

-- 
Peter Xu

[Qemu-devel] [PATCH 3/4] exec: further use is_mmio

[Peter Xu]

This is MMIO-specific tunes on the size. Let's skip it for non-MMIO
translations.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 0adae94..32e5394 100644
--- a/exec.c
+++ b/exec.c
@@ -455,7 +455,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
      * everything works fine.  If the incoming length is large, however,
      * the caller really has to do the clamping through memory_access_size.
      */
-    if (memory_region_is_ram(mr)) {
+    if (is_mmio && memory_region_is_ram(mr)) {
         diff = int128_sub(section->size, int128_make64(addr));
         *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/4] exec: further use is_mmio

[Paolo Bonzini]

On 15/05/2017 10:50, Peter Xu wrote:
> This is MMIO-specific tunes on the size. Let's skip it for non-MMIO
> translations.

Can you explain this better?  This is not specific to MMIO, in fact it's
for RAM...

Paolo

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  exec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index 0adae94..32e5394 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -455,7 +455,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
>       * everything works fine.  If the incoming length is large, however,
>       * the caller really has to do the clamping through memory_access_size.
>       */
> -    if (memory_region_is_ram(mr)) {
> +    if (is_mmio && memory_region_is_ram(mr)) {
>          diff = int128_sub(section->size, int128_make64(addr));
>          *plen = int128_get64(int128_min(diff, int128_make64(*plen)));

Re: [Qemu-devel] [PATCH 3/4] exec: further use is_mmio

[Peter Xu]

On Mon, May 15, 2017 at 11:04:52AM +0200, Paolo Bonzini wrote:
> 
> 
> On 15/05/2017 10:50, Peter Xu wrote:
> > This is MMIO-specific tunes on the size. Let's skip it for non-MMIO
> > translations.
> 
> Can you explain this better?  This is not specific to MMIO, in fact it's
> for RAM...

Yes, I misread the codes and comments, sorry...

Please ignore this patch.

-- 
Peter Xu

[Qemu-devel] [PATCH 4/4] exec: abstract address_space_do_translate()

[Peter Xu]

This function is an abstraction helper for address_space_translate() and
address_space_get_iotlb_entry(). It does the lookup of address into
memory region section, then do proper IOMMU translation if necessary.
With that, refactor the two existing functions a bit to use it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 exec.c | 99 +++++++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 65 insertions(+), 34 deletions(-)

diff --git a/exec.c b/exec.c
index 32e5394..efc80a8 100644
--- a/exec.c
+++ b/exec.c
@@ -463,18 +463,20 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
 }
 
 /* Called from RCU critical section */
-IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
-                                            bool is_write)
+static MemoryRegionSection address_space_do_translate(AddressSpace *as,
+                                                      hwaddr addr,
+                                                      hwaddr *xlat,
+                                                      hwaddr *plen,
+                                                      bool is_write,
+                                                      bool is_mmio)
 {
-    IOMMUTLBEntry iotlb = {0};
+    IOMMUTLBEntry iotlb;
     MemoryRegionSection *section;
     MemoryRegion *mr;
 
     for (;;) {
         AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
-        section = address_space_lookup_region(d, addr, false);
-        addr = addr - section->offset_within_address_space
-               + section->offset_within_region;
+        section = address_space_translate_internal(d, addr, &addr, plen, is_mmio);
         mr = section->mr;
 
         if (!mr->iommu_ops) {
@@ -482,55 +484,84 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
         }
 
         iotlb = mr->iommu_ops->translate(mr, addr, is_write);
+        addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
+                | (addr & iotlb.addr_mask));
+        *plen = MIN(*plen, (addr | iotlb.addr_mask) - addr + 1);
         if (!(iotlb.perm & (1 << is_write))) {
-            iotlb.target_as = NULL;
-            break;
+            goto translate_fail;
         }
 
-        addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
-                | (addr & iotlb.addr_mask));
         as = iotlb.target_as;
     }
 
-    return iotlb;
+    *xlat = addr;
+
+    return *section;
+
+translate_fail:
+    return (MemoryRegionSection) { .mr = &io_mem_unassigned };
 }
 
 /* Called from RCU critical section */
-MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
-                                      hwaddr *xlat, hwaddr *plen,
-                                      bool is_write)
+IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
+                                            bool is_write)
 {
-    IOMMUTLBEntry iotlb;
-    MemoryRegionSection *section;
-    MemoryRegion *mr;
+    MemoryRegionSection section;
+    hwaddr xlat, plen;
 
-    for (;;) {
-        AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
-        section = address_space_translate_internal(d, addr, &addr, plen, true);
-        mr = section->mr;
+    /* Try to get maximum page mask during translation. */
+    plen = (hwaddr)-1;
 
-        if (!mr->iommu_ops) {
-            break;
-        }
+    /* This can never be MMIO. */
+    section = address_space_do_translate(as, addr, &xlat, &plen,
+                                         is_write, false);
 
-        iotlb = mr->iommu_ops->translate(mr, addr, is_write);
-        addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
-                | (addr & iotlb.addr_mask));
-        *plen = MIN(*plen, (addr | iotlb.addr_mask) - addr + 1);
-        if (!(iotlb.perm & (1 << is_write))) {
-            mr = &io_mem_unassigned;
-            break;
-        }
+    /* Illegal translation */
+    if (section.mr == &io_mem_unassigned) {
+        goto iotlb_fail;
+    }
 
-        as = iotlb.target_as;
+    if (plen == (hwaddr)-1) {
+        /*
+         * We use default page size here. Logically it only happens
+         * for identity mappings.
+         */
+        plen = TARGET_PAGE_SIZE;
     }
 
+    /* Convert to address mask */
+    plen -= 1;
+
+    return (IOMMUTLBEntry) {
+        .target_as = section.address_space,
+        .iova = addr & ~plen,
+        .translated_addr = xlat & ~plen,
+        .addr_mask = plen,
+        /* IOTLBs are for DMAs, and DMA only allows on RAMs. */
+        .perm = IOMMU_RW,
+    };
+
+iotlb_fail:
+    return (IOMMUTLBEntry) {0};
+}
+
+/* Called from RCU critical section */
+MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
+                                      hwaddr *xlat, hwaddr *plen,
+                                      bool is_write)
+{
+    MemoryRegion *mr;
+    MemoryRegionSection section;
+
+    /* This can be MMIO, so setup MMIO bit. */
+    section = address_space_do_translate(as, addr, xlat, plen, is_write, true);
+    mr = section.mr;
+
     if (xen_enabled() && memory_access_is_direct(mr, is_write)) {
         hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
         *plen = MIN(page, *plen);
     }
 
-    *xlat = addr;
     return mr;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Maxime Coquelin]

On 05/15/2017 10:50 AM, Peter Xu wrote:
> The problem is that, address_space_get_iotlb_entry() shares a lot with
> address_space_translate(). This patch tries to abstract the
> shared elements.
> 
> Originally, this work is derived from discussion from VT-d passthrough
> series discussions [1]. But for sure we can just see this series as a
> standalone cleanup. So I posted it separately here.
> 
> Smoke tests are done with general VM boots, IOs, especially with vhost
> dmar configurations.
> 
> I believe with current series I can throw away the old patch [1],
> which may be good. But before that, please kindly review. Thanks.

I faced the problem the old patch fixes when declaring and attaching an 
IOMMU device, but booting the kernel with intel_iommu=off.

I tested again with patches 1 & 4 of your series, and I confirm it fixes 
the issue:
Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks!
Maxime

> 
> References:
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg02335.html
> 
> Peter Xu (4):
>    exec: simplify phys_page_find() params
>    exec: rename resolve_subpage
>    exec: further use is_mmio
>    exec: abstract address_space_do_translate()
> 
>   exec.c | 122 ++++++++++++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 76 insertions(+), 46 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Maxime Coquelin]

Hi Peter,

On 05/16/2017 03:24 PM, Maxime Coquelin wrote:
> 
> 
> On 05/15/2017 10:50 AM, Peter Xu wrote:
>> The problem is that, address_space_get_iotlb_entry() shares a lot with
>> address_space_translate(). This patch tries to abstract the
>> shared elements.
>>
>> Originally, this work is derived from discussion from VT-d passthrough
>> series discussions [1]. But for sure we can just see this series as a
>> standalone cleanup. So I posted it separately here.
>>
>> Smoke tests are done with general VM boots, IOs, especially with vhost
>> dmar configurations.
>>
>> I believe with current series I can throw away the old patch [1],
>> which may be good. But before that, please kindly review. Thanks.
> 
> I faced the problem the old patch fixes when declaring and attaching an 
> IOMMU device, but booting the kernel with intel_iommu=off.
> 
> I tested again with patches 1 & 4 of your series, and I confirm it fixes 
> the issue:
> Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I did some more testing with my "vhost-user IOMMU" setup, and the series
actually breaks with IOMMU device attached, and intel_iommu=on.

The main difference with the previous passing test is the guest RAM
size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
hugepages in the failing one. Note that I also reproduce with 
vhost-kernel backend.

The error happens in the first vhost_device_iotlb_miss() call:
qemu-system-x86_64: Fail to lookup the translated address b5d7c000

I don't have the root cause yet, I'll keep you updated.

Maxime
> Thanks!
> Maxime
> 
>>
>> References:
>>
>> [1] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg02335.html
>>
>> Peter Xu (4):
>>    exec: simplify phys_page_find() params
>>    exec: rename resolve_subpage
>>    exec: further use is_mmio
>>    exec: abstract address_space_do_translate()
>>
>>   exec.c | 122 
>> ++++++++++++++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 76 insertions(+), 46 deletions(-)
>>
> 

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Paolo Bonzini]

On 16/05/2017 18:51, Maxime Coquelin wrote:
>>
>> I faced the problem the old patch fixes when declaring and attaching
>> an IOMMU device, but booting the kernel with intel_iommu=off.
>>
>> I tested again with patches 1 & 4 of your series, and I confirm it
>> fixes the issue:
>> Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I did some more testing with my "vhost-user IOMMU" setup, and the series
> actually breaks with IOMMU device attached, and intel_iommu=on.
> 
> The main difference with the previous passing test is the guest RAM
> size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
> hugepages in the failing one. Note that I also reproduce with
> vhost-kernel backend.
> 
> The error happens in the first vhost_device_iotlb_miss() call:
> qemu-system-x86_64: Fail to lookup the translated address b5d7c000
> 
> I don't have the root cause yet, I'll keep you updated.

I suppose it is patch 4?

Paolo

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Maxime Coquelin]

On 05/16/2017 06:54 PM, Paolo Bonzini wrote:
> 
> 
> On 16/05/2017 18:51, Maxime Coquelin wrote:
>>>
>>> I faced the problem the old patch fixes when declaring and attaching
>>> an IOMMU device, but booting the kernel with intel_iommu=off.
>>>
>>> I tested again with patches 1 & 4 of your series, and I confirm it
>>> fixes the issue:
>>> Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> I did some more testing with my "vhost-user IOMMU" setup, and the series
>> actually breaks with IOMMU device attached, and intel_iommu=on.
>>
>> The main difference with the previous passing test is the guest RAM
>> size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
>> hugepages in the failing one. Note that I also reproduce with
>> vhost-kernel backend.
>>
>> The error happens in the first vhost_device_iotlb_miss() call:
>> qemu-system-x86_64: Fail to lookup the translated address b5d7c000
>>
>> I don't have the root cause yet, I'll keep you updated.
> 
> I suppose it is patch 4?

Yes it is.

Maxime

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Peter Xu]

On Tue, May 16, 2017 at 06:51:03PM +0200, Maxime Coquelin wrote:
> Hi Peter,
> 
> On 05/16/2017 03:24 PM, Maxime Coquelin wrote:
> >
> >
> >On 05/15/2017 10:50 AM, Peter Xu wrote:
> >>The problem is that, address_space_get_iotlb_entry() shares a lot with
> >>address_space_translate(). This patch tries to abstract the
> >>shared elements.
> >>
> >>Originally, this work is derived from discussion from VT-d passthrough
> >>series discussions [1]. But for sure we can just see this series as a
> >>standalone cleanup. So I posted it separately here.
> >>
> >>Smoke tests are done with general VM boots, IOs, especially with vhost
> >>dmar configurations.
> >>
> >>I believe with current series I can throw away the old patch [1],
> >>which may be good. But before that, please kindly review. Thanks.
> >
> >I faced the problem the old patch fixes when declaring and attaching an
> >IOMMU device, but booting the kernel with intel_iommu=off.
> >
> >I tested again with patches 1 & 4 of your series, and I confirm it fixes
> >the issue:
> >Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I did some more testing with my "vhost-user IOMMU" setup, and the series
> actually breaks with IOMMU device attached, and intel_iommu=on.
> 
> The main difference with the previous passing test is the guest RAM
> size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
> hugepages in the failing one. Note that I also reproduce with vhost-kernel
> backend.
> 
> The error happens in the first vhost_device_iotlb_miss() call:
> qemu-system-x86_64: Fail to lookup the translated address b5d7c000
> 
> I don't have the root cause yet, I'll keep you updated.

Maxime,

Thanks a lot for help testing this series!

I reproduced this problem, and this is not a problem obvious enough
for me. Let me investigate as well.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Peter Xu]

On Wed, May 17, 2017 at 12:23:42PM +0800, Peter Xu wrote:
> On Tue, May 16, 2017 at 06:51:03PM +0200, Maxime Coquelin wrote:
> > Hi Peter,
> > 
> > On 05/16/2017 03:24 PM, Maxime Coquelin wrote:
> > >
> > >
> > >On 05/15/2017 10:50 AM, Peter Xu wrote:
> > >>The problem is that, address_space_get_iotlb_entry() shares a lot with
> > >>address_space_translate(). This patch tries to abstract the
> > >>shared elements.
> > >>
> > >>Originally, this work is derived from discussion from VT-d passthrough
> > >>series discussions [1]. But for sure we can just see this series as a
> > >>standalone cleanup. So I posted it separately here.
> > >>
> > >>Smoke tests are done with general VM boots, IOs, especially with vhost
> > >>dmar configurations.
> > >>
> > >>I believe with current series I can throw away the old patch [1],
> > >>which may be good. But before that, please kindly review. Thanks.
> > >
> > >I faced the problem the old patch fixes when declaring and attaching an
> > >IOMMU device, but booting the kernel with intel_iommu=off.
> > >
> > >I tested again with patches 1 & 4 of your series, and I confirm it fixes
> > >the issue:
> > >Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > 
> > I did some more testing with my "vhost-user IOMMU" setup, and the series
> > actually breaks with IOMMU device attached, and intel_iommu=on.
> > 
> > The main difference with the previous passing test is the guest RAM
> > size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
> > hugepages in the failing one. Note that I also reproduce with vhost-kernel
> > backend.
> > 
> > The error happens in the first vhost_device_iotlb_miss() call:
> > qemu-system-x86_64: Fail to lookup the translated address b5d7c000
> > 
> > I don't have the root cause yet, I'll keep you updated.
> 
> Maxime,
> 
> Thanks a lot for help testing this series!
> 
> I reproduced this problem, and this is not a problem obvious enough
> for me. Let me investigate as well.
> 
> -- 
> Peter Xu

Maxime,

Could you help try adding this change upon current to see whether
problem solved?

diff --git a/exec.c b/exec.c
index 697d902..68576a2 100644
--- a/exec.c
+++ b/exec.c
@@ -521,6 +521,10 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
         goto iotlb_fail;
     }
 
+    /* Convert memory region offset into address space offset */
+    xlat += section.offset_within_address_space -
+        section.offset_within_region;
+
     if (plen == (hwaddr)-1) {
         /*
          * We use default page size here. Logically it only happens

Thanks in advance,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Maxime Coquelin]

On 05/17/2017 07:57 AM, Peter Xu wrote:
> On Wed, May 17, 2017 at 12:23:42PM +0800, Peter Xu wrote:
>> On Tue, May 16, 2017 at 06:51:03PM +0200, Maxime Coquelin wrote:
>>> Hi Peter,
>>>
>>> On 05/16/2017 03:24 PM, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 05/15/2017 10:50 AM, Peter Xu wrote:
>>>>> The problem is that, address_space_get_iotlb_entry() shares a lot with
>>>>> address_space_translate(). This patch tries to abstract the
>>>>> shared elements.
>>>>>
>>>>> Originally, this work is derived from discussion from VT-d passthrough
>>>>> series discussions [1]. But for sure we can just see this series as a
>>>>> standalone cleanup. So I posted it separately here.
>>>>>
>>>>> Smoke tests are done with general VM boots, IOs, especially with vhost
>>>>> dmar configurations.
>>>>>
>>>>> I believe with current series I can throw away the old patch [1],
>>>>> which may be good. But before that, please kindly review. Thanks.
>>>>
>>>> I faced the problem the old patch fixes when declaring and attaching an
>>>> IOMMU device, but booting the kernel with intel_iommu=off.
>>>>
>>>> I tested again with patches 1 & 4 of your series, and I confirm it fixes
>>>> the issue:
>>>> Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> I did some more testing with my "vhost-user IOMMU" setup, and the series
>>> actually breaks with IOMMU device attached, and intel_iommu=on.
>>>
>>> The main difference with the previous passing test is the guest RAM
>>> size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
>>> hugepages in the failing one. Note that I also reproduce with vhost-kernel
>>> backend.
>>>
>>> The error happens in the first vhost_device_iotlb_miss() call:
>>> qemu-system-x86_64: Fail to lookup the translated address b5d7c000
>>>
>>> I don't have the root cause yet, I'll keep you updated.
>>
>> Maxime,
>>
>> Thanks a lot for help testing this series!
>>
>> I reproduced this problem, and this is not a problem obvious enough
>> for me. Let me investigate as well.
>>
>> -- 
>> Peter Xu
> 
> Maxime,
> 
> Could you help try adding this change upon current to see whether
> problem solved?

Hi Peter,

Yes, problem is solved with below change.

Thanks!
Maxime

> diff --git a/exec.c b/exec.c
> index 697d902..68576a2 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -521,6 +521,10 @@ IOMMUTLBEntry address_space_get_iotlb_entry(AddressSpace *as, hwaddr addr,
>           goto iotlb_fail;
>       }
>   
> +    /* Convert memory region offset into address space offset */
> +    xlat += section.offset_within_address_space -
> +        section.offset_within_region;
> +
>       if (plen == (hwaddr)-1) {
>           /*
>            * We use default page size here. Logically it only happens
> 
> Thanks in advance,
> 

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Paolo Bonzini]

On 17/05/2017 09:17, Maxime Coquelin wrote:
> 
> 
> On 05/17/2017 07:57 AM, Peter Xu wrote:
>> On Wed, May 17, 2017 at 12:23:42PM +0800, Peter Xu wrote:
>>> On Tue, May 16, 2017 at 06:51:03PM +0200, Maxime Coquelin wrote:
>>>> Hi Peter,
>>>>
>>>> On 05/16/2017 03:24 PM, Maxime Coquelin wrote:
>>>>>
>>>>>
>>>>> On 05/15/2017 10:50 AM, Peter Xu wrote:
>>>>>> The problem is that, address_space_get_iotlb_entry() shares a lot
>>>>>> with
>>>>>> address_space_translate(). This patch tries to abstract the
>>>>>> shared elements.
>>>>>>
>>>>>> Originally, this work is derived from discussion from VT-d
>>>>>> passthrough
>>>>>> series discussions [1]. But for sure we can just see this series as a
>>>>>> standalone cleanup. So I posted it separately here.
>>>>>>
>>>>>> Smoke tests are done with general VM boots, IOs, especially with
>>>>>> vhost
>>>>>> dmar configurations.
>>>>>>
>>>>>> I believe with current series I can throw away the old patch [1],
>>>>>> which may be good. But before that, please kindly review. Thanks.
>>>>>
>>>>> I faced the problem the old patch fixes when declaring and
>>>>> attaching an
>>>>> IOMMU device, but booting the kernel with intel_iommu=off.
>>>>>
>>>>> I tested again with patches 1 & 4 of your series, and I confirm it
>>>>> fixes
>>>>> the issue:
>>>>> Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>
>>>> I did some more testing with my "vhost-user IOMMU" setup, and the
>>>> series
>>>> actually breaks with IOMMU device attached, and intel_iommu=on.
>>>>
>>>> The main difference with the previous passing test is the guest RAM
>>>> size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
>>>> hugepages in the failing one. Note that I also reproduce with
>>>> vhost-kernel
>>>> backend.
>>>>
>>>> The error happens in the first vhost_device_iotlb_miss() call:
>>>> qemu-system-x86_64: Fail to lookup the translated address b5d7c000
>>>>
>>>> I don't have the root cause yet, I'll keep you updated.
>>>
>>> Maxime,
>>>
>>> Thanks a lot for help testing this series!
>>>
>>> I reproduced this problem, and this is not a problem obvious enough
>>> for me. Let me investigate as well.
>>>
>>> -- 
>>> Peter Xu
>>
>> Maxime,
>>
>> Could you help try adding this change upon current to see whether
>> problem solved?
> 
> Hi Peter,
> 
> Yes, problem is solved with below change.

Cool---Peter, please send the new patch 4 as toplevel message!

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 0/4] exec: address space translation cleanups

[Peter Xu]

On Wed, May 17, 2017 at 09:26:25AM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/05/2017 09:17, Maxime Coquelin wrote:
> > 
> > 
> > On 05/17/2017 07:57 AM, Peter Xu wrote:
> >> On Wed, May 17, 2017 at 12:23:42PM +0800, Peter Xu wrote:
> >>> On Tue, May 16, 2017 at 06:51:03PM +0200, Maxime Coquelin wrote:
> >>>> Hi Peter,
> >>>>
> >>>> On 05/16/2017 03:24 PM, Maxime Coquelin wrote:
> >>>>>
> >>>>>
> >>>>> On 05/15/2017 10:50 AM, Peter Xu wrote:
> >>>>>> The problem is that, address_space_get_iotlb_entry() shares a lot
> >>>>>> with
> >>>>>> address_space_translate(). This patch tries to abstract the
> >>>>>> shared elements.
> >>>>>>
> >>>>>> Originally, this work is derived from discussion from VT-d
> >>>>>> passthrough
> >>>>>> series discussions [1]. But for sure we can just see this series as a
> >>>>>> standalone cleanup. So I posted it separately here.
> >>>>>>
> >>>>>> Smoke tests are done with general VM boots, IOs, especially with
> >>>>>> vhost
> >>>>>> dmar configurations.
> >>>>>>
> >>>>>> I believe with current series I can throw away the old patch [1],
> >>>>>> which may be good. But before that, please kindly review. Thanks.
> >>>>>
> >>>>> I faced the problem the old patch fixes when declaring and
> >>>>> attaching an
> >>>>> IOMMU device, but booting the kernel with intel_iommu=off.
> >>>>>
> >>>>> I tested again with patches 1 & 4 of your series, and I confirm it
> >>>>> fixes
> >>>>> the issue:
> >>>>> Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>
> >>>> I did some more testing with my "vhost-user IOMMU" setup, and the
> >>>> series
> >>>> actually breaks with IOMMU device attached, and intel_iommu=on.
> >>>>
> >>>> The main difference with the previous passing test is the guest RAM
> >>>> size. In the working setup, it is 2G of 2M hugepages, vs. 4G of 2M
> >>>> hugepages in the failing one. Note that I also reproduce with
> >>>> vhost-kernel
> >>>> backend.
> >>>>
> >>>> The error happens in the first vhost_device_iotlb_miss() call:
> >>>> qemu-system-x86_64: Fail to lookup the translated address b5d7c000
> >>>>
> >>>> I don't have the root cause yet, I'll keep you updated.
> >>>
> >>> Maxime,
> >>>
> >>> Thanks a lot for help testing this series!
> >>>
> >>> I reproduced this problem, and this is not a problem obvious enough
> >>> for me. Let me investigate as well.
> >>>
> >>> -- 
> >>> Peter Xu
> >>
> >> Maxime,
> >>
> >> Could you help try adding this change upon current to see whether
> >> problem solved?
> > 
> > Hi Peter,
> > 
> > Yes, problem is solved with below change.
> 
> Cool---Peter, please send the new patch 4 as toplevel message!

Will do. Thanks!

-- 
Peter Xu