Re: [kvm-unit-tests PATCH v3 5/7] s390x: Add library functions for exiting from snippet

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Tue, 2024-06-25 at 12:43 +1000, Nicholas Piggin wrote:
> On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> > It is useful to be able to force an exit to the host from the snippet,
> > as well as do so while returning a value.
> > Add this functionality, also add helper functions for the host to check
> > for an exit and get or check the value.
> > Use diag 0x44 and 0x9c for this.
> > Add a guest specific snippet header file and rename snippet.h to reflect
> > that it is host specific.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> >  s390x/Makefile                          |  1 +
> >  lib/s390x/asm/arch_def.h                | 13 ++++++++
> >  lib/s390x/snippet-guest.h               | 26 +++++++++++++++
> >  lib/s390x/{snippet.h => snippet-host.h} | 10 ++++--
> >  lib/s390x/snippet-host.c                | 42 +++++++++++++++++++++++++
> >  lib/s390x/uv.c                          |  2 +-
> >  s390x/mvpg-sie.c                        |  2 +-
> >  s390x/pv-diags.c                        |  2 +-
> >  s390x/pv-icptcode.c                     |  2 +-
> >  s390x/pv-ipl.c                          |  2 +-
> >  s390x/sie-dat.c                         |  2 +-
> >  s390x/spec_ex-sie.c                     |  2 +-
> >  s390x/uv-host.c                         |  2 +-
> >  13 files changed, 97 insertions(+), 11 deletions(-)
> >  create mode 100644 lib/s390x/snippet-guest.h
> >  rename lib/s390x/{snippet.h => snippet-host.h} (92%)
> >  create mode 100644 lib/s390x/snippet-host.c
> > 
[...]

> > diff --git a/lib/s390x/snippet-guest.h b/lib/s390x/snippet-guest.h
> > new file mode 100644
> > index 00000000..3cc098e1
> > --- /dev/null
> > +++ b/lib/s390x/snippet-guest.h
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Snippet functionality for the guest.
> > + *
> > + * Copyright IBM Corp. 2023
> > + */
> > +
> > +#ifndef _S390X_SNIPPET_GUEST_H_
> > +#define _S390X_SNIPPET_GUEST_H_
> > +
> > +#include <asm/arch_def.h>
> > +#include <asm/barrier.h>
> > +
> > +static inline void force_exit(void)
> > +{
> > +	diag44();
> > +	mb(); /* allow host to modify guest memory */
> > +}
> > +
> > +static inline void force_exit_value(uint64_t val)
> > +{
> > +	diag9c(val);
> > +	mb(); /* allow host to modify guest memory */
> > +}
> 
> You have barriers here, but couldn't the diag get moved before a prior
> store by the guest?

Yeah, makes sense to add another before.
> 
> Silly question since I don't understand the s390x arch or snippet design
> too well... the diag here causes a guest exit to the host. After the
> host handles that, it may resume guest at the next instruction? If that
> is correct, then the barrier here (I think) is for when the guest
> resumes it would not reorder subsequent loads from guest memory before
> the diag, because the host might have modified it.

[...]

> > diff --git a/lib/s390x/snippet-host.c b/lib/s390x/snippet-host.c
> > new file mode 100644
> > index 00000000..44a60bb9
> > --- /dev/null
> > +++ b/lib/s390x/snippet-host.c

[...]

> > +void snippet_check_force_exit_value(struct vm *vm, uint64_t value_exp)
> > +{
> > +	uint64_t value;
> > +
> > +	if (snippet_is_force_exit_value(vm)) {
> > +		value = snippet_get_force_exit_value(vm);
> > +		report(value == value_exp, "guest forced exit with value (0x%lx == 0x%lx)",
> > +		       value, value_exp);
> 
> This is like kvm selftests guest/host synch design, which is quite
> nice and useful.
> 
> > +	} else {
> > +		report_fail("guest forced exit with value");
> > +	}
> 
> Guest forced exit without value?

It's this way round so the output reads:

FAIL: guest forced exit with value

What's after the colon is what failed and the message
is the same for PASS/FAIL. Indeed a bit confusing.

> And do you also need to check for non-value force
> exit to distinguish from a normal snippet exit?

No, the function does just this check and if you need to handle
more complicated situations you need to do that in the caller.

> 
> Thanks,
> Nick