| Message ID | 20230918112148.28855-9-paul@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: xen: update shared_info and vcpu_info handling | expand |
On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote: > - This is because KVM may > - not be aware of the Xen CPU id which is used as the index into the > - vcpu_info[] array, so may know the correct default location. Hm, that *was* true at the time of writing, but we did end up teaching KVM about the Xen vcpu_id so that we can handle timers. (We required userspace to provide the APIC ID for all the other event channel stuff, but timer hypercalls come straight from the guest). But I think the *only* thing we use vcpu->arch.xen.vcpu_id for right now is acceleration of the timer hypercalls that are restricted to the vCPU that they're called from, e.g.: case VCPUOP_set_singleshot_timer: if (vcpu->arch.xen.vcpu_id != vcpu_id) { *r = -EINVAL; So it's never mattered much before now if the Xen vcpu_id changes at runtime. Maybe you now want to invalidate the vcpu_info pfncache for a vCPU if its Xen vcpu_id changes? Or just *forbid* such a change after the shinfo page has been set up? What locking prevents the xen.vcpu_id changing in the middle of your new get_vcpu_info_cache() function?
On 18/09/2023 12:49, David Woodhouse wrote: > On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote: >> - This is because KVM may >> - not be aware of the Xen CPU id which is used as the index into the >> - vcpu_info[] array, so may know the correct default location. > > Hm, that *was* true at the time of writing, but we did end up teaching > KVM about the Xen vcpu_id so that we can handle timers. (We required > userspace to provide the APIC ID for all the other event channel stuff, > but timer hypercalls come straight from the guest). > > But I think the *only* thing we use vcpu->arch.xen.vcpu_id for right > now is acceleration of the timer hypercalls that are restricted to the > vCPU that they're called from, e.g.: > > case VCPUOP_set_singleshot_timer: > if (vcpu->arch.xen.vcpu_id != vcpu_id) { > *r = -EINVAL; > > So it's never mattered much before now if the Xen vcpu_id changes at > runtime. > > Maybe you now want to invalidate the vcpu_info pfncache for a vCPU if > its Xen vcpu_id changes? Or just *forbid* such a change after the > shinfo page has been set up? What locking prevents the xen.vcpu_id > changing in the middle of your new get_vcpu_info_cache() function? That's a good point; the vcpu lock itself won't be enough. I think forbidding a change of id while a shared_info page is in place is probably the most sensible semantic. Paul
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index e9df4df6fe48..54f5d7392fe8 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -5442,13 +5442,7 @@ KVM_XEN_ATTR_TYPE_LONG_MODE KVM_XEN_ATTR_TYPE_SHARED_INFO Sets the guest physical frame number at which the Xen shared_info - page resides. Note that although Xen places vcpu_info for the first - 32 vCPUs in the shared_info page, KVM does not automatically do so - and instead requires that KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO be used - explicitly even when the vcpu_info for a given vCPU resides at the - "default" location in the shared_info page. This is because KVM may - not be aware of the Xen CPU id which is used as the index into the - vcpu_info[] array, so may know the correct default location. + page resides. Note that the shared_info page may be constantly written to by KVM; it contains the event channel bitmap used to deliver interrupts to @@ -5564,6 +5558,14 @@ type values: KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO Sets the guest physical address of the vcpu_info for a given vCPU. + The vcpu_info for the first 32 vCPUs defaults to the structures + embedded in the shared_info page. + + If the KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA flag is also set in the + Xen capabilities then the VMM is not required to set this default + location; KVM will handle that internally. Otherwise this attribute + must be set for all vCPUs. + As with the shared_info page for the VM, the corresponding page may be dirtied at any time if event channel interrupt delivery is enabled, so userspace should always assume that the page is dirty without relying diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 7fc4fc2e54d8..7cb5f8b55205 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -491,6 +491,21 @@ static void kvm_xen_inject_vcpu_vector(struct kvm_vcpu *v) static struct gfn_to_pfn_cache *get_vcpu_info_cache(struct kvm_vcpu *v, unsigned long *offset) { + if (!v->arch.xen.vcpu_info_cache.active && v->arch.xen.vcpu_id < MAX_VIRT_CPUS) { + struct kvm *kvm = v->kvm; + + if (offset) { + if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) + *offset = offsetof(struct shared_info, + vcpu_info[v->arch.xen.vcpu_id]); + else + *offset = offsetof(struct compat_shared_info, + vcpu_info[v->arch.xen.vcpu_id]); + } + + return &kvm->arch.xen.shinfo_cache; + } + if (offset) *offset = 0;