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

[Gabriel L. Somlo]

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" <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.

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