Re: [Qemu-devel] [PATCH v4 43/47] Host page!=target page: Cleanup bitmaps

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 3 October 2014 at 18:47, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> 
> Just noticed after writing most of this email that this is an
> old patch with new review comments. I'm sending the below
> anyway in case some of it is still valid...

Still useful; I'm just looking at doing a new post soon.

> >  /*
> > + * Utility for the outgoing postcopy code.
> > + *
> > + * Discard any partially sent host-page size chunks, mark any partially
> > + * dirty host-page size chunks as all dirty.
> > + *
> > + * Returns: 0 on success
> > + */
> > +static int postcopy_chunk_hostpages(MigrationState *ms)
> > +{
> > +    struct RAMBlock *block;
> > +    unsigned int host_bits = sysconf(_SC_PAGESIZE) / TARGET_PAGE_SIZE;
> 
> I'm guessing this won't build on Win32. Can you use getpagesize() ?
> We provide a compat wrapper for that in util/ as necessary.

I'd used sysconf since the manpage of getpagesize() says 'Portable
applications should employ sysconf(_SC_PAGESIZE) instead of getpagesize()'
and I can see there are a couple of other places in qemu that use the
same sysconf; however if it's not on Win32 then yes I'm happy to change
over.

> What happens if the TARGET_PAGE_SIZE is larger than the
> host page size? (If you want MIN(host page size, TARGET_PAGE_SIZE)
> try qemu_host_page_size, see page_size_init()).

Thanks; I hadn't realised that was possible - but yes I should probably
just use qemu_host_page_size  instead of my sysconf in most places.

What happens where the target wants to map a RAMBlock with Target-page-size
alignment?

> > +    uint32_t host_mask;
> > +
> > +    /* Should be a power of 2 */
> > +    assert(host_bits && !(host_bits & (host_bits - 1)));
> 
> assert(is_power_of_2(host_bits));

Thanks; fixed.

> > +    /*
> > +     * If the host_bits isn't a division of 32 (the minimum long size)
> > +     * then the code gets a lot more complex; disallow for now
> > +     * (I'm not aware of a system where it's true anyway)
> > +     */
> > +    assert((32 % host_bits) == 0);
> > +
> > +    /* A mask, starting at bit 0, containing host_bits continuous set bits */
> > +    host_mask =  (1u << host_bits) - 1;
> 
> If the host has 64K pages and the guest TARGET_PAGE_SIZE is 1K
> (eg ARM) then this will shift off the end of your uint32_t.

Gah! That's going to make things a lot hairier; OK, that's going to take
some rework, I'll have a think how.

Note, keep an eye out for the RAM_SAVE_FLAG definitions in arch_init,
they're one-bit-per-type of message (for no good reason) and with a
TPS of 1K there are only a couple spare.

Thanks for the comments,

Dave
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK