Re: [Xen-devel] [PATCH] x86/msr: Fix handling of MSR_AMD_PATCHLEVEL/MSR_IA32_UCODE_REV

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/04/2019 15:55, Jan Beulich wrote:
>
>>  Secondly, when a guest executes CPUID, this doesn't
>> typically result in Xen executing a CPUID instruction in practice.
> Wait - this is true for DomU, but used to be false for (PV) Dom0,
> until you've switched over from pv_cpuid() to guest_cpuid(). I
> would assume it was an unintended side effect to, this way,
> eliminate the actual CPUID access from the special ucode rev
> reading sequence Dom0 may perform.

It certainly was an unintended side effect of the cpuid_policy work.  In
practice, dom0 still doesn't have faulting activated (due to the domain
builder not having switched to DOMCTL_set_cpu_policy yet), so can still
execute real CPUID instructions.

This is not going to stay true for very much longer.

> As to DomU, I severely doubt it's a good idea to expose the
> ucode revision to a guest, at least as long as the guest can be
> migrated. But yes, we can leave avoiding to do so to the guest
> itself: We won't mislead it any worse by handing it the real
> revision than we would by handing it some synthetized one (e.g.
> constant 0).

All guests in practice read the current microcode version.  As it is
model specific, we can't #GP on read, and faking up 0 is the only other
option.

Furthermore, the speculative sidechannels mean that guests in practice
do need to consult the microcode version for cases where MSR_ARCH_CAPS
isn't provided.  On Broadwell at least, it is a non-optional part of
evaluating the safety of retpoline.

We have always made the microcode version available, and even the
visibility of differences across migrate.  It would be better if guests
ignored microcode entirely if they found themselves virtualised, but I
doubt that is going to happen.

>> With the dom0 special case removed, there are now no writes to this MSR other
>> than Xen's microcode loading facilities, which means that the value held in
>> the MSR will be properly up-to-date.  Forward it directly, without jumping
>> through any hoops.
> I agree with this, so
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks, but I'll wait to see if we get a Shanghai answer in a reasonable
period of time.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: David Wang <davidwang@xxxxxxxxxxx>
>>
>> This patch texturally (but not functionally) interacts with "xen/sched: 
>> Remove
>> d->is_pinned" but rebasing either is easy.  It would also be best applied 
>> with
>> Sergey's "x86/microcode: always collect_cpu_info() during boot".
> Well, "best" is probably an understatement, as you'd supply
> stale data if we never came through that code. So I'd call it a
> strict prereq.

Hmm - I've missed part of the explanation.

a) This behaviour is what happens currently for HVM guests.

b) In practice, it is only the very original P6 with no microcode which
may have a fully read/write MSR here without anything to update the
version.  AFACIT, for all CPUs which Xen will boot on, all that matters
is that a CPUID instruction has been executed previously, which is
covered by the AP boot path.

i.e. I think the logic would be more consistent to follow if this patch
was behind Sergey's, but there is no functional dependency.

~Andrew

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