From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-2.mimecast.com ([207.211.31.81]:36074 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2409930AbfJXQyd (ORCPT ); Thu, 24 Oct 2019 12:54:33 -0400 Subject: Re: [RFC 02/37] s390/protvirt: introduce host side setup References: <20191024114059.102802-1-frankja@linux.ibm.com> <20191024114059.102802-3-frankja@linux.ibm.com> <20191024183009.1cb1ec50@p-imbrenda.boeblingen.de.ibm.com> From: David Hildenbrand Message-ID: Date: Thu, 24 Oct 2019 18:54:25 +0200 MIME-Version: 1.0 In-Reply-To: <20191024183009.1cb1ec50@p-imbrenda.boeblingen.de.ibm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Sender: linux-s390-owner@vger.kernel.org List-ID: To: Claudio Imbrenda Cc: Christian Borntraeger , Janosch Frank , kvm@vger.kernel.org, linux-s390@vger.kernel.org, thuth@redhat.com, mihajlov@linux.ibm.com, mimu@linux.ibm.com, cohuck@redhat.com, gor@linux.ibm.com On 24.10.19 18:30, Claudio Imbrenda wrote: > On Thu, 24 Oct 2019 17:52:31 +0200 > David Hildenbrand wrote: >=20 >> On 24.10.19 15:40, Christian Borntraeger wrote: >>> >>> >>> On 24.10.19 15:27, David Hildenbrand wrote: >>>> On 24.10.19 15:25, David Hildenbrand wrote: >>>>> On 24.10.19 13:40, Janosch Frank wrote: >>>>>> From: Vasily Gorbik >>>>>> >>>>>> Introduce KVM_S390_PROTECTED_VIRTUALIZATION_HOST kbuild option >>>>>> for protected virtual machines hosting support code. >>>>>> >>>>>> Add "prot_virt" command line option which controls if the kernel >>>>>> protected VMs support is enabled at runtime. >>>>>> >>>>>> Extend ultravisor info definitions and expose it via uv_info >>>>>> struct filled in during startup. >>>>>> >>>>>> Signed-off-by: Vasily Gorbik >>>>>> --- >>>>>> =C2=A0=C2=A0 .../admin-guide/kernel-parameters.txt=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 5 ++ >>>>>> =C2=A0=C2=A0 arch/s390/boot/Makefile=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 2 +- >>>>>> =C2=A0=C2=A0 arch/s390/boot/uv.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 20 +++++++- >>>>>> =C2=A0=C2=A0 arch/s390/include/asm/uv.h=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 | 46 >>>>>> ++++++++++++++++-- arch/s390/kernel/Makefile >>>>>> |=C2=A0 1 + arch/s390/kernel/setup.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 |=C2=A0 4 -- >>>>>> =C2=A0=C2=A0 arch/s390/kernel/uv.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 48 >>>>>> +++++++++++++++++++ >>>>>> arch/s390/kvm/Kconfig=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 9 ++++ 8 files >>>>>> changed, 126 insertions(+), 9 deletions(-) create mode 100644 >>>>>> arch/s390/kernel/uv.c >>>>>> >>>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt >>>>>> b/Documentation/admin-guide/kernel-parameters.txt index >>>>>> c7ac2f3ac99f..aa22e36b3105 100644 --- >>>>>> a/Documentation/admin-guide/kernel-parameters.txt +++ >>>>>> b/Documentation/admin-guide/kernel-parameters.txt @@ -3693,6 >>>>>> +3693,11 @@ before loading. >>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 See >>>>>> Documentation/admin-guide/blockdev/ramdisk.rst. >>>>>> =C2=A0=C2=A0 +=C2=A0=C2=A0=C2=A0 prot_virt=3D=C2=A0=C2=A0=C2=A0 [S= 390] enable hosting protected virtual >>>>>> machines >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = isolated from the hypervisor (if hardware supports >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = that). >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = Format: >>>>> >>>>> Isn't that a virt driver detail that should come in via KVM module >>>>> parameters? I don't see quite yet why this has to be a kernel >>>>> parameter (that can be changed at runtime). >>>>> =20 >>>> >>>> I was confused by "runtime" in "which controls if the kernel >>>> protected VMs support is enabled at runtime" >>>> >>>> So this can't be changed at runtime. Can you clarify why kvm can't >>>> initialize that when loaded and why we need a kernel parameter? >>> >>> We have to do the opt-in very early for several reasons: >>> - we have to donate a potentially largish contiguous (in real) >>> range of memory to the ultravisor >> >> If you'd be using CMA (or alloc_contig_pages() with less guarantees) >> you could be making good use of the memory until you actually start >> an encrypted guest. >=20 > no, the memory needs to be allocated before any other interaction with > the ultravisor is attempted, and the size depends on the size of the I fail to see why you need interaction with the UV before you actually=20 start/create an encrypted guest (IOW why you can't defer uv_init()) -=20 but I am not past "[RFC 07/37] KVM: s390: protvirt: Secure memory is not=20 mergeable" yet and ... > _host_ memory. it can be a very substantial amount of memory, and thus > it's very likely to fail unless it's done very early at boot time. ... I guess you could still do that via CMA ... but it doesn't really=20 matter right now :) I understood the rational. >=20 >> >>> - The opt-in will also disable some features in the host that could >>> affect guest integrity (e.g. time sync via STP to avoid the host >>> messing with the guest time stepping). Linux is not happy when you >>> remove features at a later point in time >> >> At least disabling STP shouldn't be a real issue if I'm not wrong >> (maybe I am). But there seem to be more features. >> >> (when I saw "prot_virt" it felt like somebody is using a big hammer >> for small nails (e.g., compared to "stp=3Doff").) >> >> Can you guys add these details to the patch description? >=20 > yeah, probably a good idea >=20 If a patch explains why it does something and not only what it does=20 usually makes me ask less stupid questions :) --=20 Thanks, David / dhildenb