qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v4 3/7] fw-cfg: expose "file_slots" parameter in


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH v4 3/7] fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
Date: Tue, 6 Dec 2016 19:09:56 +0100

On Tue, 6 Dec 2016 17:46:51 +0100
Laszlo Ersek <address@hidden> wrote:

> On 12/06/16 12:49, Igor Mammedov wrote:
> > On Thu,  1 Dec 2016 18:06:20 +0100
> > Laszlo Ersek <address@hidden> wrote:
> > 
> >> Accordingly, generalize the "file_slots" minimum calculation in
> >> fw_cfg_init_io_dma(), and move the constant FW_CFG_FILE_SLOTS_TRAD
> >> argument to the callers of fw_cfg_init_io_dma().
> > If I get idea correctly you're extending fw_cfg_init_io_dma() and
> > setting
> >  qdev_prop_set_uint32(dev, "file_slots", file_slots);
> > manually to keep old fw_cfg_init_io() the same without touching
> > xen/sun4u machines.
> 
> First, to my knowledge, Xen does not use fw_cfg. The following call
> chains depend on (!xen_enabled()):
maybe not, it's just grep gave me:
  xen_load_linux() -> fw_cfg_init_io()
maybe it's dead code now

> 
> pc_init1() | pc_q35_init()
>   if (!xen_enabled()):
>     pc_memory_init()
>       bochs_bios_init()
>         fw_cfg_init_io_dma()
> 
> Second, I wanted to keep the fw_cfg_init_io() call sites un-touched. In
> this patch, the fw_cfg_init_io() function passes an additional /
> internal-only parameter to fw_cfg_init_io_dma(), and the unconditional
> qdev_prop_set_uint32() call in fw_cfg_init_io_dma() is replaced with a
> conditional one (so that the property can only be lowered, not raised,
> by board code).
looking one way, it's easier to just fixup current usage
by using more duct tape, however is one considers where default
is originated from it would become 3 place to consider witch is not nice.

PS:
Keeping defaults in well known places like DEFINE_PROP_FOO and compat props
would also help in the view of introspection that Eduardo works on
trying to provide interface that shows default property values for
devices/machines.


