[PATCH v2 0/8] VMD add second rootbus support

[PATCH v2 0/8] VMD add second rootbus support

[Szymon Durawa]

This patch series implements second rootbus support inside Intel VMD module.
Current implementation allows VMD to take ownership of devices only on first
bus (Rootbus0). Starting from Intel Arrow Lake, VMD exposes second bus 
(Rootbus1) to allow VMD to own devices on this bus as well. VMD MMIO BARs
(CFGBAR. MEMBAR1 and MEMBAR2) are now shared between Rootbus0 and Rootbus1.
Reconfiguration of 3 MMIO BARs is required by resizing current MMIO BARs ranges.
It allows to find/register Rootbus1 and discovers devices behind it.

Patches 1 to 6 introduce code refactoring without functional changes.
Patch 7 implements VMD Rootbus1 and patch 8 provides workaround for rootbus
number hardwired to fixed non-zero value. Patch 8 is necessary for correct 
enumeration attached devices behind Rottbus1. Without it user cannot access
those devices.

Changes from v1:
- splitting series into more commits, requested by Bjorn
- adding helper functions, suggested by Bjorn
- minor typos and unclear wording updated, suggested by Bjorn

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: linux-pci@vger.kernel.org
Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>

Szymon Durawa (8):
  PCI: vmd: Add vmd_bus_enumeration()
  PCI: vmd: Add vmd_configure_cfgbar()
  PCI: vmd: Add vmd_configure_membar() and
    vmd_configure_membar1_membar2()
  PCI: vmd: Add vmd_create_bus()
  PCI: vmd: Replace hardcoded values with enum and defines
  PCI: vmd: Convert bus and busn_start to an array
  PCI: vmd: Add support for second rootbus under VMD
  PCI: vmd: Add workaround for rootbus number hardwired to fixed
    non-zero value

 drivers/pci/controller/vmd.c | 467 ++++++++++++++++++++++++++---------
 1 file changed, 357 insertions(+), 110 deletions(-)
 mode change 100644 => 100755 drivers/pci/controller/vmd.c

-- 
2.39.3

Re: [PATCH v2 0/8] VMD add second rootbus support

[Bjorn Helgaas]

On Fri, Nov 15, 2024 at 10:22:48AM +0100, Szymon Durawa wrote:
> This patch series implements second rootbus support inside Intel VMD module.
> Current implementation allows VMD to take ownership of devices only on first
> bus (Rootbus0). Starting from Intel Arrow Lake, VMD exposes second bus 
> (Rootbus1) to allow VMD to own devices on this bus as well. VMD MMIO BARs
> (CFGBAR. MEMBAR1 and MEMBAR2) are now shared between Rootbus0 and Rootbus1.
> Reconfiguration of 3 MMIO BARs is required by resizing current MMIO BARs ranges.
> It allows to find/register Rootbus1 and discovers devices behind it.
> 
> Patches 1 to 6 introduce code refactoring without functional changes.
> Patch 7 implements VMD Rootbus1 and patch 8 provides workaround for rootbus
> number hardwired to fixed non-zero value. Patch 8 is necessary for correct 
> enumeration attached devices behind Rottbus1. Without it user cannot access
> those devices.
> 
> Changes from v1:
> - splitting series into more commits, requested by Bjorn
> - adding helper functions, suggested by Bjorn
> - minor typos and unclear wording updated, suggested by Bjorn
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: linux-pci@vger.kernel.org
> Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
> 
> Szymon Durawa (8):
>   PCI: vmd: Add vmd_bus_enumeration()
>   PCI: vmd: Add vmd_configure_cfgbar()
>   PCI: vmd: Add vmd_configure_membar() and
>     vmd_configure_membar1_membar2()
>   PCI: vmd: Add vmd_create_bus()
>   PCI: vmd: Replace hardcoded values with enum and defines
>   PCI: vmd: Convert bus and busn_start to an array
>   PCI: vmd: Add support for second rootbus under VMD
>   PCI: vmd: Add workaround for rootbus number hardwired to fixed
>     non-zero value
> 
>  drivers/pci/controller/vmd.c | 467 ++++++++++++++++++++++++++---------
>  1 file changed, 357 insertions(+), 110 deletions(-)
>  mode change 100644 => 100755 drivers/pci/controller/vmd.c

It looks like only the cover letter was sent to linux-pci, and the
actual patches weren't.  Can you repost this with the correct cc list?
You might as well address the nits below at the same time and make it
a v3 to avoid confusion.

