Re: [PATCH v2 4/7] hw/ipmi: Refactor IPMI interface

[Corey Minyard]

On Mon, Mar 27, 2023 at 10:11:50AM -0700, Hao Wu wrote:
> Hi, Cedric
> 
> The naming scheme is suggested by Corey in a previous review:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02691.html
> 
> I originally kept "IpmIBmc" for the host side code talking to BMC but it
> might also cause confusion as well. I'm not sure which name is the best
> here. Maybe Corey can shed some light on this one? Thank you!

I agree with Cédric here, Bmc and BmcClient sound more clear than what I
proposed earlier.

-corey

> 
> Best Regards,
> 
> On Mon, Mar 27, 2023 at 5:34 AM Cédric Le Goater <clg@kaod.org> wrote:
> 
> > Hello Hao,
> >
> > On 3/25/23 00:09, Hao Wu wrote:
> > > This patch refactors the IPMI interface so that it can be used by both
> > > the BMC side and core-side simulation.
> > >
> > > Detail changes:
> > > (1) Split IPMIInterface into IPMIInterfaceHost (for host side
> > >      simulation) and IPMIInterfaceClient (for BMC side simulation).
> > > (2) rename handle_rsp -> handle_msg so the name fits both BMC side and
> > >      Core side.
> > > (3) Add a new class IPMICore. This class represents a simulator/external
> > >      connection for both BMC and Core side emulation.
> > > (4) Change the original IPMIBmc to IPMIBmcHost, representing host side
> > >      simulation.
> > > (5) Add a new type IPMIBmcClient representing BMC side simulation.
> > > (6) Appy the changes to  the entire IPMI library.
> >
> > 'IPMIBmcHost' is a BMC object model (internal or external) and
> > 'IPMIBmcClient' is a host object model ?
> >
> > [ ... ]
> >
> > > @@ -267,15 +267,15 @@ void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
> > >    * Instantiate the machine BMC. PowerNV uses the QEMU internal
> > >    * simulator but it could also be external.
> > >    */
> > > -IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
> > > +IPMIBmcHost *pnv_bmc_create(PnvPnor *pnor)
> > >   {
> > >       Object *obj;
> > >
> > >       obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
> > >       qdev_realize(DEVICE(obj), NULL, &error_fatal);
> > > -    pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
> > > +    pnv_bmc_set_pnor(IPMI_BMC_HOST(obj), pnor);
> > >
> > > -    return IPMI_BMC(obj);
> > > +    return IPMI_BMC_HOST(obj);
> >
> > QEMU PowerNV machines model the host side of OpenPOWER systems which
> > have an Aspeed SoC based BMC for management. The routine above creates
> > an Aspeed *BMC* object model for the PowerNV *host* machine. I find
> > 'IPMIBmcHost' confusing. It shouldn't have a 'Host' suffix I think.
> >
> > 'IPMIBmcClient' sounds ok, or 'IPMIBmcPeer' maybe.
> >
> > Thanks,
> >
> > C.
> >
> >