mbox series

[0/5] replace magic numbers in GDT descriptors

Message ID 20231219151200.2878271-1-vegard.nossum@oracle.com
Headers
Series replace magic numbers in GDT descriptors |

Message

Vegard Nossum Dec. 19, 2023, 3:11 p.m. UTC 
  Linus suggested replacing the magic numbers in the GDT descriptors
using preprocessor macros [1].

[1] https://lore.kernel.org/all/CAHk-=wib5XLebuEra7y2YH96wxdk=8vJnA8XoVq0FExpzVvN=Q@mail.gmail.com/

For patch 3 ("x86: replace magic numbers in GDT descriptors, part 2") I've
verified it to the best of my abilities on 32-bit and 64-bit by ensuring
that the object files are bitwise identical before and after applying the
patch and ensuring that all the object files were rebuilt on at least one
of the two configs:

32-bit:
  arch/x86/boot/pm.o -- no change
  arch/x86/kernel/apm_32.o -- no change
  arch/x86/kernel/cpu/common.o -- no change
  arch/x86/kernel/head64.o -- not built for 32
  arch/x86/kernel/setup_percpu.o -- no change
  arch/x86/platform/pvh/head.o -- not built for 32
  arch/x86/realmode/rm/reboot.o -- no change
  drivers/firmware/efi/libstub/x86-5lvl.o -- not built for 32
  drivers/pnp/pnpbios/bioscalls.o -- no change

64-bit:
  arch/x86/boot/pm.o -- no change
  arch/x86/kernel/apm_32.o -- not built for 64
  arch/x86/kernel/cpu/common.o -- no change
  arch/x86/kernel/head64.o -- no change
  arch/x86/kernel/setup_percpu.o -- no change
  arch/x86/platform/pvh/head.o -- no change
  arch/x86/realmode/rm/reboot.o -- no change
  drivers/firmware/efi/libstub/x86-5lvl.o -- no change
  drivers/pnp/pnpbios/bioscalls.o -- not built for 64

Patches 2+3 can be squashed to a single commit, but I've submitted them
separately because it makes verifying correctness easier.

I've done basic boot testing on both 32-bit and 64-bit with all of the
patches.

Vegard Nossum (5):
  x86: provide new infrastructure for GDT descriptors
  x86: replace magic numbers in GDT descriptors, part 1
  x86: replace magic numbers in GDT descriptors, part 2
  x86: always set A (accessed) flag in GDT descriptors
  x86: add DB flag to 32-bit percpu GDT entry

 arch/x86/boot/pm.c                      |  7 +--
 arch/x86/include/asm/desc_defs.h        | 68 ++++++++++++++++++++++---
 arch/x86/kernel/apm_32.c                |  2 +-
 arch/x86/kernel/cpu/common.c            | 50 ++++++++----------
 arch/x86/kernel/head64.c                |  6 +--
 arch/x86/kernel/setup_percpu.c          |  4 +-
 arch/x86/platform/pvh/head.S            |  7 +--
 arch/x86/realmode/rm/reboot.S           |  3 +-
 drivers/firmware/efi/libstub/x86-5lvl.c |  4 +-
 drivers/pnp/pnpbios/bioscalls.c         |  2 +-
 10 files changed, 100 insertions(+), 53 deletions(-)

Comments

Linus Torvalds Dec. 19, 2023, 5:33 p.m. UTC | #1 
On Tue, 19 Dec 2023 at 07:12, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>
> Vegard Nossum (5):
>   x86: provide new infrastructure for GDT descriptors
>   x86: replace magic numbers in GDT descriptors, part 1
>   x86: replace magic numbers in GDT descriptors, part 2
>   x86: always set A (accessed) flag in GDT descriptors
>   x86: add DB flag to 32-bit percpu GDT entry

All these patches look fine to me, but I will again leave it to the
x86 maintainers whether they want to apply them. But feel free to add
my Ack if y ou do.

The end result does look a *lot* more legible, with something like

   DESC_DATA64 | DESC_USER

instead of just a raw number like 0xc0f3.

So while this is unlikely to be a maintenance burden (since we look at
these things so seldom, and they never really change), I think it's a
nice readability improvement.

The fact that Vegard found two oddities while doing this series just
reinforces that readability issue. Neither of them were bugs, but they
were odd inconsistencies.

               Linus
Ingo Molnar Dec. 20, 2023, 9:59 a.m. UTC | #2 
* Linus Torvalds <torvalds@linuxfoundation.org> wrote:

> On Tue, 19 Dec 2023 at 07:12, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> >
> > Vegard Nossum (5):
> >   x86: provide new infrastructure for GDT descriptors
> >   x86: replace magic numbers in GDT descriptors, part 1
> >   x86: replace magic numbers in GDT descriptors, part 2
> >   x86: always set A (accessed) flag in GDT descriptors
> >   x86: add DB flag to 32-bit percpu GDT entry
> 
> All these patches look fine to me, but I will again leave it to the
> x86 maintainers whether they want to apply them. But feel free to add
> my Ack if y ou do.

Thanks - I've applied your acks. These are obviously useful cleanups,
so no objections whatsoever.

> The end result does look a *lot* more legible, with something like
> 
>    DESC_DATA64 | DESC_USER
> 
> instead of just a raw number like 0xc0f3.
>
> So while this is unlikely to be a maintenance burden (since we look at
> these things so seldom, and they never really change), I think it's a
> nice readability improvement.

Yeah, absolutely.

> The fact that Vegard found two oddities while doing this series just
> reinforces that readability issue. Neither of them were bugs, but they
> were odd inconsistencies.

Indeed ...

Thanks,

	Ingo