linux-kernel - Re: [PATCH v2 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210616232140.GC25185@alison-desk.jf.intel.com>
Date:   Wed, 16 Jun 2021 16:21:40 -0700
From:   Alison Schofield <alison.schofield@...el.com>
To:     Ben Widawsky <ben.widawsky@...el.com>
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linux ACPI <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] cxl/acpi: Use the ACPI CFMWS to create static
 decoder objects


Thanks for the review Ben -

On Wed, Jun 16, 2021 at 09:17:40AM -0700, Ben Widawsky wrote:
> On 21-06-15 17:20:39, Alison Schofield wrote:

snip

> > +static unsigned long cfmws_to_decoder_flags(int restrictions)
> > +{
> > +	unsigned long flags = 0;
> > +
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
> > +		flags |= CXL_DECODER_F_TYPE2;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
> > +		flags |= CXL_DECODER_F_TYPE3;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
> > +		flags |= CXL_DECODER_F_RAM;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
> > +		flags |= CXL_DECODER_F_PMEM;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
> > +		flags |= CXL_DECODER_F_LOCK;
> > +
> > +	return flags;
> > +}
> 
> I know these flags aren't introduced by this patch, but I'm wondering if it
> makes sense to not just use the spec definitions rather than defining our own.
> It doesn't do much harm, but it's extra typing everytime the spec adds new flags
> and I don't really see the upside.
> 

I think Dan's email in this thread covered this.

snip
> > +
> > +static int cxl_acpi_cfmws_verify(struct device *dev,
> > +				 struct acpi_cedt_cfmws *cfmws)
> > +{

snip

> > +
> > +
> > +	expected_len = struct_size((cfmws), interleave_targets,
> > +				   CFMWS_INTERLEAVE_WAYS(cfmws));
> > +
> > +	if (expected_len != cfmws->header.length) {
> 
> I'd switch this to:
> if (expected_len < cfmws->header.length)
> 
> If it's too big, just print a dev_dbg.
> 

Got it. 

snip

> > +	void *cedt_base;
> > +	int rc;
> > +
> > +	len = cedt_table->length - sizeof(*cedt_table);
> > +	cedt_base = cedt_table + 1;
> 
> naming suggestions per previous patch... up to you though.
>

Ditto w previous patch.

snip
> > +
> > +		}
> > +
> > +		cxld = devm_cxl_add_decoder(dev, root_port,
> > +				CFMWS_INTERLEAVE_WAYS(cfmws),
> > +				cfmws->base_hpa, cfmws->window_size,
> > +				CFMWS_INTERLEAVE_WAYS(cfmws),
> 
> Interesting... this made me question, how can we have a different number of
> targets and ways?
> 

Dan explained this previously:

"nr_targets is the number of possible targets that this decoder can
target. For CFMWS it just equals interleave_ways because the target
list can't be changed. A switch on the other hand could support up to
16 possible targets, but be dynamically configured to only do a 1-way
interleave. So this is an artifact of 'struct cxl_decoder'
representing both fixed CFMWS entries and dynamically programmable
switch entries. nr_targets tells devm_cxl_add_decoder() how much
memory to allocate for its target list, interleave_ways tells
devm_cxl_add_decoder() what the decoder is currently programmed to
decode."


> 
snip
>