Re: [PATCH v5 5/5] arm64/kvm: control accessibility of ptrauth key registers
Hi,
On 2/13/19 11:24 PM, Dave P Martin wrote:
On Wed, Feb 13, 2019 at 05:35:46PM +, 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
Cc: Mark Rutland
Cc: Christoffer Dall
Cc: Marc Zyngier
Cc: Kristina Martsenko
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan
Cc: Will Deacon
---
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/1547757219-19439-13-git-send-email-dave.mar...@arm.com/
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
Re: [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
Hi,
On 2/13/19 11:05 PM, Kristina Martsenko wrote:
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
This feature will allow the KVM guest to allow the handling of
pointer authentication instructions or to treat them as undefined
if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
supply this parameter instead of creating a new API.
A new register is not created to pass this parameter via
SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
supplied is enough to enable this feature.
[...]
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index b200c14..b6950df 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -346,6 +346,10 @@ static inline int kvm_arm_have_ssbd(void)
static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu) {}
+static inline bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
+{
+ return false;
+}
It seems like this is only ever called from arm64 code, so do we need an
arch/arm/ definition?
Yes not required. Nice catch.
+/**
+ * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
+ *
+ * @vcpu: The VCPU pointer
+ *
+ * This function will be used to enable/disable ptrauth in guest as configured
+ * by the KVM userspace API.
+ */
+bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
+{
+ return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features);
+}
I'm not sure, but should there also be something like
if (test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features) &&
!kvm_supports_ptrauth())
return -EINVAL;
in kvm_reset_vcpu?
Yes makes sense. I missed it and with Dave martin patch this may be done
cleanly.
Thanks,
Amit D
Thanks,
Kristina
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register
Hi,
On 2/13/19 11:05 PM, Kristina Martsenko wrote:
On 31/01/2019 16:25, James Morse wrote:
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
When pointer authentication is supported, a guest may wish to use it.
This patch adds the necessary KVM infrastructure for this to work, with
a semi-lazy context switch of the pointer auth state.
[...]
+void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
+ struct kvm_cpu_context *host_ctxt,
+ struct kvm_cpu_context *guest_ctxt)
+{
+ if (!__ptrauth_is_enabled(vcpu))
+ return;
+
+ ptrauth_keys_store((struct ptrauth_keys *)
&host_ctxt->sys_regs[APIAKEYLO_EL1]);
We can't cast part of an array to a structure like this. What happens if the
compiler inserts padding in struct-ptrauth_keys, or the struct randomization
thing gets hold of it: https://lwn.net/Articles/722293/
If we want to use the helpers that take a struct-ptrauth_keys, we need to keep
the keys in a struct-ptrauth_keys. To do this we'd need to provide accessors so
that GET_ONE_REG() of APIAKEYLO_EL1 comes from the struct-ptrauth_keys, instead
of the sys_reg array.
If I've understood correctly, the idea is to have a struct ptrauth_keys
in struct kvm_vcpu_arch, instead of having the keys in the
kvm_cpu_context->sys_regs array. This is to avoid having similar code in
__ptrauth_key_install/ptrauth_keys_switch and
__ptrauth_restore_key/__ptrauth_restore_state, and so that future
patches (that add pointer auth in the kernel) would only need to update
one place instead of two.
Yes your observation is correct.
But it also means we'll have to special case pointer auth in
kvm_arm_sys_reg_set_reg/kvm_arm_sys_reg_get_reg and kvm_vcpu_arch. Is it
worth it? I'd prefer to keep the slight code duplication but avoid the
special casing.
In my local implementation I implemented above by separating ptrauth
registers from sys registers but if I use the new way suggested by Dave
[1] then those things are not possible as reg ID is used for matching
entries.
So I will stick to the single sys_reg list for next iteration using [1].
[1]:
https://lore.kernel.org/linux-arm-kernel/1547757219-19439-11-git-send-email-dave.mar...@arm.com/
Wouldn't the host keys be available somewhere else? (they must get transfer to
secondary CPUs somehow). Can we skip the save step when switching from the host?
+ ptrauth_keys_switch((struct ptrauth_keys *)
&guest_ctxt->sys_regs[APIAKEYLO_EL1]);
+}
[...]
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 03b36f1..301d332 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -483,6 +483,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
sysreg_restore_guest_state_vhe(guest_ctxt);
__debug_switch_to_guest(vcpu);
+ __ptrauth_switch_to_guest(vcpu, host_ctxt, guest_ctxt);
+
__set_guest_arch_workaround_state(vcpu);
do {
@@ -494,6 +496,8 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
__set_host_arch_workaround_state(vcpu);
+ __ptrauth_switch_to_host(vcpu, host_ctxt, guest_ctxt);
+
sysreg_save_guest_state_vhe(guest_ctxt);
__deactivate_traps(vcpu);
...This makes me nervous...
__guest_enter() is a function that (might) change the keys, then we change them
again here. We can't have any signed return address between these two points. I
don't trust the compiler not to generate any.
~
I had a chat with some friendly compiler folk... because there are two identical
sequences in kvm_vcpu_run_vhe() and __kvm_vcpu_run_nvhe(), the compiler could
move the common code to a function it then calls. Apparently this is called
'function outlining'.
If the compiler does this, and the guest changes the keys, I think we would fail
the return address check.
Painting the whole thing with no_prauth would solve this, but this code then
becomes a target.
Because the compiler can't anticipate the keys changing, we ought to treat them
the same way we do the callee saved registers, stack-pointer etc, and
save/restore them in the __guest_enter() assembly code.
(we can still keep the save/restore in C, but call it from assembly so we know
nothing new is going on the stack).
I agree that this should be called from assembly if we were building the
kernel with pointer auth. But as we are not doing that yet in this
series, can't we keep the calls in kvm_vcpu_run_vhe for now?
Well if we keep them in kvm_vcpu_run_vhe then there is not much issue
also in calling those C functions from assembly guest_enter/guest_exit.
It works fine in my local implementation. This will also save code
churning again when kernel ptrauth support is added. The only extra
change required to be done is to assign attribute _noptrauth to those
functions. I will add these comments properly in function description.
In general I would prefer if the keys were switched in
kvm_arc
Re: [PATCH V2 00/10] X86/KVM/Hyper-V: Add HV ept tlb range list flush support in KVM
On 02/02/19 02:38, lantianyu1...@gmail.com wrote: > From: Lan Tianyu > > This patchset is to introduce hv ept tlb range list flush function > support in the KVM MMU component. Flushing ept tlbs of several address > range can be done via single hypercall and new list flush function is > used in the kvm_mmu_commit_zap_page() and FNAME(sync_page). This patchset > also adds more hv ept tlb range flush support in more KVM MMU function. > > Change since v1: >1) Make flush list as a hlist instead of list in order to >keep struct kvm_mmu_page size. >2) Add last_level flag in the struct kvm_mmu_page instead >of spte pointer >3) Move tlb flush from kvm_mmu_notifier_clear_flush_young() to > kvm_age_hva() >4) Use range flush in the kvm_vm_ioctl_get/clear_dirty_log() Looks good except for the confusion on sp->last_level (which was mostly mine---sorry about that). I think it can still make 5.1. However, note that I've never received "KVM/MMU: Use tlb range flush in the kvm_age_hva()". Paolo ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH V2 3/10] KVM/MMU: Add last_level in the struct mmu_spte_page
On 02/02/19 02:38, lantianyu1...@gmail.com wrote:
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index ce770b446238..70cafd3f95ab 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2918,6 +2918,9 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>
> if (level > PT_PAGE_TABLE_LEVEL)
> spte |= PT_PAGE_SIZE_MASK;
> +
> + sp->last_level = is_last_spte(spte, level);
Wait, I wasn't thinking straight. If a struct kvm_mmu_page exists, it
is never the last level. Page table entries for the last level do not
have a struct kvm_mmu_page.
Therefore you don't need the flag after all. I suspect your
calculations in patch 2 are off by one, and you actually need
hlist_for_each_entry(sp, range->flush_list, flush_link) {
int pages = KVM_PAGES_PER_HPAGE(sp->role.level + 1);
...
}
For example, if sp->role.level is 1 then the struct kvm_mmu_page is for
a page containing PTEs and covers an area of 2 MiB.
Thanks,
Paolo
> if (tdp_enabled)
> spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn,
> kvm_is_mmio_pfn(pfn));
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH V2 3/10] KVM/MMU: Add last_level in the struct mmu_spte_page
On 02/02/19 02:38, lantianyu1...@gmail.com wrote: > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index ce770b446238..70cafd3f95ab 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2918,6 +2918,9 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, > > if (level > PT_PAGE_TABLE_LEVEL) > spte |= PT_PAGE_SIZE_MASK; > + > + sp->last_level = is_last_spte(spte, level); sp->last_level is always true here. Paolo > if (tdp_enabled) > spte |= kvm_x86_ops->get_mt_mask(vcpu, gfn, > kvm_is_mmio_pfn(pfn)); ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 5/5] KVM: arm/arm64: support chained PMU counters
On 05/02/2019 14:33, Julien Thierry wrote:
Hi Andrew,
On 04/02/2019 16:53, Andrew Murray wrote:
Emulate chained PMU counters by creating a single 64 bit event counter
for a pair of chained KVM counters.
Signed-off-by: Andrew Murray
---
include/kvm/arm_pmu.h | 1 +
virt/kvm/arm/pmu.c| 321 +-
2 files changed, 269 insertions(+), 53 deletions(-)
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index b73f31b..8e691ee 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -29,6 +29,7 @@ struct kvm_pmc {
u8 idx; /* index into the pmu->pmc array */
struct perf_event *perf_event;
u64 bitmask;
+ u64 overflow_count;
};
struct kvm_pmu {
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index a64aeb2..9318130 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -24,9 +24,25 @@
#include
#include
+#define ARMV8_PMUV3_PERFCTR_CHAIN 0x1E
I find it a bit awkward to have this redefined here.
Maybe we could define a helper in kvm_host.h:
bool kvm_pmu_typer_is_chain(u64 typer);
That would always return false for arm32?
We don't support ARMv7 host, so that doesn't matter. But
it is a good idea to wrap it in a function here.
kvm_vcpu_kick(vcpu);
@@ -440,6 +632,10 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu
*vcpu, u64 select_idx)
select_idx != ARMV8_PMU_CYCLE_IDX)
return;
+ /* Handled by even event */
+ if (eventsel == ARMV8_PMUV3_PERFCTR_CHAIN)
+ return;
+
memset(&attr, 0, sizeof(struct perf_event_attr));
attr.type = PERF_TYPE_RAW;
attr.size = sizeof(attr);
@@ -452,6 +648,9 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu
*vcpu, u64 select_idx)
attr.config = (select_idx == ARMV8_PMU_CYCLE_IDX) ?
ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
+ if (kvm_pmu_event_is_chained(vcpu, select_idx))
+ attr.config1 |= 0x1;
I'm not very familiar with the usage of perf attributes configs, but is
there any chance we could name this flag? Even if only for the local
file? Something like PERF_ATTR_CFG1_KVM_PMU_CHAINED (unless there is an
existing naming convention for event attributes).
There is ARPMU_EVT_64BIT in "linux/perf/arm_pmu.h". But in general we don't
specify where the Bit goes in (i.e, CFG1 etc).
Cheers
Suzuki
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 4/5] KVM: arm/arm64: lazily create perf events on enable
Hi Andrew,
On 04/02/2019 16:53, Andrew Murray wrote:
To prevent re-creating perf events everytime the counter registers
are changed, let's instead lazily create the event when the event
is first enabled and destroy it when it changes.
Signed-off-by: Andrew Murray
---
virt/kvm/arm/pmu.c | 96 ++
1 file changed, 68 insertions(+), 28 deletions(-)
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 95d74ec..a64aeb2 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -24,7 +24,10 @@
#include
#include
+static void kvm_pmu_sync_counter_enable(struct kvm_vcpu *vcpu, u64 select_idx);
static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
+static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
+
/**
* kvm_pmu_get_counter_value - get PMU counter value
* @vcpu: The vcpu pointer
@@ -59,13 +62,15 @@ u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64
select_idx)
void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
{
u64 reg;
+ struct kvm_pmu *pmu = &vcpu->arch.pmu;
+ struct kvm_pmc *pmc = &pmu->pmc[select_idx];
reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
? PMCCNTR_EL0 : PMEVCNTR0_EL0 + select_idx;
__vcpu_sys_reg(vcpu, reg) += (s64)val - kvm_pmu_get_counter_value(vcpu,
select_idx);
- /* Recreate the perf event to reflect the updated sample_period */
- kvm_pmu_create_perf_event(vcpu, select_idx);
+ kvm_pmu_stop_counter(vcpu, pmc);
+ kvm_pmu_sync_counter_enable(vcpu, select_idx);
}
/**
@@ -83,6 +88,7 @@ static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc)
/**
* kvm_pmu_stop_counter - stop PMU counter
+ * @vcpu: The vcpu pointer
* @pmc: The PMU counter pointer
*
* If this counter has been configured to monitor some event, release it here.
@@ -143,6 +149,24 @@ u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu)
}
/**
+ * kvm_pmu_enable_counter - create/enable a counter
+ * @vcpu: The vcpu pointer
+ * @select_idx: The counter index
+ */
+static void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u64 select_idx)
+{
+ struct kvm_pmu *pmu = &vcpu->arch.pmu;
+ struct kvm_pmc *pmc = &pmu->pmc[select_idx];
+
+ if (!pmc->perf_event)
+ kvm_pmu_create_perf_event(vcpu, select_idx);
+
+ perf_event_enable(pmc->perf_event);
+ if (pmc->perf_event->state != PERF_EVENT_STATE_ACTIVE)
+ kvm_debug("failed to enable perf event\n");
+}
+
+/**
* kvm_pmu_enable_counter_mask - enable selected PMU counters
* @vcpu: The vcpu pointer
* @val: the value guest writes to PMCNTENSET register
@@ -152,8 +176,6 @@ u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu)
void kvm_pmu_enable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
{
int i;
- struct kvm_pmu *pmu = &vcpu->arch.pmu;
- struct kvm_pmc *pmc;
if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E) || !val)
return;
@@ -162,16 +184,39 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu *vcpu,
u64 val)
if (!(val & BIT(i)))
continue;
- pmc = &pmu->pmc[i];
- if (pmc->perf_event) {
- perf_event_enable(pmc->perf_event);
- if (pmc->perf_event->state != PERF_EVENT_STATE_ACTIVE)
- kvm_debug("fail to enable perf event\n");
- }
+ kvm_pmu_enable_counter(vcpu, i);
}
}
/**
+ * kvm_pmu_sync_counter_enable - reenable a counter if it should be enabled
+ * @vcpu: The vcpu pointer
+ * @select_idx: The counter index
+ */
+static void kvm_pmu_sync_counter_enable(struct kvm_vcpu *vcpu,
+ u64 select_idx)
+{
+ u64 set = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
+
+ if (set & BIT(select_idx))
+ kvm_pmu_enable_counter_mask(vcpu, BIT(select_idx));
I think there is a problem here. We could be creating an event for a
counter beyond what is supported by the CPU. i.e, we don't
seem to validate that the mask we are creating is within the
kvm_pmu_valid_counter_mask(). The other callers seem to verify this.
I guess it may be better to add a check for this in the
kvm_pmu_enable_counter_mask().
minor nit: Feel free to ignore. If we move the check for PMCNTENSET_EL0 to
pmu_enable_counter_mask() we could get rid of the above function. Anyways,
we should only be enabling the counters set in PMCNTENSET_EL0.
+}
+
+/**
+ * kvm_pmu_disable_counter - disable selected PMU counter
+ * @vcpu: The vcpu pointer
+ * @pmc: The counter to disable
+ */
+static void kvm_pmu_disable_counter(struct kvm_vcpu *vcpu, u64 select_idx)
+{
+ struct kvm_pmu *pmu = &vcpu->arch.pmu;
+ struct kvm_pmc *pmc = &pmu->pmc[select_idx];
+
+ if (pmc->perf_event)
+ perf_event_disable(pmc->perf_event);
+}
Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth registers
Hi,
On 2/13/19 11:04 PM, Kristina Martsenko wrote:
Hi Amit,
(Please always Cc: everyone who commented on previous versions of the
series.)
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
When pointer authentication is supported, a guest may wish to use it.
This patch adds the necessary KVM infrastructure for this to work, with
a semi-lazy context switch of the pointer auth state.
Pointer authentication feature is only enabled when VHE is built
into the kernel and present into CPU implementation so only VHE code
paths are modified.
When we schedule a vcpu, we disable guest usage of pointer
authentication instructions and accesses to the keys. While these are
disabled, we avoid context-switching the keys. When we trap the guest
trying to use pointer authentication functionality, we change to eagerly
context-switching the keys, and enable the feature. The next time the
vcpu is scheduled out/in, we start again.
Pointer authentication consists of address authentication and generic
authentication, and CPUs in a system might have varied support for
either. Where support for either feature is not uniform, it is hidden
from guests via ID register emulation, as a result of the cpufeature
framework in the host.
Unfortunately, address authentication and generic authentication cannot
be trapped separately, as the architecture provides a single EL2 trap
covering both. If we wish to expose one without the other, we cannot
prevent a (badly-written) guest from intermittently using a feature
which is not uniformly supported (when scheduled on a physical CPU which
supports the relevant feature). When the guest is scheduled on a
physical CPU lacking the feature, these attempts will result in an UNDEF
being taken by the guest.
[...]
/*
+ * Handle the guest trying to use a ptrauth instruction, or trying to access a
+ * ptrauth register.
+ */
+void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
+{
+ if (has_vhe() && kvm_supports_ptrauth())
+ kvm_arm_vcpu_ptrauth_enable(vcpu);
+ else
+ kvm_inject_undefined(vcpu);
+}
+
+/*
* Guest usage of a ptrauth instruction (which the guest EL1 did not turn into
- * a NOP).
+ * a NOP), or guest EL1 access to a ptrauth register.
Doesn't guest EL1 access of ptrauth registers go through trap_ptrauth
instead?
Yes you are right.
*/
static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu, struct kvm_run *run)
{
- /*
-* We don't currently support ptrauth in a guest, and we mask the ID
-* registers to prevent well-behaved guests from trying to make use of
-* it.
-*
-* Inject an UNDEF, as if the feature really isn't present.
-*/
- kvm_inject_undefined(vcpu);
+ kvm_arm_vcpu_ptrauth_trap(vcpu);
return 1;
}
[...]
+static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu
*vcpu)
+{
+ return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
+ vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
+}
+
+void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
+ struct kvm_cpu_context *host_ctxt,
+ struct kvm_cpu_context *guest_ctxt)
+{
+ if (!__ptrauth_is_enabled(vcpu))
+ return;
+
+ ptrauth_keys_store((struct ptrauth_keys *)
&host_ctxt->sys_regs[APIAKEYLO_EL1]);
+ ptrauth_keys_switch((struct ptrauth_keys *)
&guest_ctxt->sys_regs[APIAKEYLO_EL1]);
+}
+
+void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct kvm_vcpu *vcpu,
We don't call this code in the !VHE case anymore, so are the __hyp_text
annotations still needed?
Yes they can be removed.
+struct kvm_cpu_context *host_ctxt,
+struct kvm_cpu_context *guest_ctxt)
+{
+ if (!__ptrauth_is_enabled(vcpu))
+ return;
+
+ ptrauth_keys_store((struct ptrauth_keys *)
&guest_ctxt->sys_regs[APIAKEYLO_EL1]);
+ ptrauth_keys_switch((struct ptrauth_keys *)
&host_ctxt->sys_regs[APIAKEYLO_EL1]);
+}
[...]
@@ -1040,14 +1066,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r,
bool raz)
kvm_debug("SVE unsupported for guests, suppressing\n");
val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
- } else if (id == SYS_ID_AA64ISAR1_EL1) {
- const u64 ptrauth_mask = (0xfUL << ID_AA64ISAR1_APA_SHIFT) |
-(0xfUL << ID_AA64ISAR1_API_SHIFT) |
-(0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
-(0xfUL << ID_AA64ISAR1_GPI_SHIFT);
- if (val & ptrauth_mask)
- kvm_debug("ptrauth unsupported for guests,
suppressing\n");
- val &= ~ptrauth_mask;
If all CPUs support address authentication, but no CPUs support generic
authentication, then I think the guest wil
Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
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. 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. //Amit D Thanks, Kristina [1] https://lore.kernel.org/linux-arm-kernel/20171127163806.31435-6-mark.rutl...@arm.com/ [2] https://lore.kernel.org/linux-arm-kernel/20180417183735.56985-5-mark.rutl...@arm.com/ ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 1/5] arm64: Add utilities to save restore pointer authentication keys
Hi,
On 2/13/19 11:02 PM, Kristina Martsenko wrote:
On 31/01/2019 16:20, James Morse wrote:
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
The keys can be switched either inside an assembly or such
functions which do not have pointer authentication checks, so a GCC
attribute is added to enable it.
A function ptrauth_keys_store is added which is similar to existing
function ptrauth_keys_switch but saves the key values in memory.
This may be useful for save/restore scenarios when CPU changes
privilege levels, suspend/resume etc.
diff --git a/arch/arm64/include/asm/pointer_auth.h
b/arch/arm64/include/asm/pointer_auth.h
index 15d4951..98441ce 100644
--- a/arch/arm64/include/asm/pointer_auth.h
+++ b/arch/arm64/include/asm/pointer_auth.h
@@ -11,6 +11,13 @@
#ifdef CONFIG_ARM64_PTR_AUTH
/*
+ * Compile the function without pointer authentication instructions. This
+ * allows pointer authentication to be enabled/disabled within the function
+ * (but leaves the function unprotected by pointer authentication).
+ */
+#define __no_ptrauth __attribute__((target("sign-return-address=none")))
The documentation[0] for this says 'none' is the default. Will this only
take-effect once the kernel supports pointer-auth for the host? (Is this just
documentation until then?)
Yes, I don't think this should be in this series, since we're not
building the kernel with pointer auth yet.
I added it to stress on some functions which should be pointer
authentication safe. Yes this can be dropped and a small comment may
also do.
//Amit D
('noptrauth' would fit with 'notrace' slightly better)
(But worse with e.g. __noreturn, __notrace_funcgraph, __init,
__always_inline, __exception. Not sure what the pattern is. Would
__noptrauth be better?)
Thanks,
Kristina
[0]
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Function-Attributes.html#AArch64-Function-Attributes
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 4/5] arm64/kvm: add a userspace option to enable pointer authentication
Hi,
On 1/31/19 9:57 PM, James Morse wrote:
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
This feature will allow the KVM guest to allow the handling of
pointer authentication instructions or to treat them as undefined
if not set. It uses the existing vcpu API KVM_ARM_VCPU_INIT to
supply this parameter instead of creating a new API.
A new register is not created to pass this parameter via
SET/GET_ONE_REG interface as just a flag (KVM_ARM_VCPU_PTRAUTH)
supplied is enough to enable this feature.
diff --git a/Documentation/arm64/pointer-authentication.txt
b/Documentation/arm64/pointer-authentication.txt
index a25cd21..0529a7d 100644
--- a/Documentation/arm64/pointer-authentication.txt
+++ b/Documentation/arm64/pointer-authentication.txt
@@ -82,7 +82,8 @@ pointers).
Virtualization
--
-Pointer authentication is not currently supported in KVM guests. KVM
-will mask the feature bits from ID_AA64ISAR1_EL1, and attempted use of
-the feature will result in an UNDEFINED exception being injected into
-the guest.
+Pointer authentication is enabled in KVM guest when virtual machine is
+created by passing a flag (KVM_ARM_VCPU_PTRAUTH)
Isn't that a VCPU flag? Shouldn't this be when each VCPU is created?
Yes it is a VCPU flag.
requesting this feature
+to be enabled. Without this flag, pointer authentication is not enabled
+in KVM guests and attempted use of the feature will result in an UNDEFINED
+exception being injected into the guest.
... what happens if KVM's user-space enables ptrauth on some vcpus, but not on
others?
Yes seems to be issue. Let me check more on this if there are other ways
of passing the userspace parameter such as in CREATE_VM type ioctl.
You removed the id-register suppression in the previous patch, but it doesn't
get hooked up to kvm_arm_vcpu_ptrauth_allowed() here. (you could add
kvm_arm_vcpu_ptrauth_allowed() earlier, and default it to true to make it
easier).
Doesn't this mean that if the CPU supports pointer auth, but user-space doesn't
specify this flag, the guest gets mysterious undef's whenever it tries to use
the advertised feature?
Agree, ID registers should be masked when userspace disables it.
(whether we support big/little virtual-machines is probably a separate issue,
but the id registers need to be consistent with our trap-and-undef behaviour)
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index c798d0c..4a6ec40 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -453,14 +453,15 @@ static inline bool kvm_arch_requires_vhe(void)
void kvm_arm_vcpu_ptrauth_enable(struct kvm_vcpu *vcpu);
void kvm_arm_vcpu_ptrauth_disable(struct kvm_vcpu *vcpu);
+bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu);
static inline void kvm_arm_vcpu_ptrauth_reset(struct kvm_vcpu *vcpu)
{
/* Disable ptrauth and use it in a lazy context via traps */
- if (has_vhe() && kvm_supports_ptrauth())
+ if (has_vhe() && kvm_supports_ptrauth()
+ && kvm_arm_vcpu_ptrauth_allowed(vcpu))
kvm_arm_vcpu_ptrauth_disable(vcpu);
}
-
void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 5b980e7..c0e5dcd 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -179,7 +179,8 @@ static int handle_sve(struct kvm_vcpu *vcpu, struct kvm_run
*run)
*/
void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu)
{
- if (has_vhe() && kvm_supports_ptrauth())
+ if (has_vhe() && kvm_supports_ptrauth()
+ && kvm_arm_vcpu_ptrauth_allowed(vcpu))
Duplication. If has_vhe() moved into kvm_supports_ptrauth(), and
kvm_supports_ptrauth() was called from kvm_arm_vcpu_ptrauth_allowed() it would
be clearer that use of this feature was becoming user-controlled policy.
(We don't need to list the dependencies at every call site)
ok.
diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
index 0576c01..369624f 100644
--- a/arch/arm64/kvm/hyp/ptrauth-sr.c
+++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
@@ -42,3 +42,16 @@ void __no_ptrauth __hyp_text __ptrauth_switch_to_host(struct
kvm_vcpu *vcpu,
ptrauth_keys_store((struct ptrauth_keys *)
&guest_ctxt->sys_regs[APIAKEYLO_EL1]);
ptrauth_keys_switch((struct ptrauth_keys *)
&host_ctxt->sys_regs[APIAKEYLO_EL1]);
}
+
+/**
+ * kvm_arm_vcpu_ptrauth_allowed - checks if ptrauth feature is present in vcpu
('enabled by KVM's user-space' may be clearer. 'Present in vcpu' could be down
to a cpufeature thing)
ok.
+ *
+ * @vcpu: The VCPU pointer
+ *
+ * This function will be used to enable/disable ptrauth in guest as configured
... but it just tests the bit ...
+ * by the KVM userspace API.
+ */
+bool kvm_arm_vcpu_ptrauth_allowed(struct kvm_vcpu *vcpu)
+{
+ return test_bit(KVM_ARM_VCPU_PTRAUTH, vcpu->arch.features
Re: [PATCH v5 3/5] arm64/kvm: context-switch ptrauth register
Hi James,
On 1/31/19 9:55 PM, James Morse wrote:
Hi Amit,
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
When pointer authentication is supported, a guest may wish to use it.
This patch adds the necessary KVM infrastructure for this to work, with
a semi-lazy context switch of the pointer auth state.
Pointer authentication feature is only enabled when VHE is built
into the kernel and present into CPU implementation so only VHE code
~s/into/in the/?
paths are modified.
When we schedule a vcpu, we disable guest usage of pointer
authentication instructions and accesses to the keys. While these are
disabled, we avoid context-switching the keys. When we trap the guest
trying to use pointer authentication functionality, we change to eagerly
context-switching the keys, and enable the feature. The next time the
vcpu is scheduled out/in, we start again.
Pointer authentication consists of address authentication and generic
authentication, and CPUs in a system might have varied support for
either. Where support for either feature is not uniform, it is hidden
from guests via ID register emulation, as a result of the cpufeature
framework in the host.
Unfortunately, address authentication and generic authentication cannot
be trapped separately, as the architecture provides a single EL2 trap
covering both. If we wish to expose one without the other, we cannot
prevent a (badly-written) guest from intermittently using a feature
which is not uniformly supported (when scheduled on a physical CPU which
supports the relevant feature). When the guest is scheduled on a
physical CPU lacking the feature, these attempts will result in an UNDEF
being taken by the guest.
This won't be fun. Can't KVM check that both are supported on all CPUs to avoid
this? ...
The above message is confusing as both checks actually present. I will
update.
diff --git a/arch/arm64/include/asm/cpufeature.h
b/arch/arm64/include/asm/cpufeature.h
index dfcfba7..e1bf2a5 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -612,6 +612,11 @@ static inline bool system_supports_generic_auth(void)
cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF));
}
+static inline bool kvm_supports_ptrauth(void)
+{
+ return system_supports_address_auth() && system_supports_generic_auth();
+}
... oh you do check. Could you cover this in the commit message? (to avoid an
UNDEF being taken by the guest we ... )
cpufeature.h is a strange place to put this, there are no other kvm symbols in
there. But there are users of system_supports_foo() in kvm_host.h.
ok will check.
diff --git a/arch/arm64/kvm/hyp/ptrauth-sr.c b/arch/arm64/kvm/hyp/ptrauth-sr.c
new file mode 100644
index 000..0576c01
--- /dev/null
+++ b/arch/arm64/kvm/hyp/ptrauth-sr.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * arch/arm64/kvm/hyp/ptrauth-sr.c: Guest/host ptrauth save/restore
+ *
+ * Copyright 2018 Arm Limited
+ * Author: Mark Rutland
+ * Amit Daniel Kachhap
+ */
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+static __always_inline bool __hyp_text __ptrauth_is_enabled(struct kvm_vcpu
*vcpu)
Why __always_inline? Doesn't the compiler decide for 'static' symbols in C
files?
This is to make the function pointer authentication safe. Although it
placed before key switch so may not be required.
+{
+ return IS_ENABLED(CONFIG_ARM64_PTR_AUTH) &&
+ vcpu->arch.ctxt.hcr_el2 & (HCR_API | HCR_APK);
+}
+
+void __no_ptrauth __hyp_text __ptrauth_switch_to_guest(struct kvm_vcpu *vcpu,
+ struct kvm_cpu_context *host_ctxt,
+ struct kvm_cpu_context *guest_ctxt)
+{
+ if (!__ptrauth_is_enabled(vcpu))
+ return;
+
+ ptrauth_keys_store((struct ptrauth_keys *)
&host_ctxt->sys_regs[APIAKEYLO_EL1]);
We can't cast part of an array to a structure like this. What happens if the
compiler inserts padding in struct-ptrauth_keys, or the struct randomization
thing gets hold of it: https://lwn.net/Articles/722293/
Yes this has got issue.
If we want to use the helpers that take a struct-ptrauth_keys, we need to keep
the keys in a struct-ptrauth_keys. To do this we'd need to provide accessors so
that GET_ONE_REG() of APIAKEYLO_EL1 comes from the struct-ptrauth_keys, instead
of the sys_reg array.
ok.
Wouldn't the host keys be available somewhere else? (they must get transfer to
secondary CPUs somehow). Can we skip the save step when switching from the host?
Yes Host save can be done during vcpu_load and it works fine. However it
does not work during hypervisor configuration stage(i.e where HCR_EL2 is
saved/restored now) as keys are different.
+ ptrauth_keys_switch((struct ptrauth_keys *)
&guest_ctxt->sys_regs[APIAKEYLO_EL1]);
+}
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 1f
Re: [PATCH] kvm: arm64: Fix comment for KVM_PHYS_SHIFT
Hi On 14/02/2019 01:45, Zenghui Yu wrote: Since Suzuki K Poulose's work on Dynamic IPA support, KVM_PHYS_SHIFT will be used only when machine_type's bits[7:0] equal to 0 (by default). Thus the outdated comment should be fixed. Cc: Suzuki K Poulose Signed-off-by: Zenghui Yu --- arch/arm64/include/asm/kvm_mmu.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h index 8af4b1b..a12bf48 100644 --- a/arch/arm64/include/asm/kvm_mmu.h +++ b/arch/arm64/include/asm/kvm_mmu.h @@ -138,7 +138,8 @@ static inline unsigned long __kern_hyp_va(unsigned long v) }) /* - * We currently only support a 40bit IPA. + * We currently support using a VM-specified IPA size. For backward + * compatibility, the default IPA size is fixed to 40bits. */ #define KVM_PHYS_SHIFT(40) Thanks for the fix. Reviewed-by: Suzuki K Poulose ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 2/5] arm64/kvm: preserve host HCR_EL2/MDCR_EL2 value
Hi James,
Little late in replying as some issue in my mail settings.
On 1/31/19 9:52 PM, James Morse wrote:
Hi Amit,
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.
For fetching HCR_EL2 during kvm initialisation, a hyp call is made using
kvm_call_hyp and is helpful in NHVE case.
For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated
to toggle the TGE bit with a RMW sequence, as we already do in
__tlb_switch_to_guest_vhe().
While at it, host MDCR_EL2 value is fetched in a similar way and restored
after every switch from host to guest. There should not be any change in
functionality due to this.
Could this step be done as a separate subsequent patch? It would make review
easier! The MDCR stuff would be a simplification if done second, done in one go
like this its pretty noisy.
Ok, agree.
There ought to be some justification for moving hcr/mdcr into the cpu_context in
the commit message.
ohh I missed adding in commit. Just added in cover letter.
If you're keeping Mark's 'Signed-off-by' its would be normal to keep Mark as the
author in git. This shows up a an extra 'From:' when you post the patch, and
gets picked up when the maintainer runs git-am.
This patch has changed substantially from Mark's version:
https://lkml.org/lkml/2017/11/27/675
If you keep the signed-off-by, could you add a [note] in the signed-off area
with a terse summary. Something like:
Signed-off-by: Mark Rutland
[ Move hcr to cpu_context, added __cpu_copy_hyp_conf()]
Signed-off-by: Amit Daniel Kachhap
(9c06602b1b92 is a good picked-at-random example for both of these)
Thanks for the information.
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index f5b79e9..2da6e43 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -80,6 +80,8 @@ extern void __vgic_v3_init_lrs(void);
extern u32 __kvm_get_mdcr_el2(void);
+extern u64 __kvm_get_hcr_el2(void);
Do we need these in separate helpers? For non-vhe this means two separate trips
to EL2. Something like kvm_populate_host_context(void), and an __ version for
the bit at EL2?
yes one wrapper for each of them will do.
We don't need to pass the host-context to EL2 as once kvm is loaded we can
access host per-cpu variables at EL2 using __hyp_this_cpu_read(). This will save
passing the vcpu around.
@@ -458,6 +457,25 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
static inline void __cpu_init_stage2(void) {}
+/**
+ * __cpu_copy_hyp_conf - copy the boot hyp configuration registers
+ *
+ * It is called once per-cpu during CPU hyp initialisation.
+ */
+static inline void __cpu_copy_hyp_conf(void)
+{
+ kvm_cpu_context_t *host_cxt = this_cpu_ptr(&kvm_host_cpu_state);
+
+ host_cxt->hcr_el2 = kvm_call_hyp(__kvm_get_hcr_el2);
+
+ /*
+* Retrieve the initial value of mdcr_el2 so we can preserve
+* MDCR_EL2.HPMN which has presumably been set-up by some
+* knowledgeable bootcode.
+*/
+ host_cxt->mdcr_el2 = kvm_call_hyp(__kvm_get_mdcr_el2);
+}
Its strange to make this an inline in a header. kvm_arm_init_debug() is a
static-inline for arch/arm, but a regular C function for arch/arm64. Can't we do
the same?
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 68d6f7c..22c854a 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -316,3 +316,14 @@ void __hyp_text __kvm_enable_ssbs(void)
"msr sctlr_el2, %0"
: "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS));
}
+
+/**
+ * __read_hyp_hcr_el2 - Returns hcr_el2 register value
+ *
+ * This function acts as a function handler parameter for kvm_call_hyp and
+ * may be called from EL1 exception level to fetch the register value.
+ */
+u64 __hyp_text __kvm_get_hcr_el2(void)
+{
+ return read_sysreg(hcr_el2);
+}
While I'm all in favour of kernel-doc comments for functions, it may be
over-kill in this case!
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 9e350fd3..2d65ada 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -1327,10 +1327,10 @@ static void cpu_hyp_reinit(void)
else
cpu_init_hyp_mode(NULL);
- kvm_arm_init_debug();
-
if (vgic_present)
kvm_vgic_init_cpu_hardware();
+
+ __cpu_copy_hyp_conf();
}
Was there a reason
