Re: [PATCH 3/3] ipmi/bmc-sim: Add 'Get Channel Info' command

[Corey Minyard <corey@minyard.net>]

On Tue, Apr 01, 2025 at 09:42:01AM +1000, Nicholas Piggin wrote:
> On Mon Mar 31, 2025 at 11:25 PM AEST, Corey Minyard wrote:
> > On Mon, Mar 31, 2025 at 10:57:24PM +1000, Nicholas Piggin wrote:
> >> +static void get_channel_info(IPMIBmcSim *ibs,
> >> +                             uint8_t *cmd, unsigned int cmd_len,
> >> +                             RspBuffer *rsp)
> >> +{
> >> +    IPMIInterface *s = ibs->parent.intf;
> >> +    IPMIInterfaceClass *k = IPMI_INTERFACE_GET_CLASS(s);
> >> +    uint8_t ch = cmd[1] & 0x0f;
> >> +
> >> +    /* Only define channel 0h (IPMB) and Fh (system interface) */
> >> +
> >> +    if (ch == 0x0e) { /* "This channel" */
> >> +        ch = IPMI_CHANNEL_SYSTEM;
> >> +    }
> >> +    rsp_buffer_push(rsp, ch);
> >> +
> >> +    if (ch != IPMI_CHANNEL_IPMB && ch != IPMI_CHANNEL_SYSTEM) {
> >> +        /* Not supported */
> >
> > I think that an all zero response is a valid response.  I think you
> > should return a IPMI_CC_INVALID_DATA_FIELD instead, right?
> 
> I can't remember, I dug the patch out from a while ago. I can't actually
> see anywhere it is made clear in the spec, do you? I agree an invalid
> error sounds better. I'll try to see how ipmi tools handles it.

I'm fairly sure an error response is ok.  Please pursue that.

> 
> >> +        int i;
> >> +        for (i = 0; i < 8; i++) {
> >> +            rsp_buffer_push(rsp, 0x00);
> >> +        }
> >> +        return;
> >> +    }
> >> +
> >> +    if (ch == IPMI_CHANNEL_IPMB) {
> >> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_IPMB);
> >> +        rsp_buffer_push(rsp, IPMI_CH_PROTOCOL_IPMB);
> >> +    } else { /* IPMI_CHANNEL_SYSTEM */
> >> +        rsp_buffer_push(rsp, IPMI_CH_MEDIUM_SYSTEM);
> >> +        rsp_buffer_push(rsp, k->protocol);
> >> +    }
> >> +
> >> +    rsp_buffer_push(rsp, 0x00); /* Session-less */
> >> +
> >> +    /* IPMI Vendor ID */
> >> +    rsp_buffer_push(rsp, 0xf2);
> >> +    rsp_buffer_push(rsp, 0x1b);
> >> +    rsp_buffer_push(rsp, 0x00);
> >
> > Where does this come from?
> 
> IPMI spec Get Channel Info Command, search "IPMI Enterprise Number"
> From my reading, all channel info responses contain this.

Oh, sorry, I should have read on this.  This is fine.

> 
> >> +
> >> +    if (ch == IPMI_CHANNEL_SYSTEM) {
> >> +        /* IRQ assigned by ACPI/PnP (XXX?) */
> >> +        rsp_buffer_push(rsp, 0x60);
> >> +        rsp_buffer_push(rsp, 0x60);
> >
> > The interrupt should be available.  For the isa versions there is a
> > get_fwinfo function pointer that you can fetch this with.  For PCI it's
> > more complicated, unfortunately.
> 
> These are for the two interrupts. QEMU seems to tie both to the
> same line, I guess that's okay?

Yes, they are the same.

> 
> That interface looks good, but what I was concerned about is whether
> that implies the irq is hard coded or whether the platform can assign
> it, does it mean unassigned? I don't know a lot about irq routing or
> what IPMI clients would use it for.

For isa-based devices, it's hard-coded by the configuration.  That one
should be easy to get.

For PCI, I'm not so sure.  It would take some research to figure it out.

> 
> Anyhow I'll respin with changes.

Thanks,

-corey

> 
> Thanks,
> Nick