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

[Xu Yilun <yilun.xu@linux.intel.com>]

> 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