> > That way we would have 2 different ways to set defaults
> > per machine type/version rather then the single COMPAT property way.
> > 
> > How about dropping this patch and adding
> >  SET_MACHINE_COMPAT
> > to xen/sun4u machines instead and dropping fw_cfg_init_io() in
> > favor of fw_cfg_init_io_dma() along the way.
> 
> For the conditional qdev_prop_set_uint32() call, added in this patch, I
> used commit e6915b5f3a87 ("fw_cfg: unbreak migration compatibility for
> 2.4 and earlier machines") as model. According to that model, I couldn't
> drop this patch completely, because the new feature -- i.e., DMA in that
> patch, and higher fw_cfg file slots in this patch -- depends on both
> machine versions and board opt-in.
maybe it's been written this way not to touch fw_cfg_init_io(), however
just using compat props could have worked out as well if fw_cfg_init_io()
call sites were update to use fw_cfg_init_io_dma() and board specific 'false'
defaults could be set machine specific compat props
(not really conventional way but it should work) keeping defaults as data.


> However, if we agree (according to your feedback on patch v4 2/7) that
> we will silently bump the fw_cfg file count for all new machine
> versions, without requiring (or permitting) board opt-in / opt-out, then
> I agree your above suggestion is consistent with that.
Question is if opt-in is needed?
My take on it that it's not for default value in this case as new machine
types would be able to migrate to the same or newer QEMU version.
and backward migration kept in check by machine/version specific compat
props. So it simplifies code a little and boards that don't wish new default
have a way to opt-out using compat prop mechanism.


> I think I don't have to drop fw_cfg_init_io() actually. But, with the
> board opt-in going away, I can drop both the additional "file_slots"
> parameter in fw_cfg_init_io_dma(), and the qdev_prop_set_uint32() call
> in it (even the unconditional one).
sure if it keeps patches simpler.
It's just seemed logical to cleanup useless fw_cfg_init_io() while at it,
making reader to do one less hop while reading code.

> In fact I like this simplicity more, I just wanted to be extra cautious
> in the first version of the series that turned file_slots into a property.
> 
> ... Actually, are sun4u machines versioned? Hm... "hw/sparc64/sun4u.c"
> doesn't seem to define versioned machine types. Doesn't that imply that
> migration is *completely* unsupported for sun4u? Because, if that's the
> case, then I wouldn't even have to add SET_MACHINE_COMPAT().
If sun4u/xen are not migratable/versioned we probably shouldn't even bother.

But we still can use SET_MACHINE_COMPAT() to set board specific defaults where 
necessary.

PS:
 CCing xen folks.

> 
> Thanks!
> Laszlo
> 
> > 
> >>
> >> Cc: "Gabriel L. Somlo" <address@hidden>
> >> Cc: "Michael S. Tsirkin" <address@hidden>
> >> Cc: Gerd Hoffmann <address@hidden>
> >> Cc: Igor Mammedov <address@hidden>
> >> Cc: Paolo Bonzini <address@hidden>
> >> Signed-off-by: Laszlo Ersek <address@hidden>
> >> ---
> >>  docs/specs/fw_cfg.txt     |  4 ++--
> >>  include/hw/nvram/fw_cfg.h |  2 +-
> >>  hw/i386/pc.c              |  3 ++-
> >>  hw/nvram/fw_cfg.c         | 13 ++++++-------
> >>  4 files changed, 11 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> >> index 84e2978706f5..4a6888b511f4 100644
> >> --- a/docs/specs/fw_cfg.txt
> >> +++ b/docs/specs/fw_cfg.txt
> >> @@ -153,12 +153,12 @@ Selector Reg.    Range Usage
> >>  0x4000 - 0x7fff  Generic (0x0000 - 0x3fff, RW, ignored in QEMU v2.4+)
> >>  0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, generally RO, possibly 
> >> RW
> >>                                   through the DMA interface in QEMU v2.9+)
> >>  0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
> >>  
> >> -In practice, the number of allowed firmware configuration items is given
> >> -by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
> >> +In practice, the number of allowed firmware configuration items depends 
> >> on the
> >> +machine type.
> > machine type/version
> > 
> >>  
> >>  = Guest-side DMA Interface =
> >>  
> >>  If bit 1 of the feature bitmap is set, the DMA interface is present. This 
> >> does
> >>  not replace the existing fw_cfg interface, it is an add-on. This interface
> >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> >> index b980cbaebf43..e9a6b6aa968c 100644
> >> --- a/include/hw/nvram/fw_cfg.h
> >> +++ b/include/hw/nvram/fw_cfg.h
> >> @@ -173,11 +173,11 @@ void fw_cfg_add_file_callback(FWCfgState *s, const 
> >> char *filename,
> >>   */
> >>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
> >>                           size_t len);
> >>  
> >>  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
> >> -                                AddressSpace *dma_as);
> >> +                                AddressSpace *dma_as, uint32_t 
> >> file_slots);
> >>  FWCfgState *fw_cfg_init_io(uint32_t iobase);
> >>  FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr);
> >>  FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
> >>                                   hwaddr data_addr, uint32_t data_width,
> >>                                   hwaddr dma_addr, AddressSpace *dma_as);
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index a9e64a88e5e7..5d929d8fc887 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -741,11 +741,12 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, 
> >> PCMachineState *pcms)
> >>  {
> >>      FWCfgState *fw_cfg;
> >>      uint64_t *numa_fw_cfg;
> >>      int i, j;
> >>  
> >> -    fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, as);
> >> +    fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, as,
> >> +                                FW_CFG_FILE_SLOTS_TRAD);
> >>      fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
> >>  
> >>      /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
> >>       *
> >>       * For machine types prior to 1.8, SeaBIOS needs FW_CFG_MAX_CPUS for
> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >> index 2e1441c09750..c33c76ab93b1 100644
> >> --- a/hw/nvram/fw_cfg.c
> >> +++ b/hw/nvram/fw_cfg.c
> >> @@ -926,11 +926,11 @@ static void fw_cfg_init1(DeviceState *dev)
> >>      s->machine_ready.notify = fw_cfg_machine_ready;
> >>      qemu_add_machine_init_done_notifier(&s->machine_ready);
> >>  }
> >>  
> >>  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
> >> -                                AddressSpace *dma_as)
> >> +                                AddressSpace *dma_as, uint32_t file_slots)
> >>  {
> >>      DeviceState *dev;
> >>      FWCfgState *s;
> >>      uint32_t version = FW_CFG_VERSION;
> >>      bool dma_requested = dma_iobase && dma_as;
> >> @@ -940,15 +940,14 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, 
> >> uint32_t dma_iobase,
> >>      qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
> >>      if (!dma_requested) {
> >>          qdev_prop_set_bit(dev, "dma_enabled", false);
> >>      }
> >>  
> >> -    /* Once we expose the "file_slots" property to callers of
> >> -     * fw_cfg_init_io_dma(), the following setting should become 
> >> conditional on
> >> -     * the input parameter being lower than the current value of the 
> >> property.
> >> -     */
> >> -    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
> >> +    if (file_slots < object_property_get_int(OBJECT(dev), "file_slots",
> >> +                                             &error_abort)) {
> >> +        qdev_prop_set_uint32(dev, "file_slots", file_slots);
> >> +    }
> >>  
> >>      fw_cfg_init1(dev);
> >>      s = FW_CFG(dev);
> >>  
> >>      if (s->dma_enabled) {
> >> @@ -964,11 +963,11 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, 
> >> uint32_t dma_iobase,
> >>      return s;
> >>  }
> >>  
> >>  FWCfgState *fw_cfg_init_io(uint32_t iobase)
> >>  {
> >> -    return fw_cfg_init_io_dma(iobase, 0, NULL);
> >> +    return fw_cfg_init_io_dma(iobase, 0, NULL, FW_CFG_FILE_SLOTS_TRAD);
> >>  }
> >>  
> >>  FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
> >>                                   hwaddr data_addr, uint32_t data_width,
> >>                                   hwaddr dma_addr, AddressSpace *dma_as)
> > 
> 
> 




reply via email to

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