On Sun, Sep 13, 2015 at 12:51:53PM +0200, Marc Marí wrote: > On Sat, 12 Sep 2015 19:30:40 -0400 > "Gabriel L. Somlo" <so...@cmu.edu> 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 <so...@cmu.edu> > > --- > > 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.
Thanks for the feedback! It does look like FW_CFG_SIZE in fw_cfg.c appears to be mainly concerned with the width of the control register, which is a "private" property of fw_cfg.c, rather than the total size of the fw_cfg ioport region, which is a property of hw/i386/pc.c (same as a15memmap[VIRT_FW_CFG] contains the same (base,size) properties for the equivalent mmio region on arm). We could rename FW_CFG_SIZE in fw_cfg.c to FW_CFG_CTL_SIZE for increased clarity, but the fact that it's equal to FW_CFG_IO_SIZE on hw/i386/... seems to me like more of a coincidence... OTOH, i386/acpi_build.c includes both pc.h and fw_cfg.h, so if I have to, I could use FW_CFG_IO_BASE from the former and FW_CFG_SIZE from the latter. It's more of a question of aesthetics at this point, so I'm happy to do it whichever way I'm told :) Thanks, --Gabriel > > I'm not going to comment on the other patches, because I don't know > ACPI. > > Thanks > Marc
