On Fri, Dec 11, 2020 at 04:26:03PM -0800, Havard Skinnemoen 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.
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 <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 > > > > > > > > > > >
