[RFC,02/37] s390/protvirt: introduce host side setup
diff mbox series

Message ID 20191024114059.102802-3-frankja@linux.ibm.com
State New
Headers show
Series
  • KVM: s390: Add support for protected VMs
Related show

Commit Message

Janosch Frank Oct. 24, 2019, 11:40 a.m. UTC
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

Comments

David Hildenbrand Oct. 24, 2019, 1:25 p.m. UTC | #1
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/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.
>   			See Documentation/admin-guide/blockdev/ramdisk.rst.
>   
> +	prot_virt=	[S390] enable hosting protected virtual machines
> +			isolated from the hypervisor (if hardware supports
> +			that).
> +			Format: <bool>

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).
David Hildenbrand Oct. 24, 2019, 1:27 p.m. UTC | #2
On 24.10.19 15:25, David Hildenbrand 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/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.
>>    			See Documentation/admin-guide/blockdev/ramdisk.rst.
>>    
>> +	prot_virt=	[S390] enable hosting protected virtual machines
>> +			isolated from the hypervisor (if hardware supports
>> +			that).
>> +			Format: <bool>
> 
> 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).
> 

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?
Christian Borntraeger Oct. 24, 2019, 1:40 p.m. UTC | #3
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 <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/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.
>>>                See Documentation/admin-guide/blockdev/ramdisk.rst.
>>>    +    prot_virt=    [S390] enable hosting protected virtual machines
>>> +            isolated from the hypervisor (if hardware supports
>>> +            that).
>>> +            Format: <bool>
>>
>> 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).
>>
> 
> 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
- 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
David Hildenbrand Oct. 24, 2019, 3:52 p.m. UTC | #4
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 <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/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.
>>>>                 See Documentation/admin-guide/blockdev/ramdisk.rst.
>>>>     +    prot_virt=    [S390] enable hosting protected virtual machines
>>>> +            isolated from the hypervisor (if hardware supports
>>>> +            that).
>>>> +            Format: <bool>
>>>
>>> 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).
>>>
>>
>> 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.

I can see that using the memblock allocator early is easier. But you 
waste "largish ... range of memory" even if you never run VMs.

Maybe something to work on in the future.

> - 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=off").)

Can you guys add these details to the patch description?
Claudio Imbrenda Oct. 24, 2019, 4:30 p.m. UTC | #5
On Thu, 24 Oct 2019 17:52:31 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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 <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/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.
> >>>>                 See
> >>>> Documentation/admin-guide/blockdev/ramdisk.rst.
> >>>>     +    prot_virt=    [S390] enable hosting protected virtual
> >>>> machines
> >>>> +            isolated from the hypervisor (if hardware supports
> >>>> +            that).
> >>>> +            Format: <bool>  
> >>>
> >>> 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).
> >>>  
> >>
> >> 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.

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
_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 can see that using the memblock allocator early is easier. But you 
> waste "largish ... range of memory" even if you never run VMs.

this is inevitable

> 
> Maybe something to work on in the future.

honestly unlikely. which is why protected virtualization needs to be
enabled explicitly

> 
> > - 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=off").)
> 
> Can you guys add these details to the patch description?

yeah, probably a good idea
David Hildenbrand Oct. 24, 2019, 4:54 p.m. UTC | #6
On 24.10.19 18:30, Claudio Imbrenda wrote:
> On Thu, 24 Oct 2019 17:52:31 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> 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 <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/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.
>>>>>>                  See
>>>>>> Documentation/admin-guide/blockdev/ramdisk.rst.
>>>>>>      +    prot_virt=    [S390] enable hosting protected virtual
>>>>>> machines
>>>>>> +            isolated from the hypervisor (if hardware supports
>>>>>> +            that).
>>>>>> +            Format: <bool>
>>>>>
>>>>> 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).
>>>>>   
>>>>
>>>> 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.
> 
> 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 
start/create an encrypted guest (IOW why you can't defer uv_init()) - 
but I am not past "[RFC 07/37] KVM: s390: protvirt: Secure memory is not 
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 
matter right now :) I understood the rational.

> 
>>
>>> - 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=off").)
>>
>> Can you guys add these details to the patch description?
> 
> yeah, probably a good idea
> 

If a patch explains why it does something and not only what it does 
usually makes me ask less stupid questions :)
Cornelia Huck Oct. 28, 2019, 2:54 p.m. UTC | #7
On Thu, 24 Oct 2019 07:40:24 -0400
Janosch Frank <frankja@linux.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.
> 
> 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.

I'm currently in the process of glancing across this patch set (won't
be able to get around to properly looking at it until next week the
earliest), so just a very high level comment:

I think there's not enough information in here to allow someone
configuring the kernel to decide what this is and if it would be useful
to them. This should probably be at least point to some document giving
some more details. Also, can you add a sentence where this feature is
actually expected to be available?

