| Message ID | 20201214083905.2017260-13-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Add minimal support for Xen HVM guests | expand |
On 12/14/20 8:39 AM, David Woodhouse wrote: > From: Joao Martins <joao.m.martins@oracle.com> > > Parameterise kvm_setup_pvclock_page() a little bit so that it can be > invoked for different gfn_to_hva_cache structures, and with different > offsets. Then we can invoke it for the normal KVM pvclock and also for > the Xen one in the vcpu_info. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > arch/x86/kvm/x86.c | 31 ++++++++++++++++++------------- > arch/x86/kvm/xen.c | 4 ++++ > 2 files changed, 22 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 64016443159c..cbdc05bb53bd 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2582,13 +2582,15 @@ u64 get_kvmclock_ns(struct kvm *kvm) > return ret; > } > > -static void kvm_setup_pvclock_page(struct kvm_vcpu *v) > +static void kvm_setup_pvclock_page(struct kvm_vcpu *v, > + struct gfn_to_hva_cache *cache, > + unsigned int offset) > { > struct kvm_vcpu_arch *vcpu = &v->arch; > struct pvclock_vcpu_time_info guest_hv_clock; > > - if (unlikely(kvm_read_guest_cached(v->kvm, &vcpu->pv_time, > - &guest_hv_clock, sizeof(guest_hv_clock)))) > + if (unlikely(kvm_read_guest_offset_cached(v->kvm, cache, > + &guest_hv_clock, offset, sizeof(guest_hv_clock)))) > return; > > /* This VCPU is paused, but it's legal for a guest to read another > @@ -2611,9 +2613,9 @@ static void kvm_setup_pvclock_page(struct kvm_vcpu *v) > ++guest_hv_clock.version; /* first time write, random junk */ > > vcpu->hv_clock.version = guest_hv_clock.version + 1; > - kvm_write_guest_cached(v->kvm, &vcpu->pv_time, > - &vcpu->hv_clock, > - sizeof(vcpu->hv_clock.version)); > + kvm_write_guest_offset_cached(v->kvm, cache, > + &vcpu->hv_clock, offset, > + sizeof(vcpu->hv_clock.version)); > > smp_wmb(); > > @@ -2627,16 +2629,16 @@ static void kvm_setup_pvclock_page(struct kvm_vcpu *v) > > trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock); > > - kvm_write_guest_cached(v->kvm, &vcpu->pv_time, > - &vcpu->hv_clock, > - sizeof(vcpu->hv_clock)); > + kvm_write_guest_offset_cached(v->kvm, cache, > + &vcpu->hv_clock, offset, > + sizeof(vcpu->hv_clock)); > > smp_wmb(); > > vcpu->hv_clock.version++; > - kvm_write_guest_cached(v->kvm, &vcpu->pv_time, > - &vcpu->hv_clock, > - sizeof(vcpu->hv_clock.version)); > + kvm_write_guest_offset_cached(v->kvm, cache, > + &vcpu->hv_clock, offset, > + sizeof(vcpu->hv_clock.version)); > } > > static int kvm_guest_time_update(struct kvm_vcpu *v) > @@ -2723,7 +2725,10 @@ static int kvm_guest_time_update(struct kvm_vcpu *v) > vcpu->hv_clock.flags = pvclock_flags; > > if (vcpu->pv_time_enabled) > - kvm_setup_pvclock_page(v); > + kvm_setup_pvclock_page(v, &vcpu->pv_time, 0); > + if (vcpu->xen.vcpu_info_set) > + kvm_setup_pvclock_page(v, &vcpu->xen.vcpu_info_cache, > + offsetof(struct compat_vcpu_info, time)); We might be missing the case where only shared_info is registered. Something like: if (vcpu->xen.shinfo_set && !vcpu->xen.vcpu_info_set) { offset = offsetof(struct compat_vcpu_info, time); offset += offsetof(struct shared_info, vcpu_info); offset += (v - kvm_get_vcpu_by_id(0)) * sizeof(struct compat_vcpu_info); kvm_setup_pvclock_page(v, &vcpu->xen.shinfo_cache, offset); } Part of the reason I had a kvm_xen_setup_pvclock_page() was to handle this all these combinations i.e. 1) shared_info && !vcpu_info 2) vcpu_info and unilaterially updating secondary time info. But maybe introducing this xen_vcpu_info() helper to accommodate all this. > if (v == kvm_get_vcpu(v->kvm, 0)) > kvm_hv_setup_tsc_page(v->kvm, &vcpu->hv_clock); > return 0; > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > index 4bc72e0b9928..d2055b60fdc1 100644 > --- a/arch/x86/kvm/xen.c > +++ b/arch/x86/kvm/xen.c > @@ -82,6 +82,9 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data) > /* No compat necessary here. */ > BUILD_BUG_ON(sizeof(struct vcpu_info) != > sizeof(struct compat_vcpu_info)); > + BUILD_BUG_ON(offsetof(struct vcpu_info, time) != > + offsetof(struct compat_vcpu_info, time)); > + > r = kvm_gfn_to_hva_cache_init(kvm, &v->arch.xen.vcpu_info_cache, > data->u.vcpu_attr.gpa, > sizeof(struct vcpu_info)); > @@ -89,6 +92,7 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data) > return r; > > v->arch.xen.vcpu_info_set = true; > + kvm_make_request(KVM_REQ_CLOCK_UPDATE, v); > break; > > default: >
On Mon, 2020-12-14 at 13:29 +0000, Joao Martins wrote: > We might be missing the case where only shared_info is registered. Something like: > > if (vcpu->xen.shinfo_set && !vcpu->xen.vcpu_info_set) { > offset = offsetof(struct compat_vcpu_info, time); > offset += offsetof(struct shared_info, vcpu_info); > offset += (v - kvm_get_vcpu_by_id(0)) * sizeof(struct compat_vcpu_info); > > kvm_setup_pvclock_page(v, &vcpu->xen.shinfo_cache, offset); > } > > Part of the reason I had a kvm_xen_setup_pvclock_page() was to handle this all these > combinations i.e. 1) shared_info && !vcpu_info 2) vcpu_info and unilaterially updating > secondary time info. > > But maybe introducing this xen_vcpu_info() helper to accommodate all this. There was complexity. I don't like complexity. I made it go away.
On 12/14/20 2:58 PM, David Woodhouse wrote: > On Mon, 2020-12-14 at 13:29 +0000, Joao Martins wrote: >> We might be missing the case where only shared_info is registered. Something like: >> >> if (vcpu->xen.shinfo_set && !vcpu->xen.vcpu_info_set) { >> offset = offsetof(struct compat_vcpu_info, time); >> offset += offsetof(struct shared_info, vcpu_info); >> offset += (v - kvm_get_vcpu_by_id(0)) * sizeof(struct compat_vcpu_info); >> >> kvm_setup_pvclock_page(v, &vcpu->xen.shinfo_cache, offset); >> } >> >> Part of the reason I had a kvm_xen_setup_pvclock_page() was to handle this all these >> combinations i.e. 1) shared_info && !vcpu_info 2) vcpu_info and unilaterially updating >> secondary time info. >> >> But maybe introducing this xen_vcpu_info() helper to accommodate all this. > > There was complexity. > > I don't like complexity. > > I made it go away. > Considering what you said earlier, yes, it would be unnecessary complexity. Joao
On Mon, 2020-12-14 at 15:20 +0000, Joao Martins wrote: > On 12/14/20 2:58 PM, David Woodhouse wrote: > > On Mon, 2020-12-14 at 13:29 +0000, Joao Martins wrote: > > > We might be missing the case where only shared_info is registered. Something like: > > > > > > if (vcpu->xen.shinfo_set && !vcpu->xen.vcpu_info_set) { > > > offset = offsetof(struct compat_vcpu_info, time); > > > offset += offsetof(struct shared_info, vcpu_info); > > > offset += (v - kvm_get_vcpu_by_id(0)) * sizeof(struct compat_vcpu_info); > > > > > > kvm_setup_pvclock_page(v, &vcpu->xen.shinfo_cache, offset); > > > } > > > > > > Part of the reason I had a kvm_xen_setup_pvclock_page() was to handle this all these > > > combinations i.e. 1) shared_info && !vcpu_info 2) vcpu_info and unilaterially updating > > > secondary time info. > > > > > > But maybe introducing this xen_vcpu_info() helper to accommodate all this. > > > > There was complexity. > > > > I don't like complexity. > > > > I made it go away. > > > > Considering what you said earlier, yes, it would be unnecessary complexity. To be fair I don't really make it go away completely; I push it up into userspace. Which now has to keep track of whether a given vCPU has an explicitly-set vcpu_info page, or whether it has just used the default one in the shared_info. And if the shared_info *changes*, as it might on a kexec or just guest weirdness, the VMM should ideally change only those vCPUs which were at the default location. When it was just v->vcpu_info?:shinfo->vcpu_info[N] in the kernel that part was slightly simpler. It was just slightly hampered by the fact that the kernel has no way of knowing what N should be :)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 64016443159c..cbdc05bb53bd 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2582,13 +2582,15 @@ u64 get_kvmclock_ns(struct kvm *kvm) return ret; } -static void kvm_setup_pvclock_page(struct kvm_vcpu *v) +static void kvm_setup_pvclock_page(struct kvm_vcpu *v, + struct gfn_to_hva_cache *cache, + unsigned int offset) { struct kvm_vcpu_arch *vcpu = &v->arch; struct pvclock_vcpu_time_info guest_hv_clock; - if (unlikely(kvm_read_guest_cached(v->kvm, &vcpu->pv_time, - &guest_hv_clock, sizeof(guest_hv_clock)))) + if (unlikely(kvm_read_guest_offset_cached(v->kvm, cache, + &guest_hv_clock, offset, sizeof(guest_hv_clock)))) return; /* This VCPU is paused, but it's legal for a guest to read another @@ -2611,9 +2613,9 @@ static void kvm_setup_pvclock_page(struct kvm_vcpu *v) ++guest_hv_clock.version; /* first time write, random junk */ vcpu->hv_clock.version = guest_hv_clock.version + 1; - kvm_write_guest_cached(v->kvm, &vcpu->pv_time, - &vcpu->hv_clock, - sizeof(vcpu->hv_clock.version)); + kvm_write_guest_offset_cached(v->kvm, cache, + &vcpu->hv_clock, offset, + sizeof(vcpu->hv_clock.version)); smp_wmb(); @@ -2627,16 +2629,16 @@ static void kvm_setup_pvclock_page(struct kvm_vcpu *v) trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock); - kvm_write_guest_cached(v->kvm, &vcpu->pv_time, - &vcpu->hv_clock, - sizeof(vcpu->hv_clock)); + kvm_write_guest_offset_cached(v->kvm, cache, + &vcpu->hv_clock, offset, + sizeof(vcpu->hv_clock)); smp_wmb(); vcpu->hv_clock.version++; - kvm_write_guest_cached(v->kvm, &vcpu->pv_time, - &vcpu->hv_clock, - sizeof(vcpu->hv_clock.version)); + kvm_write_guest_offset_cached(v->kvm, cache, + &vcpu->hv_clock, offset, + sizeof(vcpu->hv_clock.version)); } static int kvm_guest_time_update(struct kvm_vcpu *v) @@ -2723,7 +2725,10 @@ static int kvm_guest_time_update(struct kvm_vcpu *v) vcpu->hv_clock.flags = pvclock_flags; if (vcpu->pv_time_enabled) - kvm_setup_pvclock_page(v); + kvm_setup_pvclock_page(v, &vcpu->pv_time, 0); + if (vcpu->xen.vcpu_info_set) + kvm_setup_pvclock_page(v, &vcpu->xen.vcpu_info_cache, + offsetof(struct compat_vcpu_info, time)); if (v == kvm_get_vcpu(v->kvm, 0)) kvm_hv_setup_tsc_page(v->kvm, &vcpu->hv_clock); return 0; diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 4bc72e0b9928..d2055b60fdc1 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -82,6 +82,9 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data) /* No compat necessary here. */ BUILD_BUG_ON(sizeof(struct vcpu_info) != sizeof(struct compat_vcpu_info)); + BUILD_BUG_ON(offsetof(struct vcpu_info, time) != + offsetof(struct compat_vcpu_info, time)); + r = kvm_gfn_to_hva_cache_init(kvm, &v->arch.xen.vcpu_info_cache, data->u.vcpu_attr.gpa, sizeof(struct vcpu_info)); @@ -89,6 +92,7 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data) return r; v->arch.xen.vcpu_info_set = true; + kvm_make_request(KVM_REQ_CLOCK_UPDATE, v); break; default: