[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi,

On 20/04/2021 13:25, Jan Beulich wrote:
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".)
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. Alternatively, you could be a bit more verbose in your request so the other understand the reasoning behind it.


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.)

42sh> ack "printk\(\"" | ack "\..n\"" | wc -l
130
42sh> ack "printk\(\"" | wc -l
1708

So ~7.5% of the printks.

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.