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