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:   Wed, 21 Dec 2022 13:13:48 -0800
In-Reply-To: <20221018205845.770121-6-pgonda@google.com> (message from Peter
 Gonda on Tue, 18 Oct 2022 13:58:43 -0700)
Mime-Version: 1.0
Message-ID: <diqz3598v4s3.fsf@google.com>
Subject: Re: [PATCH V5 5/7] KVM: selftests: add library for
 creating/interacting with SEV guests
From:   Ackerley Tng <ackerleytng@google.com>
To:     Peter Gonda <pgonda@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        marcorr@google.com, seanjc@google.com, michael.roth@amd.com,
        thomas.lendacky@amd.com, joro@8bytes.org, mizhang@google.com,
        pbonzini@redhat.com, andrew.jones@linux.dev, pgonda@google.com,
        vannapurve@google.com
Content-Type: text/plain; charset="UTF-8"; format=flowed; delsp=yes
Precedence: bulk


> +static void encrypt_region(struct kvm_vm *vm, struct  
> userspace_mem_region *region)
> +{
> +	const struct sparsebit *protected_phy_pages =
> +		region->protected_phy_pages;
> +	const uint64_t memory_size = region->region.memory_size;
> +	const vm_paddr_t gpa_start = region->region.guest_phys_addr;
> +	sparsebit_idx_t pg = 0;
> +
> +	sev_register_user_region(vm, region);
> +
> +	while (pg < (memory_size / vm->page_size)) {
> +		sparsebit_idx_t nr_pages;
> +
> +		if (sparsebit_is_clear(protected_phy_pages, pg)) {
> +			pg = sparsebit_next_set(protected_phy_pages, pg);
> +			if (!pg)
> +				break;
> +		}
> +
> +		nr_pages = sparsebit_next_clear(protected_phy_pages, pg) - pg;
> +		if (nr_pages <= 0)
> +			nr_pages = 1;

I think this may not be correct in the case where the sparsebit has the
range [x, 2**64-1] (inclusive) set. In that case, sparsebit_next_clear()
will return 0, but the number of pages could be more than 1.

> +
> +		sev_launch_update_data(vm, gpa_start + pg * vm->page_size,

Computing the beginning of the gpa range with

gpa_start + pg * vm->page_size

only works if this memory region's gpa_start is 0.

> +				       nr_pages * vm->page_size);
> +		pg += nr_pages;
> +	}
> +}

Here's a suggestion (I'm using this on a TDX version of this patch)


/**
  * Iterate over set ranges within sparsebit @s. In each iteration,
  * @range_begin and @range_end will take the beginning and end of the set  
range,
  * which are of type sparsebit_idx_t.
  *
  * For example, if the range [3, 7] (inclusive) is set, within the  
iteration,
  * @range_begin will take the value 3 and @range_end will take the value 7.
  *
  * Ensure that there is at least one bit set before using this macro with
  * sparsebit_any_set(), because sparsebit_first_set() will abort if none are
  * set.
  */
#define sparsebit_for_each_set_range(s, range_begin, range_end)		\
	for (range_begin = sparsebit_first_set(s),			\
		     range_end =					\
		     sparsebit_next_clear(s, range_begin) - 1;		\
	     range_begin && range_end;					\
	     range_begin = sparsebit_next_set(s, range_end),		\
		     range_end =					\
		     sparsebit_next_clear(s, range_begin) - 1)
/*
  * sparsebit_next_clear() can return 0 if [x, 2**64-1] are all set, and the  
-1
  * would then cause an underflow back to 2**64 - 1. This is expected and
  * correct.
  *
  * If the last range in the sparsebit is [x, y] and we try to iterate,
  * sparsebit_next_set() will return 0, and sparsebit_next_clear() will try  
and
  * find the first range, but that's correct because the condition expression
  * would cause us to quit the loop.
  */


static void encrypt_region(struct kvm_vm *vm, struct userspace_mem_region  
*region)
{
	const struct sparsebit *protected_phy_pages =
		region->protected_phy_pages;
	const vm_paddr_t gpa_base = region->region.guest_phys_addr;
	const sparsebit_idx_t lowest_page_in_region = gpa_base >> vm->page_shift;

	sparsebit_idx_t i;
	sparsebit_idx_t j;

	if (!sparsebit_any_set(protected_phy_pages))
		return;

	sev_register_user_region(vm, region);

	sparsebit_for_each_set_range(protected_phy_pages, i, j) {
		const uint64_t size_to_load = (j - i + 1) * vm->page_size;
		const uint64_t offset = (i - lowest_page_in_region) * vm->page_size;
		const uint64_t gpa = gpa_base + offset;

		sev_launch_update_data(vm, gpa, size_to_load);
	}
}