Re: [RFC PATCH 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 4 Jan 2023 09:07:41 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 1/3/23 4:20 PM, Dave Jiang wrote:
> > 
> > 
> > On 12/19/22 9:42 AM, Jonathan Cameron wrote:  
> >> On Wed, 30 Nov 2022 16:12:38 -0700
> >> Dave Jiang <dave.jiang@intel.com> wrote:
> >>  
> >>> In the case where HDM decoder register block exists but is not 
> >>> programmed
> >>> and at the same time the DVSEC range register range is active, 
> >>> populate the
> >>> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.  
> >>
> >> I may be overthinking this...
> >>
> >> So I think this results in us enabling hdm decoder registers on
> >> a device that the BIOS already set the range registers for?  
> > 
> > The code don't actually program and enable an unprogrammed HDM decoder. 
> > It creates a locked HDM decoder structure that's filled with info from 
> > DVSEC. The interested info will show up in sysfs but there should not be 
> > any changes WRT existing HDM decoder. The DVSEC range register 
> > exclusively controls this memory range.
> >   
> >>
> >> I'm not sure the spec guarantees that is a safe operation if accesses are
> >> in flight.
> >> You can imagine a device which goes through an unsafe intermediate state
> >> when switching over from range registers to HDM decoders.  That wouldn't
> >> normally be a problem as we'd not expect traffic in flight, but if the
> >> BIOS already set up a mapping the OS might see that as normal memory 
> >> which
> >> it is using when this transition occurs.
> >>
> >> If just feels like a transition no one will test that might bite us in
> >> future in really hard to detect ways.
> >>  
> >>>
> >>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >>> ---
> >>>   drivers/cxl/core/hdm.c |   33 ++++++++++++++++++++++++++++++---
> >>>   drivers/cxl/core/pci.c |   12 ------------
> >>>   drivers/cxl/cxl.h      |    3 ++-
> >>>   drivers/cxl/port.c     |    2 +-
> >>>   4 files changed, 33 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> >>> index d1d2caea5c62..9773a5efaefd 100644
> >>> --- a/drivers/cxl/core/hdm.c
> >>> +++ b/drivers/cxl/core/hdm.c
> >>> @@ -674,9 +674,31 @@ static int cxl_decoder_reset(struct cxl_decoder 
> >>> *cxld)
> >>>       return 0;
> >>>   }
> >>> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
> >>> +                        struct cxl_decoder *cxld, int which,
> >>> +                        struct cxl_endpoint_dvsec_info *info)
> >>> +{
> >>> +    if (!is_cxl_endpoint(port))
> >>> +        return -EOPNOTSUPP;
> >>> +
> >>> +    if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
> >>> +        return -ENXIO;
> >>> +
> >>> +    cxld->target_type = CXL_DECODER_ACCELERATOR;  
> >>
> >> Why chose type 2 target type?  Definitely needs a comment.  
> > 
> > There are only 2 defines for decoder type. I picked the one that is not 
> > EXPANDER.  
> 
> Looks like there were some confusion on my part. For purpose of 
> emulation here it's sufficient to just set it to generic 
> CXL_DECODER_EXPANDER (type 3)?

I think that's the right choice as well.

Jonathan

> 
> DJ
> 
> >   
> >> Also would be good to have a precursor patch that moves these
> >> over to the CXL 3.0 naming to incorportate the fun difference
> >> between HDM-DB and HDM-H type 3 devices
> >>
> >> CXL_DEVICE_COHERENT_ADDRESS_RANGE
> >> CXL_HOST_ONLY_COHERENT_ADDRESS_RANGE  
> > 
> > I'll take a look at the spec.  
> >>
> >>
> >>  
> >>> +
> >>> +    cxld->hpa_range = (struct range) {
> >>> +        .start = info->dvsec_range[which].start,
> >>> +        .end = info->dvsec_range[which].end,
> >>> +    };
> >>> +
> >>> +    cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> >>> +    port->commit_end = cxld->id;  
> >>
> >> blank line before all simple returns like this makes the code
> >> slightly more readable.  
> > 
> > ok  
> >>  
> >>> +    return 0;
> >>> +}
> >>> +  
> >>
> >> ...
> >>  
> >>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> >>> index 385dbe9bd5f2..5e44fe23fa76 100644
> >>> --- a/drivers/cxl/core/pci.c
> >>> +++ b/drivers/cxl/core/pci.c
> >>> @@ -412,18 +412,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state 
> >>> *cxlds, struct cxl_hdm *cxlhdm,
> >>>           info->mem_enabled = 0;
> >>>       }
> >>> -    /*
> >>> -     * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 
> >>> Base
> >>> -     * [High,Low] when HDM operation is enabled the range register 
> >>> values
> >>> -     * are ignored by the device, but the spec also recommends 
> >>> matching the
> >>> -     * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero 
> >>> info->ranges
> >>> -     * are expected even though Linux does not require or maintain that
> >>> -     * match. If at least one DVSEC range is enabled and allowed, 
> >>> skip HDM
> >>> -     * Decoder Capability Enable.
> >>> -     */
> >>> -    if (info->mem_enabled)
> >>> -        return -EBUSY;
> >>> -  
> >> Dropping this condition is the bit I'm referring to at the top.  I think
> >> if (info->mem_enabled)
> >>     return 0;
> >>
> >> would avoid that transition.  
> > 
> > ok  
> >>  
> >>>       rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> >>>       if (rc)
> >>>           return rc;  
>