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

[David Edmondson <dme@dme.org>]

On Tuesday, 2021-04-20 at 18:21:42 GMT, Sean Christopherson wrote:

> On Tue, Apr 20, 2021, David Edmondson wrote:
>> 
>> 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.

Agreed.

>> 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. :-)

It's nothing more than that the interface ends up feeling a little
strange. With several flags added and some of the earlier flags unused,
ndata ends up indicating the largest extent of the flag-indicated data,
but the earlier elements of the structure are unused. Hence the question
about how many flags we anticipate using simultaneously.

(I'm not really arguing that we should be packing the stuff in and
having to decode it, as that is also unpleasant.)

>> 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  */

Agreed.

> 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;

Looks good (even with the snark).

dme.
-- 
People in love get everything wrong.