On Mon, Dec 05, 2022, Vishal Annapurve wrote:

+void vcpu_run_and_handle_mapgpa(struct kvm_vm *vm, struct kvm_vcpu *vcpu) +{

• /*

• * Loop until the guest exits with any reason other than
  

• * KVM_HC_MAP_GPA_RANGE hypercall.
  

• */
  

• while (true) {

• vcpu_run(vcpu);
  

• if ((vcpu->run->exit_reason == KVM_EXIT_HYPERCALL) &&
  

• 	(vcpu->run->hypercall.nr == KVM_HC_MAP_GPA_RANGE)) {
  

I get what you're trying to do, and I completely agree that we need better helpers and/or macros to reduce this type of boilerplate, but adding a one-off helper like this is going to be a net negative overall. This helper services exactly one use case, and also obfuscates what a test does.

In other words, this is yet another thing that needs broad, generic support (_vcpu_run() is a very special case). E.g. something like this to make it easy for tests to run a guest and handle ucalls plus specific exits (just a strawman, I think we can do better for handling ucalls).

#define vcpu_run_loop(vcpu, handlers, ucalls) \ do { \ uint32_t __exit; \ int __r = 0; \ \ while (!r) { \ vcpu_run(vcpu); \ \ __exit = vcpu->run->exit_reason; \ \ if (__exit < ARRAY_SIZE(handlers) && handlers[__exit]) \ __r = handlers[__exit](vcpu); \ else if (__exit == KVM_EXIT_IO && ucalls) \ __r = handle_exit_ucall(vcpu, ucalls, \ ARRAY_SIZE(ucalls)); \ else \ TEST_FAIL(...) \ } \ } while (0)

For this series, I think it makes sense to just open code yet another test. It really doesn't end up being much code, which is partly why we haven't added helpers :-/

On Mon, Dec 05, 2022, Vishal Annapurve wrote:

+#define TEST_AREA_SLOT 10

I vote using "DATA" instead of "TEST_AREA" just because it's shorter.

+#define TEST_AREA_GPA 0xC0000000

Put the GPA above 4GiB, that way it'll never run afoul of the PCI hole, e.g. won't overlap with the local APIC.

+#define TEST_AREA_SIZE (2 * 1024 * 1024)

Use the sizes from <linux/sizes.h>, e.g. SZ_2M.

+#define GUEST_TEST_MEM_OFFSET (1 * 1024 * 1024)

Same here.

+#define GUEST_TEST_MEM_SIZE (10 * 4096)

Why 10*4KiB? SZ_2M + PAGE_SIZE seems like the best compromise until selftests doesn't explode on mapping huge memslots in the guest. That lets us test boundary conditions wiith hugepages.

+#define VM_STAGE_PROCESSED(x) pr_info("Processed stage %s\n", #x)

Comments on this in the context of host_conv_test_fn().

+#define TEST_MEM_DATA_PATTERN1 0x66

Regarding the values of the patterns, the patterns themselves are also not at all interesting. The goal of the test is to verify that KVM/kernel plumbs through the correct memory. Verifying that hardware doesn't barf on a pattern is very much a non-goal, i.e. we aren't writing a test for DRAM.

While debugging my own goofs, I found it much, much easier to triage failures by simply using the pattern number of the value, e.g. 1 = 0x11, 2 = 0x22, etc.

+#define TEST_MEM_DATA_PATTERN2 0x99 +#define TEST_MEM_DATA_PATTERN3 0x33 +#define TEST_MEM_DATA_PATTERN4 0xaa +#define TEST_MEM_DATA_PATTERN5 0x12

While I like using macros instead of copy+pasting magic numbers, in this case the macros make the code really, really hard to read. Longish names with just one character different all look the same when glancing through the code, e.g. identifying when the test switches between patterns is difficult.

And IMO, having the guest explicitly tell the host what to expect yields a more maintanable, easier to understand test overall. And by having the guest say what pattern(s) to expect/write, then there's no needed for the guest to have a priori knowledge of the patterns, and thus no need for macros.

+static bool verify_mem_contents(void *mem, uint32_t size, uint8_t pattern) +{

• uint8_t *buf = (uint8_t *)mem;
• for (uint32_t i = 0; i < size; i++) {

• if (buf[i] != pattern)
  

• 	return false;
  

It took me blundering into a SHUTDOWN before I figured out these return booleans instead of simply asserting: using TEST_ASSERT() in the guest obviously doesn't fair well.

While I like deduplicating code, in this case it does more harm than good because the context of exactly what fails is lost, e.g. expected vs. actual pattern, offset, etc... We could use macros to dedup code, i.e. have only the assertion be different, but I don't think that ends up being a net positive either.

• }
• return true;

+}

+static void populate_test_area(void *test_area_base, uint64_t pattern) +{

• memset(test_area_base, pattern, TEST_AREA_SIZE);

+}

+static void populate_guest_test_mem(void *guest_test_mem, uint64_t pattern) +{

• memset(guest_test_mem, pattern, GUEST_TEST_MEM_SIZE);

Generally speaking, don't add one-line wrappers that don't provide novel functionality. E.g. there's nothing fancy here, and so the wrappers just end up obfuscating the size of a memset().

+}

+static bool verify_test_area(void *test_area_base, uint64_t area_pattern,

• uint64_t guest_pattern)

+{

• void *guest_test_mem = test_area_base + GUEST_TEST_MEM_OFFSET;
• void *test_area2_base = guest_test_mem + GUEST_TEST_MEM_SIZE;
• uint64_t test_area2_size = (TEST_AREA_SIZE - (GUEST_TEST_MEM_OFFSET +

• 	GUEST_TEST_MEM_SIZE));
  

• return (verify_mem_contents(test_area_base, GUEST_TEST_MEM_OFFSET, area_pattern) &&

• verify_mem_contents(guest_test_mem, GUEST_TEST_MEM_SIZE, guest_pattern) &&
  

• verify_mem_contents(test_area2_base, test_area2_size, area_pattern));
  

+}

+#define GUEST_STARTED 0 +#define GUEST_PRIVATE_MEM_POPULATED 1 +#define GUEST_SHARED_MEM_POPULATED 2 +#define GUEST_PRIVATE_MEM_POPULATED2 3

+/*

• • Run memory conversion tests with explicit conversion:
• • Execute KVM hypercall to map/unmap gpa range which will cause userspace exit
• • to back/unback private memory. Subsequent accesses by guest to the gpa range
• • will not cause exit to userspace.
• • Test memory conversion scenarios with following steps:
• • 1. Access private memory using private access and verify that memory contents
• • are not visible to userspace.
• • 2. Convert memory to shared using explicit conversions and ensure that
• • userspace is able to access the shared regions.
• • 3. Convert memory back to private using explicit conversions and ensure that
• • userspace is again not able to access converted private regions.
• */

+static void guest_conv_test_fn(void) +{

• void *test_area_base = (void *)TEST_AREA_GPA;
• void *guest_test_mem = (void *)(TEST_AREA_GPA + GUEST_TEST_MEM_OFFSET);
• uint64_t guest_test_size = GUEST_TEST_MEM_SIZE;
• GUEST_SYNC(GUEST_STARTED);
• populate_test_area(test_area_base, TEST_MEM_DATA_PATTERN1);

Tangentially related to my "the patterns themselves are uninteresting" comment, and very related to the "avoid global defines for the patterns", I would like to structure this test so that it's easy to test GPA+size combinations. E.g. to test that KVM does the right thing when a conversion spans regions that KVM is likely to map with hugepages, or is misaligned with respect to hugepages, etc.

If the guest explicit tells the host which patterns to expect/write, then testing combinations of addresses is just a matter of looping in the guest, e.g.

struct { uint64_t offset; uint64_t size; } stages[] = { GUEST_STAGE(0, PAGE_SIZE), GUEST_STAGE(0, SZ_2M), GUEST_STAGE(PAGE_SIZE, PAGE_SIZE), GUEST_STAGE(PAGE_SIZE, SZ_2M), GUEST_STAGE(SZ_2M, PAGE_SIZE), }; const uint8_t init_p = 0xcc; uint64_t j; int i;

/* Memory should be shared by default. */ memset((void *)DATA_GPA, ~init_p, DATA_SIZE); GUEST_SYNC4(DATA_GPA, DATA_SIZE, ~init_p, init_p); memcmp_g(DATA_GPA, init_p, DATA_SIZE);

for (i = 0; i < ARRAY_SIZE(stages); i++) { blah blah blah }

• GUEST_SYNC(GUEST_PRIVATE_MEM_POPULATED);

As above, use the sync to effectively tell/request the host to do something, as opposed to having the host infer what it's supposed to do based on the current stage.

Aside from wanting to be able to iterate on GPA+size, I really, really dislike the GUEST_SYNC(stage) pattern. It works ok for small tests, but the pattern doesn't scale, e.g. see xen_shinfo_test.c. Even at smaller scales, the resulting magic numbers can be unnecessarily difficult to understand, e.g. smm_test.c isn't doing anything _that_ complex, but every time I look at the test I spend several minutes relearning what it does. Using macros instead of magic numbers helps a little, but it doesn't fix the underlying issue of bundling a bunch of testcases into a single monolithic sequences.

• GUEST_ASSERT(verify_test_area(test_area_base, TEST_MEM_DATA_PATTERN1,

• TEST_MEM_DATA_PATTERN1));
  

Align params to help delineate the boundaries between the assert and the function call. E.g. if we ended up with this code:

GUEST_ASSERT(verify_test_area(test_area_base, TEST_MEM_DATA_PATTERN1, TEST_MEM_DATA_PATTERN1));

But as (much further) above, just assert in the comparison helper to avoid the problem entirely.

• kvm_hypercall_map_shared((uint64_t)guest_test_mem, guest_test_size);
• populate_guest_test_mem(guest_test_mem, TEST_MEM_DATA_PATTERN2);
• GUEST_SYNC(GUEST_SHARED_MEM_POPULATED);
• GUEST_ASSERT(verify_test_area(test_area_base, TEST_MEM_DATA_PATTERN1,

• TEST_MEM_DATA_PATTERN5));
  

• kvm_hypercall_map_private((uint64_t)guest_test_mem, guest_test_size);
• populate_guest_test_mem(guest_test_mem, TEST_MEM_DATA_PATTERN3);
• GUEST_SYNC(GUEST_PRIVATE_MEM_POPULATED2);
• GUEST_ASSERT(verify_test_area(test_area_base, TEST_MEM_DATA_PATTERN1,

• TEST_MEM_DATA_PATTERN3));
  

• GUEST_DONE();

+}

+#define ASSERT_CONV_TEST_EXIT_IO(vcpu, stage) \

• { \

• struct ucall uc; \
  

• ASSERT_EQ(vcpu->run->exit_reason, KVM_EXIT_IO); \
  

• ASSERT_EQ(get_ucall(vcpu, &uc), UCALL_SYNC); \
  

• ASSERT_EQ(uc.args[1], stage); \
  

• }

+#define ASSERT_GUEST_DONE(vcpu) \

• { \

• struct ucall uc; \
  

• ASSERT_EQ(vcpu->run->exit_reason, KVM_EXIT_IO); \
  

• ASSERT_EQ(get_ucall(vcpu, &uc), UCALL_DONE); \
  

• }

+static void host_conv_test_fn(struct kvm_vm *vm, struct kvm_vcpu *vcpu)

For future reference, "fn" is completely redundant. It's a function, no need for the label. When a function pointer is a parameter, and isn't obviously such, then "fn" can be helpful, but here it's just noise.

+{

• void *test_area_hva = addr_gpa2hva(vm, TEST_AREA_GPA);
• void *guest_test_mem_hva = (test_area_hva + GUEST_TEST_MEM_OFFSET);
• vcpu_run_and_handle_mapgpa(vm, vcpu);
• ASSERT_CONV_TEST_EXIT_IO(vcpu, GUEST_STARTED);
• populate_test_area(test_area_hva, TEST_MEM_DATA_PATTERN4);
• VM_STAGE_PROCESSED(GUEST_STARTED);

Again for future reference since I think it's better to avoid named stages, if you add macros to wrap trivial code in order to do something clever (pretty print the name of the stage), use a name that more precisely captures the triviality of the code. VM_STAGE_PROCESSED() sounds like the macro is doing bookkeeping, e.g. advancing a stage/counter or something, whereas something like print_stage() makes it fairly clear that the helper/macro is doing nothing more than printing, i.e. saves the reader from having to go look at the implementation to understand the code.

Regarding the printing itself, I suspect one of the reasons why you added the pretty printing of stages was to help debug. While there is a time and place for printf debugging, when it comes to KVM tests, the "need" to resort to printing out every step is usually the symptom of unhelpful assertions and error messages.

E.g. if pattern "encodes" its number (p1 = 0x11), capturing the line number, expected versus actual pattern, and the GPA provides pretty much all the debug info needed to figure out what failed.

Seeing the test actually make progress can be useful, e.g. as a heartbeart for long-running tests, but again outside of development/debug it's mostly noise. E.g. if a test fails in CI, earlier messages may or may not be captured depending on the whims of the CI instance/robot, and so we want as much information as possible in the error message itself.

• vcpu_run_and_handle_mapgpa(vm, vcpu);
• ASSERT_CONV_TEST_EXIT_IO(vcpu, GUEST_PRIVATE_MEM_POPULATED);
• TEST_ASSERT(verify_test_area(test_area_hva, TEST_MEM_DATA_PATTERN4,

• 	TEST_MEM_DATA_PATTERN4), "failed");
  

• VM_STAGE_PROCESSED(GUEST_PRIVATE_MEM_POPULATED);
• vcpu_run_and_handle_mapgpa(vm, vcpu);
• ASSERT_CONV_TEST_EXIT_IO(vcpu, GUEST_SHARED_MEM_POPULATED);
• TEST_ASSERT(verify_test_area(test_area_hva, TEST_MEM_DATA_PATTERN4,

• 	TEST_MEM_DATA_PATTERN2), "failed");
  

• populate_guest_test_mem(guest_test_mem_hva, TEST_MEM_DATA_PATTERN5);
• VM_STAGE_PROCESSED(GUEST_SHARED_MEM_POPULATED);
• vcpu_run_and_handle_mapgpa(vm, vcpu);
• ASSERT_CONV_TEST_EXIT_IO(vcpu, GUEST_PRIVATE_MEM_POPULATED2);
• TEST_ASSERT(verify_test_area(test_area_hva, TEST_MEM_DATA_PATTERN4,

• 	TEST_MEM_DATA_PATTERN5), "failed");
  

• VM_STAGE_PROCESSED(GUEST_PRIVATE_MEM_POPULATED2);
• vcpu_run_and_handle_mapgpa(vm, vcpu);
• ASSERT_GUEST_DONE(vcpu);

+}

+static void execute_vm_with_private_test_mem(

• 	enum vm_mem_backing_src_type test_mem_src)
  

+{

• struct kvm_vm *vm;
• struct kvm_enable_cap cap;
• struct kvm_vcpu *vcpu;
• vm = vm_create_with_one_vcpu(&vcpu, guest_conv_test_fn);
• vm_check_cap(vm, KVM_CAP_EXIT_HYPERCALL);

TEST_REQUIRE() so that the test is skipped, not failed.

• cap.cap = KVM_CAP_EXIT_HYPERCALL;
• cap.flags = 0;
• cap.args[0] = (1 << KVM_HC_MAP_GPA_RANGE);
• vm_ioctl(vm, KVM_ENABLE_CAP, &cap);

vm_enable_cap() will do most of this for you.

• vm_userspace_mem_region_add(vm, test_mem_src, TEST_AREA_GPA,

• TEST_AREA_SLOT, TEST_AREA_SIZE / vm->page_size, KVM_MEM_PRIVATE);
  

Align params.

• vm_allocate_private_mem(vm, TEST_AREA_GPA, TEST_AREA_SIZE);
• virt_map(vm, TEST_AREA_GPA, TEST_AREA_GPA, TEST_AREA_SIZE/vm->page_size);
• host_conv_test_fn(vm, vcpu);
• kvm_vm_free(vm);

+}

+int main(int argc, char *argv[]) +{

• execute_vm_with_private_test_mem(

• 		VM_MEM_SRC_ANONYMOUS_AND_RESTRICTED_MEMFD);
  

• /* Needs 2MB Hugepages */
• if (get_free_huge_2mb_pages() >= 1) {

• printf("Running private mem test with 2M pages\n");
  

• execute_vm_with_private_test_mem(
  

• 		VM_MEM_SRC_ANON_HTLB2M_AND_RESTRICTED_MEMFD);
  

• } else

• printf("Skipping private mem test with 2M pages\n");
  

• return 0;

+}

2.39.0.rc0.267.gcb52ba06e7-goog

On Tue, Jan 17, 2023 at 5:09 PM Sean Christopherson seanjc@google.com wrote:

On Mon, Dec 05, 2022, Vishal Annapurve wrote:

...

This series implements selftests targeting the feature floated by Chao via: https://lore.kernel.org/lkml/20221202061347.1070246-10-chao.p.peng@linux.int...

Below changes aim to test the fd based approach for guest private memory in context of normal (non-confidential) VMs executing on non-confidential platforms.

private_mem_test.c file adds selftest to access private memory from the guest via private/shared accesses and checking if the contents can be leaked to/accessed by vmm via shared memory view before/after conversions.

Updates in V2:

1. Simplified vcpu run loop implementation API
2. Removed VM creation logic from private mem library

I pushed a rework version of this series to:

git@github.com:sean-jc/linux.git x86/upm_base_support

Thanks for the review and spending time to rework this series. The revised version [1] looks cleaner and lighter.

Can you take a look and make sure that I didn't completely botch anything, and preserved the spirit of what you are testing?

Yeah, the reworked selftest structure [1] looks good to me in general. Few test cases that are missing in the reworked version: * Checking if contents of GPA ranges corresponding to private memory are not leaked to host userspace when accessing guest memory using HVA ranges * Checking if private to shared conversion of memory affects nearby private pages.

Going forward, no need to send a v3 at this time. Whoever sends v11 of the series will be responsible for including tests.

Sounds good to me.

No need to respond to comments either, unless of course there's something you object to, want to clarify, etc., in which case definitely pipe up.

Beyond the SEV series, do you have additional UPM testcases written? If so, can you post them, even if they're in a less-than-perfect state? If they're in a "too embarassing to post" state, feel from to send them off list :-)

Ackerley (ackerleytng@google.com) is working on publishing the rfc v3 version of TDX selftests that include UPM specific selftests. He plans to publish them this week.

Last question, do you have a list of testcases that you consider "required" for UPM? My off-the-cuff list of selftests I want to have before merging UPM is pretty short at this point:

• Negative testing of the memslot changes, e.g. bad alignment, bad fd, illegal memslot updates, etc.
• Negative testing of restrictedmem, e.g. various combinations of overlapping bindings of a single restrictedmem instance.
• Access vs. conversion stress, e.g. accessing a region in the guest while it's concurrently converted by the host, maybe with fancy guest code to try and detect TLB or ordering bugs?

List of testcases that I was tracking (covered by the current selftests) as required: 1) Ensure private memory contents are not accessible to host userspace using the HVA 2) Ensure shared memory contents are visible/accessible from both host userspace and the guest 3) Ensure 1 and 2 holds across explicit memory conversions 4) Exercise memory conversions with mixed shared/private memory pages in a huge page to catch issues like [2] 5) Ensure that explicit memory conversions don't affect nearby GPA ranges

