Re: [PATCH] cxl/port: Validate decoder nr_targets against supported ways

[<dan.j.williams@intel.com>]

Alison Schofield wrote:
> On Wed, Aug 13, 2025 at 05:57:50PM -0700, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > nr_targets is the number of downstream ports in a decoder's target
> > > list. It is limited to the supported interleave ways: 1, 2, 3, 4, 6,
> > > 8, 12, 16. The existing check only enforces nr_targets <=
> > > CXL_DECODER_MAX_INTERLEAVE (16), allowing invalid values, like 5, 7,
> > > etc, to pass through.
> > > 
> > > Add is_nr_targets_valid() to check for supported interleave ways using
> > > ways_to_eiw(). Use it before allocating a decoder to avoid alloc/free
> > > churn when nr_targets is already known to be invalid.
> > > 
> > > This was found by inspection and hardens the code.
> > 
> > Well, now seeing this and trying to answer the risk question in 1/ led
> > to me to reconsider whether this is a good idea and whether we have a
> > deeper confusion at play:
> > 
> > 1/ "Hardening" needs to come with an actual risk claim. For
> >    cxl_switch_decoder_alloc() the values come from hardware or ACPI and if
> >    those are buggy all bets are off. ACPI is already protected by a
> >    eiw_to_ways() conversion prior to calling cxl_root_decoder_alloc(), and
> >    hardware is even more accurate than this
> >    approach because it considers interleave ways capability.
> 
> If all callers comes thru ACPI then agree no need. When you voiced
> concern over valid host bridge interleave ways, which would be the
> nr_targets of the root decoder (a switch decoder) I imagined you
> were thinking of a path to root decoder alloc outside of ACPI.
> Guess not.

I was looking at the patch in isolation and wondering out loud if we
needed something, but now upon seeing the patch, not so sure.

> > 2/ nr_targets != ways. ways_to_eiw() is the wrong validity check for
> >    switch decoders, nr_targets is a topology limitation, not a
> >    interleave_ways limitation. As far as I can see nothing breaks if a
> >    decoder claims to support a target count that is not a valid
> >    interleave_ways number.
> 
> nr_targets does not have to equal interleave ways for the ways_to_eiw()
> to be a useful helper for validating nr_targets. nr_targets has to be one
> of 1,2,3,4,6,8,12,16 and if it's not ways_to_eiw() fails. I saw it as
> simple code reuse.

See "Target Count" in 8.2.4.20.1 CXL HDM Decoder Capability Register.
The only hardware valid values today for target count are 1, 2, 4, and
8. That is different than ACPI CFMWS which can target 1,2,3,4,6,8,12,16
because CFMWS has a tighter relationship between nr_targets and
interleave_ways. It wants to define regions that target host bridges and
non-host-bridge targets.

The next question is, what if a switch decoder put 5 in its target count
field? Would the driver need to fall over immediately? I think the
answer is no. It would simply be the case that a decoder would set ways
to 1, 2, or 4 and select from the 5 potential targets.

So, I led you astray to make this a wider check beyond the root decoder
special case, and even there the risk of it being a problem in practice.
is low.

> I explored using cxl/test, changing nr_targets at decoder init time,
> and predictable passes if greater than needed, and fails when less.
> But it's not a thorough simulation and not worthy of a more detailed
> simulation.
> 
> Thanks for the review. It was a worthwhile exploration :)

Thanks for the patience to work through it with me! I learned something
about being more careful about the nr_targets:ways relationship.