| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aN9iKtxzv83Y/qvy@yilunxu-OptiPlex-7050>
Date: Fri, 3 Oct 2025 13:42:02 +0800
From: Xu Yilun <yilun.xu@...ux.intel.com>
To: dan.j.williams@...el.com
Cc: linux-coco@...ts.linux.dev, linux-pci@...r.kernel.org,
yilun.xu@...el.com, baolu.lu@...ux.intel.com,
zhenzhong.duan@...el.com, aneesh.kumar@...nel.org,
bhelgaas@...gle.com, aik@....com, linux-kernel@...r.kernel.org,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 2/3] PCI/IDE: Add Address Association Register setup for
RP
> Per-above, just drop the 64-bit policy and assumption. It will naturally
> fail if the required number of address associations is insufficient.
> I.e. either we are in the AMD situation and no amount of address
> association is required, or we are in the ARM / Intel situation where it
> assigns memory then prefetch-memory (if both are present). If both of
Intel can't assign both memory now.
In my patch, the new policy only applies to pci_ide_stream_setup(rp)
which TDX won't use. pci_ide_stream_alloc() just listed the 2 memory
ranges regardless the actuall number of address association blocks.
That's why I said TDX is not the user of the new policy.
> those are required and the hardware only supports 1 address association
> then that hardware vendor is responsible for figuring out a quirk.
But if we want the address association register setting all controlled by
PCI IDE core, TDX is the practical problem and needs a quirk. I see there
is only 1 address association block for RP in my test ENV, and the test
device requires perf memory to be IDE protected and later private
assigned.
> Otherwise Linux expects that any hardware that requires address
> association always produces at least 2 address association blocks at the
> root port, or otherwise arranges for only one memory window type to be
> active.
>
> > /*
> > * Setup control register early for devices that expect
> > * stream_id is set during key programming.
> > @@ -445,7 +500,7 @@ EXPORT_SYMBOL_GPL(pci_ide_stream_setup);
> > void pci_ide_stream_teardown(struct pci_dev *pdev, struct pci_ide *ide)
> > {
> > struct pci_ide_partner *settings = pci_ide_to_settings(pdev, ide);
> > - int pos;
> > + int pos, i;
> >
> > if (!settings)
> > return;
> > @@ -453,6 +508,13 @@ void pci_ide_stream_teardown(struct pci_dev *pdev, struct pci_ide *ide)
> > pos = sel_ide_offset(pdev, settings);
> >
> > pci_write_config_dword(pdev, pos + PCI_IDE_SEL_CTL, 0);
> > +
> > + for (i = 0; i < pdev->nr_ide_mem; i++) {
> > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_1(i), 0);
> > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_2(i), 0);
> > + pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_3(i), 0);
> > + }
>
> Hmm, if we are going to clear all on stop then probably should also
> clear all unused on setup just to be consistent.
>
> > +
> > pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_2, 0);
> > pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_1, 0);
> > settings->setup = 0;
> > --
> > 2.25.1
> >
>
> Here are the proposed incremental changes addressing the above. The new
> pci_ide_stream_to_regs() helper can later be exported to TSM drivers
> that need a formatted copy of the register settings. I prefer that to
> exporting the internals (the PREP_() macros for register setup and the
> pci_ide_domain()).
I get the answer to my question in Patch #1.
[...]
> @@ -157,13 +157,13 @@ struct pci_ide *pci_ide_stream_alloc(struct pci_dev *pdev)
> {
> /* EP, RP, + HB Stream allocation */
> struct stream_index __stream[PCI_IDE_HB + 1];
> + struct pci_bus_region pref_assoc = { 0, -1 };
> + struct pci_bus_region mem_assoc = { 0, -1 };
> + struct resource *res, *mem, *pref;
> struct pci_host_bridge *hb;
> + int num_vf, rid_end;
> struct pci_dev *rp;
> struct pci_dev *br;
> - int num_vf, rid_end;
> - struct range mem32 = {}, mem64 = {};
> - struct pci_bus_region region;
> - struct resource *res;
>
> if (!pci_is_pcie(pdev))
> return NULL;
> @@ -214,18 +214,20 @@ struct pci_ide *pci_ide_stream_alloc(struct pci_dev *pdev)
> if (!br)
> return NULL;
>
> - res = &br->resource[PCI_BRIDGE_MEM_WINDOW];
> - if (res->flags & IORESOURCE_MEM) {
> - pcibios_resource_to_bus(br->bus, ®ion, res);
> - mem32.start = region.start;
> - mem32.end = region.end;
> - }
> -
> - res = &br->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
> - if (res->flags & IORESOURCE_PREFETCH) {
> - pcibios_resource_to_bus(br->bus, ®ion, res);
> - mem64.start = region.start;
> - mem64.end = region.end;
> + /*
> + * Check if the device consumes memory and/or prefetch-memory. Setup
> + * downstream address association ranges for each.
> + */
> + mem = pci_resource_n(br, PCI_BRIDGE_MEM_WINDOW);
> + pref = pci_resource_n(br, PCI_BRIDGE_PREF_MEM_WINDOW);
> + pci_dev_for_each_resource(pdev, res) {
> + if (resource_assigned(mem) && resource_contains(mem, res) &&
We still need to cover all sub-functions of the dsm device, only
check the need of the dsm device is not enough. But if we check all
functions, we don't have to check then.
[...]
> +/**
> + * pci_ide_stream_to_regs() - convert IDE settings to association register values
> + * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> + * @ide: registered IDE settings descriptor
> + * @regs: output register values
> + */
> +static void pci_ide_stream_to_regs(struct pci_dev *pdev, struct pci_ide *ide,
> + struct pci_ide_regs *regs)
> +{
> + struct pci_ide_partner *settings = pci_ide_to_settings(pdev, ide);
> + int pos, assoc_idx = 0;
> +
> + memset(regs, 0, sizeof(*regs));
> +
> + if (!settings)
> + return;
>
> - val = PREP_PCI_IDE_SEL_ADDR1(mem->start, mem->end);
> - pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_1(assoc_idx), val);
> + pos = sel_ide_offset(pdev, settings);
> +
> + regs->rid_1 = FIELD_PREP(PCI_IDE_SEL_RID_1_LIMIT, settings->rid_end);
> +
> + regs->rid_2 = FIELD_PREP(PCI_IDE_SEL_RID_2_VALID, 1) |
> + FIELD_PREP(PCI_IDE_SEL_RID_2_BASE, settings->rid_start) |
> + FIELD_PREP(PCI_IDE_SEL_RID_2_SEG, pci_ide_domain(pdev));
> +
> + if (pdev->nr_ide_mem && pci_bus_region_size(&settings->mem_assoc)) {
I image the quirk for TDX is, reset the RP side settings->mem_assoc back
to {0, -1} before calling this function.
> + mem_assoc_to_regs(&settings->mem_assoc, regs, assoc_idx);
> + assoc_idx++;
> + }
>
> - val = FIELD_GET(SEL_ADDR_UPPER, mem->end);
> - pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_2(assoc_idx), val);
> + if (pdev->nr_ide_mem > assoc_idx &&
> + pci_bus_region_size(&settings->pref_assoc)) {
> + mem_assoc_to_regs(&settings->pref_assoc, regs, assoc_idx);
> + assoc_idx++;
> + }
>
> - val = FIELD_GET(SEL_ADDR_UPPER, mem->start);
> - pci_write_config_dword(pdev, pos + PCI_IDE_SEL_ADDR_3(assoc_idx), val);
> + regs->nr_addr = assoc_idx;
> }
Powered by blists - more mailing lists