Re: [RFC 03/37] s390/protvirt: add ultravisor initialization

[David Hildenbrand <david@redhat.com>]

On 28.10.19 16:48, Vasily Gorbik wrote:
> On Fri, Oct 25, 2019 at 11:21:05AM +0200, David Hildenbrand wrote:
>> On 24.10.19 13:40, Janosch Frank wrote:
>>> From: Vasily Gorbik <gor@linux.ibm.com>
>>>
>>> Before being able to host protected virtual machines, donate some of
>>> the memory to the ultravisor. Besides that the ultravisor might impose
>>> addressing limitations for memory used to back protected VM storage. Treat
>>> that limit as protected virtualization host's virtual memory limit.
>>>
>>> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
>>> ---
>>>    arch/s390/include/asm/uv.h | 16 ++++++++++++
>>>    arch/s390/kernel/setup.c   |  3 +++
>>>    arch/s390/kernel/uv.c      | 53 ++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 72 insertions(+)
>>>
>>> --- a/arch/s390/kernel/setup.c
>>> +++ b/arch/s390/kernel/setup.c
>>> @@ -567,6 +567,8 @@ static void __init setup_memory_end(void)
>>>    			vmax = _REGION1_SIZE; /* 4-level kernel page table */
>>>    	}
>>> +	adjust_to_uv_max(&vmax);
>>
>> I do wonder what would happen if vmax < max_physmem_end. Not sure if that is
>> relevant at all.
> 
> Then identity mapping would be shorter then actual physical memory available
> and everything above would be lost. But in reality "max_sec_stor_addr"
> is big enough to not worry about it in the foreseeable future at all.
> 
>>> +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);
>>> +		goto fail;
>>> +	}
>>
>> If I'm not wrong, we could setup/reserve a CMA area here and defer the
>> actual allocation. Then, any MOVABLE data can end up on this CMA area until
>> needed.
>>
>> But I am neither an expert on CMA nor on UV, so most probably what I say is
>> wrong ;)
> 
>  From pure memory management this sounds like a good idea. And I tried
> it and cma_declare_contiguous fulfills our needs, just had to export
> cma_alloc/cma_release symbols. Nevertheless, delaying ultravisor init means we
> would be potentially left with vmax == max_sec_stor_addr even if we wouldn't
> be able to run protected VMs after all (currently setup_uv() is called
> before kernel address space layout setup). Another much more fundamental
> reason is that ultravisor init has to be called with a single cpu running,
> which means it's easy to do before bringing other cpus up and we currently
> don't have api to stop cpus at a later point (stop_machine won't cut it).

Interesting point, I guess. One could hack around that. Emphasis on 
*hack* :) In stop_machine() you caught all CPUs. You could just 
temporarily SIGP STOP all running ones, issue the UV init call, and SIGP 
START them again. Not sure how that works with SMP, though ...

But yeah, this is stuff for the future, just an idea from my side :)

-- 

Thanks,

David / dhildenb