| Message ID | 20230603233612.125879-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ppc/pnv: Add chiptod and core timebase state machine models | expand |
On 6/4/23 01:36, Nicholas Piggin wrote: > This adds support for chiptod and core timebase state machine models in > the powernv POWER9 and POWER10 models. > > This does not actually change the time or the value in TB registers > (because they are alrady synced in QEMU), but it does go through the > motions. It is enough to be able to run skiboot's chiptod initialisation > code that synchronises core timebases (after a patch to prevent skiboot > skipping chiptod for QEMU, posted to skiboot mailing list). > > Sorry there was some delay since the last posting. There is a bit more > interest in this recently but feedback and comments from RFC was not > forgotten and is much appreciated. > > https://lists.gnu.org/archive/html/qemu-ppc/2022-08/msg00324.html > > I think I accounted for everything except moving register defines to the > .h file. I'm on the fence about that but if they are only used in the .c > file I think it's okay to keep them there for now. I cut out a lot of > unused ones so it's not so cluttered now. > > Lots of other changes and fixes since that RFC. Notably: > - Register names changed to match the workbook names instead of skiboot. > - TFMR moved to timebase_helper.c from misc_helper.c > - More comprehensive model and error checking, particularly of TFMR. > - POWER10 with multi-chip support. > - chiptod and core timebase linked via specific state instead of TFMR. The chiptod units are not exposed to the OS, it is all handled at FW level AFAIK. Could the OPAL people provide some feedback on the low level models ? Thanks, C. > There is still a vast amount that is not modeled, but most of it related > to error handling, injection, failover, etc that is very complicated and > not required for normal operation. > > Thanks, > Nick > > Nicholas Piggin (4): > pnv/chiptod: Add POWER9/10 chiptod model > target/ppc: Tidy POWER book4 SPR registration > target/ppc: add TFMR SPR implementation with read and write helpers > target/ppc: Implement core timebase state machine and TFMR > > hw/ppc/meson.build | 1 + > hw/ppc/pnv.c | 38 +++ > hw/ppc/pnv_chiptod.c | 488 +++++++++++++++++++++++++++++++++++ > hw/ppc/pnv_xscom.c | 2 + > hw/ppc/trace-events | 4 + > include/hw/ppc/pnv_chip.h | 3 + > include/hw/ppc/pnv_chiptod.h | 64 +++++ > include/hw/ppc/pnv_core.h | 3 + > include/hw/ppc/pnv_xscom.h | 9 + > target/ppc/cpu.h | 40 +++ > target/ppc/cpu_init.c | 92 ++++--- > target/ppc/helper.h | 2 + > target/ppc/spr_common.h | 2 + > target/ppc/timebase_helper.c | 156 +++++++++++ > target/ppc/translate.c | 10 + > 15 files changed, 882 insertions(+), 32 deletions(-) > create mode 100644 hw/ppc/pnv_chiptod.c > create mode 100644 include/hw/ppc/pnv_chiptod.h >
On Tue Jun 6, 2023 at 11:59 PM AEST, Cédric Le Goater wrote: > On 6/4/23 01:36, Nicholas Piggin wrote: > > This adds support for chiptod and core timebase state machine models in > > the powernv POWER9 and POWER10 models. > > > > This does not actually change the time or the value in TB registers > > (because they are alrady synced in QEMU), but it does go through the > > motions. It is enough to be able to run skiboot's chiptod initialisation > > code that synchronises core timebases (after a patch to prevent skiboot > > skipping chiptod for QEMU, posted to skiboot mailing list). > > > > Sorry there was some delay since the last posting. There is a bit more > > interest in this recently but feedback and comments from RFC was not > > forgotten and is much appreciated. > > > > https://lists.gnu.org/archive/html/qemu-ppc/2022-08/msg00324.html > > > > I think I accounted for everything except moving register defines to the > > .h file. I'm on the fence about that but if they are only used in the .c > > file I think it's okay to keep them there for now. I cut out a lot of > > unused ones so it's not so cluttered now. > > > > Lots of other changes and fixes since that RFC. Notably: > > - Register names changed to match the workbook names instead of skiboot. > > - TFMR moved to timebase_helper.c from misc_helper.c > > - More comprehensive model and error checking, particularly of TFMR. > > - POWER10 with multi-chip support. > > - chiptod and core timebase linked via specific state instead of TFMR. > > > The chiptod units are not exposed to the OS, it is all handled at FW > level AFAIK. Could the OPAL people provide some feedback on the low level > models ? Well, no takers so far. I guess I'm a OPAL people :) I did some of the P10 chiptod addressing in skiboot, at least. This patch does work with the skiboot chiptod driver at least. I would eventually like to add in the ability to actually change the TB with it, and inject errors and generate HMIs because that's an area that seem to be a bit lacking (most of such testing seemed to be done on real hardware using special time facility corruption injection). But yes for now it is a bit difficult to verify it does much useful aside from booting skiboot (+ patch to enable chiptod on QEMU I posted recently). Thanks, Nick
On 6/14/23 07:14, Nicholas Piggin wrote: > On Tue Jun 6, 2023 at 11:59 PM AEST, Cédric Le Goater wrote: >> On 6/4/23 01:36, Nicholas Piggin wrote: >>> This adds support for chiptod and core timebase state machine models in >>> the powernv POWER9 and POWER10 models. >>> >>> This does not actually change the time or the value in TB registers >>> (because they are alrady synced in QEMU), but it does go through the >>> motions. It is enough to be able to run skiboot's chiptod initialisation >>> code that synchronises core timebases (after a patch to prevent skiboot >>> skipping chiptod for QEMU, posted to skiboot mailing list). >>> >>> Sorry there was some delay since the last posting. There is a bit more >>> interest in this recently but feedback and comments from RFC was not >>> forgotten and is much appreciated. >>> >>> https://lists.gnu.org/archive/html/qemu-ppc/2022-08/msg00324.html >>> >>> I think I accounted for everything except moving register defines to the >>> .h file. I'm on the fence about that but if they are only used in the .c >>> file I think it's okay to keep them there for now. I cut out a lot of >>> unused ones so it's not so cluttered now. >>> >>> Lots of other changes and fixes since that RFC. Notably: >>> - Register names changed to match the workbook names instead of skiboot. >>> - TFMR moved to timebase_helper.c from misc_helper.c >>> - More comprehensive model and error checking, particularly of TFMR. >>> - POWER10 with multi-chip support. >>> - chiptod and core timebase linked via specific state instead of TFMR. >> >> >> The chiptod units are not exposed to the OS, it is all handled at FW >> level AFAIK. Could the OPAL people provide some feedback on the low level >> models ? > > Well, no takers so far. I guess I'm a OPAL people :) >> I did some of the P10 chiptod addressing in skiboot, at least. This > patch does work with the skiboot chiptod driver at least. cool, with 2 chips ? > I would eventually like to add in the ability to actually change the > TB with it, and inject errors and generate HMIs because that's an area > that seem to be a bit lacking (most of such testing seemed to be done > on real hardware using special time facility corruption injection). The MCE injection was a nice addon https://lore.kernel.org/qemu-devel/20200325144147.221875-1-npiggin@gmail.com/ https://lore.kernel.org/qemu-devel/20211013214042.618918-1-clg@kaod.org/ I hope it will get more attention if you resend. > But yes for now it is a bit difficult to verify it does much useful > aside from booting skiboot (+ patch to enable chiptod on QEMU I posted > recently). It's difficult to review PATCH 4 without some good knowledge of HW. I know you do but you can not review your own patches ! That said, the impact is limited to PowerNV machines, I guess we are fine. Thanks, C.
On Wed Jun 14, 2023 at 6:54 PM AEST, Cédric Le Goater wrote: > On 6/14/23 07:14, Nicholas Piggin wrote: > > On Tue Jun 6, 2023 at 11:59 PM AEST, Cédric Le Goater wrote: > >> On 6/4/23 01:36, Nicholas Piggin wrote: > >>> This adds support for chiptod and core timebase state machine models in > >>> the powernv POWER9 and POWER10 models. > >>> > >>> This does not actually change the time or the value in TB registers > >>> (because they are alrady synced in QEMU), but it does go through the > >>> motions. It is enough to be able to run skiboot's chiptod initialisation > >>> code that synchronises core timebases (after a patch to prevent skiboot > >>> skipping chiptod for QEMU, posted to skiboot mailing list). > >>> > >>> Sorry there was some delay since the last posting. There is a bit more > >>> interest in this recently but feedback and comments from RFC was not > >>> forgotten and is much appreciated. > >>> > >>> https://lists.gnu.org/archive/html/qemu-ppc/2022-08/msg00324.html > >>> > >>> I think I accounted for everything except moving register defines to the > >>> .h file. I'm on the fence about that but if they are only used in the .c > >>> file I think it's okay to keep them there for now. I cut out a lot of > >>> unused ones so it's not so cluttered now. > >>> > >>> Lots of other changes and fixes since that RFC. Notably: > >>> - Register names changed to match the workbook names instead of skiboot. > >>> - TFMR moved to timebase_helper.c from misc_helper.c > >>> - More comprehensive model and error checking, particularly of TFMR. > >>> - POWER10 with multi-chip support. > >>> - chiptod and core timebase linked via specific state instead of TFMR. > >> > >> > >> The chiptod units are not exposed to the OS, it is all handled at FW > >> level AFAIK. Could the OPAL people provide some feedback on the low level > >> models ? > > > > Well, no takers so far. I guess I'm a OPAL people :) > >> I did some of the P10 chiptod addressing in skiboot, at least. This > > patch does work with the skiboot chiptod driver at least. > > cool, with 2 chips ? Yes it worked with 2 chips. > > I would eventually like to add in the ability to actually change the > > TB with it, and inject errors and generate HMIs because that's an area > > that seem to be a bit lacking (most of such testing seemed to be done > > on real hardware using special time facility corruption injection). > > The MCE injection was a nice addon > > https://lore.kernel.org/qemu-devel/20200325144147.221875-1-npiggin@gmail.com/ > https://lore.kernel.org/qemu-devel/20211013214042.618918-1-clg@kaod.org/ > > I hope it will get more attention if you resend. Will take another look. > > But yes for now it is a bit difficult to verify it does much useful > > aside from booting skiboot (+ patch to enable chiptod on QEMU I posted > > recently). > > It's difficult to review PATCH 4 without some good knowledge of HW. I know > you do but you can not review your own patches ! That said, the impact is > limited to PowerNV machines, I guess we are fine. Yeah. I appreciate all the review so far. It's pretty complicated even with the workbook. I might be able to add a simpler and higher-level description of the basic init sequence and states. You would still have to trust me, but it might make it easier to see what's going on. Thanks, Nick
[ ... ] > Yes it worked with 2 chips. I will give the next series a try. [ ... ] >> It's difficult to review PATCH 4 without some good knowledge of HW. I know >> you do but you can not review your own patches ! That said, the impact is >> limited to PowerNV machines, I guess we are fine. > > Yeah. I appreciate all the review so far. It's pretty complicated even > with the workbook. I might be able to add a simpler and higher-level > description of the basic init sequence and states. You would still have > to trust me, but it might make it easier to see what's going on. Sure. Patches 2-4 are acked. I am only looking at patch 1 now. Thanks, C.
On 6/4/23 01:36, Nicholas Piggin wrote: > This adds support for chiptod and core timebase state machine models in > the powernv POWER9 and POWER10 models. > > This does not actually change the time or the value in TB registers > (because they are alrady synced in QEMU), but it does go through the > motions. It is enough to be able to run skiboot's chiptod initialisation > code that synchronises core timebases (after a patch to prevent skiboot > skipping chiptod for QEMU, posted to skiboot mailing list). > > Sorry there was some delay since the last posting. There is a bit more > interest in this recently but feedback and comments from RFC was not > forgotten and is much appreciated. > > https://lists.gnu.org/archive/html/qemu-ppc/2022-08/msg00324.html > > I think I accounted for everything except moving register defines to the > .h file. I'm on the fence about that but if they are only used in the .c > file I think it's okay to keep them there for now. I cut out a lot of > unused ones so it's not so cluttered now. > > Lots of other changes and fixes since that RFC. Notably: > - Register names changed to match the workbook names instead of skiboot. > - TFMR moved to timebase_helper.c from misc_helper.c > - More comprehensive model and error checking, particularly of TFMR. > - POWER10 with multi-chip support. > - chiptod and core timebase linked via specific state instead of TFMR. > > There is still a vast amount that is not modeled, but most of it related > to error handling, injection, failover, etc that is very complicated and > not required for normal operation. > > Thanks, > Nick > > Nicholas Piggin (4): > pnv/chiptod: Add POWER9/10 chiptod model > target/ppc: Tidy POWER book4 SPR registration > target/ppc: add TFMR SPR implementation with read and write helpers > target/ppc: Implement core timebase state machine and TFMR patch 2-4 could be merged in the next PR. Could you please rebase on ppc-next and resend ? Then we still have 2+ weeks to polish pnv/chiptod which would be a nice addition to QEMU 8.1. Thanks, C. > > hw/ppc/meson.build | 1 + > hw/ppc/pnv.c | 38 +++ > hw/ppc/pnv_chiptod.c | 488 +++++++++++++++++++++++++++++++++++ > hw/ppc/pnv_xscom.c | 2 + > hw/ppc/trace-events | 4 + > include/hw/ppc/pnv_chip.h | 3 + > include/hw/ppc/pnv_chiptod.h | 64 +++++ > include/hw/ppc/pnv_core.h | 3 + > include/hw/ppc/pnv_xscom.h | 9 + > target/ppc/cpu.h | 40 +++ > target/ppc/cpu_init.c | 92 ++++--- > target/ppc/helper.h | 2 + > target/ppc/spr_common.h | 2 + > target/ppc/timebase_helper.c | 156 +++++++++++ > target/ppc/translate.c | 10 + > 15 files changed, 882 insertions(+), 32 deletions(-) > create mode 100644 hw/ppc/pnv_chiptod.c > create mode 100644 include/hw/ppc/pnv_chiptod.h >
On Thu Jun 22, 2023 at 5:30 PM AEST, Cédric Le Goater wrote: > On 6/4/23 01:36, Nicholas Piggin wrote: > > This adds support for chiptod and core timebase state machine models in > > the powernv POWER9 and POWER10 models. > > > > This does not actually change the time or the value in TB registers > > (because they are alrady synced in QEMU), but it does go through the > > motions. It is enough to be able to run skiboot's chiptod initialisation > > code that synchronises core timebases (after a patch to prevent skiboot > > skipping chiptod for QEMU, posted to skiboot mailing list). > > > > Sorry there was some delay since the last posting. There is a bit more > > interest in this recently but feedback and comments from RFC was not > > forgotten and is much appreciated. > > > > https://lists.gnu.org/archive/html/qemu-ppc/2022-08/msg00324.html > > > > I think I accounted for everything except moving register defines to the > > .h file. I'm on the fence about that but if they are only used in the .c > > file I think it's okay to keep them there for now. I cut out a lot of > > unused ones so it's not so cluttered now. > > > > Lots of other changes and fixes since that RFC. Notably: > > - Register names changed to match the workbook names instead of skiboot. > > - TFMR moved to timebase_helper.c from misc_helper.c > > - More comprehensive model and error checking, particularly of TFMR. > > - POWER10 with multi-chip support. > > - chiptod and core timebase linked via specific state instead of TFMR. > > > > There is still a vast amount that is not modeled, but most of it related > > to error handling, injection, failover, etc that is very complicated and > > not required for normal operation. > > > > Thanks, > > Nick > > > > Nicholas Piggin (4): > > pnv/chiptod: Add POWER9/10 chiptod model > > target/ppc: Tidy POWER book4 SPR registration > > target/ppc: add TFMR SPR implementation with read and write helpers > > target/ppc: Implement core timebase state machine and TFMR > > patch 2-4 could be merged in the next PR. Could you please rebase on > ppc-next and resend ? Good idea, I have a couple of other minor register additions that depend on patch 1 too. > Then we still have 2+ weeks to polish pnv/chiptod which would be a > nice addition to QEMU 8.1. Yeah. Been trying to get to it... Hopefully pseries SMT is close now so I will have some more time. Thanks, Nick