Re: [PATCH v9] cxl: add RAS status unmasking for CXL

[Yazen Ghannam <yazen.ghannam@amd.com>]

On 7/13/2023 5:50 PM, Smita Koralahalli wrote:
> On 7/13/2023 2:21 PM, Dan Williams wrote:
>> Smita Koralahalli wrote:
>>> Hi all,
>>>
>>> I understand this has been in upstream already. But I have a slight
>>> confusion on one of the checks been done here.
>>>
>>> On 2/21/2023 9:55 AM, Dave Jiang wrote:
>>>> By default the CXL RAS mask registers bits are defaulted to 1's and
>>>> suppress all error reporting. If the kernel has negotiated ownership
>>>> of error handling for CXL then unmask the mask registers by writing 0s.
>>>>
>>>> PCI_EXP_DEVCTL capability is checked to see uncorrectable or 
>>>> correctable
>>>> errors bits are set before unmasking the respective errors.
>>>>
>>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>  # pci_regs.h
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>>
>>>> ---
>>>
>>>> +static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>>> +{
>>>> +    struct pci_host_bridge *host_bridge = 
>>>> pci_find_host_bridge(pdev->bus);
>>>> +    struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>>>> +    void __iomem *addr;
>>>> +    u32 orig_val, val, mask;
>>>> +    u16 cap;
>>>> +    int rc;
>>>> +
>>>> +    if (!cxlds->regs.ras) {
>>>> +        dev_dbg(&pdev->dev, "No RAS registers.\n");
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    /* BIOS has CXL error control */
>>>> +    if (!host_bridge->native_cxl_error)
>>>> +        return -EOPNOTSUPP;
>>>
>>> Why are we checking for native_cxl_error (native_cxl_error is CXL Memory
>>> Error Reporting Control _OSC bit..) while unmasking RAS status?
>>>
>>> RAS registers will be reported on a protocol error and the protocol
>>> error follows the PCIe AER. Should we check for AER _OSC instead of CXL
>>> Memory Error _OSC?
>>>
>>> Because atleast on AMD systems we log RAS registers only on Protocol
>>> errors and we use this CXL Memory _OSC knob to report component errors.
>>> Is it same across everywhere? And there might be cases where protocol
>>> error reporting might be native (PCIe AER) and component/memory can be
>>> FW-First which fails this check..
>>
>> I think that's reasonable, it just was not clear from the specification
>> that CXL protocol errors are included in PCIe AER as far as _OSC is
>> concerned because they are conveyed as "internal" errors.
>>

I think the CXL spec is relatively clear here.

Protocol error _OSC description:

	CXL Protocol Error Reporting Supported

	The OS sets this bit to 1 if it supports handling of CXL Protocol 
Errors. Otherwise, the OS clears this bit to 0.

	If the OS sets this bit, it must also set either bit 0 or bit 1 above.
	Note: Firmware may retain control of AER if the OS does not support CXL 
Protocol Error reporting because the owner of AER owns CXL Protocol 
error management.

The last note shows that AER and CXL Protocol error handling are tied 
together. This is similar to how we tie AER and DPC, I think.

The key difference is that the OS can't request control of CXL Protocol 
error explicitly. The OS tells the Platform that it can support CXL 
Protocol errors, and the OS can request control of AER. The Platform can 
then decide if it wants to give the OS control of both AER and CXL 
Protocol Error handling.

>> So I believe it was an "abundance of caution" more than a requirement
>> that Linux expects control of memory-errors before proceeding to touch
>> the CXL RAS registers.
> 

The "CXL Memory Error Reporting Control" _OSC description specifically 
highlights the "Memory Error Logging and Signaling Enhancements" 
section. And this is the set of errors reported through a device's mailbox.

So CXL Protocol errors (AER + CXL RAS cap) and CXL Events (Mailbox/Event 
Logs) can be managed independently.

Thanks,
Yazen

> Got it. Also, are there any issues returning zero here, rather than an 
> error code just like we are doing in cxl_event_config()?
> 
> The error code returned from this function wouldn't grant cxl control to 
> OS (i.e fails to create device node /dev/cxl/mem0) which would confuse 
> users when they are operating with native PCIe AER support.
> 
> Thanks
> Smita