Test Cases that will be covered by TDX/SNP selftests (in addition to above scenarios): 6) Ensure 1 and 2 holds across implicit memory conversions 7) Ensure that implicit memory conversions don't affect nearby GPA ranges

Additional testcases possible: 8) Running conversion tests for non-overlapping GPA ranges of same/different memslots from multiple vcpus

[1] - https://github.com/sean-jc/linux/commit/7e536bf3c45c623425bc84e8a96634efc3a6... [2] - https://lore.kernel.org/linux-mm/CAGtprH82H_fjtRbL0KUxOkgOk4pgbaEbAydDYfZ0qx...

On Fri, Feb 10, 2023 at 11:59:23AM -0800, Vishal Annapurve wrote:

On Tue, Jan 17, 2023 at 7:11 PM Vishal Annapurve vannapurve@google.com wrote:

...

...

...

...

Last question, do you have a list of testcases that you consider "required" for UPM? My off-the-cuff list of selftests I want to have before merging UPM is pretty short at this point:

• Negative testing of the memslot changes, e.g. bad alignment, bad fd, illegal memslot updates, etc.
• Negative testing of restrictedmem, e.g. various combinations of overlapping bindings of a single restrictedmem instance.
• Access vs. conversion stress, e.g. accessing a region in the guest while it's concurrently converted by the host, maybe with fancy guest code to try and detect TLB or ordering bugs?

