On Fri, Sep 08, 2023 at 03:52:15PM -0700, Ira Weiny wrote:
> Fan Ni wrote:
> > On Mon, Aug 28, 2023 at 10:20:54PM -0700, [email protected] wrote:
> > > From: Navneet Singh <[email protected]>
> > >
>
> [snip]
>
> > >
> > > +static int cxl_dc_save_region_info(struct cxl_memdev_state *mds, int index,
> > > + struct cxl_dc_region_config *region_config)
> > > +{
> > > + struct cxl_dc_region_info *dcr = &mds->dc_region[index];
> > > + struct device *dev = mds->cxlds.dev;
> > > +
> > > + dcr->base = le64_to_cpu(region_config->region_base);
> > > + dcr->decode_len = le64_to_cpu(region_config->region_decode_length);
> > > + dcr->decode_len *= CXL_CAPACITY_MULTIPLIER;
> > > + dcr->len = le64_to_cpu(region_config->region_length);
> > > + dcr->blk_size = le64_to_cpu(region_config->region_block_size);
> > > + dcr->dsmad_handle = le32_to_cpu(region_config->region_dsmad_handle);
> > > + dcr->flags = region_config->flags;
> > > + snprintf(dcr->name, CXL_DC_REGION_STRLEN, "dc%d", index);
> > > +
> > > + /* Check regions are in increasing DPA order */
> > > + if (index > 0) {
> > > + struct cxl_dc_region_info *prev_dcr = &mds->dc_region[index - 1];
> > > +
> > > + if ((prev_dcr->base + prev_dcr->decode_len) > dcr->base) {
> > > + dev_err(dev,
> > > + "DPA ordering violation for DC region %d and %d\n",
> > > + index - 1, index);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > + /* Check the region is 256 MB aligned */
> > > + if (!IS_ALIGNED(dcr->base, SZ_256M)) {
> > > + dev_err(dev, "DC region %d not aligned to 256MB: %#llx\n",
> > > + index, dcr->base);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Check Region base and length are aligned to block size */
> > > + if (!IS_ALIGNED(dcr->base, dcr->blk_size) ||
> > > + !IS_ALIGNED(dcr->len, dcr->blk_size)) {
> > > + dev_err(dev, "DC region %d not aligned to %#llx\n", index,
> > > + dcr->blk_size);
> > > + return -EINVAL;
> > > + }
> >
> > Based on on cxl 3.0 spec: Table 8-126, we may need some extra checks
> > here:
> > 1. region len <= decode_len
> > 2. region block size should be power of 2 and a multiple of 40H.
>
> Thanks for pointing these additional checks out! I've added these.
>
> >
> > Also, if region len or block size is 0, it mentions that DC will not be
> > available, we may also need to handle that.
>
> I've just added checks for 0 in region length, length and block size.
>
> I don't think we need to handle this in any special way. Any of these
> checks will fail the device probe. From my interpretation of the spec
> reading these values as 0 would indicate an invalid device configuration.
>
> That said I think the spec is a bit vague here. On the one hand the
> number of DC regions should reflect the number of valid regions.
>
> Table 8-125 'Number of Available Regions':
> "This is the number of valid region configurations returned in
> this payload."
>
> But it also says:
> "Each region may be unconfigured or configured with a different
> block size and capacity."
>
> I don't believe that a 0 in the Region Decode Length, Region Length, or
> Region Block Size is going to happen with the code structured the way it
> is. I believe these values are used if the host specifically requests the
> configuration of a region not indicated by 'Number of Available Regions'
> through the Starting Region Index in Table 8-163. This code does not do
> that.
>
> Would you agree with this?
Agreed.
Fan
>
> Thanks again,
> Ira