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

[Dan Williams <dan.j.williams@intel.com>]

Bjorn Helgaas wrote:
> On Wed, Mar 27, 2024 at 04:57:40PM -0700, Dave Jiang wrote:
> > On 3/27/24 2:26 PM, Bjorn Helgaas wrote:
> > > On Mon, Mar 25, 2024 at 04:58:01PM -0700, Dave Jiang wrote:
> 
> > >> With the current behavior, the bus_reset would appear to have executed
> > >> successfully, however the operation is actually masked if the "Unmask
> > >> SBR" bit is set with the default value. The intention is to inform the
> > >> user that SBR for the CXL device is masked and will not go through.
> > > 
> > > The default value of Unmask SBR isn't really relevant here.
> > 
> > Changing to:
> >     When the "Unmask SBR" bit is set to 0 (default), the bus_reset would
> >     appear to have executed successfully. However the operation is actually
> >     masked. The intention is to inform the user that SBR for the CXL device
> >     is masked and will not go through.
> > 
> > > 
> > >> The expectation is that if a user overrides the "Unmask SBR" via a
> > >> user tool such as setpci then they can trigger a bus reset successfully.
> > > 
> > > ... if a user *sets* Unmask SBR ...
> > > to be more specific about what value is required.
> > > 
> > 
> >     If a user overrides the "Unmask SBR" via a user tool such as setpci and
> >     sets the value to 1, then the bus reset will execute successfully.
> 
> I'm not super enamored with the "if a user overrides" language because
> a driver may change that bit in the future, and then the suggestion of
> a "user" will be misleading.
> 
> It doesn't matter *how* it gets set to 1; it only matters that SBR
> doesn't work when "Unmask SBR" is 0 and it does work when "Unmask SBR"
> is 1.
> 
> > >> +/* Compute Express Link (CXL) */
> > >> +#define PCI_DVSEC_VENDOR_ID_CXL				0x1e98
> > > 
> > > I see that you're just moving this #define from drivers/cxl/cxlpci.h,
> > > but I'm scratching my head a bit.  As used here, this is to match the
> > > DVSEC Vendor ID (PCIe r6.0, sec 7.9.6.2).
> > > 
> > > IIUC, this should be just a PCI SIG-defined "Vendor ID", e.g.,
> > > "PCI_VENDOR_ID_CXL", that doesn't need the "DVSEC" qualifier in the
> > > name, and would normally be defined in include/linux/pci_ids.h.
> > > 
> > > But I don't see CXL in pci_ids.h, and I don't see either "CXL" or the
> > > value "1e98" in the PCI SIG list at
> > > https://pcisig.com/membership/member-companies.
> > > 
> > I'll create a new patch and move to include/linux/pci_ids.h first
> > for this define and change to PCI_VENDOR_ID_CXL. The value is
> > defined in CXL spec r3.1 sec 8.1.1.
> 
> I saw the CXL mentions of 0x1e98, but IMO that's not an authoritative
> source; no vendor is allowed to just squat on a Vendor ID value simply
> by mentioning it in their own internal specs.  That would obviously
> lead to madness.
> 
> The footnote in CXL r3.1, sec 3.1.2, about how the 1E98h value is only
> for use in DVSEC etc., is really weird.
> 
> IIUC, the PCI SIG controls the Vendor ID namespace, so I'm still
> really confused about why it is not reserved there.  I emailed the PCI
> SIG, but the footnote suggests to me that there some kind of history
> here that I don't know.
> 
> Anyway, I think all we can do here is to put the definition in
> include/linux/pci_ids.h as you did and hope 0x1e98 is never allocated
> to another vendor.

Oh, true, I think this should be PCI_DVSEC_VENDOR_ID_CXL, because afaics
it is still possible that 0x1e98 be used as a non-DVSEC vendor-id in
some future device.

In other words I think the CXL specification usage of 0x1e98 is scoped
as "DVSEC Vendor ID", not "Vendor ID".

However that would mean that a future 0x1e98 device could not publish
DVSECs without colliding with CXL DVSECs.

I note this footnote in section 3.1.2 about the 0x1e98 value (all caps
emphasis is from the spec, not me):

---
NOTICE TO USERS: THE UNIQUE VALUE THAT IS PROVIDED IN THIS CXL SPECIFICATION IS
FOR USE IN VENDOR DEFINED MESSAGE FIELDS, DESIGNATED VENDOR SPECIFIC EXTENDED
CAPABILITIES, AND ALTERNATE PROTOCOL NEGOTIATION ONLY...