> +
> +	  If unsure, say Y.

Is 'Y' really the safe choice here? AFAICS, this is introducing new
code and not only trying to call new interfaces, if available. Is there
any drawback to enabling this on a kernel that won't run on a platform
supporting this feature? Is this supposed to be a common setup?

> +
>  # OK, it's a little counter-intuitive to do this, but it puts it neatly under
>  # the virtualization menu.
>  source "drivers/vhost/Kconfig"
Christian Borntraeger Oct. 28, 2019, 8:20 p.m. UTC | #8
On 28.10.19 15:54, Cornelia Huck wrote:

> I think there's not enough information in here to allow someone
> configuring the kernel to decide what this is and if it would be useful
> to them. This should probably be at least point to some document giving
> some more details. Also, can you add a sentence where this feature is
> actually expected to be available?
> 
>> +
>> +	  If unsure, say Y.
> 
> Is 'Y' really the safe choice here? AFAICS, this is introducing new
> code and not only trying to call new interfaces, if available. Is there
> any drawback to enabling this on a kernel that won't run on a platform
> supporting this feature? Is this supposed to be a common setup?

I would expect that this is enabled on distributions in the future. So
I think we should actually get rid of this Kconfig and always enable that code.
We just must pay attention to fence of all the new code if the user does 
not opt in. (e.g. prot_virt=0). We need to do that anyway and not hanving a
Kconfig forces us to be extra careful.
Christian Borntraeger Nov. 1, 2019, 8:53 a.m. UTC | #9
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/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.
>  			See Documentation/admin-guide/blockdev/ramdisk.rst.
> 
> +	prot_virt=	[S390] enable hosting protected virtual machines
> +			isolated from the hypervisor (if hardware supports
> +			that).
> +			Format: <bool>
> +
>  	psi=		[KNL] Enable or disable pressure stall information
>  			tracking.
>  			Format: <bool>
> diff --git a/arch/s390/boot/Makefile b/arch/s390/boot/Makefile
> index e2c47d3a1c89..82247e71617a 100644
> --- a/arch/s390/boot/Makefile
> +++ b/arch/s390/boot/Makefile
> @@ -37,7 +37,7 @@ CFLAGS_sclp_early_core.o += -I$(srctree)/drivers/s390/char
>  obj-y	:= head.o als.o startup.o mem_detect.o ipl_parm.o ipl_report.o
>  obj-y	+= string.o ebcdic.o sclp_early_core.o mem.o ipl_vmparm.o cmdline.o
>  obj-y	+= version.o pgm_check_info.o ctype.o text_dma.o
> -obj-$(CONFIG_PROTECTED_VIRTUALIZATION_GUEST)	+= uv.o
> +obj-$(findstring y, $(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) $(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST))	+= uv.o
>  obj-$(CONFIG_RELOCATABLE)	+= machine_kexec_reloc.o
>  obj-$(CONFIG_RANDOMIZE_BASE)	+= kaslr.o
>  targets	:= bzImage startup.a section_cmp.boot.data section_cmp.boot.preserved.data $(obj-y)
> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
> index ed007f4a6444..88cf8825d169 100644
> --- a/arch/s390/boot/uv.c
> +++ b/arch/s390/boot/uv.c
> @@ -3,7 +3,12 @@
>  #include <asm/facility.h>
>  #include <asm/sections.h>
> 
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>  int __bootdata_preserved(prot_virt_guest);
> +#endif
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> +struct uv_info __bootdata_preserved(uv_info);
> +#endif
> 
>  void uv_query_info(void)
>  {
> @@ -18,7 +23,20 @@ void uv_query_info(void)
>  	if (uv_call(0, (uint64_t)&uvcb))
>  		return;
> 
> -	if (test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
> +	if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)) {
> +		memcpy(uv_info.inst_calls_list, uvcb.inst_calls_list, sizeof(uv_info.inst_calls_list));
> +		uv_info.uv_base_stor_len = uvcb.uv_base_stor_len;
> +		uv_info.guest_base_stor_len = uvcb.conf_base_phys_stor_len;
> +		uv_info.guest_virt_base_stor_len = uvcb.conf_base_virt_stor_len;
> +		uv_info.guest_virt_var_stor_len = uvcb.conf_virt_var_stor_len;
> +		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
> +		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
> +		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
> +		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) &&
> +	    test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
>  	    test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list))
>  		prot_virt_guest = 1;
>  }
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index ef3c00b049ab..6db1bc495e67 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -44,7 +44,19 @@ struct uv_cb_qui {
>  	struct uv_cb_header header;
>  	u64 reserved08;
>  	u64 inst_calls_list[4];
> -	u64 reserved30[15];
> +	u64 reserved30[2];
> +	u64 uv_base_stor_len;
> +	u64 reserved48;
> +	u64 conf_base_phys_stor_len;
> +	u64 conf_base_virt_stor_len;
> +	u64 conf_virt_var_stor_len;
> +	u64 cpu_stor_len;
> +	u32 reserved68[3];
> +	u32 max_num_sec_conf;
> +	u64 max_guest_stor_addr;
> +	u8  reserved80[150-128];
> +	u16 max_guest_cpus;
> +	u64 reserved98;
>  } __packed __aligned(8);
> 
>  struct uv_cb_share {
> @@ -69,9 +81,21 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
>  	return cc;
>  }
> 
> -#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> +struct uv_info {
> +	unsigned long inst_calls_list[4];
> +	unsigned long uv_base_stor_len;
> +	unsigned long guest_base_stor_len;
> +	unsigned long guest_virt_base_stor_len;
> +	unsigned long guest_virt_var_stor_len;
> +	unsigned long guest_cpu_stor_len;
> +	unsigned long max_sec_stor_addr;
> +	unsigned int max_num_sec_conf;
> +	unsigned short max_guest_cpus;
> +};
> +extern struct uv_info uv_info;
>  extern int prot_virt_guest;
> 
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>  static inline int is_prot_virt_guest(void)
>  {
>  	return prot_virt_guest;
> @@ -121,11 +145,27 @@ static inline int uv_remove_shared(unsigned long addr)
>  	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
>  }
> 
> -void uv_query_info(void);
>  #else
>  #define is_prot_virt_guest() 0
>  static inline int uv_set_shared(unsigned long addr) { return 0; }
>  static inline int uv_remove_shared(unsigned long addr) { return 0; }
> +#endif
> +
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> +extern int prot_virt_host;
> +
> +static inline int is_prot_virt_host(void)
> +{
> +	return prot_virt_host;
> +}
> +#else
> +#define is_prot_virt_host() 0
> +#endif
> +
> +#if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) ||                          \
> +	defined(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)
> +void uv_query_info(void);
> +#else
>  static inline void uv_query_info(void) {}
>  #endif
> 
> diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
> index 7edbbcd8228a..fe4fe475f526 100644
> --- a/arch/s390/kernel/Makefile
> +++ b/arch/s390/kernel/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_PERF_EVENTS)	+= perf_cpum_cf_events.o perf_regs.o
>  obj-$(CONFIG_PERF_EVENTS)	+= perf_cpum_cf_diag.o
> 
>  obj-$(CONFIG_TRACEPOINTS)	+= trace.o
> +obj-$(findstring y, $(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) $(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST))	+= uv.o
> 
>  # vdso
>  obj-y				+= vdso64/
> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> index 3ff291bc63b7..f36370f8af38 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
> @@ -92,10 +92,6 @@ char elf_platform[ELF_PLATFORM_SIZE];
> 
>  unsigned long int_hwcap = 0;
> 
> -#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> -int __bootdata_preserved(prot_virt_guest);
> -#endif
> -
>  int __bootdata(noexec_disabled);
>  int __bootdata(memory_end_set);
>  unsigned long __bootdata(memory_end);
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> new file mode 100644
> index 000000000000..35ce89695509
> --- /dev/null
> +++ b/arch/s390/kernel/uv.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Common Ultravisor functions and initialization
> + *
> + * Copyright IBM Corp. 2019
> + */
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/sizes.h>
> +#include <linux/bitmap.h>
> +#include <linux/memblock.h>
> +#include <asm/facility.h>
> +#include <asm/sections.h>
> +#include <asm/uv.h>
> +
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> +int __bootdata_preserved(prot_virt_guest);
> +#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.");
> +	}
> +
> +	if (prot_virt_host && !test_facility(158)) {
> +		prot_virt_host = 0;
> +		pr_info("The ultravisor call facility is not available.");
> +	}
> +
> +	return rc;
> +}
> +early_param("prot_virt", prot_virt_setup);
> +#endif
> 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. 
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
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.

	  If unsure, say Y.

