Re: [PATCH 5/5] cxl/region: Constrain region granularity scaling factor

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Fri, 2022-07-22 at 17:56 -0700, Dan Williams wrote:
> Consider the scenario where a platform provides for a x2 host-bridge
> interleave at a granularity of 256 bytes. The only permitted region
> granularities for that configuration are 256 or 512. Anything larger
> than 512 results in unmapped capacity within a given decoder. Also, if
> the region granularity is 512 then the interleave_ways for the region
> must be 4 to keep the scaling matched.
> 
> Here are the translations for the first (4) 256-byte blocks where an
> endpoint decoder is configured for a 512-byte granularity:
> 
> Block[0] => HB0 => DPA: 0
> Block[1] => HB1 => DPA: 0
> Block[2] => HB0 => DPA: 0
> Block[3] => HB1 => DPA: 0
> 
> In order for those translations to not alias the region interleave ways
> must be 4 resulting in:
> 
> Block[0] => HB0 => Dev0 => DPA: 0
> Block[1] => HB1 => Dev1 => DPA: 0
> Block[2] => HB0 => Dev2 => DPA: 0
> Block[3] => HB1 => Dev3 => DPA: 0
> 
> ...not 2, resulting in:
> 
> Block[0] => HB0 => Dev0 => DPA: 0
> Block[1] => HB1 => Dev1 => DPA: 0
> Block[2] => HB0 => Dev0 => DPA: 0 !
> Block[3] => HB1 => Dev1 => DPA: 0 !
> 
> Given tooling is already being built around this ABI allow for
> granularity and ways to be set in either order and validate the
> combination once both are set.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |   63 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 3 deletions(-)

Looks good, 

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 05b6212e6399..a34e537e4cb2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -348,6 +348,25 @@ static ssize_t interleave_ways_store(struct device *dev,
>                 goto out;
>         }
>  
> +       /*
> +        * If region granularity has been set and xHB interleave is active,
> +        * validate that granularity is compatible with specified ways.
> +        * Otherwise allow ways to be set now and depend on
> +        * interleave_granularity_store() to validate this constraint.
> +        */
> +       if (cxld->interleave_ways > 1 &&
> +           p->interleave_granularity > cxld->interleave_granularity &&
> +           p->interleave_granularity / cxld->interleave_granularity !=
> +                   val / cxld->interleave_ways) {
> +               dev_dbg(dev,
> +                       "ways scaling factor %d mismatch with granularity %d\n",
> +                       val / cxld->interleave_ways,
> +                       p->interleave_granularity /
> +                               cxld->interleave_granularity);
> +               rc = -EINVAL;
> +               goto out;
> +       }
> +
>         save = p->interleave_ways;
>         p->interleave_ways = val;
>         rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
> @@ -386,7 +405,7 @@ static ssize_t interleave_granularity_store(struct device *dev,
>         struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
>         struct cxl_region *cxlr = to_cxl_region(dev);
>         struct cxl_region_params *p = &cxlr->params;
> -       int rc, val;
> +       int rc, val, ways;
>         u16 ig;
>  
>         rc = kstrtoint(buf, 0, &val);
> @@ -403,9 +422,29 @@ static ssize_t interleave_granularity_store(struct device *dev,
>          * granularity less than the root interleave result in needing
>          * multiple endpoints to support a single slot in the
>          * interleave.
> +        *
> +        * When the root interleave ways is 1 then the root granularity is a
> +        * don't care.
> +        *
> +        * Limit region granularity to cxld->interleave_granularity *
> +        * rounddown_pow_of_two(cxld->interleave_ways) otherwise holes result in
> +        * the decode at each endpoint. Note that rounddown_pow_of_two()
> +        * accounts for x3, x6, and x9 root intereleave.
>          */
> -       if (val < cxld->interleave_granularity)
> -               return -EINVAL;
> +       ways = rounddown_pow_of_two(cxld->interleave_ways);
> +       if (ways > 1) {
> +               if (val < cxld->interleave_granularity) {
> +                       dev_dbg(dev, "granularity %d must be >= %d\n", val,
> +                               cxld->interleave_granularity);
> +                       return -EINVAL;
> +               }
> +
> +               if (val > cxld->interleave_granularity * ways) {
> +                       dev_dbg(dev, "granularity %d must be <= %d\n", val,
> +                               cxld->interleave_granularity * ways);
> +                       return -EINVAL;
> +               }
> +       }
>  
>         rc = down_write_killable(&cxl_region_rwsem);
>         if (rc)
> @@ -415,6 +454,24 @@ static ssize_t interleave_granularity_store(struct device *dev,
>                 goto out;
>         }
>  
> +       /*
> +        * If region ways has been set and xHB interleave is active, validate
> +        * that ways is compatible with specified granularity.  Otherwise allow
> +        * granularity to be set now and depend on interleave_ways_store() to
> +        * validate this constraint.
> +        */
> +       if (cxld->interleave_ways > 1 && p->interleave_ways &&
> +           val > cxld->interleave_granularity &&
> +           p->interleave_ways / cxld->interleave_ways !=
> +                   val / cxld->interleave_granularity) {
> +               dev_dbg(dev,
> +                       "granularity scaling factor %d mismatch with ways %d\n",
> +                       val / cxld->interleave_granularity,
> +                       p->interleave_ways / cxld->interleave_ways);
> +               rc = -EINVAL;
> +               goto out;
> +       }
> +
>         p->interleave_granularity = val;
>  out:
>         up_write(&cxl_region_rwsem);
>