Re: [PATCH 1/2] hw/ipmi/ipmi_bmc_sim: Support getting fake LAN channel config

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Yunpeng,

On 28/10/25 19:01, Yunpeng Yang wrote:
> The following IPMI commands are added or modified to support getting
> fake LAN channel configurations from the `ipmi_bmc_sim` device:
> * Get Channel Access;
> * Get Channel Info Command;
> * Get LAN Configuration Parameters.
> 
> The fake LAN channel configurations can be specified from QEMU
> commandline options for device `ipmi_bmc_sim`. Inside the guest OS, the
> configurations can be retrieved from the device using some IPMI tools,
> e.g., `ipmitool lan print`.
> Note, there is not a real LAN channel. The fake LAN channel is suitable
> for testing purposes.
> 
> Signed-off-by: Yunpeng Yang <yunpeng.yang@nutanix.com>
> ---
>   hw/ipmi/ipmi_bmc_sim.c      | 297 ++++++++++++++++++++++++++++++++++--
>   include/hw/ipmi/ipmi.h      |   1 +
>   qemu-options.hx             |  26 ++++
>   tests/qtest/ipmi-kcs-test.c |  60 ++++++++
>   4 files changed, 374 insertions(+), 10 deletions(-)


>   /* Same as a timespec struct. */
>   struct ipmi_time {
> @@ -170,6 +177,23 @@ typedef struct IPMISensor {
>   #define MAX_SENSORS 20
>   #define IPMI_WATCHDOG_SENSOR 0
>   
> +#define NBYTES_IP  4
> +#define NBYTES_MAC 6
> +
> +typedef struct IPMILan {
> +    uint8_t channel;
> +    uint8_t ipaddr[NBYTES_IP];
> +    uint8_t ipsrc;
> +    MACAddr macaddr;
> +    uint8_t netmask[NBYTES_IP];
> +    uint8_t defgw_ipaddr[NBYTES_IP];
> +    MACAddr defgw_macaddr;
> +
> +    char *arg_ipaddr;
> +    char *arg_netmask;
> +    char *arg_defgw_ipaddr;
> +} IPMILan;
> +
>   #define MAX_NETFNS 64
>   
>   typedef struct IPMIRcvBufEntry {
> @@ -215,6 +239,7 @@ struct IPMIBmcSim {
>       IPMIFru fru;
>       IPMISensor sensors[MAX_SENSORS];
>       char *sdr_filename;
> +    IPMILan lan;
>   

[...]

> +static inline bool is_valid_netmask(const uint8_t *netmask)
> +{
> +    uint32_t mask = netmask[3];
> +    uint32_t inverted;
> +
> +    mask |= (uint32_t) netmask[2] << 8;
> +    mask |= (uint32_t) netmask[1] << 16;
> +    mask |= (uint32_t) netmask[0] << 24;

Why manually byteswap?

I'd expect such helper already available in include/net/ somewhere,
otherwise what about:

   static bool is_ipv4_netmask_valid(const void *buf)
   {
       uint32_t netmask = ldl_be_p(buf);

       return clo32(netmask) + ctz32(netmask) == 32;
   }

> +    inverted = ~mask;
> +    return mask != 0 && (inverted & (inverted + 1)) == 0;
> +}


> @@ -2176,7 +2395,7 @@ static void ipmi_sdr_init(IPMIBmcSim *ibs)
>   
>   static const VMStateDescription vmstate_ipmi_sim = {
>       .name = TYPE_IPMI_BMC_SIMULATOR,
> -    .version_id = 1,
> +    .version_id = 2,
>       .minimum_version_id = 1,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINT8(bmc_global_enables, IPMIBmcSim),
> @@ -2198,6 +2417,13 @@ static const VMStateDescription vmstate_ipmi_sim = {
>           VMSTATE_UINT16(sensors[IPMI_WATCHDOG_SENSOR].deassert_states,
>                          IPMIBmcSim),
>           VMSTATE_UINT16(sensors[IPMI_WATCHDOG_SENSOR].assert_enable, IPMIBmcSim),
> +        VMSTATE_UINT8_V(lan.channel, IPMIBmcSim, 2),
> +        VMSTATE_UINT8_ARRAY_V(lan.ipaddr, IPMIBmcSim, NBYTES_IP, 2),
> +        VMSTATE_UINT8_V(lan.ipsrc, IPMIBmcSim, 2),
> +        VMSTATE_UINT8_ARRAY_V(lan.macaddr.a, IPMIBmcSim, NBYTES_MAC, 2),
> +        VMSTATE_UINT8_ARRAY_V(lan.netmask, IPMIBmcSim, NBYTES_IP, 2),
> +        VMSTATE_UINT8_ARRAY_V(lan.defgw_ipaddr, IPMIBmcSim, NBYTES_IP, 2),
> +        VMSTATE_UINT8_ARRAY_V(lan.defgw_macaddr.a, IPMIBmcSim, NBYTES_MAC, 2),

Safer would be to add a VMStateDescription for the new IPMILan
structure and link it as subsections, so other code using could just
link as subsection, not duplicating the VMSTATE_FOO() macros.

>           VMSTATE_END_OF_LIST()
>       }
>   };

Regards,

Phil.