Message-ID: <20220802142638.vw6iw3dvyl4j5iyr@kamzik>
Date:   Tue, 2 Aug 2022 16:26:38 +0200
From:   Andrew Jones <andrew.jones@...ux.dev>
To:     Peter Gonda <pgonda@...gle.com>
Cc:     kvm list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Marc Orr <marcorr@...gle.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Michael Roth <michael.roth@....com>,
        "Lendacky, Thomas" <thomas.lendacky@....com>,
        Joerg Roedel <joro@...tes.org>,
        Mingwei Zhang <mizhang@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [V2 09/11] KVM: selftests: Make ucall work with encrypted guests

On Tue, Aug 02, 2022 at 07:51:40AM -0600, Peter Gonda wrote:
> On Tue, Aug 2, 2022 at 3:49 AM Andrew Jones <andrew.jones@...ux.dev> wrote:
> >
> > On Mon, Aug 01, 2022 at 01:11:07PM -0700, Peter Gonda wrote:
> > > Add support for encrypted, SEV, guests in the ucall framework. If
> > > encryption is enabled set up a pool of ucall structs in the guests'
> > > shared memory region. This was suggested in the thread on "[RFC PATCH
> > > 00/10] KVM: selftests: Add support for test-selectable ucall
> > > implementations". Using a listed as suggested there doesn't work well
> > > because the list is setup using HVAs not GVAs so use a bitmap + array
> > > solution instead to get the same pool result.
> > >
> > > Suggested-by: Sean Christopherson <seanjc@...gle.com>
> > > Signed-off-by: Peter Gonda <pgonda@...gle.com>
> > > ---
> > >  .../selftests/kvm/include/kvm_util_base.h     |   3 +
> > >  .../selftests/kvm/include/ucall_common.h      |  14 +--
> > >  .../testing/selftests/kvm/lib/ucall_common.c  | 112 +++++++++++++++++-
> > >  3 files changed, 115 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > index 8ce9e5be70a3..ad4abc6be1ab 100644
> > > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > > @@ -102,6 +102,9 @@ struct kvm_vm {
> > >       int stats_fd;
> > >       struct kvm_stats_header stats_header;
> > >       struct kvm_stats_desc *stats_desc;
> > > +
> > > +     bool use_ucall_list;
> >
> > use_ucall_pool
> >
> > > +     struct list_head ucall_list;
> > >  };
> 
> Will do. I also need to remove this |ucall_list| member.
> 
> > >
> > >
> > > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> > > index c1bc8e33ef3f..a96220ac6024 100644
> > > --- a/tools/testing/selftests/kvm/include/ucall_common.h
> > > +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> > > @@ -22,6 +22,10 @@ enum {
> > >  struct ucall {
> > >       uint64_t cmd;
> > >       uint64_t args[UCALL_MAX_ARGS];
> > > +
> > > +     /* For encrypted guests. */
> >
> > Please, no 'encrypted' words in ucall files. ucalls shouldn't care about
> > guest types. Indeed, the summary of this patch could even drop the word
> > 'encrypted'. This patch is adding support for ucall pools, which is
> > motivated by the need to support encrypted guests (the motivation should
> > go in the commit message, but otherwise the patch should be ucall specific
> > and guest type agnostic)
> >
> > > +     uint64_t idx;
> >
> > We don't need 'idx' because 'hva' will always be at the
> > idx * sizeof(struct ucall) offset of ucall_hdr->ucalls, which means
> > we can always calculate it,
> >
> >  static inline size_t uc_pool_idx(struct ucall *uc)
> >  {
> >         return uc->hva - ucall_hdr->ucalls;
> >  }
> 
> Good call, I didn't think of that.
> 
> >
> > > +     struct ucall *hva;
> > >  };
> > >
> > >  void ucall_arch_init(struct kvm_vm *vm, void *arg);
> > > @@ -32,15 +36,9 @@ uint64_t ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
> > >  void ucall(uint64_t cmd, int nargs, ...);
> > >  uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> > >
> > > -static inline void ucall_init(struct kvm_vm *vm, void *arg)
> > > -{
> > > -     ucall_arch_init(vm, arg);
> > > -}
> > > +void ucall_init(struct kvm_vm *vm, void *arg);
> > >
> > > -static inline void ucall_uninit(struct kvm_vm *vm)
> > > -{
> > > -     ucall_arch_uninit(vm);
> > > -}
> > > +void ucall_uninit(struct kvm_vm *vm);
> > >
> > >  #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4)       \
> > >                               ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
> > > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> > > index a060252bab40..feb0173179ec 100644
> > > --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> > > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> > > @@ -1,22 +1,122 @@
> > >  // SPDX-License-Identifier: GPL-2.0-only
> > >  #include "kvm_util.h"
> > > +#include "linux/types.h"
> > > +#include "linux/bitmap.h"
> > > +#include "linux/atomic.h"
> > > +
> > > +struct ucall_header {
> > > +     DECLARE_BITMAP(in_use, KVM_MAX_VCPUS);
> > > +     struct ucall ucalls[KVM_MAX_VCPUS];
> > > +};
> > > +
> > > +static bool use_ucall_list;
> >
> > use_ucall_pool
> >
> > > +static struct ucall_header *ucall_hdr;
> >
> > ucall_pool
> 
> Will update naming here.
> 
> >
> > > +
> > > +void ucall_init(struct kvm_vm *vm, void *arg)
> > > +{
> > > +     struct ucall *uc;
> > > +     struct ucall_header *hdr;
> > > +     vm_vaddr_t vaddr;
> > > +     int i;
> > > +
> > > +     use_ucall_list = vm->use_ucall_list;
> > > +     sync_global_to_guest(vm, use_ucall_list);
> > > +     if (!use_ucall_list)
> > > +             goto out;
> > > +
> > > +     TEST_ASSERT(!ucall_hdr,
> > > +                 "Only a single encrypted guest at a time for ucalls.");
> >
> > single VM at a time
> >
> > And I'd leave that on a single line. checkpatch allows up to 100 chars and
> > I'm included to use all 110 chars of it.
> 
> Sounds good.
> 
> >
> > > +     vaddr = vm_vaddr_alloc_shared(vm, sizeof(*hdr), vm->page_size);
> > > +     hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr);
> > > +     memset(hdr, 0, sizeof(*hdr));
> > > +
> > > +     for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> > > +             uc = &hdr->ucalls[i];
> > > +             uc->hva = uc;
> > > +             uc->idx = i;
> > > +     }
> > > +
> > > +     ucall_hdr = (struct ucall_header *)vaddr;
> > > +     sync_global_to_guest(vm, ucall_hdr);
> > > +
> > > +out:
> > > +     ucall_arch_init(vm, arg);
> > > +}
> > > +
> > > +void ucall_uninit(struct kvm_vm *vm)
> > > +{
> > > +     use_ucall_list = false;
> > > +     ucall_hdr = NULL;
> >
> > It's unlikely we'd ever change the ucall setup on a running VM,
> > but we should sync these changes to the guest for good measure.
> 
> Hmm I'll need to have some notion of encrypted guests here then. Since
> the guests page tables also get encrypted we can no longer get the
> gva2gpa translations so sync_global_to_guest() cannot be done due to
> the addr_ga2hva(). So is it OK if this call references encryption like
> below?
> 
>     use_ucall_list = false;
>      ucall_hdr = NULL;
> if (!vm->memencrypt.encrypted) {
>    sync_global_to_guest(vm, use_ucall_list);
>   sync_global_to_guest(vm, ucall_hdr);
> }

Yes, I guess that's the best we can do.

If sync_global_to_guest() becomes useless once a guest is encrypted then
maybe it should assert !vm->memencrypt.encrypted.

> 
> >
> > > +
> > > +     ucall_arch_uninit(vm);
> > > +}
> > > +
> > > +static struct ucall *ucall_alloc(void)
> > > +{
> > > +     struct ucall *uc = NULL;
> > > +     int i;
> > > +
> > > +     if (!use_ucall_list)
> > > +             goto out;
> > > +
> > > +     for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> > > +             if (atomic_test_and_set_bit(i, ucall_hdr->in_use))
> > > +                     continue;
> > > +
> > > +             uc = &ucall_hdr->ucalls[i];
> > > +     }
> >
> > This is not what I suggested in the last revision and it's still wrong.
> > Here, you're still looping through all of the pool and returning the last
> > one.
> >
> > What I suggested was
> >
> >         for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> >                 if (!atomic_test_and_set_bit(i, ucall_hdr->in_use)) {
> >                         uc = &ucall_hdr->ucalls[i];
> >                         break;
> >                 }
> >         }
> >
> > Notice how we leave the loop early when we find a free uc.
> 
> Ah my mistake, I didn't get this fix into this series
> 
> >
> > We should also zero the contents of the uc buffer before returning.
> 
> Ack will do.
> 
> >
> > > +
> > > +out:
> > > +     return uc;
> > > +}
> > > +
> > > +static void ucall_free(struct ucall *uc)
> > > +{
> > > +     if (!use_ucall_list)
> > > +             return;
> > > +
> > > +     clear_bit(uc->idx, ucall_hdr->in_use);
> >
> > This seems to be the one and only use of idx which is another argument for
> > dropping the variable and just calculating it instead.
> 
> Will do.
> 
> >
> > > +}
> > > +
> > > +static vm_vaddr_t get_ucall_addr(struct ucall *uc)
> > > +{
> > > +     if (use_ucall_list)
> > > +             return (vm_vaddr_t)uc->hva;
> >
> > A comment explaining that this hva has already been synchronized
> > with the guest would be good. Or maybe a different name for the
> > pointer than hva, one which conveys that it's a pointer that works
> > as both an hva and gva would be better.
> 
> I don't think this pointer does work as a gva though since its created
> by just taking &uc during host execution. I can add a comment that
> mentions this is a hva is already translated for the host and not for
> guest use other than communicating with the host. I thought 'hva' was
> a good name here since it is literally the hva of this ucall struct,
> what name would you prefer?

My mistake, I forgot how we initialized uc->hva. 'hva' is a good name.

> 
> >
> >
> > > +
> > > +     return (vm_vaddr_t)uc;
> > > +}
> > >
> > >  void ucall(uint64_t cmd, int nargs, ...)
> > >  {
> > > -     struct ucall uc = {
> > > -             .cmd = cmd,
> > > -     };
> >
> > This zeros all members except cmd.
> >
> > > +     struct ucall *uc;
> > > +     struct ucall tmp;
> >
> > And this makes tmp full of stack garbage, so in the non uc-pool case we
> > no longer have an equivalent uc. Please initialize tmp the same way uc
> > was initialized.
> 
> Ack, I'll get this fixed.
> 
> >
> > >       va_list va;
> > >       int i;
> > >
> > > +     uc = ucall_alloc();
> > > +     if (!uc)
> > > +             uc = &tmp;
> > > +
> > > +     uc->cmd = cmd;
> > > +
> > >       nargs = min(nargs, UCALL_MAX_ARGS);
> > >
> > >       va_start(va, nargs);
> > >       for (i = 0; i < nargs; ++i)
> > > -             uc.args[i] = va_arg(va, uint64_t);
> > > +             uc->args[i] = va_arg(va, uint64_t);
> > >       va_end(va);
> > >
> > > -     ucall_arch_do_ucall((vm_vaddr_t)&uc);
> > > +     ucall_arch_do_ucall(get_ucall_addr(uc));
> >
> > We don't need get_ucall_addr(). Just do the if-else right here
> >
> >    if (use_ucall_list)
> >        ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> >    else
> >       ucall_arch_do_ucall((vm_vaddr_t)uc);
> 
> Will do.
> 
> >
> > > +
> > > +     ucall_free(uc);
> > > +}
> > > +
> > > +static void *get_ucall_hva(struct kvm_vm *vm, uint64_t uc)
> > > +{
> > > +     if (vm->use_ucall_list)
> > > +             return (void *)uc;
> > > +
> > > +     return addr_gva2hva(vm, uc);
> > >  }
> > >
> > >  uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > > @@ -27,7 +127,7 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
> > >       if (!uc)
> > >               uc = &ucall;
> > >
> > > -     addr = addr_gva2hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
> > > +     addr = get_ucall_hva(vcpu->vm, ucall_arch_get_ucall(vcpu));
> >
> > Wait, I thought we wanted ucall_arch_get_ucall() to return hvas?
> > It would make more sense if it did, since it's called from the
> > host.
> 
> As you noted in [V2 07/11] I updated ucall_arch_get_ucall() to return
> the gva. I figured this was just more boilerplate which could be
> consolidated given all archs right now just use the addr_gva2hva()
> helper to do this translation. This change also allowed for the
> "use_ucall_pool" to be completely contained in ucall_common.c instead
> of spilling into each arch specific file. If there is a preference to
> have the arch specific calls translate from gva to hva I can do that,
> I think I'll need to have them all check if "use_ucall_pool" is set
> though. Thoughts?

I just checked what I wrote for the last version of this patch,
"[RFC V1 08/10] KVM: selftests: Make ucall work with encrypted guests".
It was

> ...we're only updating x86's ucall_arch_get_ucall() to return gvas.
> What about the other architectures? Anyway, I'd rather we don't
> change ucall_arch_get_ucall() to return gvas. They should continue
> returning hvas and any trickery needed to translate a pool uc to
> an hva should be put inside ucall_arch_get_ucall().

(It's comforting to see I was consistent, at least wrt the last review.)

So, I would keep returning hvas and put the 'if use_ucall_pool' inside
the ucall_arch_get_ucall() functions.

Thanks,
drew

> 
> We can still update ucall_arch_get_ucall() to return a pointer type so
> that we can return NULL instead of 0.
> 
> >
> > >       if (addr) {
> > >               memcpy(uc, addr, sizeof(*uc));
> > >               vcpu_run_complete_io(vcpu);
> > > --
> > > 2.37.1.455.g008518b4e5-goog
> > >
> >
> > Thanks,
> > drew

Powered by blists - more mailing lists