Re: [RFC 02/37] s390/protvirt: introduce host side setup

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 1 Nov 2019 09:53:12 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 24.10.19 13:40, Janosch Frank wrote:
> > From: Vasily Gorbik <gor@linux.ibm.com>
> > 
> > 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 <gor@linux.ibm.com>
> > ---
> >  .../admin-guide/kernel-parameters.txt         |  5 ++
> >  arch/s390/boot/Makefile                       |  2 +-
> >  arch/s390/boot/uv.c                           | 20 +++++++-
> >  arch/s390/include/asm/uv.h                    | 46 ++++++++++++++++--
> >  arch/s390/kernel/Makefile                     |  1 +
> >  arch/s390/kernel/setup.c                      |  4 --
> >  arch/s390/kernel/uv.c                         | 48 +++++++++++++++++++
> >  arch/s390/kvm/Kconfig                         |  9 ++++
> >  8 files changed, 126 insertions(+), 9 deletions(-)
> >  create mode 100644 arch/s390/kernel/uv.c

(...)

> > diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
> > index d3db3d7ed077..652b36f0efca 100644
> > --- a/arch/s390/kvm/Kconfig
> > +++ b/arch/s390/kvm/Kconfig
> > @@ -55,6 +55,15 @@ config KVM_S390_UCONTROL
> > 
> >  	  If unsure, say N.
> > 
> > +config KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> > +	bool "Protected guests execution support"
> > +	depends on KVM
> > +	---help---
> > +	  Support hosting protected virtual machines isolated from the
> > +	  hypervisor.
> > +
> > +	  If unsure, say Y.
> > +
> >  # OK, it's a little counter-intuitive to do this, but it puts it neatly under
> >  # the virtualization menu.
> >  source "drivers/vhost/Kconfig"
> >   
> 
> As we have the prot_virt kernel paramter there is a way to fence this during runtime
> Not sure if we really need a build time fence. We could get rid of
> CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST and just use CONFIG_KVM instead,
> assuming that in the long run all distros will enable that anyway. 

I still need to read through the rest of this patch set to have an
informed opinion on that, which will probably take some more time.

> If other reviewers prefer to keep that extra option what about the following to the
> help section:
> 
> ----
> Support hosting protected virtual machines in KVM. The state of these machines like
> memory content or register content is protected from the host or host administrators.
> 
> Enabling this option will enable extra code that talks to a new firmware instance

"...that allows the host kernel to talk..." ?

> called ultravisor that will take care of protecting the guest while also enabling
> KVM to run this guest.
> 
> This feature must be enable by the kernel command line option prot_virt.

s/enable by/enabled via/

> 
> 	  If unsure, say Y.

Looks better. I'm continuing to read the rest of this series before I
say more, though :)