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

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

On 25/09/14 09:42, Jan Beulich wrote:
>>>> On 24.09.14 at 17:57, <andrew.cooper3@citrix.com> wrote:
>> On 24/09/14 16:34, Jan Beulich wrote:
>>> @@ -877,8 +854,16 @@ static int vlapic_write(struct vcpu *v, 
>>>  
>>>  int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
>>>  {
>>> -    uint32_t val = vlapic_get_reg(vcpu_vlapic(v), offset);
>>> -    return vlapic_reg_write(v, offset, val);
>>> +    struct vlapic *vlapic = vcpu_vlapic(v);
>>> +    uint32_t val = vlapic_get_reg(vlapic, offset);
>>> +
>>> +    if ( !vlapic_x2apic_mode(vlapic) )
>>> +        return vlapic_reg_write(v, offset, val);
>>> +
>>> +    if ( offset != APIC_SELF_IPI )
>>> +        return X86EMUL_UNHANDLEABLE;
>>> +
>>> +    return vlapic_reg_write(v, APIC_ICR, APIC_DEST_SELF | (uint8_t)val);
>> (val & APIC_VECTOR_MASK) instead of a uint8_t cast?
> Ah, one more case I failed to convert.
>
>>> @@ -891,16 +876,69 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>>>  
>>>      switch ( offset )
>>>      {
>>> -        int rc;
>>> +    case APIC_TASKPRI:
>>> +        if ( msr_content & ~APIC_TPRI_MASK )
>>> +            return X86EMUL_UNHANDLEABLE;
>>> +        break;
>>> +
>>> +    case APIC_SPIV:
>>> +        if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
>>> +                             (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
>> Need brackets for the logical (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI)
>> test to avoid making a constant expression with the precedence of the
>> ternary operator.
> Not sure what you mean or what you're thinking here: & has higher
> precedence than even &&, not to speak of ?:, so the expression is
> quite fine without further parentheses. And yes, the whole ?: is
> intended to be a compile time constant, just being prepared for
> directed-EOI support to be added (and as this doesn't seem to need
> any kind of conditional code, I'm expecting that to get added
> directly to the VLAPIC_VERSION #define).
>
> Jan
>

Ah - I had logically inverted the condition I thought I had spotted.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>