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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > ---
> > 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).
> > +
> > + 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:
> > + 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;
> > +}
> > #endif
> >
>
> Looks good to me from what I can tell.
>
> --
>
> Thanks,
>
> David / dhildenb
>