List of testcases that I was tracking (covered by the current selftests) as required:

1. Ensure private memory contents are not accessible to host userspace

using the HVA 2) Ensure shared memory contents are visible/accessible from both host userspace and the guest 3) Ensure 1 and 2 holds across explicit memory conversions 4) Exercise memory conversions with mixed shared/private memory pages in a huge page to catch issues like [2] 5) Ensure that explicit memory conversions don't affect nearby GPA ranges

Test Cases that will be covered by TDX/SNP selftests (in addition to above scenarios): 6) Ensure 1 and 2 holds across implicit memory conversions 7) Ensure that implicit memory conversions don't affect nearby GPA ranges

Additional testcases possible: 8) Running conversion tests for non-overlapping GPA ranges of same/different memslots from multiple vcpus

[1] - https://github.com/sean-jc/linux/commit/7e536bf3c45c623425bc84e8a96634efc3a6... [2] - https://lore.kernel.org/linux-mm/CAGtprH82H_fjtRbL0KUxOkgOk4pgbaEbAydDYfZ0qx...

List of additional testcases that could help increase basic coverage (including what sean mentioned earlier):

1. restrictedmem functionality testing
   • read/write/mmap should not work
   • fstat/fallocate should work as expected
2. restrictedmem registration/modification testing with:
   • bad alignment, bad fd, modifying properties of existing memslot
   • Installing multiple memslots with ranges within the same

