Messages in this thread | | | Subject | Re: [PATCH v6 10/10] arm64: Retrieve stolen time as paravirtualized guest | From | Jürgen Groß <> | Date | Mon, 21 Oct 2019 07:01:21 +0200 |
| |
On 19.10.19 22:28, Marc Zyngier wrote: > On Fri, 11 Oct 2019 13:59:30 +0100, > Steven Price <steven.price@arm.com> wrote: >> >> Enable paravirtualization features when running under a hypervisor >> supporting the PV_TIME_ST hypercall. >> >> For each (v)CPU, we ask the hypervisor for the location of a shared >> page which the hypervisor will use to report stolen time to us. We set >> pv_time_ops to the stolen time function which simply reads the stolen >> value from the shared page for a VCPU. We guarantee single-copy >> atomicity using READ_ONCE which means we can also read the stolen >> time for another VCPU than the currently running one while it is >> potentially being updated by the hypervisor. >> >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> .../admin-guide/kernel-parameters.txt | 6 +- >> arch/arm64/include/asm/paravirt.h | 9 +- >> arch/arm64/kernel/paravirt.c | 148 ++++++++++++++++++ >> arch/arm64/kernel/time.c | 3 + >> include/linux/cpuhotplug.h | 1 + >> 5 files changed, 163 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index c7ac2f3ac99f..346b1c7a4afb 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -3083,9 +3083,9 @@ >> [X86,PV_OPS] Disable paravirtualized VMware scheduler >> clock and use the default one. >> >> - no-steal-acc [X86,KVM] Disable paravirtualized steal time accounting. >> - steal time is computed, but won't influence scheduler >> - behaviour >> + no-steal-acc [X86,KVM,ARM64] Disable paravirtualized steal time >> + accounting. steal time is computed, but won't >> + influence scheduler behaviour >> >> nolapic [X86-32,APIC] Do not enable or use the local APIC. >> >> diff --git a/arch/arm64/include/asm/paravirt.h b/arch/arm64/include/asm/paravirt.h >> index 799d9dd6f7cc..125c26c42902 100644 >> --- a/arch/arm64/include/asm/paravirt.h >> +++ b/arch/arm64/include/asm/paravirt.h >> @@ -21,6 +21,13 @@ static inline u64 paravirt_steal_clock(int cpu) >> { >> return pv_ops.time.steal_clock(cpu); >> } >> -#endif >> + >> +int __init kvm_guest_init(void); >> + >> +#else >> + >> +#define kvm_guest_init() >> + >> +#endif // CONFIG_PARAVIRT >> >> #endif >> diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c >> index 4cfed91fe256..de73dbec238c 100644 >> --- a/arch/arm64/kernel/paravirt.c >> +++ b/arch/arm64/kernel/paravirt.c >> @@ -6,13 +6,161 @@ >> * Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> */ >> >> +#define pr_fmt(fmt) "kvmarm-pv: " fmt >> + >> +#include <linux/arm-smccc.h> >> +#include <linux/cpuhotplug.h> >> #include <linux/export.h> >> +#include <linux/io.h> >> #include <linux/jump_label.h> >> +#include <linux/printk.h> >> +#include <linux/psci.h> >> +#include <linux/reboot.h> >> +#include <linux/slab.h> >> #include <linux/types.h> >> + >> #include <asm/paravirt.h> >> +#include <asm/pvclock-abi.h> >> +#include <asm/smp_plat.h> >> >> struct static_key paravirt_steal_enabled; >> struct static_key paravirt_steal_rq_enabled; >> >> struct paravirt_patch_template pv_ops; >> EXPORT_SYMBOL_GPL(pv_ops); >> + >> +struct kvmarm_stolen_time_region { >> + struct pvclock_vcpu_stolen_time *kaddr; >> +}; >> + >> +static DEFINE_PER_CPU(struct kvmarm_stolen_time_region, stolen_time_region); >> + >> +static bool steal_acc = true; >> +static int __init parse_no_stealacc(char *arg) >> +{ >> + steal_acc = false; >> + return 0; >> +} >> + >> +early_param("no-steal-acc", parse_no_stealacc); >> + >> +/* return stolen time in ns by asking the hypervisor */ >> +static u64 kvm_steal_clock(int cpu) > > This isn't KVM specific. > >> +{ >> + struct kvmarm_stolen_time_region *reg; >> + >> + reg = per_cpu_ptr(&stolen_time_region, cpu); >> + if (!reg->kaddr) { >> + pr_warn_once("stolen time enabled but not configured for cpu %d\n", >> + cpu); >> + return 0; >> + } >> + >> + return le64_to_cpu(READ_ONCE(reg->kaddr->stolen_time)); >> +} >> + >> +static int disable_stolen_time_current_cpu(void) >> +{ >> + struct kvmarm_stolen_time_region *reg; >> + >> + reg = this_cpu_ptr(&stolen_time_region); >> + if (!reg->kaddr) >> + return 0; >> + >> + memunmap(reg->kaddr); >> + memset(reg, 0, sizeof(*reg)); >> + >> + return 0; >> +} >> + >> +static int stolen_time_dying_cpu(unsigned int cpu) >> +{ >> + return disable_stolen_time_current_cpu(); >> +} > > You can merge these two functions, as there is no other caller. > >> + >> +static int init_stolen_time_cpu(unsigned int cpu) >> +{ >> + struct kvmarm_stolen_time_region *reg; >> + struct arm_smccc_res res; >> + >> + reg = this_cpu_ptr(&stolen_time_region); >> + >> + arm_smccc_1_1_invoke(ARM_SMCCC_HV_PV_TIME_ST, &res); >> + >> + if ((long)res.a0 < 0) >> + return -EINVAL; > > I'd rather you check an actual error code, just in case the memory map > starts growing to a point where we have 64bit (I)PAs... > >> + >> + reg->kaddr = memremap(res.a0, >> + sizeof(struct pvclock_vcpu_stolen_time), >> + MEMREMAP_WB); >> + >> + if (!reg->kaddr) { >> + pr_warn("Failed to map stolen time data structure\n"); >> + return -ENOMEM; >> + } >> + >> + if (le32_to_cpu(reg->kaddr->revision) != 0 || >> + le32_to_cpu(reg->kaddr->attributes) != 0) { >> + pr_warn("Unexpected revision or attributes in stolen time data\n"); > > WARN_ONCE instead? You probably don't want to scream for each and > every CPU that boots... > >> + return -ENXIO; >> + } >> + >> + return 0; >> +} >> + >> +static int kvm_arm_init_stolen_time(void) >> +{ >> + int ret; >> + >> + ret = cpuhp_setup_state(CPUHP_AP_ARM_KVMPV_STARTING, >> + "hypervisor/kvmarm/pv:starting", >> + init_stolen_time_cpu, stolen_time_dying_cpu); >> + if (ret < 0) >> + return ret; >> + return 0; >> +} >> + >> +static bool has_kvm_steal_clock(void) > > This is not KVM specific either. > >> +{ >> + struct arm_smccc_res res; >> + >> + /* To detect the presence of PV time support we require SMCCC 1.1+ */ >> + if (psci_ops.smccc_version < SMCCC_VERSION_1_1) >> + return false; >> + >> + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, >> + ARM_SMCCC_HV_PV_TIME_FEATURES, &res); >> + >> + if (res.a0 != SMCCC_RET_SUCCESS) >> + return false; >> + >> + arm_smccc_1_1_invoke(ARM_SMCCC_HV_PV_TIME_FEATURES, >> + ARM_SMCCC_HV_PV_TIME_ST, &res); >> + >> + if (res.a0 != SMCCC_RET_SUCCESS) >> + return false; >> + >> + return true; > > return (res.a0 == SMCCC_RET_SUCCESS); > >> +} >> + >> +int __init kvm_guest_init(void) > > How about something like pv_time_init() instead? In the guest, this is > no way KVM specific, and I still hope for this to work on things like > Xen/HyperV/VMware (yeah, I'm foolishly optimistic). All the references > to KVM should go, and be replaced by something more generic (after > all, you're only implementing the spec, so feel free to call it > den0057_* if you really want).
Xen guests already have the needed functionality. On ARM this just needs to be hooked up.
Juergen
| |