Re: [PATCH v12 11/20] KVM: xen: allow shared_info to be mapped by fixed HVA

From: Paul Durrant <xadimgnik@gmail.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>,
	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>,
	David Woodhouse <dwmw2@infradead.org>,
	Shuah Khan <shuah@kernel.org>,
	kvm@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v12 11/20] KVM: xen: allow shared_info to be mapped by fixed HVA
Date: Thu, 8 Feb 2024 16:51:18 +0000	[thread overview]
Message-ID: <148d903c-fcc5-4a6a-aef1-c1e77e74d0fc@xen.org> (raw)
In-Reply-To: <ZcUF-TNbykWvh3r7@google.com>

On 08/02/2024 16:48, Sean Christopherson wrote:
> On Thu, Feb 08, 2024, Paul Durrant wrote:
>> On 07/02/2024 04:10, Sean Christopherson wrote:
>>> On Mon, Jan 15, 2024, Paul Durrant wrote:
>>>> @@ -638,20 +637,32 @@ int kvm_xen_hvm_set_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
>>>>    		}
>>>>    		break;
>>>> -	case KVM_XEN_ATTR_TYPE_SHARED_INFO: {
>>>> +	case KVM_XEN_ATTR_TYPE_SHARED_INFO:
>>>> +	case KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA: {
>>>>    		int idx;
>>>>    		mutex_lock(&kvm->arch.xen.xen_lock);
>>>>    		idx = srcu_read_lock(&kvm->srcu);
>>>> -		if (data->u.shared_info.gfn == KVM_XEN_INVALID_GFN) {
>>>> -			kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache);
>>>> -			r = 0;
>>>> +		if (data->type == KVM_XEN_ATTR_TYPE_SHARED_INFO) {
>>>> +			if (data->u.shared_info.gfn == KVM_XEN_INVALID_GFN) {
>>>> +				kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache);
>>>> +				r = 0;
>>>> +			} else {
>>>> +				r = kvm_gpc_activate(&kvm->arch.xen.shinfo_cache,
>>>> +						     gfn_to_gpa(data->u.shared_info.gfn),
>>>> +						     PAGE_SIZE);
>>>> +			}
>>>>    		} else {
>>>> -			r = kvm_gpc_activate(&kvm->arch.xen.shinfo_cache,
>>>> -					     gfn_to_gpa(data->u.shared_info.gfn),
>>>> -					     PAGE_SIZE);
>>>> +			if (data->u.shared_info.hva == 0) {
>>>
>>> I know I said I don't care about the KVM Xen ABI, but I still think using '0' as
>>> "invalid" is ridiculous.
>>>
>>
>> With the benefit of some sleep, I'm wondering why 0 is a 'ridiculous'
>> invalid value for a *virtual* address? Surely it's essentially a numerical
>> cast of the canonically invalid NULL pointer?
> 
> It's legal to mmap() virtual address '0', albeit not by default:
> 
>    config DEFAULT_MMAP_MIN_ADDR
> 	int "Low address space to protect from user allocation"
> 	depends on MMU
> 	default 4096
> 	help
> 	  This is the portion of low virtual memory which should be protected
> 	  from userspace allocation.  Keeping a user from writing to low pages
> 	  can help reduce the impact of kernel NULL pointer bugs.
> 
> 	  For most ppc64 and x86 users with lots of address space
> 	  a value of 65536 is reasonable and should cause no problems.
> 	  On arm and other archs it should not be higher than 32768.
> 	  Programs which use vm86 functionality or have some need to map
> 	  this low address space will need CAP_SYS_RAWIO or disable this
> 	  protection by setting the value to 0.
> 
> 	  This value can be changed after boot using the
> 	  /proc/sys/vm/mmap_min_addr tunable.
> 
> 
> Obviously it's equally ridiculous that userspace would ever mmap() '0' and pass
> that as the shared_info, but given that this is x86-only, there are architecturally
> illegal addresses that can be used, at least until Intel adds LA64 ;-)

Ok. Thanks for the reference.