Re: [PATCH 08/11] PCI/IDE: Add IDE establishment helpers

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

Alexey Kardashevskiy wrote:
> On 6/12/24 09:24, Dan Williams wrote:
> > There are two components to establishing an encrypted link, provisioning
> > the stream in config-space, and programming the keys into the link layer
> > via the IDE_KM (key management) protocol. These helpers enable the
> > former, and are in support of TSM coordinated IDE_KM. When / if native
> > IDE establishment arrives it will share this same config-space
> > provisioning flow, but for now IDE_KM, in any form, is saved for a
> > follow-on change.
> > 
> > With the TSM implementations of SEV-TIO and TDX Connect in mind this
> > abstracts small differences in those implementations. For example, TDX
> > Connect handles Root Port registers updates while SEV-TIO expects System
> > Software to update the Root Port registers. This is the rationale for
> > the PCI_IDE_SETUP_ROOT_PORT flag.
> > 
> > The other design detail for TSM-coordinated IDE establishment is that
> > the TSM manages allocation of stream-ids, this is why the stream_id is
> > passed in to pci_ide_stream_setup().
> > 
> > The flow is:
> > 
> > pci_ide_stream_probe()
> >    Gather stream settings (devid and address filters)
> > pci_ide_stream_setup()
> >    Program the stream settings into the endpoint, and optionally Root Port)
> > pci_ide_enable_stream()
> >    Run the stream after IDE_KM
> > 
> > In support of system administrators auditing where platform IDE stream
> > resources are being spent, the allocated stream is reflected as a
> > symlink from the host-bridge to the endpoint.
> > 
> > Thanks to Wu Hao for a draft implementation of this infrastructure.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> > Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> > Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
[..]
> > @@ -71,3 +74,192 @@ void pci_ide_init(struct pci_dev *pdev)
> >   	pdev->sel_ide_cap = sel_ide_cap;
> >   	pdev->nr_ide_mem = nr_ide_mem;
> >   }
> > +
> > +void pci_init_host_bridge_ide(struct pci_host_bridge *hb)
> > +{
> > +	hb->ide_stream_res =
> > +		DEFINE_RES_MEM_NAMED(0, 0, "IDE Address Association");
> 
> out of curiosity - should it be DEFINE_RES_MEM_NAMED(0, UINT_MAX, "IDE 
> Address Association");?

Hmm, yes, an empty resource only makes sense if there is going to be a
later point in the init flow where it is adjusted to map the real
platform probed boundaries, and I do not have that in this code.

The platform iomem_resource boundaries should be settled before the
host-bridge is initialized, so no need to find a later point than this
to set the bounds. Will fold in this incremental change:

@@ -77,8 +77,13 @@ void pci_ide_init(struct pci_dev *pdev)
 
 void pci_init_host_bridge_ide(struct pci_host_bridge *hb)
 {
-       hb->ide_stream_res =
-               DEFINE_RES_MEM_NAMED(0, 0, "IDE Address Association");
+       /*
+        * Match platform iomem resource boundaries for IDE address
+        * association.
+        */
+       hb->ide_stream_res = DEFINE_RES_MEM_NAMED(
+               iomem_resource.start, resource_size(&iomem_resource),
+               "IDE Address Association");
 }
 
 /*


[..]
> > +void pci_ide_enable_stream(struct pci_dev *pdev, struct pci_ide *ide)
> > +{
> > +	int pos;
> > +	u32 val;
> > +
> > +	pos = sel_ide_offset(pdev->sel_ide_cap, ide->stream_id,
> > +			     pdev->nr_ide_mem);
> > +
> > +	val = FIELD_PREP(PCI_IDE_SEL_CTL_ID_MASK, ide->stream_id) |
> > +	      FIELD_PREP(PCI_IDE_SEL_CTL_DEFAULT, 1);
> 
> 
> FIELD_PREP(PCI_IDE_SEL_CTL_EN) is missing here.

Added.

> And no PCI_IDE_SETUP_ROOT_PORT here, is the platform expected to enable 
> it explicitely? If yes, then we do not need PCI_IDE_SETUP_ROOT_PORT 
> really. If no, then this needs to enable the stream on the rootport.

Hmm, yes, looking at this now, PCI_IDE_SETUP_ROOT_PORT is giving off
"mid-layer" smells and this responsibility should be pushed to the low
level driver.

It is still the case that there is a part of the stream setup that can
fail, like registering the presence of the stream in sysfs, but the
piece that can not fail, __pci_ide_stream_setup(), should be left to the
platform TSM driver to optionally be called for the root-port.

I will rename the parts of the stream setup that needs alloc / free as
"pci_ide_stream_{register,unregister}()", export
__pci_ide_stream_setup() as a standalone helper renamed to just
pci_ide_stream_setup(), and drop the PCI_IDE_SETUP_ROOT_PORT flag
concept.

> Also, my test device wants PCI_IDE_SEL_CTL_TEE_LIMITED to work, I wonder 
> how to showel it in here, add a "unsigned dev_sel_ctl" to pci_ide?
> And the same comment for PCI_IDE_SEL_CTL_CFG_EN on the rootport. 
> "unsigned rootport_sel_ctl"?

I will add that. If the device supports limited stream I see no reason
that Linux would want to not require it (outside of device quirks). So,
the default will be to enable it if supported, but the low-level TSM
driver can always clear that support flag in the pdev (endpoint or root)
if the need arises.

[..]
> > diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
> > new file mode 100644
> > index 000000000000..24e08a413645
> > --- /dev/null
> > +++ b/include/linux/pci-ide.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> > +
> > +/* PCIe 6.2 section 6.33 Integrity & Data Encryption (IDE) */
> > +
> > +#ifndef __PCI_IDE_H__
> > +#define __PCI_IDE_H__
> > +
> > +#include <linux/range.h>
> > +
> > +struct pci_ide {
> > +	int domain;
> 
> Not much point in caching this, real easy to calculate in that one spot.

Agree, added an ide_domain_nr() helper to meet the requirement that
software must write 0 if the device has not captured its segment base.

> Thanks,
> 
> ps. I'll probably be commenting more on the same things as I try using 
> all this :)

Hey, yes please! The whole point of this effort is to find a path to
mutual acceptance on all these concerns.