Re: [Qemu-devel] i386: pc: align gpa<->hpa on 1GB boundary (v6)

[Marcelo Tosatti]

On Wed, Nov 13, 2013 at 05:58:32PM -0200, Marcelo Tosatti wrote:
> On Wed, Nov 13, 2013 at 06:13:15PM +0100, Paolo Bonzini wrote:
> > >             assert(piecetwosize <= holesize);
> > > 
> > >             piecetwosize = MIN(above_4g_mem_size, piecetwosize);
> > >             if ((above_4g_mem_size - piecetwosize) > 0) {
> > >                 memory_region_init_alias(ram_above_4g, NULL, 
> > > "ram-above-4g",
> > >                                          ram, 0x100000000ULL,
> > >                                          above_4g_mem_size - 
> > > piecetwosize);
> > >                 memory_region_add_subregion(system_memory, 0x100000000ULL,
> > >                                          ram_above_4g);
> > >             } else {
> > >                 g_free(ram_above_4g);
> > >             }
> > >             memory_region_init_alias(ram_above_4g_piecetwo, NULL,
> > >                                      "ram-above-4g-piecetwo", ram,
> > >                                      0x100000000ULL - holesize, 
> > > piecetwosize);
> > >             memory_region_add_subregion(system_memory,
> > >                                         0x100000000ULL +
> > >                                         above_4g_mem_size - piecetwosize,
> > >                                         ram_above_4g_piecetwo);
> > 
> > There is still a small problem in that the 2MB rounding must not be
> > done for old machine types.
> >
> > I did a really careful review of the code and everything else looks okay
> > to me.  However, it grew by accretion from v1 and now it took me really a
> > long time to figure it out...  I adjusted it a bit and the result seems
> > easier to understand to me.
> > 
> > Here's the hw/i386/pc.c part of the patch (the patch from v6 is unreadable):
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 12c436e..f2fd138 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1156,8 +1156,10 @@ FWCfgState *pc_memory_init(MemoryRegion 
> > *system_memory,
> >  {
> >      int linux_boot, i;
> >      MemoryRegion *ram, *option_rom_mr;
> > -    MemoryRegion *ram_below_4g, *ram_above_4g;
> > +    MemoryRegion *ram_below_4g, *ram_above_4g_pieceone, 
> > *ram_above_4g_piecetwo;
> >      FWCfgState *fw_cfg;
> > +    uint64_t holesize, pieceonesize, piecetwosize;
> > +    uint64_t memsize, align_offset;
> >  
> >      linux_boot = (kernel_filename != NULL);
> >  
> > @@ -1165,26 +1167,74 @@ FWCfgState *pc_memory_init(MemoryRegion 
> > *system_memory,
> >       * aliases to address portions of it, mostly for backwards 
> > compatibility
> >       * with older qemus that used qemu_ram_alloc().
> >       */
> > +    memsize = below_4g_mem_size + above_4g_mem_size;
> > +    holesize = 0x100000000ULL - below_4g_mem_size;
> > +
> > +    /* If 1GB hugepages are used to back guest RAM, we want the
> > +     * physical address 4GB to map to 4GB in the RAM, so that
> > +     * memory beyond 4GB is aligned on a 1GB boundary, at the
> > +     * host physical address space.  Thus, the ram block range
> > +     * [holestart, 4GB] is mapped to the last holesize bytes of RAM:
> > +     *
> > +     *                      0      h     4G     memsize-holesize
> > +     *
> > +     * contiguous-ram-block [xxxxxx][yyy][zzzzz]
> > +     *                                '-----------.
> > +     * guest-addr-space     [xxxxxx]     [zzzzz][yyy]
> > +     *
> > +     * This is only done in new-enough machine types, and of course
> > +     * it is only necessary if the [zzzzz] block exists at all.
> > +     */
> > +    if (guest_info->gb_align && above_4g_mem_size > holesize) {
> > +        /* Round the allocation up to 2 MB to use more hugepages.

To align to 2MB boundary, the number of hugepages is the same.

> > +         * Remove the slack from the [yyy] piece so that pieceonesize
> > +         * (and thus the start of piecetwo) remains aligned.
> > +         */
> > +        align_offset = ROUND_UP(memsize, 1UL << 21) - memsize;
> > +        piecetwosize = holesize - align_offset;
> > +    } else {
> > +        /* There's no "piece one", all memory above 4G starts

Piece two.

> > +         * at below_4g_mem_size in the RAM block.  Also no need
> > +         * to align anything.
> > +         */
> > +        align_offset = 0;
> > +        piecetwosize = above_4g_mem_size;
> > +    }
> > +
> >      ram = g_malloc(sizeof(*ram));
> > -    memory_region_init_ram(ram, NULL, "pc.ram",
> > -                           below_4g_mem_size + above_4g_mem_size);
> > +    memory_region_init_ram(ram, NULL, "pc.ram", memsize + align_offset);
> >      vmstate_register_ram_global(ram);
> >      *ram_memory = ram;
> > +
> >      ram_below_4g = g_malloc(sizeof(*ram_below_4g));
> >      memory_region_init_alias(ram_below_4g, NULL, "ram-below-4g", ram,
> >                               0, below_4g_mem_size);
> >      memory_region_add_subregion(system_memory, 0, ram_below_4g);
> > +
> > +    pieceonesize = above_4g_mem_size - piecetwosize;
> > +    if (pieceonesize) {
> > +        ram_above_4g_pieceone = g_malloc(sizeof(*ram_above_4g_pieceone));
> > +        memory_region_init_alias(ram_above_4g_pieceone, NULL,
> > +                                 "ram-above-4g-pieceone", ram,
> > +                                 0x100000000ULL, pieceonesize);
> > +        memory_region_add_subregion(system_memory, 0x100000000ULL,
> > +                                    ram_above_4g_pieceone);
> > +    }
> 
> Can you change the name of aliases and subregions without breaking
> migration?
> 
> Its much simpler, i'm fine with it.

Test with Q35?