linux-kernel - Re: [External] Re: [PATCH v2 2/3] firmware: introduce FFI for SMBIOS entry.

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: <CAEEQ3wmBSwn80kA_cnw766P4Q03tE6_TVtkndVeUsRrPqt0KyQ@mail.gmail.com>
Date:   Mon, 3 Jul 2023 21:26:53 +0800
From:   运辉崔 <cuiyunhui@...edance.com>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     Conor Dooley <conor@...nel.org>, ardb@...nel.org,
        palmer@...belt.com, paul.walmsley@...ive.com,
        aou@...s.berkeley.edu, linux-riscv@...ts.infradead.org,
        rminnich@...il.com, mark.rutland@....com, lpieralisi@...nel.org,
        rafael@...nel.org, lenb@...nel.org, jdelvare@...e.com,
        yc.hung@...iatek.com, angelogioacchino.delregno@...labora.com,
        allen-kh.cheng@...iatek.com, pierre-louis.bossart@...ux.intel.com,
        tinghan.shen@...iatek.com, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, geshijian@...edance.com,
        weidong.wd@...edance.com
Subject: Re: [External] Re: [PATCH v2 2/3] firmware: introduce FFI for SMBIOS entry.

Hi Conor,

On Mon, Jul 3, 2023 at 9:03 PM Conor Dooley <conor.dooley@...rochip.com> wrote:
>
> On Mon, Jul 03, 2023 at 08:41:30PM +0800, 运辉崔 wrote:
> > On Mon, Jul 3, 2023 at 4:36 PM Conor Dooley <conor.dooley@...rochip.com> wrote:
> > > On Mon, Jul 03, 2023 at 04:23:53PM +0800, 运辉崔 wrote:
>
> > > > > > +FDT FIRMWARE INTERFACE (FFI)
> > > > > > +M:   Yunhui Cui cuiyunhui@...edance.com
> > > > > > +S:   Maintained
> > > > > > +F:   drivers/firmware/ffi.c
> > > > > > +F:   include/linux/ffi.h
> > > > >
> > > > > Are you going to apply patches for this, or is someone else?
> > > > Yes,  it will be used by patch 3/3.
> > >
> > > That's not what I asked :(
> >
> > Sorry,  ok,  what do you want to ask?
>
> Who is going to apply patches for drivers/firmware/ffi*?

I'll update to v3:
F:   drivers/firmware/ffi*
F:   include/linux/ffi*
And I'll plan to maintain them.
But now,  I don't know how to apply the patches. Could you give some
suggestions?

>
> > > > > >  static void __init dmi_scan_machine(void)
> > > > > > @@ -660,58 +686,22 @@ static void __init dmi_scan_machine(void)
> > > > > >       char __iomem *p, *q;
> > > > > >       char buf[32];
> > > > > >
> > > > > > +#ifdef CONFIG_FDT_FW_INTERFACE
> > > > > > +     if (dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> > > > >
> > > > > "dmi_sacn_smbios"
> > > > >
> > > > > > +             goto error;
> > > > > > +#endif
> > > > >
> > > > > Does this not mean that if FDT_FW_INTERFACE is enabled, but the platform
> > > > > wants to use EFI, it won't be able to? The `goto error;` makes this look
> > > > > mutually exclusive to my efi-unaware eyes.
> > > >
> > > > If you have enabled FFI, then if something goes wrong, you should goto error.
> > > > Just like the origin code:
> > > >         if (efi_enabled(EFI_CONFIG_TABLES)) {
> > > >                 if (dmi_sacn_smbios(efi.smbios3, efi.smbios))
> > > >                         goto error;
> > > >         } else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK)) {
> > > >                 p = dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, 0x10000);
> > > >                 if (p == NULL)
> > > >                         goto error;
> > >
> > > Does this not make FFI and EFI mutually exclusive Kconfig options?
> > > Suppose you are on a system that does not implement FFI, but does
> > > implement EFI - what's going to happen then?
> > > AFAICT, dmi_sacn_smbios(ffi.smbios3, ffi.smbios) will fail & you'll do a
> > > `goto error` & skip the EFI code. What am I missing?
> >
> > Code is not intended to be mutually exclusive, get the correct value and return,
> > The code is going to be changed to this:
> >
> > #ifdef CONFIG_FDT_FW_INTERFACE
>
> Ideally, these would be IS_ENABLED() instead of #ifdef - but if you copy
> what EFI does, then you don't need either, as there will always be an
> ffi_enabled() defined.

Okay， this can refer to the code of EFI. :)

>
> >         if (ffi_enabled(FFI_CONFIG_TABLES)) {
>
> I don't know what this function is, but this code seems like a step in
> the right direction.

Okay, I'll update it on v3.

>
> >                 if (!dmi_sacn_smbios(ffi.smbios3, ffi.smbios))
> >                         return;
> >         }
> > #endif
>
> Thanks,
> Conor.

Thanks,
Yunhui