[Qemu-devel] [PATCH 0/2] some remaining vmgenid tweaks for 2.9

[Qemu-devel] [PATCH 0/2] some remaining vmgenid tweaks for 2.9

[Laszlo Ersek]

We've discussed both of these earlier (multiple times), and I'd been
hoping that Ben would submit them. I guess I can send out a reminder
formatted as a patch series. (I don't intend to post several iterations
of this.)

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ben Warren <ben@skyportsystems.com>
Cc: Igor Mammedov <imammedo@redhat.com>

Thanks
Laszlo

Laszlo Ersek (2):
  hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types
  hw/acpi/vmgenid: prevent more than one vmgenid device

 include/hw/acpi/vmgenid.h |  1 +
 include/hw/compat.h       |  4 ++++
 hw/acpi/vmgenid.c         | 24 ++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

-- 
2.9.3

[Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types

[Laszlo Ersek]

The WRITE_POINTER linker/loader command that underlies VMGENID depends on
commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That
commit is not available in 2.8.

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>
---
 include/hw/acpi/vmgenid.h |  1 +
 include/hw/compat.h       |  4 ++++
 hw/acpi/vmgenid.c         | 14 ++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
index db7fa0e63303..8578476baebe 100644
--- a/include/hw/acpi/vmgenid.h
+++ b/include/hw/acpi/vmgenid.h
@@ -21,6 +21,7 @@ typedef struct VmGenIdState {
     DeviceClass parent_obj;
     QemuUUID guid;                /* The 128-bit GUID seen by the guest */
     uint8_t vmgenid_addr_le[8];   /* Address of the GUID (little-endian) */
+    bool write_pointer_available;
 } VmGenIdState;
 
 static inline Object *find_vmgenid_dev(void)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index fc8c3e060007..c9caa8cd1bdd 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -42,6 +42,10 @@
         .driver   = "isa-cirrus-vga",\
         .property = "vgamem_mb",\
         .value    = "8",\
+    },{\
+        .driver   = "vmgenid",\
+        .property = "x-write-pointer-available",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_7 \
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index 7a3ad17d66ef..c3ddcc8e7cb0 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque)
     memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
 }
 
