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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.15 at 14:41, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/01/15 15:17, Jan Beulich wrote:
>>>>> On 22.01.15 at 15:41, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>
>>>>
>>>> --- 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).
+         */

Jan


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