All of this feedback sounds straightforward. I'll send a v2 out later
today. Thanks!
> On Thu, Sep 12, 2019 at 11:11:00AM -0700, Marc Orr wrote:
> > Allowing an unlimited number of MSRs to be specified via the VMX
> > load/store MSR lists (e.g., vm-entry MSR load list) is bad for two
> > reasons. First, a guest can specify an unreasonable number of MSRs,
> > forcing KVM to process all of them in software. Second, the SDM bounds
> > the number of MSRs allowed to be packed into the atomic switch MSR lists.
> > Quoting the appendix chapter, titled "MISCELLANEOUS DATA":
>
> Super Nit: There are multiple appendices in the SDM, maybe this?
>
> Quoting the "Miscellaneous Data" section in the "VMX Capability Reporting
> Facility" appendix:
Will do in v2.
> > +#define VMX_MISC_MSR_LIST_INCREMENT 512
>
> VMX_MISC_MSR_LIST_MULTIPLIER seems more appropriate. INCREMENT makes it
> sound like X number of entries get tacked on to the end.
Will do in v2.
> > +static u64 vmx_control_msr(u32 low, u32 high);
>
> vmx_control_msr() is a 'static inline', just hoist it above this function.
> It probably makes sense to move vmx_control_verify() too, maybe to just
> below nested_vmx_abort()?
Will do in v2.
> > msr.host_initiated = false;
> > for (i = 0; i < count; i++) {
> > + if (unlikely(i >= max_msr_list_size))
>
> Although the SDM gives us leeway to do whatever we please since it states
> this is undefined behavior, KVM should at least be consistent between
> nested_vmx_load_msr() and nested_vmx_load_msr(). Here it fails only after
> processing the first N MSRs, while in the store case it fails immediately.
>
> I'm guessing you opted for the divergent behavior because
> nested_vmx_load_msr() returns the failing index for VM-Enter, but
> nested_vmx_store_msr() has visible side effects too. E.g. storing the
> MSRs modifies memory, which could theoretically be read by other CPUs
> since VMX abort only brings down the current logical CPU.
Ok. I'll update nested_vmx_store_msr() to iterate over as many MSRs as
it can in v2.
