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@xxxxxxxxxx>
> > Cc: Lukas Wunner <lukas@xxxxxxxxx>
> > Cc: Samuel Ortiz <sameo@xxxxxxxxxxxx>
> > Co-developed-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > Co-developed-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
[..]
> > @@ -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.
