[PATCH] Mandatory install device check for PowerPC

[PATCH] Mandatory install device check for PowerPC

[Avnish Chouhan]

This patch adds a check on install_device while installing grub for PowerPC.
If install_device is not mentioned in grub2-install, the error will be thrown.
Running grub2-install on PowerPC without the install_device may 
result in boot corruption.

Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
---
 util/grub-install.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util/grub-install.c b/util/grub-install.c
index 5babc7a..192d2a8 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -970,6 +970,8 @@ main (int argc, char *argv[])
     case GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275:
       if (install_device)
 	is_prep = 1;
+      else
+        grub_util_error ("%s", _("install device isn't specified"));
       break;
     case GRUB_INSTALL_PLATFORM_MIPS_ARC:
     case GRUB_INSTALL_PLATFORM_MIPSEL_ARC:
-- 
2.39.3


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 1235 bytes --]

Le lun. 27 mai 2024, 16:38, Avnish Chouhan <avnish@linux.ibm.com> a écrit :

> This patch adds a check on install_device while installing grub for
> PowerPC.
> If install_device is not mentioned in grub2-install, the error will be
> thrown.
> Running grub2-install on PowerPC without the install_device may
> result in boot corruption.
>
This breaks PowerMac booting which doesn't have install device.

>
> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> ---
>  util/grub-install.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 5babc7a..192d2a8 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -970,6 +970,8 @@ main (int argc, char *argv[])
>      case GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275:
>        if (install_device)
>         is_prep = 1;
> +      else
> +        grub_util_error ("%s", _("install device isn't specified"));
>        break;
>      case GRUB_INSTALL_PLATFORM_MIPS_ARC:
>      case GRUB_INSTALL_PLATFORM_MIPSEL_ARC:
> --
> 2.39.3
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2075 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[Michal Suchánek]

Hello,

On Mon, May 27, 2024 at 07:07:19PM +0530, Avnish Chouhan wrote:
> This patch adds a check on install_device while installing grub for PowerPC.
> If install_device is not mentioned in grub2-install, the error will be thrown.
> Running grub2-install on PowerPC without the install_device may 
> result in boot corruption.
> 
> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> ---
>  util/grub-install.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 5babc7a..192d2a8 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -970,6 +970,8 @@ main (int argc, char *argv[])
>      case GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275:
>        if (install_device)
>  	is_prep = 1;
> +      else
> +        grub_util_error ("%s", _("install device isn't specified"));
>        break;

As far as I understand this is fine for us (support for pSeries and
powernv platform).

However, this is not acceptable for upstream. Upstream also supports
Apple hardawre, and there the grub binary is installed as a file on the
filesystem rather than copied to a device.

Or to put it differently, there is the PReP sub-platform of
GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275 and currently the code does not
detect that is running on such platform (eg. by examining the device
tree) but rather it infers is_prep solely based on user input (the
device to install to) and in absence of the user input non-PReP platform
is assumed, leading to bootlist corruption when no boot device is given
on a PReP platform.

To fix this the logic should be inverted: Set is_prep should be set
based on what system is detected, and require an install device for
PReP platform.

Thanks

Michal

>      case GRUB_INSTALL_PLATFORM_MIPS_ARC:
>      case GRUB_INSTALL_PLATFORM_MIPSEL_ARC:
> -- 
> 2.39.3
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[avnish]

Hi Michal,
Thank you so much for a response!

We'll work on to figure out the type of machine.

