Re: [Qemu-devel] [PATCH for 2.10] pc: make 'pc.rom' readonly when machine has PCI enabled

[Igor Mammedov]

On Tue, 1 Aug 2017 13:07:57 +0100
"Dr. David Alan Gilbert" <address@hidden> wrote:

> * Igor Mammedov (address@hidden) wrote:
> > looking at bios ROM mapping in QEMU it seems that only isapc
> > (i.e. not PCI enabled machine) requires ROM being mapped as
> > RW in other cases BIOS is mapped as RO. Do the same for option
> > ROM 'pc.rom' when machine has PCI enabled.  
> 
> Does this need to be machine-type linked?
I don't think so as RO mapping doesn't change layout
but I might be wrong, looking at code only isapc maps images
RW but it doesn't have pci and that fact is used as a condition
to make ROM RO in this patch.


> Dave
> 
> > As useful side-effect pc.rom MemoryRegion stops being
> > put in vhost memory map (filtered out by vhost_section()),
> > which reduces number of entries by 1.
> > 
> > Coincidentally it fixes migration failure reported in
> > 
> > "[PATCH V2]  vhost: fix a migration failed because of vhost region merge"
> > 
> > where following destination CLI with 
> > /sys/module/vhost/parameters/max_mem_regions = 8
> > 
> > export DIMMSCOUNT=6
> > QEMU -enable-kvm \
> >      -netdev type=tap,id=guest0,vhost=on,script=no,vhostforce \
> >      -device virtio-net-pci,netdev=guest0 \
> >      -m 256,slots=256,maxmem=2G \
> >      `i=0; while [ $i -lt $DIMMSCOUNT ]; do echo \
> >          "-object memory-backend-ram,id=m$i,size=128M \
> >           -device pc-dimm,id=d$i,memdev=m$i"; i=$(($i + 1)); \
> >      done`
> > 
> > will fail to startup with error:
> > 
> >  "-device pc-dimm,id=d5,memdev=m5: a used vhost backend has no free memory 
> > slots left"
> > 
> > while it's possible to add the 6th DIMM during hotplug
> > on source.
> > 
> > Issue is caused by the fact that number of entries in vhost map
> > is bigger on 1 entry, when -device is processed, than
> > after guest boots up, and that offending entry belongs to
> > 'pc.rom', it's not like vhost intends to do IO in ROM range
> > so making it RO hides region from vhost and makes number
> > of entries in vhost memory map at -device/machine_done time
> > match number of entries after guest boots.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > Reported-by: Peng Hao <address@hidden>
> > ---
> >  hw/i386/pc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e3fcd51..de459e2 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1449,6 +1449,9 @@ void pc_memory_init(PCMachineState *pcms,
> >      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> >      memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> >                             &error_fatal);
> > +    if (pcmc->pci_enabled) {
> > +        memory_region_set_readonly(option_rom_mr, true);
> > +    }
> >      vmstate_register_ram_global(option_rom_mr);
> >      memory_region_add_subregion_overlap(rom_memory,
> >                                          PC_ROM_MIN_VGA,
> > -- 
> > 2.7.4
> >   
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK