| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAhV-H4VOEbQLe0nPS99u_cQBbJOPi2eCQA+=0dAvQ4dT6-gpA@mail.gmail.com>
Date: Wed, 19 Nov 2025 11:01:21 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Bibo Mao <maobibo@...ngson.cn>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Tianrui Zhao <zhaotianrui@...ngson.cn>,
WANG Xuerui <kernel@...0n.name>, kvm@...r.kernel.org, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] LoongArch: KVM: Add preempt hint feature in
hypervisor side
On Wed, Nov 19, 2025 at 10:58 AM Bibo Mao <maobibo@...ngson.cn> wrote:
>
>
>
> On 2025/11/19 上午10:45, Huacai Chen wrote:
> > On Wed, Nov 19, 2025 at 9:23 AM Bibo Mao <maobibo@...ngson.cn> wrote:
> >>
> >>
> >>
> >> On 2025/11/18 下午8:46, Huacai Chen wrote:
> >>> Hi, Bibo,
> >>>
> >>> On Tue, Nov 18, 2025 at 4:07 PM Bibo Mao <maobibo@...ngson.cn> wrote:
> >>>>
> >>>> Feature KVM_FEATURE_PREEMPT_HINT is added to show whether vCPU is
> >>>> preempted or not. It is to help guest OS scheduling or lock checking
> >>>> etc. Here add KVM_FEATURE_PREEMPT_HINT feature and use one byte as
> >>>> preempted flag in steal time structure.
> >>>>
> >>>> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
> >>>> ---
> >>>> arch/loongarch/include/asm/kvm_host.h | 2 +
> >>>> arch/loongarch/include/asm/kvm_para.h | 5 +-
> >>>> arch/loongarch/include/uapi/asm/kvm.h | 1 +
> >>>> arch/loongarch/include/uapi/asm/kvm_para.h | 1 +
> >>>> arch/loongarch/kvm/vcpu.c | 54 +++++++++++++++++++++-
> >>>> arch/loongarch/kvm/vm.c | 5 +-
> >>>> 6 files changed, 65 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
> >>>> index 0cecbd038bb3..04c6dd171877 100644
> >>>> --- a/arch/loongarch/include/asm/kvm_host.h
> >>>> +++ b/arch/loongarch/include/asm/kvm_host.h
> >>>> @@ -163,6 +163,7 @@ enum emulation_result {
> >>>> #define LOONGARCH_PV_FEAT_UPDATED BIT_ULL(63)
> >>>> #define LOONGARCH_PV_FEAT_MASK (BIT(KVM_FEATURE_IPI) | \
> >>>> BIT(KVM_FEATURE_STEAL_TIME) | \
> >>>> + BIT(KVM_FEATURE_PREEMPT_HINT) |\
> >>>> BIT(KVM_FEATURE_USER_HCALL) | \
> >>>> BIT(KVM_FEATURE_VIRT_EXTIOI))
> >>>>
> >>>> @@ -250,6 +251,7 @@ struct kvm_vcpu_arch {
> >>>> u64 guest_addr;
> >>>> u64 last_steal;
> >>>> struct gfn_to_hva_cache cache;
> >>>> + u8 preempted;
> >>>> } st;
> >>>> };
> >>>>
> >>>> diff --git a/arch/loongarch/include/asm/kvm_para.h b/arch/loongarch/include/asm/kvm_para.h
> >>>> index 3e4b397f423f..d8592a7f5922 100644
> >>>> --- a/arch/loongarch/include/asm/kvm_para.h
> >>>> +++ b/arch/loongarch/include/asm/kvm_para.h
> >>>> @@ -37,8 +37,11 @@ struct kvm_steal_time {
> >>>> __u64 steal;
> >>>> __u32 version;
> >>>> __u32 flags;
> >>>> - __u32 pad[12];
> >>>> + __u8 preempted;
> >>>> + __u8 u8_pad[3];
> >>>> + __u32 pad[11];
> >>> Maybe a single __u8 pad[47] is enough?
> >> yes, pad[47] seems better unless there is definitely __u32 type
> >> requirement in future.
> >>
> >> Will do in next version.
> >>>
> >>>> };
> >>>> +#define KVM_VCPU_PREEMPTED (1 << 0)
> >>>>
> >>>> /*
> >>>> * Hypercall interface for KVM hypervisor
> >>>> diff --git a/arch/loongarch/include/uapi/asm/kvm.h b/arch/loongarch/include/uapi/asm/kvm.h
> >>>> index 57ba1a563bb1..bca7154aa651 100644
> >>>> --- a/arch/loongarch/include/uapi/asm/kvm.h
> >>>> +++ b/arch/loongarch/include/uapi/asm/kvm.h
> >>>> @@ -104,6 +104,7 @@ struct kvm_fpu {
> >>>> #define KVM_LOONGARCH_VM_FEAT_PV_IPI 6
> >>>> #define KVM_LOONGARCH_VM_FEAT_PV_STEALTIME 7
> >>>> #define KVM_LOONGARCH_VM_FEAT_PTW 8
> >>>> +#define KVM_LOONGARCH_VM_FEAT_PV_PREEMPT_HINT 10
> >>> From the name it is a "hint", from include/linux/kvm_para.h we know
> >>> features and hints are different. If preempt is really a feature,
> >>> rename it?
> >> It is a feature. yes, in generic hint is suggestion for VM and VM can
> >> selectively do or not.
> >>
> >> Will rename it with KVM_LOONGARCH_VM_FEAT_PV_PREEMPT.
> >>>
> >>>>
> >>>> /* Device Control API on vcpu fd */
> >>>> #define KVM_LOONGARCH_VCPU_CPUCFG 0
> >>>> diff --git a/arch/loongarch/include/uapi/asm/kvm_para.h b/arch/loongarch/include/uapi/asm/kvm_para.h
> >>>> index 76d802ef01ce..fe4107869ce6 100644
> >>>> --- a/arch/loongarch/include/uapi/asm/kvm_para.h
> >>>> +++ b/arch/loongarch/include/uapi/asm/kvm_para.h
> >>>> @@ -15,6 +15,7 @@
> >>>> #define CPUCFG_KVM_FEATURE (CPUCFG_KVM_BASE + 4)
> >>>> #define KVM_FEATURE_IPI 1
> >>>> #define KVM_FEATURE_STEAL_TIME 2
> >>>> +#define KVM_FEATURE_PREEMPT_HINT 3
> >>>> /* BIT 24 - 31 are features configurable by user space vmm */
> >>>> #define KVM_FEATURE_VIRT_EXTIOI 24
> >>>> #define KVM_FEATURE_USER_HCALL 25
> >>>> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> >>>> index 1245a6b35896..33a94b191b5d 100644
> >>>> --- a/arch/loongarch/kvm/vcpu.c
> >>>> +++ b/arch/loongarch/kvm/vcpu.c
> >>>> @@ -180,6 +180,11 @@ static void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
> >>>> }
> >>>>
> >>>> st = (struct kvm_steal_time __user *)ghc->hva;
> >>>> + if (kvm_guest_has_pv_feature(vcpu, KVM_FEATURE_PREEMPT_HINT)) {
> >>>> + unsafe_put_user(0, &st->preempted, out);
> >>>> + vcpu->arch.st.preempted = 0;
> >>>> + }
> >>>> +
> >>>> unsafe_get_user(version, &st->version, out);
> >>>> if (version & 1)
> >>>> version += 1; /* first time write, random junk */
> >>>> @@ -1757,11 +1762,58 @@ static int _kvm_vcpu_put(struct kvm_vcpu *vcpu, int cpu)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +static void _kvm_set_vcpu_preempted(struct kvm_vcpu *vcpu)
> >>> Just using kvm_set_vcpu_preempted() is enough, no "_".
> >>>
> >>>> +{
> >>>> + struct gfn_to_hva_cache *ghc;
> >>>> + struct kvm_steal_time __user *st;
> >>>> + struct kvm_memslots *slots;
> >>>> + static const u8 preempted = KVM_VCPU_PREEMPTED;
> >>> I'm not sure whether "static" is right, it's not reentrant.
> >> I think static is better here, it saves one cycle with assignment here.
> > I know, but I want to know whether the logic is correct.
> > vcpu->arch.st.preempted is per-cpu, but the local variable "preempted"
> > can be used across multiple VCPU? I'm not sure.
> It is read-only, of course can be used by multiple vCPUs. or remove it
> directly?
Good, remove it directly.
Huacai
>
> @@ -1767,7 +1767,6 @@ static void _kvm_set_vcpu_preempted(struct
> kvm_vcpu *vcpu)
> struct gfn_to_hva_cache *ghc;
> struct kvm_steal_time __user *st;
> struct kvm_memslots *slots;
> - static const u8 preempted = KVM_VCPU_PREEMPTED;
> gpa_t gpa;
>
> gpa = vcpu->arch.st.guest_addr;
> @@ -1793,7 +1792,7 @@ static void _kvm_set_vcpu_preempted(struct
> kvm_vcpu *vcpu)
> }
>
> st = (struct kvm_steal_time __user *)ghc->hva;
> - unsafe_put_user(preempted, &st->preempted, out);
> + unsafe_put_user(KVM_VCPU_PREEMPTED, &st->preempted, out);
> vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED;
>
> >
> > Huacai
> >
> >>
> >> Regards
> >> Bibo Mao
> >>>
> >>>
> >>> Huacai
> >>>
> >>>> + gpa_t gpa;
> >>>> +
> >>>> + gpa = vcpu->arch.st.guest_addr;
> >>>> + if (!(gpa & KVM_STEAL_PHYS_VALID))
> >>>> + return;
> >>>> +
> >>>> + /* vCPU may be preempted for many times */
> >>>> + if (vcpu->arch.st.preempted)
> >>>> + return;
> >>>> +
> >>>> + /* This happens on process exit */
> >>>> + if (unlikely(current->mm != vcpu->kvm->mm))
> >>>> + return;
> >>>> +
> >>>> + gpa &= KVM_STEAL_PHYS_MASK;
> >>>> + ghc = &vcpu->arch.st.cache;
> >>>> + slots = kvm_memslots(vcpu->kvm);
> >>>> + if (slots->generation != ghc->generation || gpa != ghc->gpa) {
> >>>> + if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st))) {
> >>>> + ghc->gpa = INVALID_GPA;
> >>>> + return;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + st = (struct kvm_steal_time __user *)ghc->hva;
> >>>> + unsafe_put_user(preempted, &st->preempted, out);
> >>>> + vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED;
> >>>> +out:
> >>>> + mark_page_dirty_in_slot(vcpu->kvm, ghc->memslot, gpa_to_gfn(ghc->gpa));
> >>>> +}
> >>>> +
> >>>> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >>>> {
> >>>> - int cpu;
> >>>> + int cpu, idx;
> >>>> unsigned long flags;
> >>>>
> >>>> + if (vcpu->preempted && kvm_guest_has_pv_feature(vcpu, KVM_FEATURE_PREEMPT_HINT)) {
> >>>> + /*
> >>>> + * Take the srcu lock as memslots will be accessed to check the gfn
> >>>> + * cache generation against the memslots generation.
> >>>> + */
> >>>> + idx = srcu_read_lock(&vcpu->kvm->srcu);
> >>>> + _kvm_set_vcpu_preempted(vcpu);
> >>>> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
> >>>> + }
> >>>> +
> >>>> local_irq_save(flags);
> >>>> cpu = smp_processor_id();
> >>>> vcpu->arch.last_sched_cpu = cpu;
> >>>> diff --git a/arch/loongarch/kvm/vm.c b/arch/loongarch/kvm/vm.c
> >>>> index a49b1c1a3dd1..b8879110a0a1 100644
> >>>> --- a/arch/loongarch/kvm/vm.c
> >>>> +++ b/arch/loongarch/kvm/vm.c
> >>>> @@ -45,8 +45,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> >>>>
> >>>> /* Enable all PV features by default */
> >>>> kvm->arch.pv_features = BIT(KVM_FEATURE_IPI);
> >>>> - if (kvm_pvtime_supported())
> >>>> + if (kvm_pvtime_supported()) {
> >>>> kvm->arch.pv_features |= BIT(KVM_FEATURE_STEAL_TIME);
> >>>> + kvm->arch.pv_features |= BIT(KVM_FEATURE_PREEMPT_HINT);
> >>>> + }
> >>>>
> >>>> /*
> >>>> * cpu_vabits means user address space only (a half of total).
> >>>> @@ -143,6 +145,7 @@ static int kvm_vm_feature_has_attr(struct kvm *kvm, struct kvm_device_attr *attr
> >>>> case KVM_LOONGARCH_VM_FEAT_PV_IPI:
> >>>> return 0;
> >>>> case KVM_LOONGARCH_VM_FEAT_PV_STEALTIME:
> >>>> + case KVM_LOONGARCH_VM_FEAT_PV_PREEMPT_HINT:
> >>>> if (kvm_pvtime_supported())
> >>>> return 0;
> >>>> return -ENXIO;
> >>>> --
> >>>> 2.39.3
> >>>>
> >>>>
> >>
> >>
>
>
Powered by blists - more mailing lists