Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

2015-12-17 Thread Eduardo Habkost 
On Thu, Dec 17, 2015 at 11:40:39AM +0200, Marcel Apfelbaum wrote:
> On 12/16/2015 09:48 PM, Eduardo Habkost wrote:
> >On Tue, Dec 15, 2015 at 04:27:51PM +0200, Marcel Apfelbaum wrote:
> >>On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> >[...]
> >>>@@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
> >>>  rom_memory = get_system_memory();
> >>>  }
> >>>
> >>>-guest_info = pc_guest_info_init(pcms);
> >>>-guest_info->isapc_ram_fw = false;
> >>
> >>This may not be an issue, I just want be sure.
> >>For Q35 isapc_ram_fw is always false, but now we are always querying
> >>!pcmc->pci_enabled.
> >>
> >>Now we have a Q35 case when !pcmc->pci_enabled *can* be true.
> >
> >Do we? pcmc->pci_enabled is always true in Q35.
> 
> OK, thanks, so all the pcmc->pci_enabled "if" clauses in
> pc_q35_init are not necessary, right?

They are not necessary, but keeping them helps us identify
duplicate code in pc_q35.c:pc_q35_init() and pc_piix.c:pc_init1()
more easily (so we can move it to common code later).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

2015-12-17 Thread Marcel Apfelbaum 

On 12/16/2015 09:48 PM, Eduardo Habkost wrote:

On Tue, Dec 15, 2015 at 04:27:51PM +0200, Marcel Apfelbaum wrote:

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:

[...]

@@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
  rom_memory = get_system_memory();
  }

-guest_info = pc_guest_info_init(pcms);
-guest_info->isapc_ram_fw = false;


This may not be an issue, I just want be sure.
For Q35 isapc_ram_fw is always false, but now we are always querying
!pcmc->pci_enabled.

Now we have a Q35 case when !pcmc->pci_enabled *can* be true.


Do we? pcmc->pci_enabled is always true in Q35.


OK, thanks, so all the pcmc->pci_enabled "if" clauses in pc_q35_init are not 
necessary, right?
Anyway, this is no connected tot his patch.


Reviewed-by: Marcel Apfelbaum 

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

2015-12-16 Thread Eduardo Habkost 
On Tue, Dec 15, 2015 at 04:27:51PM +0200, Marcel Apfelbaum wrote:
> On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
[...]
> >@@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
> >  rom_memory = get_system_memory();
> >  }
> >
> >-guest_info = pc_guest_info_init(pcms);
> >-guest_info->isapc_ram_fw = false;
> 
> This may not be an issue, I just want be sure.
> For Q35 isapc_ram_fw is always false, but now we are always querying
> !pcmc->pci_enabled.
> 
> Now we have a Q35 case when !pcmc->pci_enabled *can* be true.

Do we? pcmc->pci_enabled is always true in Q35.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

2015-12-15 Thread Marcel Apfelbaum 

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:

The code can use the PCMachineClass.pci_enabled field directly.

Signed-off-by: Eduardo Habkost 
---
  hw/i386/pc.c | 2 +-
  hw/i386/pc_piix.c| 5 +
  hw/i386/pc_q35.c | 4 +---
  include/hw/i386/pc.h | 1 -
  4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 998fe4e..02d0e19 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1365,7 +1365,7 @@ void pc_memory_init(PCMachineState *pcms,
  }

  /* Initialize PC system firmware */
-pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
+pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);

  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 fe00086..f0c2dc8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -83,7 +83,6 @@ static void pc_init1(MachineState *machine,
  MemoryRegion *ram_memory;
  MemoryRegion *pci_memory;
  MemoryRegion *rom_memory;
-PcGuestInfo *guest_info;
  ram_addr_t lowmem;

  /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
@@ -140,9 +139,7 @@ static void pc_init1(MachineState *machine,
  rom_memory = system_memory;
  }

-guest_info = pc_guest_info_init(pcms);
-
-guest_info->isapc_ram_fw = !pcmc->pci_enabled;
+pc_guest_info_init(pcms);

  if (pcmc->smbios_defaults) {
  MachineClass *mc = MACHINE_GET_CLASS(machine);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1f29943..0907746 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -70,7 +70,6 @@ static void pc_q35_init(MachineState *machine)
  int i;
  ICH9LPCState *ich9_lpc;
  PCIDevice *ahci;
-PcGuestInfo *guest_info;
  ram_addr_t lowmem;
  DriveInfo *hd[MAX_SATA_PORTS];
  MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
  rom_memory = get_system_memory();
  }

-guest_info = pc_guest_info_init(pcms);
-guest_info->isapc_ram_fw = false;


This may not be an issue, I just want be sure.
For Q35 isapc_ram_fw is always false, but now we are always querying
!pcmc->pci_enabled.

Now we have a Q35 case when !pcmc->pci_enabled *can* be true.

Do we need to care?

Thanks,
Marcel


+pc_guest_info_init(pcms);

  if (pcmc->smbios_defaults) {
  /* These values are guest ABI, do not change */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 30c7a5b..20a425c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -22,7 +22,6 @@

  /* Machine info for ACPI build: */
  struct PcGuestInfo {
-bool isapc_ram_fw;
  unsigned apic_id_limit;
  bool apic_xrupt_override;
  uint64_t numa_nodes;

[Qemu-devel] [PATCH v2 11/14] pc: Remove PcGuestInfo.isapc_ram_fw field

2015-12-11 Thread Eduardo Habkost 
The code can use the PCMachineClass.pci_enabled field directly.

Signed-off-by: Eduardo Habkost 
---
 hw/i386/pc.c | 2 +-
 hw/i386/pc_piix.c| 5 +
 hw/i386/pc_q35.c | 4 +---
 include/hw/i386/pc.h | 1 -
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 998fe4e..02d0e19 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1365,7 +1365,7 @@ void pc_memory_init(PCMachineState *pcms,
 }
 
 /* Initialize PC system firmware */
-pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
+pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
 
 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 fe00086..f0c2dc8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -83,7 +83,6 @@ static void pc_init1(MachineState *machine,
 MemoryRegion *ram_memory;
 MemoryRegion *pci_memory;
 MemoryRegion *rom_memory;
-PcGuestInfo *guest_info;
 ram_addr_t lowmem;
 
 /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
@@ -140,9 +139,7 @@ static void pc_init1(MachineState *machine,
 rom_memory = system_memory;
 }
 
-guest_info = pc_guest_info_init(pcms);
-
-guest_info->isapc_ram_fw = !pcmc->pci_enabled;
+pc_guest_info_init(pcms);
 
 if (pcmc->smbios_defaults) {
 MachineClass *mc = MACHINE_GET_CLASS(machine);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1f29943..0907746 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -70,7 +70,6 @@ static void pc_q35_init(MachineState *machine)
 int i;
 ICH9LPCState *ich9_lpc;
 PCIDevice *ahci;
-PcGuestInfo *guest_info;
 ram_addr_t lowmem;
 DriveInfo *hd[MAX_SATA_PORTS];
 MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -131,8 +130,7 @@ static void pc_q35_init(MachineState *machine)
 rom_memory = get_system_memory();
 }
 
-guest_info = pc_guest_info_init(pcms);
-guest_info->isapc_ram_fw = false;
+pc_guest_info_init(pcms);
 
 if (pcmc->smbios_defaults) {
 /* These values are guest ABI, do not change */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 30c7a5b..20a425c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -22,7 +22,6 @@
 
 /* Machine info for ACPI build: */
 struct PcGuestInfo {
-bool isapc_ram_fw;
 unsigned apic_id_limit;
 bool apic_xrupt_override;
 uint64_t numa_nodes;
-- 
2.1.0