[PATCH] target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS bits

Nicholas Piggin posted 1 patch 2 weeks ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210503125833.891752-1-npiggin@gmail.com
Maintainers: Greg Kurz <groug@kaod.org>, David Gibson <david@gibson.dropbear.id.au>
hw/ppc/spapr_hcall.c   | 5 +++++
include/hw/ppc/spapr.h | 6 ++++++
2 files changed, 11 insertions(+)

[PATCH] target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS bits

Posted by Nicholas Piggin 2 weeks ago
There are several new bits added to the hcall which reflect new issues
found and new hardware mitigations.

This adds the link stack flush behaviour, link stack flush accelerated
instruction capability, and several L1D flush type behaviours (which are
now being specified as negative in order to simplify patched kernel
compatibility with older firmware).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_hcall.c   | 5 +++++
 include/hw/ppc/spapr.h | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 7275d0bba1..f656620232 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1878,6 +1878,9 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
         behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR;
         break;
     case SPAPR_CAP_FIXED:
+        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY;
+        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS;
+        behaviour |= H_CPU_BEHAV_NO_STF_BARRIER;
         break;
     default: /* broken */
         assert(safe_cache == SPAPR_CAP_BROKEN);
@@ -1909,9 +1912,11 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
         break;
     case SPAPR_CAP_WORKAROUND:
         behaviour |= H_CPU_BEHAV_FLUSH_COUNT_CACHE;
+        behaviour |= H_CPU_BEHAV_FLUSH_LINK_STACK;
         if (count_cache_flush_assist) {
             characteristics |= H_CPU_CHAR_BCCTR_FLUSH_ASSIST;
         }
