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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAK9=C2VjOZ22smYdxDg1bjnx-+wwjngEN3c-iOpdtaADFcQ0+w@mail.gmail.com>
Date: Tue, 10 Jun 2025 10:05:27 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.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 Tue, Jun 10, 2025 at 1:34 AM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> 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?

Okay, I will add memcpy_to_le32() / memcpy_from_le32()
in include/linux/byteorder/generic.h and use it over here.

>
> > > > +     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?

We don't see any oops or warnings. This check is to avoid unnecessary
work (such as acquiring lock, checking default domain, etc) in the
msi_domain_free_irqs_all() called by platform_device_msi_free_irqs_all().

I don't mind dropping the check so I will update in the next revision.

Regards,
Anup