| Message ID | 1383863039-18875-2-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 7, 2013 at 2:23 PM, Laszlo Ersek <lersek@redhat.com> wrote: > ... upwards through the following call chain: > > pc_init1() | pc_q35_init() > pc_memory_init() > pc_system_firmware_init() > pc_system_flash_init() > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > include/hw/i386/pc.h | 6 ++++-- > hw/i386/pc.c | 5 +++-- > hw/i386/pc_piix.c | 3 ++- > hw/i386/pc_q35.c | 3 ++- > hw/i386/pc_sysfw.c | 10 +++++++--- > 5 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 03cc0ba..a9b938e 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -148,7 +148,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > ram_addr_t above_4g_mem_size, > MemoryRegion *rom_memory, > MemoryRegion **ram_memory, > - PcGuestInfo *guest_info); > + PcGuestInfo *guest_info, > + int64_t **flash_size); Do you want *flash_size here, and at various other points? After changing that I could build, and it seemed to fix the symptom. Also, should we call it firmware_size? int64_t? :) -Jordan > qemu_irq *pc_allocate_cpu_irq(void); > DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus); > void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, > @@ -232,7 +233,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd) > > /* pc_sysfw.c */ > void pc_system_firmware_init(MemoryRegion *rom_memory, > - bool isapc_ram_fw); > + bool isapc_ram_fw, > + int64_t **flash_size); > > /* pvpanic.c */ > void pvpanic_init(ISABus *bus); > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 12c436e..3ec18aa 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1152,7 +1152,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > ram_addr_t above_4g_mem_size, > MemoryRegion *rom_memory, > MemoryRegion **ram_memory, > - PcGuestInfo *guest_info) > + PcGuestInfo *guest_info, > + int64_t **flash_size) > { > int linux_boot, i; > MemoryRegion *ram, *option_rom_mr; > @@ -1186,7 +1187,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > > > /* Initialize PC system firmware */ > - pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw); > + pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw, flash_size); > > option_rom_mr = g_malloc(sizeof(*option_rom_mr)); > memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE); > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 4fdb7b6..6e2c027 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -89,6 +89,7 @@ static void pc_init1(QEMUMachineInitArgs *args, > DeviceState *icc_bridge; > FWCfgState *fw_cfg = NULL; > PcGuestInfo *guest_info; > + int64_t flash_size = 0; > > if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) { > fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); > @@ -135,7 +136,7 @@ static void pc_init1(QEMUMachineInitArgs *args, > args->kernel_filename, args->kernel_cmdline, > args->initrd_filename, > below_4g_mem_size, above_4g_mem_size, > - rom_memory, &ram_memory, guest_info); > + rom_memory, &ram_memory, guest_info, &flash_size); > } > > gsi_state = g_malloc0(sizeof(*gsi_state)); > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 4c191d3..90f29e9 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -76,6 +76,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) > PCIDevice *ahci; > DeviceState *icc_bridge; > PcGuestInfo *guest_info; > + int64_t flash_size = 0; > > if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) { > fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); > @@ -120,7 +121,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) > args->kernel_filename, args->kernel_cmdline, > args->initrd_filename, > below_4g_mem_size, above_4g_mem_size, > - rom_memory, &ram_memory, guest_info); > + rom_memory, &ram_memory, guest_info, &flash_size); > } > > /* irq lines */ > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index e917c83..0d05dec 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -73,7 +73,8 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory, > } > > static void pc_system_flash_init(MemoryRegion *rom_memory, > - DriveInfo *pflash_drv) > + DriveInfo *pflash_drv, > + int64_t **flash_size) > { > BlockDriverState *bdrv; > int64_t size; > @@ -101,6 +102,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory, > flash_mem = pflash_cfi01_get_memory(system_flash); > > pc_isa_bios_init(rom_memory, flash_mem, size); > + *flash_size = size; > } > > static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) > @@ -162,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) > bios); > } > > -void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw) > +void pc_system_firmware_init(MemoryRegion *rom_memory, > + bool isapc_ram_fw, > + int64_t **flash_size) > { > DriveInfo *pflash_drv; > > @@ -181,5 +185,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw) > exit(1); > } > > - pc_system_flash_init(rom_memory, pflash_drv); > + pc_system_flash_init(rom_memory, pflash_drv, flash_size); > } > -- > 1.8.3.1 > >
On 11/08/13 07:09, Jordan Justen wrote: > On Thu, Nov 7, 2013 at 2:23 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> ... upwards through the following call chain: >> >> pc_init1() | pc_q35_init() >> pc_memory_init() >> pc_system_firmware_init() >> pc_system_flash_init() >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> include/hw/i386/pc.h | 6 ++++-- >> hw/i386/pc.c | 5 +++-- >> hw/i386/pc_piix.c | 3 ++- >> hw/i386/pc_q35.c | 3 ++- >> hw/i386/pc_sysfw.c | 10 +++++++--- >> 5 files changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 03cc0ba..a9b938e 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -148,7 +148,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, >> ram_addr_t above_4g_mem_size, >> MemoryRegion *rom_memory, >> MemoryRegion **ram_memory, >> - PcGuestInfo *guest_info); >> + PcGuestInfo *guest_info, >> + int64_t **flash_size); > > Do you want *flash_size here, and at various other points? After > changing that I could build, and it seemed to fix the symptom. Aaargh. I hacked this up at night in a rush, trying to respond with it "interactively" in the thread, and I was apparently confused by the double indirection just a bit higher in the param list (at ram_memory). What a horrible mistake. Of course you're right. > Also, should we call it firmware_size? No preference, I don't mind :) > int64_t? :) Heh, yes, I did cringe when I wrote that, but if you check the bottom-most function, where the assignment happens, pc_system_flash_init(), it declares the local "size" variable as int64_t. I've given up on arguing for sensible unsigned types so I just went with the flow. (Hm, actually, now I think the int64_t caused me so much pain and suffering that I can justifiedly blame my fsckup with the pointers on it! :)) Anyway, I think Paolo wants to revert Marcel's patch for 1.7. I'm not sure about the direction for 1.8; the pci-master-abort change has many more tentacles than I anticipated. I expect OVMF (incl. your new flash series) will run fine on v1.7.0 once it's released (but we should certainly test it soon after), so neither of my qemu patches (prio changing or resizing) should be necessary. Thanks! Laszlo > > -Jordan > >> qemu_irq *pc_allocate_cpu_irq(void); >> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus); >> void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, >> @@ -232,7 +233,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd) >> >> /* pc_sysfw.c */ >> void pc_system_firmware_init(MemoryRegion *rom_memory, >> - bool isapc_ram_fw); >> + bool isapc_ram_fw, >> + int64_t **flash_size); >> >> /* pvpanic.c */ >> void pvpanic_init(ISABus *bus); >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 12c436e..3ec18aa 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -1152,7 +1152,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, >> ram_addr_t above_4g_mem_size, >> MemoryRegion *rom_memory, >> MemoryRegion **ram_memory, >> - PcGuestInfo *guest_info) >> + PcGuestInfo *guest_info, >> + int64_t **flash_size) >> { >> int linux_boot, i; >> MemoryRegion *ram, *option_rom_mr; >> @@ -1186,7 +1187,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, >> >> >> /* Initialize PC system firmware */ >> - pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw); >> + pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw, flash_size); >> >> option_rom_mr = g_malloc(sizeof(*option_rom_mr)); >> memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE); >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 4fdb7b6..6e2c027 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -89,6 +89,7 @@ static void pc_init1(QEMUMachineInitArgs *args, >> DeviceState *icc_bridge; >> FWCfgState *fw_cfg = NULL; >> PcGuestInfo *guest_info; >> + int64_t flash_size = 0; >> >> if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) { >> fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); >> @@ -135,7 +136,7 @@ static void pc_init1(QEMUMachineInitArgs *args, >> args->kernel_filename, args->kernel_cmdline, >> args->initrd_filename, >> below_4g_mem_size, above_4g_mem_size, >> - rom_memory, &ram_memory, guest_info); >> + rom_memory, &ram_memory, guest_info, &flash_size); >> } >> >> gsi_state = g_malloc0(sizeof(*gsi_state)); >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c >> index 4c191d3..90f29e9 100644 >> --- a/hw/i386/pc_q35.c >> +++ b/hw/i386/pc_q35.c >> @@ -76,6 +76,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) >> PCIDevice *ahci; >> DeviceState *icc_bridge; >> PcGuestInfo *guest_info; >> + int64_t flash_size = 0; >> >> if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) { >> fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); >> @@ -120,7 +121,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) >> args->kernel_filename, args->kernel_cmdline, >> args->initrd_filename, >> below_4g_mem_size, above_4g_mem_size, >> - rom_memory, &ram_memory, guest_info); >> + rom_memory, &ram_memory, guest_info, &flash_size); >> } >> >> /* irq lines */ >> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c >> index e917c83..0d05dec 100644 >> --- a/hw/i386/pc_sysfw.c >> +++ b/hw/i386/pc_sysfw.c >> @@ -73,7 +73,8 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory, >> } >> >> static void pc_system_flash_init(MemoryRegion *rom_memory, >> - DriveInfo *pflash_drv) >> + DriveInfo *pflash_drv, >> + int64_t **flash_size) >> { >> BlockDriverState *bdrv; >> int64_t size; >> @@ -101,6 +102,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory, >> flash_mem = pflash_cfi01_get_memory(system_flash); >> >> pc_isa_bios_init(rom_memory, flash_mem, size); >> + *flash_size = size; >> } >> >> static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) >> @@ -162,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) >> bios); >> } >> >> -void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw) >> +void pc_system_firmware_init(MemoryRegion *rom_memory, >> + bool isapc_ram_fw, >> + int64_t **flash_size) >> { >> DriveInfo *pflash_drv; >> >> @@ -181,5 +185,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw) >> exit(1); >> } >> >> - pc_system_flash_init(rom_memory, pflash_drv); >> + pc_system_flash_init(rom_memory, pflash_drv, flash_size); >> } >> -- >> 1.8.3.1 >> >>
On 8 November 2013 15:07, Laszlo Ersek <lersek@redhat.com> wrote: > On 11/08/13 07:09, Jordan Justen wrote: >> int64_t? :) > > Heh, yes, I did cringe when I wrote that, but if you check the > bottom-most function, where the assignment happens, > pc_system_flash_init(), it declares the local "size" variable as > int64_t. I've given up on arguing for sensible unsigned types so I just > went with the flow That's a bug in that function which should be fixed. This is a memory region size and those are uint64_t. That said, having to pass the size of a sub-sub-region all the way back up the call stack is very odd and makes me wonder if it's really the right way to do it... The top level shouldn't have to care like that about details of the bottom of the callstack. thanks -- PMM
On 11/08/13 16:16, Peter Maydell wrote: > On 8 November 2013 15:07, Laszlo Ersek <lersek@redhat.com> wrote: >> On 11/08/13 07:09, Jordan Justen wrote: >>> int64_t? :) >> >> Heh, yes, I did cringe when I wrote that, but if you check the >> bottom-most function, where the assignment happens, >> pc_system_flash_init(), it declares the local "size" variable as >> int64_t. I've given up on arguing for sensible unsigned types so I just >> went with the flow > > That's a bug in that function which should be fixed. > This is a memory region size and those are uint64_t. > > That said, having to pass the size of a sub-sub-region > all the way back up the call stack is very odd and makes > me wonder if it's really the right way to do it... > The top level shouldn't have to care like that about > details of the bottom of the callstack. I agree. It's just that system.flash and pci-hole are siblings in the same container (they shouldn't overlap, *or* they should have clearly different priorities between them). We have two call chains rooted in pc_init1(), and the "ends" of those chains need to coordinate with each other (they set up the two regions, respectively, and both need the boundary between them). We could introduce a new global, but that's not exactly a step forward :) Thanks Laszlo
On 8 November 2013 15:27, Laszlo Ersek <lersek@redhat.com> wrote: > On 11/08/13 16:16, Peter Maydell wrote: >> That said, having to pass the size of a sub-sub-region >> all the way back up the call stack is very odd and makes >> me wonder if it's really the right way to do it... >> The top level shouldn't have to care like that about >> details of the bottom of the callstack. > > I agree. It's just that system.flash and pci-hole are siblings in the > same container (they shouldn't overlap, *or* they should have clearly > different priorities between them). We have two call chains rooted in > pc_init1(), and the "ends" of those chains need to coordinate with each > other (they set up the two regions, respectively, and both need the > boundary between them). We could introduce a new global, but that's not > exactly a step forward :) I suspect the correct approach for the pci-hole is to use priorities. (Also possibly to refactor the code so that the things creating memory regions directly in the top level container aren't four levels down the callstack. The natural thing would be for the callgraph to be vaguely parallel to memory region hierarchy, so all the code managing a particular container region is in the same place in the source rather than distributed.) -- PMM
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 03cc0ba..a9b938e 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -148,7 +148,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, ram_addr_t above_4g_mem_size, MemoryRegion *rom_memory, MemoryRegion **ram_memory, - PcGuestInfo *guest_info); + PcGuestInfo *guest_info, + int64_t **flash_size); qemu_irq *pc_allocate_cpu_irq(void); DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus); void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, @@ -232,7 +233,8 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd) /* pc_sysfw.c */ void pc_system_firmware_init(MemoryRegion *rom_memory, - bool isapc_ram_fw); + bool isapc_ram_fw, + int64_t **flash_size); /* pvpanic.c */ void pvpanic_init(ISABus *bus); diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 12c436e..3ec18aa 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1152,7 +1152,8 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, ram_addr_t above_4g_mem_size, MemoryRegion *rom_memory, MemoryRegion **ram_memory, - PcGuestInfo *guest_info) + PcGuestInfo *guest_info, + int64_t **flash_size) { int linux_boot, i; MemoryRegion *ram, *option_rom_mr; @@ -1186,7 +1187,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, /* Initialize PC system firmware */ - pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw); + pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw, flash_size); option_rom_mr = g_malloc(sizeof(*option_rom_mr)); memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE); diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 4fdb7b6..6e2c027 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -89,6 +89,7 @@ static void pc_init1(QEMUMachineInitArgs *args, DeviceState *icc_bridge; FWCfgState *fw_cfg = NULL; PcGuestInfo *guest_info; + int64_t flash_size = 0; if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) { fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); @@ -135,7 +136,7 @@ static void pc_init1(QEMUMachineInitArgs *args, args->kernel_filename, args->kernel_cmdline, args->initrd_filename, below_4g_mem_size, above_4g_mem_size, - rom_memory, &ram_memory, guest_info); + rom_memory, &ram_memory, guest_info, &flash_size); } gsi_state = g_malloc0(sizeof(*gsi_state)); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 4c191d3..90f29e9 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -76,6 +76,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) PCIDevice *ahci; DeviceState *icc_bridge; PcGuestInfo *guest_info; + int64_t flash_size = 0; if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) { fprintf(stderr, "xen hardware virtual machine initialisation failed\n"); @@ -120,7 +121,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) args->kernel_filename, args->kernel_cmdline, args->initrd_filename, below_4g_mem_size, above_4g_mem_size, - rom_memory, &ram_memory, guest_info); + rom_memory, &ram_memory, guest_info, &flash_size); } /* irq lines */ diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index e917c83..0d05dec 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -73,7 +73,8 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory, } static void pc_system_flash_init(MemoryRegion *rom_memory, - DriveInfo *pflash_drv) + DriveInfo *pflash_drv, + int64_t **flash_size) { BlockDriverState *bdrv; int64_t size; @@ -101,6 +102,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory, flash_mem = pflash_cfi01_get_memory(system_flash); pc_isa_bios_init(rom_memory, flash_mem, size); + *flash_size = size; } static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) @@ -162,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) bios); } -void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw) +void pc_system_firmware_init(MemoryRegion *rom_memory, + bool isapc_ram_fw, + int64_t **flash_size) { DriveInfo *pflash_drv; @@ -181,5 +185,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw) exit(1); } - pc_system_flash_init(rom_memory, pflash_drv); + pc_system_flash_init(rom_memory, pflash_drv, flash_size); }
... upwards through the following call chain: pc_init1() | pc_q35_init() pc_memory_init() pc_system_firmware_init() pc_system_flash_init() Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- include/hw/i386/pc.h | 6 ++++-- hw/i386/pc.c | 5 +++-- hw/i386/pc_piix.c | 3 ++- hw/i386/pc_q35.c | 3 ++- hw/i386/pc_sysfw.c | 10 +++++++--- 5 files changed, 18 insertions(+), 9 deletions(-)