----


Either way, the remaining part is

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Cornelia Huck Nov. 4, 2019, 2:26 p.m. UTC | #10
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 :)
Cornelia Huck Nov. 4, 2019, 3:54 p.m. UTC | #11
On Thu, 24 Oct 2019 07:40:24 -0400
Janosch Frank <frankja@linux.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.
> 
> 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/boot/uv.c b/arch/s390/boot/uv.c
> index ed007f4a6444..88cf8825d169 100644
> --- a/arch/s390/boot/uv.c
> +++ b/arch/s390/boot/uv.c
> @@ -3,7 +3,12 @@
>  #include <asm/facility.h>
>  #include <asm/sections.h>
>  
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>  int __bootdata_preserved(prot_virt_guest);
> +#endif
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> +struct uv_info __bootdata_preserved(uv_info);
> +#endif

Two functions with the same name, but different signatures look really
ugly.

Also, what happens if I want to build just a single kernel image for
both guest and host?

>  
>  void uv_query_info(void)
>  {
> @@ -18,7 +23,20 @@ void uv_query_info(void)
>  	if (uv_call(0, (uint64_t)&uvcb))
>  		return;
>  
> -	if (test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
> +	if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)) {

Do we always have everything needed for a host if uv_call() is
successful?

> +		memcpy(uv_info.inst_calls_list, uvcb.inst_calls_list, sizeof(uv_info.inst_calls_list));
> +		uv_info.uv_base_stor_len = uvcb.uv_base_stor_len;
> +		uv_info.guest_base_stor_len = uvcb.conf_base_phys_stor_len;
> +		uv_info.guest_virt_base_stor_len = uvcb.conf_base_virt_stor_len;
> +		uv_info.guest_virt_var_stor_len = uvcb.conf_virt_var_stor_len;
> +		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
> +		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
> +		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
> +		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) &&
> +	    test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
>  	    test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list))

