Re: [Qemu-devel] [PATCH v5 05/10] ACPI: Add Virtual Machine Generation ID support

[Ben Warren]

> On Feb 6, 2017, at 11:04 AM, Michael S. Tsirkin <address@hidden> wrote:
> 
> On Mon, Feb 06, 2017 at 10:48:05AM -0800, Ben Warren wrote:
>> 
>>> On Feb 6, 2017, at 10:17 AM, Michael S. Tsirkin <address@hidden> wrote:
>>> 
>>> On Mon, Feb 06, 2017 at 09:59:55AM -0800, Ben Warren wrote:
>>>>              +void vmgenid_add_fw_cfg(FWCfgState *s, GArray *guid)
>>>>              +{
>>>>              +    Object *obj = find_vmgenid_dev(NULL);
>>>>              +    assert(obj);
>>>>              +    VmGenIdState *vms = VMGENID(obj);
>>>>              +
>>>>              +    /* Create a read-only fw_cfg file for GUID */
>>>>              +    fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, guid->data,
>>>>              +                    VMGENID_FW_CFG_SIZE);
>>>>              +    /* Create a read-write fw_cfg file for Address */
>>>>              +    fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, 
>>>> NULL,
>>>>       NULL,
>>>> 
>>>> 
>>>>          Seems wrong. What if guest updates the address after command line
>>>>          set it? You want a callback to copy guid there.
>>>> 
>>>> 
>>>>       Sure, I can do that.  My understanding was that this is a read
>>>>       callback, but if
>>>>       it also is called upon a write, we should do what you suggest.
>>>> 
>>>> 
>>>>   Hmm you are right. But we really need to call something
>>>>   on write though - unlike read, it must be called after write.
>>>>   Otherwise I don't see how it can work if you set gen id before
>>>>   guest boots.
>>>> 
>>>>   I guess this means we need yet another callback per file.
>>>>   FWCfgWriteCallback ?
>>>> 
>>>>   Can you implement this in hw/nvram/fw_cfg.c?
>>>>   It's rather straight-forward to do.
>>>> 
>>>> 
>>>> The reason it works is that we put the initial contents of the GUID (as
>>>> supplied by command-line) into the GUID fw_cfg in the 
>>>> ‘vmgenid_build_acpi()’
>>>> function, which is guaranteed to happen before the guest boots.  The only 
>>>> time
>>>> QEMU needs to know VGIA is on later updates to the GUID (via monitor) or 
>>>> when
>>>> restoring.  If you really think this extra complexity is needed, I can do 
>>>> so,
>>>> but it seems to work very well as-is.
>>> 
>>> I see. So it's a race condition I think.  It works unless you change the
>>> gen id after bios read the guid from the guid file but before it wrote
>>> out the address.
>>> 
>>> Or do I miss something?
>>> 
>> I don’t think it’s an issue because BIOS is not a consumer of the
>> GUID.  I suppose there’s a window where another GUID could be written
>> via monitor prior to bios writing back, but the change wouldn’t be
>> applied to memory in vmgenid_update_guest() because vgia=0.  That
>> would be solved by a write callback and we could remove the GUID
>> copying from vmgenid_build_acpi().  It’s a very unlikely scenario,
>> though.
> 
> When people run 100000 VMs and up, every unlikely scenario tends to
> trigger.
> 
Right, your point is valid and I don’t want to ever have to debug that type of 
race.  It turns out that the callback in fw_cfg is tied to select(), not 
read(), so it is called upon write().  There doesn’t seem to be a need to add a 
new callback after all.  I’ll go down this path to remove this potential race.
>>> -- 
>>> MST

smime.p7s
Description: S/MIME cryptographic signature