[Qemu-devel] [PATCH 0/2] exec: fixes for access clamping

[Qemu-devel] [PATCH 0/2] exec: fixes for access clamping

[Paolo Bonzini]

The first patch fixes the remaining problems with Peter Crosthwaite's
access clamping patch (which broke kvmvapic and hence Windows XP/2003).
The second patch actually puts the clamping to good use: by fixing
address_space_translate_internal, the MIPS rc4030 emulation does not need
anymore the address_space_rw hack.

Please test!

Paolo

Paolo Bonzini (2):
  exec: do not clamp accesses to MMIO regions
  exec: clamp accesses against the MemoryRegionSection

 exec.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] exec: do not clamp accesses to MMIO regions

[Paolo Bonzini]

It is common for MMIO registers to overlap, for example a 4 byte register
at 0xcf8 (totally random choice... :)) and a 1 byte register at 0xcf9.
If these registers are implemented via separate MemoryRegions, it is
wrong to clamp the accesses as the value written would be truncated.

Hence for these regions the effects of commit 23820db (exec: Respect
as_translate_internal length clamp, 2015-03-16, previously applied as
commit c3c1bb99) must be skipped.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 76bfc4a..d00e017 100644
--- a/exec.c
+++ b/exec.c
@@ -341,6 +341,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
                                  hwaddr *plen, bool resolve_subpage)
 {
     MemoryRegionSection *section;
+    MemoryRegion *mr;
     Int128 diff;
 
     section = address_space_lookup_region(d, addr, resolve_subpage);
@@ -350,8 +351,11 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
     /* Compute offset within MemoryRegion */
     *xlat = addr + section->offset_within_region;
 
-    diff = int128_sub(section->mr->size, int128_make64(addr));
-    *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
+    mr = section->mr;
+    if (memory_region_is_ram(mr)) {
+        diff = int128_sub(mr->size, int128_make64(addr));
+        *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
+    }
     return section;
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] exec: clamp accesses against the MemoryRegionSection

[Paolo Bonzini]

Because the clamping was done against the MemoryRegion,
address_space_rw was effectively broken if a write spanned
multiple sections that are not linear in underlying memory
(with the memory not being under an IOMMU).

This is visible with the MIPS rc4030 IOMMU, which is implemented
as a series of alias memory regions that point to the actual RAM.

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

diff --git a/exec.c b/exec.c
index d00e017..f7883d2 100644
--- a/exec.c
+++ b/exec.c
@@ -353,7 +353,7 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
 
     mr = section->mr;
     if (memory_region_is_ram(mr)) {
-        diff = int128_sub(mr->size, int128_make64(addr));
+        diff = int128_sub(section->size, int128_make64(addr));
         *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
     }
     return section;
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/2] exec: fixes for access clamping

[Hervé Poussineau]

Le 18/06/2015 17:43, Paolo Bonzini a écrit :
> The first patch fixes the remaining problems with Peter Crosthwaite's
> access clamping patch (which broke kvmvapic and hence Windows XP/2003).
> The second patch actually puts the clamping to good use: by fixing
> address_space_translate_internal, the MIPS rc4030 emulation does not need
> anymore the address_space_rw hack.
>
> Please test!

It works.
Tested-by: Hervé Poussineau <hpoussin@reactos.org>

>
> Paolo
>
> Paolo Bonzini (2):
>    exec: do not clamp accesses to MMIO regions
>    exec: clamp accesses against the MemoryRegionSection
>
>   exec.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/2] exec: fixes for access clamping

[Don Slutz]

On 06/18/15 11:43, Paolo Bonzini wrote:
> The first patch fixes the remaining problems with Peter Crosthwaite's
> access clamping patch (which broke kvmvapic and hence Windows XP/2003).
> The second patch actually puts the clamping to good use: by fixing
> address_space_translate_internal, the MIPS rc4030 emulation does not need
> anymore the address_space_rw hack.
> 
> Please test!
> 

My testing on vmport says that this change also fixed:

Subject: [BUGFIX][PATCH v7 1/9] vmport: The io memory region needs to be
at least a size of 4
Date: Fri, 12 Jun 2015 10:05:48 -0400
Message-Id: <1434117956-4929-2-git-send-email-dslutz@verizon.com>

So as long as this change goes in, the patch "vmport: The io memory
region" will be dropped.

   -Don Slutz

> Paolo
> 
> Paolo Bonzini (2):
>   exec: do not clamp accesses to MMIO regions
>   exec: clamp accesses against the MemoryRegionSection
> 
>  exec.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/2] exec: fixes for access clamping

[Mark Cave-Ayland]

On 18/06/15 16:43, Paolo Bonzini wrote:

> The first patch fixes the remaining problems with Peter Crosthwaite's
> access clamping patch (which broke kvmvapic and hence Windows XP/2003).
> The second patch actually puts the clamping to good use: by fixing
> address_space_translate_internal, the MIPS rc4030 emulation does not need
> anymore the address_space_rw hack.
> 
> Please test!
> 
> Paolo
> 
> Paolo Bonzini (2):
>   exec: do not clamp accesses to MMIO regions
>   exec: clamp accesses against the MemoryRegionSection
> 
>  exec.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Hi Paolo,

I can confirm that this patchset allows WinXP to get to the setup screen
similar to as before the access clamping patch was applied - thanks a
lot! I can give it a bit more testing on other archs over the next few
days and report back.


ATB,

Mark.