From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/4] x86/HVM: replace plain numbers Date: Thu, 22 Jan 2015 14:41:31 +0000 Message-ID: <54C10C1B.4090801@citrix.com> References: <54C10EE1020000780005827E@mail.emea.novell.com> <54C10FEF0200007800058295@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YEIx1-0000LO-10 for xen-devel@lists.xenproject.org; Thu, 22 Jan 2015 14:41:39 +0000 In-Reply-To: <54C10FEF0200007800058295@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Keir Fraser List-Id: xen-devel@lists.xenproject.org On 22/01/15 13:57, Jan Beulich wrote: > ... making the code better document itself. No functional change > intended. > > Signed-off-by: Jan Beulich > > --- 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; > > >