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

[Laszlo Ersek <lersek@redhat.com>]

On 12/06/16 12:49, Igor Mammedov wrote:
> On Thu,  1 Dec 2016 18:06:20 +0100
> Laszlo Ersek <lersek@redhat.com> 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()):

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

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

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.

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

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

Thanks!
Laszlo

> 
>>
>> Cc: "Gabriel L. Somlo" <somlo@cmu.edu>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  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)
>