Re: [Qemu-devel] [PATCH 1/2] i386/pc: propagate flash size from pc_system_flash_init() to pc_init1()

[Laszlo Ersek]

On 11/08/13 07:09, Jordan Justen wrote:
> On Thu, Nov 7, 2013 at 2:23 PM, Laszlo Ersek <address@hidden> 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 <address@hidden>
>> ---
>>  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
>>
>>