When you do, can you rephrase these to say they add support for a
second *Root Port*, which I think is what they do.

The additional root bus is just a consequence of having another Root
Port.  The root bus has no existence by itself, so it doesn't really
make sense to talk about "finding" or "registering" the root bus.

Also rewrap commit logs to fill 75 columns.  Include the names of new
helpers in the commit log.

Add blank lines between paragraphs (noticed in 8/8 comment, might be
elsewhere as well).

In 6/8, replace "Bus and busn_start are converted ..." with "Convert
..." as you did in the subject.

In 7/8,
s/enhacement/enhancement/
s/This patch add/Add/
s/workaraund/workaround/ (also in 8/8 comment)
Replace "rootbus" here with "Root Port".  "Devices behind rootbus"
doesn't really make sense.

In 8/8 comment, I'm not sure what "It assigns setup as broken" means.
Recast "... are updated to the same value" to imperative mood ("update
x to the same value").

The "BUS0" nomenclature seems heavily embedded in the vmd driver but
is really a misnomer.  Maybe that reflects similar terminology in an
internal spec?  Any hard-wiring of bus numbers reflects a property of
the way a *Root Port* works, so using the right name will make this
easier to understand, especially since there are other Root Ports with
the same hard wiring.

Bjorn

Re: [PATCH v2 0/8] VMD add second rootbus support

[Mariusz Tkaczyk]

On Fri, 15 Nov 2024 11:48:55 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> The "BUS0" nomenclature seems heavily embedded in the vmd driver but
> is really a misnomer.  Maybe that reflects similar terminology in an
> internal spec?  Any hard-wiring of bus numbers reflects a property of
> the way a *Root Port* works, so using the right name will make this
> easier to understand, especially since there are other Root Ports with
> the same hard wiring.

Hi Bjorn,

It is named as BUS_0 and BUS_1 because it refers to kernel's
pci_create_root_bus()`, we simplified it for better code wrapping.

Looks like it confuses you, do you like ROOT_BUS0 and ROOT_BUS1 better?

I don't see better fit than "root bus" because we are creating new root buses
accordingly.

Internally, it is called as "VMD domain" and each VMD Domain is presented as
separate pci root bus.

The original naming is presented in the sysfs links:
- "domain" -> root bus 0
= "domain1" -> root bus 1

i.e:
domain -> pci10000:e0/pci_bus/10000:e0
domain1 -> pci10000:e1/pci_bus/10000:e1

Thanks,
Mariusz

Re: [PATCH v2 0/8] VMD add second rootbus support

[Bjorn Helgaas]

On Wed, Nov 20, 2024 at 10:10:21AM +0100, Mariusz Tkaczyk wrote:
> On Fri, 15 Nov 2024 11:48:55 -0600
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > The "BUS0" nomenclature seems heavily embedded in the vmd driver but
> > is really a misnomer.  Maybe that reflects similar terminology in an
> > internal spec?  Any hard-wiring of bus numbers reflects a property of
> > the way a *Root Port* works, so using the right name will make this
> > easier to understand, especially since there are other Root Ports with
> > the same hard wiring.
> 
> It is named as BUS_0 and BUS_1 because it refers to kernel's
> pci_create_root_bus()`, we simplified it for better code wrapping.
> 
> Looks like it confuses you, do you like ROOT_BUS0 and ROOT_BUS1 better?
> 
> I don't see better fit than "root bus" because we are creating new root buses
> accordingly.

I was trying to use standard PCIe spec terminology where possible, but
I misspoke above, sorry.  I think we should pretend I never wrote that
paragraph, and also ignore my suggestion about replacing "rootbus"
with "Root Port" in the 7/8 commit log (sorry to everybody who never
got the patches I was responding to).

A "root bus" is the *primary* bus of a Root Port, so the root bus
number is determined by the host bridge, which I guess would be
considered part of a PCIe Root Complex.

> Internally, it is called as "VMD domain" and each VMD Domain is presented as
> separate pci root bus.
> 
> The original naming is presented in the sysfs links:
> - "domain" -> root bus 0
> = "domain1" -> root bus 1
> 
> i.e:
> domain -> pci10000:e0/pci_bus/10000:e0
> domain1 -> pci10000:e1/pci_bus/10000:e1

That's fine, I'm not suggesting any sysfs change to this.

Bjorn