Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
From:   Paul Durrant <xadimgnik@gmail.com>
Message-ID: <00b3e1bf-4968-474a-ac45-9c8e9549346e@xen.org>
Date:   Fri, 1 Dec 2023 15:32:40 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Reply-To: paul@xen.org
Subject: Re: [PATCH 2/2] KVM: xen: (re-)initialize shared_info if guest
 (32/64-bit) mode is set
Content-Language: en-US
To:     David Woodhouse <dwmw2@infradead.org>,
        Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20231201104536.947-1-paul@xen.org>
 <20231201104536.947-3-paul@xen.org>
Organization: Xen Project
In-Reply-To: <20231201104536.947-3-paul@xen.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/12/2023 10:45, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> If the shared_info PFN cache has already been initialized then the content
> of the shared_info page needs to be (re-)initialized if the guest mode is
> set. It is no lnger done when the PFN cache is activated.
> Setting the guest mode is either done explicitly by the VMM via the
> KVM_XEN_ATTR_TYPE_LONG_MODE attribute, or implicitly when the guest writes
> the MSR to set up the hypercall page.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
>   arch/x86/kvm/xen.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 7bead3f65e55..bfc8f6698cbc 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -624,8 +624,15 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
>   		} else {
>   			mutex_lock(&kvm->arch.xen.xen_lock);
>   			kvm->arch.xen.long_mode = !!data->u.long_mode;
> +
> +			/*
> +			 * If shared_info has already been initialized
> +			 * then re-initialize it with the new width.
> +			 */
> +			r = kvm->arch.xen.shinfo_cache.active ?
> +				kvm_xen_shared_info_init(kvm) : 0;
> +
>   			mutex_unlock(&kvm->arch.xen.xen_lock);
> -			r = 0;
>   		}
>   		break;
>   
> @@ -657,9 +664,6 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
>   		}
>   		srcu_read_unlock(&kvm->srcu, idx);
>   
> -		if (!r && kvm->arch.xen.shinfo_cache.active)
> -			r = kvm_xen_shared_info_init(kvm);
> -
>   		mutex_unlock(&kvm->arch.xen.xen_lock);
>   		break;
>   	}
> @@ -1144,7 +1148,13 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
>   	bool lm = is_long_mode(vcpu);
>   
>   	/* Latch long_mode for shared_info pages etc. */
> -	vcpu->kvm->arch.xen.long_mode = lm;
> +	kvm->arch.xen.long_mode = lm;
> +
> +	if (kvm->arch.xen.shinfo_cache.active &&
> +	    kvm_xen_shared_info_init(kvm)) {
> +		mutex_unlock(&kvm->arch.xen.xen_lock);

This unlock is bogus; it should have been removed. I'll send a v2.

   Paul

> +		return 1;
> +	}
>   
>   	/*
>   	 * If Xen hypercall intercept is enabled, fill the hypercall