| Message ID | 20241023172150.659002-1-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | AMD NB and SMN rework | expand |
On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote: > Hi all, > > The theme of this set is decoupling the "AMD node" concept from the > legacy northbridge support. > > Additionally, AMD System Management Network (SMN) access code is > decoupled and expanded too. > > Patches 1-3 begin reducing the scope of AMD_NB. > > Patches 4-9 begin moving generic AMD node support out of AMD_NB. > > Patches 10-13 move SMN support out of AMD_NB and do some refactoring. > > Patch 14 has HSMP reuse SMN functionality. > > Patches 15-16 address userspace access to SMN. > > I say "begin" above because there is more to do here. Ultimately, AMD_NB > should only be needed for code used on legacy systems with northbridges. > Also, any and all SMN users in the kernel need to be updated to use the > central SMN code. Local solutions should be avoided. Glad to see many of the PCI device IDs going away; thanks for working on that! The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not ideal since all those pci_get_*() interfaces are kind of ugly in my opinion, and using them means we have to encode topology details in the kernel. But this still seems like a big improvement. Bjorn
On Wed, Oct 23, 2024 at 12:59:28PM -0500, Bjorn Helgaas wrote: > On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote: > > Hi all, > > > > The theme of this set is decoupling the "AMD node" concept from the > > legacy northbridge support. > > > > Additionally, AMD System Management Network (SMN) access code is > > decoupled and expanded too. > > > > Patches 1-3 begin reducing the scope of AMD_NB. > > > > Patches 4-9 begin moving generic AMD node support out of AMD_NB. > > > > Patches 10-13 move SMN support out of AMD_NB and do some refactoring. > > > > Patch 14 has HSMP reuse SMN functionality. > > > > Patches 15-16 address userspace access to SMN. > > > > I say "begin" above because there is more to do here. Ultimately, AMD_NB > > should only be needed for code used on legacy systems with northbridges. > > Also, any and all SMN users in the kernel need to be updated to use the > > central SMN code. Local solutions should be avoided. > > Glad to see many of the PCI device IDs going away; thanks for working > on that! > > The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not > ideal since all those pci_get_*() interfaces are kind of ugly in my > opinion, and using them means we have to encode topology details in > the kernel. But this still seems like a big improvement. > Thanks for the feedback. Hopefully, we'll come to some improved solution. :) Can you please elaborate on your concern? Is it about saying "thing X is always at SBDF A:B:C.D" or something else? Thanks, Yazen
On Thu, Oct 24, 2024 at 12:01:59PM -0400, Yazen Ghannam wrote: > On Wed, Oct 23, 2024 at 12:59:28PM -0500, Bjorn Helgaas wrote: > > On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote: > > > Hi all, > > > > > > The theme of this set is decoupling the "AMD node" concept from the > > > legacy northbridge support. > > > > > > Additionally, AMD System Management Network (SMN) access code is > > > decoupled and expanded too. > > > > > > Patches 1-3 begin reducing the scope of AMD_NB. > > > > > > Patches 4-9 begin moving generic AMD node support out of AMD_NB. > > > > > > Patches 10-13 move SMN support out of AMD_NB and do some refactoring. > > > > > > Patch 14 has HSMP reuse SMN functionality. > > > > > > Patches 15-16 address userspace access to SMN. > > > > > > I say "begin" above because there is more to do here. Ultimately, AMD_NB > > > should only be needed for code used on legacy systems with northbridges. > > > Also, any and all SMN users in the kernel need to be updated to use the > > > central SMN code. Local solutions should be avoided. > > > > Glad to see many of the PCI device IDs going away; thanks for working > > on that! > > > > The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not > > ideal since all those pci_get_*() interfaces are kind of ugly in my > > opinion, and using them means we have to encode topology details in > > the kernel. But this still seems like a big improvement. > > Thanks for the feedback. Hopefully, we'll come to some improved > solution. :) > > Can you please elaborate on your concern? Is it about saying "thing X is > always at SBDF A:B:C.D" or something else? "Thing X is always at SBDF A:B:C.D" is one big reason. "A:B:C.D" says nothing about the actual functionality of the device. A PCI Vendor/Device ID or a PNP ID identifies the device programming model independent of its geographical location. Inferring the functionality and programming model from the location is a maintenance issue because hardware may change the address. PCI bus numbers are under software control, so in general it's not safe to rely on them, although in this case these devices are probably on root buses where the bus number is either fixed or determined by BIOS configuration of the host bridge. I don't like the pci_get_*() functions because they break the driver model. The usual .probe() model binds a device to a driver, which essentially means the driver owns the device and its resources, and the driver and doesn't have to worry about other code interfering. Unlike pci_get_*(), the .probe()/.remove() model automatically handles hotplug without extra things like notifiers in the driver. Hotplug may not be an issue in this particular case, but it requires specific platform knowledge to be sure. Some platforms do support CPU and PCI host bridge hotplug. Thanks again for doing all this work. It's a huge improvement already! Bjorn
On 10/24/2024 12:46, Bjorn Helgaas wrote: > On Thu, Oct 24, 2024 at 12:01:59PM -0400, Yazen Ghannam wrote: >> On Wed, Oct 23, 2024 at 12:59:28PM -0500, Bjorn Helgaas wrote: >>> On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote: >>>> Hi all, >>>> >>>> The theme of this set is decoupling the "AMD node" concept from the >>>> legacy northbridge support. >>>> >>>> Additionally, AMD System Management Network (SMN) access code is >>>> decoupled and expanded too. >>>> >>>> Patches 1-3 begin reducing the scope of AMD_NB. >>>> >>>> Patches 4-9 begin moving generic AMD node support out of AMD_NB. >>>> >>>> Patches 10-13 move SMN support out of AMD_NB and do some refactoring. >>>> >>>> Patch 14 has HSMP reuse SMN functionality. >>>> >>>> Patches 15-16 address userspace access to SMN. >>>> >>>> I say "begin" above because there is more to do here. Ultimately, AMD_NB >>>> should only be needed for code used on legacy systems with northbridges. >>>> Also, any and all SMN users in the kernel need to be updated to use the >>>> central SMN code. Local solutions should be avoided. >>> >>> Glad to see many of the PCI device IDs going away; thanks for working >>> on that! >>> >>> The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not >>> ideal since all those pci_get_*() interfaces are kind of ugly in my >>> opinion, and using them means we have to encode topology details in >>> the kernel. But this still seems like a big improvement. >> >> Thanks for the feedback. Hopefully, we'll come to some improved >> solution. :) >> >> Can you please elaborate on your concern? Is it about saying "thing X is >> always at SBDF A:B:C.D" or something else? > > "Thing X is always at SBDF A:B:C.D" is one big reason. "A:B:C.D" says > nothing about the actual functionality of the device. A PCI > Vendor/Device ID or a PNP ID identifies the device programming model > independent of its geographical location. Inferring the functionality > and programming model from the location is a maintenance issue because > hardware may change the address. > > PCI bus numbers are under software control, so in general it's not > safe to rely on them, although in this case these devices are probably > on root buses where the bus number is either fixed or determined by > BIOS configuration of the host bridge. > > I don't like the pci_get_*() functions because they break the driver > model. The usual .probe() model binds a device to a driver, which > essentially means the driver owns the device and its resources, and > the driver and doesn't have to worry about other code interfering. Are you suggesting that perhaps we should be introducing amd_smn (patch 10) as a PCI driver that binds "to the root device" instead? If we made this change, I would wonder if it comes up early enough, particularly considering quirk_clear_strap_no_soft_reset_dev2_f0() uses the SMN symbols as PCI fixup final which happens before a driver attaches (pci_bus_add_device()). There are some areas that do discovery (for example amd_node_get_root() in patch 6/16). I think we should aspire to do is much discovery as possible but I don't know we can get TOTALLY away from some hardcoded topology information. > > Unlike pci_get_*(), the .probe()/.remove() model automatically handles > hotplug without extra things like notifiers in the driver. Hotplug > may not be an issue in this particular case, but it requires specific > platform knowledge to be sure. Some platforms do support CPU and PCI > host bridge hotplug. > Yeah hotplug won't matter for these. > Thanks again for doing all this work. It's a huge improvement > already!
On Thu, Oct 24, 2024 at 03:08:41PM -0500, Mario Limonciello wrote: > On 10/24/2024 12:46, Bjorn Helgaas wrote: > > On Thu, Oct 24, 2024 at 12:01:59PM -0400, Yazen Ghannam wrote: > > > On Wed, Oct 23, 2024 at 12:59:28PM -0500, Bjorn Helgaas wrote: > > > > On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote: > ... > > > > The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not > > > > ideal since all those pci_get_*() interfaces are kind of ugly in my > > > > opinion, and using them means we have to encode topology details in > > > > the kernel. But this still seems like a big improvement. > > > > > > Thanks for the feedback. Hopefully, we'll come to some improved > > > solution. :) > > > > > > Can you please elaborate on your concern? Is it about saying "thing X is > > > always at SBDF A:B:C.D" or something else? > > > > "Thing X is always at SBDF A:B:C.D" is one big reason. "A:B:C.D" says > > nothing about the actual functionality of the device. A PCI > > Vendor/Device ID or a PNP ID identifies the device programming model > > independent of its geographical location. Inferring the functionality > > and programming model from the location is a maintenance issue because > > hardware may change the address. > > > > PCI bus numbers are under software control, so in general it's not > > safe to rely on them, although in this case these devices are probably > > on root buses where the bus number is either fixed or determined by > > BIOS configuration of the host bridge. > > > > I don't like the pci_get_*() functions because they break the driver > > model. The usual .probe() model binds a device to a driver, which > > essentially means the driver owns the device and its resources, and > > the driver and doesn't have to worry about other code interfering. > > Are you suggesting that perhaps we should be introducing amd_smn (patch 10) > as a PCI driver that binds "to the root device" instead? I don't know any of the specifics, so I can't really opine on that. The PCI specs envision that a Vendor/Device ID defines the programming model of the device, and you would only use a new Device ID when that programming model changes. Of course, vendors like to define a new set of Device IDs for every new chipset even when no driver changes are required, so even if a new SMN works exactly the same as in previous chipsets, you're probably back to having to add a new Device ID for every new chipset. The Subsystem Vendor ID and Subsystem ID exist to solve a similar problem (sort of in reverse). If AMD could allocate a Subsystem ID for this SMN programming model and use that same ID in every chipset, you could make a pci_driver.id_table entry that would match them all, e.g., .vendor = PCI_VENDOR_ID_AMD, .device = PCI_ANY_ID, .subvendor = PCI_VENDOR_ID_AMD, .subdevice = PCI_SUBSYSTEM_AMD_SMN, (pci_device_id.subdevice is misnamed; the spec calls it "Subsystem ID") > There are some areas that do discovery (for example amd_node_get_root() in > patch 6/16). Sort of. amd_node_get_root() and amd_node_get_func() both just grub through all the devices that the PCI core has enumerated and return the one that has the right geographical address. There's no binding to a driver, so another driver could come along and bind to the same device, and then you have a potential conflict. You also give up all the standard driver model infrastructure for hotplug, power management, etc. Granted, you probably don't care about those things here. Bjorn
On 10/24/2024 16:06, Bjorn Helgaas wrote: > On Thu, Oct 24, 2024 at 03:08:41PM -0500, Mario Limonciello wrote: >> On 10/24/2024 12:46, Bjorn Helgaas wrote: >>> On Thu, Oct 24, 2024 at 12:01:59PM -0400, Yazen Ghannam wrote: >>>> On Wed, Oct 23, 2024 at 12:59:28PM -0500, Bjorn Helgaas wrote: >>>>> On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote: >> ... > >>>>> The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not >>>>> ideal since all those pci_get_*() interfaces are kind of ugly in my >>>>> opinion, and using them means we have to encode topology details in >>>>> the kernel. But this still seems like a big improvement. >>>> >>>> Thanks for the feedback. Hopefully, we'll come to some improved >>>> solution. :) >>>> >>>> Can you please elaborate on your concern? Is it about saying "thing X is >>>> always at SBDF A:B:C.D" or something else? >>> >>> "Thing X is always at SBDF A:B:C.D" is one big reason. "A:B:C.D" says >>> nothing about the actual functionality of the device. A PCI >>> Vendor/Device ID or a PNP ID identifies the device programming model >>> independent of its geographical location. Inferring the functionality >>> and programming model from the location is a maintenance issue because >>> hardware may change the address. >>> >>> PCI bus numbers are under software control, so in general it's not >>> safe to rely on them, although in this case these devices are probably >>> on root buses where the bus number is either fixed or determined by >>> BIOS configuration of the host bridge. >>> >>> I don't like the pci_get_*() functions because they break the driver >>> model. The usual .probe() model binds a device to a driver, which >>> essentially means the driver owns the device and its resources, and >>> the driver and doesn't have to worry about other code interfering. >> >> Are you suggesting that perhaps we should be introducing amd_smn (patch 10) >> as a PCI driver that binds "to the root device" instead? > > I don't know any of the specifics, so I can't really opine on that. > > The PCI specs envision that a Vendor/Device ID defines the programming > model of the device, and you would only use a new Device ID when that > programming model changes. > > Of course, vendors like to define a new set of Device IDs for every > new chipset even when no driver changes are required, so even if a new > SMN works exactly the same as in previous chipsets, you're probably > back to having to add a new Device ID for every new chipset. Yeah; this I believe is why we're here today and trying to find something more manageable (IE this series). > > The Subsystem Vendor ID and Subsystem ID exist to solve a similar > problem (sort of in reverse). If AMD could allocate a Subsystem ID > for this SMN programming model and use that same ID in every chipset, > you could make a pci_driver.id_table entry that would match them all, > e.g., > > .vendor = PCI_VENDOR_ID_AMD, > .device = PCI_ANY_ID, > .subvendor = PCI_VENDOR_ID_AMD, > .subdevice = PCI_SUBSYSTEM_AMD_SMN, > > (pci_device_id.subdevice is misnamed; the spec calls it "Subsystem ID") Isn't the subsystem ID based typically upon the platform it's running on? For example I seem to recall on Dell systems it's used the value that was in the SBMIOS ProductSKU field here (IoW not something AMD would control). I mean I guess maybe we could do a: .vendor = PCI_VENDOR_ID_AMD, .device = PCI_ANY_ID, .class = PCI_CLASS_BRIDGE_HOST << 8 And then in probe() figure out if it's the right one, but that's still pretty ugly, eh? > >> There are some areas that do discovery (for example amd_node_get_root() in >> patch 6/16). > > Sort of. amd_node_get_root() and amd_node_get_func() both just grub > through all the devices that the PCI core has enumerated and return > the one that has the right geographical address. > > There's no binding to a driver, so another driver could come along and > bind to the same device, and then you have a potential conflict. > > You also give up all the standard driver model infrastructure for > hotplug, power management, etc. Granted, you probably don't care > about those things here. Right; I agree none of that matters here.
On Thu, Oct 24, 2024 at 04:20:35PM -0500, Mario Limonciello wrote: > On 10/24/2024 16:06, Bjorn Helgaas wrote: > > On Thu, Oct 24, 2024 at 03:08:41PM -0500, Mario Limonciello wrote: > > > On 10/24/2024 12:46, Bjorn Helgaas wrote: > > > > On Thu, Oct 24, 2024 at 12:01:59PM -0400, Yazen Ghannam wrote: > > > > > On Wed, Oct 23, 2024 at 12:59:28PM -0500, Bjorn Helgaas wrote: > > > > > > On Wed, Oct 23, 2024 at 05:21:34PM +0000, Yazen Ghannam wrote: > > > ... > > > > > > > > The use of pci_get_slot() and pci_get_domain_bus_and_slot() is not > > > > > > ideal since all those pci_get_*() interfaces are kind of ugly in my > > > > > > opinion, and using them means we have to encode topology details in > > > > > > the kernel. But this still seems like a big improvement. > > > > > > > > > > Thanks for the feedback. Hopefully, we'll come to some improved > > > > > solution. :) > > > > > > > > > > Can you please elaborate on your concern? Is it about saying "thing X is > > > > > always at SBDF A:B:C.D" or something else? > > > > > > > > "Thing X is always at SBDF A:B:C.D" is one big reason. "A:B:C.D" says > > > > nothing about the actual functionality of the device. A PCI > > > > Vendor/Device ID or a PNP ID identifies the device programming model > > > > independent of its geographical location. Inferring the functionality > > > > and programming model from the location is a maintenance issue because > > > > hardware may change the address. > > > > > > > > PCI bus numbers are under software control, so in general it's not > > > > safe to rely on them, although in this case these devices are probably > > > > on root buses where the bus number is either fixed or determined by > > > > BIOS configuration of the host bridge. > > > > > > > > I don't like the pci_get_*() functions because they break the driver > > > > model. The usual .probe() model binds a device to a driver, which > > > > essentially means the driver owns the device and its resources, and > > > > the driver and doesn't have to worry about other code interfering. > > > > > > Are you suggesting that perhaps we should be introducing amd_smn (patch 10) > > > as a PCI driver that binds "to the root device" instead? > > > > I don't know any of the specifics, so I can't really opine on that. > > > > The PCI specs envision that a Vendor/Device ID defines the programming > > model of the device, and you would only use a new Device ID when that > > programming model changes. > > > > Of course, vendors like to define a new set of Device IDs for every > > new chipset even when no driver changes are required, so even if a new > > SMN works exactly the same as in previous chipsets, you're probably > > back to having to add a new Device ID for every new chipset. > > Yeah; this I believe is why we're here today and trying to find something > more manageable (IE this series). Another alternative would be an ACPI device where you can use the same _HID (or at least a _CID) for all the chipsets. > > The Subsystem Vendor ID and Subsystem ID exist to solve a similar > > problem (sort of in reverse). If AMD could allocate a Subsystem ID > > for this SMN programming model and use that same ID in every chipset, > > you could make a pci_driver.id_table entry that would match them all, > > e.g., > > > > .vendor = PCI_VENDOR_ID_AMD, > > .device = PCI_ANY_ID, > > .subvendor = PCI_VENDOR_ID_AMD, > > .subdevice = PCI_SUBSYSTEM_AMD_SMN, > > > > (pci_device_id.subdevice is misnamed; the spec calls it "Subsystem ID") > > Isn't the subsystem ID based typically upon the platform it's > running on? For example I seem to recall on Dell systems it's used > the value that was in the SBMIOS ProductSKU field here (IoW not > something AMD would control). Right, it is typically based on the platform; that's why I said "in reverse." I think all these devices are integrated into the chipset, so I'm speculating that platform vendors would have no need (maybe even no way) to use the Subsystem ID. But maybe that's not the case. > I mean I guess maybe we could do a: > > .vendor = PCI_VENDOR_ID_AMD, > .device = PCI_ANY_ID, > .class = PCI_CLASS_BRIDGE_HOST << 8 > > And then in probe() figure out if it's the right one, but that's still > pretty ugly, eh? I think there are some drivers that do this, and it's not completely terrible. The probe() can just return failure if it doesn't want the device. Bjorn