On 12/14/20 12:02 PM, David Woodhouse wrote:
> On Mon, 2020-12-14 at 10:45 +0000, Joao Martins wrote:
>> On 12/14/20 8:38 AM, David Woodhouse wrote:
>>> From: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>>
>>> We add a new ioctl, XEN_HVM_SHARED_INFO, to allow hypervisor
>>> to know where the guest's shared info page is.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>> Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
>>> ---
>>> arch/x86/include/asm/kvm_host.h | 2 ++
>>> arch/x86/kvm/xen.c | 27 +++++++++++++++++++++++++++
>>> arch/x86/kvm/xen.h | 1 -
>>> include/uapi/linux/kvm.h | 4 ++++
>>> 4 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/kvm_host.h
>>> b/arch/x86/include/asm/kvm_host.h
>>> index c9a4feaee2e7..8bcd83dacf43 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -893,6 +893,8 @@ struct msr_bitmap_range {
>>> /* Xen emulation context */
>>> struct kvm_xen {
>>> bool long_mode;
>>> + bool shinfo_set;
>>> + struct gfn_to_hva_cache shinfo_cache;
>>> };
>>>
>>> enum kvm_irqchip_mode {
>>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>>> index 52cb9e465542..9dd9c42842b8 100644
>>> --- a/arch/x86/kvm/xen.c
>>> +++ b/arch/x86/kvm/xen.c
>>> @@ -13,9 +13,23 @@
>>> #include <linux/kvm_host.h>
>>>
>>> #include <trace/events/kvm.h>
>>> +#include <xen/interface/xen.h>
>>>
>>> #include "trace.h"
>>>
>>> +static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
>>> +{
>>> + int ret;
>>> +
>>> + ret = kvm_gfn_to_hva_cache_init(kvm, &kvm-
>>>> arch.xen.shinfo_cache,
>>> + gfn_to_gpa(gfn), PAGE_SIZE);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + kvm->arch.xen.shinfo_set = true;
>>
>> Can't you just use:
>>
>> kvm->arch.xen.shinfo_cache.gpa
>>
>> Rather than added a bool just to say you set a shinfo?
>
> I see no reason why a guest shouldn't be able to use GPA zero if it
> really wanted to. Using a separate boolean matches what KVM does for
> the wallclock info.
>
Ah OK -- I didn't notice @pv_time_enabled before suggesting this.
An helper would probably suffice without sacrificing footprint in
data structures (because you will add 3 of these). The helpers would be, say:
kvm_xen_shared_info_set(vcpu->kvm)
kvm_xen_vcpu_info_set(vcpu)
kvm_xen_vcpu_runstate_set(vcpu)
And maybe backed by a:
kvm_gfn_to_hva_cache_valid(&v->kvm->arch.xen.shinfo_cache)
Which would check whether a cache is initialized or valid.
But anyhow, I don't feel strongly about it, specially if there's an existing
one which sort of sets the precedent.
Joao
