[PATCH] MSR related clean up

[PATCH] MSR related clean up

[Sheng Yang]

rdmsr_safe() is used to access MSR unknown to Xen, and is not safe... I think
it is legacy.

Also let msr_write be parity with msr_read on MSR_IA32_MISC_ENABLE.

CC: Eddie Dong <eddie.dong@intel.com>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>

diff -r c30742011bb8 -r 64dc4510484e xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Mar 12 18:48:09 2009 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Jun 18 17:10:22 2009 +0800
@@ -1836,8 +1836,7 @@
         }
 
         if ( rdmsr_viridian_regs(ecx, &eax, &edx) ||
-             rdmsr_hypervisor_regs(ecx, &eax, &edx) ||
-             rdmsr_safe(ecx, eax, edx) == 0 )
+             rdmsr_hypervisor_regs(ecx, &eax, &edx) )
         {
             regs->eax = eax;
             regs->edx = edx;
@@ -2008,6 +2007,9 @@
     }
     case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_PROCBASED_CTLS2:
         goto gp_fault;
+    /* Ignore writing to these MSRs */
+    case MSR_IA32_MISC_ENABLE:
+        break;
     default:
         if ( vpmu_do_wrmsr(regs) )
             return X86EMUL_OKAY;

Re: [PATCH] MSR related clean up

[Sheng Yang]

On Thursday 18 June 2009 17:57:06 Sheng Yang wrote:
> rdmsr_safe() is used to access MSR unknown to Xen, and is not safe... I
> think it is legacy.
>
> Also let msr_write be parity with msr_read on MSR_IA32_MISC_ENABLE.
>
> CC: Eddie Dong <eddie.dong@intel.com>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>

Keir?

-- 
regards
Yang, Sheng

>
> diff -r c30742011bb8 -r 64dc4510484e xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Mar 12 18:48:09 2009 +0000
> +++ b/xen/arch/x86/hvm/vmx/vmx.c	Thu Jun 18 17:10:22 2009 +0800
> @@ -1836,8 +1836,7 @@
>          }
>
>          if ( rdmsr_viridian_regs(ecx, &eax, &edx) ||
> -             rdmsr_hypervisor_regs(ecx, &eax, &edx) ||
> -             rdmsr_safe(ecx, eax, edx) == 0 )
> +             rdmsr_hypervisor_regs(ecx, &eax, &edx) )
>          {
>              regs->eax = eax;
>              regs->edx = edx;
> @@ -2008,6 +2007,9 @@
>      }
>      case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_PROCBASED_CTLS2:
>          goto gp_fault;
> +    /* Ignore writing to these MSRs */
> +    case MSR_IA32_MISC_ENABLE:
> +        break;
>      default:
>          if ( vpmu_do_wrmsr(regs) )
>              return X86EMUL_OKAY;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] MSR related clean up

[Keir Fraser]

On 24/06/2009 09:50, "Sheng Yang" <sheng@linux.intel.com> wrote:

> On Thursday 18 June 2009 17:57:06 Sheng Yang wrote:
>> rdmsr_safe() is used to access MSR unknown to Xen, and is not safe... I
>> think it is legacy.
>> 
>> Also let msr_write be parity with msr_read on MSR_IA32_MISC_ENABLE.
>> 
>> CC: Eddie Dong <eddie.dong@intel.com>
>> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> 
> Keir?

Looks pretty dangerous to me. So I'm not sure. There are various MSRs that
are detected via CPU family/model (which we pass through) which would then
#GP on access. Also this doesn't change the AMD default. Overall, what we
have now does seem to work so I'm reluctant to mess with it.

 -- Keir

Re: [PATCH] MSR related clean up

[Sheng Yang]

On Wednesday 24 June 2009 17:03:56 Keir Fraser wrote:
> On 24/06/2009 09:50, "Sheng Yang" <sheng@linux.intel.com> wrote:
> > On Thursday 18 June 2009 17:57:06 Sheng Yang wrote:
> >> rdmsr_safe() is used to access MSR unknown to Xen, and is not safe... I
> >> think it is legacy.
> >>
> >> Also let msr_write be parity with msr_read on MSR_IA32_MISC_ENABLE.
> >>
> >> CC: Eddie Dong <eddie.dong@intel.com>
> >> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> >
> > Keir?
>
> Looks pretty dangerous to me. So I'm not sure. There are various MSRs that
> are detected via CPU family/model (which we pass through) which would then
> #GP on access. Also this doesn't change the AMD default. Overall, what we
> have now does seem to work so I'm reluctant to mess with it.
>
Hi Keir

What we suffered now is, there are some MSRs existed in CPU, but shouldn't be 
accessed by guest. And guest should expected a GP fault for accessing, but we 
return a real value, which is not desired at all.

And in general, reading from unknown native MSR is dangerous, and also break 
host/guest isolation. I think we at least should control what we read from 
native. Maybe add more MSR handling is necessary.

-- 
regards
Yang, Sheng

Re: [PATCH] MSR related clean up

[Keir Fraser]

On 24/06/2009 10:21, "Sheng Yang" <sheng@linux.intel.com> wrote:

> What we suffered now is, there are some MSRs existed in CPU, but shouldn't be
> accessed by guest. And guest should expected a GP fault for accessing, but we
> return a real value, which is not desired at all.
>
> And in general, reading from unknown native MSR is dangerous, and also break
> host/guest isolation. I think we at least should control what we read from
> native. Maybe add more MSR handling is necessary.

I kind of agree with you, up to but not including delivering #GPs that would
not be delivered when running the guest OS natively. A middle ground might
be to return all zeros for unknown MSRs for which rdmsr_safe() succeeds.
That is by far the most popular bodge value we manually use to fix up cases
where returning the real MSR value was actually a proven problem.

 -- Keir

RE: [PATCH] MSR related clean up

[Dong, Eddie]

Keir Fraser wrote:
> On 24/06/2009 10:21, "Sheng Yang" <sheng@linux.intel.com> wrote:
> 
>> What we suffered now is, there are some MSRs existed in CPU, but
>> shouldn't be accessed by guest. And guest should expected a GP fault
>> for accessing, but we return a real value, which is not desired at
>> all. 
>> 
>> And in general, reading from unknown native MSR is dangerous, and
>> also break host/guest isolation. I think we at least should control
>> what we read from native. Maybe add more MSR handling is necessary.
> 
> I kind of agree with you, up to but not including delivering #GPs
> that would not be delivered when running the guest OS natively. A
> middle ground might be to return all zeros for unknown MSRs for which
> rdmsr_safe() succeeds. That is by far the most popular bodge value we
> manually use to fix up cases where returning the real MSR value was
> actually a proven problem. 
> 
>  -- Keir

Returning 0 solves the security concern. But the argument is still that if the guest should see same MSR sets with native. The CPUID virtualization provides close features with native, but still not identical.
An ideal solution for those MSR read should consult guest CPUID and then decide to either inject #GP if guest CPUID doesn't indicate this MSR, or return a virtual MSR. In this case MSR write side should provide the virtual MSR too. 
BTW, user can identify certain filtering policy or force some bits of guest CPUID, so current approach can't satisfy both cases.

Eddie

Re: [PATCH] MSR related clean up

[Keir Fraser]

On 24/06/2009 10:45, "Dong, Eddie" <eddie.dong@intel.com> wrote:

> Returning 0 solves the security concern. But the argument is still that if the
> guest should see same MSR sets with native. The CPUID virtualization provides
> close features with native, but still not identical.
> An ideal solution for those MSR read should consult guest CPUID and then
> decide to either inject #GP if guest CPUID doesn't indicate this MSR, or
> return a virtual MSR. In this case MSR write side should provide the virtual
> MSR too.

Nice plan, but apart from my doubts about anyone actually bothering to a
comprehensive job of this for current processors, there's also the problem
that future processors may have MSRs detected via means such as
model/family-id which we currently pass through.

 -- Keir

> BTW, user can identify certain filtering policy or force some bits of guest
> CPUID, so current approach can't satisfy both cases.

RE: [PATCH] MSR related clean up

[Dong, Eddie]

Keir Fraser wrote:
> On 24/06/2009 10:45, "Dong, Eddie" <eddie.dong@intel.com> wrote:
> 
>> Returning 0 solves the security concern. But the argument is still
>> that if the guest should see same MSR sets with native. The CPUID
>> virtualization provides close features with native, but still not
>> identical. 
>> An ideal solution for those MSR read should consult guest CPUID and
>> then decide to either inject #GP if guest CPUID doesn't indicate
>> this MSR, or return a virtual MSR. In this case MSR write side
>> should provide the virtual MSR too.
> 
> Nice plan, but apart from my doubts about anyone actually bothering
> to a comprehensive job of this for current processors, there's also
> the problem that future processors may have MSRs detected via means
> such as model/family-id which we currently pass through.
> 
The simpliest change is to create the list of guest MSR so that guest read/write MSR can be correctly emulated. The size of guest MSR is probably not very big (several tens?). Of course we can have a detect of full guest MSR list at domain create time if guest CPUID (model/family etc) is same with native to keep architectural correctness. Of course the initial value can come from native.

A guest may use a non existing MSR to trigger into #GP handler. BTW those kind of native capability detect mechanism hurts migration.

Thx, eddie