grub2 code

grub2 code

[Robert Parnell]

Hi

I was just looking at the grub2 source code and saw that
https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO
<https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO>
says to contact with questions.
I was looking at the source because I encountered a bug on my machine 
that has been previously reported
(https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1027694) and
thought I'd have a look to see if I could find the problem.
So I have 2 questions:
1. Is the code on launchpad the up to date code?
2. Line 177 of
https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S
<https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S>
is a function that flushes the keyboard buffer, could this be what is
causing the bug?

Sorry if 2 seems like a stupid question- I've never looked at the grub
source code before.

Thanks

Rob

Re: grub2 code

[SevenBits]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

On Sunday, February 2, 2014, Robert Parnell <wwolfe9.rp@gmail.com> wrote:

> Hi
>
> I was just looking at the grub2 source code and saw that
>
> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO
> <
> https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO
> >
> says to contact with questions.
> I was looking at the source because I encountered a bug on my machine
> that has been previously reported
> (https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1027694) and
> thought I'd have a look to see if I could find the problem.
> So I have 2 questions:
> 1. Is the code on launchpad the up to date code?


That is probably maintained by the Ubuntu people, so I'd consult them. This
list is generally for general and not distribution specific code since
every distro can potentially use their own modified copies of GRUB.


> 2. Line 177 of
>
> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S
> <
> https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S
> >
> is a function that flushes the keyboard buffer, could this be what is
> causing the bug?
>
> Sorry if 2 seems like a stupid question- I've never looked at the grub
> source code before.
>
> Thanks
>
> Rob
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org <javascript:;>
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2771 bytes --]

Re: grub2 code

[Colin Watson]

On Sun, Feb 02, 2014 at 07:25:51PM +0000, Robert Parnell wrote:
> I was just looking at the grub2 source code and saw that
> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO
> <https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO>
> says to contact with questions.
> I was looking at the source because I encountered a bug on my machine 
> that has been previously reported
> (https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1027694) and
> thought I'd have a look to see if I could find the problem.
> So I have 2 questions:
> 1. Is the code on launchpad the up to date code?

It's an automatically-imported version of the Ubuntu packaging.  It's
up-to-date with what we're shipping, yes, but we do our work in git
these days.  The upstream code is here:

  http://git.savannah.gnu.org/gitweb/?p=grub.git

... and the packaging used by both Debian and Ubuntu is here (the
"experimental" branch is most current):

  http://anonscm.debian.org/gitweb/?p=pkg-grub/grub.git

> 2. Line 177 of
> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S
> <https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S>
> is a function that flushes the keyboard buffer, could this be what is
> causing the bug?

That seems unrelated.

Ben's patch referenced in https://savannah.gnu.org/bugs/?34547 touches
code that I wrote and that I maintain as a patch, so this would
presumably be my problem to fix.  But I don't understand why Ben's patch
fixes anything related to NumLock.  The code being commented out is
essentially:

        movw    $(GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR + 0x17), %bx
        andb    $3, (%bx)

