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;
Date:   Thu, 6 Oct 2022 17:48:10 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Peter Gonda <pgonda@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
Subject: Re: [V4 3/8] KVM: selftests: add hooks for managing encrypted guest
 memory
Message-ID: <Yz8U2k7Tu8QQNhhq@google.com>
References: <20220829171021.701198-1-pgonda@google.com>
 <20220829171021.701198-4-pgonda@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220829171021.701198-4-pgonda@google.com>
Precedence: bulk

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

> +		    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.

> +			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.

> +}
> +
>  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

> +			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.

> +{
> +	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.

> +		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
>