Especially as it looks like we need to test for those two commands to
determine whether we have support for a guest.

>  		prot_virt_guest = 1;
>  }
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index ef3c00b049ab..6db1bc495e67 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -44,7 +44,19 @@ struct uv_cb_qui {
>  	struct uv_cb_header header;
>  	u64 reserved08;
>  	u64 inst_calls_list[4];
> -	u64 reserved30[15];
> +	u64 reserved30[2];
> +	u64 uv_base_stor_len;
> +	u64 reserved48;
> +	u64 conf_base_phys_stor_len;
> +	u64 conf_base_virt_stor_len;
> +	u64 conf_virt_var_stor_len;
> +	u64 cpu_stor_len;
> +	u32 reserved68[3];
> +	u32 max_num_sec_conf;
> +	u64 max_guest_stor_addr;
> +	u8  reserved80[150-128];
> +	u16 max_guest_cpus;
> +	u64 reserved98;
>  } __packed __aligned(8);
>  
>  struct uv_cb_share {
> @@ -69,9 +81,21 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
>  	return cc;
>  }
>  
> -#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> +struct uv_info {
> +	unsigned long inst_calls_list[4];
> +	unsigned long uv_base_stor_len;
> +	unsigned long guest_base_stor_len;
> +	unsigned long guest_virt_base_stor_len;
> +	unsigned long guest_virt_var_stor_len;
> +	unsigned long guest_cpu_stor_len;
> +	unsigned long max_sec_stor_addr;
> +	unsigned int max_num_sec_conf;
> +	unsigned short max_guest_cpus;
> +};

What is the main difference between uv_info and uv_cb_qui? The
alignment of max_sec_stor_addr?

> +extern struct uv_info uv_info;
>  extern int prot_virt_guest;
>  
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>  static inline int is_prot_virt_guest(void)
>  {
>  	return prot_virt_guest;
> @@ -121,11 +145,27 @@ static inline int uv_remove_shared(unsigned long addr)
>  	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
>  }
>  
> -void uv_query_info(void);
>  #else
>  #define is_prot_virt_guest() 0
>  static inline int uv_set_shared(unsigned long addr) { return 0; }
>  static inline int uv_remove_shared(unsigned long addr) { return 0; }
> +#endif
> +
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> +extern int prot_virt_host;
> +
> +static inline int is_prot_virt_host(void)
> +{
> +	return prot_virt_host;
> +}
> +#else
> +#define is_prot_virt_host() 0
> +#endif
> +
> +#if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) ||                          \
> +	defined(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)
> +void uv_query_info(void);
> +#else
>  static inline void uv_query_info(void) {}
>  #endif

(...)

> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> new file mode 100644
> index 000000000000..35ce89695509
> --- /dev/null
> +++ b/arch/s390/kernel/uv.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Common Ultravisor functions and initialization
> + *
> + * Copyright IBM Corp. 2019
> + */
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/sizes.h>
> +#include <linux/bitmap.h>
> +#include <linux/memblock.h>
> +#include <asm/facility.h>
> +#include <asm/sections.h>
> +#include <asm/uv.h>
> +
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> +int __bootdata_preserved(prot_virt_guest);
> +#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.");
> +	}
> +
> +	if (prot_virt_host && !test_facility(158)) {

Why not check for that facility earlier? If it is not present, we
cannot run as a prot virt guest, either.

> +		prot_virt_host = 0;
> +		pr_info("The ultravisor call facility is not available.");
> +	}
> +
> +	return rc;
> +}
> +early_param("prot_virt", prot_virt_setup);

Maybe rename this to prot_virt_host?

> +#endif

