[Qemu-devel] [PATCH v2 0/2] Add a UUID device property to IPMI

[Qemu-devel] [PATCH v2 0/2] Add a UUID device property to IPMI

[minyard]

I sent this out a while ago and didn't really receive any comments then
kind of forgot about it.  These changes are not critical, but are really
necessary for certain situations and testing to make things behave like
they really should:

* It allows a BMC to be created with no UUID, returning an error, which
  is the behavior of many BMCs in the world.
* It lets the user set the UUID to a fixed value.

Some software using IPMI will get confused if it gets different UUIDs
from what should be the same device, which is what happens now if qemu
quits and restarts.

So sending out for review again.

Thanks,

-corey

[Qemu-devel] [PATCH v2 1/2] qdev: Add a no default uuid property

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This is for IPMI, which will behave differently if the UUID is
not set.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Fam Zheng <famz@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/qdev-properties.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 1eae5ab056..7fd887af84 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -237,6 +237,13 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
         .set_default = true,                                       \
         }
 
+#define DEFINE_PROP_UUID_NODEFAULT(_name, _state, _field) {        \
+        .name      = (_name),                                      \
+        .info      = &qdev_prop_uuid,                              \
+        .offset    = offsetof(_state, _field)                      \
+            + type_check(QemuUUID, typeof_field(_state, _field)),  \
+        }
+
 #define DEFINE_PROP_END_OF_LIST()               \
     {}
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 1/2] qdev: Add a no default uuid property

[Philippe Mathieu-Daudé]

Cc'ing Markus & Eric.

On 7/1/19 8:30 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This is for IPMI, which will behave differently if the UUID is
> not set.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/hw/qdev-properties.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 1eae5ab056..7fd887af84 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -237,6 +237,13 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>          .set_default = true,                                       \
>          }
>  
> +#define DEFINE_PROP_UUID_NODEFAULT(_name, _state, _field) {        \
> +        .name      = (_name),                                      \
> +        .info      = &qdev_prop_uuid,                              \
> +        .offset    = offsetof(_state, _field)                      \
> +            + type_check(QemuUUID, typeof_field(_state, _field)),  \
> +        }
> +
>  #define DEFINE_PROP_END_OF_LIST()               \
>      {}
>  
> 

[Qemu-devel] [PATCH v2 2/2] ipmi: Add a UUID device property

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Using the UUID that qemu generates probably isn't the best thing
to do, allow it to be passed in via properties, and use QemuUUID
for the type.

If the UUID is not set, return an unsupported command error.  This
way we are not providing an all-zero (or randomly generated) GUID
to the IPMI user.  This lets the host fall back to the other
method of using the get device id command to determind the BMC
being accessed.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Cédric Le Goater <clg@kaod.org>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ipmi/ipmi_bmc_sim.c | 22 ++++++++++++++--------
 qemu-options.hx        | 10 +++++++---
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 1980536517..007e2c6588 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -221,7 +221,7 @@ struct IPMIBmcSim {
     uint8_t restart_cause;
 
     uint8_t acpi_power_state[2];
-    uint8_t uuid[16];
+    QemuUUID uuid;
 
     IPMISel sel;
     IPMISdr sdr;
@@ -937,8 +937,19 @@ static void get_device_guid(IPMIBmcSim *ibs,
 {
     unsigned int i;
 
+    /* An uninitialized uuid is all zeros, use that to know if it is set. */
     for (i = 0; i < 16; i++) {
-        rsp_buffer_push(rsp, ibs->uuid[i]);
+        if (ibs->uuid.data[i]) {
+            goto uuid_set;
+        }
+    }
+    /* No uuid is set, return an error. */
+    rsp_buffer_set_error(rsp, IPMI_CC_INVALID_CMD);
+    return;
+
+ uuid_set:
+    for (i = 0; i < 16; i++) {
+        rsp_buffer_push(rsp, ibs->uuid.data[i]);
     }
 }
 
@@ -1980,12 +1991,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
     ibs->acpi_power_state[0] = 0;
     ibs->acpi_power_state[1] = 0;
 
-    if (qemu_uuid_set) {
-        memcpy(&ibs->uuid, &qemu_uuid, 16);
-    } else {
-        memset(&ibs->uuid, 0, 16);
-    }
-
     ipmi_init_sensors_from_sdrs(ibs);
     register_cmds(ibs);
 
@@ -2005,6 +2010,7 @@ static Property ipmi_sim_properties[] = {
     DEFINE_PROP_UINT8("fwrev2", IPMIBmcSim, fwrev2, 0),
     DEFINE_PROP_UINT32("mfg_id", IPMIBmcSim, mfg_id, 0),
     DEFINE_PROP_UINT16("product_id", IPMIBmcSim, product_id, 0),
+    DEFINE_PROP_UUID_NODEFAULT("guid", IPMIBmcSim, uuid),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 0d8beb4afd..ec56ab8f6f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -700,7 +700,7 @@ possible drivers and properties, use @code{-device help} and
 @code{-device @var{driver},help}.
 
 Some drivers are:
-@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
+@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}][,guid=@var{uuid}]
 
 Add an IPMI BMC.  This is a simulation of a hardware management
 interface processor that normally sits on a system.  It provides
@@ -713,8 +713,8 @@ controllers.  If you don't know what this means, it is safe to ignore
 it.
 
 @table @option
-@item bmc=@var{id}
-The BMC to connect to, one of ipmi-bmc-sim or ipmi-bmc-extern above.
+@item id=@var{id}
+The BMC id for interfaces to use this device.
 @item slave_addr=@var{val}
 Define slave address to use for the BMC.  The default is 0x20.
 @item sdrfile=@var{file}
@@ -723,6 +723,10 @@ file containing raw Sensor Data Records (SDR) data. The default is none.
 size of a Field Replaceable Unit (FRU) area.  The default is 1024.
 @item frudatafile=@var{file}
 file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
+@item guid=@var{uuid}
+value for the GUID for the BMC, in standard UUID format.  If this is set,
+get "Get GUID" command to the BMC will return it.  Otherwise "Get GUID"
+will return an error.
 @end table
 
 @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/2] ipmi: Add a UUID device property

