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

[Michael S. Tsirkin]

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.

> > 
> >>>
> >>>>
> >>>>
> >>>>> ---
> >>>>>  hw/acpi/vmgenid.c | 10 ++++++++++
> >>>>>  1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> >>>>> index c3ddcc8e7cb0..b5c0dfcf19e1 100644
> >>>>> --- a/hw/acpi/vmgenid.c
> >>>>> +++ b/hw/acpi/vmgenid.c
> >>>>> @@ -214,6 +214,8 @@ static Property vmgenid_properties[] = {
> >>>>>  static void vmgenid_realize(DeviceState *dev, Error **errp)
> >>>>>  {
> >>>>>      VmGenIdState *vms = VMGENID(dev);
> >>>>> +    Object *one_vmgenid;
> >>>>> +    bool ambiguous;
> >>>>>  
> >>>>>      if (!vms->write_pointer_available) {
> >>>>>          error_setg(errp, "%s requires DMA write support in fw_cfg, "
> >>>>> @@ -221,6 +223,14 @@ static void vmgenid_realize(DeviceState *dev, 
> >>>>> Error **errp)
> >>>>>          return;
> >>>>>      }
> >>>>>  
> >>>>> +    one_vmgenid = object_resolve_path_type("", VMGENID_DEVICE, 
> >>>>> &ambiguous);
> >>>>> +    if (one_vmgenid == NULL) {
> >>>>> +        assert(ambiguous);
> >>>>> +        error_setg(errp, "at most one %s device is permitted", 
> >>>>> VMGENID_DEVICE);
> >>>>> +        return;
> >>>>> +    }
> >>>>> +    assert(one_vmgenid == OBJECT(vms));
> >>>>> +
> >>>>>      qemu_register_reset(vmgenid_handle_reset, vms);
> >>>>>  }
> >>>>>  
> >>>>> -- 
> >>>>> 2.9.3
> >>>