Re: [PATCH v2 2/2] KVM: s390: selftests: memop: add a simple AR test

[Eric Farman <farman@linux.ibm.com>]

On Tue, 2024-02-20 at 14:34 +0100, Nina Schoetterl-Glausch wrote:
> On Thu, 2024-02-15 at 21:53 +0100, Eric Farman wrote:
> > There is a selftest that checks for an (expected) error when an
> > invalid AR is specified, but not one that exercises the AR path.
> > 
> > Add a simple test that mirrors the vanilla write/read test while
> > providing an AR. An AR that contains zero will direct the CPU to
> > use the primary address space normally used anyway. AR[1] is
> > selected for this test because the host AR[1] is usually non-zero,
> > and KVM needs to correctly swap those values.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> > ---
> >  tools/testing/selftests/kvm/s390x/memop.c | 28
> > +++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/s390x/memop.c
> > b/tools/testing/selftests/kvm/s390x/memop.c
> > index bb3ca9a5d731..be20c26ee545 100644
> > --- a/tools/testing/selftests/kvm/s390x/memop.c
> > +++ b/tools/testing/selftests/kvm/s390x/memop.c
> > @@ -375,6 +375,29 @@ static void test_copy(void)
> >  	kvm_vm_free(t.kvm_vm);
> >  }
> > 
> > +static void test_copy_access_register(void)
> > +{
> > +	struct test_default t = test_default_init(guest_copy);
> > +
> > +	HOST_SYNC(t.vcpu, STAGE_INITED);
> > +
> > +	prepare_mem12();
> > +	t.run->psw_mask &= ~(3UL << (63 - 17));
> > +	t.run->psw_mask |= 1UL << (63 - 17);  /* Enable AR mode */
> > +
> 
> I feel like part of the commit message should be a comment here
> 
> /*
>  * Guest AR[1] should be zero, in which case the primary address
> space is used.
>  * The host makes use of AR[1], its value must not be used for the
> memop.
>  */
> > +	CHECK_N_DO(MOP, t.vcpu, LOGICAL, WRITE, mem1, t.size,
> > +		   GADDR_V(mem1), AR(1));
> > +	HOST_SYNC(t.vcpu, STAGE_COPIED);
> > +
> > +	CHECK_N_DO(MOP, t.vcpu, LOGICAL, READ, mem2, t.size,
> > +		   GADDR_V(mem2), AR(1));
> > +	ASSERT_MEM_EQ(mem1, mem2, t.size);
> > +
> > +	t.run->psw_mask &= ~(3UL << (63 - 17));   /* Disable AR
> > mode */
> 
> Any reason for this? It's not necessary since the vm is going to be
> destroyed.

Nah. I think I was just putting it back the way I was, in case I did
slap another test on there, but never did.

> > +
> > +	kvm_vm_free(t.kvm_vm);
> > +}
> > +
> >  static void set_storage_key_range(void *addr, size_t len, uint8_t
> > key)
> >  {
> >  	uintptr_t _addr, abs, i;
> > @@ -1101,6 +1124,11 @@ int main(int argc, char *argv[])
> >  			.test = test_copy_key_fetch_prot_override,
> >  			.requirements_met = extension_cap > 0,
> >  		},
> > +		{
> > +			.name = "copy with access register mode",
> > +			.test = test_copy_access_register,
> > +			.requirements_met = true,
> > +		},
> >  		{
> >  			.name = "error checks with key",
> >  			.test = test_errors_key,
>