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, 18 Aug 2022 00:33:39 +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, vannapurve@google.com
Subject: Re: [V3 08/11] KVM: selftests: add library for creating/interacting
 with SEV guests
Message-ID: <Yv2I48GqFpc9PHIy@google.com>
References: <20220810152033.946942-1-pgonda@google.com>
 <20220810152033.946942-9-pgonda@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220810152033.946942-9-pgonda@google.com>
Precedence: bulk

On Wed, Aug 10, 2022, Peter Gonda wrote:
> +enum {
> +       SEV_GSTATE_UNINIT = 0,
> +       SEV_GSTATE_LUPDATE,
> +       SEV_GSTATE_LSECRET,
> +       SEV_GSTATE_RUNNING,
> +};
> +

Name the enum, e.g. enum sev_guest_state?

And s/GSTATE/GUEST?  Ugh, AMD's documentation uses GSTATE.

But looking at the docs, I only see GSTATE_LAUNCH?  Or does SEV have different
status codes than -ES and/or -SNP?

> +struct kvm_vm *sev_get_vm(struct sev_vm *sev)
> +{
> +	return sev->vm;

Why bother with a wrapper?

> +}
> +
> +uint8_t sev_get_enc_bit(struct sev_vm *sev)
> +{

Same here, IMO it just obfuscates code with no real benefit.  ANd it's inconsistent,
e.g. why have a wrapper for enc_bit but not sev->fd?

> +	return sev->enc_bit;
> +}
> +
> +void sev_ioctl(int sev_fd, int cmd, void *data)
> +{
> +	int ret;
> +	struct sev_issue_cmd arg;
> +
> +	arg.cmd = cmd;
> +	arg.data = (unsigned long)data;
> +	ret = ioctl(sev_fd, SEV_ISSUE_CMD, &arg);
> +	TEST_ASSERT(ret == 0,
> +		    "SEV ioctl %d failed, error: %d, fw_error: %d",
> +		    cmd, ret, arg.error);
> +}
> +
> +void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data)
> +{
> +	struct kvm_sev_cmd arg = {0};
> +	int ret;
> +
> +	arg.id = cmd;
> +	arg.sev_fd = sev->fd;
> +	arg.data = (__u64)data;
> +
> +	ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_OP, &arg);
> +	TEST_ASSERT(ret == 0,
> +		    "SEV KVM ioctl %d failed, rc: %i errno: %i (%s), fw_error: %d",
> +		    cmd, ret, errno, strerror(errno), arg.error);
> +}
> +
> +/* Local helpers. */
> +
> +static void

Don't split here, e.g. a grep/search for the function, should also show the return
type and any attributes, e.g. "static" vs. something else is typically much more
interesting than the parameters (and parameters is not a fully solvable problem).

> +sev_register_user_region(struct sev_vm *sev, void *hva, uint64_t size)

Align like so:

static void sev_register_user_region(struct sev_vm *sev, void *hva,
				     uint64_t size)

or maybe even let it poke out.

> +{
> +	struct kvm_enc_region range = {0};
> +	int ret;
> +
> +	pr_debug("%s: hva: %p, size: %lu\n", __func__, hva, size);
> +
> +	range.addr = (__u64)hva;
> +	range.size = size;
> +
> +	ret = ioctl(sev->vm->fd, KVM_MEMORY_ENCRYPT_REG_REGION, &range);
> +	TEST_ASSERT(ret == 0, "failed to register user range, errno: %i\n", errno);
> +}
> +
> +static void
> +sev_encrypt_phy_range(struct sev_vm *sev, vm_paddr_t gpa, uint64_t size)

Same thing here.

> +{
> +	struct kvm_sev_launch_update_data ksev_update_data = {0};
> +
> +	pr_debug("%s: addr: 0x%lx, size: %lu\n", __func__, gpa, size);
> +
> +	ksev_update_data.uaddr = (__u64)addr_gpa2hva(sev->vm, gpa);
> +	ksev_update_data.len = size;
> +
> +	kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_UPDATE_DATA, &ksev_update_data);
> +}
> +
> +static void sev_encrypt(struct sev_vm *sev)
> +{
> +	const struct sparsebit *enc_phy_pages;
> +	struct kvm_vm *vm = sev->vm;
> +	sparsebit_idx_t pg = 0;
> +	vm_paddr_t gpa_start;
> +	uint64_t memory_size;
> +
> +	/* Only memslot 0 supported for now. */

Eww.  Haven't looked at this in depth, but is there a way to avoid hardcoding the
memslot in this code?

> +void sev_vm_launch(struct sev_vm *sev)
> +{
> +	struct kvm_sev_launch_start ksev_launch_start = {0};
> +	struct kvm_sev_guest_status ksev_status = {0};

Doesn't " = {};" do the same thing?  And for the status, and any other cases where
userspace is reading, wouldn't it be better from a test coverage perspective to
_not_ zero the data?  Hmm, though I suppose false passes are possible in that case...

> +	/* Need to use ucall_shared for synchronization. */
> +	//ucall_init_ops(sev_get_vm(sev), NULL, &ucall_ops_halt);

Can this be deleted?  If not, what's up?