On Wed, Sep 21, 2022, Michal Luczaj wrote:
> Move the assignment of immutable properties @kvm, @vcpu, @usage, @len
> to the initializer. Make _init(), _activate() and _deactivate() use
> stored values.
>
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Michal Luczaj <mhal@xxxxxxx>
> @@ -1818,9 +1810,12 @@ void kvm_xen_init_vcpu(struct kvm_vcpu *vcpu)
>
> timer_setup(&vcpu->arch.xen.poll_timer, cancel_evtchn_poll, 0);
>
> - kvm_gpc_init(&vcpu->arch.xen.runstate_cache);
> - kvm_gpc_init(&vcpu->arch.xen.vcpu_info_cache);
> - kvm_gpc_init(&vcpu->arch.xen.vcpu_time_info_cache);
> + kvm_gpc_init(&vcpu->arch.xen.runstate_cache, vcpu->kvm, NULL,
> + KVM_HOST_USES_PFN, sizeof(struct vcpu_runstate_info));
> + kvm_gpc_init(&vcpu->arch.xen.vcpu_info_cache, vcpu->kvm, NULL,
> + KVM_HOST_USES_PFN, sizeof(struct vcpu_info));
Argh, KVM Xen doesn't actually treat these two as immutable values. I suspect
you encountered this as well since check() and refresh() didn't drop @len. I'm
99% certain we can still make the length immutable, it'll just require a bit of
massaging, i.e. extra patches.
The vcpu_info_cache length is effectively immutable, the use of the common helper
kvm_setup_guest_pvclock() just makes it less obvious. This can be addressed by
making the param a "max_len" or "max_size" or whatever, e.g. so that accessing a
subset still verifies the cache as a whole.
The runstate_cache is messier since it actually consumes two different sizes, but
that's arguably a bug that was introduced by commit a795cd43c5b5 ("KVM: x86/xen:
Use gfn_to_pfn_cache for runstate area"). Prior to that, KVM always used the larger
non-compat size. And KVM still uses the larger size during activation, i.e. KVM
will "fail" activation but allow a sneaky 32-bit guest to access a rejected struct
sitting at the very end of the page. I'm pretty sure that hole can be fixed without
breaking KVM's ABI.
With those addressed, the max length becomes immutable and @len can be dropped.
I'll fiddle with this tomorrow and hopefully include patches for that in v2.
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index b2be60c6efa4..9e79ef2cca99 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -212,10 +212,7 @@ void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, int state)
if (!vx->runstate_cache.active)
return;
- if (IS_ENABLED(CONFIG_64BIT) && v->kvm->arch.xen.long_mode)
- user_len = sizeof(struct vcpu_runstate_info);
- else
- user_len = sizeof(struct compat_vcpu_runstate_info);
+ user_len = sizeof(struct vcpu_runstate_info);
read_lock_irqsave(&gpc->lock, flags);
while (!kvm_gfn_to_pfn_cache_check(v->kvm, gpc, gpc->gpa,
