Re: [kvm-unit-tests PATCH v5] s390x: Test effect of storage keys on some instructions

[Janosch Frank <frankja@xxxxxxxxxxxxx>]

On 4/26/22 09:53, Janosch Frank wrote:
> On 4/25/22 18:17, Janis Schoetterl-Glausch 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>
[...]

We need to:
a) Fence this in our CI environments
b) Fence this for the gitlab ci s390x kvm entry

Could you please add a patch that removes the skey test from 
.gitlab-ci.yml? It's at the very end of that file.

If the nit below is fixed:
Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx>


> Please add a comment that we're testing the fetch access of the operand
> since the prefix is only checked for addressing.
>
> > +static void test_set_prefix(void)
> > +{
> > +	uint32_t *prefix_ptr = (uint32_t *)pagebuf;
> > +	uint32_t *no_override_prefix_ptr;
> > +	uint32_t old_prefix;
> > +	pgd_t *root;
> > +
> > +	report_prefix_push("SET PREFIX");
> > +	root = (pgd_t *)(stctg(1) & PAGE_MASK);
> > +	old_prefix = get_prefix();
> > +	memcpy(lowcore_tmp, 0, sizeof(lowcore_tmp));
> > +	assert(((uint64_t)&lowcore_tmp >> 31) == 0);
> > +	*prefix_ptr = (uint32_t)(uint64_t)&lowcore_tmp;
> > +
> > +	report_prefix_push("zero key");
> > +	set_prefix(old_prefix);
> > +	set_storage_key(prefix_ptr, 0x20, 0);
> > +	set_prefix(*prefix_ptr);
> > +	report(get_prefix() == *prefix_ptr, "set prefix");
> > +	report_prefix_pop();
> > +
> > +	report_prefix_push("matching key");
> > +	set_prefix(old_prefix);
> > +	set_storage_key(pagebuf, 0x10, 0);
> > +	set_prefix_key_1(prefix_ptr);
> > +	report(get_prefix() == *prefix_ptr, "set prefix");
> > +	report_prefix_pop();
> > +
> > +	report_prefix_push("mismatching key");
> > +
> > +	report_prefix_push("no fetch protection");
> > +	set_prefix(old_prefix);
> > +	set_storage_key(pagebuf, 0x20, 0);
> > +	set_prefix_key_1(prefix_ptr);
> > +	report(get_prefix() == *prefix_ptr, "set prefix");
> > +	report_prefix_pop();
> > +
> > +	report_prefix_push("fetch protection");
> > +	set_prefix(old_prefix);
> > +	set_storage_key(pagebuf, 0x28, 0);
> > +	expect_pgm_int();
> > +	set_prefix_key_1(prefix_ptr);
> > +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> > +	report(get_prefix() == old_prefix, "did not set prefix");
> > +	report_prefix_pop();
> > +
> > +	register_pgm_cleanup_func(dat_fixup_pgm_int);
> > +
> > +	report_prefix_push("remapped page, fetch protection");
> > +	set_prefix(old_prefix);
> > +	set_storage_key(pagebuf, 0x28, 0);
> > +	expect_pgm_int();
> > +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> > +	set_prefix_key_1((uint32_t *)0);
> > +	install_page(root, 0, 0);
> > +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> > +	report(get_prefix() == old_prefix, "did not set prefix");
> > +	report_prefix_pop();
> > +
> > +	ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> > +
> > +	report_prefix_push("fetch protection override applies");
> > +	set_prefix(old_prefix);
> > +	set_storage_key(pagebuf, 0x28, 0);
> > +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> > +	set_prefix_key_1((uint32_t *)0);
> > +	install_page(root, 0, 0);
> > +	report(get_prefix() == *prefix_ptr, "set prefix");
> > +	report_prefix_pop();
> > +
> > +	no_override_prefix_ptr = (uint32_t *)(pagebuf + 2048);
> > +	WRITE_ONCE(*no_override_prefix_ptr, (uint32_t)(uint64_t)&lowcore_tmp);
> > +	report_prefix_push("fetch protection override does not apply");
> > +	set_prefix(old_prefix);
> > +	set_storage_key(pagebuf, 0x28, 0);
> > +	expect_pgm_int();
> > +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> > +	set_prefix_key_1((uint32_t *)2048);
> > +	install_page(root, 0, 0);
> > +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> > +	report(get_prefix() == old_prefix, "did not set prefix");
> > +	report_prefix_pop();
> > +
> > +	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> > +	register_pgm_cleanup_func(NULL);
> > +	report_prefix_pop();
> > +	set_storage_key(pagebuf, 0x00, 0);
> > +	report_prefix_pop();
> > +}
> > +
> >    int main(void)
> >    {
> >    	report_prefix_push("skey");
> > @@ -130,6 +352,11 @@ int main(void)
> >    	test_set();
> >    	test_set_mb();
> >    	test_chg();
> > +	test_test_protection();
> > +	test_store_cpu_address();
> > +
> > +	setup_vm();
> > +	test_set_prefix();
> >    done:
> >    	report_prefix_pop();
> >    	return report_summary();
> >
> > base-commit: 6a7a83ed106211fc0ee530a3a05f171f6a4c4e66