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

[Aaron Lewis <aaronlewis@google.com>]

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

It's hard to say how many flags will be used, but I suspect it will be
a small set (I mentioned previously a possibility of 2 more), so I
like Sean's suggestion to keep it simple and just use the flags to
indicate which fields are used, instead of trying to pack everything
in tightly.

On Wed, Apr 21, 2021 at 1:39 AM David Edmondson <dme@dme.org> wrote:
>
> On Tuesday, 2021-04-20 at 18:34:48 UTC, Sean Christopherson wrote:
>
> > On Fri, Apr 16, 2021, Aaron Lewis wrote:
> >> +                    KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
> >> +            vcpu->run->emulation_failure.insn_size = insn_size;
> >> +            memcpy(vcpu->run->emulation_failure.insn_bytes,
> >> +                   ctxt->fetch.data, sizeof(ctxt->fetch.data));
> >
> > Doesn't truly matter, but I think it's less confusing to copy over insn_size
> > bytes.
>
> And zero out the rest?
>

I left the memcpy alone as this seems like a more concervative / safer
approach.  I've had and have seen too many memcpy fails that my
preference is to be conservative here.  I could add a comment to
clarify any confusion that this may bring by not using insn_size if
that will help.


Cheers,
Aaron