Re: [Qemu-devel] [PATCH v2 for-1.6 v2 2/2] loader: put FW CFG ROM files into RAM

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Aug 12, 2013 at 07:37:21PM +0100, Peter Maydell wrote:
> On 12 August 2013 19:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> > +static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
> > +{
> > +    /*
> > +     * Migration code expects that all RAM blocks are full pages.
> > +     * Round MR size up to satisfy this condition.
> > +     */
> > +    unsigned size = ROUND_UP(rom->datasize, qemu_migration_page_size);
> > +    void *data = g_malloc0(size);
> > +
> > +    memcpy(data, rom->data, rom->datasize);
> > +
> > +    rom->mr = g_malloc(sizeof(*rom->mr));
> > +    memory_region_init_ram_ptr(rom->mr, owner, name, size, data);
> > +    memory_region_set_readonly(rom->mr, true);
> > +    vmstate_register_ram_global(rom->mr);
> 
> So having thought about this a little I think the right answer
> here is "don't use memory_region_init_ram_ptr()". At the moment
> in-tree we have five users of this function:
> 
> hw/display/g364fb.c
> hw/i386/kvm/pci-assign.c
> hw/misc/ivshmem.c
> hw/misc/vfio.c
> target-ppc/kvm.c
> 
> The last four of these all absolutely have to have the guest
> use a specific host pointer, typically the result of mmap()ing
> something [shared file, PCI device, KVM_ALLOCATE_RMA fd, etc].
> The first one I think should be converted to use
> memory_region_init_ram() instead, because it doesn't need
> to use a particular buffer.
> 
> Similarly, what you're trying to do here doesn't require
> that the guest sees any specific host pointer, so you should
> just use memory_region_init_ram().

I was concerned that we are wasting resources here.
In particular, huge page memory might get allocated
and there's no need for it as it's never mapped
into guest.

Still if Paolo is OK with this too, I'll switch, and do
+ memory_region_init_ram_ptr(...)
+ data = memory_region_get_ram_ptr(rom->mr);

Paolo could you please confirm?


> 
> We should add an assert to the _init_ram_ptr functions that
> checks that the size is OK, as well.

At least for pci-assign and vfio it's the wrong thing
to do - they block migration so we don't need
the ram to be a multiple of migration page size.

> I seem to recall having a conversation with Paolo along these
> lines a few months back (we fixed the exynos devices which
> were incorrectly using the _ram_ptr function); he can correct
> me if I'm off-base here.
> 
> -- PMM