Re: [PATCH v4 1/4] x86/HVM: fix miscellaneous aspects of x2APIC emulation

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/09/14 14:40, Jan Beulich wrote:
>>>> On 22.09.14 at 15:14, <andrew.cooper3@citrix.com> wrote:
>> On 18/09/14 15:43, Jan Beulich wrote:
>>> @@ -891,16 +876,33 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>>>  
>>>      switch ( offset )
>>>      {
>>> -        int rc;
>>> +    case APIC_TASKPRI:
>>> +    case APIC_EOI:
>>> +    case APIC_SPIV:
>>> +    case APIC_CMCI:
>>> +    case APIC_LVTT ... APIC_LVTERR:
>>> +    case APIC_TMICT:
>>> +    case APIC_TMCCT:
>>> +    case APIC_TDCR:
>> Most (all?) of these MSRs have reserved bits, which should fail with a
>> #GP(0) for attempts to set. vlapic_reg_read() masks most of the relevant
>> bits, but doesn't appear to hit a misbehaving VM.
> Perhaps, but not in this patch.

Does it not count towards fixing misc aspects of x2APIC emulation?

>
>>> +        break;
>>>  
>>>      case APIC_ICR:
>>> -        rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
>>> -        if ( rc )
>>> -            return rc;
>>> +        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
>>>          break;
>>>  
>>> -    case APIC_ICR2:
>>> -        return X86EMUL_UNHANDLEABLE;
>>> +    case APIC_SELF_IPI:
>>> +        offset = APIC_ICR;
>>> +        msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
>>> +        break;
>>> +
>>> +    case APIC_ESR:
>>> +        if ( msr_content )
>>> +        {
>>> +            printk(XENLOG_G_WARNING "%pv: non-zero (%lx) LAPIC ESR write\n",
>>> +                   v, msr_content);
>> I know this is just moving an existing error message, but is it actually
>> useful? ESR is no more special than the other registers with some/all
>> bits reserved.
> It is different in that legacy APIC mode allows these to be written.
> But yes, I wondered about the usefulness of this message too.

Even for legacy APIC, the value written is discarded by hardware.  I
would just drop the message.

>
>>> @@ -910,7 +912,10 @@ static int vlapic_range(struct vcpu *v, 
>>>  {
>>>      struct vlapic *vlapic = vcpu_vlapic(v);
>>>      unsigned long offset  = addr - vlapic_base_address(vlapic);
>>> -    return (!vlapic_hw_disabled(vlapic) && (offset < PAGE_SIZE));
>>> +
>>> +    return !vlapic_hw_disabled(vlapic) &&
>>> +           !vlapic_x2apic_mode(vlapic) &&
>>> +           (offset < PAGE_SIZE);
>> This check is too restrictive, at least on Intel. From SDM Vol 3 29.4.3.3
>>
>> "As noted in Section 29.5, execution of WRMSR with ECX = 83FH (self-IPI
>> MSR) can lead to an APIC-write VM exit
>> if the “virtual-interrupt delivery” VM-execution control is 1. The exit
>> qualification for such an APIC-write VM exit is
>> 3F0H."
>>
>> So we can still end up wandering the vlapic MMIO codepaths even in
>> x2apic mode.
> Not exactly: These accesses would arrive at vlapic_apicv_write()
> (where APIC_SELF_IPI is being handled even for the x2APIC case)
> and hence won't go the normal MMIO path (including vlapic_range()).

Ah yes, which bypasses the vlapic_range() test.

In which case, this change appears ok.

~Andrew


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel