linux-kernel - Re: [PATCH] arch_topology: Do not set llc_sibling if llc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220412085839.hkpyookqdl6bcjbn@bogus>
Date:   Tue, 12 Apr 2022 09:58:39 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Qing Wang <wangqing@...o.com>, Sudeep Holla <sudeep.holla@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] arch_topology: Do not set llc_sibling if llc_id is
 invalid

On Mon, Apr 11, 2022 at 06:07:45PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2022 at 12:10 PM Qing Wang <wangqing@...o.com> wrote:
> >
> > From: Wang Qing <wangqing@...o.com>
> >
> > When ACPI is not enabled, cpuid_topo->llc_id = cpu_topo->llc_id = -1, which
> > will set llc_sibling 0xff(...), this is misleading.
> 
> Shouldn't it be a Fixes tag then?
>

I thought about that. One the file has moved and lot of refactoring before the
move after the code was first introduced. Since no one has seen any issues as
the mask matches all the CPUs on a single chip SoC and this is not user visible,
I didn't push for the fixes tag.

Anyways the original commit introducing the feature is
Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings")
which was merged in v4.18 if I read git log correctly.

I am happy to backport if needed.

> > Don't set llc_sibling(default 0) if we don't know the cache topology.
> 
> ...
> 
> > -               if (cpuid_topo->llc_id == cpu_topo->llc_id) {
> > +               if (cpu_topo->llc_id != -1 && cpuid_topo->llc_id == cpu_topo->llc_id) {
> 
> I'm wondering if more strict check is better here, i.e.
> 
>                if (cpu_topo->llc_id >= 0 && cpuid_topo->llc_id ==
> cpu_topo->llc_id) {
>

Yes I would agree. But I think Qing is just following other similar checks in
the file. All such ids are initialised to -1 and are assigned only valid
values >= 0. I am OK to keep it as is to keep it aligned with other similar
checks.

-- 
Regards,
Sudeep