[PATCH 3/3] virt-host-validate: Detect SMMU presence on ARMs by parsing IORT table

[Michal Prívozník]

On 4/5/23 19:31, Andrea Bolognani wrote:
> On Wed, Apr 05, 2023 at 01:30:19PM +0200, Michal Privoznik wrote:
>> In my previous commit v9.2.0-rc1~3 I've made virt-host-validate
> 
> You're including the commit hash below in the Fixes: pseudo-header,
> so mentioning it here again feels unnecessary. But it's fine if you
> prefer keeping this.
> 
>> to report host IOMMU check pass if IORT table is present. This is
>> not sufficient though, because IORT describes much more than just
>> IOMMU (well, it's called SMMU in ARM world). In fact, this can be
>> seen in previous commit which adds test cases: there are tables
>> (IORT_virt_aarch64) which does not contain any SMMU records.
>>
>> But after previous commits, we can parse the table so switch to
>> that.
>>
>> Fixes: 2c13a2a7c9c368ea81eccd4ba12d9cf34bdd331b
> 
> Personally I've grown to quite like the alternative take on this
> pseudo-header
> 
>   Fixes: 2c13a2a ("virt-host-validate: Detect SMMU support on ARMs")
> 
> as seen very frequently in QEMU and other projects. I feel that it's
> more immediately informative than just the raw hash. But your version
> is perfectly okay too.

Since you're bringing this up, I find the QEMU style horrible. It boils
down to why we include 'Fixes' in the first place. IMO, the part of
commit message above is for humans, and that's why I tend to put output
of: 'git describe $hash' or 'git describe --contains $hash' there
because that gives me (as a human) a hint which version the regression
occurred in (v9.2.0 in this case). But for 'Fixes:' field - my
understanding is that it's for maintainers so that they know what other
commits to backport. And thus it should be as unique as possible. But
these abbreviated hashes are horrible in that sense.

TLDR: the length of abbreviated hashes changes over time

Longer version:
So, when you do 'git log --oneline' and see those abbreviated hashes,
for both libvirt.git and qemu.git they're both shortened to 10
characters. And it makes sense because each repo has relatively small
number of objects in the pack (~42K for libvirt and ~703K for qemu;
obtained via 'git count-objects -v' and looking at 'in-pack' value) and
10 chars are sufficient to guarantee uniqueness. But if you take a look
at a bigger repo, say kernel.git (~9M objects), 10 characters are no
longer sufficient and git needs to use 12 characters. IOW as the
repository grows, abbreviated hashes are less and less unique.

BTw: the code that picks the length for abbreviated hashes can be found
here:

  https://github.com/git/git/blob/master/object-name.c#L762

We can expect length to bump to 11 soon-ish for qemu. Not to mention,
that these calculations set the starting position, lower limit, after
which git still has to walk through all hashes to make sure there's no
conflict using that lower limit. Therefore, even adding one new commit
can cause conflict on say 10 characters.

And if I look at this from a downstream maintainer POV and my (probably
unique) workflow, then whenever I backport a patch, I also bring up
git-log (in less) and do a quick search of the hash '/$hash'. This finds
all 'Fixes: $hash' lines and thus brings my attention to fixes of the
commit I'm about to backport. Using these abbreviated hashes doesn't
work with my workflow (obviously). Now, sure - I can search for just an
abbreviated hash - but how much abbreviated?

Also, what good is the commit subject? I certainly do not remember
commits by their subjects. And thus I need to look at the commit anyway.
For instance, from your example I'd still need to:

  git show $abbrevHash

to understand what the commit did (okay, not in this example, because
I'm author of both commits and wrote them in span of a a couple of days;
but if I were a downstream maintainer you can bet I'd need to do that).

Therefore, using:

  Fixes: $abbrevHash ($commitSubject)

is a horrible idea that looks good on paper but in fact hurts everybody
involved.

> 
> Please include a reference to
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=2178885
> 
> though, since this is finishing the fix for that bug.

Yeah, there's still a case with device tree (i.e. no ACPI tables) so I
was dubious whether to put it there. But you're right, it can't hurt.

> 
> 
> Regardless of whether you decide to follow any of the suggestions
> above,
> 
>   Reviewed-by: Andrea Bolognani <abologna at redhat.com>
> 

Thanks,

Michal