| 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: <689270bf9aa67_55f09100a0@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 5 Aug 2025 13:59:43 -0700
From: <dan.j.williams@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Dan Williams
<dan.j.williams@...el.com>
CC: <linux-coco@...ts.linux.dev>, <linux-pci@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>, <aik@....com>,
<lukas@...ner.de>, Yilun Xu <yilun.xu@...el.com>, Aneesh Kumar K.V
<aneesh.kumar@...nel.org>
Subject: Re: [PATCH v4 02/10] PCI/IDE: Enumerate Selective Stream IDE
capabilities
Jonathan Cameron wrote:
[..]
> > diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> > new file mode 100644
> > index 000000000000..e15937cdb2a4
> > --- /dev/null
> > +++ b/drivers/pci/ide.c
> > @@ -0,0 +1,93 @@
>
> > +static int __sel_ide_offset(u16 ide_cap, u8 nr_link_ide, u8 stream_index,
> > + u8 nr_ide_mem)
> > +{
> > + u32 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
> > + */
> > + offset += stream_index * PCI_IDE_SEL_BLOCK_SIZE(nr_ide_mem);
> > + return offset;
>
> return offset + stream_index * PCI_IDE_SEL_BLOCK_SIZE(nr_ide_mem);
>
> is perhaps a little bit neater?
Sure.
>
> > +}
>
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index a3a3e942dedf..ab4ebf0f8a46 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > +/* Integrity and Data Encryption Extended Capability */
> > +#define PCI_IDE_CAP 0x4
>
> Spec uses two digits. Things are a bit inconsistent in this file but
> 0x04 looks like the most common syntax if hex. Curiously some are
> not in hex. Anyhow, I'd go with 0x04 etc for all register offsets
> unless Bjorn or someone else shouts otherwise!
While I'm here, might as well.
> > +#define PCI_IDE_CAP_LINK 0x1 /* Link IDE Stream Supported */
> > +#define PCI_IDE_CAP_SELECTIVE 0x2 /* Selective IDE Streams Supported */
> > +#define PCI_IDE_CAP_FLOWTHROUGH 0x4 /* Flow-Through IDE Stream Supported */
> > +#define PCI_IDE_CAP_PARTIAL_HEADER_ENC 0x8 /* Partial Header Encryption Supported */
> > +#define PCI_IDE_CAP_AGGREGATION 0x10 /* Aggregation Supported */
> > +#define PCI_IDE_CAP_PCRC 0x20 /* PCRC Supported */
> > +#define PCI_IDE_CAP_IDE_KM 0x40 /* IDE_KM Protocol Supported */
> > +#define PCI_IDE_CAP_SEL_CFG 0x80 /* Selective IDE for Config Request Support */
> > +#define PCI_IDE_CAP_ALG_MASK __GENMASK(12, 8) /* Supported Algorithms */
> > +#define PCI_IDE_CAP_ALG_AES_GCM_256 0 /* AES-GCM 256 key size, 96b MAC */
> Looking at the rest of this file I think this should be.
> #define PCI_IDE_CAP_ALG_MASK __GENMASK(12, 8) /* Supported Algorithms */
> #define PCI_IDE_CAP_ALG_AES_GCM_256 0 /* AES-GCM 256 key size, 96b MAC */
>
> So indent one more space. Example being PCI_LPH_LOC_NONE
ok.
>
> > +#define PCI_IDE_CAP_LINK_TC_NUM_MASK __GENMASK(15, 13) /* Link IDE TCs */
> > +#define PCI_IDE_CAP_SEL_NUM_MASK __GENMASK(23, 16)/* Supported Selective IDE Streams */
>
> Space before comment missing?
Got it.
>
> > +#define PCI_IDE_CAP_TEE_LIMITED 0x1000000 /* TEE-Limited Stream Supported */
> > +#define PCI_IDE_CTL 0x8
> As above 0x08 more consistent with rest of the file. Same for remaining cases.
> > +#define PCI_IDE_CTL_FLOWTHROUGH_IDE 0x4 /* Flow-Through IDE Stream Enabled */
> > +
> > +#define PCI_IDE_LINK_STREAM_0 0xc /* First Link Stream Register Block */
> > +#define PCI_IDE_LINK_BLOCK_SIZE 8
> > +/* Link IDE Stream block, up to PCI_IDE_CAP_LINK_TC_NUM */
> > +#define PCI_IDE_LINK_CTL_0 0x0 /* First Link Control Register Offset in block */
>
> Event this I think should be 0x01 for consistency
You mean 0x00, right?
>
> > +#define PCI_IDE_LINK_CTL_EN 0x1 /* Link IDE Stream Enable */
> > +#define PCI_IDE_LINK_CTL_TX_AGGR_NPR_MASK __GENMASK(3, 2) /* Tx Aggregation Mode NPR */
> > +#define PCI_IDE_LINK_CTL_TX_AGGR_PR_MASK __GENMASK(5, 4) /* Tx Aggregation Mode PR */
> > +#define PCI_IDE_LINK_CTL_TX_AGGR_CPL_MASK __GENMASK(7, 6) /* Tx Aggregation Mode CPL */
> > +#define PCI_IDE_LINK_CTL_PCRC_EN 0x100 /* PCRC Enable */
> > +#define PCI_IDE_LINK_CTL_PART_ENC_MASK __GENMASK(13, 10) /* Partial Header Encryption Mode */
> > +#define PCI_IDE_LINK_CTL_ALG_MASK __GENMASK(18, 14) /* Selection from PCI_IDE_CAP_ALG */
> > +#define PCI_IDE_LINK_CTL_TC_MASK __GENMASK(21, 19) /* Traffic Class */
> > +#define PCI_IDE_LINK_CTL_ID_MASK __GENMASK(31, 24) /* Stream ID */
> > +#define PCI_IDE_LINK_STS_0 0x4 /* First Link Status Register Offset in block */
> > +#define PCI_IDE_LINK_STS_STATE __GENMASK(3, 0) /* Link IDE Stream State */
> > +#define PCI_IDE_LINK_STS_RECVD_INTEGRITY_CHECK 0x80000000 /* Received Integrity Check Fail Msg */
> Naming here is drawing on stuff not in the Status register description (in 6.2 anyway which is what I'm
> checking against). That just calls this Received IDE Fail Message.
> The text else where calls it out 'Upon transition from Secure to Insecure for any reason, other than
> corresponding Link/Selective IDE Stream Enable bit is Cleared, for a given Stream, the Port must transmit an
> IDE Fail Message indicating the Stream ID to the Partner port'
>
> To me the integrity check naming doesn't really cover that.
>
> I did some minimal digging. Your text matches 6.0.
Will update to:
#define PCI_IDE_LINK_STS_IDE_FAIL 0x80000000 /* IDE fail message received */
...and same for selective.
[ snip the other occurences of 0-padding register offsets ]
Powered by blists - more mailing lists