diff mbox

[2/2] hw/acpi/vmgenid: prevent more than one vmgenid device

Message ID 20170320115951.25345-3-lersek@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laszlo Ersek March 20, 2017, 11:59 a.m. UTC
Multiple instances make no sense.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/acpi/vmgenid.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Michael S. Tsirkin March 20, 2017, 2:16 p.m. UTC | #1
On Mon, Mar 20, 2017 at 12:59:51PM +0100, Laszlo Ersek wrote:
> Multiple instances make no sense.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Ben Warren <ben@skyportsystems.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

find_vmgenid_dev would be a better place for this.
This is where the single instance assumption comes from ATM.


> ---
>  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
Laszlo Ersek March 20, 2017, 3:13 p.m. UTC | #2
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" <mst@redhat.com>
>> Cc: Ben Warren <ben@skyportsystems.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> 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.

Thanks
Laszlo

> 
> 
>> ---
>>  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
Laszlo Ersek March 20, 2017, 4:22 p.m. UTC | #3
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" <mst@redhat.com>
>>> Cc: Ben Warren <ben@skyportsystems.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> 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.

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

> 
>>
>>
>>> ---
>>>  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
>
Michael S. Tsirkin March 20, 2017, 4:26 p.m. UTC | #4
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" <mst@redhat.com>
> >>> Cc: Ben Warren <ben@skyportsystems.com>
> >>> Cc: Igor Mammedov <imammedo@redhat.com>
> >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>
> >> 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?

> 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.

> > 
> >>
> >>
> >>> ---
> >>>  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
> >
Laszlo Ersek March 20, 2017, 4:39 p.m. UTC | #5
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" <mst@redhat.com>
>>>>> Cc: Ben Warren <ben@skyportsystems.com>
>>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>> 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.

>> 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

> 
>>>
>>>>
>>>>
>>>>> ---
>>>>>  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
>>>
Michael S. Tsirkin March 20, 2017, 4:57 p.m. UTC | #6
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" <mst@redhat.com>
> >>>>> Cc: Ben Warren <ben@skyportsystems.com>
> >>>>> Cc: Igor Mammedov <imammedo@redhat.com>
> >>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>>>
> >>>> 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
> >>>
Laszlo Ersek March 20, 2017, 5:05 p.m. UTC | #7
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" <mst@redhat.com>
>>>>>>> Cc: Ben Warren <ben@skyportsystems.com>
>>>>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>
>>>>>> 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
diff mbox

Patch

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);
 }