On no_prep, as per my analysis. Even with no install device given while 
installing. We observe boot corruption on some specific setups (mostly 
where we have multiple valid PReP partition available on the machine. 
And corruption probably due to wrong PReP picked up by installer. And as 
the installer picked the valid PReP available, we don't get any errors.
Thank you!


Regards,
Avnish Chouhan

On 2024-05-27 19:44, Michal Suchánek wrote:
> Hello,
> 
> On Mon, May 27, 2024 at 07:07:19PM +0530, Avnish Chouhan wrote:
>> This patch adds a check on install_device while installing grub for 
>> PowerPC.
>> If install_device is not mentioned in grub2-install, the error will be 
>> thrown.
>> Running grub2-install on PowerPC without the install_device may
>> result in boot corruption.
>> 
>> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
>> ---
>>  util/grub-install.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/util/grub-install.c b/util/grub-install.c
>> index 5babc7a..192d2a8 100644
>> --- a/util/grub-install.c
>> +++ b/util/grub-install.c
>> @@ -970,6 +970,8 @@ main (int argc, char *argv[])
>>      case GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275:
>>        if (install_device)
>>  	is_prep = 1;
>> +      else
>> +        grub_util_error ("%s", _("install device isn't specified"));
>>        break;
> 
> As far as I understand this is fine for us (support for pSeries and
> powernv platform).
> 
> However, this is not acceptable for upstream. Upstream also supports
> Apple hardawre, and there the grub binary is installed as a file on the
> filesystem rather than copied to a device.
> 
> Or to put it differently, there is the PReP sub-platform of
> GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275 and currently the code does not
> detect that is running on such platform (eg. by examining the device
> tree) but rather it infers is_prep solely based on user input (the
> device to install to) and in absence of the user input non-PReP 
> platform
> is assumed, leading to bootlist corruption when no boot device is given
> on a PReP platform.
> 
> To fix this the logic should be inverted: Set is_prep should be set
> based on what system is detected, and require an install device for
> PReP platform.
> 
> Thanks
> 
> Michal
> 
>>      case GRUB_INSTALL_PLATFORM_MIPS_ARC:
>>      case GRUB_INSTALL_PLATFORM_MIPSEL_ARC:
>> --
>> 2.39.3
>> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH] Mandatory install device check for PowerPC

[Avnish Chouhan]

This patch adds a check on install_device while installing grub for PowerPC.
If install_device is not mentioned in grub2-install and machine is detected
as PowerPC, the error will be thrown and it will terminates the grub2-install 
operation. Running grub2-install on PowerPC without the install_device may 
result in bootlist corruption. When no install device is specified, it attempts 
to load images from the filesystem, which leads to nvram bootlist corruption. 
The idea is to fail the operation and avoid creating the invalid boot entry.

Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
---
 grub-install.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/util/grub-install.c b/util/grub-install.c
index 7dc5657..a049f53 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1289,6 +1289,17 @@ main (int argc, char *argv[])
 	      is_prep = 0;
 	    }
 	}
+      else
+        {
+         /*
+	   * As the machine has been detected as PowerPC and not the PowerMac. We need to check
+	   * whether the install_device has been mentioned while installing. If no device has been
+	   * mentioned, we need to exit and mark it as an error as the install_device is required for
+	   * PowerPC installation. An installation with no device mentioned may lead to corruptions.
+	   */
+           if (!install_device)
+             grub_util_error ("%s", _("install device isn't specified required for PowerPC"));
+        }
     }
 
   size_t ndev = 0;
-- 
2.39.3


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 2044 bytes --]

As discussed in another thread, this breaks installing from x86 onto
removable disk for PPC Mac which is a supported workflow

Le ven. 8 nov. 2024, 14:13, Avnish Chouhan <avnish@linux.ibm.com> a écrit :

> This patch adds a check on install_device while installing grub for
> PowerPC.
> If install_device is not mentioned in grub2-install and machine is detected
> as PowerPC, the error will be thrown and it will terminates the
> grub2-install
> operation. Running grub2-install on PowerPC without the install_device may
> result in bootlist corruption. When no install device is specified, it
> attempts
> to load images from the filesystem, which leads to nvram bootlist
> corruption.
> The idea is to fail the operation and avoid creating the invalid boot
> entry.
>
> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> ---
>  grub-install.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 7dc5657..a049f53 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1289,6 +1289,17 @@ main (int argc, char *argv[])
>               is_prep = 0;
>             }
>         }
> +      else
> +        {
> +         /*
> +          * As the machine has been detected as PowerPC and not the
> PowerMac. We need to check
> +          * whether the install_device has been mentioned while
> installing. If no device has been
> +          * mentioned, we need to exit and mark it as an error as the
> install_device is required for
> +          * PowerPC installation. An installation with no device
> mentioned may lead to corruptions.
> +          */
> +           if (!install_device)
> +             grub_util_error ("%s", _("install device isn't specified
> required for PowerPC"));
> +        }
>      }
>
>    size_t ndev = 0;
> --
> 2.39.3
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2729 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[avnish]

Hi Vladimir,
Thank you so much for your response!

I have fine tuned the patch as per the last discussion (sorry, I missed 
the v2 tag). This latest patch will add install device check only to 
PowerPC machines. PowerMacs aren't affected by this change. The check is 
added when platform is detected as 
"GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275" along with machine detected as 
non PowerMac. As per my Power platform analysis, currently in 
"grub_install.c", it detects PowerMacs based on the file system detected 
(HFS or HFS+) and set the "is_prep" as 0 based on this finding. This new 
check will only be applicable to PowerPC. And in case of PowerMacs, it 
will allow grub_install even without mentioning the install device.
Thank you!


Regards,
Avnish Chouhan
> ------------------------------
> 
> Message: 5
> Date: Fri, 8 Nov 2024 15:07:29 +0300
> From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
> To: The development of GNU GRUB <grub-devel@gnu.org>
> Subject: Re: [PATCH] Mandatory install device check for PowerPC
> Message-ID:
> 	<CAEaD8JMqP4_uP5cZutSMGWvGMxbHAvNh10VCMO4ZcbqvLAQ9zw@mail.gmail.com>
> Content-Type: text/plain; charset="utf-8"
> 
> As discussed in another thread, this breaks installing from x86 onto
> removable disk for PPC Mac which is a supported workflow
> 
> Le ven. 8 nov. 2024, 14:13, Avnish Chouhan <avnish@linux.ibm.com> a 
> écrit :
> 
>> This patch adds a check on install_device while installing grub for
>> PowerPC.
>> If install_device is not mentioned in grub2-install and machine is 
>> detected
>> as PowerPC, the error will be thrown and it will terminates the
>> grub2-install
>> operation. Running grub2-install on PowerPC without the install_device 
>> may
>> result in bootlist corruption. When no install device is specified, it
>> attempts
>> to load images from the filesystem, which leads to nvram bootlist
>> corruption.
>> The idea is to fail the operation and avoid creating the invalid boot
>> entry.
>> 
>> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
>> ---
>>  grub-install.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/util/grub-install.c b/util/grub-install.c
>> index 7dc5657..a049f53 100644
>> --- a/util/grub-install.c
>> +++ b/util/grub-install.c
>> @@ -1289,6 +1289,17 @@ main (int argc, char *argv[])
>>               is_prep = 0;
>>             }
>>         }
>> +      else
>> +        {
>> +         /*
>> +          * As the machine has been detected as PowerPC and not the
>> PowerMac. We need to check
>> +          * whether the install_device has been mentioned while
>> installing. If no device has been
>> +          * mentioned, we need to exit and mark it as an error as the
>> install_device is required for
>> +          * PowerPC installation. An installation with no device
>> mentioned may lead to corruptions.
>> +          */
>> +           if (!install_device)
>> +             grub_util_error ("%s", _("install device isn't specified
>> required for PowerPC"));
>> +        }
>>      }
>> 
>>    size_t ndev = 0;
>> --
>> 2.39.3
>> 
>> 
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>> 
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> <https://lists.gnu.org/archive/html/grub-devel/attachments/20241108/22e8b62a/attachment.htm>
> 
> ------------------------------
> 
> Subject: Digest Footer
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> ------------------------------
> 
> End of Grub-devel Digest, Vol 249, Issue 30
> *******************************************

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[Michal Suchánek]

Hello,

thanks for the patch!

On Sat, Nov 09, 2024 at 11:20:08AM +0530, avnish wrote:
> Hi Vladimir,
> Thank you so much for your response!
> 
> I have fine tuned the patch as per the last discussion (sorry, I missed the
> v2 tag). This latest patch will add install device check only to PowerPC
> machines. PowerMacs aren't affected by this change. The check is added when
> platform is detected as "GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275" along with
> machine detected as non PowerMac. As per my Power platform analysis,
> currently in "grub_install.c", it detects PowerMacs based on the file system
> detected (HFS or HFS+) and set the "is_prep" as 0 based on this finding.
> This new check will only be applicable to PowerPC. And in case of PowerMacs,
> it will allow grub_install even without mentioning the install device.
> Thank you!
> 
> 
> Regards,
> Avnish Chouhan
> > ------------------------------
> > 
> > Message: 5
> > Date: Fri, 8 Nov 2024 15:07:29 +0300
> > From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
> > To: The development of GNU GRUB <grub-devel@gnu.org>
> > Subject: Re: [PATCH] Mandatory install device check for PowerPC
> > Message-ID:
> > 	<CAEaD8JMqP4_uP5cZutSMGWvGMxbHAvNh10VCMO4ZcbqvLAQ9zw@mail.gmail.com>
> > Content-Type: text/plain; charset="utf-8"
> > 
> > As discussed in another thread, this breaks installing from x86 onto
> > removable disk for PPC Mac which is a supported workflow

Please be more specific. I cannot find how this version of the patch
still breaks other platforms. Given that you are talking about
cross-installation form x86 this should be eeasy to test given detailed
description.

> > 
> > Le ven. 8 nov. 2024, 14:13, Avnish Chouhan <avnish@linux.ibm.com> a
> > écrit :
> > 
> > > This patch adds a check on install_device while installing grub for
> > > PowerPC.
> > > If install_device is not mentioned in grub2-install and machine is
> > > detected
> > > as PowerPC, the error will be thrown and it will terminates the
> > > grub2-install
> > > operation. Running grub2-install on PowerPC without the
> > > install_device may
> > > result in bootlist corruption. When no install device is specified, it
> > > attempts
> > > to load images from the filesystem, which leads to nvram bootlist
> > > corruption.
> > > The idea is to fail the operation and avoid creating the invalid boot
> > > entry.
> > > 
> > > Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> > > ---
> > >  grub-install.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)

