diff mbox

[memory] abort with head a8170e5

Message ID 508942F6.5050001@redhat.com
State New
Headers show

Commit Message

Avi Kivity Oct. 25, 2012, 1:47 p.m. UTC 
On 10/24/2012 04:00 PM, Aurelien Jarno wrote:
> 
> mips is also broken but by commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d:
> 
> | [    0.436000] PCI: Enabling device 0000:00:0a.1 (0000 -> 0001)
> | Segmentation fault (core dumped)
> 

How do you reproduce it?

Does this patch fix it for you?

From: Avi Kivity <avi@redhat.com>
Date: Thu, 11 Oct 2012 12:40:24 +0200
Subject: [PATCH] memory: limit sections in the radix tree to the actual
 address space size

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@redhat.com>

Comments

Aurelien Jarno Oct. 25, 2012, 2:39 p.m. UTC | #1 
On Thu, Oct 25, 2012 at 03:47:34PM +0200, Avi Kivity wrote:
> On 10/24/2012 04:00 PM, Aurelien Jarno wrote:
> > 
> > mips is also broken but by commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d:
> > 
> > | [    0.436000] PCI: Enabling device 0000:00:0a.1 (0000 -> 0001)
> > | Segmentation fault (core dumped)
> > 
> 
> How do you reproduce it?

You can use the mips kernel version 2.6.32 from:
  http://people.debian.org/~aurel32/qemu/mips/

Then just run it with the following command:
  qemu-system-mips -M malta -kernel vmlinux-2.6.32-5-4kc-malta -append "console=tty0"

(You can also get the README command line if you don't care about
downloading the disk image).

> Does this patch fix it for you?

Thanks for this patch. Unfortunately it doesn't. In the mean time, I 
have also found that it's possible to workaround the issue by using 
-vga none or -vga std (instead of the default cirrus). I don't know
if it rings a bell for you.

> From: Avi Kivity <avi@redhat.com>
> Date: Thu, 11 Oct 2012 12:40:24 +0200
> Subject: [PATCH] memory: limit sections in the radix tree to the actual
>  address space size
> 
> 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@redhat.com>
> 
> diff --git a/exec.c b/exec.c
> index b0ed593..deee8ec 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2280,10 +2280,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)) {
> 
> 
> 
> -- 
> error compiling committee.c: too many arguments to function
> 
>
Avi Kivity Oct. 25, 2012, 4:12 p.m. UTC | #2 
On 10/25/2012 04:39 PM, Aurelien Jarno wrote:
> On Thu, Oct 25, 2012 at 03:47:34PM +0200, Avi Kivity wrote:
>> On 10/24/2012 04:00 PM, Aurelien Jarno wrote:
>> > 
>> > mips is also broken but by commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d:
>> > 
>> > | [    0.436000] PCI: Enabling device 0000:00:0a.1 (0000 -> 0001)
>> > | Segmentation fault (core dumped)
>> > 
>> 
>> How do you reproduce it?
> 
> You can use the mips kernel version 2.6.32 from:
>   http://people.debian.org/~aurel32/qemu/mips/
> 
> Then just run it with the following command:
>   qemu-system-mips -M malta -kernel vmlinux-2.6.32-5-4kc-malta -append "console=tty0"
> 
> (You can also get the README command line if you don't care about
> downloading the disk image).

Doesn't reproduce here with this command line (upstream + the bridge patch).

[    0.568000] PCI: Enabling device 0000:00:12.0 (0000 -> 0002)
[    0.572000] cirrusfb 0000:00:12.0: Cirrus Logic chipset on PCI bus,
RAM (4096 kB) at 0x10000000

...

[    1.172000] PCI: Enabling device 0000:00:0a.1 (0000 -> 0001)
[    1.188000] scsi0 : ata_piix

(with console=ttyS0)

What's lp - p when the segfault occurs?  What's *index?

| #3  0x00007f4e10f3477f in phys_page_set (leaf=<optimized out>, nb=16,
index=65696, d=0x7f4e124ffb50) at /home/aurel32/qemu/exec.c:458

We're setting 16 pages around address 269090816.  Should be totally
straightforward.

If you make memory_region_transaction_begin()/_commit() no-ops, we can
get a clearer stack trace.
Aurelien Jarno Oct. 29, 2012, 7:54 a.m. UTC | #3 
On Thu, Oct 25, 2012 at 06:12:06PM +0200, Avi Kivity wrote:
> On 10/25/2012 04:39 PM, Aurelien Jarno wrote:
> > On Thu, Oct 25, 2012 at 03:47:34PM +0200, Avi Kivity wrote:
> >> On 10/24/2012 04:00 PM, Aurelien Jarno wrote:
> >> > 
> >> > mips is also broken but by commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d:
> >> > 
> >> > | [    0.436000] PCI: Enabling device 0000:00:0a.1 (0000 -> 0001)
> >> > | Segmentation fault (core dumped)
> >> > 
> >> 
> >> How do you reproduce it?
> > 
> > You can use the mips kernel version 2.6.32 from:
> >   http://people.debian.org/~aurel32/qemu/mips/
> > 
> > Then just run it with the following command:
> >   qemu-system-mips -M malta -kernel vmlinux-2.6.32-5-4kc-malta -append "console=tty0"
> > 
> > (You can also get the README command line if you don't care about
> > downloading the disk image).
> 
> Doesn't reproduce here with this command line (upstream + the bridge patch).
> 
> [    0.568000] PCI: Enabling device 0000:00:12.0 (0000 -> 0002)
> [    0.572000] cirrusfb 0000:00:12.0: Cirrus Logic chipset on PCI bus,
> RAM (4096 kB) at 0x10000000
> 
> ...
> 
> [    1.172000] PCI: Enabling device 0000:00:0a.1 (0000 -> 0001)
> [    1.188000] scsi0 : ata_piix
> 
> (with console=ttyS0)

Ok, looks like I didn't provide the right command line. I am only able
to reproduce it when using -nographic, and only with -vga cirrus (yes it
starts to be quite strange). In that case it's better to pass 
console=ttyS0, even if you can reproduce it with console=tty0.

In short it seems heavily related to the cirrus VGA card.

> What's lp - p when the segfault occurs?  What's *index?

lp - p = 0xa0
*index = 0x100a0

> | #3  0x00007f4e10f3477f in phys_page_set (leaf=<optimized out>, nb=16,
> index=65696, d=0x7f4e124ffb50) at /home/aurel32/qemu/exec.c:458
> 
> We're setting 16 pages around address 269090816.  Should be totally
> straightforward.
> 
> If you make memory_region_transaction_begin()/_commit() no-ops, we can
> get a clearer stack trace.

I'll try to get that.

Thanks,
Aurelien
diff mbox

Patch

diff --git a/exec.c b/exec.c
index b0ed593..deee8ec 100644
--- a/exec.c
+++ b/exec.c
@@ -2280,10 +2280,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)) {