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

[Matthew Barnes <matthew.barnes@xxxxxxxxx>]

On Fri, Jun 21, 2024 at 03:38:36PM +0000, Anthony PERARD wrote:
> 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.)

I will remove the unnecessary headers in patch v2.

> > +    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_*)

To my understanding, the event types enum refer to the kind of interrupt
being handled. In this case, it is a hardware exception, which already
exists as an entry in the enum definition.

The `vector` parameter describes the kind of exception. I just found
that exception vector macros are defined in `x86-defns.h`, so I will
include that and instead use `X86_EXC_DF` in patch v2.

The only other usage of `xc_hvm_inject_trap` is in `xen-access.c`, which
defines all the required vectors as macros at the top of the source file.
Hence, I did the same in `xen-hvmcrash.c` for patch v1.

Would it be a good idea to rewrite `xen-access.c` to use `x86-defns.h`
as well, in a later patch?

> > +                                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.

I will use strerror and one print call in patch v2.

> Are you meant to keep inject into other vcpus even if one have failed?

Yes; xen-hvmcrash doesn't have to inject *all* vcpus to cause it to
crash.

> Should `xen-hvmcrash` return success when it failed to inject the double
> fault to all vcpus?

I will make xen-hvmcrash yield an error if no vcpus could be injected in
patch v2.

Matt