diff mbox series

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

Message ID 20240311185200.2185753-7-npiggin@gmail.com
State Not Applicable
Headers show
Series misc ppc patches | expand

Commit Message

Nicholas Piggin March 11, 2024, 6:51 p.m. UTC 
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(+)

Comments

Philippe Mathieu-Daudé March 11, 2024, 8:05 p.m. UTC | #1 
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.

> +        /* 0: MMU|FPU|SLB|RUN|DABR|NX, 1: fri[nzpm]|DABRX|SPRG3|SLB0|PP110 */
> +        /* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, 5: LE|CFAR|EB|LSQ */
> +        0xf6, 0x1f, 0xc7, 0xc0, 0x00, 0xf0, /* 0 - 5 */
> +        /* 6: DS207 */
> +        0x80, 0x00, 0x00, 0x00, 0x00, 0x00, /* 6 - 11 */
> +        /* 16: Vector */
> +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 12 - 17 */
> +        /* 18: Vec. Scalar, 20: Vec. XOR */
> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 18 - 23 */
> +        /* 24: Ext. Dec, 26: 64 bit ftrs, 28: PM ftrs */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 24 - 29 */
> +        /* 32: LE atomic, 34: EBB + ext EBB */
> +        0x00, 0x00, 0x80, 0x00, 0xC0, 0x00, /* 30 - 35 */
> +        /* 40: Radix MMU */
> +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 36 - 41 */
> +        /* 42: PM, 44: PC RA, 46: SC vec'd */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 42 - 47 */
> +        /* 48: SIMD, 50: QP BFP, 52: String */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
> +        /* 54: DecFP, 56: DecI, 58: SHA */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
> +        /* 60: NM atomic, 62: RNG */
> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
> +        /* 68: DEXCR[SBHE|IBRTPDUS|SRAPD|NPHIE|PHIE] */
> +        0x00, 0x00, 0xce, 0x00, 0x00, 0x00, /* 66 - 71 */
> +        /* 72: [P]HASHCHK */
> +        0x80, 0x00,                         /* 72 - 73 */
> +    };
>       uint8_t *pa_features = NULL;
>       size_t pa_size;
>   
> @@ -280,6 +310,10 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
>           pa_features = pa_features_300;
>           pa_size = sizeof(pa_features_300);
>       }
> +    if (ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, cpu->compat_pvr)) {
> +        pa_features = pa_features_31;
> +        pa_size = sizeof(pa_features_31);
> +    }
>       if (!pa_features) {
>           return;
>       }
BALATON Zoltan March 11, 2024, 9:07 p.m. UTC | #2 
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.

Regards,
BALATON Zoltan

>> +        /* 0: MMU|FPU|SLB|RUN|DABR|NX, 1: fri[nzpm]|DABRX|SPRG3|SLB0|PP110 
>> */
>> +        /* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, 5: LE|CFAR|EB|LSQ */
>> +        0xf6, 0x1f, 0xc7, 0xc0, 0x00, 0xf0, /* 0 - 5 */
>> +        /* 6: DS207 */
>> +        0x80, 0x00, 0x00, 0x00, 0x00, 0x00, /* 6 - 11 */
>> +        /* 16: Vector */
>> +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 12 - 17 */
>> +        /* 18: Vec. Scalar, 20: Vec. XOR */
>> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 18 - 23 */
>> +        /* 24: Ext. Dec, 26: 64 bit ftrs, 28: PM ftrs */
>> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 24 - 29 */
>> +        /* 32: LE atomic, 34: EBB + ext EBB */
>> +        0x00, 0x00, 0x80, 0x00, 0xC0, 0x00, /* 30 - 35 */
>> +        /* 40: Radix MMU */
>> +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 36 - 41 */
>> +        /* 42: PM, 44: PC RA, 46: SC vec'd */
>> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 42 - 47 */
>> +        /* 48: SIMD, 50: QP BFP, 52: String */
>> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
>> +        /* 54: DecFP, 56: DecI, 58: SHA */
>> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
>> +        /* 60: NM atomic, 62: RNG */
>> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
>> +        /* 68: DEXCR[SBHE|IBRTPDUS|SRAPD|NPHIE|PHIE] */
>> +        0x00, 0x00, 0xce, 0x00, 0x00, 0x00, /* 66 - 71 */
>> +        /* 72: [P]HASHCHK */
>> +        0x80, 0x00,                         /* 72 - 73 */
>> +    };
>>       uint8_t *pa_features = NULL;
>>       size_t pa_size;
>>   @@ -280,6 +310,10 @@ static void spapr_dt_pa_features(SpaprMachineState 
>> *spapr,
>>           pa_features = pa_features_300;
>>           pa_size = sizeof(pa_features_300);
>>       }
>> +    if (ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, 
>> cpu->compat_pvr)) {
>> +        pa_features = pa_features_31;
>> +        pa_size = sizeof(pa_features_31);
>> +    }
>>       if (!pa_features) {
>>           return;
>>       }
>
>
>
Nicholas Piggin March 12, 2024, 4:45 a.m. UTC | #3 
On Tue Mar 12, 2024 at 6:05 AM AEST, 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.

That's true. I was looking for a nicer way to do it, probably generate
the bits with macros and share between spapr and pnv. This is just a
quick dumb approach to getting the missing bits in for now.

Thanks,
Nick
Nicholas Piggin March 12, 2024, 4:50 a.m. UTC | #4 
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
Harsh Prateek Bora March 12, 2024, 9:34 a.m. UTC | #5 
On 3/12/24 00:21, 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

s/and and/and

> 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,
> +        /* 0: MMU|FPU|SLB|RUN|DABR|NX, 1: fri[nzpm]|DABRX|SPRG3|SLB0|PP110 */
> +        /* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, 5: LE|CFAR|EB|LSQ */
> +        0xf6, 0x1f, 0xc7, 0xc0, 0x00, 0xf0, /* 0 - 5 */
> +        /* 6: DS207 */
> +        0x80, 0x00, 0x00, 0x00, 0x00, 0x00, /* 6 - 11 */
> +        /* 16: Vector */
> +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 12 - 17 */
> +        /* 18: Vec. Scalar, 20: Vec. XOR */
> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 18 - 23 */
> +        /* 24: Ext. Dec, 26: 64 bit ftrs, 28: PM ftrs */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 24 - 29 */
> +        /* 32: LE atomic, 34: EBB + ext EBB */
> +        0x00, 0x00, 0x80, 0x00, 0xC0, 0x00, /* 30 - 35 */
> +        /* 40: Radix MMU */
> +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 36 - 41 */
> +        /* 42: PM, 44: PC RA, 46: SC vec'd */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 42 - 47 */
> +        /* 48: SIMD, 50: QP BFP, 52: String */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
> +        /* 54: DecFP, 56: DecI, 58: SHA */
> +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
> +        /* 60: NM atomic, 62: RNG */
> +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
> +        /* 68: DEXCR[SBHE|IBRTPDUS|SRAPD|NPHIE|PHIE] */
> +        0x00, 0x00, 0xce, 0x00, 0x00, 0x00, /* 66 - 71 */
> +        /* 72: [P]HASHCHK */

Do we want to mention [P]HASHST as well in comment above ?

> +        0x80, 0x00,                         /* 72 - 73 */
> +    };
>       uint8_t *pa_features = NULL;
>       size_t pa_size;
>   

In future, we may want to have helpers returning pointer to the
pa_features array and corresponding size conditionally based on the
required ISA support needed, instead of having local arrays bloat this
routine.

For now, with cosmetic fixes,

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> @@ -280,6 +310,10 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
>           pa_features = pa_features_300;
>           pa_size = sizeof(pa_features_300);
>       }
> +    if (ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, cpu->compat_pvr)) {
> +        pa_features = pa_features_31;
> +        pa_size = sizeof(pa_features_31);
> +    }
>       if (!pa_features) {
>           return;
>       }
BALATON Zoltan March 12, 2024, 9:59 a.m. UTC | #6 
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. 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.

Regards,
BALATON Zoltan

