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

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

[minyard]

I believe we are not in softfreeze yet, and this is the only real
fix I have for IPMI at the moment.

This was posted Nov 2018 with little commentary.

The following changes since commit 474f3938d79ab36b9231c9ad3b5a9314c2aeacde:

  Merge remote-tracking branch 'remotes/amarkovic/tags/mips-queue-jun-21-2019' into staging (2019-06-21 15:40:50 +0100)

are available in the Git repository at:

  git://github.com/cminyard/qemu.git tags/ipmi-for-release-20190627

for you to fetch changes up to bddef5881d0c935a5d9d8e15f822d9d700666ae6:

  ipmi: Add a UUID device property (2019-06-26 15:31:33 -0500)

----------------------------------------------------------------
Add a UUID device property to IPMI

This is fairly important for two reasons:

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

----------------------------------------------------------------
Corey Minyard (2):
      qdev: Add a no default uuid property
      ipmi: Add a UUID device property

 hw/ipmi/ipmi_bmc_sim.c       | 22 ++++++++++++++--------
 include/hw/qdev-properties.h |  7 +++++++
 qemu-options.hx              | 10 +++++++---
 3 files changed, 28 insertions(+), 11 deletions(-)

[Qemu-devel] [PULL 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

[Qemu-devel] [PULL 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] [PULL 0/2] Add a UUID device property to IPMI

[Peter Maydell]

On Thu, 27 Jun 2019 at 13:48, <minyard@acm.org> wrote:
>
> I believe we are not in softfreeze yet, and this is the only real
> fix I have for IPMI at the moment.
>
> This was posted Nov 2018 with little commentary.
>
> The following changes since commit 474f3938d79ab36b9231c9ad3b5a9314c2aeacde:
>
>   Merge remote-tracking branch 'remotes/amarkovic/tags/mips-queue-jun-21-2019' into staging (2019-06-21 15:40:50 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/cminyard/qemu.git tags/ipmi-for-release-20190627
>
> for you to fetch changes up to bddef5881d0c935a5d9d8e15f822d9d700666ae6:
>
>   ipmi: Add a UUID device property (2019-06-26 15:31:33 -0500)
>
> ----------------------------------------------------------------
> Add a UUID device property to IPMI
>
> This is fairly important for two reasons:
>
> * 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.
>
> ----------------------------------------------------------------
> Corey Minyard (2):
>       qdev: Add a no default uuid property
>       ipmi: Add a UUID device property

I have to say I'm not entirely happy with applying a pullreq
with patches that are unreviewed and were last posted on list
over six months ago. Can you post a v2 to try to solicit code
review for them before we put them into master, please?

(Sometimes patches don't get review, and we generally take
them anyway; I do that myself from time to time. It's the
combination of the six-months-since-patches-posted plus the
imminent freeze deadline that gives me pause in this case.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/2] Add a UUID device property to IPMI

[Corey Minyard]

On Mon, Jul 01, 2019 at 07:10:50PM +0100, Peter Maydell wrote:
> On Thu, 27 Jun 2019 at 13:48, <minyard@acm.org> wrote:
> >
> > I believe we are not in softfreeze yet, and this is the only real
> > fix I have for IPMI at the moment.
> >
> > This was posted Nov 2018 with little commentary.
> >
> > The following changes since commit 474f3938d79ab36b9231c9ad3b5a9314c2aeacde:
> >
> >   Merge remote-tracking branch 'remotes/amarkovic/tags/mips-queue-jun-21-2019' into staging (2019-06-21 15:40:50 +0100)
> >
> > are available in the Git repository at:
> >
> >   git://github.com/cminyard/qemu.git tags/ipmi-for-release-20190627
> >
> > for you to fetch changes up to bddef5881d0c935a5d9d8e15f822d9d700666ae6:
> >
> >   ipmi: Add a UUID device property (2019-06-26 15:31:33 -0500)
> >
> > ----------------------------------------------------------------
> > Add a UUID device property to IPMI
> >
> > This is fairly important for two reasons:
> >
> > * 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.
> >
> > ----------------------------------------------------------------
> > Corey Minyard (2):
> >       qdev: Add a no default uuid property
> >       ipmi: Add a UUID device property
> 
> I have to say I'm not entirely happy with applying a pullreq
> with patches that are unreviewed and were last posted on list
> over six months ago. Can you post a v2 to try to solicit code
> review for them before we put them into master, please?
> 
> (Sometimes patches don't get review, and we generally take
> them anyway; I do that myself from time to time. It's the
> combination of the six-months-since-patches-posted plus the
> imminent freeze deadline that gives me pause in this case.)

Will do.

I looked around and tried to find the freeze dates, and I couldn't
find anything published.  If I had known it was close, I would have
waited.

-corey

Re: [Qemu-devel] [PULL 0/2] Add a UUID device property to IPMI

[Peter Maydell]

On Mon, 1 Jul 2019 at 19:25, Corey Minyard <minyard@acm.org> wrote:
>
> On Mon, Jul 01, 2019 at 07:10:50PM +0100, Peter Maydell wrote:
> > I have to say I'm not entirely happy with applying a pullreq
> > with patches that are unreviewed and were last posted on list
> > over six months ago. Can you post a v2 to try to solicit code
> > review for them before we put them into master, please?
> >
> > (Sometimes patches don't get review, and we generally take
> > them anyway; I do that myself from time to time. It's the
> > combination of the six-months-since-patches-posted plus the
> > imminent freeze deadline that gives me pause in this case.)
>
> Will do.
>
> I looked around and tried to find the freeze dates, and I couldn't
> find anything published.  If I had known it was close, I would have
> waited.

Thanks. (Our schedule is on the wiki at
https://wiki.qemu.org/Planning/4.1 -- but we don't publicise
that url very much so it's easy to overlook it.)

This is a pretty small change so if it gets review we can
probably fit it in before rc0 or rc1.

-- PMM