+static Property vmgenid_properties[] = {
+    DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState,
+                     write_pointer_available, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void vmgenid_realize(DeviceState *dev, Error **errp)
 {
     VmGenIdState *vms = VMGENID(dev);
+
+    if (!vms->write_pointer_available) {
+        error_setg(errp, "%s requires DMA write support in fw_cfg, "
+                   "which this machine type does not provide", VMGENID_DEVICE);
+        return;
+    }
+
     qemu_register_reset(vmgenid_handle_reset, vms);
 }
 
@@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_vmgenid;
     dc->realize = vmgenid_realize;
     dc->hotpluggable = false;
+    dc->props = vmgenid_properties;
 
     object_class_property_add_str(klass, VMGENID_GUID, NULL,
                                   vmgenid_set_guid, NULL);
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types

[Michael S. Tsirkin]

On Mon, Mar 20, 2017 at 12:59:50PM +0100, Laszlo Ersek wrote:
> The WRITE_POINTER linker/loader command that underlies VMGENID depends on
> commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That
> commit is not available in 2.8.
> 
> 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>

I don't understand why do we want to add code to break this
intentionally. What issue does this fix?

> ---
>  include/hw/acpi/vmgenid.h |  1 +
>  include/hw/compat.h       |  4 ++++
>  hw/acpi/vmgenid.c         | 14 ++++++++++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
> index db7fa0e63303..8578476baebe 100644
> --- a/include/hw/acpi/vmgenid.h
> +++ b/include/hw/acpi/vmgenid.h
> @@ -21,6 +21,7 @@ typedef struct VmGenIdState {
>      DeviceClass parent_obj;
>      QemuUUID guid;                /* The 128-bit GUID seen by the guest */
>      uint8_t vmgenid_addr_le[8];   /* Address of the GUID (little-endian) */
> +    bool write_pointer_available;
>  } VmGenIdState;
>  
>  static inline Object *find_vmgenid_dev(void)
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index fc8c3e060007..c9caa8cd1bdd 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -42,6 +42,10 @@
>          .driver   = "isa-cirrus-vga",\
>          .property = "vgamem_mb",\
>          .value    = "8",\
> +    },{\
> +        .driver   = "vmgenid",\
> +        .property = "x-write-pointer-available",\
> +        .value    = "off",\
>      },
>  
>  #define HW_COMPAT_2_7 \
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index 7a3ad17d66ef..c3ddcc8e7cb0 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque)
>      memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
>  }
>  
> +static Property vmgenid_properties[] = {
> +    DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState,
> +                     write_pointer_available, true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void vmgenid_realize(DeviceState *dev, Error **errp)
>  {
>      VmGenIdState *vms = VMGENID(dev);
> +
> +    if (!vms->write_pointer_available) {
> +        error_setg(errp, "%s requires DMA write support in fw_cfg, "
> +                   "which this machine type does not provide", VMGENID_DEVICE);
> +        return;
> +    }
> +
>      qemu_register_reset(vmgenid_handle_reset, vms);
>  }
>  
> @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &vmstate_vmgenid;
>      dc->realize = vmgenid_realize;
>      dc->hotpluggable = false;
> +    dc->props = vmgenid_properties;
>  
>      object_class_property_add_str(klass, VMGENID_GUID, NULL,
>                                    vmgenid_set_guid, NULL);
> -- 
> 2.9.3
> 

Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types

[Paolo Bonzini]

On 20/03/2017 15:19, Michael S. Tsirkin wrote:
> On Mon, Mar 20, 2017 at 12:59:50PM +0100, Laszlo Ersek wrote:
>> The WRITE_POINTER linker/loader command that underlies VMGENID depends on
>> commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That
>> commit is not available in 2.8.
>>
>> 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>
> 
> I don't understand why do we want to add code to break this
> intentionally. What issue does this fix?

Agreed.  If vmgenid can work with old machine types and new QEMU, it
should.  For example, when using libvirt machine types identify the
machine type where the VM was _created_ (unless someone changes it), not
necessarily the minimal QEMU version that can run it.

Paolo

>> ---
>>  include/hw/acpi/vmgenid.h |  1 +
>>  include/hw/compat.h       |  4 ++++
>>  hw/acpi/vmgenid.c         | 14 ++++++++++++++
>>  3 files changed, 19 insertions(+)
>>
>> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
>> index db7fa0e63303..8578476baebe 100644
>> --- a/include/hw/acpi/vmgenid.h
>> +++ b/include/hw/acpi/vmgenid.h
>> @@ -21,6 +21,7 @@ typedef struct VmGenIdState {
>>      DeviceClass parent_obj;
>>      QemuUUID guid;                /* The 128-bit GUID seen by the guest */
>>      uint8_t vmgenid_addr_le[8];   /* Address of the GUID (little-endian) */
>> +    bool write_pointer_available;
>>  } VmGenIdState;
>>  
>>  static inline Object *find_vmgenid_dev(void)
>> diff --git a/include/hw/compat.h b/include/hw/compat.h
>> index fc8c3e060007..c9caa8cd1bdd 100644
>> --- a/include/hw/compat.h
>> +++ b/include/hw/compat.h
>> @@ -42,6 +42,10 @@
>>          .driver   = "isa-cirrus-vga",\
>>          .property = "vgamem_mb",\
>>          .value    = "8",\
>> +    },{\
>> +        .driver   = "vmgenid",\
>> +        .property = "x-write-pointer-available",\
>> +        .value    = "off",\
>>      },
>>  
>>  #define HW_COMPAT_2_7 \
>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>> index 7a3ad17d66ef..c3ddcc8e7cb0 100644
>> --- a/hw/acpi/vmgenid.c
>> +++ b/hw/acpi/vmgenid.c
>> @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque)
>>      memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
>>  }
>>  
>> +static Property vmgenid_properties[] = {
>> +    DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState,
>> +                     write_pointer_available, true),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void vmgenid_realize(DeviceState *dev, Error **errp)
>>  {
>>      VmGenIdState *vms = VMGENID(dev);
>> +
>> +    if (!vms->write_pointer_available) {
>> +        error_setg(errp, "%s requires DMA write support in fw_cfg, "
>> +                   "which this machine type does not provide", VMGENID_DEVICE);
>> +        return;
>> +    }
>> +
>>      qemu_register_reset(vmgenid_handle_reset, vms);
>>  }
>>  
>> @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data)
>>      dc->vmsd = &vmstate_vmgenid;
>>      dc->realize = vmgenid_realize;
>>      dc->hotpluggable = false;
>> +    dc->props = vmgenid_properties;
>>  
>>      object_class_property_add_str(klass, VMGENID_GUID, NULL,
>>                                    vmgenid_set_guid, NULL);
>> -- 
>> 2.9.3
>>
> 

Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types

[Laszlo Ersek]

On 03/20/17 15:39, Paolo Bonzini wrote:
> 
> 
> On 20/03/2017 15:19, Michael S. Tsirkin wrote:
>> On Mon, Mar 20, 2017 at 12:59:50PM +0100, Laszlo Ersek wrote:
>>> The WRITE_POINTER linker/loader command that underlies VMGENID depends on
>>> commit baf2d5bfbac0 ("fw-cfg: support writeable blobs", 2017-01-12). That
>>> commit is not available in 2.8.
>>>
>>> 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>
>>
>> I don't understand why do we want to add code to break this
>> intentionally. What issue does this fix?
> 
> Agreed.  If vmgenid can work with old machine types and new QEMU, it
> should.  For example, when using libvirt machine types identify the
> machine type where the VM was _created_ (unless someone changes it), not
> necessarily the minimal QEMU version that can run it.

