Re: [Qemu-devel] [PATCH 2/2] hw/acpi/vmgenid: prevent more than one vmgenid device

[Laszlo Ersek]

On 03/20/17 17:57, Michael S. Tsirkin wrote:
> On Mon, Mar 20, 2017 at 05:39:18PM +0100, Laszlo Ersek wrote:
>> On 03/20/17 17:26, Michael S. Tsirkin wrote:
>>> On Mon, Mar 20, 2017 at 05:22:16PM +0100, Laszlo Ersek wrote:
>>>> On 03/20/17 16:13, Laszlo Ersek wrote:
>>>>> On 03/20/17 15:16, Michael S. Tsirkin wrote:
>>>>>> On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote:
>>>>>>> Multiple instances make no sense.
>>>>>>>
>>>>>>> Cc: "Michael S. Tsirkin" <address@hidden>
>>>>>>> Cc: Ben Warren <address@hidden>
>>>>>>> Cc: Igor Mammedov <address@hidden>
>>>>>>> Signed-off-by: Laszlo Ersek <address@hidden>
>>>>>>
>>>>>> find_vmgenid_dev would be a better place for this.
>>>>>> This is where the single instance assumption comes from ATM.
>>>>>
>>>>> object_resolve_path_type() -- used internally in find_vmgenid_dev() --
>>>>> returns NULL in either of two cases: there is no such device, or there
>>>>> are multiple devices. You can tell them apart by looking at the last
>>>>> parameter (called "ambiguous"), but find_vmgenid_dev() doesn't use that
>>>>> parameter.
>>>>>
>>>>> By the time we are in the vmgenid_realize() function, at least one
>>>>> vmgenid device is guaranteed to exist (the one which we are realizing).
>>>>> Therefore, this patch could be simplified as:
>>>>>
>>>>> if (find_vmgenid_dev() == NULL) {
>>>>>   error_setg(errp, "at most one %s device is permitted", VMGENID_DEVICE);
>>>>>   return;
>>>>> }
>>>>>
>>>>> I found that confusing, and wanted to spell out "ambiguous" with the
>>>>> assert(). If you prefer the above simpler (but harder to understand)
>>>>> check, I can do that too.
>>>>
>>>> Also, find_vmgenid_dev() only captures the single instance assumption,
>>>> it does not dictate the assumption. The assumption comes from the spec.
>>>
>>> I don't see this assumption anywhere in spec. What do you have in mind?
>>
>> It has language like
>>
>> "1. Put the generation ID in an 8-byte aligned buffer in guest RAM [...]"
>>
>> "2. Expose a device somewhere in the ACPI namespace [...]"
>>
>> "5. When the generation ID changes, execute an ACPI Notify operation on
>> the generation ID device [...]"
>>
>> "After the identifier has been made persistent in the configuration [...]"
>>
>> The spec defines a system-wide feature, and in all contexts it implies
>> there is only one of those things. The multiple device case is undefined
>> by omission, if you will.
> 
> I see.
> 
>>>> With the above in mind, what do you say about this patch? Do you want me
>>>> to call find_vmgenid_dev() in the realize function (which will require a
>>>> comment about object_resolve_path_type's behavior), or are you okay with
>>>> the patch as-is? (The asserts make it clear IMO.)
>>>>
>>>> Thanks
>>>> Laszlo
>>>
>>> I prefer calling find_vmgenid_dev, and adding a comment
>>> near find_vmgenid_dev.
>>
>> Near the function definition in "include/hw/acpi/vmgenid.h", or the call
>> site in the realize function?
>>
>> Thanks
>> Laszlo
> 
> I'd put it near the function itself.

Thanks -- meanwhile I figured I'd put a comment at both locations, just
to be sure. I'm about to post v2.

Thanks!
Laszlo