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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ 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! >
