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 <armbru@redhat.com>
[PMD: Extracted from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé <philmd@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;
     }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3ff127ebd0..eb7360feac 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -278,8 +278,7 @@ extern PCIDevice *piix4_dev;
 int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
 /* pc_sysfw.c */
-void pc_system_firmware_init(MemoryRegion *rom_memory,
-                             bool isapc_ram_fw);
+void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 
 /* acpi-build.c */
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-- 
2.20.1

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 <armbru@redhat.com>
> [PMD: Extracted from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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.

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.

Thanks
Laszlo

> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3ff127ebd0..eb7360feac 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -278,8 +278,7 @@ extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
>  
>  /* pc_sysfw.c */
> -void pc_system_firmware_init(MemoryRegion *rom_memory,
> -                             bool isapc_ram_fw);
> +void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  
>  /* acpi-build.c */
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> 

Laszlo Ersek <lersek@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 <armbru@redhat.com>
>> [PMD: Extracted from bigger patch]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@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!