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
>>
>
>