(...)
Christian Borntraeger Nov. 4, 2019, 5:50 p.m. UTC | #12
On 04.11.19 16:54, Cornelia Huck wrote:
> On Thu, 24 Oct 2019 07:40:24 -0400
> Janosch Frank <frankja@linux.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.
>>
>> 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/boot/uv.c b/arch/s390/boot/uv.c
>> index ed007f4a6444..88cf8825d169 100644
>> --- a/arch/s390/boot/uv.c
>> +++ b/arch/s390/boot/uv.c
>> @@ -3,7 +3,12 @@
>>  #include <asm/facility.h>
>>  #include <asm/sections.h>
>>  
>> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>>  int __bootdata_preserved(prot_virt_guest);
>> +#endif
>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
>> +struct uv_info __bootdata_preserved(uv_info);
>> +#endif
> 
> Two functions with the same name, but different signatures look really
> ugly.
> 
> Also, what happens if I want to build just a single kernel image for
> both guest and host?

This is not two functions with the same name. It is 2 variable declarations with
the __bootdata_preserved helper. We expect to have all distro kernels to enable
both. 

> 
>>  
>>  void uv_query_info(void)
>>  {
>> @@ -18,7 +23,20 @@ void uv_query_info(void)
>>  	if (uv_call(0, (uint64_t)&uvcb))
>>  		return;
>>  
>> -	if (test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
>> +	if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)) {
> 
> Do we always have everything needed for a host if uv_call() is
> successful?

The uv_call is the query call. It will provide the list of features. We check that
later on.

> 
>> +		memcpy(uv_info.inst_calls_list, uvcb.inst_calls_list, sizeof(uv_info.inst_calls_list));
>> +		uv_info.uv_base_stor_len = uvcb.uv_base_stor_len;
>> +		uv_info.guest_base_stor_len = uvcb.conf_base_phys_stor_len;
>> +		uv_info.guest_virt_base_stor_len = uvcb.conf_base_virt_stor_len;
>> +		uv_info.guest_virt_var_stor_len = uvcb.conf_virt_var_stor_len;
>> +		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
>> +		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
>> +		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
>> +		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
>> +	}
>> +
>> +	if (IS_ENABLED(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) &&
>> +	    test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
>>  	    test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list))
> 
> Especially as it looks like we need to test for those two commands to
> determine whether we have support for a guest.
> 
>>  		prot_virt_guest = 1;
>>  }
>> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
>> index ef3c00b049ab..6db1bc495e67 100644
>> --- a/arch/s390/include/asm/uv.h
>> +++ b/arch/s390/include/asm/uv.h
>> @@ -44,7 +44,19 @@ struct uv_cb_qui {
>>  	struct uv_cb_header header;
>>  	u64 reserved08;
>>  	u64 inst_calls_list[4];
>> -	u64 reserved30[15];
>> +	u64 reserved30[2];
>> +	u64 uv_base_stor_len;
>> +	u64 reserved48;
>> +	u64 conf_base_phys_stor_len;
>> +	u64 conf_base_virt_stor_len;
>> +	u64 conf_virt_var_stor_len;
>> +	u64 cpu_stor_len;
>> +	u32 reserved68[3];
>> +	u32 max_num_sec_conf;
>> +	u64 max_guest_stor_addr;
>> +	u8  reserved80[150-128];
>> +	u16 max_guest_cpus;
>> +	u64 reserved98;
>>  } __packed __aligned(8);
>>  
>>  struct uv_cb_share {
>> @@ -69,9 +81,21 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
>>  	return cc;
>>  }
>>  
>> -#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>> +struct uv_info {
>> +	unsigned long inst_calls_list[4];
>> +	unsigned long uv_base_stor_len;
>> +	unsigned long guest_base_stor_len;
>> +	unsigned long guest_virt_base_stor_len;
>> +	unsigned long guest_virt_var_stor_len;
>> +	unsigned long guest_cpu_stor_len;
>> +	unsigned long max_sec_stor_addr;
>> +	unsigned int max_num_sec_conf;
>> +	unsigned short max_guest_cpus;
>> +};
> 
> What is the main difference between uv_info and uv_cb_qui? The
> alignment of max_sec_stor_addr?

One is the hardware data structure for query, the other one is the Linux
internal state.

