Re: [PATCH 1/2] kvm: x86: Allow userspace to handle emulation errors

Sean Christopherson
2021-04-20T18:21:52Z

On Tue, Apr 20, 2021, David Edmondson wrote:
> On Tuesday, 2021-04-20 at 07:57:27 -07, Aaron Lewis wrote:
>
> >> >> Why not add a new exit reason, particularly given that the caller has to
> >> >> enable the capability to get the relevant data? (It would also remove
> >> >> the need for the flag field and any mechanism for packing multiple bits
> >> >> of detail into the structure.)
> >> >
> >> > I considered that, but I opted for the extensibility of the exiting
> >> > KVM_EXIT_INTERNAL_ERROR instead. To me it was six of one or half a
> >> > dozen of the other. With either strategy I still wanted to provide
> >> > for future extensibility, and had a flags field in place. That way we
> >> > can add to this in the future if we find something that is missing
> >> > (ie: potentially wanting a way to mark dirty pages, possibly passing a
> >> > fault address, etc...)
> >>
> >> How many of the flag based optional fields do you anticipate needing for
> >> any one particular exit scenario?
> >>
> >> If it's one, then using the flags to disambiguate the emulation failure
> >> cases after choosing to stuff all of the cases into
> >> KVM_EXIT_INTERNAL_ERROR / KVM_INTERNAL_ERROR_EMULATION would be odd.
> >>
> >> (I'm presuming that it's not one, but don't understand the use case.)
> >
> > The motivation was to allow for maximum flexibility in the future, and
> > not be tied down to something we potentially missed now. I agree the
> > flags aren't needed if we are only adding to what's currently there,
> > but they are needed if we want to remove something or pack something
> > differently. I didn't see how I could achieve that without adding a
> > flags field. Seemed like low overhead to be more future proof.
>
> With what you have now, the ndata field seems unnecessary - I should be
> able to determine the contents of the rest of the structure based on the
> flags.

Keeping ndata is necessary if we piggyback KVM_INTERNAL_ERROR_EMULATION,
otherwise we'll break for VMMs that are not aware of the new format. E.g. if
ndata gets stuffed with a large number, KVM could cause a buffer overrun in an
old VMM.

> That also suggests to me that using something other than
> KVM_INTERNAL_ERROR_EMULATION would make sense.

Like Aaron, I'm on the fence as to whether or not a new exit reason is in order.
On one hand, it would be slightly cleaner. On the other hand, the existing
"KVM_INTERNAL_ERROR_EMULATION" really is the best name. It implies nothing
about the userspace VMM, only that KVM attempted to emulate an instruction and
failed.

The other motivation is that KVM can opportunistically start dumping extra info
for old VMMs, though this patch does not do that; feedback imminent. :-)

> This comment:
>
> >> >> > + * When using the suberror KVM_INTERNAL_ERROR_EMULATION, these flags are used
> >> >> > + * to describe what is contained in the exit struct. The flags are used to
> >> >> > + * describe it's contents, and the contents should be in ascending numerical
> >> >> > + * order of the flag values. For example, if the flag
> >> >> > + * KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES is set, the instruction
> >> >> > + * length and instruction bytes would be expected to show up first because this
> >> >> > + * flag has the lowest numerical value (1) of all the other flags.
>
> originally made me think that the flag-indicated elements were going to
> be packed into the remaining space of the structure at a position
> depending on which flags are set.
>
> For example, if I add a new flag
> KVM_INTERNAL_ERROR_EMULATION_FLAG_EXIT_CODE, value 2, and then want to
> pass back an exit code but *not* instruction bytes, the comment appears
> to suggest that the exit code will appear immediately after the flags.
>
> This is contradicted by your other reply:
>
> >> > Just add the fields you need to
> >> > the end of emulation_failure struct, increase 'ndata' to the new
> >> > count, add a new flag to 'flags' so we know its contents.
>
> Given this, the ordering of flag values does not seem significant - the
> structure elements corresponding to a flag value will always be present,
> just not filled with relevant data.

I think what Aaron is trying to say is that the order in the aliased data[] is
associated with the lowest _defined_ flag value, not the lowest _set_ flag.

That said, I would just omit the "ascending numerical" stuff entirely, e.g. I
think for the #defines, this will suffice:

/* Flags that describe what fields in emulation_failure hold valid data */

As for not breaking userspace if/when additional fields are added, we can instead
document the new struct (and drop my snarky comment :-D), e.g.:

/*
* KVM_INTERNAL_ERROR_EMULATION
*
* "struct emulation_failure" is an overlay of "struct internal"
* that is used for the KVM_INTERNAL_ERROR_EMULATION sub-type of
* KVM_EXIT_INTERNAL_ERROR. Note, unlike other internal error
* sub-types, this struct is ABI! It also needs to be backwards
* compabile with "struct internal". Take special care that
* "ndata" is correct, that new fields are enumerated in "flags",
* and that each flag enumerates fields that are 64-bit aligned
* and sized (so that ndata+internal.data[] is valid/accurate).
*/
struct {
__u32 suberror;
__u32 ndata;
__u64 flags;
__u8 insn_size;
__u8 insn_bytes[15];
} emulation_failure;