| Message ID | 20161201170624.26496-4-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. 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. > > 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)
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) >
Mark, Artyom, do the sun4u machines support any kind of migration, given that they are not versioned? Alex, David, do the mac_newworld / mac_oldworld machines support migration, given that they are not versioned? Thanks Laszlo On 12/06/16 17:46, Laszlo Ersek wrote: > 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) >> >
On Tue, 6 Dec 2016 17:46:51 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > 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()): 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" <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) > > > >
On Tue, 6 Dec 2016, Igor Mammedov wrote: > > 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() We have a couple of idiosyncrasies with Xen support in QEMU; this is one of them. Xen doesn't use fw_cfg, because hvmloader [1][2] runs in guest context before SeaBios setting things up, including ACPI tables. So usually we don't need it. However direct kernel boot is done via xen_load_linux which is based on fw_cfg. So when kernel= and ramdisk= are specified in a Xen HVM config file (which is not common), they are passed to QEMU as -kernel and -initrd, then QEMU ends up using fw_cfg to load the kernel in the guest. That's the only use case we have for it at the moment. Direct kernel boot with Xen HVM guests is not tested by OSSTest (Xen Project CI loop), as a consequence it broke in the past without anybody noticing. [1]: http://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/firmware/hvmloader;h=069e29fa3820364c69c95798d56ef097148fddd2;hb=HEAD [2]: https://wiki.xenproject.org/wiki/Hvmloader
On Tue, Dec 06, 2016 at 05:52:26PM +0100, Laszlo Ersek wrote: > Mark, Artyom, do the sun4u machines support any kind of migration, given > that they are not versioned? > > Alex, David, do the mac_newworld / mac_oldworld machines support > migration, given that they are not versioned? I don't believe so. There might be some bits and pieces of migration code, but I don't think it's ever been in a state where it was really expected to work. > > Thanks > Laszlo > > On 12/06/16 17:46, Laszlo Ersek wrote: > > 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) > >> > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, Dec 01, 2016 at 06:06:20PM +0100, Laszlo Ersek 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(). > > 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> Reviewed-by: Michael S. Tsirkin <mst@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. > > = 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) > -- > 2.9.2 >
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. = 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)
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(). 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(-) -- 2.9.2