On Wed, Feb 23, 2022 at 2:31 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > On 22-02-23 14:24:00, Dan Williams wrote: > > On Wed, Feb 23, 2022 at 1:50 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > > > > > On 22-02-17 11:57:59, Dan Williams wrote: > > > > On Thu, Feb 17, 2022 at 10:36 AM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > > > > > > > > > Consolidating earlier discussions... > > > > > > > > > > On 22-01-28 16:25:34, Dan Williams wrote: > > > > > > On Thu, Jan 27, 2022 at 4:27 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > > > > > > > > > > > > > The region creation APIs create a vacant region. Configuring the region > > > > > > > works in the same way as similar subsystems such as devdax. Sysfs attrs > > > > > > > will be provided to allow userspace to configure the region. Finally > > > > > > > once all configuration is complete, userspace may activate the region. > > > > > > > > > > > > > > Introduced here are the most basic attributes needed to configure a > > > > > > > region. Details of these attribute are described in the ABI > > > > > > > > > > > > s/attribute/attributes/ > > > > > > > > > > > > > Documentation. Sanity checking of configuration parameters are done at > > > > > > > region binding time. This consolidates all such logic in one place, > > > > > > > rather than being strewn across multiple places. > > > > > > > > > > > > I think that's too late for some of the validation. The complex > > > > > > validation that the region driver does throughout the topology is > > > > > > different from the basic input validation that can be done at the > > > > > > sysfs write time. For example ,this patch allows negative > > > > > > interleave_granularity values to specified, just return -EINVAL. I > > > > > > agree that sysfs should not validate everything, I disagree with > > > > > > pushing all validation to cxl_region_probe(). > > > > > > > > > > > > > > > > Okay. It might save us some back and forth if you could outline everything you'd > > > > > expect to be validated, but I can also make an attempt to figure out the > > > > > reasonable set of things. > > > > > > > > Input validation. Every value that gets written to a sysfs attribute > > > > should be checked for validity, more below: > > > > > > > > > > > > > > > > > > > > > > > A example is provided below: > > > > > > > > > > > > > > /sys/bus/cxl/devices/region0.0:0 > > > > > > > ├── interleave_granularity > > > > > > > > ...validate granularity is within spec and can be supported by the root decoder. > > > > > > > > > > > ├── interleave_ways > > > > > > > > ...validate ways is within spec and can be supported by the root decoder. > > > > > > I'm not sure how to do this one. Validation requires device positions and we > > > can't set the targets until ways is set. Can you please provide some more > > > insight on what you'd like me to check in addition to the value being within > > > spec? > > > > For example you could check that interleave_ways is >= to the root > > level interleave. I.e. it would be invalid to attempt a x1 interleave > > on a decoder that is x2 interleaved at the host-bridge level. > > I tried to convince myself that that assertion always holds and didn't feel > super comfortable. If you do, I can add those kinds of checks. The only way to support a x1 region on an x2 interleave is to have the size be equal to interleave granularity so that accesses stay contained to that one device. In fact that's another validation step, which you might already have, region size must be >= and aligned to interleave_granularity * interleave_ways.
