Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2021 14:39, Julien Grall wrote:
> On 20/04/2021 13:25, Jan Beulich wrote:
>> On 20.04.2021 14:14, Julien Grall wrote:
>>> It is not really my area of expertise but I wanted to jump on one
>>> comment below...
>>>
>>> On 20/04/2021 12:38, Jan Beulich wrote:
>>>> On 01.04.2020 22:06, Chao Gao wrote:
>>>>> ---
>>>>> Changes in v2:
>>>>>    - verify system suspension and resumption with this patch applied
>>>>>    - don't fall back to register-based interface if enabling qinval 
>>>>> failed.
>>>>>      see the change in init_vtd_hw().
>>>>>    - remove unnecessary "queued_inval_supported" variable
>>>>>    - constify the "struct vtd_iommu *" of 
>>>>> has_register_based_invalidation()
>>>>>    - coding-style changes
>>>>
>>>> ... while this suggests this is v2 of a recently sent patch, the
>>>> submission is dated a little over a year ago. This is confusing.
>>>> It is additionally confusing that there were two copies of it in
>>>> my inbox, despite mails coming from a list normally getting
>>>> de-duplicated somewhere at our end (I believe).
>>>>
>>>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>>>> @@ -1193,6 +1193,14 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>>>>>    
>>>>>        iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
>>>>>        iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG);
>>>>> +    iommu->version = dmar_readl(iommu->reg, DMAR_VER_REG);
>>>>> +
>>>>> +    if ( !iommu_qinval && !has_register_based_invalidation(iommu) )
>>>>> +    {
>>>>> +        printk(XENLOG_WARNING VTDPREFIX "IOMMU %d: cannot disable Queued 
>>>>> Invalidation.\n",
>>>>> +               iommu->index);
>>>>
>>>> Here (and at least once more yet further down): We don't normally end
>>>> log messages with a full stop. Easily addressable while committing, of
>>>> course.
>>>
>>> I can find a large number of cases where log messages are ended with a
>>> full stop... Actually it looks more natural to me than your suggestion.
>>
>> Interesting. From purely a language perspective it indeed looks more
>> natural, I agree. But when considering (serial) console bandwidth, we
>> ought to try to eliminate _any_ output that's there without conveying
>> information or making the conveyed information understandable. In fact
>> I recall a number of cases (without having commit hashes to hand)
>> where we deliberately dropped full stops. (The messages here aren't at
>> risk of causing bandwidth issues, but as with any such generic item I
>> think the goal ought to be consistency, and hence either full stops
>> everywhere, or nowhere. If bandwidth was an issue here, I might also
>> have suggested to shorten "Queued Invalidation" to just "QI".)
> I wasn't aware of such requirement in Xen... Although, I can see how 
> this can be a concern. If you really want to enforce it, then it should 
> be written in the CODING_STYLE.

Agreed, but since I've had no success with prior adjustments to that
file (not even worth a reply to tell me why a change might be a bad
one, in at least some of the cases), I'm afraid I've given up making
attempts to get adjustments into there.

> Alternatively, you could be a bit more 
> verbose in your request so the other understand the reasoning behind it.

Well, yes, perhaps. But then there's the desire to not repeat oneself
all the time.

Jan