Re: [Qemu-devel] [PATCH v4 2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property

[Laszlo Ersek]

On 12/02/16 12:10, Gerd Hoffmann wrote:
> On Do, 2016-12-01 at 18:06 +0100, Laszlo Ersek 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.
> 
> I don't think we need this.  fw_cfg changes are guest-visible so they
> must not happen for a given machine type.

Agreed.

> So if current machine types
> don't hit the limit

Please check one of the links in the blurb, under which Paolo noted that
we're already above the limit in the worst (theoretical) case.

In practice they don't hit the limit, indeed.

> that should continue to be the case even if we
> simply raise FW_CFG_FILE_SLOTS.

I'm not trying to make it hard for myself :), so in theory I don't
disagree with the idea. However, do consider backwards migration. (As
noted under the patch, I'm aware that upstream doesn't care, but that
shouldn't be reason enough to reject a patch that does care.)

Let's say we start a guest in the pc-q35-2.8 machtype on a new QEMU
release, as source QEMU host, which has the raised FW_CFG_FILE_SLOTS
value. The guest writes a high key value to the selector register, which
is valid under the raised limit, so the key value is stored (i.e.,
fw_cfg_select() won't store FW_CFG_INVALID in cur_entry).

Then the guest is migrated to the older release QEMU, where the value of
cur_entry is larger than or equal to FW_CFG_MAX_ENTRY. The guest then
reads the data register, and fw_cfg_data_read() does this:

    FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
                    &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
    uint64_t value = 0;

    assert(size > 0 && size <= sizeof(value));
    if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset <
e->len) {

Just computing the pointer "e" is undefined behavior, let alone
evaluating "e->data".

Once again, I know upstream doesn't care about backward migration, but
we (at Red Hat) do, for specific machine types at least. I would rather
carry this patch in upstream than in downstream only.

As far as I understand, this is the first time in QEMU history that
we're looking into raising FW_CFG_FILE_SLOTS, so I'd rather be careful.
(I definitely don't want to win the privilege to implement the patch in
downstream!)

> But we have to take care that new files show up on new machine types
> only.

The series covers that, yes -- if the SMI host features bitmap that is
returned by the new machtype-specific callback function at board
initialization is zero (or the callback doesn't exist), then the fw_cfg
files are not created. See how ich9_lpc_pm_init() is called, and how it
handles the new "smi_host_features" parameter.

Thanks
Laszlo