[Philippe Mathieu-Daudé]

On 7/1/19 8:31 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Using the UUID that qemu generates probably isn't the best thing
> to do, allow it to be passed in via properties, and use QemuUUID
> for the type.
> 
> If the UUID is not set, return an unsupported command error.  This
> way we are not providing an all-zero (or randomly generated) GUID
> to the IPMI user.  This lets the host fall back to the other
> method of using the get device id command to determind the BMC
> being accessed.

A qtest for this would be much appreciated! (for 4.2 cycle).

> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ipmi/ipmi_bmc_sim.c | 22 ++++++++++++++--------
>  qemu-options.hx        | 10 +++++++---
>  2 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 1980536517..007e2c6588 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -221,7 +221,7 @@ struct IPMIBmcSim {
>      uint8_t restart_cause;
>  
>      uint8_t acpi_power_state[2];
> -    uint8_t uuid[16];
> +    QemuUUID uuid;
>  
>      IPMISel sel;
>      IPMISdr sdr;
> @@ -937,8 +937,19 @@ static void get_device_guid(IPMIBmcSim *ibs,
>  {
>      unsigned int i;
>  
> +    /* An uninitialized uuid is all zeros, use that to know if it is set. */
>      for (i = 0; i < 16; i++) {
> -        rsp_buffer_push(rsp, ibs->uuid[i]);
> +        if (ibs->uuid.data[i]) {
> +            goto uuid_set;
> +        }
> +    }

You can drop this for() loop and simplify as:

       if (qemu_uuid_is_null(&ibs->uuid) {

> +    /* No uuid is set, return an error. */
> +    rsp_buffer_set_error(rsp, IPMI_CC_INVALID_CMD);
> +    return;

}

> +
> + uuid_set:
> +    for (i = 0; i < 16; i++) {

16 -> ARRAY_SIZE(ibs->uuid.data) ?

> +        rsp_buffer_push(rsp, ibs->uuid.data[i]);
>      }
>  }
>  
> @@ -1980,12 +1991,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>      ibs->acpi_power_state[0] = 0;
>      ibs->acpi_power_state[1] = 0;
>  
> -    if (qemu_uuid_set) {
> -        memcpy(&ibs->uuid, &qemu_uuid, 16);
> -    } else {
> -        memset(&ibs->uuid, 0, 16);
> -    }
> -
>      ipmi_init_sensors_from_sdrs(ibs);
>      register_cmds(ibs);
>  
> @@ -2005,6 +2010,7 @@ static Property ipmi_sim_properties[] = {
>      DEFINE_PROP_UINT8("fwrev2", IPMIBmcSim, fwrev2, 0),
>      DEFINE_PROP_UINT32("mfg_id", IPMIBmcSim, mfg_id, 0),
>      DEFINE_PROP_UINT16("product_id", IPMIBmcSim, product_id, 0),
> +    DEFINE_PROP_UUID_NODEFAULT("guid", IPMIBmcSim, uuid),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 0d8beb4afd..ec56ab8f6f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -700,7 +700,7 @@ possible drivers and properties, use @code{-device help} and
>  @code{-device @var{driver},help}.
>  
>  Some drivers are:
> -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
> +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}][,guid=@var{uuid}]
>  
>  Add an IPMI BMC.  This is a simulation of a hardware management
>  interface processor that normally sits on a system.  It provides
> @@ -713,8 +713,8 @@ controllers.  If you don't know what this means, it is safe to ignore
>  it.
>  
>  @table @option
> -@item bmc=@var{id}
> -The BMC to connect to, one of ipmi-bmc-sim or ipmi-bmc-extern above.
> +@item id=@var{id}
> +The BMC id for interfaces to use this device.
>  @item slave_addr=@var{val}
>  Define slave address to use for the BMC.  The default is 0x20.
>  @item sdrfile=@var{file}
> @@ -723,6 +723,10 @@ file containing raw Sensor Data Records (SDR) data. The default is none.
>  size of a Field Replaceable Unit (FRU) area.  The default is 1024.
>  @item frudatafile=@var{file}
>  file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
> +@item guid=@var{uuid}
> +value for the GUID for the BMC, in standard UUID format.  If this is set,
> +get "Get GUID" command to the BMC will return it.  Otherwise "Get GUID"
> +will return an error.
>  @end table
>  
>  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
> 

Using qemu_uuid_is_null:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/2] ipmi: Add a UUID device property

[Daniel P. Berrangé]

On Mon, Jul 01, 2019 at 01:31:00PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Using the UUID that qemu generates probably isn't the best thing
> to do, allow it to be passed in via properties, and use QemuUUID
> for the type.

AFAICT, QEMU isn't generating a UUID in the current code.

The device is taking the UUID that the user has set
explicitly via the --uuid argument to QEMU. If --uuid
is not set, then it is leaving it all zeros.

Defaulting to the UUID from --uuid looks quite reasonable
to me & I don't think we should break that current usage.

I can see justification for being able to further override
that default with a device level property though.


> If the UUID is not set, return an unsupported command error.  This
> way we are not providing an all-zero (or randomly generated) GUID
> to the IPMI user.  This lets the host fall back to the other
> method of using the get device id command to determind the BMC
> being accessed.

Reporting an error would be a guest ABI regression from current QEMU
behaviour for anyone who is using the IPMI device right now, without
--uuid.

I'm not sure how much we care about guest ABI preservation for
the IPIMI device right now though ? Does it support migration
at all ?

> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ipmi/ipmi_bmc_sim.c | 22 ++++++++++++++--------
>  qemu-options.hx        | 10 +++++++---
>  2 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 1980536517..007e2c6588 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -221,7 +221,7 @@ struct IPMIBmcSim {
>      uint8_t restart_cause;
>  
>      uint8_t acpi_power_state[2];
> -    uint8_t uuid[16];
> +    QemuUUID uuid;
>  
>      IPMISel sel;
>      IPMISdr sdr;
> @@ -937,8 +937,19 @@ static void get_device_guid(IPMIBmcSim *ibs,
>  {
>      unsigned int i;
>  
> +    /* An uninitialized uuid is all zeros, use that to know if it is set. */
>      for (i = 0; i < 16; i++) {
> -        rsp_buffer_push(rsp, ibs->uuid[i]);
> +        if (ibs->uuid.data[i]) {
> +            goto uuid_set;
> +        }
> +    }
> +    /* No uuid is set, return an error. */
> +    rsp_buffer_set_error(rsp, IPMI_CC_INVALID_CMD);
> +    return;
> +
> + uuid_set:
> +    for (i = 0; i < 16; i++) {
> +        rsp_buffer_push(rsp, ibs->uuid.data[i]);
>      }
>  }
>  
> @@ -1980,12 +1991,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>      ibs->acpi_power_state[0] = 0;
>      ibs->acpi_power_state[1] = 0;
>  
> -    if (qemu_uuid_set) {
> -        memcpy(&ibs->uuid, &qemu_uuid, 16);
> -    } else {
> -        memset(&ibs->uuid, 0, 16);
> -    }
> -
>      ipmi_init_sensors_from_sdrs(ibs);
>      register_cmds(ibs);
>  
> @@ -2005,6 +2010,7 @@ static Property ipmi_sim_properties[] = {
>      DEFINE_PROP_UINT8("fwrev2", IPMIBmcSim, fwrev2, 0),
>      DEFINE_PROP_UINT32("mfg_id", IPMIBmcSim, mfg_id, 0),
>      DEFINE_PROP_UINT16("product_id", IPMIBmcSim, product_id, 0),
> +    DEFINE_PROP_UUID_NODEFAULT("guid", IPMIBmcSim, uuid),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 0d8beb4afd..ec56ab8f6f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -700,7 +700,7 @@ possible drivers and properties, use @code{-device help} and
>  @code{-device @var{driver},help}.
>  
>  Some drivers are:
> -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
> +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}][,guid=@var{uuid}]
>  
>  Add an IPMI BMC.  This is a simulation of a hardware management
>  interface processor that normally sits on a system.  It provides
> @@ -713,8 +713,8 @@ controllers.  If you don't know what this means, it is safe to ignore
>  it.
>  
>  @table @option
> -@item bmc=@var{id}
> -The BMC to connect to, one of ipmi-bmc-sim or ipmi-bmc-extern above.
> +@item id=@var{id}
> +The BMC id for interfaces to use this device.
>  @item slave_addr=@var{val}
>  Define slave address to use for the BMC.  The default is 0x20.
>  @item sdrfile=@var{file}
> @@ -723,6 +723,10 @@ file containing raw Sensor Data Records (SDR) data. The default is none.
>  size of a Field Replaceable Unit (FRU) area.  The default is 1024.
>  @item frudatafile=@var{file}
>  file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
> +@item guid=@var{uuid}
> +value for the GUID for the BMC, in standard UUID format.  If this is set,
> +get "Get GUID" command to the BMC will return it.  Otherwise "Get GUID"
> +will return an error.
>  @end table
>  
>  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
> -- 
> 2.17.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/2] ipmi: Add a UUID device property

[Corey Minyard]

On Wed, Jul 03, 2019 at 03:26:25PM +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 01, 2019 at 01:31:00PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > Using the UUID that qemu generates probably isn't the best thing
> > to do, allow it to be passed in via properties, and use QemuUUID
> > for the type.
> 
> AFAICT, QEMU isn't generating a UUID in the current code.

Yeah, it doesn't appear to.

> 
> The device is taking the UUID that the user has set
> explicitly via the --uuid argument to QEMU. If --uuid
> is not set, then it is leaving it all zeros.
> 
> Defaulting to the UUID from --uuid looks quite reasonable
> to me & I don't think we should break that current usage.

I originally thought that, but thinking it through some more,
that's not the way real systems work.  The system UUID and the
BMC UUID are generally (probably always) different.

Plus, defaulting it to the system UUID might result in a
surprise for the user.  In retrospect, I think it's better
to leave the UUID unset unless explicitly set by the user.

> 
> I can see justification for being able to further override
> that default with a device level property though.
> 
> 
> > If the UUID is not set, return an unsupported command error.  This
> > way we are not providing an all-zero (or randomly generated) GUID
> > to the IPMI user.  This lets the host fall back to the other
> > method of using the get device id command to determind the BMC
> > being accessed.
> 
> Reporting an error would be a guest ABI regression from current QEMU
> behaviour for anyone who is using the IPMI device right now, without
> --uuid.

Actually, it would only be a regression for anyone using --uuid.  If
the system UUID is all zero, then it will return an error currently.

> 
> I'm not sure how much we care about guest ABI preservation for
> the IPIMI device right now though ? Does it support migration
> at all ?

It does support migration, but the UUID is not migrated, since it
is a property set on the command line.  That means if you
migrated with --uuid and didn't set the IPMI UUID on the command
line for the migration target, it would change behavior.

ABI preservation is probably not important for this particular
feature, but there is a workaround.

-corey

> 
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > Cc: Cédric Le Goater <clg@kaod.org>
> > Cc: David Gibson <david@gibson.dropbear.id.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/ipmi/ipmi_bmc_sim.c | 22 ++++++++++++++--------
> >  qemu-options.hx        | 10 +++++++---
> >  2 files changed, 21 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> > index 1980536517..007e2c6588 100644
> > --- a/hw/ipmi/ipmi_bmc_sim.c
> > +++ b/hw/ipmi/ipmi_bmc_sim.c
> > @@ -221,7 +221,7 @@ struct IPMIBmcSim {
> >      uint8_t restart_cause;
> >  
> >      uint8_t acpi_power_state[2];
> > -    uint8_t uuid[16];
> > +    QemuUUID uuid;
> >  
> >      IPMISel sel;
> >      IPMISdr sdr;
> > @@ -937,8 +937,19 @@ static void get_device_guid(IPMIBmcSim *ibs,
> >  {
> >      unsigned int i;
> >  
> > +    /* An uninitialized uuid is all zeros, use that to know if it is set. */
> >      for (i = 0; i < 16; i++) {
> > -        rsp_buffer_push(rsp, ibs->uuid[i]);
> > +        if (ibs->uuid.data[i]) {
> > +            goto uuid_set;
> > +        }
> > +    }
> > +    /* No uuid is set, return an error. */
> > +    rsp_buffer_set_error(rsp, IPMI_CC_INVALID_CMD);
> > +    return;
> > +
> > + uuid_set:
> > +    for (i = 0; i < 16; i++) {
> > +        rsp_buffer_push(rsp, ibs->uuid.data[i]);
> >      }
> >  }
> >  
> > @@ -1980,12 +1991,6 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
> >      ibs->acpi_power_state[0] = 0;
> >      ibs->acpi_power_state[1] = 0;
> >  
> > -    if (qemu_uuid_set) {
> > -        memcpy(&ibs->uuid, &qemu_uuid, 16);
> > -    } else {
> > -        memset(&ibs->uuid, 0, 16);
> > -    }
> > -
> >      ipmi_init_sensors_from_sdrs(ibs);
> >      register_cmds(ibs);
> >  
> > @@ -2005,6 +2010,7 @@ static Property ipmi_sim_properties[] = {
> >      DEFINE_PROP_UINT8("fwrev2", IPMIBmcSim, fwrev2, 0),
> >      DEFINE_PROP_UINT32("mfg_id", IPMIBmcSim, mfg_id, 0),
> >      DEFINE_PROP_UINT16("product_id", IPMIBmcSim, product_id, 0),
> > +    DEFINE_PROP_UUID_NODEFAULT("guid", IPMIBmcSim, uuid),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 0d8beb4afd..ec56ab8f6f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -700,7 +700,7 @@ possible drivers and properties, use @code{-device help} and
> >  @code{-device @var{driver},help}.
> >  
> >  Some drivers are:
> > -@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}]
> > +@item -device ipmi-bmc-sim,id=@var{id}[,slave_addr=@var{val}][,sdrfile=@var{file}][,furareasize=@var{val}][,furdatafile=@var{file}][,guid=@var{uuid}]
> >  
> >  Add an IPMI BMC.  This is a simulation of a hardware management
> >  interface processor that normally sits on a system.  It provides
> > @@ -713,8 +713,8 @@ controllers.  If you don't know what this means, it is safe to ignore
> >  it.
> >  
> >  @table @option
> > -@item bmc=@var{id}
> > -The BMC to connect to, one of ipmi-bmc-sim or ipmi-bmc-extern above.
> > +@item id=@var{id}
> > +The BMC id for interfaces to use this device.
> >  @item slave_addr=@var{val}
> >  Define slave address to use for the BMC.  The default is 0x20.
> >  @item sdrfile=@var{file}
> > @@ -723,6 +723,10 @@ file containing raw Sensor Data Records (SDR) data. The default is none.
> >  size of a Field Replaceable Unit (FRU) area.  The default is 1024.
> >  @item frudatafile=@var{file}
> >  file containing raw Field Replaceable Unit (FRU) inventory data. The default is none.
> > +@item guid=@var{uuid}
> > +value for the GUID for the BMC, in standard UUID format.  If this is set,
> > +get "Get GUID" command to the BMC will return it.  Otherwise "Get GUID"
> > +will return an error.
> >  @end table
> >  
> >  @item -device ipmi-bmc-extern,id=@var{id},chardev=@var{id}[,slave_addr=@var{val}]
> > -- 
> > 2.17.1
> > 
> > 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|