Before here there is this code:

if (install_device)
  is_prep = 1;

This is the root of the problem. The code sets is_prep based on user
input, and when the input is wrong is_prep remains wrongly unset,
leading to bogus entry written to bootlist, and the system becoming
unbootable.

Instead this code shuld be removed, and is_prep initialized to 1.

> > > 
> > > diff --git a/util/grub-install.c b/util/grub-install.c
> > > index 7dc5657..a049f53 100644
> > > --- a/util/grub-install.c
> > > +++ b/util/grub-install.c
> > > @@ -1289,6 +1289,17 @@ main (int argc, char *argv[])
> > >               is_prep = 0;

Here is_prep is unset when a Mac boot partition is found. With that
initializing is_prep to 1 gives sound logic for determining if the
system looks like a PowerMac or not.

There is the possibility that grub-install would run on a PowerMac, and
the Mac boot partition is not given by the user nor autodetected but
there is not much that can be done about that given cross-installation
is supported. This case can't work currently either.

> > >             }
> > >         }
> > > +      else

This logic is still not sound. Though unlikely grub-install can find
something that looks like a Mac boot partition (is_guess is 1) but is
not one (does not unset is_prep).

Instead of

else if (!install_device)

if (is_prep && !install_device)

can be used when is_prep is initialized to true unconditionally. Then
the code above sets is_prep to false when a Mac partition is found, and
if none is found and install device is not set it's an error.

