On Tue, Mar 19, 2019 at 07:49:42AM -0700, Sean Christopherson wrote:
> On Mon, Mar 18, 2019 at 09:46:22PM -0400, Krish Sadhukhan wrote:
> > According to section "Checks on Host Control Registers and MSRs" in Intel
> > SDM vol 3C, the following check is performed on vmentry of L2 guests:
>
> I think you can leave off "of L2 guests". That implies VM-Entry to other
> levels are not subject to this check.
>
> >
> > If the "load IA32_PAT" VM-exit control is 1, the value of the field
> > for the IA32_PAT MSR must be one that could be written by WRMSR
> > without fault at CPL 0. Specifically, each of the 8 bytes in the
> > field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
> > 6 (WB), or 7 (UC-).
> >
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> > Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 3170e291215d..a1b44d930d26 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2583,6 +2583,24 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
> > return 0;
> > }
> >
> > +/*
> > + * Fields in IA32_PAT MSR are allowed to contain one of the
> > + * following values:
> > + * 0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-)
> > + */
> > +static int nested_check_pat_fields(u64 msr)
>
> Maybe just nested_check_pat()? The SDM uses "field" to refer to the
> VMCS field, not the individual bytes in the MSR.
>
> > +{
> > + int i;
> > + u64 tmp = (msr >> i * 8) & 0xff;
>
> This uses 'i' uninitialized and is obviously intended to be in the for
> loop. You can actually avoid 'i' and the multiplication entirely since
> '0' is a legal PAT value (UC), e.g.:
>
> u64 tmp;
>
> for ( ; msr; msr >>= 8) {
> tmp = msr & 0xff;
> if (tmp == 0x2 || tmp == 0x3 || tmp > 0x7)
> return -EINVAL;
> }
> return 0;
Even better, just use kvm_mtrr_valid().
>
> > +
> > + for (i = 0; i < 8; i++) {
> > + if (tmp == 0x2 || tmp == 0x3 || tmp > 0x7)
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Checks related to Host Control Registers and MSRs
> > */
> > @@ -2595,6 +2613,12 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> > !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
> > !nested_cr3_valid(vcpu, vmcs12->host_cr3))
> > return -EINVAL;
> > +
> > + if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
> > + if (nested_check_pat_fields(vmcs12->host_ia32_pat))
>
> This can be a single if statement connected with &&.
>
> > + return -EINVAL;
> > + }
> > +
> > /*
> > * If the load IA32_EFER VM-exit control is 1, bits reserved in the
> > * IA32_EFER MSR must be 0 in the field for that register. In addition,
> > --
> > 2.17.2
> >