> 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
>
>
Nicholas Piggin March 12, 2024, 10:33 a.m. UTC | #7 
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
Nicholas Piggin March 12, 2024, 10:34 a.m. UTC | #8 
On Tue Mar 12, 2024 at 7:34 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 3/12/24 00:21, 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
>
> s/and and/and
>
> > 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,
> > +        /* 0: MMU|FPU|SLB|RUN|DABR|NX, 1: fri[nzpm]|DABRX|SPRG3|SLB0|PP110 */
> > +        /* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, 5: LE|CFAR|EB|LSQ */
> > +        0xf6, 0x1f, 0xc7, 0xc0, 0x00, 0xf0, /* 0 - 5 */
> > +        /* 6: DS207 */
> > +        0x80, 0x00, 0x00, 0x00, 0x00, 0x00, /* 6 - 11 */
> > +        /* 16: Vector */
> > +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 12 - 17 */
> > +        /* 18: Vec. Scalar, 20: Vec. XOR */
> > +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 18 - 23 */
> > +        /* 24: Ext. Dec, 26: 64 bit ftrs, 28: PM ftrs */
> > +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 24 - 29 */
> > +        /* 32: LE atomic, 34: EBB + ext EBB */
> > +        0x00, 0x00, 0x80, 0x00, 0xC0, 0x00, /* 30 - 35 */
> > +        /* 40: Radix MMU */
> > +        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 36 - 41 */
> > +        /* 42: PM, 44: PC RA, 46: SC vec'd */
> > +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 42 - 47 */
> > +        /* 48: SIMD, 50: QP BFP, 52: String */
> > +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
> > +        /* 54: DecFP, 56: DecI, 58: SHA */
> > +        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
> > +        /* 60: NM atomic, 62: RNG */
> > +        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
> > +        /* 68: DEXCR[SBHE|IBRTPDUS|SRAPD|NPHIE|PHIE] */
> > +        0x00, 0x00, 0xce, 0x00, 0x00, 0x00, /* 66 - 71 */
> > +        /* 72: [P]HASHCHK */
>
> Do we want to mention [P]HASHST as well in comment above ?

Sure. I'll do a quick respin.

Thanks,
Nick

>
> > +        0x80, 0x00,                         /* 72 - 73 */
> > +    };
> >       uint8_t *pa_features = NULL;
> >       size_t pa_size;
> >   
>
> In future, we may want to have helpers returning pointer to the
> pa_features array and corresponding size conditionally based on the
> required ISA support needed, instead of having local arrays bloat this
> routine.
>
> For now, with cosmetic fixes,
>
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
> > @@ -280,6 +310,10 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
> >           pa_features = pa_features_300;
> >           pa_size = sizeof(pa_features_300);
> >       }
> > +    if (ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, cpu->compat_pvr)) {
> > +        pa_features = pa_features_31;
> > +        pa_size = sizeof(pa_features_31);
> > +    }
> >       if (!pa_features) {
> >           return;
> >       }
diff mbox series

Patch

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,
+        /* 0: MMU|FPU|SLB|RUN|DABR|NX, 1: fri[nzpm]|DABRX|SPRG3|SLB0|PP110 */
+        /* 2: VPM|DS205|PPR|DS202|DS206, 3: LSD|URG, 5: LE|CFAR|EB|LSQ */
+        0xf6, 0x1f, 0xc7, 0xc0, 0x00, 0xf0, /* 0 - 5 */
+        /* 6: DS207 */
+        0x80, 0x00, 0x00, 0x00, 0x00, 0x00, /* 6 - 11 */
+        /* 16: Vector */
+        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 12 - 17 */
+        /* 18: Vec. Scalar, 20: Vec. XOR */
+        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 18 - 23 */
+        /* 24: Ext. Dec, 26: 64 bit ftrs, 28: PM ftrs */
+        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 24 - 29 */
+        /* 32: LE atomic, 34: EBB + ext EBB */
+        0x00, 0x00, 0x80, 0x00, 0xC0, 0x00, /* 30 - 35 */
+        /* 40: Radix MMU */
+        0x00, 0x00, 0x00, 0x00, 0x80, 0x00, /* 36 - 41 */
+        /* 42: PM, 44: PC RA, 46: SC vec'd */
+        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 42 - 47 */
+        /* 48: SIMD, 50: QP BFP, 52: String */
+        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
+        /* 54: DecFP, 56: DecI, 58: SHA */
+        0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
+        /* 60: NM atomic, 62: RNG */
+        0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
+        /* 68: DEXCR[SBHE|IBRTPDUS|SRAPD|NPHIE|PHIE] */
+        0x00, 0x00, 0xce, 0x00, 0x00, 0x00, /* 66 - 71 */
+        /* 72: [P]HASHCHK */
+        0x80, 0x00,                         /* 72 - 73 */
+    };
     uint8_t *pa_features = NULL;
     size_t pa_size;
 
@@ -280,6 +310,10 @@  static void spapr_dt_pa_features(SpaprMachineState *spapr,
         pa_features = pa_features_300;
         pa_size = sizeof(pa_features_300);
     }
+    if (ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, cpu->compat_pvr)) {
+        pa_features = pa_features_31;
+        pa_size = sizeof(pa_features_31);
+    }
     if (!pa_features) {
         return;
     }