On Monday, 2021-04-19 at 09:47:19 -07, Aaron Lewis wrote:
>> > Add a fallback mechanism to the in-kernel instruction emulator that
>> > allows userspace the opportunity to process an instruction the emulator
>> > was unable to. When the in-kernel instruction emulator fails to process
>> > an instruction it will either inject a #UD into the guest or exit to
>> > userspace with exit reason KVM_INTERNAL_ERROR. This is because it does
>> > not know how to proceed in an appropriate manner. This feature lets
>> > userspace get involved to see if it can figure out a better path
>> > forward.
>>
>> Given that you are intending to try and handle the instruction in
>> user-space, it seems a little odd to overload the
>> KVM_EXIT_INTERNAL_ERROR/KVM_INTERNAL_ERROR_EMULATION exit reason/sub
>> error.
>>
>> 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.)
>> > +/*
>> > + * 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.
>> > + */
>> > +#define KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES (1ULL << 0)
>> > +
>> > /* for KVM_RUN, returned by mmap(vcpu_fd, offset=0) */
>> > struct kvm_run {
>> > /* in */
>> > @@ -382,6 +393,14 @@ struct kvm_run {
>> > __u32 ndata;
>> > __u64 data[16];
>> > } internal;
>> > + /* KVM_EXIT_INTERNAL_ERROR, too (not 2) */
>> > + struct {
>> > + __u32 suberror;
>> > + __u32 ndata;
>> > + __u64 flags;
>> > + __u8 insn_size;
>> > + __u8 insn_bytes[15];
>> > + } emulation_failure;
>> > +/*
>> > + * 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.
>>
>> When adding a new flag, do I steal bytes from insn_bytes[] for my
>> associated payload? If so, how many do I have to leave?
>>
>
> The emulation_failure struct mirrors the internal struct, so if you
> are just adding to what I have, you can safely add up to 16 __u64's.
> I'm currently using the size equivalent to 3 of them (flags,
> insn_size, insn_bytes), so there should be plenty of space left for
> you to add what you need to the end. 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.
My apologies, I mis-read the u8 as u64, so figured that you'd eaten all
of the remaining space.
dme.
--
I walk like a building, I never get wet.
