Message ID | 1449859353-1574-12-git-send-email-ehabkost@redhat.com |
---|---|
State | New |
Headers | show |
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 <ehabkost@redhat.com> > --- > 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; >
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.
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 <marcel@redhat.com> Thanks, Marcel >
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).
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;
The code can use the PCMachineClass.pci_enabled field directly. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- 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(-)