On 4/22/22 13:56, Claudio Imbrenda wrote: > On Thu, 21 Apr 2022 11:04:21 +0200 > Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> wrote: > >> Some instructions are emulated by KVM. Test that KVM correctly emulates >> storage key checking for two of those instructions (STORE CPU ADDRESS, >> SET PREFIX). >> Test success and error conditions, including coverage of storage and >> fetch protection override. >> Also add test for TEST PROTECTION, even if that instruction will not be >> emulated by KVM under normal conditions. >> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> >> --- >> v2 -> v3: >> * fix asm for SET PREFIX zero key test: make input >> * implement Thomas' suggestions: >> https://lore.kernel.org/kvm/f050da01-4d50-5da5-7f08-6da30f5dbbbe@xxxxxxxxxx/ >> >> v1 -> v2: >> * use install_page instead of manual page table entry manipulation >> * check that no store occurred if none is expected >> * try to check that no fetch occured if not expected, although in >> practice a fetch would probably cause the test to crash >> * reset storage key to 0 after test >> [...] >> +static void test_set_prefix(void) >> +{ >> + uint32_t *out = (uint32_t *)pagebuf; >> + pgd_t *root; >> + >> + report_prefix_push("SET PREFIX"); >> + root = (pgd_t *)(stctg(1) & PAGE_MASK); >> + >> + asm volatile("stpx %0" : "=Q"(*out)); >> + >> + report_prefix_push("zero key"); >> + set_storage_key(pagebuf, 0x20, 0); >> + asm volatile("spx %0" :: "Q" (*out)); > > so you are changing the prefix to... the old prefix (so nothing > changes). how do you know that something happened at all? Basically, I'm only checking that no exception occurs, which would crash the test. > > (see my longer comment below) > >> + report_pass("no exception"); >> + report_prefix_pop(); >> + >> + report_prefix_push("matching key"); >> + set_storage_key(pagebuf, 0x10, 0); >> + set_prefix_key_1(out); >> + report_pass("no exception"); >> + report_prefix_pop(); >> + >> + report_prefix_push("mismatching key, no fetch protection"); >> + set_storage_key(pagebuf, 0x20, 0); >> + set_prefix_key_1(out); >> + report_pass("no exception"); >> + report_prefix_pop(); >> + >> + report_prefix_push("mismatching key, fetch protection"); >> + set_storage_key(pagebuf, 0x28, 0); >> + expect_pgm_int(); >> + *out = 0xdeadbeef; > > so here you are trying to set 0xdeadbeef as prefix, right? In the sense that I'm executing SPX with that as operand, yes, but the hypervisor is not supposed to ever read that value, so the content should not matter. > which would fail for other reasons I guess, since that would be outside > memory (unless otherwise specified, kvm unit tests run with 128M of ram) > >> + set_prefix_key_1(out); >> + check_pgm_int_code(PGM_INT_CODE_PROTECTION); This ^ is the important check, the two lines below indeed questionable, I considered just dropping them. >> + asm volatile("stpx %0" : "=Q"(*out)); >> + report(*out != 0xdeadbeef, "no fetch occurred"); > > and here you check that the prefix has not changed to that "wrong > value", which would be impossible anyway because it would be outside of > memory. moreover the address you give is not even in the lower 2G, so > in case the address has been changed, it would not match your magic > value anyway. > > for this (and the following) tests, I propose the following: > > add a new > char tmplowcore[2 * PAGE_SIZE] __attribute((aligned(2*PAGE_SIZE))); > > initialize it properly (memcpy the actual lowcore into it), and use > that address for SPX. this way if SPX does not fail, at least you would > have a valid lowcore. and at that point you can check against that > address instead of your magic I'll try it out, maybe it will make tcg not crash and just fail the test. [...]
