Laszlo Ersek <ler...@redhat.com> writes:
> On 03/04/19 20:48, Philippe Mathieu-Daudé wrote:
>> PCMachineState will be used in the next patch.
>> Since We can access PCMachineClass from it, directly use it.
>> We can also access pcmc->pci_enabled, remove the isapc_ram_fw
>> argument.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> [PMD: Extracted from bigger patch]
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>> ---
>> hw/i386/pc.c | 2 +-
>> hw/i386/pc_sysfw.c | 8 +++++---
>> include/hw/i386/pc.h | 3 +--
>> 3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 3889eccdc3..3cd8ed1794 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1830,7 +1830,7 @@ void pc_memory_init(PCMachineState *pcms,
>> }
>>
>> /* Initialize PC system firmware */
>> - pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
>> + pc_system_firmware_init(pcms, rom_memory);
>>
>> 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_sysfw.c b/hw/i386/pc_sysfw.c
>> index 46b87afe23..55a3c563df 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -231,15 +231,17 @@ 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(PCMachineState *pcms,
>> + MemoryRegion *rom_memory)
>> {
>> + PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> DriveInfo *pflash_drv;
>>
>> pflash_drv = drive_get(IF_PFLASH, 0, 0);
>>
>> - if (isapc_ram_fw || pflash_drv == NULL) {
>> + if (!pcmc->pci_enabled || pflash_drv == NULL) {
>> /* When a pflash drive is not found, use rom-mode */
>> - old_pc_system_rom_init(rom_memory, isapc_ram_fw);
>> + old_pc_system_rom_init(rom_memory, true);
>> return;
>> }
>>
>
> This code extraction (as a pure refactoring) is incorrect.
>
> In Markus's patch at
> <https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06804.html>,
> the condition that guards the old_pc_system_rom_init() call loses one
> half of the disjunction:
>
> - if (isapc_ram_fw || pflash_drv == NULL) {
> + if (!pcmc->pci_enabled) {
>
> namely the "pflash_drv == NULL" sub-condition. Therefore the
> old_pc_system_rom_init() call can hardwire the second argument as "true":
>
> - old_pc_system_rom_init(rom_memory, isapc_ram_fw);
> + old_pc_system_rom_init(rom_memory, true);
>
> However, in this refactoring, the (pflash_drv == NULL) subcondition is
> not deleted; only the original condition is expressed differently.
> Therefore we can't replace the "isapc_ram_fw" argument of
> old_pc_system_rom_init() with plain "true", we have to use
> "!pcmc->pci_enabled" instead.
You're right
> However, if we write down "!pcmc->pci_enabled" twice for this purpose,
> then it becomes too complex to read (for me anyway). So in that case I'd
> even suggest keeping the "isapc_ram_fw" local variable, just turn it
> into a normal local variable rather than take it as a parameter.
>
> In the next patch, "isapc_ram_fw" can be killed just the same.
I'm doing that in my tree now. Thanks!