> > > +        {
> > > +         /*
> > > +          * As the machine has been detected as PowerPC and not the
> > > PowerMac. We need to check
> > > +          * whether the install_device has been mentioned while
> > > installing. If no device has been
> > > +          * mentioned, we need to exit and mark it as an error as the
> > > install_device is required for
> > > +          * PowerPC installation. An installation with no device
> > > mentioned may lead to corruptions.
> > > +          */
> > > +           if (!install_device)
> > > +             grub_util_error ("%s", _("install device isn't specified
> > > required for PowerPC"));

This message is rather awkward and misleading.

I think something like "install device required on PReP platform" is as
good as it gets.

The platform naming is quite unfortunate. The port to Power family of
CPUs is called powerpc or ppc but PowerPC refers to the desktop family
of CPUs found mainly in PowerMac hardware.

PReP is a standard originally meant for use with PowerPC based hardware,
and that's where the partition name comes from but was superseded by
CHRP and PAPR. PAPR specifically is used for server and embedded CPUs,
not desktop, and that's what is supported besides PowerMac. I doubt
current version of GRUB would work on the original PReP hardware.

Thanks

Michal

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[Avnish Chouhan]

(Resending the email due to missing proper subject line caused by some 
glitches in my application)

Hi Vladimir,
Hope you're doing wonderful!

Did you get a change to review my response to your query?

A similar handling of install device has been done for "SPARC64_IEEE1275 
Platform". The issue we had in my v1 is to identify whether the machine 
PowerPC or PowerMac. With v2, we have achieved this and the change is 
limited to PowerPC now.
Thank you!

Regards,
Avnish Chouhan

> ------------------------------
> 
> Message: 4
> Date: Mon, 11 Nov 2024 10:13:57 +0100
> From: Michal Suchánek <msuchanek@suse.de>
> To: avnish <avnish@imap.linux.ibm.com>
> Cc: grub-devel@gnu.org, grub-devel-request@gnu.org, phcoder@gmail.com,
> 	mchang@suse.com
> Subject: Re: [PATCH] Mandatory install device check for PowerPC
> Message-ID: <ZzHK1f8-J82o-J9A@kitsune.suse.cz>
> Content-Type: text/plain; charset=iso-8859-1
> 
> Hello,
> 
> thanks for the patch!
> 
> On Sat, Nov 09, 2024 at 11:20:08AM +0530, avnish wrote:
>> Hi Vladimir,
>> Thank you so much for your response!
>> 
>> I have fine tuned the patch as per the last discussion (sorry, I 
>> missed the
>> v2 tag). This latest patch will add install device check only to 
>> PowerPC
>> machines. PowerMacs aren't affected by this change. The check is added 
>> when
>> platform is detected as "GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275" along 
>> with
>> machine detected as non PowerMac. As per my Power platform analysis,
>> currently in "grub_install.c", it detects PowerMacs based on the file 
>> system
>> detected (HFS or HFS+) and set the "is_prep" as 0 based on this 
>> finding.
>> This new check will only be applicable to PowerPC. And in case of 
>> PowerMacs,
>> it will allow grub_install even without mentioning the install device.
>> Thank you!
>> 
>> 
>> Regards,
>> Avnish Chouhan
>> > ------------------------------
>> >
>> > Message: 5
>> > Date: Fri, 8 Nov 2024 15:07:29 +0300
>> > From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
>> > To: The development of GNU GRUB <grub-devel@gnu.org>
>> > Subject: Re: [PATCH] Mandatory install device check for PowerPC
>> > Message-ID:
>> > 	<CAEaD8JMqP4_uP5cZutSMGWvGMxbHAvNh10VCMO4ZcbqvLAQ9zw@mail.gmail.com>
>> > Content-Type: text/plain; charset="utf-8"
>> >
>> > As discussed in another thread, this breaks installing from x86 onto
>> > removable disk for PPC Mac which is a supported workflow
> 
> Please be more specific. I cannot find how this version of the patch
> still breaks other platforms. Given that you are talking about
> cross-installation form x86 this should be eeasy to test given detailed
> description.
> 
>> >
>> > Le ven. 8 nov. 2024, 14:13, Avnish Chouhan <avnish@linux.ibm.com> a
>> > écrit :
>> >
>> > > This patch adds a check on install_device while installing grub for
>> > > PowerPC.
>> > > If install_device is not mentioned in grub2-install and machine is
>> > > detected
>> > > as PowerPC, the error will be thrown and it will terminates the
>> > > grub2-install
>> > > operation. Running grub2-install on PowerPC without the
>> > > install_device may
>> > > result in bootlist corruption. When no install device is specified, it
>> > > attempts
>> > > to load images from the filesystem, which leads to nvram bootlist
>> > > corruption.
>> > > The idea is to fail the operation and avoid creating the invalid boot
>> > > entry.
>> > >
>> > > Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
>> > > ---
>> > >  grub-install.c | 11 +++++++++++
>> > >  1 file changed, 11 insertions(+)
> 
> Before here there is this code:
> 
> if (install_device)
>   is_prep = 1;
> 
> This is the root of the problem. The code sets is_prep based on user
> input, and when the input is wrong is_prep remains wrongly unset,
> leading to bogus entry written to bootlist, and the system becoming
> unbootable.
> 
> Instead this code shuld be removed, and is_prep initialized to 1.
> 
>> > >
>> > > diff --git a/util/grub-install.c b/util/grub-install.c
>> > > index 7dc5657..a049f53 100644
>> > > --- a/util/grub-install.c
>> > > +++ b/util/grub-install.c
>> > > @@ -1289,6 +1289,17 @@ main (int argc, char *argv[])
>> > >               is_prep = 0;
> 
> Here is_prep is unset when a Mac boot partition is found. With that
> initializing is_prep to 1 gives sound logic for determining if the
> system looks like a PowerMac or not.
> 
> There is the possibility that grub-install would run on a PowerMac, and
> the Mac boot partition is not given by the user nor autodetected but
> there is not much that can be done about that given cross-installation
> is supported. This case can't work currently either.
> 
>> > >             }
>> > >         }
>> > > +      else
> 
> This logic is still not sound. Though unlikely grub-install can find
> something that looks like a Mac boot partition (is_guess is 1) but is
> not one (does not unset is_prep).
> 
> Instead of
> 
> else if (!install_device)
> 
> if (is_prep && !install_device)
> 
> can be used when is_prep is initialized to true unconditionally. Then
> the code above sets is_prep to false when a Mac partition is found, and
> if none is found and install device is not set it's an error.
> 
>> > > +        {
>> > > +         /*
>> > > +          * As the machine has been detected as PowerPC and not the
>> > > PowerMac. We need to check
>> > > +          * whether the install_device has been mentioned while
>> > > installing. If no device has been
>> > > +          * mentioned, we need to exit and mark it as an error as the
>> > > install_device is required for
>> > > +          * PowerPC installation. An installation with no device
>> > > mentioned may lead to corruptions.
>> > > +          */
>> > > +           if (!install_device)
>> > > +             grub_util_error ("%s", _("install device isn't specified
>> > > required for PowerPC"));
> 
> This message is rather awkward and misleading.
> 
> I think something like "install device required on PReP platform" is as
> good as it gets.
> 
> The platform naming is quite unfortunate. The port to Power family of
> CPUs is called powerpc or ppc but PowerPC refers to the desktop family
> of CPUs found mainly in PowerMac hardware.
> 
> PReP is a standard originally meant for use with PowerPC based 
> hardware,
> and that's where the partition name comes from but was superseded by
> CHRP and PAPR. PAPR specifically is used for server and embedded CPUs,
> not desktop, and that's what is supported besides PowerMac. I doubt
> current version of GRUB would work on the original PReP hardware.
> 
> Thanks
> 
> Michal
> 
> 
> 
> ------------------------------
> 
> Subject: Digest Footer
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> ------------------------------
> 
> End of Grub-devel Digest, Vol 249, Issue 46
> *******************************************

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Mandatory install device check for PowerPC

[Avnish Chouhan]

Hi Vladimir,

Did you get a chance to review my response to your query?
Thank you!

Regards,
Avnish Chouhan

On 2024-11-28 16:11, Avnish Chouhan wrote:
> (Resending the email due to missing proper subject line caused by some
> glitches in my application)
> 
> Hi Vladimir,
> Hope you're doing wonderful!
> 
> Did you get a change to review my response to your query?
> 
> A similar handling of install device has been done for
> "SPARC64_IEEE1275 Platform". The issue we had in my v1 is to identify
> whether the machine PowerPC or PowerMac. With v2, we have achieved
> this and the change is limited to PowerPC now.
> Thank you!
> 
> Regards,
> Avnish Chouhan
> 
>> ------------------------------
>> 
>> Message: 4
>> Date: Mon, 11 Nov 2024 10:13:57 +0100
>> From: Michal Suchánek <msuchanek@suse.de>
>> To: avnish <avnish@imap.linux.ibm.com>
>> Cc: grub-devel@gnu.org, grub-devel-request@gnu.org, phcoder@gmail.com,
>> 	mchang@suse.com
>> Subject: Re: [PATCH] Mandatory install device check for PowerPC
>> Message-ID: <ZzHK1f8-J82o-J9A@kitsune.suse.cz>
>> Content-Type: text/plain; charset=iso-8859-1
>> 
>> Hello,
>> 
>> thanks for the patch!
>> 
>> On Sat, Nov 09, 2024 at 11:20:08AM +0530, avnish wrote:
>>> Hi Vladimir,
>>> Thank you so much for your response!
>>> 
>>> I have fine tuned the patch as per the last discussion (sorry, I 
>>> missed the
>>> v2 tag). This latest patch will add install device check only to 
>>> PowerPC
>>> machines. PowerMacs aren't affected by this change. The check is 
>>> added when
>>> platform is detected as "GRUB_INSTALL_PLATFORM_POWERPC_IEEE1275" 
>>> along with
>>> machine detected as non PowerMac. As per my Power platform analysis,
>>> currently in "grub_install.c", it detects PowerMacs based on the file 
>>> system
>>> detected (HFS or HFS+) and set the "is_prep" as 0 based on this 
>>> finding.
>>> This new check will only be applicable to PowerPC. And in case of 
>>> PowerMacs,
>>> it will allow grub_install even without mentioning the install 
>>> device.
>>> Thank you!
>>> 
>>> 
>>> Regards,
>>> Avnish Chouhan
>>> > ------------------------------
>>> >
>>> > Message: 5
>>> > Date: Fri, 8 Nov 2024 15:07:29 +0300
>>> > From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
>>> > To: The development of GNU GRUB <grub-devel@gnu.org>
>>> > Subject: Re: [PATCH] Mandatory install device check for PowerPC
>>> > Message-ID:
>>> > 	<CAEaD8JMqP4_uP5cZutSMGWvGMxbHAvNh10VCMO4ZcbqvLAQ9zw@mail.gmail.com>
>>> > Content-Type: text/plain; charset="utf-8"
>>> >
>>> > As discussed in another thread, this breaks installing from x86 onto
>>> > removable disk for PPC Mac which is a supported workflow
>> 
>> Please be more specific. I cannot find how this version of the patch
>> still breaks other platforms. Given that you are talking about
>> cross-installation form x86 this should be eeasy to test given 
>> detailed
>> description.
>> 
>>> >
>>> > Le ven. 8 nov. 2024, 14:13, Avnish Chouhan <avnish@linux.ibm.com> a
>>> > écrit :
>>> >
>>> > > This patch adds a check on install_device while installing grub for
>>> > > PowerPC.
>>> > > If install_device is not mentioned in grub2-install and machine is
>>> > > detected
>>> > > as PowerPC, the error will be thrown and it will terminates the
>>> > > grub2-install
>>> > > operation. Running grub2-install on PowerPC without the
>>> > > install_device may
>>> > > result in bootlist corruption. When no install device is specified, it
>>> > > attempts
>>> > > to load images from the filesystem, which leads to nvram bootlist
>>> > > corruption.
>>> > > The idea is to fail the operation and avoid creating the invalid boot
>>> > > entry.
>>> > >
>>> > > Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
>>> > > ---
>>> > >  grub-install.c | 11 +++++++++++
>>> > >  1 file changed, 11 insertions(+)
>> 
>> Before here there is this code:
>> 
>> if (install_device)
>>   is_prep = 1;
>> 
>> This is the root of the problem. The code sets is_prep based on user
>> input, and when the input is wrong is_prep remains wrongly unset,
>> leading to bogus entry written to bootlist, and the system becoming
>> unbootable.
>> 
>> Instead this code shuld be removed, and is_prep initialized to 1.
>> 
>>> > >
>>> > > diff --git a/util/grub-install.c b/util/grub-install.c
>>> > > index 7dc5657..a049f53 100644
>>> > > --- a/util/grub-install.c
>>> > > +++ b/util/grub-install.c
>>> > > @@ -1289,6 +1289,17 @@ main (int argc, char *argv[])
>>> > >               is_prep = 0;
>> 
>> Here is_prep is unset when a Mac boot partition is found. With that
>> initializing is_prep to 1 gives sound logic for determining if the
>> system looks like a PowerMac or not.
>> 
>> There is the possibility that grub-install would run on a PowerMac, 
>> and
>> the Mac boot partition is not given by the user nor autodetected but
>> there is not much that can be done about that given cross-installation
>> is supported. This case can't work currently either.
>> 
>>> > >             }
>>> > >         }
>>> > > +      else
>> 
>> This logic is still not sound. Though unlikely grub-install can find
>> something that looks like a Mac boot partition (is_guess is 1) but is
>> not one (does not unset is_prep).
>> 
>> Instead of
>> 
>> else if (!install_device)
>> 
>> if (is_prep && !install_device)
>> 
>> can be used when is_prep is initialized to true unconditionally. Then
>> the code above sets is_prep to false when a Mac partition is found, 
>> and
>> if none is found and install device is not set it's an error.
>> 
>>> > > +        {
>>> > > +         /*
>>> > > +          * As the machine has been detected as PowerPC and not the
>>> > > PowerMac. We need to check
>>> > > +          * whether the install_device has been mentioned while
>>> > > installing. If no device has been
>>> > > +          * mentioned, we need to exit and mark it as an error as the
>>> > > install_device is required for
>>> > > +          * PowerPC installation. An installation with no device
>>> > > mentioned may lead to corruptions.
>>> > > +          */
>>> > > +           if (!install_device)
>>> > > +             grub_util_error ("%s", _("install device isn't specified
>>> > > required for PowerPC"));
>> 
>> This message is rather awkward and misleading.
>> 
>> I think something like "install device required on PReP platform" is 
>> as
>> good as it gets.
>> 
>> The platform naming is quite unfortunate. The port to Power family of
>> CPUs is called powerpc or ppc but PowerPC refers to the desktop family
>> of CPUs found mainly in PowerMac hardware.
>> 
>> PReP is a standard originally meant for use with PowerPC based 
>> hardware,
>> and that's where the partition name comes from but was superseded by
>> CHRP and PAPR. PAPR specifically is used for server and embedded CPUs,
>> not desktop, and that's what is supported besides PowerMac. I doubt
>> current version of GRUB would work on the original PReP hardware.
>> 
>> Thanks
>> 
>> Michal
>> 
>> 
>> 
>> ------------------------------
>> 
>> Subject: Digest Footer
>> 
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>> 
>> 
>> ------------------------------
>> 
>> End of Grub-devel Digest, Vol 249, Issue 46
>> *******************************************

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel