On Mon, 4 Nov 2019 18:50:12 +0100
Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
> On 04.11.19 16:54, Cornelia Huck wrote:
> > On Thu, 24 Oct 2019 07:40:24 -0400
> > Janosch Frank <frankja@xxxxxxxxxxxxx> 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.
