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: 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.

> 
> 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.

As I said, it's already there :-)

> 
> >          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.

Ok, I guess the cosmetic change of s/offset/reg is probably small enough to 
fold in.

  Paul

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