restricted mem files - deleting memslots with restricted memfd while guests are being executed

In case you havn't started, I will work on 1) and 2) for the following days. As a start, I will first add restrictedmem tests (without KVM) then move to new memslots related tests.

Chao

3. Runtime restricted mem testing:
   • Access vs conversion testing from multiple vcpus
   • conversion and access to non-overlapping ranges from multiple vcpus

Regards, Vishal

On Mon, Mar 06, 2023 at 06:21:24PM +0000, Ackerley Tng wrote:

Chao Peng chao.p.peng@linux.intel.com writes:

...

On Fri, Feb 10, 2023 at 11:59:23AM -0800, Vishal Annapurve wrote:

...

On Tue, Jan 17, 2023 at 7:11 PM Vishal Annapurve vannapurve@google.com wrote:

...

...

...

...

...

...

Last question, do you have a list of testcases that you consider

"required" for

...

...

UPM? My off-the-cuff list of selftests I want to have before

merging UPM is pretty

...

...

short at this point:

• Negative testing of the memslot changes, e.g. bad alignment,

bad fd,

...

...

illegal memslot updates, etc.

• Negative testing of restrictedmem, e.g. various combinations

of overlapping

...

...

bindings of a single restrictedmem instance.

• Access vs. conversion stress, e.g. accessing a region in the

