Re: [XEN PATCH] tools/misc: xen-hvmcrash: Inject #DF instead of overwriting RIP

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Jun 03, 2024 at 03:59:18PM +0100, Matthew Barnes wrote:
> diff --git a/tools/misc/xen-hvmcrash.c b/tools/misc/xen-hvmcrash.c
> index 1d058fa40a47..8ef1beb388f8 100644
> --- a/tools/misc/xen-hvmcrash.c
> +++ b/tools/misc/xen-hvmcrash.c
> @@ -38,22 +38,21 @@
>  #include <sys/stat.h>
>  #include <arpa/inet.h>
>
> +#define XC_WANT_COMPAT_DEVICEMODEL_API
>  #include <xenctrl.h>
>  #include <xen/xen.h>
>  #include <xen/domctl.h>
>  #include <xen/hvm/save.h>

There's lots of headers that aren't used by the new codes and can be
removed. (They were probably way too many headers included when this
utility was introduced.)

> +    for (vcpu_id = 0; vcpu_id <= dominfo.max_vcpu_id; vcpu_id++) {
> +        printf("Injecting #DF to vcpu ID #%d...\n", vcpu_id);
> +        ret = xc_hvm_inject_trap(xch, domid, vcpu_id,
> +                                X86_ABORT_DF,

In the definition of xendevicemodel_inject_event(), the comment say to
look at "enum x86_event_type" for possible event "type", but there's no
"#DF" type, can we add this new one there before using it? (It's going
to be something like X86_EVENTTYPE_*)

> +                                XEN_DMOP_EVENT_hw_exc, 0,
> +                                NULL, NULL);

The new code doesn't build, "NULL" aren't integers.

> +        if (ret < 0) {
> +            fprintf(stderr, "Could not inject #DF to vcpu ID #%d\n", 
> vcpu_id);
> +            perror("xc_hvm_inject_trap");

Are you meant to print two error lines when there's an error? You can
also use strerror() to convert an "errno" to a string.

Also, perror() might print an error from fprintf() if that last one
failed.

Are you meant to keep inject into other vcpus even if one have failed?
Should `xen-hvmcrash` return success when it failed to inject the double
fault to all vcpus?

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech