* Re: [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run
2026-07-16 1:38 [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run Bibo Mao
@ 2026-07-16 2:03 ` sashiko-bot
2026-07-16 4:27 ` Huacai Chen
2026-07-16 4:46 ` Tao Cui
2 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-07-16 2:03 UTC (permalink / raw)
To: Bibo Mao; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Per-vCPU execution state is used to protect VM-wide configuration, allowing malicious userspace to modify PV features while the guest is running.
- [High] The enforcement mechanism ensuring all vCPUs use identical PV features has been removed, silently allowing cross-vCPU mismatch.
--
--- Patch [1]: [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run ---
commit bf9fe2ebf34418b4704a3fcafd58fb42df1697c5
Author: Bibo Mao <maobibo@loongson.cn>
LoongArch: KVM: Allow to set pv_feature until vCPU run
This patch modifies KVM on LoongArch to allow setting the pv_feature
multiple times during VM creation. The restriction is relaxed so that
features can be modified until the vCPU starts running, which aims to help
with VM migration.
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31a..ce6a1b06d50db 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
[ ... ]
> @@ -1164,11 +1164,14 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
> if (val & ~valid)
> return -EINVAL;
>
> - /* All vCPUs need set the same PV features */
> - if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
> - && ((kvm->arch.pv_features & valid) != val))
> + if ((kvm->arch.pv_features & valid) == val)
> + return 0;
> +
> + if (vcpu->arch.ran_atleast_once)
> return -EINVAL;
[Severity: High]
Can this allow a user space process to modify PV features while the guest is
running?
Because the check in kvm_loongarch_cpucfg_set_attr() uses per-vCPU state
(vcpu->arch.ran_atleast_once) to gate the VM-wide configuration, what
happens if userspace runs vCPU 0 (setting its ran_atleast_once flag to
true), and then invokes KVM_SET_DEVICE_ATTR on vCPU 1 (whose
ran_atleast_once is still false)?
Wouldn't this bypass the check and mutate the VM-wide kvm->arch.pv_features
while vCPU 0 is actively executing?
> - kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
> +
> + /* All vCPUs need set the same PV features */
> + kvm->arch.pv_features = val;
[Severity: High]
Does this silently mask misconfigurations if userspace sets different
features on different vCPUs?
Without the LOONGARCH_PV_FEAT_UPDATED latch, if userspace configures
different PV features on different unrun vCPUs, it appears KVM will accept
both operations and simply overwrite kvm->arch.pv_features with the latest
value rather than returning an error.
Could this fail to enforce the architectural invariant mentioned in the
comment that all vCPUs must have the same features?
> return 0;
> default:
> return -ENXIO;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716013823.3259816-1-maobibo@loongson.cn?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run
2026-07-16 1:38 [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run Bibo Mao
2026-07-16 2:03 ` sashiko-bot
@ 2026-07-16 4:27 ` Huacai Chen
2026-07-16 6:08 ` Bibo Mao
2026-07-16 4:46 ` Tao Cui
2 siblings, 1 reply; 6+ messages in thread
From: Huacai Chen @ 2026-07-16 4:27 UTC (permalink / raw)
To: Bibo Mao
Cc: WANG Xuerui, Tao Cui, kvm, loongarch, linux-kernel,
linux-kselftest
Hi, Bibo,
On Thu, Jul 16, 2026 at 9:45 AM Bibo Mao <maobibo@loongson.cn> wrote:
>
> Now pv_feature can be set only once, there is problem with VM migration.
> Where it is set when vCPU is created and after migration, here it is
> allow to set for many times, until vCPU starts to run.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
> arch/loongarch/include/asm/kvm_host.h | 4 +++-
> arch/loongarch/kvm/vcpu.c | 15 +++++++++++----
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
> index 23cfbecebbd7..af376fc44c44 100644
> --- a/arch/loongarch/include/asm/kvm_host.h
> +++ b/arch/loongarch/include/asm/kvm_host.h
> @@ -163,7 +163,6 @@ enum emulation_result {
> #define KVM_LARCH_SWCSR_LATEST (0x1 << 3)
> #define KVM_LARCH_HWCSR_USABLE (0x1 << 4)
>
> -#define LOONGARCH_PV_FEAT_UPDATED BIT_ULL(63)
> #define LOONGARCH_PV_FEAT_MASK (BIT(KVM_FEATURE_IPI) | \
> BIT(KVM_FEATURE_PREEMPT) | \
> BIT(KVM_FEATURE_STEAL_TIME) | \
> @@ -250,6 +249,9 @@ struct kvm_vcpu_arch {
> /* cpucfg */
> u32 cpucfg[KVM_MAX_CPUCFG_REGS];
>
> + /* VCPU ran at least once */
> + bool ran_atleast_once;
So long a name? Maybe has_run is enough?
Huacai
> +
> /* paravirt steal time */
> struct {
> u64 guest_addr;
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31..ce6a1b06d50d 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -1164,11 +1164,14 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
> if (val & ~valid)
> return -EINVAL;
>
> - /* All vCPUs need set the same PV features */
> - if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
> - && ((kvm->arch.pv_features & valid) != val))
> + if ((kvm->arch.pv_features & valid) == val)
> + return 0;
> +
> + if (vcpu->arch.ran_atleast_once)
> return -EINVAL;
> - kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
> +
> + /* All vCPUs need set the same PV features */
> + kvm->arch.pv_features = val;
> return 0;
> default:
> return -ENXIO;
> @@ -1851,6 +1854,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> int r = -EINTR;
> struct kvm_run *run = vcpu->run;
>
> + /* Mark this VCPU ran at least once */
> + if (!vcpu->arch.ran_atleast_once)
> + vcpu->arch.ran_atleast_once = true;
> +
> if (vcpu->mmio_needed) {
> if (!vcpu->mmio_is_write)
> kvm_complete_mmio_read(vcpu, run);
>
> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
> --
> 2.39.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run
2026-07-16 4:27 ` Huacai Chen
@ 2026-07-16 6:08 ` Bibo Mao
0 siblings, 0 replies; 6+ messages in thread
From: Bibo Mao @ 2026-07-16 6:08 UTC (permalink / raw)
To: Huacai Chen
Cc: WANG Xuerui, Tao Cui, kvm, loongarch, linux-kernel,
linux-kselftest
On 2026/7/16 下午12:27, Huacai Chen wrote:
> Hi, Bibo,
>
> On Thu, Jul 16, 2026 at 9:45 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> Now pv_feature can be set only once, there is problem with VM migration.
>> Where it is set when vCPU is created and after migration, here it is
>> allow to set for many times, until vCPU starts to run.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>> arch/loongarch/include/asm/kvm_host.h | 4 +++-
>> arch/loongarch/kvm/vcpu.c | 15 +++++++++++----
>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
>> index 23cfbecebbd7..af376fc44c44 100644
>> --- a/arch/loongarch/include/asm/kvm_host.h
>> +++ b/arch/loongarch/include/asm/kvm_host.h
>> @@ -163,7 +163,6 @@ enum emulation_result {
>> #define KVM_LARCH_SWCSR_LATEST (0x1 << 3)
>> #define KVM_LARCH_HWCSR_USABLE (0x1 << 4)
>>
>> -#define LOONGARCH_PV_FEAT_UPDATED BIT_ULL(63)
>> #define LOONGARCH_PV_FEAT_MASK (BIT(KVM_FEATURE_IPI) | \
>> BIT(KVM_FEATURE_PREEMPT) | \
>> BIT(KVM_FEATURE_STEAL_TIME) | \
>> @@ -250,6 +249,9 @@ struct kvm_vcpu_arch {
>> /* cpucfg */
>> u32 cpucfg[KVM_MAX_CPUCFG_REGS];
>>
>> + /* VCPU ran at least once */
>> + bool ran_atleast_once;
> So long a name? Maybe has_run is enough?
well, will use has_run for short.
Regards
Bibo Mao
>
> Huacai
>> +
>> /* paravirt steal time */
>> struct {
>> u64 guest_addr;
>> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
>> index 20c207d80e31..ce6a1b06d50d 100644
>> --- a/arch/loongarch/kvm/vcpu.c
>> +++ b/arch/loongarch/kvm/vcpu.c
>> @@ -1164,11 +1164,14 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
>> if (val & ~valid)
>> return -EINVAL;
>>
>> - /* All vCPUs need set the same PV features */
>> - if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
>> - && ((kvm->arch.pv_features & valid) != val))
>> + if ((kvm->arch.pv_features & valid) == val)
>> + return 0;
>> +
>> + if (vcpu->arch.ran_atleast_once)
>> return -EINVAL;
>> - kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
>> +
>> + /* All vCPUs need set the same PV features */
>> + kvm->arch.pv_features = val;
>> return 0;
>> default:
>> return -ENXIO;
>> @@ -1851,6 +1854,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>> int r = -EINTR;
>> struct kvm_run *run = vcpu->run;
>>
>> + /* Mark this VCPU ran at least once */
>> + if (!vcpu->arch.ran_atleast_once)
>> + vcpu->arch.ran_atleast_once = true;
>> +
>> if (vcpu->mmio_needed) {
>> if (!vcpu->mmio_is_write)
>> kvm_complete_mmio_read(vcpu, run);
>>
>> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
>> --
>> 2.39.3
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run
2026-07-16 1:38 [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run Bibo Mao
2026-07-16 2:03 ` sashiko-bot
2026-07-16 4:27 ` Huacai Chen
@ 2026-07-16 4:46 ` Tao Cui
2026-07-16 6:12 ` Bibo Mao
2 siblings, 1 reply; 6+ messages in thread
From: Tao Cui @ 2026-07-16 4:46 UTC (permalink / raw)
To: Bibo Mao, Huacai Chen
Cc: cui.tao, WANG Xuerui, kvm, loongarch, linux-kernel,
linux-kselftest
在 2026/7/16 09:38, Bibo Mao 写道:
> Now pv_feature can be set only once, there is problem with VM migration.
> Where it is set when vCPU is created and after migration, here it is
> allow to set for many times, until vCPU starts to run.
>
Hi Bibo,
The Sashiko AI review raised two concerns on this patch that I think
are valid:
1. Since ran_atleast_once is per-vCPU but pv_features is VM-wide,
userspace could run vCPU 0 and then use vCPU 1 (whose
ran_atleast_once is still false) to change pv_features while
vCPU 0 is executing.
2. Without the old LOONGARCH_PV_FEAT_UPDATED latch, setting different
PV features on different un-run vCPUs silently overwrites
pv_features instead of returning -EINVAL.
Both stem from using a per-vCPU flag to protect VM-wide state.
Would a VM-level bool in kvm_arch (e.g. pv_features_configured),
set on first SET_ATTR or first RUN of any vCPU, work? It would
not be migrated since it is not exposed via any ioctl.
Thanks,
Tao
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
> arch/loongarch/include/asm/kvm_host.h | 4 +++-
> arch/loongarch/kvm/vcpu.c | 15 +++++++++++----
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
> index 23cfbecebbd7..af376fc44c44 100644
> --- a/arch/loongarch/include/asm/kvm_host.h
> +++ b/arch/loongarch/include/asm/kvm_host.h
> @@ -163,7 +163,6 @@ enum emulation_result {
> #define KVM_LARCH_SWCSR_LATEST (0x1 << 3)
> #define KVM_LARCH_HWCSR_USABLE (0x1 << 4)
>
> -#define LOONGARCH_PV_FEAT_UPDATED BIT_ULL(63)
> #define LOONGARCH_PV_FEAT_MASK (BIT(KVM_FEATURE_IPI) | \
> BIT(KVM_FEATURE_PREEMPT) | \
> BIT(KVM_FEATURE_STEAL_TIME) | \
> @@ -250,6 +249,9 @@ struct kvm_vcpu_arch {
> /* cpucfg */
> u32 cpucfg[KVM_MAX_CPUCFG_REGS];
>
> + /* VCPU ran at least once */
> + bool ran_atleast_once;
> +
> /* paravirt steal time */
> struct {
> u64 guest_addr;
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31..ce6a1b06d50d 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -1164,11 +1164,14 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
> if (val & ~valid)
> return -EINVAL;
>
> - /* All vCPUs need set the same PV features */
> - if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
> - && ((kvm->arch.pv_features & valid) != val))
> + if ((kvm->arch.pv_features & valid) == val)
> + return 0;
> +
> + if (vcpu->arch.ran_atleast_once)
> return -EINVAL;
> - kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
> +
> + /* All vCPUs need set the same PV features */
> + kvm->arch.pv_features = val;
> return 0;
> default:
> return -ENXIO;
> @@ -1851,6 +1854,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> int r = -EINTR;
> struct kvm_run *run = vcpu->run;
>
> + /* Mark this VCPU ran at least once */
> + if (!vcpu->arch.ran_atleast_once)
> + vcpu->arch.ran_atleast_once = true;
> +
> if (vcpu->mmio_needed) {
> if (!vcpu->mmio_is_write)
> kvm_complete_mmio_read(vcpu, run);
>
> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] LoongArch: KVM: Allow to set pv_feature until vCPU run
2026-07-16 4:46 ` Tao Cui
@ 2026-07-16 6:12 ` Bibo Mao
0 siblings, 0 replies; 6+ messages in thread
From: Bibo Mao @ 2026-07-16 6:12 UTC (permalink / raw)
To: Tao Cui, Huacai Chen
Cc: WANG Xuerui, kvm, loongarch, linux-kernel, linux-kselftest
On 2026/7/16 下午12:46, Tao Cui wrote:
>
>
> 在 2026/7/16 09:38, Bibo Mao 写道:
>> Now pv_feature can be set only once, there is problem with VM migration.
>> Where it is set when vCPU is created and after migration, here it is
>> allow to set for many times, until vCPU starts to run.
>>
> Hi Bibo,
>
> The Sashiko AI review raised two concerns on this patch that I think
> are valid:
>
> 1. Since ran_atleast_once is per-vCPU but pv_features is VM-wide,
> userspace could run vCPU 0 and then use vCPU 1 (whose
> ran_atleast_once is still false) to change pv_features while
> vCPU 0 is executing.
>
> 2. Without the old LOONGARCH_PV_FEAT_UPDATED latch, setting different
> PV features on different un-run vCPUs silently overwrites
> pv_features instead of returning -EINVAL.
>
> Both stem from using a per-vCPU flag to protect VM-wide state.
> Would a VM-level bool in kvm_arch (e.g. pv_features_configured),
> set on first SET_ATTR or first RUN of any vCPU, work? It would
> not be migrated since it is not exposed via any ioctl.
Now pv_features is per VM, will set it as per CPU to avoid access
contention in next version. Consistent checking should with per CPU
pv_features will be done in VMM, rather than KVM, similar with per CPU
cpucfg feature.
Regards
Bibo Mao
>
> Thanks,
> Tao
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>> arch/loongarch/include/asm/kvm_host.h | 4 +++-
>> arch/loongarch/kvm/vcpu.c | 15 +++++++++++----
>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/kvm_host.h b/arch/loongarch/include/asm/kvm_host.h
>> index 23cfbecebbd7..af376fc44c44 100644
>> --- a/arch/loongarch/include/asm/kvm_host.h
>> +++ b/arch/loongarch/include/asm/kvm_host.h
>> @@ -163,7 +163,6 @@ enum emulation_result {
>> #define KVM_LARCH_SWCSR_LATEST (0x1 << 3)
>> #define KVM_LARCH_HWCSR_USABLE (0x1 << 4)
>>
>> -#define LOONGARCH_PV_FEAT_UPDATED BIT_ULL(63)
>> #define LOONGARCH_PV_FEAT_MASK (BIT(KVM_FEATURE_IPI) | \
>> BIT(KVM_FEATURE_PREEMPT) | \
>> BIT(KVM_FEATURE_STEAL_TIME) | \
>> @@ -250,6 +249,9 @@ struct kvm_vcpu_arch {
>> /* cpucfg */
>> u32 cpucfg[KVM_MAX_CPUCFG_REGS];
>>
>> + /* VCPU ran at least once */
>> + bool ran_atleast_once;
>> +
>> /* paravirt steal time */
>> struct {
>> u64 guest_addr;
>> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
>> index 20c207d80e31..ce6a1b06d50d 100644
>> --- a/arch/loongarch/kvm/vcpu.c
>> +++ b/arch/loongarch/kvm/vcpu.c
>> @@ -1164,11 +1164,14 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
>> if (val & ~valid)
>> return -EINVAL;
>>
>> - /* All vCPUs need set the same PV features */
>> - if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
>> - && ((kvm->arch.pv_features & valid) != val))
>> + if ((kvm->arch.pv_features & valid) == val)
>> + return 0;
>> +
>> + if (vcpu->arch.ran_atleast_once)
>> return -EINVAL;
>> - kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
>> +
>> + /* All vCPUs need set the same PV features */
>> + kvm->arch.pv_features = val;
>> return 0;
>> default:
>> return -ENXIO;
>> @@ -1851,6 +1854,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>> int r = -EINTR;
>> struct kvm_run *run = vcpu->run;
>>
>> + /* Mark this VCPU ran at least once */
>> + if (!vcpu->arch.ran_atleast_once)
>> + vcpu->arch.ran_atleast_once = true;
>> +
>> if (vcpu->mmio_needed) {
>> if (!vcpu->mmio_is_write)
>> kvm_complete_mmio_read(vcpu, run);
>>
>> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
>
^ permalink raw reply [flat|nested] 6+ messages in thread