diff mbox series

target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS L1D cache flush bits

Message ID 20210615044107.1481608-1-npiggin@gmail.com
State New
Headers show
Series target/ppc/spapr: Update H_GET_CPU_CHARACTERISTICS L1D cache flush bits | expand

Commit Message

Nicholas Piggin June 15, 2021, 4:41 a.m. UTC 
There are several new L1D cache flush bits added to the hcall which reflect
hardware security features for speculative cache access issues.

These behaviours are now being specified as negative in order to simplify
patched kernel compatibility with older firmware (a new problem found in
existing systems would automatically be vulnerable).

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

Comments

David Gibson June 19, 2021, 9:26 a.m. UTC | #1 
On Tue, Jun 15, 2021 at 02:41:07PM +1000, Nicholas Piggin wrote:
> There are several new L1D cache flush bits added to the hcall which reflect
> hardware security features for speculative cache access issues.
> 
> These behaviours are now being specified as negative in order to simplify
> patched kernel compatibility with older firmware (a new problem found in
> existing systems would automatically be vulnerable).

I don't really understand all the consequences of that.  What I need
to know here, is if it's safe to unconditionally enable these bits,
even for older machine types.

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c   | 2 ++
>  include/hw/ppc/spapr.h | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f25014afda..dfd9df469d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1299,6 +1299,8 @@ 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;
>          break;
>      default: /* broken */
>          assert(safe_cache == SPAPR_CAP_BROKEN);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index f05219f75e..0f25d081a8 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -398,10 +398,13 @@ 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_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_NO_L1D_FLUSH_ENTRY          PPC_BIT(7)
> +#define H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS        PPC_BIT(8)
>  
>  /* Each control block has to be on a 4K boundary */
>  #define H_CB_ALIGNMENT     4096
Nicholas Piggin June 20, 2021, 2:22 a.m. UTC | #2 
Excerpts from David Gibson's message of June 19, 2021 7:26 pm:
> On Tue, Jun 15, 2021 at 02:41:07PM +1000, Nicholas Piggin wrote:
>> There are several new L1D cache flush bits added to the hcall which reflect
>> hardware security features for speculative cache access issues.
>> 
>> These behaviours are now being specified as negative in order to simplify
>> patched kernel compatibility with older firmware (a new problem found in
>> existing systems would automatically be vulnerable).
> 
> I don't really understand all the consequences of that.  What I need
> to know here, is if it's safe to unconditionally enable these bits,
> even for older machine types.

Unconditionally on the condition that the cap is set to fixed? It
should be fine AFAIKS. If the older machine types are running on
older hardware that acutally does require the flush, then the fixed
cap would cause the existing flush bit to be clear and the kernel to 
skip the exit flush, so that would be broken already. Does that sound
right?

One thing I'm not entirely clear on is:

All these (entry/exit/uaccess) flush requirements stem from basically 
the same underlying mechanism, so that gets resolved in hardware and
software can stop doing all of them. That's fine, but it was decided to
add different bits to the hcall basically to have flexibility let's say
in case a new issue is discovered one day that requires just the uaccess 
flush, for example.

In that case we can just set the right combination of bits in firmware,
and kernels in the field will just do the right thing, and we don't
need to do all the other flushes that would be worse for performance.

How would that work with qemu? I assume we don't have a cap per bit in 
the hcall, but rather a cap per vulnerability class, so you would set 
that new class as vulnerable, and this code will have to translate those
and work out the correct combination of bits to set in these fields.

If I'm way off base or there's a better way to do it, that could mean
this patch needs to be done a different way.

Thanks,
Nick

> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_hcall.c   | 2 ++
>>  include/hw/ppc/spapr.h | 3 +++
>>  2 files changed, 5 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index f25014afda..dfd9df469d 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1299,6 +1299,8 @@ 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;
>>          break;
>>      default: /* broken */
>>          assert(safe_cache == SPAPR_CAP_BROKEN);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index f05219f75e..0f25d081a8 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -398,10 +398,13 @@ 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_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_NO_L1D_FLUSH_ENTRY          PPC_BIT(7)
>> +#define H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS        PPC_BIT(8)
>>  
>>  /* 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
>
David Gibson July 5, 2021, 5:12 a.m. UTC | #3 
On Sun, Jun 20, 2021 at 12:22:53PM +1000, Nicholas Piggin wrote:
> Excerpts from David Gibson's message of June 19, 2021 7:26 pm:
> > On Tue, Jun 15, 2021 at 02:41:07PM +1000, Nicholas Piggin wrote:
> >> There are several new L1D cache flush bits added to the hcall which reflect
> >> hardware security features for speculative cache access issues.
> >> 
> >> These behaviours are now being specified as negative in order to simplify
> >> patched kernel compatibility with older firmware (a new problem found in
> >> existing systems would automatically be vulnerable).
> > 
> > I don't really understand all the consequences of that.  What I need
> > to know here, is if it's safe to unconditionally enable these bits,
> > even for older machine types.
> 
> Unconditionally on the condition that the cap is set to fixed?

Sorry, poor phrashing.  I meant it's not conditional on the machine
version, which means we are technically changing behaviour for
existing machine versions.  That's a big red flag, though there can be
circumstances in which it's justified.

> It
> should be fine AFAIKS. If the older machine types are running on
> older hardware that acutally does require the flush, then the fixed
> cap would cause the existing flush bit to be clear and the kernel to 
> skip the exit flush, so that would be broken already. Does that sound
> right?

Urgh.  The fact that some bits have different sense to others is doing
my head in.

> One thing I'm not entirely clear on is:
> 
> All these (entry/exit/uaccess) flush requirements stem from basically 
> the same underlying mechanism, so that gets resolved in hardware and
> software can stop doing all of them. That's fine, but it was decided to
> add different bits to the hcall basically to have flexibility let's say
> in case a new issue is discovered one day that requires just the uaccess 
> flush, for example.
> 
> In that case we can just set the right combination of bits in firmware,
> and kernels in the field will just do the right thing, and we don't
> need to do all the other flushes that would be worse for performance.

Right, but that doesn't work for qemu guests.  Qemu needs to advertise
things so that guests will do all the things that are necessary not
just on this host, but on any host we might be migrated to.

> How would that work with qemu? I assume we don't have a cap per bit in 
> the hcall, but rather a cap per vulnerability class, so you would set 
> that new class as vulnerable, and this code will have to translate those
> and work out the correct combination of bits to set in these fields.

Something like that, yes.  Actually doing so can be pretty complicated
because of the need to have a consistent migration domain.

> If I'm way off base or there's a better way to do it, that could mean
> this patch needs to be done a different way.

I think we need to find a time to discuss this (Slack or call or
whatever), so you can step me through this bit by bit.
David Gibson July 8, 2021, 3:54 a.m. UTC | #4 
On Tue, Jun 15, 2021 at 02:41:07PM +1000, Nicholas Piggin wrote:
> There are several new L1D cache flush bits added to the hcall which reflect
> hardware security features for speculative cache access issues.
> 
> These behaviours are now being specified as negative in order to simplify
> patched kernel compatibility with older firmware (a new problem found in
> existing systems would automatically be vulnerable).
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

After our discussion, I'm convinced that the small behaviour change
for old machine types is safe.  I've added an explanatory note to the
commit message and merged to ppc-for-6.1.

> ---
>  hw/ppc/spapr_hcall.c   | 2 ++
>  include/hw/ppc/spapr.h | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f25014afda..dfd9df469d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1299,6 +1299,8 @@ 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;
>          break;
>      default: /* broken */
>          assert(safe_cache == SPAPR_CAP_BROKEN);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index f05219f75e..0f25d081a8 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -398,10 +398,13 @@ 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_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_NO_L1D_FLUSH_ENTRY          PPC_BIT(7)
> +#define H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS        PPC_BIT(8)
>  
>  /* Each control block has to be on a 4K boundary */
>  #define H_CB_ALIGNMENT     4096
diff mbox series

Patch

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f25014afda..dfd9df469d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1299,6 +1299,8 @@  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;
         break;
     default: /* broken */
         assert(safe_cache == SPAPR_CAP_BROKEN);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f05219f75e..0f25d081a8 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -398,10 +398,13 @@  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_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_NO_L1D_FLUSH_ENTRY          PPC_BIT(7)
+#define H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS        PPC_BIT(8)
 
 /* Each control block has to be on a 4K boundary */
 #define H_CB_ALIGNMENT     4096