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

[Nicholas Piggin]

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. But probably better
more readable and  less error prone would be like -

    PA_FEATURE_SET(pa_features_31,  6, 0); /* DS207 */
    PA_FEATURE_SET(pa_features_31, 18, 0); /* Vector scalar */

I just didn't quite find something I like yet. I won't change style
before adding the missing bits either way, but certainly would be
good to clean it up after.

Thanks,
Nick