> 
>> +extern struct uv_info uv_info;
>>  extern int prot_virt_guest;
>>  
>> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>>  static inline int is_prot_virt_guest(void)
>>  {
>>  	return prot_virt_guest;
>> @@ -121,11 +145,27 @@ static inline int uv_remove_shared(unsigned long addr)
>>  	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
>>  }
>>  
>> -void uv_query_info(void);
>>  #else
>>  #define is_prot_virt_guest() 0
>>  static inline int uv_set_shared(unsigned long addr) { return 0; }
>>  static inline int uv_remove_shared(unsigned long addr) { return 0; }
>> +#endif
>> +
>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
>> +extern int prot_virt_host;
>> +
>> +static inline int is_prot_virt_host(void)
>> +{
>> +	return prot_virt_host;
>> +}
>> +#else
>> +#define is_prot_virt_host() 0
>> +#endif
>> +
>> +#if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) ||                          \
>> +	defined(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)
>> +void uv_query_info(void);
>> +#else
>>  static inline void uv_query_info(void) {}
>>  #endif
> 
> (...)
> 
[...]
Cornelia Huck Nov. 5, 2019, 9:26 a.m. UTC | #13
On Mon, 4 Nov 2019 18:50:12 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 04.11.19 16:54, Cornelia Huck wrote:
> > On Thu, 24 Oct 2019 07:40:24 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:

> >> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
> >> index ed007f4a6444..88cf8825d169 100644
> >> --- a/arch/s390/boot/uv.c
> >> +++ b/arch/s390/boot/uv.c
> >> @@ -3,7 +3,12 @@
> >>  #include <asm/facility.h>
> >>  #include <asm/sections.h>
> >>  
> >> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> >>  int __bootdata_preserved(prot_virt_guest);
> >> +#endif
> >> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> >> +struct uv_info __bootdata_preserved(uv_info);
> >> +#endif  
> > 
> > Two functions with the same name, but different signatures look really
> > ugly.
> > 
> > Also, what happens if I want to build just a single kernel image for
> > both guest and host?  
> 
> This is not two functions with the same name. It is 2 variable declarations with
> the __bootdata_preserved helper. We expect to have all distro kernels to enable
> both. 

Ah ok, I misread that. (I'm blaming lack of sleep :/)

> 
> >   
> >>  
> >>  void uv_query_info(void)
> >>  {
> >> @@ -18,7 +23,20 @@ void uv_query_info(void)
> >>  	if (uv_call(0, (uint64_t)&uvcb))
> >>  		return;
> >>  
> >> -	if (test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
> >> +	if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)) {  
> > 
> > Do we always have everything needed for a host if uv_call() is
> > successful?  
> 
> The uv_call is the query call. It will provide the list of features. We check that
> later on.

Hm yes. I'm just seeing the guest side check for features, while the
host code just seems to go ahead and copies things. (later on == later
patches?)

> 
> >   
> >> +		memcpy(uv_info.inst_calls_list, uvcb.inst_calls_list, sizeof(uv_info.inst_calls_list));
> >> +		uv_info.uv_base_stor_len = uvcb.uv_base_stor_len;
> >> +		uv_info.guest_base_stor_len = uvcb.conf_base_phys_stor_len;
> >> +		uv_info.guest_virt_base_stor_len = uvcb.conf_base_virt_stor_len;
> >> +		uv_info.guest_virt_var_stor_len = uvcb.conf_virt_var_stor_len;
> >> +		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
> >> +		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
> >> +		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
> >> +		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
> >> +	}
> >> +
> >> +	if (IS_ENABLED(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) &&
> >> +	    test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
> >>  	    test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list))  
> > 
> > Especially as it looks like we need to test for those two commands to
> > determine whether we have support for a guest.
> >   
> >>  		prot_virt_guest = 1;
> >>  }
> >> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> >> index ef3c00b049ab..6db1bc495e67 100644
> >> --- a/arch/s390/include/asm/uv.h
> >> +++ b/arch/s390/include/asm/uv.h
> >> @@ -44,7 +44,19 @@ struct uv_cb_qui {
> >>  	struct uv_cb_header header;
> >>  	u64 reserved08;
> >>  	u64 inst_calls_list[4];
> >> -	u64 reserved30[15];
> >> +	u64 reserved30[2];
> >> +	u64 uv_base_stor_len;
> >> +	u64 reserved48;
> >> +	u64 conf_base_phys_stor_len;
> >> +	u64 conf_base_virt_stor_len;
> >> +	u64 conf_virt_var_stor_len;
> >> +	u64 cpu_stor_len;
> >> +	u32 reserved68[3];
> >> +	u32 max_num_sec_conf;
> >> +	u64 max_guest_stor_addr;
> >> +	u8  reserved80[150-128];
> >> +	u16 max_guest_cpus;
> >> +	u64 reserved98;
> >>  } __packed __aligned(8);
> >>  
> >>  struct uv_cb_share {
> >> @@ -69,9 +81,21 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
> >>  	return cc;
> >>  }
> >>  
> >> -#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> >> +struct uv_info {
> >> +	unsigned long inst_calls_list[4];
> >> +	unsigned long uv_base_stor_len;
> >> +	unsigned long guest_base_stor_len;
> >> +	unsigned long guest_virt_base_stor_len;
> >> +	unsigned long guest_virt_var_stor_len;
> >> +	unsigned long guest_cpu_stor_len;
> >> +	unsigned long max_sec_stor_addr;
> >> +	unsigned int max_num_sec_conf;
> >> +	unsigned short max_guest_cpus;
> >> +};  
> > 
> > What is the main difference between uv_info and uv_cb_qui? The
> > alignment of max_sec_stor_addr?  
> 
> One is the hardware data structure for query, the other one is the Linux
> internal state.

