On 13.10.2020 16:08, Roger Pau Monné wrote:
> On Fri, Oct 02, 2020 at 10:48:07AM +0200, Jan Beulich wrote:
>> On 30.09.2020 12:40, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/vlapic.c
>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>> @@ -459,13 +459,10 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>>>
>>> void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>>> {
>>> - struct vcpu *v = vlapic_vcpu(vlapic);
>>> - struct domain *d = v->domain;
>>> -
>>> if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>>> - vioapic_update_EOI(d, vector);
>>> + vioapic_update_EOI(vector);
>>>
>>> - hvm_dpci_msi_eoi(d, vector);
>>> + hvm_dpci_msi_eoi(vector);
>>> }
>>
>> What about viridian_synic_wrmsr() -> vlapic_EOI_set() ->
>> vlapic_handle_EOI()? You'd probably have noticed this if you
>> had tried to (consistently) drop the respective parameters from
>> the intermediate functions as well.
>>
>> Question of course is in how far viridian_synic_wrmsr() for
>> HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei?
>
> There's already an assert at the top of viridian_synic_wrmsr of v ==
> current, which I assume is why I thought this change was fine. I can
> purge the passing of v (current) further, but it wasn't really needed
> for the rest of the series.
To a large degree that's up to you. It's just that, as said, if
you had done so, you'd likely have noticed the issue, and hence
doing so here and elsewhere may provide reassurance that there's
no further similar case lurking anywhere.
>> A secondary question of course is whether passing around the
>> pointers isn't really cheaper than the obtaining of 'current'.
>
> Well, while there's indeed a performance aspect here, I think
> using current is much clearer than passing a vcpu around. I could
> rename the parameter to curr or some such, but I think using
> current and not passing a vcpu parameter is clearer.
Personally I'd prefer "curr" named function parameters. But if
Andrew or Wei agree with your approach, I'm not going to object.
Jan
