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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew


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