Re: [PATCH v2 08/11] PCI/IDE: Add IDE establishment helpers

[Dan Williams <dan.j.williams@intel.com>]

Aneesh Kumar K.V wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > There are two components to establishing an encrypted link, provisioning
> > the stream in Partner Port config-space, and programming the keys into
> > the link layer via IDE_KM (IDE Key Management). This new library,
> > drivers/pci/ide.c, enables the former. IDE_KM, via a TSM low-level
> > driver, is saved for later.
> >
> ....
> 
> > +/**
> > + * pci_ide_stream_setup() - program settings to Selective IDE Stream registers
> > + * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> > + * @ide: registered IDE settings descriptor
> > + *
> > + * When @pdev is a PCI_EXP_TYPE_ENDPOINT then the PCI_IDE_EP partner
> > + * settings are written to @pdev's Selective IDE Stream register block,
> > + * and when @pdev is a PCI_EXP_TYPE_ROOT_PORT, the PCI_IDE_RP settings
> > + * are selected.
> > + */
> > +void pci_ide_stream_setup(struct pci_dev *pdev, struct pci_ide *ide)
> > +{
> > +	struct pci_ide_partner *settings = to_settings(pdev, ide);
> > +	int pos;
> > +	u32 val;
> > +
> > +	if (!settings)
> > +		return;
> > +
> > +	pos = sel_ide_offset(pdev->nr_link_ide, settings->stream_index,
> > +			     pdev->nr_ide_mem);
> >
> 
> This and the similar offset caclulation below needs the EXT_CAP_ID_IDE offset 

*facepalm*

So it seems no one is trying to build on top of this framework yet.

> 
> modified   drivers/pci/ide.c
> @@ -10,11 +10,13 @@
>  #include <linux/bitfield.h>
>  #include "pci.h"
>  
> -static int sel_ide_offset(int nr_link_ide, int stream_index, int nr_ide_mem)
> +static int sel_ide_offset(struct pci_dev *pdev, int nr_link_ide,
> +			  int stream_index, int nr_ide_mem)
>  {
>  	int offset;
>  
> -	offset = PCI_IDE_LINK_STREAM_0 + nr_link_ide * PCI_IDE_LINK_BLOCK_SIZE;
> +	offset = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_IDE);
> +	offset += PCI_IDE_LINK_STREAM_0 + nr_link_ide * PCI_IDE_LINK_BLOCK_SIZE;

Will fix this up to the following since ide_cap is already cached:

static int __sel_ide_offset(int ide_cap, int nr_link_ide, int stream_index,
                            int nr_ide_mem)
{
        int offset;
        
        offset = ide_cap + PCI_IDE_LINK_STREAM_0 + nr_link_ide * PCI_IDE_LINK_BLOCK_SIZE;
        
        /*
         * Assume a constant number of address association resources per
         * stream index
         */
        if (stream_index > 0)
                offset += stream_index * PCI_IDE_SEL_BLOCK_SIZE(nr_ide_mem);
        return offset;
}

static int sel_ide_offset(struct pci_dev *pdev,
                          struct pci_ide_partner *settings)
{
        return sel_ide_offset(pdev->ide_cap, pdev->nr_link_ide,
                              settings->stream_index, pdev->nr_ide_mem);
}

[..]
> > +/**
> > + * pci_ide_stream_enable() - after setup, enable the stream
> > + * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> > + * @ide: registered and setup IDE settings descriptor
> > + *
> > + * Activate the stream by writing to the Selective IDE Stream Control Register.
> > + */
> > +void pci_ide_stream_enable(struct pci_dev *pdev, struct pci_ide *ide)
> > +{
> > +	struct pci_ide_partner *settings = to_settings(pdev, ide);
> > +	int pos;
> > +	u32 val;
> > +
> > +	if (!settings)
> > +		return;
> > +
> > +	pos = sel_ide_offset(pdev->nr_link_ide, settings->stream_index,
> > +			     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_CFG_EN, pdev->ide_cfg) |
> > +	      FIELD_PREP(PCI_IDE_SEL_CTL_TEE_LIMITED, pdev->ide_tee_limit) |
> >
> 
> Does enabling pdev->ide_tee_limit here will prevent a device from operating 
> as expected before we get to TDISP RUN state? 

My expectation is that non-IDE TLPs can always be sent. I.e. a TDISP
device outside the RUN state should still be operational without needing
to send T=0 traffic over the IDE stream.