Patchwork [08/40] memory: limit sections in the radix tree to the actual address space size

login
register
mail settings
Submitter Paolo Bonzini
Date May 7, 2013, 2:16 p.m.
Message ID <1367936238-12196-9-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/242346/
State New
Headers show

Comments

Paolo Bonzini - May 7, 2013, 2:16 p.m.
From: Avi Kivity <avi.kivity@gmail.com>

The radix tree is statically sized to fit TARGET_PHYS_ADDR_SPACE_BITS.
If a larger memory region is registered, it will overflow.

Fix by limiting any section in the radix tree to the supported size.

This problem was not observed earlier since artificial regions (containers
and aliases) are eliminated by the memory core, leaving only device regions
which have reasonable sizes.  An IOMMU however cannot be eliminated by the
memory core, and may have an artificial size.

Signed-off-by: Avi Kivity <avi.kivity@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
Peter Maydell - May 7, 2013, 5:13 p.m.
On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Avi Kivity <avi.kivity@gmail.com>
>
> The radix tree is statically sized to fit TARGET_PHYS_ADDR_SPACE_BITS.
> If a larger memory region is registered, it will overflow.
>
> Fix by limiting any section in the radix tree to the supported size.
>
> This problem was not observed earlier since artificial regions (containers
> and aliases) are eliminated by the memory core, leaving only device regions
> which have reasonable sizes.  An IOMMU however cannot be eliminated by the
> memory core, and may have an artificial size.

