Re: [Xen-devel] [PATCH] x86/hvm/viridian: stop open coding updates to APIC registers

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/12/2018 17:50, Paul Durrant wrote:
> The code in viridian_synic_wrmsr() duplicates logic in vlapic_reg_write()
> to update the ICR, ICR2 and TASKPRI registers. Instead of doing this,
> make vlapic_reg_write() non-static and call it.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/viridian/synic.c | 15 +++++----------
>  xen/arch/x86/hvm/vlapic.c         |  3 +--
>  xen/include/asm-x86/hvm/vlapic.h  |  2 ++
>  3 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/viridian/synic.c 
> b/xen/arch/x86/hvm/viridian/synic.c
> index 845029b568..a6ebbbc9f5 100644
> --- a/xen/arch/x86/hvm/viridian/synic.c
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -84,18 +84,13 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, 
> uint64_t val)
>          vlapic_EOI_set(vcpu_vlapic(v));
>          break;
>  
> -    case HV_X64_MSR_ICR: {
> -        u32 eax = (u32)val, edx = (u32)(val >> 32);
> -        struct vlapic *vlapic = vcpu_vlapic(v);
> -        eax &= ~(1 << 12);
> -        edx &= 0xff000000;
> -        vlapic_set_reg(vlapic, APIC_ICR2, edx);
> -        vlapic_ipi(vlapic, eax, edx);
> -        vlapic_set_reg(vlapic, APIC_ICR, eax);
> +    case HV_X64_MSR_ICR:
> +        vlapic_reg_write(v, APIC_ICR2, val >> 32);
> +        vlapic_reg_write(v, APIC_ICR, val);
>          break;
> -    }
> +
>      case HV_X64_MSR_TPR:
> -        vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI, (uint8_t)val);
> +        vlapic_reg_write(v, APIC_TASKPRI, val);

This uint8_t cast isn't implemented in reg_write.

It is unclear what the behaviour in real hardware is.  The upper bits
are reserved even in xAPIC mode, but the Intel manual isn't clear on
whether they get dropped from writes, or preserved.  The AMD manual
lists them as MBZ, but again is unclear as to whether updates get dropped.

To be on the safe side, I'd recommend implementing the masking internally.

>          break;
>  
>      case HV_X64_MSR_VP_ASSIST_PAGE:
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 4f02499b3b..6f1879d4df 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -769,8 +769,7 @@ static void vlapic_update_timer(struct vlapic *vlapic, 
> uint32_t lvtt,
>      }
>  }
>  
> -static void vlapic_reg_write(struct vcpu *v,
> -                             unsigned int offset, uint32_t val)
> +void vlapic_reg_write(struct vcpu *v, unsigned int offset, uint32_t val)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
>  
> diff --git a/xen/include/asm-x86/hvm/vlapic.h 
> b/xen/include/asm-x86/hvm/vlapic.h
> index 8dbec90ab0..40434afd7b 100644
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -145,4 +145,6 @@ bool_t vlapic_match_dest(
>      const struct vlapic *target, const struct vlapic *source,
>      int short_hand, uint32_t dest, bool_t dest_mode);
>  
> +void vlapic_reg_write(struct vcpu *v, unsigned int offset, uint32_t val);

This export ought to be next to vlapic_{set,set}_reg(), and we should
s/offset/reg/ for consistency with the rest of the code.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel