On 12/06/16 12:49, Igor Mammedov wrote: > On Thu, 1 Dec 2016 18:06:20 +0100 > Laszlo Ersek <ler...@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" <so...@cmu.edu> >> Cc: "Michael S. Tsirkin" <m...@redhat.com> >> Cc: Gerd Hoffmann <kra...@redhat.com> >> Cc: Igor Mammedov <imamm...@redhat.com> >> Cc: Paolo Bonzini <pbonz...@redhat.com> >> Signed-off-by: Laszlo Ersek <ler...@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) >