[PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models

Nicholas Piggin posted 4 patches 11 months ago
Failed in applying to current master (apply log)
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
[PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Nicholas Piggin 11 months ago
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

 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

-- 
2.40.1
Re: [PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Cédric Le Goater 10 months, 2 weeks ago
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
>
Re: [PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Nicholas Piggin 10 months, 2 weeks ago
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
Re: [PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Cédric Le Goater 11 months ago
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
>
Re: [PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Nicholas Piggin 10 months, 3 weeks ago
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
Re: [PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Cédric Le Goater 10 months, 3 weeks ago
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.





Re: [PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Nicholas Piggin 10 months, 3 weeks ago
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
Re: [PATCH 0/4] ppc/pnv: Add chiptod and core timebase state machine models
Posted by Cédric Le Goater 10 months, 3 weeks ago
[ ... ]
> 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.