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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

~Andrew

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