Re: [RFC PATCH v2 07/10] KVM: PPC: Ultravisor: Restrict LDBAR access

[Claudio Carvalho <cclaudio@linux.ibm.com>]

On 5/21/19 2:24 AM, Madhavan Srinivasan wrote:
>
> On 18/05/19 7:55 PM, Claudio Carvalho wrote:
>> From: Ram Pai <linuxram@us.ibm.com> When the ultravisor firmware is
>> available, it takes control over the LDBAR register. In this case,
>> thread-imc updates and save/restore operations on the LDBAR register are
>> handled by ultravisor.
> we should remove the core and thread imc nodes in the skiboot
> if the ultravisor is enabled. We dont need imc-pmu.c file changes, imc-pmu.c
> inits only if we have the corresponding nodes. I will post a skiboot patch.

Hi Maddy,

Thanks for the feedback and for taking care of the change needed in skiboot.

Right, if the core and thread imc devtree nodes were not created by
skiboot, then we don't need imc-pmu changes. As a sanity check, should we
have something like the following to make sure that the imc-pmu code will
not be executed even in the situation where ultravisor is enabled, but for
some reason skiboot did not remove those devtree nodes (e.g. your skiboot
patch was not applied)?

-------------------------

--- a/arch/powerpc/platforms/powernv/opal-imc.c
+++ b/arch/powerpc/platforms/powernv/opal-imc.c
@@ -258,6 +258,15 @@ static int opal_imc_counters_probe(struct
platform_device *pdev)
        bool core_imc_reg = false, thread_imc_reg = false;
        u32 type;
 