guest while it's

...

...

concurrently converted by the host, maybe with fancy guest

code to try and

...

...

detect TLB or ordering bugs?

List of testcases that I was tracking (covered by the current selftests) as required:

1. Ensure private memory contents are not accessible to host userspace

using the HVA 2) Ensure shared memory contents are visible/accessible from both host userspace and the guest 3) Ensure 1 and 2 holds across explicit memory conversions 4) Exercise memory conversions with mixed shared/private memory pages in a huge page to catch issues like [2] 5) Ensure that explicit memory conversions don't affect nearby GPA

ranges

...

Test Cases that will be covered by TDX/SNP selftests (in addition to above scenarios): 6) Ensure 1 and 2 holds across implicit memory conversions 7) Ensure that implicit memory conversions don't affect nearby GPA

ranges

...

Additional testcases possible: 8) Running conversion tests for non-overlapping GPA ranges of same/different memslots from multiple vcpus

[1] - https://github.com/sean-jc/linux/commit/7e536bf3c45c623425bc84e8a96634efc3a6... [2] - https://lore.kernel.org/linux-mm/CAGtprH82H_fjtRbL0KUxOkgOk4pgbaEbAydDYfZ0qx...

...

...

List of additional testcases that could help increase basic coverage (including what sean mentioned earlier):

