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

[Philippe Mathieu-Daudé]

On 7/1/19 8:31 PM, 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.
> 
> 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 <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;
> +        }
> +    }

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é <address@hidden>