Re: [PATCH] KVM: arm64: Do not size-order align pkvm_alloc_private_va_range()

[Vincent Donnefort <vdonnefort@google.com>]

On Thu, Aug 10, 2023 at 09:42:54AM -0700, Kalesh Singh wrote:
> On Thu, Aug 10, 2023 at 6:34 AM Vincent Donnefort <vdonnefort@google.com> wrote:
> >
> > commit f922c13e778d ("KVM: arm64: Introduce pkvm_alloc_private_va_range()")
> > added an alignment for the start address of any allocation into the nVHE
> > protected hypervisor private VA range.
> >
> > This alignment (order of the size of the allocation) intends to enable
> > efficient stack guard verification (if the PAGE_SHIFT bit is zero, the
> > stack pointer is on the stack guard page, a stack overflow occurred).
> >
> > But a such alignment only makes sense for stack allocation and can waste
> > a lot of VA space. So instead make a stack specific allocation function
> > (hyp_create_stack()) handling the stack guard page requirements, while
> > other users (e.g. fixmap) will only get page alignment.
> >
> > Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
> >
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/mm.h b/arch/arm64/kvm/hyp/include/nvhe/mm.h
> > index d5ec972b5c1e..71d17ddb562f 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/mm.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/mm.h
> > @@ -18,6 +18,7 @@ void *hyp_fixmap_map(phys_addr_t phys);
> >  void hyp_fixmap_unmap(void);
> >
> >  int hyp_create_idmap(u32 hyp_va_bits);
> > +int hyp_create_stack(unsigned long stack_pa, unsigned long *stack_va);
> >  int hyp_map_vectors(void);
> >  int hyp_back_vmemmap(phys_addr_t back);
> >  int pkvm_cpu_set_vector(enum arm64_hyp_spectre_vector slot);
> > diff --git a/arch/arm64/kvm/hyp/nvhe/mm.c b/arch/arm64/kvm/hyp/nvhe/mm.c
> > index 318298eb3d6b..275e33a8be9a 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/mm.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/mm.c
> > @@ -44,6 +44,28 @@ static int __pkvm_create_mappings(unsigned long start, unsigned long size,
> >         return err;
> >  }
> >
> > +static int __pkvm_alloc_private_va_range(unsigned long start, size_t size)
> > +{
> > +       unsigned long cur;
> > +       int ret = 0;
> > +
> > +       hyp_assert_lock_held(&pkvm_pgd_lock);
> > +
> > +       if (!start || start < __io_map_base)
> > +               return -EINVAL;
> > +
> > +       /* The allocated size is always a multiple of PAGE_SIZE */
> > +       cur = start + PAGE_ALIGN(size);
> > +
> > +       /* Are we overflowing on the vmemmap ? */
> > +       if (cur > __hyp_vmemmap)
> > +               ret = -ENOMEM;
> > +       else
> > +               __io_map_base = cur;
> > +
> > +       return ret;
> > +}
> > +
> >  /**
> >   * pkvm_alloc_private_va_range - Allocates a private VA range.
> >   * @size:      The size of the VA range to reserve.
> > @@ -56,27 +78,16 @@ static int __pkvm_create_mappings(unsigned long start, unsigned long size,
> >   */
> >  int pkvm_alloc_private_va_range(size_t size, unsigned long *haddr)
> >  {
> > -       unsigned long base, addr;
> > -       int ret = 0;
> > +       unsigned long addr;
> > +       int ret;
> >
> >         hyp_spin_lock(&pkvm_pgd_lock);
> > -
> > -       /* Align the allocation based on the order of its size */
> > -       addr = ALIGN(__io_map_base, PAGE_SIZE << get_order(size));
> > -
> > -       /* The allocated size is always a multiple of PAGE_SIZE */
> > -       base = addr + PAGE_ALIGN(size);
> > -
> > -       /* Are we overflowing on the vmemmap ? */
> > -       if (!addr || base > __hyp_vmemmap)
> > -               ret = -ENOMEM;
> > -       else {
> > -               __io_map_base = base;
> > -               *haddr = addr;
> > -       }
> > -
> > +       addr = __io_map_base;
> > +       ret = __pkvm_alloc_private_va_range(addr, size);
> >         hyp_spin_unlock(&pkvm_pgd_lock);
> >
> > +       *haddr = addr;
> > +
> >         return ret;
> >  }
> >
> > @@ -340,6 +351,39 @@ int hyp_create_idmap(u32 hyp_va_bits)
> >         return __pkvm_create_mappings(start, end - start, start, PAGE_HYP_EXEC);
> >  }
> >
> > +int hyp_create_stack(unsigned long stack_pa, unsigned long *stack_va)
> > +{
> 
> Hi Vincent,
> 
> Can you rename this instead as pkvm_create_stack(), since it's for the
> protected case.

Not sure about the pkvm_create_stack(), I wanted to have something that looks
like hyp_create_idmap() that is also pkvm only. And as the latter, the stack
creation happens only during the setup, so I thought I'd better keep this
format.

Thoughts?

> 
> And I assume we want the same thing for the conventional nVHE mode:
> remove the alignment from hyp_alloc_private_va_range() into a
> hyp_create_stack().

Ah yes, good catch, even though it'll be less of a problem for non-protected
nVHE, it'd probably be a good thing of aligning both. Might do that in a
separated patch though.

> 
> Thanks,
> Kalesh

Thanks for having a look!

> 
> 
> > +       unsigned long addr;
> > +       size_t size;
> > +       int ret;
> > +
> > +       hyp_spin_lock(&pkvm_pgd_lock);
> > +
> > +       /* Make room for the guard page */
> > +       size = PAGE_SIZE * 2;
> > +       addr = ALIGN(__io_map_base, size);
> > +
> > +       ret = __pkvm_alloc_private_va_range(addr, size);
> > +       if (!ret) {
> > +               /*
> > +                * Since the stack grows downwards, map the stack to the page
> > +                * at the higher address and leave the lower guard page
> > +                * unbacked.
> > +                *
> > +                * Any valid stack address now has the PAGE_SHIFT bit as 1
> > +                * and addresses corresponding to the guard page have the
> > +                * PAGE_SHIFT bit as 0 - this is used for overflow detection.
> > +                */
> > +               ret = kvm_pgtable_hyp_map(&pkvm_pgtable, addr + PAGE_SIZE,
> > +                                         PAGE_SIZE, stack_pa, PAGE_HYP);
> > +       }
> > +       hyp_spin_unlock(&pkvm_pgd_lock);
> > +
> > +       *stack_va = addr + size;
> > +
> > +       return ret;
> > +}
> > +
> >  static void *admit_host_page(void *arg)
> >  {
> >         struct kvm_hyp_memcache *host_mc = arg;
> > diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> > index bb98630dfeaf..782c8d0fb905 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> > @@ -121,33 +121,11 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,
> >                 if (ret)
> >                         return ret;
> >
> > -               /*
> > -                * Allocate a contiguous HYP private VA range for the stack
> > -                * and guard page. The allocation is also aligned based on
> > -                * the order of its size.
> > -                */
> > -               ret = pkvm_alloc_private_va_range(PAGE_SIZE * 2, &hyp_addr);
> > +               ret = hyp_create_stack(params->stack_pa, &hyp_addr);
> >                 if (ret)
> >                         return ret;
> >
> > -               /*
> > -                * Since the stack grows downwards, map the stack to the page
> > -                * at the higher address and leave the lower guard page
> > -                * unbacked.
> > -                *
> > -                * Any valid stack address now has the PAGE_SHIFT bit as 1
> > -                * and addresses corresponding to the guard page have the
> > -                * PAGE_SHIFT bit as 0 - this is used for overflow detection.
> > -                */
> > -               hyp_spin_lock(&pkvm_pgd_lock);
> > -               ret = kvm_pgtable_hyp_map(&pkvm_pgtable, hyp_addr + PAGE_SIZE,
> > -                                       PAGE_SIZE, params->stack_pa, PAGE_HYP);
> > -               hyp_spin_unlock(&pkvm_pgd_lock);
> > -               if (ret)
> > -                       return ret;
> > -
> > -               /* Update stack_hyp_va to end of the stack's private VA range */
> > -               params->stack_hyp_va = hyp_addr + (2 * PAGE_SIZE);
> > +               params->stack_hyp_va = hyp_addr;
> >         }
> >
> >         /*
> >
> > base-commit: 52a93d39b17dc7eb98b6aa3edb93943248e03b2f
> > --
> > 2.41.0.640.ga95def55d0-goog
> >