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, address@hidden wrote:
> From: Corey Minyard <address@hidden>
> 
> 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 <address@hidden>
> Cc: Cédric Le Goater <address@hidden>
> Cc: David Gibson <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> ---
>  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 :|