All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86: ignore guest microcode loading attempts
@ 2018-03-13 10:13 Jan Beulich
  2018-03-13 10:36 ` Andrew Cooper
  2018-03-15  9:59 ` Andrew Cooper
  0 siblings, 2 replies; 11+ messages in thread
From: Jan Beulich @ 2018-03-13 10:13 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper

The respective MSRs are write-only, and hence attempts by guests to
write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
impression of WRMSR succeeding") no longer ignored. Restore original
behavior for the two affected MSRs.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While what is being logged for the current osstest failure on the 4.7
branch (I have to admit that I don't understand why it's only that
branch which shows a regression) doesn't fully prove this to be the
problem, RCX holding 0x79 and there being a recorded hypervisor level
#GP recovery immediately before the guest triple fault is sufficient
indication imo.
What I'm unsure about is whether we want to ignore such writes also for
PV guests. If not, at least the WRMSR change would need to move into
hvm/hvm.c.

--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -147,6 +147,8 @@ int guest_rdmsr(const struct vcpu *v, ui
 
     switch ( msr )
     {
+    case MSR_AMD_PATCHLOADER:
+    case MSR_IA32_UCODE_WRITE:
     case MSR_PRED_CMD:
         /* Write-only */
         goto gp_fault;
@@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
         /* Read-only */
         goto gp_fault;
 
+    case MSR_AMD_PATCHLOADER:
+        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
+            goto gp_fault;
+        break;
+
+    case MSR_IA32_UCODE_WRITE:
+        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
+            goto gp_fault;
+        break;
+
     case MSR_SPEC_CTRL:
         if ( !cp->feat.ibrsb )
             goto gp_fault; /* MSR available? */




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-13 10:13 [PATCH] x86: ignore guest microcode loading attempts Jan Beulich
@ 2018-03-13 10:36 ` Andrew Cooper
  2018-03-13 11:51   ` Jan Beulich
  2018-03-15  9:59 ` Andrew Cooper
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-03-13 10:36 UTC (permalink / raw)
  To: Jan Beulich, xen-devel

On 13/03/2018 10:13, Jan Beulich wrote:
> The respective MSRs are write-only, and hence attempts by guests to
> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
> impression of WRMSR succeeding") no longer ignored. Restore original
> behavior for the two affected MSRs.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> While what is being logged for the current osstest failure on the 4.7
> branch (I have to admit that I don't understand why it's only that
> branch which shows a regression)

Differences in advertised viridian?

>  doesn't fully prove this to be the
> problem, RCX holding 0x79 and there being a recorded hypervisor level
> #GP recovery immediately before the guest triple fault is sufficient
> indication imo.
> What I'm unsure about is whether we want to ignore such writes also for
> PV guests. If not, at least the WRMSR change would need to move into
> hvm/hvm.c.

Hmm - I'd very much like not to make this change, because it sets a bad
precedent for making the MSR handling sane.  We shouldn't be silently
dropping MSR writes, as that will cause more problems for the guests,
rather than less.

Given that it is appears to be just 4.7 which is affected, I think it is
worth trying to work out what is causing 4.8 and later to be fine, and
whether that is a better solution to backport.

>
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -147,6 +147,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>  
>      switch ( msr )
>      {
> +    case MSR_AMD_PATCHLOADER:
> +    case MSR_IA32_UCODE_WRITE:
>      case MSR_PRED_CMD:
>          /* Write-only */
>          goto gp_fault;
> @@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>          /* Read-only */
>          goto gp_fault;
>  
> +    case MSR_AMD_PATCHLOADER:
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )

If we do end up going with this patch, then there is probably a cp-> in
context.  Also, I'd need to double check the indices order.

~Andrew

> +            goto gp_fault;
> +        break;
> +
> +    case MSR_IA32_UCODE_WRITE:
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
> +            goto gp_fault;
> +        break;
> +
>      case MSR_SPEC_CTRL:
>          if ( !cp->feat.ibrsb )
>              goto gp_fault; /* MSR available? */
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-13 10:36 ` Andrew Cooper
@ 2018-03-13 11:51   ` Jan Beulich
  2018-03-15  8:37     ` Ping: " Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2018-03-13 11:51 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel

>>> On 13.03.18 at 11:36, <andrew.cooper3@citrix.com> wrote:
> On 13/03/2018 10:13, Jan Beulich wrote:
>> The respective MSRs are write-only, and hence attempts by guests to
>> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
>> impression of WRMSR succeeding") no longer ignored. Restore original
>> behavior for the two affected MSRs.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> While what is being logged for the current osstest failure on the 4.7
>> branch (I have to admit that I don't understand why it's only that
>> branch which shows a regression)
> 
> Differences in advertised viridian?
> 
>>  doesn't fully prove this to be the
>> problem, RCX holding 0x79 and there being a recorded hypervisor level
>> #GP recovery immediately before the guest triple fault is sufficient
>> indication imo.
>> What I'm unsure about is whether we want to ignore such writes also for
>> PV guests. If not, at least the WRMSR change would need to move into
>> hvm/hvm.c.
> 
> Hmm - I'd very much like not to make this change, because it sets a bad
> precedent for making the MSR handling sane.  We shouldn't be silently
> dropping MSR writes, as that will cause more problems for the guests,
> rather than less.
> 
> Given that it is appears to be just 4.7 which is affected, I think it is
> worth trying to work out what is causing 4.8 and later to be fine, and
> whether that is a better solution to backport.

The latest flight on 4.9 shows the same issue.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Ping: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-13 11:51   ` Jan Beulich
@ 2018-03-15  8:37     ` Jan Beulich
  2018-03-15  9:43       ` Sergey Dyasli
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2018-03-15  8:37 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Paul Durrant

>>> On 13.03.18 at 12:51, <JBeulich@suse.com> wrote:
>>>> On 13.03.18 at 11:36, <andrew.cooper3@citrix.com> wrote:
>> On 13/03/2018 10:13, Jan Beulich wrote:
>>> The respective MSRs are write-only, and hence attempts by guests to
>>> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
>>> impression of WRMSR succeeding") no longer ignored. Restore original
>>> behavior for the two affected MSRs.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> While what is being logged for the current osstest failure on the 4.7
>>> branch (I have to admit that I don't understand why it's only that
>>> branch which shows a regression)
>> 
>> Differences in advertised viridian?
>> 
>>>  doesn't fully prove this to be the
>>> problem, RCX holding 0x79 and there being a recorded hypervisor level
>>> #GP recovery immediately before the guest triple fault is sufficient
>>> indication imo.
>>> What I'm unsure about is whether we want to ignore such writes also for
>>> PV guests. If not, at least the WRMSR change would need to move into
>>> hvm/hvm.c.
>> 
>> Hmm - I'd very much like not to make this change, because it sets a bad
>> precedent for making the MSR handling sane.  We shouldn't be silently
>> dropping MSR writes, as that will cause more problems for the guests,
>> rather than less.
>> 
>> Given that it is appears to be just 4.7 which is affected, I think it is
>> worth trying to work out what is causing 4.8 and later to be fine, and
>> whether that is a better solution to backport.
> 
> The latest flight on 4.9 shows the same issue.

I realize it's generally too early for a ping, but with at least two of
the stable trees now blocked on this (and specifically the ones we
want to cut a release from soon), I'd really like to either see this
patch acked or viable alternative proposals be made. FTR, I don't
think reverting the change that caused the issue to surface is an
option: We had specifically agreed to deal with fallout on a case by
case basis.

From the pattern of the failures it's only a matter of time for other
branches to also become blocked on this.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Ping: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-15  8:37     ` Ping: " Jan Beulich
@ 2018-03-15  9:43       ` Sergey Dyasli
  0 siblings, 0 replies; 11+ messages in thread
From: Sergey Dyasli @ 2018-03-15  9:43 UTC (permalink / raw)
  To: JBeulich, Andrew Cooper; +Cc: Sergey Dyasli, Paul Durrant, xen-devel

On Thu, 2018-03-15 at 02:37 -0600, Jan Beulich wrote:
> > > > On 13.03.18 at 12:51, <JBeulich@suse.com> wrote:
> > > > > On 13.03.18 at 11:36, <andrew.cooper3@citrix.com> wrote:
> > > 
> > > On 13/03/2018 10:13, Jan Beulich wrote:
> > > > The respective MSRs are write-only, and hence attempts by guests to
> > > > write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
> > > > impression of WRMSR succeeding") no longer ignored. Restore original
> > > > behavior for the two affected MSRs.
> > > > 
> > > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > > ---
> > > > While what is being logged for the current osstest failure on the 4.7
> > > > branch (I have to admit that I don't understand why it's only that
> > > > branch which shows a regression)
> > > 
> > > Differences in advertised viridian?
> > > 
> > > >  doesn't fully prove this to be the
> > > > problem, RCX holding 0x79 and there being a recorded hypervisor level
> > > > #GP recovery immediately before the guest triple fault is sufficient
> > > > indication imo.
> > > > What I'm unsure about is whether we want to ignore such writes also for
> > > > PV guests. If not, at least the WRMSR change would need to move into
> > > > hvm/hvm.c.
> > > 
> > > Hmm - I'd very much like not to make this change, because it sets a bad
> > > precedent for making the MSR handling sane.  We shouldn't be silently
> > > dropping MSR writes, as that will cause more problems for the guests,
> > > rather than less.
> > > 
> > > Given that it is appears to be just 4.7 which is affected, I think it is
> > > worth trying to work out what is causing 4.8 and later to be fine, and
> > > whether that is a better solution to backport.
> > 
> > The latest flight on 4.9 shows the same issue.
> 
> I realize it's generally too early for a ping, but with at least two of
> the stable trees now blocked on this (and specifically the ones we
> want to cut a release from soon), I'd really like to either see this
> patch acked or viable alternative proposals be made. FTR, I don't
> think reverting the change that caused the issue to surface is an
> option: We had specifically agreed to deal with fallout on a case by
> case basis.
> 
> From the pattern of the failures it's only a matter of time for other
> branches to also become blocked on this.

SDM states that this MSR is architectural and hence should always be
available to a guest. I couldn't find any information about how a #GP
fault may be raised during wrmsr or what happens if microcode update
fails. Looks like the guest should just check the resulting value in
MSR_IA32_UCODE_REV (which can be emulated by Xen).

Overall, the patch looks sensible to me. The only thing I would like to
see is some code comments about why this wrmsr is silently dropped.

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-13 10:13 [PATCH] x86: ignore guest microcode loading attempts Jan Beulich
  2018-03-13 10:36 ` Andrew Cooper
@ 2018-03-15  9:59 ` Andrew Cooper
  2018-03-15 10:40   ` Jan Beulich
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-03-15  9:59 UTC (permalink / raw)
  To: Jan Beulich, xen-devel

On 13/03/18 10:13, Jan Beulich wrote:
> The respective MSRs are write-only, and hence attempts by guests to
> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
> impression of WRMSR succeeding") no longer ignored. Restore original
> behavior for the two affected MSRs.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> While what is being logged for the current osstest failure on the 4.7
> branch (I have to admit that I don't understand why it's only that
> branch which shows a regression) doesn't fully prove this to be the
> problem, RCX holding 0x79 and there being a recorded hypervisor level
> #GP recovery immediately before the guest triple fault is sufficient
> indication imo.
> What I'm unsure about is whether we want to ignore such writes also for
> PV guests. If not, at least the WRMSR change would need to move into
> hvm/hvm.c.
>
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -147,6 +147,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>  
>      switch ( msr )
>      {
> +    case MSR_AMD_PATCHLOADER:
> +    case MSR_IA32_UCODE_WRITE:
>      case MSR_PRED_CMD:

I've been keeping the labels in numeric order (grouped by
read/write-only status where applicable) on the expectation that you'd
insist on them being in numeric order.

>          /* Write-only */
>          goto gp_fault;
> @@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>          /* Read-only */
>          goto gp_fault;
>  
> +    case MSR_AMD_PATCHLOADER:
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
> +            goto gp_fault;
> +        break;
> +
> +    case MSR_IA32_UCODE_WRITE:
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
> +            goto gp_fault;

Can we leave a note here that Windows at least on some hardware loads
microcode before setting up an IDT/GDT, and will triple fault if we hand
it back #GP.  Ignoring the write means windows will see the same
microcode version after the load attempt, and conclude that it didn't
succeed?

~Andrew

> +        break;
> +
>      case MSR_SPEC_CTRL:
>          if ( !cp->feat.ibrsb )
>              goto gp_fault; /* MSR available? */
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-15  9:59 ` Andrew Cooper
@ 2018-03-15 10:40   ` Jan Beulich
  2018-03-15 11:03     ` Andrew Cooper
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2018-03-15 10:40 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel

>>> On 15.03.18 at 10:59, <andrew.cooper3@citrix.com> wrote:
> On 13/03/18 10:13, Jan Beulich wrote:
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -147,6 +147,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>>  
>>      switch ( msr )
>>      {
>> +    case MSR_AMD_PATCHLOADER:
>> +    case MSR_IA32_UCODE_WRITE:
>>      case MSR_PRED_CMD:
> 
> I've been keeping the labels in numeric order (grouped by
> read/write-only status where applicable) on the expectation that you'd
> insist on them being in numeric order.

Oh, that's sort of unexpected - when the numbers aren't visible,
and when it's not likely that readers would easily be able to associate
them, it looked more reasonable to me to sort alphabetically.

>> @@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>>          /* Read-only */
>>          goto gp_fault;
>>  
>> +    case MSR_AMD_PATCHLOADER:
>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
>> +            goto gp_fault;
>> +        break;
>> +
>> +    case MSR_IA32_UCODE_WRITE:
>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
>> +            goto gp_fault;
> 
> Can we leave a note here that Windows at least on some hardware loads
> microcode before setting up an IDT/GDT, and will triple fault if we hand
> it back #GP.

Will do.

>  Ignoring the write means windows will see the same
> microcode version after the load attempt, and conclude that it didn't
> succeed?

That's what I imply. After all things have worked before, where
we also silently dropped these writes.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-15 10:40   ` Jan Beulich
@ 2018-03-15 11:03     ` Andrew Cooper
  2018-03-15 11:13       ` Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-03-15 11:03 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel

On 15/03/18 10:40, Jan Beulich wrote:
>
>>> @@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>>>          /* Read-only */
>>>          goto gp_fault;
>>>  
>>> +    case MSR_AMD_PATCHLOADER:
>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
>>> +            goto gp_fault;
>>> +        break;
>>> +
>>> +    case MSR_IA32_UCODE_WRITE:
>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
>>> +            goto gp_fault;
>> Can we leave a note here that Windows at least on some hardware loads
>> microcode before setting up an IDT/GDT, and will triple fault if we hand
>> it back #GP.
> Will do.
>
>>  Ignoring the write means windows will see the same
>> microcode version after the load attempt, and conclude that it didn't
>> succeed?
> That's what I imply. After all things have worked before, where
> we also silently dropped these writes.

Actually, on further investigation, we've always had a read_safe() test
for PV, which means that PV guests have always unilaterally seen #GP. 
Can we retain that behaviour please?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-15 11:03     ` Andrew Cooper
@ 2018-03-15 11:13       ` Jan Beulich
  2018-03-15 11:21         ` Andrew Cooper
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2018-03-15 11:13 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel

>>> On 15.03.18 at 12:03, <andrew.cooper3@citrix.com> wrote:
> On 15/03/18 10:40, Jan Beulich wrote:
>>
>>>> @@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>>>>          /* Read-only */
>>>>          goto gp_fault;
>>>>  
>>>> +    case MSR_AMD_PATCHLOADER:
>>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
>>>> +            goto gp_fault;
>>>> +        break;
>>>> +
>>>> +    case MSR_IA32_UCODE_WRITE:
>>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
>>>> +            goto gp_fault;
>>> Can we leave a note here that Windows at least on some hardware loads
>>> microcode before setting up an IDT/GDT, and will triple fault if we hand
>>> it back #GP.
>> Will do.
>>
>>>  Ignoring the write means windows will see the same
>>> microcode version after the load attempt, and conclude that it didn't
>>> succeed?
>> That's what I imply. After all things have worked before, where
>> we also silently dropped these writes.
> 
> Actually, on further investigation, we've always had a read_safe() test
> for PV, which means that PV guests have always unilaterally seen #GP. 
> Can we retain that behaviour please?

Well, that's exactly what I wanted to gather opinions on by having
written:

"What I'm unsure about is whether we want to ignore such writes also for
 PV guests. If not, at least the WRMSR change would need to move into
 hvm/hvm.c."

I've just sent v2, but I can certainly send v3 with the WRMSR side
code moved (to be honest I'm not convinced we want all sorts of
is_{hvm,pv}_{domain,vcpu}() checks in guest_{rd,wr}msr(), but if
that was your plan, then the code could also stay where it is).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-15 11:13       ` Jan Beulich
@ 2018-03-15 11:21         ` Andrew Cooper
  2018-03-15 11:27           ` Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-03-15 11:21 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel

On 15/03/18 11:13, Jan Beulich wrote:
>>>> On 15.03.18 at 12:03, <andrew.cooper3@citrix.com> wrote:
>> On 15/03/18 10:40, Jan Beulich wrote:
>>>>> @@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>>>>>          /* Read-only */
>>>>>          goto gp_fault;
>>>>>  
>>>>> +    case MSR_AMD_PATCHLOADER:
>>>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
>>>>> +            goto gp_fault;
>>>>> +        break;
>>>>> +
>>>>> +    case MSR_IA32_UCODE_WRITE:
>>>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
>>>>> +            goto gp_fault;
>>>> Can we leave a note here that Windows at least on some hardware loads
>>>> microcode before setting up an IDT/GDT, and will triple fault if we hand
>>>> it back #GP.
>>> Will do.
>>>
>>>>  Ignoring the write means windows will see the same
>>>> microcode version after the load attempt, and conclude that it didn't
>>>> succeed?
>>> That's what I imply. After all things have worked before, where
>>> we also silently dropped these writes.
>> Actually, on further investigation, we've always had a read_safe() test
>> for PV, which means that PV guests have always unilaterally seen #GP. 
>> Can we retain that behaviour please?
> Well, that's exactly what I wanted to gather opinions on by having
> written:
>
> "What I'm unsure about is whether we want to ignore such writes also for
>  PV guests. If not, at least the WRMSR change would need to move into
>  hvm/hvm.c."
>
> I've just sent v2, but I can certainly send v3 with the WRMSR side
> code moved (to be honest I'm not convinced we want all sorts of
> is_{hvm,pv}_{domain,vcpu}() checks in guest_{rd,wr}msr(), but if
> that was your plan, then the code could also stay where it is).

Yeah - I noticed and replied there.  If you agree with my suggestion
then there is no need to post a v3.

I was always expecting to have some pv/hvm split in the
guest_{rd,wr}msr() side of things, similar to how we do the legacy CPUID
handling.

However, I want everything to be implemented in guest_{rd,wr}msr() to
set the expectation that new additions are symmetric WRT guest types,
and I hope that the overwhelming majority of code will be based on
proper architectural information in the CPUID/MSR policies, rather than
on domain type specifically.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86: ignore guest microcode loading attempts
  2018-03-15 11:21         ` Andrew Cooper
@ 2018-03-15 11:27           ` Jan Beulich
  0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2018-03-15 11:27 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel

>>> On 15.03.18 at 12:21, <andrew.cooper3@citrix.com> wrote:
> On 15/03/18 11:13, Jan Beulich wrote:
>>>>> On 15.03.18 at 12:03, <andrew.cooper3@citrix.com> wrote:
>>> On 15/03/18 10:40, Jan Beulich wrote:
>>>>>> @@ -200,6 +202,16 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>>>>>>          /* Read-only */
>>>>>>          goto gp_fault;
>>>>>>  
>>>>>> +    case MSR_AMD_PATCHLOADER:
>>>>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
>>>>>> +            goto gp_fault;
>>>>>> +        break;
>>>>>> +
>>>>>> +    case MSR_IA32_UCODE_WRITE:
>>>>>> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
>>>>>> +            goto gp_fault;
>>>>> Can we leave a note here that Windows at least on some hardware loads
>>>>> microcode before setting up an IDT/GDT, and will triple fault if we hand
>>>>> it back #GP.
>>>> Will do.
>>>>
>>>>>  Ignoring the write means windows will see the same
>>>>> microcode version after the load attempt, and conclude that it didn't
>>>>> succeed?
>>>> That's what I imply. After all things have worked before, where
>>>> we also silently dropped these writes.
>>> Actually, on further investigation, we've always had a read_safe() test
>>> for PV, which means that PV guests have always unilaterally seen #GP. 
>>> Can we retain that behaviour please?
>> Well, that's exactly what I wanted to gather opinions on by having
>> written:
>>
>> "What I'm unsure about is whether we want to ignore such writes also for
>>  PV guests. If not, at least the WRMSR change would need to move into
>>  hvm/hvm.c."
>>
>> I've just sent v2, but I can certainly send v3 with the WRMSR side
>> code moved (to be honest I'm not convinced we want all sorts of
>> is_{hvm,pv}_{domain,vcpu}() checks in guest_{rd,wr}msr(), but if
>> that was your plan, then the code could also stay where it is).
> 
> Yeah - I noticed and replied there.  If you agree with my suggestion
> then there is no need to post a v3.
> 
> I was always expecting to have some pv/hvm split in the
> guest_{rd,wr}msr() side of things, similar to how we do the legacy CPUID
> handling.
> 
> However, I want everything to be implemented in guest_{rd,wr}msr() to
> set the expectation that new additions are symmetric WRT guest types,
> and I hope that the overwhelming majority of code will be based on
> proper architectural information in the CPUID/MSR policies, rather than
> on domain type specifically.

Okay, I'll add conditionals and commit.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-03-15 11:28 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 10:13 [PATCH] x86: ignore guest microcode loading attempts Jan Beulich
2018-03-13 10:36 ` Andrew Cooper
2018-03-13 11:51   ` Jan Beulich
2018-03-15  8:37     ` Ping: " Jan Beulich
2018-03-15  9:43       ` Sergey Dyasli
2018-03-15  9:59 ` Andrew Cooper
2018-03-15 10:40   ` Jan Beulich
2018-03-15 11:03     ` Andrew Cooper
2018-03-15 11:13       ` Jan Beulich
2018-03-15 11:21         ` Andrew Cooper
2018-03-15 11:27           ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.