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

[Corey Minyard <minyard@acm.org>]

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.

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
> > >
> >