[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory re
From: |
Marcel Apfelbaum |
Subject: |
Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size |
Date: |
Mon, 04 Nov 2013 11:33:56 +0200 |
On Mon, 2013-11-04 at 08:18 +0200, Michael S. Tsirkin wrote:
> On Sun, Nov 03, 2013 at 09:26:06PM +0000, Peter Maydell wrote:
> > On 3 November 2013 20:48, Marcel Apfelbaum <address@hidden> wrote:
> > > The problem appears when a root memory region within an
> > > address space with size < UINT64_MAX has overlapping children
> > > with the same size. If the size of the root memory region is UINT64_MAX
> > > everyting is ok.
> > >
> > > Solved the regression by making the system-memory region
> > > of size UINT64_MAX instead of INT64_MAX.
> > >
> > > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > > ---
> > > In the mean time I am investigating why the
> > > root memory region has to be UINT64_MAX size in order
> > > to have overlapping children
> >
> > > system_memory = g_malloc(sizeof(*system_memory));
> > > - memory_region_init(system_memory, NULL, "system", INT64_MAX);
> > > + memory_region_init(system_memory, NULL, "system", UINT64_MAX);
> > > address_space_init(&address_space_memory, system_memory, "memory");
> >
> > As you say above we should investigate why this caused a
> > problem, but I was surprised the system memory space isn't
> > already maximum size. It turns out that that change was
> > introduced in commit 8417cebf in an attempt to avoid overflow
> > issues by sticking to signed 64 bit arithmetic. This approach was
> > subsequently ditched in favour of using proper 128 bit arithmetic
> > in commit 08dafab4, but we never changed the init call for
> > the system memory back to UINT64_MAX. So I think this is
> > a good change in itself.
> >
> > -- PMM
>
> I think I debugged it.
>
> So this patch seems to help simply because we only have
> sanity checking asserts in the subpage path. UINT64_MAX will make
> the region a number of full pages and avoid
> hitting the checks.
>
>
> I think I see what the issue is: exec.c
> assumes that TARGET_PHYS_ADDR_SPACE_BITS is enough
> to render any section in system memory:
> number of page table levels is calculated from that:
>
> #define P_L2_LEVELS \
> (((TARGET_PHYS_ADDR_SPACE_BITS - TARGET_PAGE_BITS - 1) / L2_BITS) + 1)
>
> any other bits are simply ignored:
>
> for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
> if (lp.ptr == PHYS_MAP_NODE_NIL) {
> return §ions[PHYS_SECTION_UNASSIGNED];
> }
> p = nodes[lp.ptr];
> lp = p[(index >> (i * L2_BITS)) & (L2_SIZE - 1)];
> }
>
> so mask by L2_SIZE - 1 means that each round looks at L2_BITS bits,
> and there are at most P_L2_LEVELS.
>
> Any other bits are simply ignored.
Michael, thanks for helping to debug this issue.
Let me see if I got it right:
If the system memory size is INT64_MAX (0x7fffffffffffffff), the address of the
last page (0x7ffffffffffff) has more bits (55) that TARGET_PHYS_ADDR_SPACE_BITS
(52)
and cannot be correctly mapped into page levels?
Thanks,
Marcel
> This is very wrong and can break in a number of other ways,
> for example I think we will also hit this assert
> if we have a non aligned 64 bit BAR of a PCI device.
>
> I think the fastest solution is to just limit
> system memory size of TARGET_PAGE_BITS.
> I sent a patch like this.
>
>
>
- [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/03
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Jordan Justen, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Laszlo Ersek, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Paolo Bonzini, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Peter Maydell, 2013/11/07