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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2021 14:14, Julien Grall wrote:
> Hi,
> 
> 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".)

Also, when you say "large number" - did you compare to the number of
cases where there are no full stops? (I sincerely hope we don't have
that many full stops left.)

Jan