Re: [RFC PATCH 2/2] acpi/pci_root: negotiate CXL _OSC

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

On Wed, 2022-03-16 at 20:19 -0700, Dan Williams wrote:
> On Wed, Mar 16, 2022 at 5:27 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > Add full support for negotiating _OSC as defined in the CXL 2.0 spec, as
> > applicable to CXL-enabled platforms. Advertise support for the CXL
> > features we support - 'CXL 2.0 port/device register access', 'Protocol
> > Error Reporting', and 'CL Native Hot Plug'. Request control for 'CXL
> > Memory Error Reporting'. The requests are dependent on CONFIG_* based
> > pre-requisites, and prior PCI enabling, similar to how the standard PCI
> > _OSC bits are determined.
> 
> Might want to add a clarification here of why this just reflects the
> PCIe support into the similar CXL fields. For example:
> 
> The CXL specification does not define any additional constraints on
> the hotplug flow beyond PCIe native hotplug, so a kernel that supports
> native PCIe hotplug, supports CXL hotplug. For error handling protocol
> and link errors just use PCIe AER. There is nascent support for
> amending AER events with CXL specific status [1], but there's
> otherwise no additional OS responsibility for CXL errors beyond PCIe
> AER. CXL Memory Errors behave the same as typical memory errors so
> CONFIG_MEMORY_FAILURE is sufficient to indicate support to platform
> firmware.
> 
> [1]: https://lore.kernel.org/linux-cxl/164740402242.3912056.8303625392871313860.stgit@dwillia2-desk3.amr.corp.intel.com/

Yes that sounds good, will add.

> > 

[..]

> > @@ -440,6 +511,18 @@ static u32 calculate_support(void)
> >         return support;
> >  }
> > 
> > +static u32 calculate_cxl_support(void)
> > +{
> > +       u32 support;
> > +
> > +       support = OSC_CXL_2_0_PORT_DEV_REG_ACCESS_SUPPORT;
> > +       support |= OSC_CXL_PER_SUPPORT;
> 
> I would expect this one to be gated by pci_aer_available() since these
> errors are reported by PCIe AER.
> 
> Perhaps also s/PER/PORT_ERROR/? I keep reading PER like 'per' as in 'per-cpu'.

Expanding the acronym sounds good, though it is Protocol Error
Reporting, not Port.

> 
> > +       if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
> > +               support |= OSC_CXL_NATIVE_HP_SUPPORT;
> > +
> > +       return support;
> > +}
> > +
> >  static u32 calculate_control(void)
> >  {
> >         u32 control;
> > @@ -471,6 +554,16 @@ static u32 calculate_control(void)
> >         return control;
> >  }
> > 
> > +static u32 calculate_cxl_control(void)
> > +{
> > +       u32 control;
> > +
> > +       if (pci_aer_available())
> > +               control |= OSC_CXL_ERROR_REPORTING_CONTROL;
> 
> In this case the error handling is for memory errors, so this one should be:
> 
> if (IS_ENABLED(CONFIG_MEMORY_FAILURE))
>         control |= OSC_CXL_ERROR_REPORTING_CONTROL;

Makes sense, I'll change to this.

> 
> ...other than that looks good to me.

Thanks for the review!

>