Re: [PATCH 06/13] ppc/spapr: Add pa-features for POWER10 machines

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Mar 12, 2024 at 7:59 PM AEST, BALATON Zoltan wrote:
> On Tue, 12 Mar 2024, Nicholas Piggin wrote:
> > On Tue Mar 12, 2024 at 7:07 AM AEST, BALATON Zoltan wrote:
> >> On Mon, 11 Mar 2024, Philippe Mathieu-Daudé wrote:
> >>> On 11/3/24 19:51, Nicholas Piggin wrote:
> >>>> From: Benjamin Gray <bgray@linux.ibm.com>
> >>>>
> >>>> Add POWER10 pa-features entry.
> >>>>
> >>>> Notably DEXCR and and [P]HASHST/[P]HASHCHK instruction support is
> >>>> advertised. Each DEXCR aspect is allocated a bit in the device tree,
> >>>> using the 68--71 byte range (inclusive). The functionality of the
> >>>> [P]HASHST/[P]HASHCHK instructions is separately declared in byte 72,
> >>>> bit 0 (BE).
> >>>>
> >>>> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> >>>> [npiggin: reword title and changelog, adjust a few bits]
> >>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >>>> ---
> >>>>   hw/ppc/spapr.c | 34 ++++++++++++++++++++++++++++++++++
> >>>>   1 file changed, 34 insertions(+)
> >>>>
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 247f920f07..128bfe11a8 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -265,6 +265,36 @@ static void spapr_dt_pa_features(SpaprMachineState
> >>>> *spapr,
> >>>>           /* 60: NM atomic, 62: RNG */
> >>>>           0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
> >>>>       };
> >>>> +    /* 3.1 removes SAO, HTM support */
> >>>> +    uint8_t pa_features_31[] = { 74, 0,
> >>>
> >>> Nitpicking because pre-existing, all these arrays could be static const.
> >>
> >> If we are at it then maybe also s/0x00/   0/ because having a stream of
> >> 0x80 and 0x00 is not the most readable.
> >
> > Eh, it's more readable because it aligns colums.
>
> Not sure it you've noticed the 3 spaces before the 0 replacing 0x0 that 
> would keep alignment.

Oh, yeah I guess that would be a more obvious zero rather than hunting
for 8s. You're right.

> But it's not something that needs to be changed just 
> commented on it as it came up but I don't expect it to be done now on the 
> day of the freeze. It's more important to get the already reviewed and 
> queued patches in a pull request to not miss the release. So this comment 
> is just for the fuuture.

Yeah we should rework it completely. Anyway thanks for taking a look.

Thanks,
Nick