Re: [Xen-devel] [PATCH] x86/hvm: Disallow moving the APIC MMIO window

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Dec 10, 2018 at 11:45:13AM +0000, Andrew Cooper wrote:
> See the code comment for a full discussion, but in short: guests which
> currently run under Xen don't move the window, because moving it has never
> worked properly.  Implementing support for moving the window is never going to
> work architecturally unless we switch to per-vcpu P2Ms (which seems very
> unlikely), and would still be a substantial quantity of work for a feature
> which is unused in practice.
> 
> Take the opportunity to rename vlapic_msr_set() to be consistent with the
> other MSR handling functions, and return X86EMUL_* constants.  Move the
> guest_{rd,wr}msr_x2apic() declarations into vlapic.h which is a more
> appropriate place for them to live.
> 
> 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>
> ---
>  xen/arch/x86/hvm/hvm.c           |  4 +--
>  xen/arch/x86/hvm/vlapic.c        | 60 
> ++++++++++++++++++++++++++++++++++++----
>  xen/include/asm-x86/hvm/hvm.h    |  3 --
>  xen/include/asm-x86/hvm/vlapic.h |  5 +++-
>  4 files changed, 59 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0039e8c..50fa995 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3565,9 +3565,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t 
> msr_content,
>          break;
>  
>      case MSR_APIC_BASE:
> -        if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
> -            goto gp_fault;
> -        break;
> +        return guest_wrmsr_apic_base(v, msr_content);
>  
>      case MSR_IA32_TSC_DEADLINE:
>          vlapic_tdt_msr_set(vcpu_vlapic(v), msr_content);
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index d3a5fb5..1c72573 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1072,15 +1072,63 @@ static void set_x2apic_id(struct vlapic *vlapic)
>      vlapic_set_reg(vlapic, APIC_LDR, ldr);
>  }
>  
> -bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
> +int guest_wrmsr_apic_base(struct vcpu *v, uint64_t value)
>  {
> -    if ( !has_vlapic(vlapic_domain(vlapic)) )
> -        return 0;
> +    struct vlapic *vlapic = vcpu_vlapic(v);
> +
> +    if ( !has_vlapic(v->domain) )
> +        return X86EMUL_EXCEPTION;
> +
> +    /*
> +     * Architecturally speaking, we should allow a guest to move the xAPIC
> +     * MMIO window (within reason - not even hardware allows arbitrary
> +     * positions).  However, virtualising the behaviour for multi-vcpu guests
> +     * is problematic.
> +     *
> +     * The ability to move the MMIO window was introduced with the Pentium 
> Pro
> +     * processor, to deconflict the window with other MMIO in the system.  
> The
> +     * need to move the MMIO window was obsoleted by the Netburst 
> architecture
> +     * which reserved the space in physical address space for MSIs.
> +     *
> +     * As such, it appears to be a rarely used feature before the turn of the
> +     * millennium, and entirely unused after.
> +     *
> +     * Xen uses a per-domain P2M, but MSR_APIC_BASE is per-vcpu.  In
> +     * principle, we could emulate the MMIO windows being in different
> +     * locations by ensuring that all windows are unmapped in the P2M and 
> trap
> +     * for emulation.  Xen has never had code to modify the P2M in response 
> to
> +     * APIC_BASE updates, so guests which actually try this are likely to end
> +     * up without a working APIC.
> +     *
> +     * Things are more complicated with hardware APIC acceleration, where Xen
> +     * has to map a sink-page into the P2M for APIC accesses to be recognised
> +     * and accelerated by microcode.  Again, this could in principle be
> +     * emulated, but the visible result in the guest would be multiple 
> working
> +     * APIC MMIO windows.  Moving the APIC window has never caused the
> +     * sink-page to move in the P2M, meaning that on all modern hardware, the
> +     * APIC definitely ceases working if the guest tries to move the window.
> +     *
> +     * As such, when the APIC is configured in xAPIC mode, require the MMIO
> +     * window to be in its default location.  We don't expect any guests 
> which
> +     * currently run on Xen to be impacted by this restriction, and the #GP
> +     * fault will be far more obvious to debug than a malfunctioning MMIO
> +     * window.
> +     */
> +    if ( ((value & (APIC_BASE_EXTD | APIC_BASE_ENABLE)) == APIC_BASE_ENABLE) 
> &&
> +         ((value & ~(APIC_BASE_BSP | APIC_BASE_EXTD | APIC_BASE_ENABLE)) !=

You could use MSR_IA32_APICBASE_BASE here AFAICT.

> +          APIC_DEFAULT_PHYS_BASE) )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "%pv attempted to move the APIC MMIO window to 
> 0x%08"PRIx64"\n",
> +                v, value);

I think you should use PAGE_MASK, or else the message is misleading
because you are actually printing the MSR value, which apart from the
address also contains flags in the low 12 bits.

Thanks, Roger.

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