Re: [PATCH RFC] x86/HVM: fix various aspects of x2APIC emulation

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

[-- Attachment #1.1: Type: text/plain, Size: 9623 bytes --]

On 09/09/14 16:51, Jan Beulich wrote:
> - properly change ID when switching into x2APIC mode (instead of
>   mimicking necessary behavior in hvm_x2apic_msr_read())
> - correctly (meaningfully) set LDR (so far it ended up being 1 on all
>   vCPU-s)
> - fail reads from the self-IPI register (which is write-only)
> - handle self-IPI writes and the ICR2 half of ICR writes largely in
>   hvm_x2apic_msr_write()
> - even if we don't support more than 128 vCPU-s in a HVM guest for now,
>   we should properly handle IDs as 32-bit values (i.e. not ignore the
>   top 24 bits)
> - with that, properly do cluster ID and bit mask check in
>   vlapic_match_logical_addr()
> - slightly adjust other parameter types of vlapic_match_dest() and
>   vlapic_lowest_prio() (and related local variable ones)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC reason: The previously broken APIC ID handling would seem to cause
> problems on migration; so far I didn't have a good idea how to
> transparently and correctly convert the bogus old ID (and derived LDR)
> to the correct one. Since for a single vCPU guest this doesn't matter
> (the only vCPU has ID zero), perhaps checking for LDR being wrongly 1
> on all vCPU-s might be a usable approach. I further hope that dealing
> with migration _to_ an old, broken hypervisor isn't going to be
> necessary.

Hmm - This could explain our apicv bugs with migration.  The symptoms
were that migrating a VM between two identical pieces of hardware, one
with apicv enabled and one without resulted in a loss of interrupts
being delivered to the VM.  The VMs in question were using x2apic, and
this mixup with APIC IDs sounds very plausible.  Once I have got libxl
migrationv2 posted, I will reinvestigate the apicv issue with this patch
applied.

Having said all of that, I too am very concerned about the migration
implication.  This is certainly something which we absolutely don't want
to break.

In the past, it has been suggested that doing compatibility fixing like
this in the toolstack is far better than in Xen.  A toolstack is in a
better position to know that a migration is occurring from an older
version of xen to a newer version, and across this difference, LDR needs
fixing up in the lapic state.  Furthermore, an ability like this can be
used in reverse to safely migrate backwards, a requested feature of
XenServer incase a partial upgrade needs to be stopped or undone.

Is it possible to split this patch between the safe cleanup and the
unsafe cleanup?

~Andrew

>
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -173,18 +173,17 @@ uint32_t vlapic_set_ppr(struct vlapic *v
>     return ppr;
>  }
>  
> -static int vlapic_match_logical_addr(struct vlapic *vlapic, uint8_t mda)
> +static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
>  {
>      int result = 0;
> -    uint32_t logical_id;
> +    uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
>  
>      if ( vlapic_x2apic_mode(vlapic) )
> -    {
> -        logical_id = vlapic_get_reg(vlapic, APIC_LDR);
> -        return !!(logical_id & mda);
> -    }
> +        return ((logical_id >> 16) == (mda >> 16)) &&
> +               (uint16_t)(logical_id & mda);
>  
> -    logical_id = GET_xAPIC_LOGICAL_ID(vlapic_get_reg(vlapic, APIC_LDR));
> +    logical_id = GET_xAPIC_LOGICAL_ID(logical_id);
> +    mda = (uint8_t)mda;
>  
>      switch ( vlapic_get_reg(vlapic, APIC_DFR) )
>      {
> @@ -207,8 +206,8 @@ static int vlapic_match_logical_addr(str
>  }
>  
>  bool_t vlapic_match_dest(
> -    struct vlapic *target, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode)
> +    struct vlapic *target, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode)
>  {
>      HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "target %p, source %p, dest %#x, "
>                  "dest_mode %#x, short_hand %#x",
> @@ -219,7 +218,8 @@ bool_t vlapic_match_dest(
>      case APIC_DEST_NOSHORT:
>          if ( dest_mode )
>              return vlapic_match_logical_addr(target, dest);
> -        return ((dest == 0xFF) || (dest == VLAPIC_ID(target)));
> +        return (dest == _VLAPIC_ID(target, 0xffffffff)) ||
> +               (dest == VLAPIC_ID(target));
>  
>      case APIC_DEST_SELF:
>          return (target == source);
> @@ -286,7 +286,7 @@ static void vlapic_init_sipi_action(unsi
>      uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
>      uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
>      uint32_t short_hand = icr & APIC_SHORT_MASK;
> -    uint32_t dest_mode  = !!(icr & APIC_DEST_MASK);
> +    bool_t dest_mode  = !!(icr & APIC_DEST_MASK);
>      struct vcpu *v;
>  
>      if ( icr == 0 )
> @@ -352,8 +352,8 @@ static void vlapic_accept_irq(struct vcp
>  }
>  
>  struct vlapic *vlapic_lowest_prio(
> -    struct domain *d, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode)
> +    struct domain *d, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode)
>  {
>      int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
>      uint32_t ppr, target_ppr = UINT_MAX;
> @@ -414,13 +414,11 @@ void vlapic_ipi(
>  {
>      unsigned int dest;
>      unsigned int short_hand = icr_low & APIC_SHORT_MASK;
> -    unsigned int dest_mode  = !!(icr_low & APIC_DEST_MASK);
> +    bool_t dest_mode = !!(icr_low & APIC_DEST_MASK);
>  
>      HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "icr = 0x%08x:%08x", icr_high, icr_low);
>  
> -    dest = (vlapic_x2apic_mode(vlapic)
> -            ? icr_high
> -            : GET_xAPIC_DEST_FIELD(icr_high));
> +    dest = _VLAPIC_ID(vlapic, icr_high);
>  
>      switch ( icr_low & APIC_MODE_MASK )
>      {
> @@ -611,15 +609,12 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
>      vlapic_read_aligned(vlapic, offset, &low);
>      switch ( offset )
>      {
> -    case APIC_ID:
> -        low = GET_xAPIC_ID(low);
> -        break;
> -
>      case APIC_ICR:
>          vlapic_read_aligned(vlapic, APIC_ICR2, &high);
>          break;
>  
>      case APIC_ICR2:
> +    case APIC_SELF_IPI:
>          return 1;
>      }
>  
> @@ -710,9 +705,7 @@ static int vlapic_reg_write(struct vcpu 
>          break;
>  
>      case APIC_SELF_IPI:
> -        rc = vlapic_x2apic_mode(vlapic)
> -            ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
> -            : X86EMUL_UNHANDLEABLE;
> +        rc = X86EMUL_UNHANDLEABLE;
>          break;
>  
>      case APIC_ICR:
> @@ -722,9 +715,7 @@ static int vlapic_reg_write(struct vcpu 
>          break;
>  
>      case APIC_ICR2:
> -        if ( !vlapic_x2apic_mode(vlapic) )
> -            val &= 0xff000000;
> -        vlapic_set_reg(vlapic, APIC_ICR2, val);
> +        vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000);
>          break;
>  
>      case APIC_LVTT:         /* LVT Timer Reg */
> @@ -883,16 +874,17 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>  
>      switch ( offset )
>      {
> -        int rc;
> -
>      case APIC_ICR:
> -        rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
> -        if ( rc )
> -            return rc;
> +        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
>          break;
>  
>      case APIC_ICR2:
>          return X86EMUL_UNHANDLEABLE;
> +
> +    case APIC_SELF_IPI:
> +        offset = APIC_ICR;
> +        msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
> +        break;
>      }
>  
>      return vlapic_reg_write(v, offset, (uint32_t)msr_content);
> @@ -932,8 +924,10 @@ void vlapic_msr_set(struct vlapic *vlapi
>  
>      if ( vlapic_x2apic_mode(vlapic) )
>      {
> -        u32 id = vlapic_get_reg(vlapic, APIC_ID);
> -        u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
> +        u32 id = vlapic_vcpu(vlapic)->vcpu_id;
> +        u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
> +
> +        vlapic_set_reg(vlapic, APIC_ID, id * 2);
>          vlapic_set_reg(vlapic, APIC_LDR, ldr);
>      }
>  
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -30,8 +30,9 @@
>  #define vlapic_vcpu(x)   (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
>  #define vlapic_domain(x) (vlapic_vcpu(x)->domain)
>
> -#define VLAPIC_ID(vlapic)   \
> -    (GET_xAPIC_ID(vlapic_get_reg((vlapic), APIC_ID)))
> +#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
> +                                ? (id) : GET_xAPIC_ID(id))
> +#define VLAPIC_ID(vlapic) _VLAPIC_ID(vlapic, vlapic_get_reg(vlapic, APIC_ID))
>  
>  /*
>   * APIC can be disabled in two ways:
> @@ -123,11 +124,11 @@ void vlapic_ipi(struct vlapic *vlapic, u
>  int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
>  
>  struct vlapic *vlapic_lowest_prio(
> -    struct domain *d, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode);
> +    struct domain *d, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode);
>  
>  bool_t vlapic_match_dest(
> -    struct vlapic *target, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode);
> +    struct vlapic *target, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode);
>  
>  #endif /* __ASM_X86_HVM_VLAPIC_H__ */
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 10087 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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