Date: Tue, 29 Nov 2016 17:53:25 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH] KVM: x86: restrict maximal physical address
Message-ID: <20161129165321.GA15101@potion>
References: <20161125145105.9508-1-rkrcmar@redhat.com>
 <88fa28cd-6d81-6f88-871c-484973b98292@redhat.com>
 <20161125161450.GB5878@potion>
 <1cca4e4d-97f8-91b9-4e71-66f4a3c3cfb2@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1cca4e4d-97f8-91b9-4e71-66f4a3c3cfb2@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2974
Lines: 65

2016-11-25 17:43+0100, David Hildenbrand:
>> > This check is correct.
>> > 
>> > However, I wonder if there is any way for user space to query this property?
>> 
>> Do you mean boot_cpu_data.x86_phys_bits?
>> Userspace can execute CPUID instruction and read the value; QEMU does.
> 
> Thanks, good to know. I remember that on s390x we explicitly decided to
> query the maximum address from KVM (KVM_S390_VM_MEM_LIMIT_SIZE) for two
> reasons. One of them was "just because our CPU supports it doesn't mean KVM
> supports it". Just like with all CPU features.
> 
> However, this applies only for configuring hardware virtualization. The
> value that is exposed to the guest comes from the cpu model (with s390x cpu
> model support). So it will also not change during migration.
> 
> But if this will never be relevant for x86 (KVM will always support host
> x86_phys_bits), fine.
> 
>> 
>> > On s390x, there is a kvm capability to export this information to user
>> > space. So QEMU can fail (e.g. migration) with a nice error message about
>> > missing hardware support.
>> > 
>> > (most probably we still want to block this case, as migration will seem to
>> > work but than simply fail due to missing hardware support I guess). Maybe
>> > there is also already a nice check in QEMU that I am not yet aware of :)
>> 
>> This patch is bad.  It would break QEMU on all old machines, because
>> QEMU sets 40 by default.
> 
> Not sure if rounding that value down (so it is at least consistent in KVM)
> makes sense (and documenting this behavior "may be rounded down"). And then
> implementing appropriate checks in QEMU (if not already present).

Silently rouding down doesn't fix bugs that we introduce to the guest,
just makes them behave differently and changing the value while the
guest is running could introduce more bugs. :(

I slightly prefer doing nothing for the case I was writing this patch
for:  VMX checks for CR3 reserved bits -- doing nothing means that the
guest gets killed; rouding down would make the guest misbehave, which a
bit harder to debug.

Changing QEMU makes sense even if KVM stays the same.  I'd touch QEMU
first, actually and after few years (decades), we could just apply this
patch. :)

>> Heh, QEMU doesn't check at all -- it even allows migration with
>> "host-phys-bits" feature and will happily change phys-bits when
>> migrating to another machine.
>> 
> 
> Either migrate that value (hmmm... ) or glue it to a command line parameter,
> so it won't change while migrating. E.g.
> - cpu models (if this value was always the same for a CPU generation - no
> expert on x86 cpu models).
> - "-cpu maxmem..." - could be a fit when thinking about "maximum VM size ==
> max phys bits for our guest". But depends how this value is actually
> interpreted by guests.

Yes, the host value has to be migrated in that case.  QEMU also has
"phys-bits=N" feature and the default protected by machine types is 40,
so both work as expected on migration.