1. restrictedmem functionality testing
   • read/write/mmap should not work
   • fstat/fallocate should work as expected
2. restrictedmem registration/modification testing with:
   • bad alignment, bad fd, modifying properties of existing memslot
   • Installing multiple memslots with ranges within the same

restricted mem files - deleting memslots with restricted memfd while guests are being executed

...

In case you havn't started, I will work on 1) and 2) for the following days. As a start, I will first add restrictedmem tests (without KVM) then move to new memslots related tests.

...

Chao

...

...

3. Runtime restricted mem testing:
   • Access vs conversion testing from multiple vcpus
   • conversion and access to non-overlapping ranges from multiple vcpus

...

...

Regards, Vishal

Chao, I'll work on

• Running conversion tests for non-overlapping GPA ranges of same/different memslots from multiple vcpus
• Deleting memslots with restricted memfd while guests are being executed
• Installing multiple memslots with ranges within the same restricted mem files

this week.

Thanks Ackerley. Looks good to me.

BTW, for whom may have interest, below are the testcases I added: https://github.com/chao-p/linux/commit/24dd1257d5c93acb8c8cc6c76c51cf6869970... https://github.com/chao-p/linux/commit/39a872ef09d539ce0c953451152eb05276b87... https://github.com/chao-p/linux/commit/ddd2c92b268a2fdc6158f82a6169ad1a57f2a...

Chao

On Wed, Mar 08, 2023, Ackerley Tng wrote:

While I was working on the selftests I noticed that this could perhaps be improved:

https://github.com/chao-p/linux/blob/ddd2c92b268a2fdc6158f82a6169ad1a57f2a01...

We should use a temporary variable to hold the result of fget(fd).

As it is now, if the user provides any invalide fd, like -1, slot->restrictedmem.file would be overwritten and lost.

Meh, that can happen if and only if KVM has a bug elsehwere. If slot->restrictedmem.file is anything but NULL, KVM is hosed. E.g. waiting to set slot->restrictedmem.file until the very end wouldn't magically prevent a file descriptor leak if slot->restrictedmem.file is non-NULL.

We cannot update slot->restrictedmem.file until after the file_is_restrictedmem() check.

For now there isn't a big problem because kvm_restrictedmem_bind() is only called on a new struct kvm_memory_slot, but I think this should be changed in case the function is used elsewhere in future.

Nah, if anything we could add

if (WARN_ON_ONCE(slot->restrictedmem.file)) return -EIO;

but I don't see the point. There's exactly one caller and the entire scheme depends on binding the memslot to restricted memory when the memslot is created, i.e. this would be but one of many changes if KVM were to allowed re-binding a memslot.