From: Keith Busch <keith.busch@intel.com>
To: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
linux-mm@kvack.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Rafael Wysocki <rafael@kernel.org>,
Dave Hansen <dave.hansen@intel.com>,
Dan Williams <dan.j.williams@intel.com>
Subject: Re: [PATCHv5 03/10] acpi/hmat: Parse and report heterogeneous memory
Date: Wed, 6 Feb 2019 09:06:14 -0700 [thread overview]
Message-ID: <20190206160613.GG28064@localhost.localdomain> (raw)
In-Reply-To: <20190206122814.00000127@huawei.com>
On Wed, Feb 06, 2019 at 12:28:14PM +0000, Jonathan Cameron wrote:
> On Thu, 24 Jan 2019 16:07:17 -0700
> Keith Busch <keith.busch@intel.com> wrote:
>
> > + pr_info("HMAT: Locality: Flags:%02x Type:%s Initiator Domains:%d Target Domains:%d Base:%lld\n",
> > + hmat_loc->flags, hmat_data_type(type), ipds, tpds,
> > + hmat_loc->entry_base_unit);
> > +
> > + inits = (u32 *)(hmat_loc + 1);
> > + targs = &inits[ipds];
>
> This line is a bit of an oddity as it's indexing off the end of the data.
> targs = inits + ipds;
> would be nicer to my mind as doesn't even hint that we are in inits still.
>
>
> > + entries = (u16 *)(&targs[tpds]);
Sure, I can change these to addition rather than indexing. I have no
preference either way.
> As above I'd prefer we did the pointer arithmetic explicitly rather
> than used an index off the end of the array.
>
> > + for (init = 0; init < ipds; init++) {
> > + for (targ = 0; targ < tpds; targ++) {
> > + value = entries[init * tpds + targ];
> > + value = (value * hmat_loc->entry_base_unit) / 10;
> > + pr_info(" Initiator-Target[%d-%d]:%d%s\n",
> > + inits[init], targs[targ], value,
> > + hmat_data_type_suffix(type));
>
> Worth checking at this early stage that the domains exist in SRAT?
> + screaming if they don't.
Sure, I think it should be sufficient to check pxm_to_node() for a valid
value to validate the table is okay..
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static __init int hmat_parse_cache(union acpi_subtable_headers *header,
> > + const unsigned long end)
> > +{
> > + struct acpi_hmat_cache *cache = (void *)header;
> > + u32 attrs;
> > +
> > + if (cache->header.length < sizeof(*cache)) {
> > + pr_debug("HMAT: Unexpected cache header length: %d\n",
> > + cache->header.length);
> > + return -EINVAL;
> > + }
> > +
> > + attrs = cache->cache_attributes;
> > + pr_info("HMAT: Cache: Domain:%d Size:%llu Attrs:%08x SMBIOS Handles:%d\n",
> > + cache->memory_PD, cache->cache_size, attrs,
> > + cache->number_of_SMBIOShandles);
>
> Can we sanity check those smbios handles actually match anything?
Will do.
> > +
> > + return 0;
> > +}
> > +
> > +static int __init hmat_parse_address_range(union acpi_subtable_headers *header,
> > + const unsigned long end)
> > +{
> > + struct acpi_hmat_address_range *spa = (void *)header;
> > +
> > + if (spa->header.length != sizeof(*spa)) {
> > + pr_debug("HMAT: Unexpected address range header length: %d\n",
> > + spa->header.length);
>
> My gut feeling is that it's much more useful to make this always print rather
> than debug. Same with other error paths above. Given the number of times
> broken ACPI tables show up, it's nice to complain really loudly!
>
> Perhaps others prefer to not do so though so I'll defer to subsystem norms.
Yeah, I demoted these to debug based on earlier feedback. We should
still be operational even with broken HMAT, so I don't want to create
unnecessary panic if its broken, but I agree something should be
immediately noticable if the firmware tables are incorrect. Maybe like
what bad_srat() provides.
next prev parent reply other threads:[~2019-02-06 16:06 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-24 23:07 [PATCHv5 00/10] Heterogeneuos memory node attributes Keith Busch
2019-01-24 23:07 ` [PATCHv5 01/10] acpi: Create subtable parsing infrastructure Keith Busch
2019-01-24 23:07 ` [PATCHv5 02/10] acpi: Add HMAT to generic parsing tables Keith Busch
2019-01-24 23:07 ` [PATCHv5 03/10] acpi/hmat: Parse and report heterogeneous memory Keith Busch
2019-02-05 12:12 ` Rafael J. Wysocki
2019-02-06 12:28 ` Jonathan Cameron
2019-02-06 16:06 ` Keith Busch [this message]
2019-02-06 16:39 ` Jonathan Cameron
2019-01-24 23:07 ` [PATCHv5 04/10] node: Link memory nodes to their compute nodes Keith Busch
2019-02-05 12:33 ` Rafael J. Wysocki
2019-02-05 14:48 ` Keith Busch
2019-02-05 14:52 ` Greg Kroah-Hartman
2019-02-05 15:17 ` Rafael J. Wysocki
2019-02-06 23:09 ` Keith Busch
2019-02-06 23:48 ` Rafael J. Wysocki
2019-02-06 12:26 ` Jonathan Cameron
2019-02-06 16:12 ` Keith Busch
2019-02-06 16:47 ` Jonathan Cameron
2019-02-07 11:35 ` Rafael J. Wysocki
2019-01-24 23:07 ` [PATCHv5 05/10] acpi/hmat: Register processor domain to its memory Keith Busch
2019-02-06 12:26 ` Jonathan Cameron
2019-01-24 23:07 ` [PATCHv5 06/10] node: Add heterogenous memory access attributes Keith Busch
2019-01-24 23:07 ` [PATCHv5 07/10] acpi/hmat: Register performance attributes Keith Busch
2019-02-06 12:24 ` Jonathan Cameron
2019-01-24 23:07 ` [PATCHv5 08/10] node: Add memory caching attributes Keith Busch
2019-02-06 12:24 ` Jonathan Cameron
2019-01-24 23:07 ` [PATCHv5 09/10] acpi/hmat: Register memory side cache attributes Keith Busch
2019-02-06 12:17 ` Jonathan Cameron
2019-01-24 23:07 ` [PATCHv5 10/10] doc/mm: New documentation for memory performance Keith Busch
2019-02-06 10:45 ` Jonathan Cameron
2019-02-06 16:25 ` Keith Busch
2019-01-28 14:00 ` [PATCHv5 00/10] Heterogeneuos memory node attributes Michal Hocko
2019-02-06 12:31 ` Jonathan Cameron
2019-02-06 17:19 ` Keith Busch
2019-02-06 17:30 ` Jonathan Cameron
2019-02-07 9:53 ` Jonathan Cameron
2019-02-07 15:08 ` Keith Busch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190206160613.GG28064@localhost.localdomain \
--to=keith.busch@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jonathan.cameron@huawei.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).