[Qemu-devel] [PATCH 0/2] ipmi: Allow UUID to be set for a BMC

[Qemu-devel] [PATCH 0/2] ipmi: Allow UUID to be set for a BMC

[minyard]

The code was using the qemu UUID for the BMC.  But that's really
not a good method.  In general, you don't want the GUID to change
when you migrate, and you want the GUID to be the same between
invocations of qemu (if you have a GUID).

Plus, if you have multiple BMCs, they need to have different
GUIDs or the host code cannot tell them apart.  I'm not sure
anyone really uses multiple BMCs, but I do a lot of testing
with that scenario.

This change lets the user set the GUID on the command line, and
if the GUID is not set return an error for the GUID fetch command.
This maps better to how IPMI should work.

This change relies on the UUID being set to all zeros to know that
it is not set.  This is not optimal, perhaps, but an all zero
UUID isn't valid (it's the Nil UUID), so it should be ok.

[Qemu-devel] [PATCH 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 4f60cc88f3..d614f931c9 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -226,6 +226,13 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
         .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

[Qemu-devel] [PATCH 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 9b509f829b..b88c021977 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -220,7 +220,7 @@ struct IPMIBmcSim {
     uint8_t restart_cause;
 
     uint8_t acpi_power_state[2];
-    uint8_t uuid[16];
+    QemuUUID uuid;
 
     IPMISel sel;
     IPMISdr sdr;
@@ -936,8 +936,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]);
     }
 }
 
@@ -1979,12 +1990,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);
 
@@ -2004,6 +2009,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 38c7a978c1..37e57e27d0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -470,7 +470,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
@@ -483,8 +483,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}
@@ -493,6 +493,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 0/2] ipmi: Allow UUID to be set for a BMC

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On Thu, Nov 08, 2018 at 08:19:42AM -0600, minyard@acm.org wrote:
> The code was using the qemu UUID for the BMC.  But that's really
> not a good method.  In general, you don't want the GUID to change
> when you migrate, and you want the GUID to be the same between
> invocations of qemu (if you have a GUID).

Hrm.  Generally the qemu UUID should remain the same across a
migration too, and I think that will be the case if using libvirt.
Maybe not if running qemu by hand and not specifying the uuid on the
command line.

I don't really have an objection to allowing the BMC's id to be
explicitly controlled, but the rationale above seems a bit
disingenuous.

> Plus, if you have multiple BMCs, they need to have different
> GUIDs or the host code cannot tell them apart.  I'm not sure
> anyone really uses multiple BMCs, but I do a lot of testing
> with that scenario.
> 
> This change lets the user set the GUID on the command line, and
> if the GUID is not set return an error for the GUID fetch command.
> This maps better to how IPMI should work.
> 
> This change relies on the UUID being set to all zeros to know that
> it is not set.  This is not optimal, perhaps, but an all zero
> UUID isn't valid (it's the Nil UUID), so it should be ok.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/2] ipmi: Allow UUID to be set for a BMC

[Corey Minyard]

On 11/8/18 5:22 PM, David Gibson wrote:
> On Thu, Nov 08, 2018 at 08:19:42AM -0600, minyard@acm.org wrote:
>> The code was using the qemu UUID for the BMC.  But that's really
>> not a good method.  In general, you don't want the GUID to change
>> when you migrate, and you want the GUID to be the same between
>> invocations of qemu (if you have a GUID).
> Hrm.  Generally the qemu UUID should remain the same across a
> migration too, and I think that will be the case if using libvirt.
> Maybe not if running qemu by hand and not specifying the uuid on the
> command line.
>
> I don't really have an objection to allowing the BMC's id to be
> explicitly controlled, but the rationale above seems a bit
> disingenuous.


I'm not sure about migration.  I suppose it could be migrated, but I
would consider the BMC part of the hardware that needs to be the
same on both sides.  It's a fuzzy line, I suppose.  The qemu UUID
is migrated, so I suppose that's not an issue.

Controlling it explicitly is important for some testing I do, and might
be for other people at some point in time, if you are trying to
emulate something specific.  And when re-invoking qemu, you
might want to keep it the same to avoid confusing software.

The big question for you is the lack of a GUID if it's not supplied.
With this change, the get GUID command will return a "command
not supported" error if the GUID is not supplied.

Thanks,

-corey

>> Plus, if you have multiple BMCs, they need to have different
>> GUIDs or the host code cannot tell them apart.  I'm not sure
>> anyone really uses multiple BMCs, but I do a lot of testing
>> with that scenario.
>>
>> This change lets the user set the GUID on the command line, and
>> if the GUID is not set return an error for the GUID fetch command.
>> This maps better to how IPMI should work.
>>
>> This change relies on the UUID being set to all zeros to know that
>> it is not set.  This is not optimal, perhaps, but an all zero
>> UUID isn't valid (it's the Nil UUID), so it should be ok.

Re: [Qemu-devel] [PATCH 0/2] ipmi: Allow UUID to be set for a BMC

[Paolo Bonzini]

On 09/11/18 14:33, Corey Minyard wrote:
> On 11/8/18 5:22 PM, David Gibson wrote:
> I'm not sure about migration.  I suppose it could be migrated, but I
> would consider the BMC part of the hardware that needs to be the
> same on both sides.  It's a fuzzy line, I suppose.  The qemu UUID
> is migrated, so I suppose that's not an issue.
> 
> Controlling it explicitly is important for some testing I do, and might
> be for other people at some point in time, if you are trying to
> emulate something specific.  And when re-invoking qemu, you
> might want to keep it the same to avoid confusing software.

I don't understand, do you need it to be different from the dmicode
UUID?  On real hardware are they the same or different?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 0/2] ipmi: Allow UUID to be set for a BMC

[Corey Minyard]

On 12/6/18 3:10 PM, Paolo Bonzini wrote:
> On 09/11/18 14:33, Corey Minyard wrote:
>> On 11/8/18 5:22 PM, David Gibson wrote:
>> I'm not sure about migration.  I suppose it could be migrated, but I
>> would consider the BMC part of the hardware that needs to be the
>> same on both sides.  It's a fuzzy line, I suppose.  The qemu UUID
>> is migrated, so I suppose that's not an issue.
>>
>> Controlling it explicitly is important for some testing I do, and might
>> be for other people at some point in time, if you are trying to
>> emulate something specific.  And when re-invoking qemu, you
>> might want to keep it the same to avoid confusing software.
> I don't understand, do you need it to be different from the dmicode
> UUID?  On real hardware are they the same or different?

They are different on real hardware, and software that uses IPMI
can expect that it doesn't change unless the BMC changes.  It's
not used that often, though.

It's often used to know that you are talking to the same BMC if
you have multiple connections (like a local connection and one
coming in over the LAN).

I primarily need it for testing things in the Linux IPMI driver,
identifying if BMCs connections are the same or not.

-corey

> Thanks,
>
> Paolo

Re: [Qemu-devel] [PATCH 0/2] ipmi: Allow UUID to be set for a BMC

[Paolo Bonzini]

On 06/12/18 22:27, Corey Minyard wrote:
> On 12/6/18 3:10 PM, Paolo Bonzini wrote:
>> On 09/11/18 14:33, Corey Minyard wrote:
>>> On 11/8/18 5:22 PM, David Gibson wrote:
>>> I'm not sure about migration.  I suppose it could be migrated, but I
>>> would consider the BMC part of the hardware that needs to be the
>>> same on both sides.  It's a fuzzy line, I suppose.  The qemu UUID
>>> is migrated, so I suppose that's not an issue.
>>>
>>> Controlling it explicitly is important for some testing I do, and might
>>> be for other people at some point in time, if you are trying to
>>> emulate something specific.  And when re-invoking qemu, you
>>> might want to keep it the same to avoid confusing software.
>> I don't understand, do you need it to be different from the dmicode
>> UUID?  On real hardware are they the same or different?
> 
> They are different on real hardware, and software that uses IPMI
> can expect that it doesn't change unless the BMC changes.

You can say the latter of the dmidecode UUID too, it's not supposed to
change.  However, if they are different in real hardware they should be
different in QEMU too.  Or at least it should be possible to make them
different. :)

I guess it's up to you whether the default should be qemu_uuid or no
support for the get UUID command.

Thanks,

Paolo