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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 December 2018 11:04
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Roger
> Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH] x86/hvm/viridian: stop open coding updates to APIC
> registers
> 
> On 08/12/2018 11:34, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper
> >> Sent: 07 December 2018 18:23
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> Roger
> >> Pau Monne <roger.pau@xxxxxxxxxx>
> >> Subject: Re: [PATCH] x86/hvm/viridian: stop open coding updates to APIC
> >> registers
> >>
> >> 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.
> > No, it's not, but reg_write does do a '& 0xff' which will have the same
> effect.
> 
> Bah - so it is.  (I've lost count of the number of times I've mixed up
> TPR and TASKPRI when grepping)
> 
> >>> 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.
> > Ok, I guess the cosmetic change of s/offset/reg is probably small enough
> to fold in.
> 
> I can fold this on commit if you're happy with the change?

Yes, absolutely. Go ahead. Thanks,

  Paul

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