by James Morse

[permalink] [raw]

Subject: Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register

2019-01-31 16:27:52

by James Morse

[permalink] [raw]

Hi,

On 2/13/19 11:24 PM, Dave P Martin wrote:
> On Wed, Feb 13, 2019 at 05:35:46PM +0000, Kristina Martsenko wrote:
>> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>>> According to userspace settings, ptrauth key registers are conditionally
>>> present in guest system register list based on user specified flag
>>> KVM_ARM_VCPU_PTRAUTH.
>>>
>>> Signed-off-by: Amit Daniel Kachhap <[email protected]>
>>> Cc: Mark Rutland <[email protected]>
>>> Cc: Christoffer Dall <[email protected]>
>>> Cc: Marc Zyngier <[email protected]>
>>> Cc: Kristina Martsenko <[email protected]>
>>> Cc: [email protected]
>>> Cc: Ramana Radhakrishnan <[email protected]>
>>> Cc: Will Deacon <[email protected]>
>>> ---
>>> Documentation/arm64/pointer-authentication.txt | 3 ++
>>> arch/arm64/kvm/sys_regs.c | 42 +++++++++++++++++++-------
>>> 2 files changed, 34 insertions(+), 11 deletions(-)
>>>
>
> [...]
>
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>
> [...]
>
>>> @@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
>>> }
>>>
>>> /* Assumed ordered tables, see kvm_sys_reg_table_init. */
>>> -static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
>>> +static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
>>> + const struct sys_reg_desc *desc, unsigned int len)
>>> {
>>> const struct sys_reg_desc *i1, *i2, *end1, *end2;
>>> unsigned int total = 0;
>>> size_t num;
>>> int err;
>>>
>>> + if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
>>> + return total;
>>> +
>>> /* We check for duplicates here, to allow arch-specific overrides. */
>>> i1 = get_target_table(vcpu->arch.target, true, &num);
>>> end1 = i1 + num;
>>> - i2 = sys_reg_descs;
>>> - end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
>>> + i2 = desc;
>>> + end2 = desc + len;
>>>
>>> BUG_ON(i1 == end1 || i2 == end2);
>>>
>>> @@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
>>> {
>>> return ARRAY_SIZE(invariant_sys_regs)
>>> + num_demux_regs()
>>> - + walk_sys_regs(vcpu, (u64 __user *)NULL);
>>> + + walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
>>> + ARRAY_SIZE(sys_reg_descs))
>>> + + walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
>>> + ARRAY_SIZE(ptrauth_reg_descs));
>>> }
>>>
>>> int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>>> @@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>>> uindices++;
>>> }
>>>
>>> - err = walk_sys_regs(vcpu, uindices);
>>> + err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>>> + if (err < 0)
>>> + return err;
>>> + uindices += err;
>>> +
>>> + err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>>> if (err < 0)
>>> return err;
>>> uindices += err;
>>> @@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
>>> BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
>>> BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
>>> BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
>>> + BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
>>>
>>> /* We abuse the reset function to overwrite the table itself. */
>>> for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
>>> @@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>>>
>>> /* Generic chip reset first (so target could override). */
>>> reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>>> + reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>>>
>>> table = get_target_table(vcpu->arch.target, true, &num);
>>> reset_sys_reg_descs(vcpu, table, num);
>>
>> This isn't very scalable, since we'd need to duplicate all the above
>> code every time we add new system registers that are conditionally
>> accessible.
>
> Agreed, putting feature-specific checks in walk_sys_regs() is probably
> best avoided. Over time we would likely accumulate a fair number of
> these checks.
>
>> It seems that the SVE patches [1] solved this problem by adding a
>> check_present() callback into struct sys_reg_desc. It probably makes
>> sense to rebase onto that patch and just implement the callback for the
>> ptrauth key registers as well.
>>
>> [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> Note, I'm currently refactoring this so that enumeration through
> KVM_GET_REG_LIST can be disabled independently of access to the
> register. This may not be the best approach, but for SVE I have a
> feature-specific ID register to handle too (ID_AA64ZFR0_EL1), which
> needs to be hidden from the enumeration but still accessible with
> read-as-zero behaviour.
>
> This changes the API a bit: I move to a .restrictions() callback which
> returns flags to say what is disabled, and this callback is used in the
> common code so that you don't have repeat your "feature present" check
> in every accessor, as is currently the case.
>
> I'm aiming to post a respun series in the next day or two. The code
> may of course change again after it gets reviewed...
>
>
> Basing on [1] is probably a reasonable starting point, though.

Thanks Kristina and Dave for this pointer. I will rebase my next
iteration based on it.

Thanks,
Amit
>
> Cheers
> ---Dave
>

2019-02-15 16:38:29

by Kristina Martsenko

[permalink] [raw]

Subject: Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value

On 14/02/2019 11:03, Amit Daniel Kachhap wrote:
> Hi,
>
> On 2/13/19 11:04 PM, Kristina Martsenko wrote:
>> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>>> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which
>>> is a constant value. This works today, as the host HCR_EL2 value is
>>> always the same, but this will get in the way of supporting extensions
>>> that require HCR_EL2 bits to be set conditionally for the host.
>>>
>>> To allow such features to work without KVM having to explicitly handle
>>> every possible host feature combination, this patch has KVM save/restore
>>> the host HCR when switching to/from a guest HCR. The saving of the
>>> register is done once during cpu hypervisor initialization state and is
>>> just restored after switch from guest.
>>
>> Why is this patch needed? I couldn't find anything in this series that
>> sets HCR_EL2 conditionally for the host. It seems like the kernel still
>> always sets it to HCR_HOST_VHE_FLAGS/HCR_HOST_NVHE_FLAGS.
>
> This patch is not directly related to pointer authentication but just a
> helper to optimize save/restore. In this way save may be avoided for
> each switch and only restore is done. Patch 3 does sets HCR_EL2 in VHE_RUN.

Patch 3 sets the HCR_EL2.{API,APK} bits for the *guest*, not the host.
This patch here adds saving/restoring for the *host* HCR_EL2. As far as
I can tell, the value of the host HCR_EL2 never changes.

Regarding save/restore, currently the kernel never saves the host
HCR_EL2, because it always restores HCR_EL2 to HCR_HOST_{,N}VHE_FLAGS (a
constant value!) when returning to the host. With this patch, we
effectively just save HCR_HOST_{,N}VHE_FLAGS into kvm_host_cpu_state,
and restore it from there when returning to the host.

Unless we actually change the host HCR_EL2 value to something other than
HCR_HOST_{,N}VHE_FLAGS somewhere in this series, this patch is unnecessary.

>>
>> Looking back at v2 of the userspace pointer auth series, it seems that
>> the API/APK bits were set conditionally [1], so this patch would have
>> been needed to preserve HCR_EL2. But as of v3 of that series, the bits
>> have been set unconditionally through HCR_HOST_NVHE_FLAGS [2].
>>
>> Is there something else I've missed?
> Now HCR_EL2 is modified during switch time and NHVE doesnt support
> ptrauth so [2] doesn't makes sense.

In case of NVHE, we do support pointer auth in the *host* userspace, so
the patch [2] is necessary. In case of NVHE we do not support pointer
auth for KVM *guests*.

Thanks,
Kristina

>> [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
>> [2] https://lore.kernel.org/linux-arm-kernel/[email protected]/