linux-kernel - Re: [PATCH v4 07/23] mailbox: Add RISC-V SBI message proxy (MPXY) based mailbox driver

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aEc-SHvL187xdj-m@smile.fi.intel.com>
Date: Mon, 9 Jun 2025 23:04:24 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Anup Patel <apatel@...tanamicro.com>
Cc: Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jassi Brar <jassisinghbrar@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Palmer Dabbelt <palmer@...belt.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Len Brown <lenb@...nel.org>, Sunil V L <sunilvl@...tanamicro.com>,
	Rahul Pathak <rpathak@...tanamicro.com>,
	Leyfoon Tan <leyfoon.tan@...rfivetech.com>,
	Atish Patra <atish.patra@...ux.dev>,
	Andrew Jones <ajones@...tanamicro.com>,
	Samuel Holland <samuel.holland@...ive.com>,
	Anup Patel <anup@...infault.org>, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/23] mailbox: Add RISC-V SBI message proxy (MPXY)
 based mailbox driver

On Mon, Jun 09, 2025 at 05:59:40PM +0530, Anup Patel wrote:
> On Wed, May 28, 2025 at 4:23 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> > On Sun, May 25, 2025 at 02:16:54PM +0530, Anup Patel wrote:

...

> > > +#include <asm/sbi.h>
> >
> > asm/* usually goes after generic linux/* ones. Why here?
> 
> I am not aware of any such convention but I will update anyway.

It's just a common sense. We include most generic first and most custom at
last.

...

> > > +static int mpxy_write_attrs(u32 channel_id, u32 base_attrid, u32 attr_count,
> > > +                         u32 *attrs_buf)
> > > +{
> > > +     struct mpxy_local *mpxy = this_cpu_ptr(&mpxy_local);
> > > +     struct sbiret sret;
> > > +     u32 i;
> > > +
> > > +     if (!mpxy->shmem_active)
> > > +             return -ENODEV;
> > > +     if (!attr_count || !attrs_buf)
> > > +             return -EINVAL;
> > > +
> > > +     get_cpu();
> > > +
> > > +     for (i = 0; i < attr_count; i++)
> > > +             ((__le32 *)mpxy->shmem)[i] = cpu_to_le32(attrs_buf[i]);
> >
> > Don't we have helpers for this? They are suffixed with _array.
> > https://elixir.bootlin.com/linux/v6.15-rc6/source/include/linux/byteorder/generic.h#L168
> > Don't forget to have asm/byteorder.h being included.
> >
> > Ditto for the similar case(s).
> 
> The cpu_to_le32_array() and le32_to_cpu_array() helpers update data
> in-place but over here we have separate source and destination.

Fair enough. Perhaps add something like memcpy_to_le32() / memcpy_from_le32()
or alike for your case?

> > > +     sret = sbi_ecall(SBI_EXT_MPXY, SBI_EXT_MPXY_WRITE_ATTRS,
> > > +                      channel_id, base_attrid, attr_count, 0, 0, 0);
> > > +
> > > +     put_cpu();
> > > +     return sbi_err_map_linux_errno(sret.error);
> > > +}

...

> > > +                            sizeof(mchan->rpmi_attrs) / sizeof(u32),
> > > +                            (u32 *)&mchan->rpmi_attrs);
> >
> > Why casting? What about alignment?
> 
> The RPMI attributes (aka struct sbi_mpxy_rpmi_channel_attrs) are
> a collection of u32 attributes hence we can also treat rpmi_attrs
> as a u32 array. Further, the rpmi_attrs is XLEN aligned within the
> struct mpxy_mbox_channel so no alignment issue with the casting
> on both RV32 and RV64.
> 
> If we want to avoid the casting then we will have to use a temporary
> u32 array plus additional memcpy().

OK.

...

> > > +     if (mbox->msi_count)
> >
> > Is this check really needed?
> 
> MSIs are optional for the SBI MPXY mailbox so we should only use
> platform_device_msi_xyz() APIs only when MSIs are available.

> > > +             platform_device_msi_free_irqs_all(mbox->dev);

Hmm... I am not sure why. Do you have any Oops or warnings if the check
is not there and no MSI provided?

> > > +}

-- 
With Best Regards,
Andy Shevchenko