Re: [PATCH v3 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2021 09:41, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 01:06:35PM +0200, Jan Beulich wrote:
>> On 31.03.2021 12:32, Roger Pau Monne wrote:
>>> EOIs are always executed in guest vCPU context, so there's no reason to
>>> pass a vCPU parameter around as can be fetched from current.
>>
>> While not overly problematic, I'd like to point out that there's not a
>> single vcpu parameter being dropped here - in both cases it's struct
>> domain *.
>>
>>> --- 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);
>>>  }
>>
>> The Viridian path pointed out before was only an example. I'm afraid
>> the call from vlapic_has_pending_irq() to vlapic_EOI_set() is also
>> far from obvious that it always has "v == current". What we end up
>> with here is a mix of passed in value (vlapic) and assumption of the
>> call being for the vCPU / domain we're running on. At the very least
>> I think this would want documenting here in some way (maybe ASSERT(),
>> definitely mentioning in the description), but even better would
>> perhaps be if the parameter of the function here as well as further
>> ones involved would also be dropped then.
> 
> I've kind of attempted to purge the vlapic parameter further, but the
> proper way to do it would be to audit all vlapic functions.
> 
> For example I've removed the parameter from vlapic_EOI_set and
> vlapic_handle_EOI, but I'm afraid that would also raise questions
> about purging it vlapic_has_pending_irq for example.
> 
> Let me know if the patch below would be acceptable, or if I should
> rather not make the EOI callbacks depends on this cleanup, as I could
> certainly do the cleanup later.

While I'm not opposed in principle, the patch moves us further away
from what Andrew has asked for (to retain the vcpu pointers), if I
understood him correctly. I'm also not entirely certain if there
couldn't be, down the road, emulators needing to signal an EOI to
Xen on behalf of a guest.

Jan