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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew

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