Re: [PATCH 2/4] x86/HVM: replace plain numbers

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

On 23/01/15 13:58, Jan Beulich wrote:
>>>> On 23.01.15 at 14:41, <andrew.cooper3@citrix.com> wrote:
>> On 22/01/15 15:17, Jan Beulich wrote:
>>>>>> On 22.01.15 at 15:41, <andrew.cooper3@citrix.com> wrote:
>>>> On 22/01/15 13:57, Jan Beulich wrote:
>>>>> ... making the code better document itself. No functional change
>>>>> intended.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> --- a/xen/arch/x86/hvm/vioapic.c
>>>>> +++ b/xen/arch/x86/hvm/vioapic.c
>>>>> @@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
>>>>>      switch ( vioapic->ioregsel )
>>>>>      {
>>>>>      case VIOAPIC_REG_VERSION:
>>>>> -        result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
>>>>> -                  | (VIOAPIC_VERSION_ID & 0xff));
>>>>> +        result = ((union IO_APIC_reg_01){
>>>>> +                  .bits = { .version = VIOAPIC_VERSION_ID,
>>>>> +                            .entries = VIOAPIC_NUM_PINS - 1 }
>>>>> +                  }).raw;
>>>>>          break;
>>>>>  
>>>>>      case VIOAPIC_REG_APIC_ID:
>>>>> +        /*
>>>>> +         * Using union IO_APIC_reg_02 for the ID register too, as
>>>>> +         * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
>>>>> +         */
>>>> Having looked into this, Intel has a 4 bit wide ID with the top 4 bits
>>>> reserved, while AMD has the top 4 bits as the Extended ID which may be
>>>> used if an appropriate northbridge register has been set.
>>>>
>>>> I think it might be better to use IO_APIC_reg_00 here and mask the write
>>>> operations, leaving a note about Intel vs AMD and the fact that emulate
>>>> an Intel IOAPIC to match the PIIX3 chipset in Qemu.
>>>>
>>>> Modifying IO_APIC_reg_00 is not appropriate as Xens IOAPIC code needs to
>>>> deal with AMD systems as well.
>>> I had it that way first, but for the purpose of making very clear that
>>> there is no functional change, I decided against doing such a conversion.
>> Ok, but please adjust the comment.
>>
>> "for some reason" is not helpful, whereas "because we emulate an Intel
>> IOAPIC which only has a 4 bit ID field, compared to 8 for AMD" would be
>> better.
> When I wrote the comment, I didn't have a clue why this was
> inconsistent. Now we have at least a guess. I'll therefore use
> your wording with "because" preceded by "presumably":
>
> +        /*
> +         * Presumably because we emulate an Intel IOAPIC which only has a
> +         * 4 bit ID field (compared to 8 for AMD), using union IO_APIC_reg_02
> +         * for the ID register (union IO_APIC_reg_00's ID field is 8 bits).
> +         */

Looks much better.

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