... followed by something that tests the condition flags.  But all that
does is test whether either Shift key is held down, not actually set the
NumLock state (see http://www.bioscentral.com/misc/bda.htm).  That is,
it reads from the BIOS Data Area, but it doesn't write to it.

Ben (CCed), could you shed some light on why your patch would fix this?

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: grub2 code

[Ben Guthro]

On Mon, Feb 3, 2014 at 6:57 AM, Colin Watson <cjwatson@ubuntu.com> wrote:
> On Sun, Feb 02, 2014 at 07:25:51PM +0000, Robert Parnell wrote:
>> I was just looking at the grub2 source code and saw that
>> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO
>> <https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/TODO>
>> says to contact with questions.
>> I was looking at the source because I encountered a bug on my machine
>> that has been previously reported
>> (https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1027694) and
>> thought I'd have a look to see if I could find the problem.
>> So I have 2 questions:
>> 1. Is the code on launchpad the up to date code?
>
> It's an automatically-imported version of the Ubuntu packaging.  It's
> up-to-date with what we're shipping, yes, but we do our work in git
> these days.  The upstream code is here:
>
>   http://git.savannah.gnu.org/gitweb/?p=grub.git
>
> ... and the packaging used by both Debian and Ubuntu is here (the
> "experimental" branch is most current):
>
>   http://anonscm.debian.org/gitweb/?p=pkg-grub/grub.git
>
>> 2. Line 177 of
>> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S
>> <https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S>
>> is a function that flushes the keyboard buffer, could this be what is
>> causing the bug?
>
> That seems unrelated.
>
> Ben's patch referenced in https://savannah.gnu.org/bugs/?34547 touches
> code that I wrote and that I maintain as a patch, so this would
> presumably be my problem to fix.  But I don't understand why Ben's patch
> fixes anything related to NumLock.  The code being commented out is
> essentially:
>
>         movw    $(GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR + 0x17), %bx
>         andb    $3, (%bx)
>
> ... followed by something that tests the condition flags.  But all that
> does is test whether either Shift key is held down, not actually set the
> NumLock state (see http://www.bioscentral.com/misc/bda.htm).  That is,
> it reads from the BIOS Data Area, but it doesn't write to it.
>
> Ben (CCed), could you shed some light on why your patch would fix this?

Adding Roger Cruz, as he was the original author of this fix, where I
was just trying to get it upstreamed - though, this was quite some
time since he developed it (Oct 2011), so I'm sure this isn't exactly
fresh in his mind.

Roger, could you shed some light on  this?

Ben

>
> Thanks,
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]

RE: grub2 code

[Roger Cruz]

This was a bug that was introduced into GRUB when the code to detect if the SHIFT key is being pressed was added.  The symptom was that customers were complaining that even though they had turn on NUMLOCK in the BIOS, once we booted our software, NUMLOCK was off.  It was easy to "see" when the bug was happening because the NUMLOCK LED would turn off sometime during GRUB running. 

I tracked it down to that one line which ANDs the BIOS state area and blows away the NUMLOCK state in it  (it stores the value in the address in BX via indirect addressing).  

Hope this helps.  If it is not clear, please feel free to ask for more details.

Roger R. Cruz


-----Original Message-----
From: ben.guthro@gmail.com [mailto:ben.guthro@gmail.com] On Behalf Of Ben Guthro
Sent: Monday, February 03, 2014 7:42 AM
To: Colin Watson
Cc: grub-devel@gnu.org; Roger Cruz
Subject: Re: grub2 code

On Mon, Feb 3, 2014 at 6:57 AM, Colin Watson <cjwatson@ubuntu.com> wrote:
> On Sun, Feb 02, 2014 at 07:25:51PM +0000, Robert Parnell wrote:
>> I was just looking at the grub2 source code and saw that 
>> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/tru
>> sty/view/head:/TODO 
>> <https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/
>> trusty/view/head:/TODO>
>> says to contact with questions.
>> I was looking at the source because I encountered a bug on my machine 
>> that has been previously reported
>> (https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1027694) and 
>> thought I'd have a look to see if I could find the problem.
>> So I have 2 questions:
>> 1. Is the code on launchpad the up to date code?
>
> It's an automatically-imported version of the Ubuntu packaging.  It's 
> up-to-date with what we're shipping, yes, but we do our work in git 
> these days.  The upstream code is here:
>
>   http://git.savannah.gnu.org/gitweb/?p=grub.git
>
> ... and the packaging used by both Debian and Ubuntu is here (the 
> "experimental" branch is most current):
>
>   http://anonscm.debian.org/gitweb/?p=pkg-grub/grub.git
>
>> 2. Line 177 of
>> https://bazaar.launchpad.net/~ubuntu-branches/ubuntu/trusty/grub2/tru
>> sty/view/head:/grub-core/boot/i386/pc/startup_raw.S
>> <https://bazaar.launchpad.net/%7Eubuntu-branches/ubuntu/trusty/grub2/
>> trusty/view/head:/grub-core/boot/i386/pc/startup_raw.S>
>> is a function that flushes the keyboard buffer, could this be what is 
>> causing the bug?
>
> That seems unrelated.
>
> Ben's patch referenced in https://savannah.gnu.org/bugs/?34547 touches 
> code that I wrote and that I maintain as a patch, so this would 
> presumably be my problem to fix.  But I don't understand why Ben's 
> patch fixes anything related to NumLock.  The code being commented out 
> is
> essentially:
>
>         movw    $(GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR + 0x17), %bx
>         andb    $3, (%bx)
>
> ... followed by something that tests the condition flags.  But all 
> that does is test whether either Shift key is held down, not actually 
> set the NumLock state (see http://www.bioscentral.com/misc/bda.htm).  
> That is, it reads from the BIOS Data Area, but it doesn't write to it.
>
> Ben (CCed), could you shed some light on why your patch would fix this?

Adding Roger Cruz, as he was the original author of this fix, where I was just trying to get it upstreamed - though, this was quite some time since he developed it (Oct 2011), so I'm sure this isn't exactly fresh in his mind.

Roger, could you shed some light on  this?

Ben

>
> Thanks,
>
> --
> Colin Watson                                       [cjwatson@ubuntu.com]

Re: grub2 code

[Colin Watson]

On Mon, Feb 03, 2014 at 03:44:01PM +0000, Roger Cruz wrote:
> This was a bug that was introduced into GRUB when the code to detect
> if the SHIFT key is being pressed was added.  The symptom was that
> customers were complaining that even though they had turn on NUMLOCK
> in the BIOS, once we booted our software, NUMLOCK was off.  It was
> easy to "see" when the bug was happening because the NUMLOCK LED would
> turn off sometime during GRUB running. 
> 
> I tracked it down to that one line which ANDs the BIOS state area and
> blows away the NUMLOCK state in it  (it stores the value in the
> address in BX via indirect addressing).  

Oh, of course, I missed the indirect store even on a second reading.
I'm pretty sure that the correct fix isn't to remove the code, but
rather to use testb instead of andb.  I'll see about getting that fixed
soon.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

RE: grub2 code

[Roger Cruz]

Yes,  that certainly would be a better fix.  Thanks for addressing it.

Roger

-----Original Message-----
From: Colin Watson [mailto:cjwatson@ubuntu.com] 
Sent: Monday, February 03, 2014 10:51 AM
To: Roger Cruz
Cc: Ben Guthro; grub-devel@gnu.org
Subject: Re: grub2 code

On Mon, Feb 03, 2014 at 03:44:01PM +0000, Roger Cruz wrote:
> This was a bug that was introduced into GRUB when the code to detect 
> if the SHIFT key is being pressed was added.  The symptom was that 
> customers were complaining that even though they had turn on NUMLOCK 
> in the BIOS, once we booted our software, NUMLOCK was off.  It was 
> easy to "see" when the bug was happening because the NUMLOCK LED would 
> turn off sometime during GRUB running.
> 
> I tracked it down to that one line which ANDs the BIOS state area and 
> blows away the NUMLOCK state in it  (it stores the value in the 
> address in BX via indirect addressing).

Oh, of course, I missed the indirect store even on a second reading.
I'm pretty sure that the correct fix isn't to remove the code, but rather to use testb instead of andb.  I'll see about getting that fixed soon.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: grub2 code

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On 03.02.2014 12:57, Colin Watson wrote:
>         movw    $(GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR + 0x17), %bx
>         andb    $3, (%bx)
andb $3, (%bx) is equivalent to C code:
*(grub_uint8_t *)bx &= 3;
andb modifies the value at (%bx). You probably meant to use testb which
has same flag semantics but doesn't change operands


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 274 bytes --]