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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux