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

[Paul Durrant]

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.