Re: [PATCH v2 04/11] x86/vmsi: use the newly introduced EOI callbacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2020 16:47, Roger Pau Monné wrote:
> On Fri, Oct 02, 2020 at 05:25:34PM +0200, Jan Beulich wrote:
>> On 30.09.2020 12:41, Roger Pau Monne wrote:
>>> @@ -119,7 +126,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct 
>>> hvm_pirq_dpci *pirq_dpci)
>>>  
>>>      ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI);
>>>  
>>> -    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
>>> +    vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode, 
>>> trig_mode,
>>> +                          hvm_dpci_msi_eoi, NULL);
>>>  }
>>
>> While I agree with your reply to Paul regarding Dom0, I still think
>> the entire if() in hvm_dpci_msi_eoi() should be converted into a
>> conditional here. There's no point registering the callback if it's
>> not going to do anything.
>>
>> However, looking further, the "!hvm_domain_irq(d)->dpci &&
>> !is_hardware_domain(d)" can be simply dropped altogether, right away.
>> It's now fulfilled by the identical check at the top of
>> hvm_dirq_assist(), thus guarding the sole call site of this function.
>>
>> The !is_iommu_enabled(d) is slightly more involved to prove, but it
>> should also be possible to simply drop. What might help here is a
>> separate change to suppress opening of HVM_DPCI_SOFTIRQ when there's
>> no IOMMU in the system, as then it becomes obvious that this part of
>> the condition is guaranteed by hvm_do_IRQ_dpci(), being the only
>> site where the softirq can get raised (apart from the softirq
>> handler itself).
>>
>> To sum up - the call above can probably stay as is, but the callback
>> can be simplified as a result of the change.
> 
> Yes, I agree. Would you be fine with converting the check in the
> callback into an assert, or would you rather have it removed
> completely?

Either way is fine with me, I think.

Jan