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

["Cédric Le Goater" <clg@kaod.org>]

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.