| Message ID | 20191024114059.102802-3-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Add support for protected VMs | expand |
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).
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?
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
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?
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
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 :)
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"
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.
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>
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 :)
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 (...)
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 > > (...) > [...]
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.
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
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 :) >
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"