Re: [Qemu-devel] [RFC PATCH 1/3] pc: fw_cfg: move ioport base constant to pc.h

[Marc Marí]

On Sat, 12 Sep 2015 19:30:40 -0400
"Gabriel L. Somlo" <address@hidden> wrote:

> Move BIOS_CFG_IOPORT define from pc.c to pc.h, and rename
> it to FW_CFG_IO_BASE. Also, add FW_CFG_IO_SIZE define (set
> to 0x02, to cover the overlapping 16-bit control and 8-bit
> data ports).
> 
> Signed-off-by: Gabriel Somlo <address@hidden>
> ---
>  hw/i386/pc.c         | 5 ++---
>  include/hw/i386/pc.h | 3 +++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b5107f7..1a92b4f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -86,7 +86,6 @@ void pc_set_legacy_acpi_data_size(void)
>      acpi_data_size = 0x10000;
>  }
>  
> -#define BIOS_CFG_IOPORT 0x510
>  #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
>  #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
>  #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> @@ -760,7 +759,7 @@ static FWCfgState *bochs_bios_init(void)
>      int i, j;
>      unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
>  
> -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
>      /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
>       *
>       * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU
> hotplug @@ -1292,7 +1291,7 @@ FWCfgState
> *xen_load_linux(PCMachineState *pcms, 
>      assert(MACHINE(pcms)->kernel_filename != NULL);
>  
> -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
>      rom_set_fw(fw_cfg);
>  
>      load_linux(pcms, fw_cfg);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3e002c9..0cab3c5 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -206,6 +206,9 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
>  
>  void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
>  
> +#define FW_CFG_IO_BASE 0x510
> +#define FW_CFG_IO_SIZE  0x02
> +
>  /* acpi_piix.c */
>  
>  I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,

There is already a size defined in hw/nvram/fw_cfg.c (FW_CFG_SIZE). You
could move this definition to the .h and reuse it for ACPI. This way,
it is easier to modify.

Note that this value is used both for the size of the IO port and the
size of the CTL field when using memory regions. You can split it now in
your patches, or it will be split in my patches.

I'm not going to comment on the other patches, because I don't know
ACPI.

Thanks
Marc