On Mon, Jul 01, 2019 at 01:31:00PM -0500, miny...@acm.org wrote:
> From: Corey Minyard <cminy...@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 <cminy...@mvista.com>
> Cc: Cédric Le Goater <c...@kaod.org>
> Cc: David Gibson <da...@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <m...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@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 :|
