1. Patchew
  2. QEMU
  3. View series

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

Igor Mammedov posted 1 patch 6 years, 8 months ago
Failed in applying to current master (apply log)
hw/i386/pc.c | 3 +++
1 file changed, 3 insertions(+)
[Qemu-devel] [PATCH for 2.10] pc: make 'pc.rom' readonly when machine has PCI enabled
Posted by Igor Mammedov 6 years, 8 months ago 
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.

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 <imammedo@redhat.com>
Reported-by: Peng Hao <peng.hao2@zte.com.cn>
---
 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
Re: [Qemu-devel] [PATCH for 2.10] pc: make 'pc.rom' readonly when machine has PCI enabled
Posted by Dr. David Alan Gilbert 6 years, 8 months ago 
* Igor Mammedov (imammedo@redhat.com) 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?

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 <imammedo@redhat.com>
> Reported-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  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 / dgilbert@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH for 2.10] pc: make 'pc.rom' readonly when machine has PCI enabled
Posted by Igor Mammedov 6 years, 8 months ago 
On Tue, 1 Aug 2017 13:07:57 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Igor Mammedov (imammedo@redhat.com) 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 <imammedo@redhat.com>
> > Reported-by: Peng Hao <peng.hao2@zte.com.cn>
> > ---
> >  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 / dgilbert@redhat.com / Manchester, UK

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.