On Fri, Apr 24, 2020 at 10:47:04AM -0700, Jim Mattson wrote:
> On Fri, Apr 24, 2020 at 10:19 AM Sean Christopherson
> <sean.j.christopherson@xxxxxxxxx> wrote:
> > if (from_vmentry) {
> > exit_reason = EXIT_REASON_MSR_LOAD_FAIL;
> > - exit_qual = nested_vmx_load_msr(vcpu,
> > - vmcs12->vm_entry_msr_load_addr,
> > - vmcs12->vm_entry_msr_load_count);
> > - if (exit_qual)
> > + failed_msr = nested_vmx_load_msr(vcpu,
> > + vmcs12->vm_entry_msr_load_addr,
> > + vmcs12->vm_entry_msr_load_count);
> > + if (failed_msr) {
> > + entry_failure_code = failed_msr;
>
> This assignment is a bit dodgy from a type perspective, and suggests
> that perhaps a better type for the local variable is an
> undiscriminated union of the enumerated type and a sufficiently large
> unsigned integer type. But I won't be a stickler if you add a comment.
> :-)
This is a bit ugly. A union doesn't work well because writing the enum
field isn't guaranteed to write the full width of the union, i.e. could
lead to uninitialized variable usage without additional initialization
of the union. The reverse is true as well, e.g. if the compiler sizes the
enum to be larger than an unisigned int.
Rather than use a common local variable, I think it's best to set vmcs12
directly. That also provides an opportunity to set exit_reason on demand
instead of speculatively setting it, which has always bugged me.
v2 incoming.
