Project

General

Profile

Actions

Bug #14997

closed

unnecessary jmp in desctbls_asm.s

Added by Dan Cross 6 days ago. Updated 3 days ago.

Status:
Closed
Priority:
Low
Assignee:
Category:
kernel
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
Gerrit CR:
External Bug:

Description

The x86 code for manipulating descriptor tables seems largely untouched since Solaris, and includes some curious sequences.

Notably, wr_gdtr contains a jmp over a nop immediately after lgdtr, none of which is necessary. The jmp seems like something a SPARC programmer might have written (delay slots), and the jmp seems like a vestige from when this code used to include an lretq to pick up a (potentially) new code segment; that code was removed in commit ae115bc77f6fcde83175c75b4206dc2e50747966 (which rolls up a bunch of commits from somewhere else; Sun perhaps?).

Also, the functions in desctbls_asm.s were changed to push a valid frame onto the stack, but inconsistently; the usual, pushq %rbp; movq %rsp, %rbp sequence creates the frame, but then a leave instruction is used to pop. We should be consistent on pairing enter/leave or the explicit pushq ...; movq and popq.

Actions #1

Updated by Dan Cross 6 days ago

Of course the relevant file is desctbls_asm.s, not descrtbls_asm.s.

(I'm trying to cut down on caffeine; I'm not sure this is a good idea.)

Actions #2

Updated by Electric Monk 6 days ago

  • Gerrit CR set to 2375
Actions #3

Updated by Patrick Mooney 6 days ago

  • Subject changed from unnecessary `jmp` in descrtbls_asm.s to unnecessary jmp in desctbls_asm.s
  • Description updated (diff)
Actions #4

Updated by Dan Cross 6 days ago

Thank you, Patrick!

Actions #5

Updated by Dan Cross 6 days ago

Tested: built nightly, installed on a machine, booted on hardware.

Note that the replacement of the leave instructions with popq %rbp is mostly a nop (one could argue setting up the frame in these trivial wrappers around privileged instructions should be removed entirely: note that frame setup/tear-down is not consistent across the functions in this file); the only real significant change the removal of the nop and jmp in wr_gdtr, but also the previous instruction sequence is not common.

Actions #6

Updated by Electric Monk 3 days ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 61b899723556289ed14dbcc0792ee6ed33c3edf9

commit  61b899723556289ed14dbcc0792ee6ed33c3edf9
Author: Dan Cross <cross@oxidecomputer.com>
Date:   2022-09-22T18:55:21.000Z

    14997 unnecessary jmp in desctbls_asm.s
    Reviewed by: Robert Mustacchi <rm@fingolfin.org>
    Approved by: Dan McDonald <danmcd@mnx.io>

Actions

Also available in: Atom PDF