+       /*
+        * When ultravisor is enabled, it is responsible for thread-imc
+        * updates
+        */
+       if (firmware_has_feature(FW_FEATURE_ULTRAVISOR)) {
+               pr_info("IMC Ultravisor enabled\n");
+               return -EACCES;
+       }
+
        /*
         * Check whether this is kdump kernel. If yes, force the engines to
         * stop and return.

----------------------------

Claudio


> Maddy
>
>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> [Restrict LDBAR access in assembly code and some in C, update the commit
>>   message]
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> ---
>>   arch/powerpc/kvm/book3s_hv.c                 |  4 +-
>>   arch/powerpc/kvm/book3s_hv_rmhandlers.S      |  2 +
>>   arch/powerpc/perf/imc-pmu.c                  | 64 ++++++++++++--------
>>   arch/powerpc/platforms/powernv/idle.c        |  6 +-
>>   arch/powerpc/platforms/powernv/subcore-asm.S |  4 ++
>>   5 files changed, 52 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 0fab0a201027..81f35f955d16 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -75,6 +75,7 @@
>>   #include <asm/xics.h>
>>   #include <asm/xive.h>
>>   #include <asm/hw_breakpoint.h>
>> +#include <asm/firmware.h>
>>
>>   #include "book3s.h"
>>
>> @@ -3117,7 +3118,8 @@ static noinline void kvmppc_run_core(struct
>> kvmppc_vcore *vc)
>>               subcore_size = MAX_SMT_THREADS / split;
>>               split_info.rpr = mfspr(SPRN_RPR);
>>               split_info.pmmar = mfspr(SPRN_PMMAR);
>> -            split_info.ldbar = mfspr(SPRN_LDBAR);
>> +            if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> +                split_info.ldbar = mfspr(SPRN_LDBAR);
>>               split_info.subcore_size = subcore_size;
>>           } else {
>>               split_info.subcore_size = 1;
>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> index dd014308f065..938cfa5dceed 100644
>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> @@ -375,8 +375,10 @@ BEGIN_FTR_SECTION
>>       mtspr    SPRN_RPR, r0
>>       ld    r0, KVM_SPLIT_PMMAR(r6)
>>       mtspr    SPRN_PMMAR, r0
>> +BEGIN_FW_FTR_SECTION_NESTED(70)
>>       ld    r0, KVM_SPLIT_LDBAR(r6)
>>       mtspr    SPRN_LDBAR, r0
>> +END_FW_FTR_SECTION_NESTED(FW_FEATURE_ULTRAVISOR, 0, 70)
>>       isync
>>   FTR_SECTION_ELSE
>>       /* On P9 we use the split_info for coordinating LPCR changes */
>> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
>> index 31fa753e2eb2..39c84de74da9 100644
>> --- a/arch/powerpc/perf/imc-pmu.c
>> +++ b/arch/powerpc/perf/imc-pmu.c
>> @@ -17,6 +17,7 @@
>>   #include <asm/cputhreads.h>
>>   #include <asm/smp.h>
>>   #include <linux/string.h>
>> +#include <asm/firmware.h>
>>
>>   /* Nest IMC data structures and variables */
>>
>> @@ -816,6 +817,17 @@ static int core_imc_event_init(struct perf_event
>> *event)
>>       return 0;
>>   }
>>
>> +static void thread_imc_ldbar_disable(void *dummy)
>> +{
>> +    /*
>> +     * By Zeroing LDBAR, we disable thread-imc updates. When the
>> ultravisor
>> +     * firmware is available, it is responsible for handling thread-imc
>> +     * updates, though
>> +     */
>> +    if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> +        mtspr(SPRN_LDBAR, 0);
>> +}
>> +
>>   /*
>>    * Allocates a page of memory for each of the online cpus, and load
>>    * LDBAR with 0.
>> @@ -856,7 +868,7 @@ static int thread_imc_mem_alloc(int cpu_id, int size)
>>           per_cpu(thread_imc_mem, cpu_id) = local_mem;
>>       }
>>
>> -    mtspr(SPRN_LDBAR, 0);
>> +    thread_imc_ldbar_disable(NULL);
>>       return 0;
>>   }
>>
>> @@ -867,7 +879,7 @@ static int ppc_thread_imc_cpu_online(unsigned int cpu)
>>
>>   static int ppc_thread_imc_cpu_offline(unsigned int cpu)
>>   {
>> -    mtspr(SPRN_LDBAR, 0);
>> +    thread_imc_ldbar_disable(NULL);
>>       return 0;
>>   }
>>
>> @@ -1010,7 +1022,6 @@ static int thread_imc_event_add(struct perf_event
>> *event, int flags)
>>   {
>>       int core_id;
>>       struct imc_pmu_ref *ref;
>> -    u64 ldbar_value, *local_mem = per_cpu(thread_imc_mem,
>> smp_processor_id());
>>
>>       if (flags & PERF_EF_START)
>>           imc_event_start(event, flags);
>> @@ -1019,8 +1030,14 @@ static int thread_imc_event_add(struct perf_event
>> *event, int flags)
>>           return -EINVAL;
>>
>>       core_id = smp_processor_id() / threads_per_core;
>> -    ldbar_value = ((u64)local_mem & THREAD_IMC_LDBAR_MASK) |
>> THREAD_IMC_ENABLE;
>> -    mtspr(SPRN_LDBAR, ldbar_value);
>> +    if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR)) {
>> +        u64 ldbar_value, *local_mem;
>> +
>> +        local_mem = per_cpu(thread_imc_mem, smp_processor_id());
>> +        ldbar_value = ((u64)local_mem & THREAD_IMC_LDBAR_MASK) |
>> +                THREAD_IMC_ENABLE;
>> +        mtspr(SPRN_LDBAR, ldbar_value);
>> +    }
>>
>>       /*
>>        * imc pmus are enabled only when it is used.
>> @@ -1053,7 +1070,7 @@ static void thread_imc_event_del(struct perf_event
>> *event, int flags)
>>       int core_id;
>>       struct imc_pmu_ref *ref;
>>
>> -    mtspr(SPRN_LDBAR, 0);
>> +    thread_imc_ldbar_disable(NULL);
>>
>>       core_id = smp_processor_id() / threads_per_core;
>>       ref = &core_imc_refc[core_id];
>> @@ -1109,7 +1126,7 @@ static int trace_imc_mem_alloc(int cpu_id, int size)
>>       trace_imc_refc[core_id].id = core_id;
>>       mutex_init(&trace_imc_refc[core_id].lock);
>>
>> -    mtspr(SPRN_LDBAR, 0);
>> +    thread_imc_ldbar_disable(NULL);
>>       return 0;
>>   }
>>
>> @@ -1120,7 +1137,7 @@ static int ppc_trace_imc_cpu_online(unsigned int cpu)
>>
>>   static int ppc_trace_imc_cpu_offline(unsigned int cpu)
>>   {
>> -    mtspr(SPRN_LDBAR, 0);
>> +    thread_imc_ldbar_disable(NULL);
>>       return 0;
>>   }
>>
>> @@ -1207,11 +1224,6 @@ static int trace_imc_event_add(struct perf_event
>> *event, int flags)
>>   {
>>       int core_id = smp_processor_id() / threads_per_core;
>>       struct imc_pmu_ref *ref = NULL;
>> -    u64 local_mem, ldbar_value;
>> -
>> -    /* Set trace-imc bit in ldbar and load ldbar with per-thread memory
>> address */
>> -    local_mem = get_trace_imc_event_base_addr();
>> -    ldbar_value = ((u64)local_mem & THREAD_IMC_LDBAR_MASK) |
>> TRACE_IMC_ENABLE;
>>
>>       if (core_imc_refc)
>>           ref = &core_imc_refc[core_id];
>> @@ -1222,14 +1234,25 @@ static int trace_imc_event_add(struct perf_event
>> *event, int flags)
>>           if (!ref)
>>               return -EINVAL;
>>       }
>> -    mtspr(SPRN_LDBAR, ldbar_value);
>> +    if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR)) {
>> +        u64 local_mem, ldbar_value;
>> +
>> +        /*
>> +         * Set trace-imc bit in ldbar and load ldbar with per-thread
>> +         * memory address
>> +         */
>> +        local_mem = get_trace_imc_event_base_addr();
>> +        ldbar_value = ((u64)local_mem & THREAD_IMC_LDBAR_MASK) |
>> +                TRACE_IMC_ENABLE;
>> +        mtspr(SPRN_LDBAR, ldbar_value);
>> +    }
>>       mutex_lock(&ref->lock);
>>       if (ref->refc == 0) {
>>           if (opal_imc_counters_start(OPAL_IMC_COUNTERS_TRACE,
>>                   get_hard_smp_processor_id(smp_processor_id()))) {
>>               mutex_unlock(&ref->lock);
>>               pr_err("trace-imc: Unable to start the counters for core
>> %d\n", core_id);
>> -            mtspr(SPRN_LDBAR, 0);
>> +            thread_imc_ldbar_disable(NULL);
>>               return -EINVAL;
>>           }
>>       }
>> @@ -1270,7 +1293,7 @@ static void trace_imc_event_del(struct perf_event
>> *event, int flags)
>>           if (!ref)
>>               return;
>>       }
>> -    mtspr(SPRN_LDBAR, 0);
>> +    thread_imc_ldbar_disable(NULL);
>>       mutex_lock(&ref->lock);
>>       ref->refc--;
>>       if (ref->refc == 0) {
>> @@ -1413,15 +1436,6 @@ static void cleanup_all_core_imc_memory(void)
>>       kfree(core_imc_refc);
>>   }
>>
>> -static void thread_imc_ldbar_disable(void *dummy)
>> -{
>> -    /*
>> -     * By Zeroing LDBAR, we disable thread-imc
>> -     * updates.
>> -     */
>> -    mtspr(SPRN_LDBAR, 0);
>> -}
>> -
>>   void thread_imc_disable(void)
>>   {
>>       on_each_cpu(thread_imc_ldbar_disable, NULL, 1);
>> diff --git a/arch/powerpc/platforms/powernv/idle.c
>> b/arch/powerpc/platforms/powernv/idle.c
>> index c9133f7908ca..fd62435e3267 100644
>> --- a/arch/powerpc/platforms/powernv/idle.c
>> +++ b/arch/powerpc/platforms/powernv/idle.c
>> @@ -679,7 +679,8 @@ static unsigned long power9_idle_stop(unsigned long
>> psscr, bool mmu_on)
>>           sprs.ptcr    = mfspr(SPRN_PTCR);
>>           sprs.rpr    = mfspr(SPRN_RPR);
>>           sprs.tscr    = mfspr(SPRN_TSCR);
>> -        sprs.ldbar    = mfspr(SPRN_LDBAR);
>> +        if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> +            sprs.ldbar    = mfspr(SPRN_LDBAR);
>>
>>           sprs_saved = true;
>>
>> @@ -762,7 +763,8 @@ static unsigned long power9_idle_stop(unsigned long
>> psscr, bool mmu_on)
>>       mtspr(SPRN_PTCR,    sprs.ptcr);
>>       mtspr(SPRN_RPR,        sprs.rpr);
>>       mtspr(SPRN_TSCR,    sprs.tscr);
>> -    mtspr(SPRN_LDBAR,    sprs.ldbar);
>> +    if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
>> +        mtspr(SPRN_LDBAR,    sprs.ldbar);
>>
>>       if (pls >= pnv_first_tb_loss_level) {
>>           /* TB loss */
>> diff --git a/arch/powerpc/platforms/powernv/subcore-asm.S
>> b/arch/powerpc/platforms/powernv/subcore-asm.S
>> index 39bb24aa8f34..e4383fa5e150 100644
>> --- a/arch/powerpc/platforms/powernv/subcore-asm.S
>> +++ b/arch/powerpc/platforms/powernv/subcore-asm.S
>> @@ -44,7 +44,9 @@ _GLOBAL(split_core_secondary_loop)
>>
>>   real_mode:
>>       /* Grab values from unsplit SPRs */
>> +BEGIN_FW_FTR_SECTION
>>       mfspr    r6,  SPRN_LDBAR
>> +END_FW_FTR_SECTION_IFCLR(FW_FEATURE_ULTRAVISOR)
>>       mfspr    r7,  SPRN_PMMAR
>>       mfspr    r8,  SPRN_PMCR
>>       mfspr    r9,  SPRN_RPR
>> @@ -77,7 +79,9 @@ real_mode:
>>       mtspr    SPRN_HDEC, r4
>>
>>       /* Restore SPR values now we are split */
>> +BEGIN_FW_FTR_SECTION
>>       mtspr    SPRN_LDBAR, r6
>> +END_FW_FTR_SECTION_IFCLR(FW_FEATURE_ULTRAVISOR)
>>       mtspr    SPRN_PMMAR, r7
>>       mtspr    SPRN_PMCR, r8
>>       mtspr    SPRN_RPR, r9
>