Re: [PATCH 0/7] Additional NPCM7xx devices and IPMI BMC emulation support

[Corey Minyard <minyard@acm.org>]

On Fri, Dec 11, 2020 at 04:26:03PM -0800, Havard Skinnemoen wrote:
> On Fri, Dec 11, 2020 at 4:16 PM Corey Minyard <minyard@acm.org> wrote:
> 
> > On Fri, Dec 11, 2020 at 12:56:07PM -0800, Hao Wu wrote:
> > > Tl,dr: We'll remove the IPMI changes from the current patch set and
> > > refactor
> > >           them in a separate patch set.
> > >
> > > Thank you for your review! On high level, we are trying to emulate the
> > BMC
> > > side of the IPMI protocol. So we cannot directly use the existing IPMI
> > code.
> > > However, they do have a lot in duplication as you pointed out. So we'll
> > > refactor
> > > the existing IPMI code and update in a way that we only add the required
> > > functionality.
> >
> > Ah, I didn't figure that out from what you posted.  So the idea is you
> > can create the BMC side of the system in one qemu session with your
> > changes and then you connect it to a host system running qemu with the
> > host side of the interface.
> >
> > The wire protocol is basically symmetric, but the command handling will
> > need to be separate.  So you probably want to split out the base
> > protocol from ipmi_bmc_extern into its own file and use that from your
> > own file, to avoid the duplication.
> >
> > You need to do proper ATTN handling on the BMC side.  And you will also
> > need ties into GPIOs and whatnot for doing the reset, NMI, etc.
> >
> > "ipmi_host" is probably not the best name.  At least to me that implied
> > the host side of the interface.  I'm not coming up with something I
> > really like, though.  Maybe "bmc_host"?  That's more descriptive, though
> > I'm sure a better name exists.  Then you could have "bmc_host_extern"
> > for the protocol.  If you come up with a better naming scheme, the
> > existing files can be renamed, too.
> >
> 
> The naming is my fault.
> 
> My thinking was that ipmi-host-extern is to the BMC what ipmi-bmc-extern is
> to the host. That is, the former represents the host as seen by the BMC,
> and the latter represents the BMC as seen by the host.

Yeah, I kind of figured that, which is why I suggested that the existing
files can be renamed.  I really like it when I can look at a directory
and tell what everything does.  Unfortunately, that's generally harder
to achieve than it sounds.

We could also create a separate bmc directory under hw.  Then the names
you have make more sense.  It would also make things a bit cleaner,
since I wouldn't really be maintaining the BMC side of things.  I'm
already dealing with that on the Linux side.

> 
> I sent some docs to the list earlier this year, but sadly, I never got
> around to follow up. You can see the generated docs here:
> 
> https://hskinnemoen.github.io/qemu/specs/ipmi.html
> 
> Hao, perhaps you should include my documentation patches in your next IPMI
> series? If we come up with a better naming scheme for both sides, I can
> update the docs accordingly.

I remember that now.  Yes, documentation would be good.

Thanks,

-corey

