linux-kernel - Re: [PATCH 2/3] PCI/IDE: Add Address Association Register setup for RP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68e0136bd9bcf_1992810012@dwillia2-mobl4.notmuch>
Date: Fri, 3 Oct 2025 11:18:19 -0700
From: <dan.j.williams@...el.com>
To: Xu Yilun <yilun.xu@...ux.intel.com>, <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

Xu Yilun wrote:
> > 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.

The address association setting is not *controlled* by the PCI IDE core,
it is simply *initialized* by the PCI IDE core. The PCI IDE core should
always be a library, not a mid-layer when it comes to policy. So, if TDX
knows that only prefetchable memory will ever be protected then it can
do the following:

        struct pci_ide *ide __free(pci_ide_stream_release) =
                pci_ide_stream_alloc(pdev);
        if (!ide)
                return -ENOMEM;
	...
        rp_settings = pci_ide_to_settings(rp, ide);
        /* only support address association for prefetchable memory */
        rp_settings->mem_assoc = { 0, -1 };

[..]
> > +	/*
> > +	 * 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.

True, good point, no real need to limit the stream just based on the DSM
device just do:

        if (resource_assigned(mem))
                pcibios_resource_to_bus(br->bus, &mem_assoc, mem);
        if (resource_assigned(pref))
                pcibios_resource_to_bus(br->bus, &pref_assoc, pref);

I was hoping that limiting it to the bridge windows that are used might
naturally result in the non-prefetchable memory window dropping out.
However, better to associate all downstream memory by default and let
the TSM driver trim associations it does not want to support.

[..]
> > + * 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.

Oh, yup, you predicted my response above.

Now, I worry that some device will need its memory window protected in
addition to its prefetch window, but that is an architecture limitation
that the TDX Module will need to solve when / if it happens.