[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a de
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property |
Date: |
Tue, 6 Dec 2016 17:22:48 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
On 12/06/16 13:02, Igor Mammedov wrote:
> On Tue, 6 Dec 2016 12:43:06 +0100
> Laszlo Ersek <address@hidden> wrote:
>
>> On 12/06/16 11:50, Igor Mammedov wrote:
>>> On Thu, 1 Dec 2016 18:06:19 +0100
>>> Laszlo Ersek <address@hidden> wrote:
>>>
>>>> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
>>>> lead to problems with backward migration: a more recent QEMU (running an
>>>> older machine type) would allow the guest, in fw_cfg_select(), to select a
>>>> high key value that is unavailable in the same machine type implemented by
>>>> the older (target) QEMU. On the target host, fw_cfg_data_read() for
>>>> example could dereference nonexistent entries.
>>>>
>>>> As first step, size the FWCfgState.entries[*] and FWCfgState.entry_order
>>>> arrays dynamically. All three array sizes will be influenced by the new
>>>> field (and device property) FWCfgState.file_slots.
>>>>
>>>> Make the following changes:
>>>>
>>>> - Replace the FW_CFG_FILE_SLOTS macro with FW_CFG_FILE_SLOTS_TRAD
>>>> (traditional count of fw_cfg file slots) in the header file. The value
>>>> remains 0x10.
>>>>
>>>> - Replace all uses of FW_CFG_FILE_SLOTS with a helper function called
>>>> fw_cfg_file_slots(), returning the new property.
>>>>
>>>> - Eliminate the macro FW_CFG_MAX_ENTRY, and replace all its uses with a
>>>> helper function called fw_cfg_max_entry().
>>>>
>>>> - In the MMIO- and IO-mapped realize functions both, allocate all three
>>>> arrays dynamically, based on the new property.
>>>>
>>>> - The new property defaults to 0x20; however at the moment we forcibly set
>>>> it to FW_CFG_FILE_SLOTS_TRAD on all code paths available to board code
>>>> (namely in the fw_cfg_init_io_dma() and fw_cfg_init_mem_wide() helper
>>>> functions). This is going to be customized in the following patches.
>>>>
>>>> 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>
>>>> ---
>>>>
>>>> Notes:
>>>> I know that upstream doesn't care about backward migration, but some
>>>> downstreams might.
>>>>
>>>> docs/specs/fw_cfg.txt | 2 +-
>>>> include/hw/nvram/fw_cfg_keys.h | 3 +-
>>>> hw/nvram/fw_cfg.c | 85
>>>> ++++++++++++++++++++++++++++++++++++++----
>>>> 3 files changed, 79 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>>>> index a19e2adbe1c6..84e2978706f5 100644
>>>> --- a/docs/specs/fw_cfg.txt
>>>> +++ b/docs/specs/fw_cfg.txt
>>>> @@ -154,11 +154,11 @@ Selector Reg. Range Usage
>>>> 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 of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>>>> +by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
>>>>
>>>> = 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_keys.h
>>>> b/include/hw/nvram/fw_cfg_keys.h
>>>> index 0f3e871884c0..627589793671 100644
>>>> --- a/include/hw/nvram/fw_cfg_keys.h
>>>> +++ b/include/hw/nvram/fw_cfg_keys.h
>>>> @@ -27,12 +27,11 @@
>>>> #define FW_CFG_SETUP_SIZE 0x17
>>>> #define FW_CFG_SETUP_DATA 0x18
>>>> #define FW_CFG_FILE_DIR 0x19
>>>>
>>>> #define FW_CFG_FILE_FIRST 0x20
>>>> -#define FW_CFG_FILE_SLOTS 0x10
>>>> -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS)
>>>> +#define FW_CFG_FILE_SLOTS_TRAD 0x10
>>>>
>>>> #define FW_CFG_WRITE_CHANNEL 0x4000
>>>> #define FW_CFG_ARCH_LOCAL 0x8000
>>>> #define FW_CFG_ENTRY_MASK (~(FW_CFG_WRITE_CHANNEL |
>>>> FW_CFG_ARCH_LOCAL))
>>>>
>>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>>> index e0145c11a19b..2e1441c09750 100644
>>>> --- a/hw/nvram/fw_cfg.c
>>>> +++ b/hw/nvram/fw_cfg.c
>>>> @@ -31,10 +31,13 @@
>>>> #include "hw/sysbus.h"
>>>> #include "trace.h"
>>>> #include "qemu/error-report.h"
>>>> #include "qemu/config-file.h"
>>>> #include "qemu/cutils.h"
>>>> +#include "qapi/error.h"
>>>> +
>>>> +#define FW_CFG_FILE_SLOTS_DFLT 0x20
>>>>
>>>> #define FW_CFG_NAME "fw_cfg"
>>>> #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>>>>
>>>> #define TYPE_FW_CFG "fw_cfg"
>>>> @@ -69,12 +72,13 @@ typedef struct FWCfgEntry {
>>>> struct FWCfgState {
>>>> /*< private >*/
>>>> SysBusDevice parent_obj;
>>>> /*< public >*/
>>>>
>>>> - FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>>>> - int entry_order[FW_CFG_MAX_ENTRY];
>>>> + uint32_t file_slots;
>>> should it be uint16_t?
>>> As below you use "uint16_t file_slots_max;" and do some UINT16
>>> to calculate max limit.
>>
>> I think I had a reason for making this uint32_t. I think the argument
>> was that fw_cfg_max_entry() could theoretically return a value that
>> doesn't fit in a uint16_t. Looking again at the patch however, I think I
>> can try to make this a uint16_t for the next version.
>>
>>>
>>>> + FWCfgEntry *entries[2];
>>>> + int *entry_order;
>>>> FWCfgFiles *files;
>>>> uint16_t cur_entry;
>>>> uint32_t cur_offset;
>>>> Notifier machine_ready;
>>>>
>>>> @@ -255,17 +259,27 @@ static void fw_cfg_reboot(FWCfgState *s)
>>>> static void fw_cfg_write(FWCfgState *s, uint8_t value)
>>>> {
>>>> /* nothing, write support removed in QEMU v2.4+ */
>>>> }
>>>>
>>>> +static inline uint32_t fw_cfg_file_slots(const FWCfgState *s)
>>>> +{
>>>> + return s->file_slots;
>>>> +}
>>> so far it doesn't look like this wrapper function is necessary,
>>> I'd prefer accessing field directly as it's used only internally.
>>> Or do you have plans to extend wrapper later?
>>> (then it would be better introduce wrapper at that time)
>>
>> Originally I used "s->file_slots" in this patch, like you say, without
>> this small wrapper function,, but the resultant patch was harder to
>> review. With this wrapper, you have two kinds of changes in the patch:
>>
>> * FW_CFG_MAX_ENTRY
>> --> fw_cfg_max_entry(s)
>>
>> * FW_CFG_FILE_SLOTS
>> --> fw_cfg_file_slots(s)
>>
>> Without the wrapper, the second bullet looks
>>
>> * FW_CFG_FILE_SLOTS
>> --> s->file_slots
>>
>> and to me that made the patch harder to verify.
> to me this variant look clearer as I don't have to recall that
> fw_cfg_file_slots(s) is s->file_slots and does nothing more.
> But if you prefer wrapper I'm also fine with it.
>
>>
>>>
>>>> +
>>>> +static inline uint32_t fw_cfg_max_entry(const FWCfgState *s)
>>>> +{
>>>> + return FW_CFG_FILE_FIRST + fw_cfg_file_slots(s);
>>>> +}
>>>> +
>>>> static int fw_cfg_select(FWCfgState *s, uint16_t key)
>>>> {
>>>> int arch, ret;
>>>> FWCfgEntry *e;
>>>>
>>>> s->cur_offset = 0;
>>>> - if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
>>>> + if ((key & FW_CFG_ENTRY_MASK) >= fw_cfg_max_entry(s)) {
>>>> s->cur_entry = FW_CFG_INVALID;
>>>> ret = 0;
>>>> } else {
>>>> s->cur_entry = key;
>>>> ret = 1;
>>>> @@ -608,11 +622,11 @@ static void
>>>> fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>>>> {
>>>> int arch = !!(key & FW_CFG_ARCH_LOCAL);
>>>>
>>>> key &= FW_CFG_ENTRY_MASK;
>>>>
>>>> - assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
>>>> + assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>>>> assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
>>>>
>>>> s->entries[arch][key].data = data;
>>>> s->entries[arch][key].len = (uint32_t)len;
>>>> s->entries[arch][key].read_callback = callback;
>>>> @@ -626,11 +640,11 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s,
>>>> uint16_t key,
>>>> void *ptr;
>>>> int arch = !!(key & FW_CFG_ARCH_LOCAL);
>>>>
>>>> key &= FW_CFG_ENTRY_MASK;
>>>>
>>>> - assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
>>>> + assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>>>>
>>>> /* return the old data to the function caller, avoid memory leak */
>>>> ptr = s->entries[arch][key].data;
>>>> s->entries[arch][key].data = data;
>>>> s->entries[arch][key].len = len;
>>>> @@ -775,17 +789,17 @@ void fw_cfg_add_file_callback(FWCfgState *s, const
>>>> char *filename,
>>>> size_t dsize;
>>>> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>>>> int order = 0;
>>>>
>>>> if (!s->files) {
>>>> - dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * FW_CFG_FILE_SLOTS;
>>>> + dsize = sizeof(uint32_t) + sizeof(FWCfgFile) *
>>>> fw_cfg_file_slots(s);
>>>> s->files = g_malloc0(dsize);
>>>> fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
>>>> }
>>>>
>>>> count = be32_to_cpu(s->files->count);
>>>> - assert(count < FW_CFG_FILE_SLOTS);
>>>> + assert(count < fw_cfg_file_slots(s));
>>>>
>>>> /* Find the insertion point. */
>>>> if (mc->legacy_fw_cfg_order) {
>>>> /*
>>>> * Sort by order. For files with the same order, we keep them
>>>> @@ -855,11 +869,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char
>>>> *filename,
>>>> void *ptr = NULL;
>>>>
>>>> assert(s->files);
>>>>
>>>> index = be32_to_cpu(s->files->count);
>>>> - assert(index < FW_CFG_FILE_SLOTS);
>>>> + assert(index < fw_cfg_file_slots(s));
>>>>
>>>> for (i = 0; i < index; i++) {
>>>> if (strcmp(filename, s->files->f[i].name) == 0) {
>>>> ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
>>>> data, len);
>>>> @@ -926,10 +940,16 @@ 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);
>>>> +
>>>> fw_cfg_init1(dev);
>>>> s = FW_CFG(dev);
>>>>
>>>> if (s->dma_enabled) {
>>>> /* 64 bits for the address field */
>>>> @@ -963,10 +983,17 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>>>> qdev_prop_set_uint32(dev, "data_width", data_width);
>>>> if (!dma_requested) {
>>>> qdev_prop_set_bit(dev, "dma_enabled", false);
>>>> }
>>>>
>>>> + /* Once we expose the "file_slots" property to callers of
>>>> + * fw_cfg_init_mem_wide(), the following setting should become
>>>> conditional
>>>> + * on the input parameter being lower than the current value of the
>>>> + * property. Refer to fw_cfg_init_io_dma().
>>>> + */
>>>> + qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
>>> both above cases of qdev_prop_set_uint32() could be replaced by
>>> compat property for fwcfg device which would clamp "file_slots"
>>> to old value for old machine types in a cleaner way.
>>
>> The unconditional qdev_prop_set_uint32() call is already replaced, in
>> fw_cfg_init_io_dma(), in the next patch, and the compat properties are
>> added in the patch after it.
>>
>> The current approach is:
>> - patch #2 adds the property with the raised default, but clamps it
>> down in code that calls fw_cfg_init1() directly
>> - patch #3 propagates the clamping-down a little bit outwards, towards
>> board code, but only in the IO-port mapped case,
>> - patch #4 adds the 2.8 compat props and raises the limit in 2.9 pc
>> board code (i.e., 2.9 pc opts in)
>>
>> The point is that
>> (a) no old machine type should see any change
>> (b) even for new machine types, the higher file slot count is opt-in for
>> board code (i.e., it shouldn't affect callers of
>> fw_cfg_init_mem_wide() and fw_cfg_init_io())
> I'm not sure that we need (b) (i.e. optin for new machine versions)
> Maybe all new machine types should use new default and we should
> clamp it down for all old machine types the same way (compat prop).
> It would be uniform and less confusing possibly making series simpler.
Works for me if (b) is a non-goal. I'll update the patches.
Thanks!
Laszlo
>
>>
>> The qdev_prop_set_uint32() call that unconditionally remains in
>> fw_cfg_init_mem_wide() at the end of this series is for ensuring (b).
>>
>> The "hw/arm/virt.c" board calls fw_cfg_init_mem_wide(), and that board
>> should not receive the increased fw_cfg file count even in 2.9+ (to
>> which the compat props would not apply).
>>
>> In order to remove the unconditional property setting from
>> fw_cfg_init_mem_wide() too, I'd either have to modify the
>> fw_cfg_init_mem_wide() prototype and also the call site in
>> "hw/arm/virt.c", or we'd have to carry forward the compat property to
>> 2.9 and later, indefinitely.
>>
>> I'm open to reworking this code, but both goals (a) and (b) should be
>> considered in any alternative implementation.
>>
>> Thanks!
>> Laszlo
>>
>>>
>>>> +
>>>> fw_cfg_init1(dev);
>>>>
>>>> sbd = SYS_BUS_DEVICE(dev);
>>>> sysbus_mmio_map(sbd, 0, ctl_addr);
>>>> sysbus_mmio_map(sbd, 1, data_addr);
>>>> @@ -1012,23 +1039,56 @@ static const TypeInfo fw_cfg_info = {
>>>> .abstract = true,
>>>> .instance_size = sizeof(FWCfgState),
>>>> .class_init = fw_cfg_class_init,
>>>> };
>>>>
>>>> +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>>>> +{
>>>> + uint16_t file_slots_max;
>>>> +
>>>> + if (fw_cfg_file_slots(s) < FW_CFG_FILE_SLOTS_TRAD) {
>>>> + error_setg(errp, "\"file_slots\" must be at least 0x%x",
>>>> + FW_CFG_FILE_SLOTS_TRAD);
>>>> + return;
>>>> + }
>>>> +
>>>> + /* (UINT16_MAX & FW_CFG_ENTRY_MASK) is the highest inclusive selector
>>>> value
>>>> + * that we permit. The actual (exclusive) value coming from the
>>>> + * configuration is (FW_CFG_FILE_FIRST + fw_cfg_file_slots(s)). */
>>>> + file_slots_max = (UINT16_MAX & FW_CFG_ENTRY_MASK) - FW_CFG_FILE_FIRST
>>>> + 1;
>>>> + if (fw_cfg_file_slots(s) > file_slots_max) {
>>>> + error_setg(errp, "\"file_slots\" must not exceed 0x%" PRIx16,
>>>> + file_slots_max);
>>>> + return;
>>>> + }
>>>> +
>>>> + s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>>> + s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>>> + s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>>>> +}
>>>>
>>>> static Property fw_cfg_io_properties[] = {
>>>> DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
>>>> DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>>>> DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>>>> true),
>>>> + DEFINE_PROP_UINT32("file_slots", FWCfgIoState, parent_obj.file_slots,
>>>> + FW_CFG_FILE_SLOTS_DFLT),
>>>> DEFINE_PROP_END_OF_LIST(),
>>>> };
>>>>
>>>> static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>>>> {
>>>> FWCfgIoState *s = FW_CFG_IO(dev);
>>>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>> + Error *local_err = NULL;
>>>> +
>>>> + fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + return;
>>>> + }
>>>>
>>>> /* when using port i/o, the 8-bit data register ALWAYS overlaps
>>>> * with half of the 16-bit control register. Hence, the total size
>>>> * of the i/o region used is FW_CFG_CTL_SIZE */
>>>> memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>>>> @@ -1061,18 +1121,27 @@ static const TypeInfo fw_cfg_io_info = {
>>>>
>>>> static Property fw_cfg_mem_properties[] = {
>>>> DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>>>> DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>>>> true),
>>>> + DEFINE_PROP_UINT32("file_slots", FWCfgMemState, parent_obj.file_slots,
>>>> + FW_CFG_FILE_SLOTS_DFLT),
>>>> DEFINE_PROP_END_OF_LIST(),
>>>> };
>>>>
>>>> static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>>>> {
>>>> FWCfgMemState *s = FW_CFG_MEM(dev);
>>>> SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>> const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
>>>> + Error *local_err = NULL;
>>>> +
>>>> + fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + return;
>>>> + }
>>>>
>>>> memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
>>>> FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
>>>> sysbus_init_mmio(sbd, &s->ctl_iomem);
>>>>
>>>
>>
>
- Re: [Qemu-devel] [PATCH v4 3/7] fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma(), (continued)
[Qemu-devel] [PATCH v4 2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property, Laszlo Ersek, 2016/12/01
Re: [Qemu-devel] [PATCH v4 2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property, Igor Mammedov, 2016/12/06
[Qemu-devel] [PATCH v4 4/7] hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots, Laszlo Ersek, 2016/12/01
[Qemu-devel] [PATCH v4 6/7] hw/isa/lpc_ich9: add broadcast SMI feature, Laszlo Ersek, 2016/12/01
[Qemu-devel] [PATCH v4 1/7] fw-cfg: support writeable blobs, Laszlo Ersek, 2016/12/01
[Qemu-devel] [PATCH v4 5/7] hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg, Laszlo Ersek, 2016/12/01
[Qemu-devel] [PATCH v4 7/7] hw/i386/pc_q35: advertise broadcast SMI if VCPU hotplug is turned off, Laszlo Ersek, 2016/12/01