| 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:
<SN7PR12MB7201E0303AE7A1469A9BC62F8BF12@SN7PR12MB7201.namprd12.prod.outlook.com>
Date: Fri, 7 Feb 2025 16:45:58 +0000
From: "Havalige, Thippeswamy" <thippeswamy.havalige@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
<lpieralisi@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jingoohan1@...il.com" <jingoohan1@...il.com>, "Simek, Michal"
<michal.simek@....com>, "Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>
Subject: RE: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Wednesday, February 5, 2025 7:51 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@....com>
> Cc: bhelgaas@...gle.com; lpieralisi@...nel.org; kw@...ux.com;
> manivannan.sadhasivam@...aro.org; robh@...nel.org; krzk+dt@...nel.org;
> conor+dt@...nel.org; linux-pci@...r.kernel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; jingoohan1@...il.com; Simek, Michal
> <michal.simek@....com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@....com>
> Subject: Re: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
>
> On Wed, Feb 05, 2025 at 11:53:53AM +0000, Havalige, Thippeswamy wrote:
> > > -----Original Message-----
> > > From: Havalige, Thippeswamy
> > > Sent: Wednesday, February 5, 2025 5:08 PM
> > > To: Bjorn Helgaas <helgaas@...nel.org>
> > > Cc: bhelgaas@...gle.com; lpieralisi@...nel.org; kw@...ux.com;
> > > manivannan.sadhasivam@...aro.org; robh@...nel.org;
> krzk+dt@...nel.org;
> > > conor+dt@...nel.org; linux-pci@...r.kernel.org;
> devicetree@...r.kernel.org;
> > > linux-kernel@...r.kernel.org; jingoohan1@...il.com; Simek, Michal
> > > <michal.simek@....com>; Gogada, Bharat Kumar
> > > <bharat.kumar.gogada@....com>
> > > Subject: RE: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port
> driver
>
> > > > > > It's kind of weird that these skip the odd-numbered bits,
> > > > > > since dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> > > > > > amd_mdb_unmask_intx_irq() only use bits 19:16. Something
> > > > > > seems wrong and needs either a fix or a comment about why
> > > > > > this is the way it is.
> > > > >
> > > > > ... the odd bits are meant for deasserting inta, intb intc &
> > > > > intd I ll include this in my next patch
>
> Tangent: I don't know what "deassert" would mean here, since INTx is
> an *incoming* interrupt and the Root Port is the receiver and can mask
> or acknowledge the interrupt but not really deassert it.
>
Yes, I agree but our controller, however, provides explicit tracking of both transitions using internal registers
(TLP_IR_STATUS_MISC).
> > > > > > > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > > > > > > +#define AMD_MDB_PCIE_IMR_ALL_MASK \
> > > > > > > + ( \
> > > > > > > + IMR(CMPL_TIMEOUT) | \
> > > > > > > + IMR(INTA_ASSERT) | \
> > > > > > > + IMR(INTB_ASSERT) | \
> > > > > > > + IMR(INTC_ASSERT) | \
> > > > > > > + IMR(INTD_ASSERT) | \
> > > > > > > + IMR(PM_PME_RCVD) | \
> > > > > > > + IMR(PME_TO_ACK_RCVD) | \
> > > > > > > + IMR(MISC_CORRECTABLE) | \
> > > > > > > + IMR(NONFATAL) | \
> > > > > > > + IMR(FATAL) \
> > > > > > > + )
> > > > > > > +
> > > > > > > +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16)
> > > > > >
> > > > > > I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just
> > > > > > use AMD_MDB_TLP_PCIE_INTX_MASK in the
> > > > > > AMD_MDB_PCIE_IMR_ALL_MASK definition.
> > > > > >
> > > > > > If there are really eight bits of INTx-related things here
> > > > > > for the four INTx interrupts, I think you should make two
> > > > > > #defines to separate them out.
> > > >
> > > > > Yes, there are 8 intx related bits I ll define them in my next
> > > > > patch. I was in confusion here regarding "PCI_NUM_INTX "
> > > > > since this macro indicates INTA INTB INTC INTD bits so I
> > > > > discarded deassert bits here.
> > > >
> > > > It seems like what you have is a single 8-bit field that
> > > > contains both assert and deassert info, interspersed.
> > > > GENMASK()/FIELD_GET() isn't enough to really separate them.
> > > > Maybe you can do something like this:
> > > >
> > > > #define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16)
> > > >
> > > > #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT(1 << x)
> > > >
> > > > If you don't need the deassert bits, a comment would be useful,
> > > > but there's no point in adding a #define for them. If you do
> > > > need them, maybe this:
> > > >
> > > > #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x) BIT((1 << x) + 1)
> > > >
> > > > > > > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args) {
> > > > > > > + struct amd_mdb_pcie *pcie = args;
> > > > > > > + unsigned long val;
> > > > > > > + int i;
> > > > > > > +
> > > > > > > + val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > > > > > + pcie_read(pcie,
> AMD_MDB_TLP_IR_STATUS_MISC));
> > > > > > > +
> > > > > > > + for_each_set_bit(i, &val, 4)
> > > > > >
> > > > > > for_each_set_bit(..., PCI_NUM_INTX)
> > > >
> > > > > In next patch I will update value to 8 here.
> > > >
> > > > And here you could do:
> > > >
> > > > val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > > > pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > > >
> > > > for (i = 0; i < PCI_NUM_INTX; i++) {
> > > > if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
>
> > > This condition never met observing zero here.
>
The val variable will hold the bits that are currently set. For example, if the INTA bit is set (10000 in binary),
then val will have a value of 1 after applying FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC)).
Issue with these Macros:
#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT(x << 1)
#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x) BIT((x << 1) + 1)
When x = 0, the condition inside the loop:
if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
expands to:
if (val & BIT(1 << 0))
Since 1 << 0 evaluates to 1, this becomes:
if (val & BIT(1))
If val = 1, this results in 1 & BIT(1), which evaluates to 0, meaning the condition is never satisfied.
> > To satisfy this condition need to modify macros as following.
> > #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT(x)
> > #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x) BIT(x+1)
>
> Maybe I don't understand how the assert/deassert bits are laid out in
> the register.
>
> The original patch has this:
>
> +#define AMD_MDB_PCIE_INTR_INTA_ASSERT 16
> +#define AMD_MDB_PCIE_INTR_INTB_ASSERT 18
> +#define AMD_MDB_PCIE_INTR_INTC_ASSERT 20
> +#define AMD_MDB_PCIE_INTR_INTD_ASSERT 22
>
> and if the odd bits are for deassert I thought that meant they would
> look like this:
>
> #define AMD_MDB_PCIE_INTR_INTA_DEASSERT 17
> #define AMD_MDB_PCIE_INTR_INTB_DEASSERT 19
> #define AMD_MDB_PCIE_INTR_INTC_DEASSERT 21
> #define AMD_MDB_PCIE_INTR_INTD_DEASSERT 23
>
Yes, your correct. ASSERT & DEASSERT bits in the register are laid in the same way.
> +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16)
>
> If we extract AMD_MDB_TLP_PCIE_INTX_MASK with FIELD_GET(),
> the field gets shifted right by 16, so we should end up with
> something like this:
>
> INTA assert 0000 0001 == BIT(0)
> INTA deassert 0000 0010 == BIT(1)
> INTB assert 0000 0100 == BIT(2)
> INTB deassert 0000 1000 == BIT(3)
> INTC assert 0001 0000 == BIT(4)
> INTC deassert 0010 0000 == BIT(5)
> INTD assert 0100 0000 == BIT(6)
> INTD deassert 1000 0000 == BIT(7)
>
> But maybe that's not how they're actually laid out?
>
> I think the argument to AMD_MDB_PCIE_INTR_INTX_ASSERT() should
> be the hwirq (0..3 for INTA..INTD), so if we use
>
> #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT(x)
> #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x) BIT(x+1)
>
> as you propose, don't the assert/deassert bits collide?
>
> AMD_MDB_PCIE_INTR_INTX_ASSERT(0) == BIT(0) for INTA assert
> AMD_MDB_PCIE_INTR_INTX_ASSERT(1) == BIT(1) for INTB assert
Your analysis is correct-using above macros does not yield the expected results
>
> AMD_MDB_PCIE_INTR_INTX_DEASSERT(0) == BIT(1) for INTA deassert
>
> > > > generic_handle_domain_irq(pcie->intx_domain, i);
> > > >
> > > > > > > + generic_handle_domain_irq(pcie->intx_domain, i);
To ensure proper handling of INTx interrupts, I plan to use the following macros:
#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT(x * 2)
#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x) BIT(x * 2 + 1)
With this approach, the conditions will be evaluated as follows:
For an assert bit:
if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
Example:
AMD_MDB_PCIE_INTR_INTX_ASSERT(0) -> BIT(0 * 2) -> BIT(0),
If val = 1, then 1 & BIT(0) is true, and the interrupt will be handled.
For a deassert bit:
if (val & AMD_MDB_PCIE_INTR_INTX_DEASSERT(i))
Example:
AMD_MDB_PCIE_INTR_INTX_DEASSERT(0) -> BIT((0 * 2) + 1) -> BIT(1),
If val = 2, then 2 & BIT(1) is true, indicating that the deassert condition is met.
To incorporate this logic, I propose updating the loop as follows:
for_each_set_bit(i, &val, 8) {
if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
generic_handle_domain_irq(pcie->intx_domain, i);
if (val & AMD_MDB_PCIE_INTR_INTX_DEASSERT(i))
generic_handle_domain_irq(pcie->intx_domain, i);
}
This ensures that both assert and deassert conditions are handled correctly.
Let me know if you have any suggestions or correct me if am missing anything.
Regards,
Thippeswamy H
Powered by blists - more mailing lists