> 
> Havard
> 
> 
> > Thanks,
> >
> > -corey
> >
> > >
> > > As for the KCS module, the BMC side of the protocol is the opposite
> > > direction
> > > of the existing ipmi_kcs.c code which is on the host/CPU side. For
> > example,
> > > in READ_STATE the CPU would read data while the BMC would write data.
> > > So we can't directly use the same implementation. (They're different
> > files
> > > in
> > > Linux either.) However, we can refactor it to re-use some of the common
> > > definitions.
> > >
> > > We would like to remove the IPMI and KCS stuff from the current patch
> > set.
> > > We'll send the refactored code in a separate patch set after addressing
> > > your concerns.
> > >
> > > Thanks again for the review!
> > >
> > > On Thu, Dec 10, 2020 at 7:04 PM Corey Minyard <minyard@acm.org> wrote:
> > >
> > > > On Thu, Dec 10, 2020 at 05:51:49PM -0800, Hao Wu wrote:
> > > > > This patch series include a few more NPCM7XX devices including
> > > > >
> > > > > - Analog Digital Converter (ADC)
> > > > > - Pulse Width Modulation (PWM)
> > > > > - Keyboard Style Controller (KSC)
> > > > >
> > > > > To utilize these modules we also add two extra functionalities:
> > > > >
> > > > > 1. We modified the CLK module to generate clock values using
> > qdev_clock.
> > > > >    These clocks are used to determine various clocks in NPCM7XX
> > devices.
> > > > > 2. We added support for emulating IPMI responder devices in BMC
> > machines,
> > > > >    similar to the existing IPMI device support for CPU emulation.
> > This
> > > > allows
> > > > >    a qemu instance running BMC firmware to serve as an external BMC
> > for
> > > > a qemu
> > > > >    instance running server software. It utilizes the KCS module we
> > > > implemented.
> > > >
> > > > Looking at the IPMI changes, why didn't you just re-use the existing
> > > > IPMI infrastructure?  ipmi_host.[ch] is basically a subset of
> > ipmi.[ch],
> > > > and the ipmi_host_extern looks like a copy of of ipmi_bmc_extern with
> > > > some names changed.  That kind of code duplication is not acceptable.
> > > > Plus you copied my code and removed my copyrights, which is really
> > > > not acceptable and illegal.
> > > >
> > > > I'm not exactly sure why you needed you own KCS interface, either.  It
> > > > looks like the interface is somewhat different in some ways, but
> > > > integrating it into the current KCS code is probably a better choice if
> > > > that can be done.
> > > >
> > > > -corey
> > > >
> > > > >
> > > > > Hao Wu (7):
> > > > >   hw/misc: Add clock converter in NPCM7XX CLK module
> > > > >   hw/timer: Refactor NPCM7XX Timer to use CLK clock
> > > > >   hw/adc: Add an ADC module for NPCM7XX
> > > > >   hw/misc: Add a PWM module for NPCM7XX
> > > > >   hw/ipmi: Add an IPMI host interface
> > > > >   hw/ipmi: Add a KCS Module for NPCM7XX
> > > > >   hw/ipmi: Add an IPMI external host device
> > > > >
> > > > >  default-configs/devices/arm-softmmu.mak |   2 +
> > > > >  docs/system/arm/nuvoton.rst             |   6 +-
> > > > >  hw/adc/meson.build                      |   1 +
> > > > >  hw/adc/npcm7xx_adc.c                    | 318 ++++++++++
> > > > >  hw/arm/npcm7xx.c                        |  65 +-
> > > > >  hw/ipmi/Kconfig                         |   5 +
> > > > >  hw/ipmi/ipmi_host.c                     |  40 ++
> > > > >  hw/ipmi/ipmi_host_extern.c              | 435 +++++++++++++
> > > > >  hw/ipmi/meson.build                     |   3 +
> > > > >  hw/ipmi/npcm7xx_kcs.c                   | 570 +++++++++++++++++
> > > > >  hw/misc/meson.build                     |   1 +
> > > > >  hw/misc/npcm7xx_clk.c                   | 795
> > +++++++++++++++++++++++-
> > > > >  hw/misc/npcm7xx_pwm.c                   | 535 ++++++++++++++++
> > > > >  hw/timer/npcm7xx_timer.c                |  25 +-
> > > > >  include/hw/adc/npcm7xx_adc.h            |  72 +++
> > > > >  include/hw/arm/npcm7xx.h                |   6 +
> > > > >  include/hw/ipmi/ipmi_host.h             |  56 ++
> > > > >  include/hw/ipmi/ipmi_responder.h        |  66 ++
> > > > >  include/hw/ipmi/npcm7xx_kcs.h           | 105 ++++
> > > > >  include/hw/misc/npcm7xx_clk.h           | 146 ++++-
> > > > >  include/hw/misc/npcm7xx_pwm.h           | 106 ++++
> > > > >  include/hw/timer/npcm7xx_timer.h        |   1 +
> > > > >  tests/qtest/meson.build                 |   4 +-
> > > > >  tests/qtest/npcm7xx_adc-test.c          | 400 ++++++++++++
> > > > >  tests/qtest/npcm7xx_pwm-test.c          | 490 +++++++++++++++
> > > > >  25 files changed, 4221 insertions(+), 32 deletions(-)
> > > > >  create mode 100644 hw/adc/npcm7xx_adc.c
> > > > >  create mode 100644 hw/ipmi/ipmi_host.c
> > > > >  create mode 100644 hw/ipmi/ipmi_host_extern.c
> > > > >  create mode 100644 hw/ipmi/npcm7xx_kcs.c
> > > > >  create mode 100644 hw/misc/npcm7xx_pwm.c
> > > > >  create mode 100644 include/hw/adc/npcm7xx_adc.h
> > > > >  create mode 100644 include/hw/ipmi/ipmi_host.h
> > > > >  create mode 100644 include/hw/ipmi/ipmi_responder.h
> > > > >  create mode 100644 include/hw/ipmi/npcm7xx_kcs.h
> > > > >  create mode 100644 include/hw/misc/npcm7xx_pwm.h
> > > > >  create mode 100644 tests/qtest/npcm7xx_adc-test.c
> > > > >  create mode 100644 tests/qtest/npcm7xx_pwm-test.c
> > > > >
> > > > > --
> > > > > 2.29.2.684.gfbc64c5ab5-goog
> > > > >
> > > >
> >