That's clear; I'm mainly wondering about what is simply copied vs. what
needs to be calculated.
Thomas Huth Nov. 8, 2019, 12:14 p.m. UTC | #14
On 05/11/2019 10.26, Cornelia Huck wrote:
> On Mon, 4 Nov 2019 18:50:12 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 04.11.19 16:54, Cornelia Huck wrote:
>>> On Thu, 24 Oct 2019 07:40:24 -0400
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>>>> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
>>>> index ed007f4a6444..88cf8825d169 100644
>>>> --- a/arch/s390/boot/uv.c
>>>> +++ b/arch/s390/boot/uv.c
>>>> @@ -3,7 +3,12 @@
>>>>   #include <asm/facility.h>
>>>>   #include <asm/sections.h>
>>>>   
>>>> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
>>>>   int __bootdata_preserved(prot_virt_guest);
>>>> +#endif
>>>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
>>>> +struct uv_info __bootdata_preserved(uv_info);
>>>> +#endif
>>>
>>> Two functions with the same name, but different signatures look really
>>> ugly.
>>>
>>> Also, what happens if I want to build just a single kernel image for
>>> both guest and host?
>>
>> This is not two functions with the same name. It is 2 variable declarations with
>> the __bootdata_preserved helper. We expect to have all distro kernels to enable
>> both.
> 
> Ah ok, I misread that. (I'm blaming lack of sleep :/)

Honestly, I have to admit that I mis-read this in the same way as 
Cornelia at the first glance. Why is that macro not using capital 
letters? ... then it would be way more obvious that it's not about a 
function prototype...

  Thomas
Janosch Frank Nov. 12, 2019, 2:47 p.m. UTC | #15
On 11/4/19 3:26 PM, Cornelia Huck wrote:
> 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..." ?

"allows a Linux hypervisor 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 :)
>

Patch
diff mbox series

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.
 			See Documentation/admin-guide/blockdev/ramdisk.rst.
 
+	prot_virt=	[S390] enable hosting protected virtual machines
+			isolated from the hypervisor (if hardware supports
+			that).
+			Format: <bool>
+
 	psi=		[KNL] Enable or disable pressure stall information
 			tracking.
 			Format: <bool>
diff --git a/arch/s390/boot/Makefile b/arch/s390/boot/Makefile
index e2c47d3a1c89..82247e71617a 100644
--- a/arch/s390/boot/Makefile
+++ b/arch/s390/boot/Makefile
@@ -37,7 +37,7 @@  CFLAGS_sclp_early_core.o += -I$(srctree)/drivers/s390/char
 obj-y	:= head.o als.o startup.o mem_detect.o ipl_parm.o ipl_report.o
 obj-y	+= string.o ebcdic.o sclp_early_core.o mem.o ipl_vmparm.o cmdline.o
 obj-y	+= version.o pgm_check_info.o ctype.o text_dma.o
-obj-$(CONFIG_PROTECTED_VIRTUALIZATION_GUEST)	+= uv.o
+obj-$(findstring y, $(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) $(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST))	+= uv.o
 obj-$(CONFIG_RELOCATABLE)	+= machine_kexec_reloc.o
 obj-$(CONFIG_RANDOMIZE_BASE)	+= kaslr.o
 targets	:= bzImage startup.a section_cmp.boot.data section_cmp.boot.preserved.data $(obj-y)
diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
index ed007f4a6444..88cf8825d169 100644
--- a/arch/s390/boot/uv.c
+++ b/arch/s390/boot/uv.c
@@ -3,7 +3,12 @@ 
 #include <asm/facility.h>
 #include <asm/sections.h>
 
+#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
 int __bootdata_preserved(prot_virt_guest);
+#endif
+#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
+struct uv_info __bootdata_preserved(uv_info);
+#endif
 
 void uv_query_info(void)
 {
@@ -18,7 +23,20 @@  void uv_query_info(void)
 	if (uv_call(0, (uint64_t)&uvcb))
 		return;
 
-	if (test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
+	if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)) {
+		memcpy(uv_info.inst_calls_list, uvcb.inst_calls_list, sizeof(uv_info.inst_calls_list));
+		uv_info.uv_base_stor_len = uvcb.uv_base_stor_len;
+		uv_info.guest_base_stor_len = uvcb.conf_base_phys_stor_len;
+		uv_info.guest_virt_base_stor_len = uvcb.conf_base_virt_stor_len;
+		uv_info.guest_virt_var_stor_len = uvcb.conf_virt_var_stor_len;
+		uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
+		uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
+		uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
+		uv_info.max_guest_cpus = uvcb.max_guest_cpus;
+	}
+
+	if (IS_ENABLED(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) &&
+	    test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
 	    test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list))
 		prot_virt_guest = 1;
 }
diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index ef3c00b049ab..6db1bc495e67 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -44,7 +44,19 @@  struct uv_cb_qui {
 	struct uv_cb_header header;
 	u64 reserved08;
 	u64 inst_calls_list[4];
-	u64 reserved30[15];
+	u64 reserved30[2];
+	u64 uv_base_stor_len;
+	u64 reserved48;
+	u64 conf_base_phys_stor_len;
+	u64 conf_base_virt_stor_len;
+	u64 conf_virt_var_stor_len;
+	u64 cpu_stor_len;
+	u32 reserved68[3];
+	u32 max_num_sec_conf;
+	u64 max_guest_stor_addr;
+	u8  reserved80[150-128];
+	u16 max_guest_cpus;
+	u64 reserved98;
 } __packed __aligned(8);
 
 struct uv_cb_share {
@@ -69,9 +81,21 @@  static inline int uv_call(unsigned long r1, unsigned long r2)
 	return cc;
 }
 
-#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
+struct uv_info {
+	unsigned long inst_calls_list[4];
+	unsigned long uv_base_stor_len;
+	unsigned long guest_base_stor_len;
+	unsigned long guest_virt_base_stor_len;
+	unsigned long guest_virt_var_stor_len;
+	unsigned long guest_cpu_stor_len;
+	unsigned long max_sec_stor_addr;
+	unsigned int max_num_sec_conf;
+	unsigned short max_guest_cpus;
+};
+extern struct uv_info uv_info;
 extern int prot_virt_guest;
 
+#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
 static inline int is_prot_virt_guest(void)
 {
 	return prot_virt_guest;
@@ -121,11 +145,27 @@  static inline int uv_remove_shared(unsigned long addr)
 	return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
 }
 
-void uv_query_info(void);
 #else
 #define is_prot_virt_guest() 0
 static inline int uv_set_shared(unsigned long addr) { return 0; }
 static inline int uv_remove_shared(unsigned long addr) { return 0; }
+#endif
+
+#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
+extern int prot_virt_host;
+
+static inline int is_prot_virt_host(void)
+{
+	return prot_virt_host;
+}
+#else
+#define is_prot_virt_host() 0
+#endif
+
+#if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) ||                          \
+	defined(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)
+void uv_query_info(void);
+#else
 static inline void uv_query_info(void) {}
 #endif
 
diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
index 7edbbcd8228a..fe4fe475f526 100644
--- a/arch/s390/kernel/Makefile
+++ b/arch/s390/kernel/Makefile
@@ -78,6 +78,7 @@  obj-$(CONFIG_PERF_EVENTS)	+= perf_cpum_cf_events.o perf_regs.o
 obj-$(CONFIG_PERF_EVENTS)	+= perf_cpum_cf_diag.o
 
 obj-$(CONFIG_TRACEPOINTS)	+= trace.o
+obj-$(findstring y, $(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) $(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST))	+= uv.o
 
 # vdso
 obj-y				+= vdso64/
diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
index 3ff291bc63b7..f36370f8af38 100644
--- a/arch/s390/kernel/setup.c
+++ b/arch/s390/kernel/setup.c
@@ -92,10 +92,6 @@  char elf_platform[ELF_PLATFORM_SIZE];
 
 unsigned long int_hwcap = 0;
 
-#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
-int __bootdata_preserved(prot_virt_guest);
-#endif
-
 int __bootdata(noexec_disabled);
 int __bootdata(memory_end_set);
 unsigned long __bootdata(memory_end);
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
new file mode 100644
index 000000000000..35ce89695509
--- /dev/null
+++ b/arch/s390/kernel/uv.c
@@ -0,0 +1,48 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common Ultravisor functions and initialization
+ *
+ * Copyright IBM Corp. 2019
+ */
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/sizes.h>
+#include <linux/bitmap.h>
+#include <linux/memblock.h>
+#include <asm/facility.h>
+#include <asm/sections.h>
+#include <asm/uv.h>
+
+#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
+int __bootdata_preserved(prot_virt_guest);
+#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.");
+	}
+
+	if (prot_virt_host && !test_facility(158)) {
+		prot_virt_host = 0;
+		pr_info("The ultravisor call facility is not available.");
+	}
+
+	return rc;
+}
+early_param("prot_virt", prot_virt_setup);
+#endif
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"