Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220829171021.701198-1-pgonda@google.com> <20220829171021.701198-4-pgonda@google.com>
 <Yz8U2k7Tu8QQNhhq@google.com>
In-Reply-To: <Yz8U2k7Tu8QQNhhq@google.com>
From:   Peter Gonda <pgonda@google.com>
Date:   Tue, 11 Oct 2022 11:38:01 -0600
Message-ID: <CAMkAt6qQLO-6W8Ek-syUSzZpWLPDe8EzzfuWvY3iQZhczti7Pw@mail.gmail.com>
Subject: Re: [V4 3/8] KVM: selftests: add hooks for managing encrypted guest memory
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        marcorr@google.com, michael.roth@amd.com, thomas.lendacky@amd.com,
        joro@8bytes.org, mizhang@google.com, pbonzini@redhat.com,
        andrew.jones@linux.dev
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 6, 2022 at 11:48 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Aug 29, 2022, Peter Gonda wrote:
> > +static vm_paddr_t
> > +_vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, vm_paddr_t paddr_min,
>
> Do not wrap before the function name.  Linus has a nice explanation/rant on this[*].
> Note to self, add a Vim macro for this...
>
> [*] https://lore.kernel.org/all/CAHk-=wjoLAYG446ZNHfg=GhjSY6nFmuB_wA8fYd5iLBNXjo9Bw@mail.gmail.com
>

Fixed. Thanks. I'll work on a fix to my VIM setup.

> > +                 uint32_t memslot, bool encrypt)
> >  {
> >       struct userspace_mem_region *region;
> >       sparsebit_idx_t pg, base;
> > @@ -1152,12 +1156,22 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> >               abort();
> >       }
> >
> > -     for (pg = base; pg < base + num; ++pg)
> > +     for (pg = base; pg < base + num; ++pg) {
> >               sparsebit_clear(region->unused_phy_pages, pg);
> > +             if (encrypt)
>
> prefer s/encrypt/private, and s/encrypted_phy_pages/private_phy_pages.  pKVM
> doesn't rely on encryption, and it's not impossible that x86 will someday gain
> similar functionality.  And "encrypted" is also technically wrong for SEV and TDX,
> as shared memory can also be encrypted with a common key.

Makes sense. Private or protected sound better.

>
> > +                     sparsebit_set(region->encrypted_phy_pages, pg);
> > +     }
> >
> >       return base * vm->page_size;
> >  }
> >
> > +vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> > +                           vm_paddr_t paddr_min, uint32_t memslot)
> > +{
> > +     return _vm_phy_pages_alloc(vm, num, paddr_min, memslot,
> > +                                vm->memcrypt.enc_by_default);
>
> enc_by_default yields a bizarre API.  The behavior depends on whether or not the
> VM is protected, and whether or not the VM wants to protect memory by default.
>
> For simplicity, IMO vm_phy_pages_alloc() should allocate memory as private if the
> VM supports protected memory, i.e. just have vm->protected or whatever and use
> that here.

Removed "enc_by_default" concept.

>
> > +}
> > +
> >  vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
> >                            uint32_t memslot)
> >  {
> > @@ -1741,6 +1755,10 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
> >                       region->host_mem);
> >               fprintf(stream, "%*sunused_phy_pages: ", indent + 2, "");
> >               sparsebit_dump(stream, region->unused_phy_pages, 0);
> > +             if (vm->memcrypt.enabled) {
>
> vm->protected

renamed memcrypt -> protected.

>
> > +                     fprintf(stream, "%*sencrypted_phy_pages: ", indent + 2, "");
> > +                     sparsebit_dump(stream, region->encrypted_phy_pages, 0);
> > +             }
> >       }
> >       fprintf(stream, "%*sMapped Virtual Pages:\n", indent, "");
> >       sparsebit_dump(stream, vm->vpages_mapped, indent + 2);
> > @@ -1989,3 +2007,31 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
> >               break;
> >       }
> >  }
> > +
> > +void vm_set_memory_encryption(struct kvm_vm *vm, bool enc_by_default, bool has_enc_bit,
> > +                           uint8_t enc_bit)
> > +{
> > +     vm->memcrypt.enabled = true;
> > +     vm->memcrypt.enc_by_default = enc_by_default;
> > +     vm->memcrypt.has_enc_bit = has_enc_bit;
> > +     vm->memcrypt.enc_bit = enc_bit;
> > +}
> > +
> > +const struct sparsebit *
> > +vm_get_encrypted_phy_pages(struct kvm_vm *vm, int slot, vm_paddr_t *gpa_start,
> > +                        uint64_t *size)
>
> Bad wrap.

Fixed.

>
> > +{
> > +     struct userspace_mem_region *region;
> > +
> > +     if (!vm->memcrypt.enabled)
>
> This seems rather silly, why not TEST_ASSERT()?
>
> > +             return NULL;
> > +
> > +     region = memslot2region(vm, slot);
> > +     if (!region)
>
> Same here, TEST_ASSERT() seems more appropriate.
>
> Actually, I can't envision a use outside of SEV.  AFAIK, no other architecture
> does the whole "launch update" thing.  I.e. just open code this in sev_encrypt().
> The more generic API that will be useful for other VM types will be to query if a
> specific GPA is private vs. shared.

Good point. I'll move this code into that sev_encrypt() flow and
remove this function completely.

>
> > +             return NULL;
> > +
> > +     *size = region->region.memory_size;
> > +     *gpa_start = region->region.guest_phys_addr;
> > +
> > +     return region->encrypted_phy_pages;
> > +}
> > --
> > 2.37.2.672.g94769d06f0-goog
> >