1. Patchew
  2. Xen
  3. View series

[PATCH] x86/boot: Don't use INC to set defaults

Andrew Cooper posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20241003145810.2217075-1-andrew.cooper3@citrix.com
xen/arch/x86/boot/head.S | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
[PATCH] x86/boot: Don't use INC to set defaults
Posted by Andrew Cooper 1 month, 2 weeks ago 
__efi64_mb2_start() makes some bold assumptions about the efi_platform and
skip_realmode booleans.  Set them to 1 explicitly, which is more robust.

Make the comment a little more consice.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/head.S | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d1856d8012c9..af776c201a15 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -279,14 +279,12 @@ __efi64_mb2_start:
         pop     %rbx
         pop     %rax
 
-        /* We are on EFI platform and EFI boot services are available. */
-        incb    efi_platform(%rip)
-
         /*
-         * Disable real mode and other legacy stuff which should not
-         * be run on EFI platforms.
+         * efi_multiboot2_prelude() is happy that we're on EFI platform.  Skip
+         * the BIOS initialisation path.
          */
-        incb    skip_realmode(%rip)
+        movb    $1, efi_platform(%rip)
+        movb    $1, skip_realmode(%rip)
 
         /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
         lea     trampoline_setup(%rip),%r15

base-commit: eb21ce14d709ef0c0030d0625028a4868c81126f
-- 
2.39.5
Re: [PATCH] x86/boot: Don't use INC to set defaults
Posted by Frediano Ziglio 1 month, 2 weeks ago 
On Thu, Oct 3, 2024 at 3:58 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> __efi64_mb2_start() makes some bold assumptions about the efi_platform and
> skip_realmode booleans.  Set them to 1 explicitly, which is more robust.
>
> Make the comment a little more consice.
>
> No practical change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
>  xen/arch/x86/boot/head.S | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index d1856d8012c9..af776c201a15 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -279,14 +279,12 @@ __efi64_mb2_start:
>          pop     %rbx
>          pop     %rax
>
> -        /* We are on EFI platform and EFI boot services are available. */
> -        incb    efi_platform(%rip)
> -
>          /*
> -         * Disable real mode and other legacy stuff which should not
> -         * be run on EFI platforms.
> +         * efi_multiboot2_prelude() is happy that we're on EFI platform.  Skip
> +         * the BIOS initialisation path.
>           */
> -        incb    skip_realmode(%rip)
> +        movb    $1, efi_platform(%rip)
> +        movb    $1, skip_realmode(%rip)
>
>          /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
>          lea     trampoline_setup(%rip),%r15
>
> base-commit: eb21ce14d709ef0c0030d0625028a4868c81126f

Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>

I like _Bool !

Frediano
Re: [PATCH] x86/boot: Don't use INC to set defaults
Posted by Roger Pau Monné 1 month, 2 weeks ago 
On Thu, Oct 03, 2024 at 04:42:23PM +0100, Frediano Ziglio wrote:
> On Thu, Oct 3, 2024 at 3:58 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> > __efi64_mb2_start() makes some bold assumptions about the efi_platform and
> > skip_realmode booleans.  Set them to 1 explicitly, which is more robust.
> >
> > Make the comment a little more consice.
> >
> > No practical change.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Roger Pau Monné <roger.pau@citrix.com>
> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> > CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> > ---
> >  xen/arch/x86/boot/head.S | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index d1856d8012c9..af776c201a15 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -279,14 +279,12 @@ __efi64_mb2_start:
> >          pop     %rbx
> >          pop     %rax
> >
> > -        /* We are on EFI platform and EFI boot services are available. */
> > -        incb    efi_platform(%rip)
> > -
> >          /*
> > -         * Disable real mode and other legacy stuff which should not
> > -         * be run on EFI platforms.
> > +         * efi_multiboot2_prelude() is happy that we're on EFI platform.  Skip
> > +         * the BIOS initialisation path.
> >           */
> > -        incb    skip_realmode(%rip)
> > +        movb    $1, efi_platform(%rip)
> > +        movb    $1, skip_realmode(%rip)
> >
> >          /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
> >          lea     trampoline_setup(%rip),%r15
> >
> > base-commit: eb21ce14d709ef0c0030d0625028a4868c81126f
> 
> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.
Re: [PATCH] x86/boot: Don't use INC to set defaults
Posted by Andrew Cooper 1 month, 2 weeks ago 
On 03/10/2024 4:48 pm, Roger Pau Monné wrote:
> On Thu, Oct 03, 2024 at 04:42:23PM +0100, Frediano Ziglio wrote:
>> On Thu, Oct 3, 2024 at 3:58 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> __efi64_mb2_start() makes some bold assumptions about the efi_platform and
>>> skip_realmode booleans.  Set them to 1 explicitly, which is more robust.
>>>
>>> Make the comment a little more consice.
>>>
>>> No practical change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
>>> ---
>>>  xen/arch/x86/boot/head.S | 10 ++++------
>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>>> index d1856d8012c9..af776c201a15 100644
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -279,14 +279,12 @@ __efi64_mb2_start:
>>>          pop     %rbx
>>>          pop     %rax
>>>
>>> -        /* We are on EFI platform and EFI boot services are available. */
>>> -        incb    efi_platform(%rip)
>>> -
>>>          /*
>>> -         * Disable real mode and other legacy stuff which should not
>>> -         * be run on EFI platforms.
>>> +         * efi_multiboot2_prelude() is happy that we're on EFI platform.  Skip
>>> +         * the BIOS initialisation path.
>>>           */
>>> -        incb    skip_realmode(%rip)
>>> +        movb    $1, efi_platform(%rip)
>>> +        movb    $1, skip_realmode(%rip)
>>>
>>>          /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
>>>          lea     trampoline_setup(%rip),%r15
>>>
>>> base-commit: eb21ce14d709ef0c0030d0625028a4868c81126f
>> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>


Thanks both.

I further note that efi_platform is used exclusively to skip early
cmdline parsing, and I'm sure there's probably a better way of doing
this, but nothing obvious jumps out to me.

I suspect that getting rid of it will be easier when we've moved more
logic to be in C.

~Andrew

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.