Hi Andre,
On 5/3/19 11:33 AM, Andre Przywara wrote:
> On Fri, 26 Apr 2019 17:37:47 +0200
> Auger Eric <eric.auger@xxxxxxxxxx> wrote:
>
> Hi,
>
>> Hi Andre,
>>
>> On 4/15/19 4:03 PM, Steven Price wrote:
>>> On 15/04/2019 12:15, Andre Przywara wrote:
>>>> Recent commits added the explicit notion of "Not affected" to the state
>>>> of the Spectre v2 (aka. BP_HARDENING) workaround, where we just had
>>>> "needed" and "unknown" before.
>>>>
>>>> Export this knowledge to the rest of the kernel and enhance the existing
>>>> kvm_arm_harden_branch_predictor() to report this new state as well.
>>>> Export this new state to guests when they use KVM's firmware interface
>>>> emulation.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>>> ---
>>>> arch/arm/include/asm/kvm_host.h | 12 +++++++++---
>>>> arch/arm64/include/asm/cpufeature.h | 6 ++++++
>>>> arch/arm64/include/asm/kvm_host.h | 16 ++++++++++++++--
>>>> arch/arm64/kernel/cpu_errata.c | 23 ++++++++++++++++++-----
>>>> virt/kvm/arm/psci.c | 10 +++++++++-
>>>> 5 files changed, 56 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>> index 770d73257ad9..836479e4b340 100644
>>>> --- a/arch/arm/include/asm/kvm_host.h
>>>> +++ b/arch/arm/include/asm/kvm_host.h
>>>> @@ -364,7 +364,11 @@ static inline void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) {}
>>>> static inline void kvm_arm_vhe_guest_enter(void) {}
>>>> static inline void kvm_arm_vhe_guest_exit(void) {}
>>>>
>>>> -static inline bool kvm_arm_harden_branch_predictor(void)
>>>> +#define KVM_BP_HARDEN_UNKNOWN -1
>>>> +#define KVM_BP_HARDEN_NEEDED 0
>>>> +#define KVM_BP_HARDEN_MITIGATED 1
>>>
>>> I find the naming here a little confusing - it's not really clear what
>>> "mitigated" means (see below).
>
> That's indeed slightly confusing, but was modelled after the SSBD
> workaround below, which reads:
> #define KVM_SSBD_UNKNOWN -1
> #define KVM_SSBD_FORCE_DISABLE 0
> #define KVM_SSBD_KERNEL 1
> #define KVM_SSBD_FORCE_ENABLE 2
> #define KVM_SSBD_MITIGATED 3
>
> I changed the naming (for this and the other derived definitions) to:
> #define KVM_BP_HARDEN_UNKNOWN -1
> #define KVM_BP_HARDEN_WA_NEEDED 0
> #define KVM_BP_HARDEN_NOT_REQUIRED 1
>
>>>
>>>> +
>>>> +static inline int kvm_arm_harden_branch_predictor(void)
>>>> {
>>>> switch(read_cpuid_part()) {
>>>> #ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
>>>> @@ -372,10 +376,12 @@ static inline bool kvm_arm_harden_branch_predictor(void)
>>>> case ARM_CPU_PART_CORTEX_A12:
>>>> case ARM_CPU_PART_CORTEX_A15:
>>>> case ARM_CPU_PART_CORTEX_A17:
>>>> - return true;
>>>> + return KVM_BP_HARDEN_NEEDED;
>>>> #endif
>>>> + case ARM_CPU_PART_CORTEX_A7:
>>>> + return KVM_BP_HARDEN_MITIGATED;
>>>> default:
>>>> - return false;
>>>> + return KVM_BP_HARDEN_UNKNOWN;
>>>> }
>>>> }
>>>>
>>>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>>>> index 6ccdc97e5d6a..3c5b25c1bda1 100644
>>>> --- a/arch/arm64/include/asm/cpufeature.h
>>>> +++ b/arch/arm64/include/asm/cpufeature.h
>>>> @@ -622,6 +622,12 @@ static inline bool system_uses_irq_prio_masking(void)
>>>> cpus_have_const_cap(ARM64_HAS_IRQ_PRIO_MASKING);
>>>> }
>>>>
>>>> +#define ARM64_BP_HARDEN_UNKNOWN -1
>>>> +#define ARM64_BP_HARDEN_NEEDED 0
>>>> +#define ARM64_BP_HARDEN_MITIGATED 1
>>>> +
>>>> +int get_spectre_v2_workaround_state(void);
>>>> +
>>>> #define ARM64_SSBD_UNKNOWN -1
>>>> #define ARM64_SSBD_FORCE_DISABLE 0
>>>> #define ARM64_SSBD_KERNEL 1
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>> index a01fe087e022..bf9a59b7d1ce 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -555,9 +555,21 @@ static inline void kvm_arm_vhe_guest_exit(void)
>>>> isb();
>>>> }
>>>>
>>>> -static inline bool kvm_arm_harden_branch_predictor(void)
>>>> +#define KVM_BP_HARDEN_UNKNOWN -1
>>>> +#define KVM_BP_HARDEN_NEEDED 0
>>>> +#define KVM_BP_HARDEN_MITIGATED 1
>>>> +
>>>> +static inline int kvm_arm_harden_branch_predictor(void)
>>>> {
>>>> - return cpus_have_const_cap(ARM64_HARDEN_BRANCH_PREDICTOR);
>>>> + switch (get_spectre_v2_workaround_state()) {
>>>> + case ARM64_BP_HARDEN_NEEDED:
>>>> + return KVM_BP_HARDEN_NEEDED;
>>>> + case ARM64_BP_HARDEN_MITIGATED:
>>>> + return KVM_BP_HARDEN_MITIGATED;
>>>> + case ARM64_BP_HARDEN_UNKNOWN:
>>>> + default:
>>>> + return KVM_BP_HARDEN_UNKNOWN;
>>>> + }
>>>> }
>>>>
>>>> #define KVM_SSBD_UNKNOWN -1
>>>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>>>> index a1f3188c7be0..7fa23ab09d4e 100644
>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>> @@ -555,6 +555,17 @@ cpu_enable_cache_maint_trap(const struct arm64_cpu_capabilities *__unused)
>>>> static bool __hardenbp_enab = true;
>>>> static bool __spectrev2_safe = true;
>>>>
>>>> +int get_spectre_v2_workaround_state(void)
>>>> +{
>>>> + if (__spectrev2_safe)
>>>> + return ARM64_BP_HARDEN_MITIGATED;
>>>> +
>>>> + if (!__hardenbp_enab)
>>>> + return ARM64_BP_HARDEN_UNKNOWN;
>>>> +
>>>> + return ARM64_BP_HARDEN_NEEDED;
>>>> +}
>>>> +
>>>> /*
>>>> * List of CPUs that do not need any Spectre-v2 mitigation at all.
>>>> */
>>>> @@ -834,13 +845,15 @@ ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr,
>>>> ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr,
>>>> char *buf)
>>>> {
>>>> - if (__spectrev2_safe)
>>>> + switch (get_spectre_v2_workaround_state()) {
>>>> + case ARM64_BP_HARDEN_MITIGATED:
>>>> return sprintf(buf, "Not affected\n");
>>>
>>> Here "mitigated" means "not affected".
>>>
>>>> -
>>>> - if (__hardenbp_enab)
>>>> + case ARM64_BP_HARDEN_NEEDED:
>>>> return sprintf(buf, "Mitigation: Branch predictor hardening\n");
>>>
>>> And "harden needed" means mitigated.
>>>
>>>> -
>>>> - return sprintf(buf, "Vulnerable\n");
>>>> + case ARM64_BP_HARDEN_UNKNOWN:
>>>> + default:
>>>> + return sprintf(buf, "Vulnerable\n");
>>>> + }
>>>> }
>>>>
>>>> ssize_t cpu_show_spec_store_bypass(struct device *dev,
>>>> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
>>>> index 34d08ee63747..1da53e0e38f7 100644
>>>> --- a/virt/kvm/arm/psci.c
>>>> +++ b/virt/kvm/arm/psci.c
>>>> @@ -412,8 +412,16 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>>>> feature = smccc_get_arg1(vcpu);
>>>> switch(feature) {
>>>> case ARM_SMCCC_ARCH_WORKAROUND_1:
>>>> - if (kvm_arm_harden_branch_predictor())
>>>> + switch (kvm_arm_harden_branch_predictor()) {
>>>> + case KVM_BP_HARDEN_UNKNOWN:
>>>> + break;
>>>> + case KVM_BP_HARDEN_NEEDED:
>>>> val = SMCCC_RET_SUCCESS;
>>>> + break;
>>>> + case KVM_BP_HARDEN_MITIGATED:
>>>> + val = SMCCC_RET_NOT_REQUIRED;
>>>
>>> Would KVM_BP_HARDEN_NOT_REQUIRED be a more logical name?
>> I tend to agree with Steven's comment
>>
>> But then why not also choosing the same terminology for the uapi:
>> KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED?
>
> You mean using ..._NOT_REQUIRED here?
yes sorry.
> Makes sense, as "unaffected" is different from "not required", and we
> cannot guarantee the first.
>
>> For the same case we seem to have 3 different terminologies. But maybe I
>> miss something.
>>
>> In the uapi doc, in case the workaround is not needed do we actually
>> care to mention the wa is supported?
>
> I think yes, as it's important to know that a guest could call into the
> "firmware", regardless of the effect.
OK
Thanks
Eric
>
> Cheers,
> Andre.
>
>> Thanks
>>
>> Eric
>>>
>>> Steve
>>>
>>>> + break;
>>>> + }
>>>> break;
>>>> case ARM_SMCCC_ARCH_WORKAROUND_2:
>>>> switch (kvm_arm_have_ssbd()) {
>>>>
>>>
>
