[Qemu-devel] [PATCH v2] memory: fix limiting of translation at a page boundary

[Qemu-devel] [PATCH v2] memory: fix limiting of translation at a page boundary

[Paolo Bonzini]

Commit 360e607 (address_space_translate: do not cross page boundaries,
2014-01-30) broke MMIO accesses in cases where the section is shorter
than the full register width.  This can happen for example with the
Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
long MemoryRegion (if you write to the "second byte" of the register
your access is discarded; it doesn't write only to half of the register).

Restrict the action of commit 360e607 to direct RAM accesses.  This
is enough for Xen, since MMIO will not go through the mapcache.

Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: also revert the change in the call to
        address_space_translate_internal.  Removed Anthony's
        Tested-by to be safe.

 exec.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/exec.c b/exec.c
index 9ad0a4b..be93ebb 100644
--- a/exec.c
+++ b/exec.c
@@ -325,7 +325,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
                                  hwaddr *plen, bool resolve_subpage)
 {
     MemoryRegionSection *section;
-    Int128 diff, diff_page;
+    Int128 diff;
 
     section = address_space_lookup_region(d, addr, resolve_subpage);
     /* Compute offset within MemoryRegionSection */
@@ -334,13 +334,23 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
     /* Compute offset within MemoryRegion */
     *xlat = addr + section->offset_within_region;
 
-    diff_page = int128_make64(((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr);
     diff = int128_sub(section->mr->size, int128_make64(addr));
-    diff = int128_min(diff, diff_page);
     *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
     return section;
 }
 
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+{
+    if (memory_region_is_ram(mr)) {
+        return !(is_write && mr->readonly);
+    }
+    if (memory_region_is_romd(mr)) {
+        return !is_write;
+    }
+
+    return false;
+}
+
 MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
                                       hwaddr *xlat, hwaddr *plen,
                                       bool is_write)
@@ -351,7 +361,7 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
     hwaddr len = *plen;
 
     for (;;) {
-        section = address_space_translate_internal(as->dispatch, addr, &addr, &len, true);
+        section = address_space_translate_internal(as->dispatch, addr, &addr, plen, true);
         mr = section->mr;
 
         if (!mr->iommu_ops) {
@@ -370,6 +380,11 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
         as = iotlb.target_as;
     }
 
+    if (memory_access_is_direct(mr, is_write)) {
+        hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
+        len = MIN(page, len);
+    }
+
     *plen = len;
     *xlat = addr;
     return mr;
@@ -1925,18 +1940,6 @@ static void invalidate_and_set_dirty(hwaddr addr,
     xen_modified_memory(addr, length);
 }
 
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
-{
-    if (memory_region_is_ram(mr)) {
-        return !(is_write && mr->readonly);
-    }
-    if (memory_region_is_romd(mr)) {
-        return !is_write;
-    }
-
-    return false;
-}
-
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
     unsigned access_size_max = mr->ops->valid.max_access_size;
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2] memory: fix limiting of translation at a page boundary

[Stefano Stabellini]

On Fri, 7 Feb 2014, Paolo Bonzini wrote:
> Commit 360e607 (address_space_translate: do not cross page boundaries,
> 2014-01-30) broke MMIO accesses in cases where the section is shorter
> than the full register width.  This can happen for example with the
> Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
> long MemoryRegion (if you write to the "second byte" of the register
> your access is discarded; it doesn't write only to half of the register).
> 
> Restrict the action of commit 360e607 to direct RAM accesses.  This
> is enough for Xen, since MMIO will not go through the mapcache.
> 
> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

The patch looks OK to me.
Anthony has just left for a week of vacation and he is the only one able
to reproduce the bug.
Given that we might want to unbreak qemu ppc as soon as possible, you
can go ahead with the patch anyway. In the worst case, if it turns out
that it breaks Xen again, we can fix it with an incremental change later.


>         v1->v2: also revert the change in the call to
>         address_space_translate_internal.  Removed Anthony's
>         Tested-by to be safe.
> 
>  exec.c | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 9ad0a4b..be93ebb 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -325,7 +325,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
>                                   hwaddr *plen, bool resolve_subpage)
>  {
>      MemoryRegionSection *section;
> -    Int128 diff, diff_page;
> +    Int128 diff;
>  
>      section = address_space_lookup_region(d, addr, resolve_subpage);
>      /* Compute offset within MemoryRegionSection */
> @@ -334,13 +334,23 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
>      /* Compute offset within MemoryRegion */
>      *xlat = addr + section->offset_within_region;
>  
> -    diff_page = int128_make64(((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr);
>      diff = int128_sub(section->mr->size, int128_make64(addr));
> -    diff = int128_min(diff, diff_page);
>      *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
>      return section;
>  }
>  
> +static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
> +{
> +    if (memory_region_is_ram(mr)) {
> +        return !(is_write && mr->readonly);
> +    }
> +    if (memory_region_is_romd(mr)) {
> +        return !is_write;
> +    }
> +
> +    return false;
> +}
> +
>  MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>                                        hwaddr *xlat, hwaddr *plen,
>                                        bool is_write)
> @@ -351,7 +361,7 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>      hwaddr len = *plen;
>  
>      for (;;) {
> -        section = address_space_translate_internal(as->dispatch, addr, &addr, &len, true);
> +        section = address_space_translate_internal(as->dispatch, addr, &addr, plen, true);
>          mr = section->mr;
>  
>          if (!mr->iommu_ops) {
> @@ -370,6 +380,11 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>          as = iotlb.target_as;
>      }
>  
> +    if (memory_access_is_direct(mr, is_write)) {
> +        hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
> +        len = MIN(page, len);
> +    }
> +
>      *plen = len;
>      *xlat = addr;
>      return mr;
> @@ -1925,18 +1940,6 @@ static void invalidate_and_set_dirty(hwaddr addr,
>      xen_modified_memory(addr, length);
>  }
>  
> -static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
> -{
> -    if (memory_region_is_ram(mr)) {
> -        return !(is_write && mr->readonly);
> -    }
> -    if (memory_region_is_romd(mr)) {
> -        return !is_write;
> -    }
> -
> -    return false;
> -}
> -
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>  {
>      unsigned access_size_max = mr->ops->valid.max_access_size;
> -- 
> 1.8.5.3
> 

Re: [Qemu-devel] [PATCH v2] memory: fix limiting of translation at a page boundary

[Mark Cave-Ayland]

On 07/02/14 14:47, Paolo Bonzini wrote:

> Commit 360e607 (address_space_translate: do not cross page boundaries,
> 2014-01-30) broke MMIO accesses in cases where the section is shorter
> than the full register width.  This can happen for example with the
> Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
> long MemoryRegion (if you write to the "second byte" of the register
> your access is discarded; it doesn't write only to half of the register).
>
> Restrict the action of commit 360e607 to direct RAM accesses.  This
> is enough for Xen, since MMIO will not go through the mapcache.
>
> Reported-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Hi Paolo,

Thanks for this - I can confirm that v2 fixes the qemu-system-ppc VGA 
issue for me :)

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH v2] memory: fix limiting of translation at a page boundary

[Peter Maydell]

On 7 February 2014 14:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Commit 360e607 (address_space_translate: do not cross page boundaries,
> 2014-01-30) broke MMIO accesses in cases where the section is shorter
> than the full register width.  This can happen for example with the
> Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
> long MemoryRegion (if you write to the "second byte" of the register
> your access is discarded; it doesn't write only to half of the register).
>
> Restrict the action of commit 360e607 to direct RAM accesses.  This
> is enough for Xen, since MMIO will not go through the mapcache.
>
> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Applied to master, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v2] memory: fix limiting of translation at a page boundary

[Anthony PERARD]

On Fri, Feb 07, 2014 at 02:56:48PM +0000, Stefano Stabellini wrote:
> On Fri, 7 Feb 2014, Paolo Bonzini wrote:
> > Commit 360e607 (address_space_translate: do not cross page boundaries,
> > 2014-01-30) broke MMIO accesses in cases where the section is shorter
> > than the full register width.  This can happen for example with the
> > Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
> > long MemoryRegion (if you write to the "second byte" of the register
> > your access is discarded; it doesn't write only to half of the register).
> > 
> > Restrict the action of commit 360e607 to direct RAM accesses.  This
> > is enough for Xen, since MMIO will not go through the mapcache.
> > 
> > Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> The patch looks OK to me.
> Anthony has just left for a week of vacation and he is the only one able
> to reproduce the bug.
> Given that we might want to unbreak qemu ppc as soon as possible, you
> can go ahead with the patch anyway. In the worst case, if it turns out
> that it breaks Xen again, we can fix it with an incremental change later.
> 
> 
> >         v1->v2: also revert the change in the call to
> >         address_space_translate_internal.  Removed Anthony's
> >         Tested-by to be safe.

After a quick test, this patch works fine with Xen.

-- 
Anthony PERARD