+        /* Should have a way to enable BCCTR_LINK_FLUSH_ASSIST */
         break;
     default: /* broken */
         assert(safe_indirect_branch == SPAPR_CAP_BROKEN);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 7f40a158f4..883ac1db3c 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -395,10 +395,16 @@ struct SpaprMachineState {
 #define H_CPU_CHAR_THR_RECONF_TRIG              PPC_BIT(6)
 #define H_CPU_CHAR_CACHE_COUNT_DIS              PPC_BIT(7)
 #define H_CPU_CHAR_BCCTR_FLUSH_ASSIST           PPC_BIT(9)
+#define H_CPU_CHAR_BCCTR_LINK_FLUSH_ASSIST      PPC_BIT(11)
+
 #define H_CPU_BEHAV_FAVOUR_SECURITY             PPC_BIT(0)
 #define H_CPU_BEHAV_L1D_FLUSH_PR                PPC_BIT(1)
 #define H_CPU_BEHAV_BNDS_CHK_SPEC_BAR           PPC_BIT(2)
 #define H_CPU_BEHAV_FLUSH_COUNT_CACHE           PPC_BIT(5)
+#define H_CPU_BEHAV_FLUSH_LINK_STACK            PPC_BIT(6)
+#define H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY          PPC_BIT(7)
+#define H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS        PPC_BIT(8)
+#define H_CPU_BEHAV_NO_STF_BARRIER              PPC_BIT(9)
 
 /* Each control block has to be on a 4K boundary */
 #define H_CB_ALIGNMENT     4096
-- 
2.23.0


Re: [PATCH] target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS bits

Posted by David Gibson 2 weeks ago
On Mon, May 03, 2021 at 10:58:33PM +1000, Nicholas Piggin wrote:
> There are several new bits added to the hcall which reflect new issues
> found and new hardware mitigations.
> 
> This adds the link stack flush behaviour, link stack flush accelerated
> instruction capability, and several L1D flush type behaviours (which are
> now being specified as negative in order to simplify patched kernel
> compatibility with older firmware).

So, to clarify here, the bits your adding aren't advertising any new
behaviour on qemu/KVM's part, they're just new ways of advertising the
same behaviour?

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c   | 5 +++++
>  include/hw/ppc/spapr.h | 6 ++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 7275d0bba1..f656620232 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1878,6 +1878,9 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
>          behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR;
>          break;
>      case SPAPR_CAP_FIXED:
> +        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY;
> +        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS;
> +        behaviour |= H_CPU_BEHAV_NO_STF_BARRIER;
>          break;
>      default: /* broken */
>          assert(safe_cache == SPAPR_CAP_BROKEN);
> @@ -1909,9 +1912,11 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
>          break;
>      case SPAPR_CAP_WORKAROUND:
>          behaviour |= H_CPU_BEHAV_FLUSH_COUNT_CACHE;
> +        behaviour |= H_CPU_BEHAV_FLUSH_LINK_STACK;
>          if (count_cache_flush_assist) {
>              characteristics |= H_CPU_CHAR_BCCTR_FLUSH_ASSIST;
>          }
> +        /* Should have a way to enable BCCTR_LINK_FLUSH_ASSIST */

Do we need a new spapr capability for this link flush thing?

>          break;
>      default: /* broken */
>          assert(safe_indirect_branch == SPAPR_CAP_BROKEN);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 7f40a158f4..883ac1db3c 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -395,10 +395,16 @@ struct SpaprMachineState {
>  #define H_CPU_CHAR_THR_RECONF_TRIG              PPC_BIT(6)
>  #define H_CPU_CHAR_CACHE_COUNT_DIS              PPC_BIT(7)
>  #define H_CPU_CHAR_BCCTR_FLUSH_ASSIST           PPC_BIT(9)
> +#define H_CPU_CHAR_BCCTR_LINK_FLUSH_ASSIST      PPC_BIT(11)
> +
>  #define H_CPU_BEHAV_FAVOUR_SECURITY             PPC_BIT(0)
>  #define H_CPU_BEHAV_L1D_FLUSH_PR                PPC_BIT(1)
>  #define H_CPU_BEHAV_BNDS_CHK_SPEC_BAR           PPC_BIT(2)
>  #define H_CPU_BEHAV_FLUSH_COUNT_CACHE           PPC_BIT(5)
> +#define H_CPU_BEHAV_FLUSH_LINK_STACK            PPC_BIT(6)
> +#define H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY          PPC_BIT(7)
> +#define H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS        PPC_BIT(8)
> +#define H_CPU_BEHAV_NO_STF_BARRIER              PPC_BIT(9)
>  
>  /* Each control block has to be on a 4K boundary */
>  #define H_CB_ALIGNMENT     4096

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS bits

Posted by Nicholas Piggin 2 weeks ago
Excerpts from David Gibson's message of May 4, 2021 10:41 am:
> On Mon, May 03, 2021 at 10:58:33PM +1000, Nicholas Piggin wrote:
>> There are several new bits added to the hcall which reflect new issues
>> found and new hardware mitigations.
>> 
>> This adds the link stack flush behaviour, link stack flush accelerated
>> instruction capability, and several L1D flush type behaviours (which are
>> now being specified as negative in order to simplify patched kernel
>> compatibility with older firmware).
> 
> So, to clarify here, the bits your adding aren't advertising any new
> behaviour on qemu/KVM's part, they're just new ways of advertising the
> same behaviour?

I... think so. "Behaviour" is in context of the hcall that advertises
how the processor behaves (or what the guest must do for security).

The new NO_ bits added are for processors that don't require a particular
flush. The FLUSH_LINK_STACK was basically always required but I think
Linux just keyed off the count cache flush and did both at once.

The new LINK_FLUSH_ASSIST is a new processor feature the guest will use
to implement link stack flushing, so maybe that does need a cap?

> 
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_hcall.c   | 5 +++++
>>  include/hw/ppc/spapr.h | 6 ++++++
>>  2 files changed, 11 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 7275d0bba1..f656620232 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1878,6 +1878,9 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
>>          behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR;
>>          break;
>>      case SPAPR_CAP_FIXED:
>> +        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY;
>> +        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS;
>> +        behaviour |= H_CPU_BEHAV_NO_STF_BARRIER;
>>          break;
>>      default: /* broken */
>>          assert(safe_cache == SPAPR_CAP_BROKEN);
>> @@ -1909,9 +1912,11 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
>>          break;
>>      case SPAPR_CAP_WORKAROUND:
>>          behaviour |= H_CPU_BEHAV_FLUSH_COUNT_CACHE;
>> +        behaviour |= H_CPU_BEHAV_FLUSH_LINK_STACK;
>>          if (count_cache_flush_assist) {
>>              characteristics |= H_CPU_CHAR_BCCTR_FLUSH_ASSIST;
>>          }
>> +        /* Should have a way to enable BCCTR_LINK_FLUSH_ASSIST */
> 
> Do we need a new spapr capability for this link flush thing?

It is independent of the FLUSH_COUNT_CACHE capability, so it seems like
it should I think? Should that be added as a following patch?

Thanks,
Nick

Re: [PATCH] target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS bits

Posted by David Gibson 1 week, 6 days ago
On Tue, May 04, 2021 at 06:50:54PM +1000, Nicholas Piggin wrote:
> Excerpts from David Gibson's message of May 4, 2021 10:41 am:
> > On Mon, May 03, 2021 at 10:58:33PM +1000, Nicholas Piggin wrote:
> >> There are several new bits added to the hcall which reflect new issues
> >> found and new hardware mitigations.
> >> 
> >> This adds the link stack flush behaviour, link stack flush accelerated
> >> instruction capability, and several L1D flush type behaviours (which are
> >> now being specified as negative in order to simplify patched kernel
> >> compatibility with older firmware).
> > 
> > So, to clarify here, the bits your adding aren't advertising any new
> > behaviour on qemu/KVM's part, they're just new ways of advertising the
> > same behaviour?
> 
> I... think so. "Behaviour" is in context of the hcall that advertises
> how the processor behaves (or what the guest must do for security).

Heh.  Don't get me started on how the difference between
"characteristics" and "behaviours" in the fields is totally
non-obvious.

> The new NO_ bits added are for processors that don't require a particular
> flush. The FLUSH_LINK_STACK was basically always required but I think
> Linux just keyed off the count cache flush and did both at once.
> 
> The new LINK_FLUSH_ASSIST is a new processor feature the guest will use
> to implement link stack flushing, so maybe that does need a cap?

Yeah, sounds like it will.

> 
> > 
> >> 
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >>  hw/ppc/spapr_hcall.c   | 5 +++++
> >>  include/hw/ppc/spapr.h | 6 ++++++
> >>  2 files changed, 11 insertions(+)
> >> 
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 7275d0bba1..f656620232 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -1878,6 +1878,9 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> >>          behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR;
> >>          break;
> >>      case SPAPR_CAP_FIXED:
> >> +        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY;
> >> +        behaviour |= H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS;
> >> +        behaviour |= H_CPU_BEHAV_NO_STF_BARRIER;
> >>          break;
> >>      default: /* broken */
> >>          assert(safe_cache == SPAPR_CAP_BROKEN);
> >> @@ -1909,9 +1912,11 @@ static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> >>          break;
> >>      case SPAPR_CAP_WORKAROUND:
> >>          behaviour |= H_CPU_BEHAV_FLUSH_COUNT_CACHE;
> >> +        behaviour |= H_CPU_BEHAV_FLUSH_LINK_STACK;
> >>          if (count_cache_flush_assist) {
> >>              characteristics |= H_CPU_CHAR_BCCTR_FLUSH_ASSIST;
> >>          }
> >> +        /* Should have a way to enable BCCTR_LINK_FLUSH_ASSIST */
> > 
> > Do we need a new spapr capability for this link flush thing?
> 
> It is independent of the FLUSH_COUNT_CACHE capability, so it seems like
> it should I think? Should that be added as a following patch?

No, it will have to go in first or at the same time.  Otherwise we'll
be errneously advertising things.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson