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

["Jan Beulich" <JBeulich@suse.com>]

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

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

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

Jan

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