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

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

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.

~Andrew

>      case VIOAPIC_REG_ARB_ID:
> -        result = ((vioapic->id & 0xf) << 24);
> +        result = ((union IO_APIC_reg_02){
> +                  .bits = { .arbitration = vioapic->id }
> +                  }).raw;
>          break;
>  
>      default:
>      {
> -        uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
> +        uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
>          uint64_t redir_content;
>  
>          if ( redir_index >= VIOAPIC_NUM_PINS )
> @@ -173,7 +181,11 @@ static void vioapic_write_indirect(
>          break;
>  
>      case VIOAPIC_REG_APIC_ID:
> -        vioapic->id = (val >> 24) & 0xf;
> +        /*
> +         * Using union IO_APIC_reg_02 for the ID register, as for some
> +         * reason union IO_APIC_reg_00's ID field is 8 bits wide.
> +         */
> +        vioapic->id = ((union IO_APIC_reg_02){ .raw = val }).bits.arbitration;
>          break;
>  
>      case VIOAPIC_REG_ARB_ID:
> @@ -181,7 +193,7 @@ static void vioapic_write_indirect(
>  
>      default:
>      {
> -        uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
> +        uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
>  
>          HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "rte[%02x].%s = %08x",
>                      redir_index, vioapic->ioregsel & 1 ? "hi" : "lo", val);
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -835,7 +835,7 @@ static int vlapic_write(struct vcpu *v, 
>      unsigned int offset = address - vlapic_base_address(vlapic);
>      int rc = X86EMUL_OKAY;
>  
> -    if ( offset != 0xb0 )
> +    if ( offset != APIC_EOI )
>          HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
>                      "offset %#x with length %#lx, and value is %#lx",
>                      offset, len, val);
> --- a/xen/include/asm-x86/hvm/vioapic.h
> +++ b/xen/include/asm-x86/hvm/vioapic.h
> @@ -47,6 +47,7 @@
>  #define VIOAPIC_REG_APIC_ID 0x00 /* x86 IOAPIC only */
>  #define VIOAPIC_REG_VERSION 0x01
>  #define VIOAPIC_REG_ARB_ID  0x02 /* x86 IOAPIC only */
> +#define VIOAPIC_REG_RTE0    0x10
>  
>  struct hvm_vioapic {
>      struct hvm_hw_vioapic hvm_hw_vioapic;
>
>
>