Yes, I understand that.

The problem is this:

(1) machine types up to and including 2.4 do not enable DMA for fw_cfg.
Without DMA, the WRITE_POINTER linker/loader commands have no chance of
working in the firmware.

(2) ... the other part of the problem was actually in my thinking, sorry
about that. I forgot that that "-device vmgenid" itself is not available
in QEMU releases prior to 2.9. So we shouldn't worry about the case
where write support is missing from fw_cfg, although DMA is available.

In this patch I tried to address (non-)problem (2), thereby covering
(real) problem (1). I now see that (2) doesn't actually exist. (1) does
have to be addressed however.

If you agree, I'll respin this patch.

Thanks
Laszlo



> 
> Paolo
> 
>>> ---
>>>  include/hw/acpi/vmgenid.h |  1 +
>>>  include/hw/compat.h       |  4 ++++
>>>  hw/acpi/vmgenid.c         | 14 ++++++++++++++
>>>  3 files changed, 19 insertions(+)
>>>
>>> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
>>> index db7fa0e63303..8578476baebe 100644
>>> --- a/include/hw/acpi/vmgenid.h
>>> +++ b/include/hw/acpi/vmgenid.h
>>> @@ -21,6 +21,7 @@ typedef struct VmGenIdState {
>>>      DeviceClass parent_obj;
>>>      QemuUUID guid;                /* The 128-bit GUID seen by the guest */
>>>      uint8_t vmgenid_addr_le[8];   /* Address of the GUID (little-endian) */
>>> +    bool write_pointer_available;
>>>  } VmGenIdState;
>>>  
>>>  static inline Object *find_vmgenid_dev(void)
>>> diff --git a/include/hw/compat.h b/include/hw/compat.h
>>> index fc8c3e060007..c9caa8cd1bdd 100644
>>> --- a/include/hw/compat.h
>>> +++ b/include/hw/compat.h
>>> @@ -42,6 +42,10 @@
>>>          .driver   = "isa-cirrus-vga",\
>>>          .property = "vgamem_mb",\
>>>          .value    = "8",\
>>> +    },{\
>>> +        .driver   = "vmgenid",\
>>> +        .property = "x-write-pointer-available",\
>>> +        .value    = "off",\
>>>      },
>>>  
>>>  #define HW_COMPAT_2_7 \
>>> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
>>> index 7a3ad17d66ef..c3ddcc8e7cb0 100644
>>> --- a/hw/acpi/vmgenid.c
>>> +++ b/hw/acpi/vmgenid.c
>>> @@ -205,9 +205,22 @@ static void vmgenid_handle_reset(void *opaque)
>>>      memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
>>>  }
>>>  
>>> +static Property vmgenid_properties[] = {
>>> +    DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState,
>>> +                     write_pointer_available, true),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>>  static void vmgenid_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      VmGenIdState *vms = VMGENID(dev);
>>> +
>>> +    if (!vms->write_pointer_available) {
>>> +        error_setg(errp, "%s requires DMA write support in fw_cfg, "
>>> +                   "which this machine type does not provide", VMGENID_DEVICE);
>>> +        return;
>>> +    }
>>> +
>>>      qemu_register_reset(vmgenid_handle_reset, vms);
>>>  }
>>>  
>>> @@ -218,6 +231,7 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data)
>>>      dc->vmsd = &vmstate_vmgenid;
>>>      dc->realize = vmgenid_realize;
>>>      dc->hotpluggable = false;
>>> +    dc->props = vmgenid_properties;
>>>  
>>>      object_class_property_add_str(klass, VMGENID_GUID, NULL,
>>>                                    vmgenid_set_guid, NULL);
>>> -- 
>>> 2.9.3
>>>
>>

Re: [Qemu-devel] [PATCH 1/2] hw/acpi/vmgenid: prevent device realization on pre-2.9 machine types

[Paolo Bonzini]

On 20/03/2017 16:08, Laszlo Ersek wrote:
> (1) machine types up to and including 2.4 do not enable DMA for fw_cfg.
> Without DMA, the WRITE_POINTER linker/loader commands have no chance of
> working in the firmware.
> 
> (2) ... the other part of the problem was actually in my thinking, sorry
> about that. I forgot that that "-device vmgenid" itself is not available
> in QEMU releases prior to 2.9. So we shouldn't worry about the case
> where write support is missing from fw_cfg, although DMA is available.
> 
> In this patch I tried to address (non-)problem (2), thereby covering
> (real) problem (1). I now see that (2) doesn't actually exist. (1) does
> have to be addressed however.
> 
> If you agree, I'll respin this patch.

Yes, please do respin it with pre-2.4 machine types limited only.

Paolo

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

[Laszlo Ersek]

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(+)

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

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

[Michael S. Tsirkin]

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

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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
> 

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

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

[Laszlo Ersek]

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

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

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