diff mbox

[RFC,1/3] pc: fw_cfg: move ioport base constant to pc.h

Message ID 1442100642-7258-2-git-send-email-somlo@cmu.edu
State New
Headers show

Commit Message

Gabriel L. Somlo Sept. 12, 2015, 11:30 p.m. UTC
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 <somlo@cmu.edu>
---
 hw/i386/pc.c         | 5 ++---
 include/hw/i386/pc.h | 3 +++
 2 files changed, 5 insertions(+), 3 deletions(-)

Comments

Marc Marí Sept. 13, 2015, 10:51 a.m. UTC | #1
On Sat, 12 Sep 2015 19:30:40 -0400
"Gabriel L. Somlo" <somlo@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 <somlo@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.

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

Thanks
Marc
Gabriel L. Somlo Sept. 13, 2015, 5:28 p.m. UTC | #2
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" <somlo@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 <somlo@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
Marc Marí Sept. 13, 2015, 8:16 p.m. UTC | #3
On Sun, 13 Sep 2015 13:28:24 -0400
"Gabriel L. Somlo" <somlo@cmu.edu> 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" <somlo@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 <somlo@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...

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
diff mbox

Patch

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,