lkml.org 
[lkml]   [2013]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v7u1 22/31] x86, boot: add fields to support load bzImage and ramdisk above 4G
    On Sun, Jan 13, 2013 at 09:37:08PM -0800, Yinghai Lu wrote:
    > > Question: if the bootloader sets ext_* properly, is it going to set
    > > sentinel to 0 so that it can signal to the code further on that ext_*
    > > are valid?
    >
    > old bootloaders have no idea of sentinel, but if they initialize boot_param
    > properly that new sentinel will be 0 and new kernel will know.
    >
    > >
    > > This is kinda missing from the mechanism of the sentinel and it should
    > > be documented too.
    >
    > No, we should have too much duplicated info.

    This is not a complicated info - it should explain the basic mechanism
    of the sentinel.

    > >> -v7: change to 0x1ef instead of 0x1f0, HPA said:
    > >> it is quite plausible that someone may (fairly sanely) start the
    > >> copy range at 0x1f0 instead of 0x1f1
    > >
    > > Right, all those -vX notes are all important and should *definitely* be
    > > at least in the commit message.
    >
    > No, I want to keep them in order to track the reviewing progress.

    Are you saying "no" just for the fun of it? Or do you have a general
    aversion to documenting your code?

    Give me *one* good reason where having a short, concise and clear
    comment which helps people understand what the intent of the mechanism
    is a bad thing.

    [ … ]

    > >> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
    > >> index b4c913c..bffd73b 100644
    > >> --- a/arch/x86/boot/compressed/cmdline.c
    > >> +++ b/arch/x86/boot/compressed/cmdline.c
    > >> @@ -17,6 +17,8 @@ static unsigned long get_cmd_line_ptr(void)
    > >> {
    > >> unsigned long cmd_line_ptr = real_mode->hdr.cmd_line_ptr;
    > >>
    > >> + cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32;
    > >> +
    > >> return cmd_line_ptr;
    > >> }
    > >
    > > On 32-bit, this unsigned long cmd_line_ptr is 4 bytes and the OR doesn't
    > > have any effect on the final result. You probably want to do:
    >
    > yes, that is what we want to keep 32bit and 64bit unified.
    >
    > >
    > > #ifdef CONFIG_64BIT
    > > cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32;
    > > #endif
    > >
    > > right?
    > >
    > > Or instead look at ->sentinel to know whether the ext_* fields are valid
    > > or not, and save yourself the OR if not.
    >
    > no.
    >
    > that is whole point of sentinel, we don't need to check sentinel everywhere
    > because ext_* are valid.

    Dude, do you even read my comments? This line:

    cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32;

    doesn't do a whit on 32-bit. So execute it *only* on 32-bit!

    [ … ]

    > >> diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld
    > >> index 03c0683..9333d37 100644
    > >> --- a/arch/x86/boot/setup.ld
    > >> +++ b/arch/x86/boot/setup.ld
    > >> @@ -13,6 +13,13 @@ SECTIONS
    > >> .bstext : { *(.bstext) }
    > >> .bsdata : { *(.bsdata) }
    > >>
    > >> + /* sentinel: make sure if boot_params from bootloader is right */
    > >
    > > This should say:
    > >
    > > /*
    > > * The bootloader signals the validity of the three ext_* boot params with this.
    > > */
    >
    > no, bootloader does not signal that.
    > old bootloaders have no idea of sentinel, but if they initialize boot_param
    > properly that new sentinel will be 0 and new kernel will know.

    So say

    * A RECENT bootloader signals the validity of the three ext_* boot params with this.

    but say something.

    Understand this (and we've been chewing this same shit for two weeks
    now): you need to document your code and you need to document it
    properly for other people to understand what you're doing. I'm not
    talking about writing an essay or whatever - I'm talking about helpful
    comments placed where it makes most sense so that others can understand
    the mechanism.

    [ … ]

    > >> + __u16 xloadflags;
    > >> +#define CAN_BE_LOADED_ABOVE_4G (1<<0)
    > >> __u32 cmdline_size;
    > >> __u32 hardware_subarch;
    > >> __u64 hardware_subarch_data;
    > >> @@ -106,7 +108,10 @@ struct boot_params {
    > >> __u8 hd1_info[16]; /* obsolete! */ /* 0x090 */
    > >> struct sys_desc_table sys_desc_table; /* 0x0a0 */
    > >> struct olpc_ofw_header olpc_ofw_header; /* 0x0b0 */
    > >> - __u8 _pad4[128]; /* 0x0c0 */
    > >> + __u32 ext_ramdisk_image; /* 0x0c0 */
    > >> + __u32 ext_ramdisk_size; /* 0x0c4 */
    > >> + __u32 ext_cmd_line_ptr; /* 0x0c8 */
    > >> + __u8 _pad4[116]; /* 0x0cc */
    > >> struct edid_info edid_info; /* 0x140 */
    > >> struct efi_info efi_info; /* 0x1c0 */
    > >> __u32 alt_mem_k; /* 0x1e0 */
    > >> @@ -115,7 +120,9 @@ struct boot_params {
    > >> __u8 eddbuf_entries; /* 0x1e9 */
    > >> __u8 edd_mbr_sig_buf_entries; /* 0x1ea */
    > >> __u8 kbd_status; /* 0x1eb */
    > >> - __u8 _pad6[5]; /* 0x1ec */
    > >> + __u8 _pad5[3]; /* 0x1ec */
    > >> + __u8 sentinel; /* 0x1ef */
    > >> + __u8 _pad6[1]; /* 0x1f0 */
    > >
    > > This needs the -v7 explanation from above as a comment here or somewhere
    > > around here, for why we've chosen 0x1ef offset.
    >
    > no, there is no such comment for other fields there.

    That's why I f*cking said "here or somewhere around here"! Or put
    it somewhere else altogether, if you don't like it here but PUT IT
    SOMEWHERE!

    --
    Regards/Gruss,
    Boris.

    Sent from a fat crate under my desk. Formatting is fine.
    --
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2013-01-14 11:42    [W:4.200 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site