[1/3] PCI: Add check for CXL Secondary Bus Reset

Per CXL spec r3.1 8.1.5.2, secondary bus reset is masked unless the
"Unmask SBR" bit is set. Add a check to the PCI secondary bus reset
path to fail the CXL SBR request if the "Unmask SBR" bit is clear in
the CXL Port Control Extensions regiser by returning -EPERM.

The expectation is that if a user overrides the "Unmask SBR" via a
user tool such as setpci, they can trigger a bus reset after that.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/cxlpci.h          |  2 --
 drivers/pci/pci.c             | 45 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/pci_regs.h |  7 ++++++
 3 files changed, 52 insertions(+), 2 deletions(-)

On Mon, Mar 11, 2024 at 01:39:53PM -0700, Dave Jiang wrote:
> +static bool is_cxl_device(struct pci_dev *dev)
> +{
> +	return pci_find_dvsec_capability(dev, PCI_DVSEC_VENDOR_ID_CXL,
> +					 CXL_DVSEC_PCIE_DEVICE);
> +}

If this was my bikeshed, I'd call it pci_is_cxl() to match pci_is_pcie().


> +static bool is_cxl_port_sbr_masked(struct pci_dev *dev)
> +{
> +	int dvsec;
> +	int rc;
> +	u16 reg;

Nit: Inverse Christmas tree?


>  static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>  {
>  	int rc;
>  
> +	/* If it's a CXL port and the SBR control is masked, fail the SBR */
> +	if (is_cxl_device(dev) && dev->bus->self &&
> +	    is_cxl_port_sbr_masked(dev->bus->self)) {
> +		if (probe)
> +			return 0;
> +
> +		return -EPERM;
> +	}
> +

Is this also necessary if CONFIG_CXL_PCI=n?

Return code on non-availability of a reset method is generally -ENOTTY.
Or is the choice deliberate to expose this reset method despite the bit
being set and thus allow user space to unmask it in the first place?

Also, we mostly use pci_upstream_bridge(dev) in lieu of dev->bus->self.
Is the choice to use the latter deliberate because maybe is_virtfn is
never set and the device can never be on the root bus?  (What about
RCiEP CXL devices?)

Thanks,

Lukas

On 3/12/24 12:30 AM, Lukas Wunner wrote:
> On Mon, Mar 11, 2024 at 01:39:53PM -0700, Dave Jiang wrote:
>> +static bool is_cxl_device(struct pci_dev *dev)
>> +{
>> +	return pci_find_dvsec_capability(dev, PCI_DVSEC_VENDOR_ID_CXL,
>> +					 CXL_DVSEC_PCIE_DEVICE);
>> +}
> 
> If this was my bikeshed, I'd call it pci_is_cxl() to match pci_is_pcie().

Ok will change.
> 
> 
>> +static bool is_cxl_port_sbr_masked(struct pci_dev *dev)
>> +{
>> +	int dvsec;
>> +	int rc;
>> +	u16 reg;
> 
> Nit: Inverse Christmas tree?

Will change.
> 
> 
>>  static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>>  {
>>  	int rc;
>>  
>> +	/* If it's a CXL port and the SBR control is masked, fail the SBR */
>> +	if (is_cxl_device(dev) && dev->bus->self &&
>> +	    is_cxl_port_sbr_masked(dev->bus->self)) {
>> +		if (probe)
>> +			return 0;
>> +
>> +		return -EPERM;
>> +	}
>> +
> 
> Is this also necessary if CONFIG_CXL_PCI=n?

Yes. As the kernel only loads type3 mem class CXL device driver. This is attempt to cover all CXL devices and not dependent on a loaded driver.

> 
> Return code on non-availability of a reset method is generally -ENOTTY.

Ok I can change it to that.

> Or is the choice deliberate to expose this reset method despite the bit
> being set and thus allow user space to unmask it in the first place?

Yes the idea is if user intentionally unmasks the bit via a user tool then reset can go through.

> 
> Also, we mostly use pci_upstream_bridge(dev) in lieu of dev->bus->self.
> Is the choice to use the latter deliberate because maybe is_virtfn is
> never set and the device can never be on the root bus?  (What about
> RCiEP CXL devices?)

I'll change to pci_upstream_bridge() call. I didn't realize that existed.

> 
> Thanks,
> 
> Lukas