Thanks for the comments! I've removed IPMI part from the patch sets. I'll send a separate patch sets once the refactor is done. I'll also include Havard's documentation in it.
I haven't thought of a better name. We can update the name accordingly. On Fri, Dec 11, 2020 at 4:26 PM Havard Skinnemoen <hskinnem...@google.com> wrote: > On Fri, Dec 11, 2020 at 4:16 PM Corey Minyard <miny...@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. > > 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. > > 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 <miny...@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 >> > > > >> > > >> >
