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

[Julien Grall <julien@xxxxxxx>]

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