| Message ID | 20201211015156.2053372-1-wuhaotsh@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Additional NPCM7xx devices and IPMI BMC emulation support | expand |
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 >
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.
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
> >
>
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 > > > > >
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. 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 <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 > > > > > > > >
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 <hskinnemoen@google.com> 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. > > 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 <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 >> > > > >> > > >> >
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 > > > > > > > > > > >