> +static MemoryRegionSection limit(MemoryRegionSection section)
> +{
> +    unsigned practical_as_bits = MIN(TARGET_PHYS_ADDR_SPACE_BITS, 62);
> +    hwaddr as_limit;
> +
> +    as_limit = (hwaddr)1 << practical_as_bits;
> +
> +    section.size = MIN(section.offset_within_address_space + section.size, as_limit)
> +                   - section.offset_within_address_space;

Isn't this going to give you a negative size for a section
which is up at the top of physical memory in a CPU with
a 63 or 64 bit physical address space? [ie one where the
section.offset_within_address_space > as_limit]

(also, overly long lines)

thanks
-- PMM
Paolo Bonzini - May 7, 2013, 5:24 p.m.
----- Messaggio originale -----
> Da: "Peter Maydell" <peter.maydell@linaro.org>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, aik@ozlabs.ru, "jan kiszka" <jan.kiszka@siemens.com>, qemulist@gmail.com, "Avi Kivity"
> <avi.kivity@gmail.com>, stefanha@redhat.com, david@gibson.dropbear.id.au
> Inviato: Martedì, 7 maggio 2013 19:13:16
> Oggetto: Re: [Qemu-devel] [PATCH 08/40] memory: limit sections in the radix tree to the actual address space size
> 
> On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > From: Avi Kivity <avi.kivity@gmail.com>
> >
> > The radix tree is statically sized to fit TARGET_PHYS_ADDR_SPACE_BITS.
> > If a larger memory region is registered, it will overflow.
> >
> > Fix by limiting any section in the radix tree to the supported size.
> >
> > This problem was not observed earlier since artificial regions (containers
> > and aliases) are eliminated by the memory core, leaving only device regions
> > which have reasonable sizes.  An IOMMU however cannot be eliminated by the
> > memory core, and may have an artificial size.
> 
> > +static MemoryRegionSection limit(MemoryRegionSection section)
> > +{
> > +    unsigned practical_as_bits = MIN(TARGET_PHYS_ADDR_SPACE_BITS, 62);
> > +    hwaddr as_limit;
> > +
> > +    as_limit = (hwaddr)1 << practical_as_bits;
> > +
> > +    section.size = MIN(section.offset_within_address_space + section.size,
> > as_limit)
> > +                   - section.offset_within_address_space;
> 
> Isn't this going to give you a negative size for a section
> which is up at the top of physical memory in a CPU with
> a 63 or 64 bit physical address space? [ie one where the
> section.offset_within_address_space > as_limit]

Yes, this assumes that TARGET_PHYS_ADDR_SPACE_BITS < 62 in practice,
and that only artificial regions are larger.  But perhaps we should
have a BUILD_BUG_ON instead of the MIN.

The only target that has a bigger _physical_ address space is s390.
Alex, is that definition correct?

Paolo
Alexander Graf - May 7, 2013, 5:37 p.m.
On 05/07/2013 07:24 PM, Paolo Bonzini wrote:
>
> ----- Messaggio originale -----
>> Da: "Peter Maydell"<peter.maydell@linaro.org>
>> A: "Paolo Bonzini"<pbonzini@redhat.com>
>> Cc: qemu-devel@nongnu.org, aik@ozlabs.ru, "jan kiszka"<jan.kiszka@siemens.com>, qemulist@gmail.com, "Avi Kivity"
>> <avi.kivity@gmail.com>, stefanha@redhat.com, david@gibson.dropbear.id.au
>> Inviato: Martedì, 7 maggio 2013 19:13:16
>> Oggetto: Re: [Qemu-devel] [PATCH 08/40] memory: limit sections in the radix tree to the actual address space size
>>
>> On 7 May 2013 15:16, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>>> From: Avi Kivity<avi.kivity@gmail.com>
>>>
>>> The radix tree is statically sized to fit TARGET_PHYS_ADDR_SPACE_BITS.
>>> If a larger memory region is registered, it will overflow.
>>>
>>> Fix by limiting any section in the radix tree to the supported size.
>>>
>>> This problem was not observed earlier since artificial regions (containers
>>> and aliases) are eliminated by the memory core, leaving only device regions
>>> which have reasonable sizes.  An IOMMU however cannot be eliminated by the
>>> memory core, and may have an artificial size.
>>> +static MemoryRegionSection limit(MemoryRegionSection section)
>>> +{
>>> +    unsigned practical_as_bits = MIN(TARGET_PHYS_ADDR_SPACE_BITS, 62);
>>> +    hwaddr as_limit;
>>> +
>>> +    as_limit = (hwaddr)1<<  practical_as_bits;
>>> +
>>> +    section.size = MIN(section.offset_within_address_space + section.size,
>>> as_limit)
>>> +                   - section.offset_within_address_space;
>> Isn't this going to give you a negative size for a section
>> which is up at the top of physical memory in a CPU with
>> a 63 or 64 bit physical address space? [ie one where the
>> section.offset_within_address_space>  as_limit]
> Yes, this assumes that TARGET_PHYS_ADDR_SPACE_BITS<  62 in practice,
> and that only artificial regions are larger.  But perhaps we should
> have a BUILD_BUG_ON instead of the MIN.
>
> The only target that has a bigger _physical_ address space is s390.
> Alex, is that definition correct?

IIRC s390x can handle up to 64 bit of physical address space from its 
page tables. But we never use that much, so it's perfectly fine for me 
if you just decrease the value.


Alex

Patch

diff --git a/exec.c b/exec.c
index 2e5b89a..fccecf6 100644
--- a/exec.c
+++ b/exec.c
@@ -777,10 +777,23 @@  static void register_multipage(AddressSpaceDispatch *d, MemoryRegionSection *sec
                   section_index);
 }
 
+static MemoryRegionSection limit(MemoryRegionSection section)
+{
+    unsigned practical_as_bits = MIN(TARGET_PHYS_ADDR_SPACE_BITS, 62);
+    hwaddr as_limit;
+
+    as_limit = (hwaddr)1 << practical_as_bits;
+
+    section.size = MIN(section.offset_within_address_space + section.size, as_limit)
+                   - section.offset_within_address_space;
+
+    return section;
+}
+
 static void mem_add(MemoryListener *listener, MemoryRegionSection *section)
 {
     AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, listener);
-    MemoryRegionSection now = *section, remain = *section;
+    MemoryRegionSection now = limit(*section), remain = limit(*section);
 
     if ((now.offset_within_address_space & ~TARGET_PAGE_MASK)
         || (now.size < TARGET_PAGE_SIZE)) {