qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marc Marí
Subject: Re: [Qemu-devel] [RFC PATCH 1/3] pc: fw_cfg: move ioport base constant to pc.h
Date: Sun, 13 Sep 2015 22:16:51 +0200

On Sun, 13 Sep 2015 13:28:24 -0400
"Gabriel L. Somlo" <address@hidden> wrote:

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

In hw/nvram/fw_cfg.c

L. 675, in fw_cfg_io_realize():
memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                      FW_CFG(s), "fwcfg", FW_CFG_SIZE); 

L. 707, in fw_cfg_mem_realize():
memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
                      FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);

The first one is the size of all the IO region, and the second one is
the size of the memory mapped CTL field.

The value for the ACPI size could be the same value used in
memory_region_init_io. But it needs to be deatached from the CTL size.
That's what I meant before. Of course, it can be the same, but it's
better to split it.

Thanks
Marc

> 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 :)
> 
> > 
> > I'm not going to comment on the other patches, because I don't know
> > ACPI.
> > 
> > Thanks
> > Marc




reply via email to

[Prev in Thread] Current Thread [Next in Thread]