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

From: Cornelia Huck <cohuck@redhat.com>
To: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Janosch Frank <frankja@linux.vnet.ibm.com>,
	KVM <kvm@vger.kernel.org>, David Hildenbrand <david@redhat.com>,
	Thomas Huth <thuth@redhat.com>,
	Ulrich Weigand <Ulrich.Weigand@de.ibm.com>,
	Claudio Imbrenda <imbrenda@linux.ibm.com>,
	Andrea Arcangeli <aarcange@redhat.com>
Subject: Re: [RFCv2 02/37] s390/protvirt: introduce host side setup
Date: Tue, 4 Feb 2020 10:28:20 +0100	[thread overview]
Message-ID: <20200204102820.51081649.cohuck@redhat.com> (raw)
In-Reply-To: <0310f99f-6d1e-b1bb-9313-be2a92c601ba@de.ibm.com>

On Mon, 3 Feb 2020 23:03:42 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 03.02.20 18:12, Cornelia Huck wrote:
> > On Mon,  3 Feb 2020 08:19:22 -0500
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> From: Vasily Gorbik <gor@linux.ibm.com>
> >>
> >> Introduce KVM_S390_PROTECTED_VIRTUALIZATION_HOST kbuild option for
> >> protected virtual machines hosting support code.  
> > 
> > Hm... I seem to remember that you wanted to drop this config option and
> > always build the code, in order to reduce complexity. Have you
> > reconsidered this?  
> 
> I am still in favour of removing this, but I did not get an "yes, lets do
> it" answer. Since removing is easier than re-adding its still in.

ok

> 
>  [...]
> >> + * Copyright IBM Corp. 2019  
> > 
> > Happy new year?  
> 
> yep :-)
> [..]
> 
> >> +
> >> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> >> +int __bootdata_preserved(prot_virt_guest);  
> > 
> > Confused. You have this and uv_info below both in this file and in
> > boot/uv.c. Is there some magic happening in __bootdata_preserved()?  
> 
> Yes, this is information that is transferred from the decompressor
> to Linux. 
> I think we discussed about this the last time as well?

I think I was confused about different things last time...

But that is probably a sign that this wants a comment :)

> 
> 
> >   
> >> +#endif
> >> +
> >> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> >> +int prot_virt_host;
> >> +EXPORT_SYMBOL(prot_virt_host);
> >> +struct uv_info __bootdata_preserved(uv_info);
> >> +EXPORT_SYMBOL(uv_info);
> >> +
> >> +static int __init prot_virt_setup(char *val)
> >> +{
> >> +	bool enabled;
> >> +	int rc;
> >> +
> >> +	rc = kstrtobool(val, &enabled);
> >> +	if (!rc && enabled)
> >> +		prot_virt_host = 1;
> >> +
> >> +	if (is_prot_virt_guest() && prot_virt_host) {
> >> +		prot_virt_host = 0;
> >> +		pr_info("Running as protected virtualization guest.");  
> > 
> > Trying to disentangle that a bit in my mind...
> > 
> > If we don't have facility 158, is_prot_virt_guest() will return 0. If
> > protected host support has been requested, we'll print a message below
> > (and turn it off).  
> 
> yes, a guest cannot be a host. 
> > 
> > If the hardware provides the facilities for running as a protected virt
> > guest, we turn off protected virt host support if requested and print a
> > messages that we're a guest.
> > 
> > Two questions:
> > - Can the hardware ever provide both host and guest interfaces at the
> >   same time? I guess not; maybe add a comment?  
> 
> Right, you are either guest or host. 
> 
> > - Do we also want to print a message that we're running as a guest if
> >   the user didn't enable host support? If not, maybe prefix the message
> >   with "Cannot enable support for protected virtualization host:" or
> >   so? (Maybe also a good idea for the message below.)  
> 
> Line too long and I hate breaking string over multiple lines.
> I can change if somebody comes up with a proper message that is not too long.
> 

Fair enough; it's just that it's not very clear from the messages in
the log what happened. Maybe

"prot_virt: Running as protected virtualization guest."
"prot_virt: The ultravisor call facility is not available."

That at least links back to the kernel parameter.

[Aside: Would prot_virt_host be a better name? But that probably moves
us into bikeshed territory.]