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

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

On Mon, Aug 19, 2013 at 01:06:07PM +0200, Laszlo Ersek wrote:
> On 08/13/13 00:43, Michael S. Tsirkin wrote:
> > ROM files that are put in FW CFG are copied to guest ram, by BIOS, but
> > they are not backed by RAM so they don't get migrated.
> 
> Can you please elaborate on this? Do you mean the 384 KB range between
> 640KB and 1MB that is covered by RAMBlock, but no MemoryRegion?

No. Answered below.

> > 
> > Each time we change two bytes in such a ROM this breaks cross-version
> > migration: since we can migrate after BIOS has read the first byte but
> > before it has read the second one, getting an inconsistent state.
> > 
> > Future-proof this by creating, for each such ROM,
> > an MR serving as the backing store.
> > This MR is never mapped into guest memory, but it's registered
> > as RAM so it's migrated with the guest.
> 
> savevm seems to work off RAMBlocks (ram_list.blocks), not memory regions.
> 
> ... Ah. You probably don't mean the *target* memory where the guest
> copies the fw_cfg contents from the ioport. You probably mean the
> *source* (internal representation) of the fw_cfg contents that qemu
> passes down via the ioport.
> 
> Currently those bytes appear out of thin air.
> 
> Is my understanding correct that this patch stuffs them into RAM (that
> the guest has no knowledge about), so that migration automatically
> copies over the *source* of fw_cfg contents too?

Exactly.

> fw_cfg state (current entry selector etc) is tracked according to
> "vmstate_fw_cfg". It only includes some metadata, not the actual contents.
> 
> I wonder if the "right" fix would be to save the fw_cfg files in
> "vmstate_fw_cfg" too.

This was proposed, and NACKed by Anthony.
Generally vmstate is for small bits of data,
ROM files are too large ...

You can look at this as some RAM internal to device,
we are migrating it as we would any RAM
even though guest can't access it directly.


