Re: [PATCH] Mandatory install device check for PowerPC

[Avnish Chouhan <avnish@linux.ibm.com>]

(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