On 08/12/2018 11:34, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 07 December 2018 18:23
>> To: Paul Durrant <[email protected]>; [email protected]
>> Cc: Jan Beulich <[email protected]>; Wei Liu <[email protected]>; Roger
>> Pau Monne <[email protected]>
>> 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 <[email protected]>
>>> ---
>>> Cc: Jan Beulich <[email protected]>
>>> Cc: Andrew Cooper <[email protected]>
>>> Cc: Wei Liu <[email protected]>
>>> Cc: "Roger Pau Monné" <[email protected]>
>>> ---
>>> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
