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

[Alison Schofield]

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
>