Re: [PATCH 04/35] s390/protvirt: add ultravisor initialization

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

 




On 14.02.20 11:25, David Hildenbrand wrote:
> [...]
> 
>>  #if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) ||                          \
>> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
>> index f2ab2528859f..5f178d557cc8 100644
>> --- a/arch/s390/kernel/setup.c
>> +++ b/arch/s390/kernel/setup.c
>> @@ -560,6 +560,8 @@ static void __init setup_memory_end(void)
>>  			vmax = _REGION1_SIZE; /* 4-level kernel page table */
>>  	}
>>  
>> +	adjust_to_uv_max(&vmax);
> 
> I'd somewhat prefer
> 
> if (prot_virt_host)
> 	adjust_to_uv_max(&vmax);
> 
>> +

fine with me. ack

>>  	/* module area is at the end of the kernel address space. */
>>  	MODULES_END = vmax;
>>  	MODULES_VADDR = MODULES_END - MODULES_LEN;
>> @@ -1140,6 +1142,7 @@ void __init setup_arch(char **cmdline_p)
>>  	 */
>>  	memblock_trim_memory(1UL << (MAX_ORDER - 1 + PAGE_SHIFT));
>>  
>> +	setup_uv();
> 
> and
> 
> if (prot_virt_host)
> 	setup_uv();

ack
> 
> Moving the checks out of the functions. Makes it clearer that this is
> optional.
> 
>>  	setup_memory_end();
>>  	setup_memory();
>>  	dma_contiguous_reserve(memory_end);
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index fbf2a98de642..a06a628a88da 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -46,4 +46,57 @@ static int __init prot_virt_setup(char *val)
>>  	return rc;
>>  }
>>  early_param("prot_virt", prot_virt_setup);
>> +
>> +static int __init uv_init(unsigned long stor_base, unsigned long stor_len)
>> +{
>> +	struct uv_cb_init uvcb = {
>> +		.header.cmd = UVC_CMD_INIT_UV,
>> +		.header.len = sizeof(uvcb),
>> +		.stor_origin = stor_base,
>> +		.stor_len = stor_len,
>> +	};
>> +	int cc;
>> +
>> +	cc = uv_call(0, (uint64_t)&uvcb);
> 
> Could do
> 
> int cc = uv_call(0, (uint64_t)&uvcb);

I could actually get rid of the cc and the comparison with UVC_RC_EXECUTED.
When the condition code is 0, rc must be 1.

Something like
	if (uv_call(0,.....

> 
>> +	if (cc || uvcb.header.rc != UVC_RC_EXECUTED) {
>> +		pr_err("Ultravisor init failed with cc: %d rc: 0x%hx\n", cc,
>> +		       uvcb.header.rc);
>> +		return -1;
>> +	}
>> +	return 0;
>> +}
>> +
>> +void __init setup_uv(void)
>> +{
>> +	unsigned long uv_stor_base;
>> +
>> +	if (!prot_virt_host)
>> +		return;
>> +
>> +	uv_stor_base = (unsigned long)memblock_alloc_try_nid(
>> +		uv_info.uv_base_stor_len, SZ_1M, SZ_2G,
>> +		MEMBLOCK_ALLOC_ACCESSIBLE, NUMA_NO_NODE);
>> +	if (!uv_stor_base) {
>> +		pr_info("Failed to reserve %lu bytes for ultravisor base storage\n",
>> +			uv_info.uv_base_stor_len);
> 
> pr_err() ? pr_warn()

ack.


> 
>> +		goto fail;
>> +	}
>> +
>> +	if (uv_init(uv_stor_base, uv_info.uv_base_stor_len)) {
>> +		memblock_free(uv_stor_base, uv_info.uv_base_stor_len);
>> +		goto fail;
>> +	}
>> +
>> +	pr_info("Reserving %luMB as ultravisor base storage\n",
>> +		uv_info.uv_base_stor_len >> 20);
>> +	return;
>> +fail:
> 
> I'd add here:
> 
> pr_info("Disabling support for protected virtualization");

ack

> 
>> +	prot_virt_host = 0;> +}
>> +
>> +void adjust_to_uv_max(unsigned long *vmax)
>> +{
>> +	if (prot_virt_host && *vmax > uv_info.max_sec_stor_addr)
>> +		*vmax = uv_info.max_sec_stor_addr;
> 
> Once you move the prot virt check out of this function


> 
> 	*vmax = max_t(unsigned long, *vmax, uv_info.max_sec_stor_addr);

ack
> 
>> +}
>>  #endif
>>
> 
> 




[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