Hi Daniel,
Thanks a lot for your review.
> On 30 Jun 2025, at 8:58 PM, Daniel Kiper <dki...@net-space.pl> wrote:
>
> On Tue, Jun 10, 2025 at 09:20:51PM +0530, Sudhakar wrote:
>> From: Sudhakar Kuppusamy <sudha...@linux.ibm.com>
>>
>> enhancing the infrastructure to enable the Platform Keystore (PKS) feature,
>> which provides access to the SB VERSION, DB, and DBX secure boot variables
>> from PKS.
>>
>> Signed-off-by: Sudhakar Kuppusamy <sudha...@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stef...@linux.ibm.com>
>> Reviewed-by: Avnish Chouhan <avn...@linux.ibm.com>
>> ---
>> grub-core/Makefile.am | 1 +
>> grub-core/Makefile.core.def | 1 +
>> grub-core/kern/powerpc/ieee1275/ieee1275.c | 141 +++++++++++++++++++++
>> include/grub/powerpc/ieee1275/ieee1275.h | 18 +++
>> 4 files changed, 161 insertions(+)
>> create mode 100644 grub-core/kern/powerpc/ieee1275/ieee1275.c
>>
>> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
>> index e50db8106..b516e139b 100644
>> --- a/grub-core/Makefile.am
>> +++ b/grub-core/Makefile.am
>> @@ -241,6 +241,7 @@ KERNEL_HEADER_FILES +=
>> $(top_builddir)/include/grub/machine/kernel.h
>> endif
>>
>> if COND_powerpc_ieee1275
>> +KERNEL_HEADER_FILES +=
>> $(top_srcdir)/include/grub/powerpc/ieee1275/ieee1275.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/ieee1275/ieee1275.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/ieee1275/alloc.h
>> KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/terminfo.h
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 94492c431..52d2fe491 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -328,6 +328,7 @@ kernel = {
>> extra_dist = video/sis315_init.c;
>> mips_loongson = commands/keylayouts.c;
>>
>> + powerpc_ieee1275 = kern/powerpc/ieee1275/ieee1275.c;
>> powerpc_ieee1275 = kern/powerpc/cache.S;
>> powerpc_ieee1275 = kern/powerpc/dl.c;
>> powerpc_ieee1275 = kern/powerpc/compiler-rt.S;
>> diff --git a/grub-core/kern/powerpc/ieee1275/ieee1275.c
>> b/grub-core/kern/powerpc/ieee1275/ieee1275.c
>> new file mode 100644
>> index 000000000..4746b6249
>> --- /dev/null
>> +++ b/grub-core/kern/powerpc/ieee1275/ieee1275.c
>> @@ -0,0 +1,141 @@
>> +/* of.c - Access the Open Firmware client interface. */
>
> s/of.c// or s/of.c/ieee1275.c/?
Sure. Will change it
>
>> +/*
>> + * GRUB -- GRand Unified Bootloader
>> + * Copyright (C) 2003,2004,2005,2007,2008,2009 Free Software Foundation,
>> Inc.
>> + * Copyright (C) 2020, 2021, 2022, 2023, 2024, 2025 IBM Corporation
>> + *
>> + * GRUB is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 3 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * GRUB is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <grub/ieee1275/ieee1275.h>
>> +#include <grub/powerpc/ieee1275/ieee1275.h>
>> +#include <grub/misc.h>
>> +
>> +#define IEEE1275_CELL_INVALID ((grub_ieee1275_cell_t) - 1)
>
> I can see this defined and used in the grub-core/kern/ieee1275/ieee1275.c.
> Why cannot you define GRUB_IEEE1275_CELL_INVALID in
> include/grub/ieee1275/ieee1275.h
> somewhere around GRUB_IEEE1275_CELL_FALSE/GRUB_IEEE1275_CELL_TRUE and
> use here and in grub-core/kern/ieee1275/ieee1275.c?
Sure. Will do it.
>
>> +int
>> +grub_ieee1275_test (const char *name, grub_ieee1275_cell_t *missing)
>
> Where are these functions used? Without that knowledge it is difficult
> to say they are correct or not including return values types. I think
> this patch should be merged with patch which call this functions.
>
These functions used by Patch -18 ([PATCH v3 18/25] ieee1275: Read the DB and
DBX secure boot variables)
Still do you think it should be merged with Patch -18.
>> +{
>> + struct test_args
>> + {
>> + struct grub_ieee1275_common_hdr common;
>> + grub_ieee1275_cell_t name;
>> + grub_ieee1275_cell_t missing;
>> + } args;
>> +
>> + INIT_IEEE1275_COMMON (&args.common, "test", 1, 1);
>> + args.name = (grub_ieee1275_cell_t) name;
>> +
>> + if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
>> + return -1;
>> +
>> + if (args.missing == IEEE1275_CELL_INVALID)
>> + return -1;
>> +
>> + *missing = args.missing;
>> +
>> + return 0;
>> +}
>>
>> +int
>> +grub_ieee1275_pks_max_object_size (grub_size_t *result)
>
> Ditto and below…
>
Thanks
Sudhakar
>> +{
>> + struct mos_args
>> + {
>> + struct grub_ieee1275_common_hdr common;
>> + grub_ieee1275_cell_t size;
>> + } args;
>> +
>> + INIT_IEEE1275_COMMON (&args.common, "pks-max-object-size", 0, 1);
>> +
>> + if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
>> + return -1;
>> +
>> + if (args.size == IEEE1275_CELL_INVALID)
>> + return -1;
>> +
>> + *result = args.size;
>> +
>> + return 0;
>> +}
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
