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

[Nicholas Piggin]

Excerpts from David Gibson's message of May 5, 2021 2:20 pm:
> 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.

Sorry to take so long to get back to this.

I think it might be more complex than I first had. IBS is for indirect 
branches except blr type AFAIK, so link stack may still need to be 
flushed if count cache is disabled, for example. So then we'd need a
RBS (return branch speculation) or something for the link stack, as well 
as the cap for link stack flush assist instruction.

While STF barrier may not be quite right to key off safe cache cap, 
maybe it needs its own cap as well? Although CPUs today follow the rule 
that if the cache flush is required then so is the stf barrier.

At any rate, the cache flush ones are most important so I'll send a 
patch for them alone first.

Thanks,
Nick