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.garciavall...@amd.com>
>> >> >> ---
>> >> >> 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
