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

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

On Mon, Dec 10, 2018 at 04:20:07PM +0000, Andrew Cooper wrote:
> On 10/12/2018 16:14, Roger Pau Monné wrote:
> > 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.
> 
> That no longer exists in staging, but either way, that would be buggy. 
> I suppose I should split the actually-reserved bit checking out into an
> earlier check.  Nothing at the moment prevents the guest from setting
> the reserved bits.

Oh, I was looking at an old version of the tree, that's
APIC_BASE_ADDR_MASK now.

Moving the reserved bit checking vs the MMIO window movement would be
better.

> >
> >> +          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.
> 
> Printing the lower metadata is important for diagnostics, even if only
> to double check the logic leading up to the printk().  (Also, I note it
> should be XENLOG_G_INFO)

I agree, but I think the message is kind of misleading because you are
not printing an address but the attempted MSR write value, that was
the point I was trying to make with my comment. I would reword to
"attempted to move the APIC MMIO window, MSR write: ..." or something
similar.

Thanks, Roger.

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