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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.04.19 at 16:01, <andrew.cooper3@xxxxxxxxxx> wrote:
> There are a number of bugs.  There are no read/write hooks on the HVM side, so
> guest accesses fall into the "read/write-discard" defaults, which bypass the
> correct faulting behaviour and the Intel special case.
> 
> For the PV side, writes are discarded (again, bypassing proper faulting),
> except for a pinned dom0, which is permitted to actually write the values
> other than 0.  This is pointless with read hook implementing the Intel special
> case.
> 
> However, implementing the Intel special case is pointless.  First of all, OS
> software can't guarentee to read back 0 in the first place, because a) this
> behaviour isn't guarenteed in the SDM, and b) there are SMM handlers which use
> the CPUID instruction.

I didn't think there was ever an intention to be able to (reliably)
hand back zero on a subsequent read.

>  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.

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).

> 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>

> 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.

Jan


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