> > 
> > Naturally, this only helps for -M 1.7 and up, older machine types
> > will still have the cross-version migration bug.
> > Luckily the race window for the problem to trigger is very small,
> > which is also likely why we didn't notice the cross-version
> > migration bug in testing yet.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/core/loader.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  hw/i386/pc_piix.c   |  2 ++
> >  hw/i386/pc_q35.c    |  2 ++
> >  include/hw/loader.h |  1 +
> >  4 files changed, 56 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 6875b7e..32d807a 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -54,6 +54,8 @@
> >  
> >  #include <zlib.h>
> >  
> > +bool rom_file_in_ram = true;
> > +
> >  static int roms_loaded;
> >  
> >  /* return the size or -1 if error */
> > @@ -576,6 +578,7 @@ struct Rom {
> >      size_t datasize;
> >  
> >      uint8_t *data;
> > +    MemoryRegion *mr;
> >      int isrom;
> >      char *fw_dir;
> >      char *fw_file;
> > @@ -605,6 +608,26 @@ static void rom_insert(Rom *rom)
> >      QTAILQ_INSERT_TAIL(&roms, rom, next);
> >  }
> >  
> > +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());
> 
> I'm not sure this is needed at all (see my comments for 1/2 -- they can
> be wrong of course). In any case, wouldn't TARGET_PAGE_ALIGN() be more
> appropriate?
> 
> > +    void *data;
> > +
> > +    rom->mr = g_malloc(sizeof(*rom->mr));
> > +    memory_region_init_ram(rom->mr, owner, name, size);
> 
> memory_region_init_ram()
>   qemu_ram_alloc()
>     qemu_ram_alloc_from_ptr()
>       TARGET_PAGE_ALIGN()
> 
> So we don't have to round up the size ourselves, and patch #1 might be
> unnecessary after all.
> 
> I also understand now that we're allocating a brand new RAMBlock, which
> we won't map into guest-phys address space: we'll call *none* of the
> memory API functions that would do that, eg:
> - memory_region_init_alias(),
> - memory_region_add_subregion[_overlap](),
> - memory_region_set_address().
> 
> > +    memory_region_set_readonly(rom->mr, true);
> > +    vmstate_register_ram_global(rom->mr);
> 
> OK, this call adds the new RAMBlock to "ram_list.blocks", via
> qemu_ram_set_idstr().
> 
> > +
> > +    data = memory_region_get_ram_ptr(rom->mr);
> > +    memcpy(data, rom->data, rom->datasize);
> > +
> > +    return data;
> > +}
> > +
> 
> So, this function allocates a new RAMBlock, doesn't map it into
> guest-phys address-space, and copies the fw_cfg contents into it.
> 
> 
> >  int rom_add_file(const char *file, const char *fw_dir,
> >                   hwaddr addr, int32_t bootindex)
> >  {
> > @@ -646,6 +669,7 @@ int rom_add_file(const char *file, const char *fw_dir,
> >      if (rom->fw_file && fw_cfg) {
> >          const char *basename;
> >          char fw_file_name[56];
> > +        void *data;
> >  
> >          basename = strrchr(rom->fw_file, '/');
> >          if (basename) {
> > @@ -655,8 +679,15 @@ int rom_add_file(const char *file, const char *fw_dir,
> >          }
> >          snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
> >                   basename);
> > -        fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
> >          snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> > +
> > +        if (rom_file_in_ram) {
> > +            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
> > +        } else {
> > +            data = rom->data;
> > +        }
> > +
> > +        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
> 
> This seems OK, but if "rom_file_in_ram" is nonzero, then we'll store the
> ROM contents in the qemu process twice -- once in "rom->data" (allocated
> just a bit higher up, not shown in context), and in the new RAMBlock.
> 
> This is no bug of course, I'm just wondering if we could drop/repoint
> "rom->data" in this case.
> 
> >      } else {
> >          snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
> >      }
> > @@ -731,7 +762,12 @@ static void rom_reset(void *unused)
> >          if (rom->data == NULL) {
> >              continue;
> >          }
> > -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> > +        if (rom->mr) {
> > +            void *host = memory_region_get_ram_ptr(rom->mr);
> > +            memcpy(host, rom->data, rom->datasize);
> > +        } else {
> > +            cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> > +        }
> 
> Hmmm. Why is this (ie. the pre-patch resetting) necessary at all?
> 
> Is this due to the writeability of fw_cfg files via the ioport
> (fw_cfg_write())? I think that modifies "rom->data" unconditionally
> (which is currently kept separate from the RAMBlock, see above).
> 
> So, regarding the patched version:
> - not sure if the RAMBlock can change at all -- it is neither mapped
> into guest-phys address space, nor does fw_cfg_write() touch it,
> - *if* the guest modifies the contents under "rom->addr", via
> fw_cfg_write(), then the hva-space memcpy() is insufficient.
> 
> 
> >          if (rom->isrom) {
> >              /* rom needs to be written only once */
> >              g_free(rom->data);
> > @@ -781,6 +817,9 @@ static Rom *find_rom(hwaddr addr)
> >          if (rom->fw_file) {
> >              continue;
> >          }
> > +        if (rom->mr) {
> > +            continue;
> > +        }
> 
> This means that (rom->fw_file || rom->mr) is sufficient not to return
> the ROM.
> 
> "rom->mr" depends on "rom->fw_file":
> 
> rom_add_file()
>   rom->fw_file = NULL // via g_malloc()
>   sets "rom->fw_file" // if (fw_dir)
>   rom_set_mr()        // if (rom->fw_file && fw_cfg && rom_file_in_ram)
>     sets "rom->mr"
> 
> So I believe this explicit check here is not necessary; it will always
> evaluate to false. (It doesn't hurt of course, for robustness).
> 
> >          if (rom->addr > addr) {
> >              continue;
> >          }
> > @@ -808,6 +847,9 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
> >          if (rom->fw_file) {
> >              continue;
> >          }
> > +        if (rom->mr) {
> > +            continue;
> > +        }
> >          if (rom->addr + rom->romsize < addr) {
> >              continue;
> >          }
> 
> Ditto.
> 
> > @@ -866,7 +908,13 @@ void do_info_roms(Monitor *mon, const QDict *qdict)
> >      Rom *rom;
> >  
> >      QTAILQ_FOREACH(rom, &roms, next) {
> > -        if (!rom->fw_file) {
> > +        if (rom->mr) {
> > +            monitor_printf(mon, "%s"
> > +                           " size=0x%06zx name=\"%s\"\n",
> > +                           rom->mr->name,
> > +                           rom->romsize,
> > +                           rom->name);
> > +        } else if (!rom->fw_file) {
> >              monitor_printf(mon, "addr=" TARGET_FMT_plx
> >                             " size=0x%06zx mem=%s name=\"%s\"\n",
> >                             rom->addr, rom->romsize,
> 
> "rom->mr" implies "rom->fw_file", which is equivalent to "!rom->fw_file"
> implying "!rom->mr".
> 
> However "!rom->mr" doesn't imply "!rom->fw_file", so the check for the
> latter is needed & correct.
> 
> Not sure what size we should print in the "rom->mr" case. The RAMBlock
> size is a multiple of the target page size. Maybe we could print both sizes.
> 
> What do you think about my comments?
> 
> Thanks,
> Laszlo
> 
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 95c45b8..2a5348e 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -25,6 +25,7 @@
> >  #include <glib.h>
> >  
> >  #include "hw/hw.h"
> > +#include "hw/loader.h"
> >  #include "hw/i386/pc.h"
> >  #include "hw/i386/apic.h"
> >  #include "hw/pci/pci.h"
> > @@ -252,6 +253,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
> >  static void pc_init_pci_1_6(QEMUMachineInitArgs *args)
> >  {
> >      has_pci_info = false;
> > +    rom_file_in_ram = false;
> >      pc_init_pci(args);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 6bfc2ca..66e7e1b 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -28,6 +28,7 @@
> >   * THE SOFTWARE.
> >   */
> >  #include "hw/hw.h"
> > +#include "hw/loader.h"
> >  #include "sysemu/arch_init.h"
> >  #include "hw/i2c/smbus.h"
> >  #include "hw/boards.h"
> > @@ -220,6 +221,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> >  static void pc_q35_init_1_6(QEMUMachineInitArgs *args)
> >  {
> >      has_pci_info = false;
> > +    rom_file_in_ram = false;
> >      pc_q35_init(args);
> >  }
> >  
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index eb9c9a3..6145736 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -36,6 +36,7 @@ void pstrcpy_targphys(const char *name,
> >                        hwaddr dest, int buf_size,
> >                        const char *source);
> >  
> > +extern bool rom_file_in_ram;
> >  
> >  int rom_add_file(const char *file, const char *fw_dir,
> >                   hwaddr addr, int32_t bootindex);
> >