Re: [XTF PATCH v2 2/2] x86: Allow exiting QEMU in TCG/QEMU

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Fri Oct 3, 2025 at 10:06 AM CEST, Roger Pau Monné wrote:
> On Thu, Oct 02, 2025 at 07:48:28PM +0200, Alejandro Vallejo wrote:
>> On Thu Oct 2, 2025 at 5:37 PM CEST, Roger Pau Monné wrote:
>> > On Thu, Oct 02, 2025 at 04:48:38PM +0200, Alejandro Vallejo wrote:
>> >> On Thu Oct 2, 2025 at 4:22 PM CEST, Roger Pau Monné wrote:
>> >> > On Thu, Oct 02, 2025 at 03:55:34PM +0200, Alejandro Vallejo wrote:
>> >> >> If QEMU has a debug isa-debug-exit device, we can simply write to it
>> >> >> to exit rather than spinning after a failed hypercall.
>> >> >> 
>> >> >> While at it, reorder an out-of-order include.
>> >> >> 
>> >> >> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
>> >> >> ---
>> >> >>  arch/x86/hvm/traps.c    | 16 +++++++++++++++-
>> >> >>  arch/x86/pv/traps.c     |  5 +++++
>> >> >>  common/lib.c            |  2 +-
>> >> >>  common/report.c         |  8 +++++---
>> >> >>  include/xtf/framework.h |  3 +++
>> >> >>  5 files changed, 29 insertions(+), 5 deletions(-)
>> >> >> 
>> >> >> diff --git a/arch/x86/hvm/traps.c b/arch/x86/hvm/traps.c
>> >> >> index ad7b8cb..b8c4d0c 100644
>> >> >> --- a/arch/x86/hvm/traps.c
>> >> >> +++ b/arch/x86/hvm/traps.c
>> >> >> @@ -1,5 +1,6 @@
>> >> >> -#include <xtf/traps.h>
>> >> >> +#include <xtf/hypercall.h>
>> >> >>  #include <xtf/lib.h>
>> >> >> +#include <xtf/traps.h>
>> >> >>  
>> >> >>  #include <arch/idt.h>
>> >> >>  #include <arch/lib.h>
>> >> >> @@ -139,6 +140,19 @@ void arch_init_traps(void)
>> >> >>                 virt_to_gfn(__end_user_bss));
>> >> >>  }
>> >> >>  
>> >> >> +void arch_shutdown(unsigned int reason)
>> >> >> +{
>> >> >> +    hypercall_shutdown(reason);
>> >> >
>> >> > This relies on the hypercall page being poised with `ret`, which is
>> >> > IMO fragile.  I would rather have it poisoned with `int3` and prevent
>> >> > such stray accesses in the first place.
>> >> 
>> >> I dont' mind caching Xen presence somewhere, but that involves some code 
>> >> motion
>> >> from setup.c, which I wanted to avoid.
>> >
>> > I think it's very likely that at some point we will need to cache this?
>> >
>> > enum {
>> >     NATIVE,
>> >     XEN,
>> >     QEMU,
>> >     ...
>> > } hypervisor_env;
>> >
>> > Or similar.
>> 
>> Maybe NATIVE, XEN_VIRT and NON_XEN_VIRT? I see no reason to distinguish 
>> between
>> TCG, KVM and any other accelerator; and QEMU is imprecise because we use for
>> HVM. You could imagine chainloading XTF from GRUB to test the HVM env.
>
> Maybe not for XTF.  IIRC KVM also offers some PV interfaces (like the
> PV timer) that native QEMU doesn't.

Sure, but we don't want to test KVM PV. It _could_ be used for it, but KVM has
its own unit testing facilities already.

https://gitlab.com/kvm-unit-tests/kvm-unit-tests.git

>
> Rather than having an exclusive hypervisor mode, we could signal what
> interfaces are available.  For example Xen (and I bet KVM too) can
> expose native interfaces plus viridian extensions, in which case we
> might want to detect both if present.  That would require using a
> separate boolean for each extra interface.  IOW:
>
> bool xen_hypercall;
> bool viridian_foo;
> bool qemu_debug;
> ...
>
> (Possibly not the best naming)

I'm of the opinion of not adding things not strictly required.

>
> BTW, is it possible for a guest to discover whether the
> "isa-debug-exit" functionality is present?

Besides ensuring a read gets zero, no. From the QEMU sources:

  static uint64_t debug_exit_read(void *opaque, hwaddr addr, unsigned size)
  {
      return 0;
  }

  static void debug_exit_write(void *opaque, hwaddr addr, uint64_t val,
                               unsigned width)
  {
      qemu_system_shutdown_request_with_code(SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
                                             (val << 1) | 1);
  }

I didn't see any signaling anywhere in CPUID or elsewhere. Though I admit it
was years ago that I last checked, this isn't the sort of feature that changes
very often.

>
> Sorry, I'm possibly derailing this patch series.

Can only mean you find it interesting. That's always good :)

But to concretise actions, I think I'll keep it simple for the time being and
add a single `cpu_has_xen` global boolean; then place the shutdown hypercall
before the QEMU exit device write, gated by cpu_has_xen.

That prevents making a hypercall when the "wrong" hypervisor is present (or
none).

Cheers,
Alejandro