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

Xu Yilun
2025-10-06T03:18:11Z

> 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.

Yeah, good to me.

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

Delete the pos stuff, not used in this function.

> > > +
> > > + 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.

Agree.

I've tested all your changes, work for me.

Thanks,
Yilun