Folks,
I am happy to take this patch but I need it in proper form. Michael,
"git format-patch" and "git send-email" are your friends.
Daniel
On Wed, Sep 11, 2019 at 12:29:42AM +0200, Michael Bideau wrote:
> Hi Paul and everyone,
>
> My turn to be sorry for the late reply.
> I was on my first sailing cruise last week, no computer on board. Scary
> (45 knots), beautiful and a real team effort. Like grub dev' ahah.
>
> My answer inline below...
>
> Best regards,
>
> Michael
>
> Le mardi 03 septembre 2019 à 13:48 +0200, Paul Menzel a écrit :
> > Dear Michael,
> >
> >
> > I am sorry for the late reply.
> >
> >
> > On 2019-08-29 00:24, Michael Bideau wrote:
> >
> > > Le mardi 27 août 2019 à 11:57 +0200, Paul Menzel a écrit :
> > > > On 8/24/19 9:09 PM, Michael Bideau wrote:
> > > >
> > > > > This patch fixes an issue that prevented the 'at_keyboard'
> > > > > module
> > > > > to work (for me).
> > > > >
> > > > > The cause is a bad/wrong return value in the function
> > > > > 'grub_at_keyboard_getkey()' in file
> > > > > 'grub-core/term/at_keyboard.c' at line 234.
> > > > >
> > > > > ///////// patch /////////
> > > > > diff --git a/grub-core/term/at_keyboard.c b/grub-
> > > > > core/term/at_keyboard.c
> > > > > index f0a986eb1..597111077 100644
> > > > > --- a/grub-core/term/at_keyboard.c
> > > > > +++ b/grub-core/term/at_keyboard.c
> > > > > @@ -234,7 +234,7 @@ grub_at_keyboard_getkey (struct
> > > > > grub_term_input
> > > > > *term __attribute__ ((unused)))
> > > > > return GRUB_TERM_NO_KEY;
> > > > >
> > > > > if (! KEYBOARD_ISREADY (grub_inb (KEYBOARD_REG_STATUS)))
> > > > > - return -1;
> > > > > + return GRUB_TERM_NO_KEY;
> > > > > at_key = grub_inb (KEYBOARD_REG_DATA);
> > > > > old_led = ps2_state.led_status;
> > > > > ///////// end of patch /////////
> > > > >
> > > > >
> > > > > My symptoms were to have an unresponsive keyboard: keys needed
> > > > > to be pressed 10x and more to effectively be printed, sometimes
> > > > > generating multiple key presses (after 1 or 2 sec of no
> > > > > printing). Very problematic for typing passphrase in early
> > > > > stage
> > > > > (with GRUB_ENABLE_CRYPTODISK). When switching to 'console'
> > > > > terminal input, keyboard works perfectly. It also worked great
> > > > > with grub 2.02 packaged by Debian (2.02+dfsg1- 20). It was not
> > > > > an
> > > > > output issue, but an input one.
> > > >
> > > > […]
> > > > I think I had a similar issue and tried to fix it in commit
> > > > d3a3543a
> > > > (normal/menu: Do not treat error values as key presses) [1],
> > > > present
> > > > in GRUB 2.04. Do you have that commit in your tree?
> > >
> > > Yes I have this/your commit in my tree.
> > >
> > > ///////// your patch d3a3543a /////////
> > > diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
> > > index e7a83c2d6..d5e0c79a7 100644
> > > --- a/grub-core/normal/menu.c
> > > +++ b/grub-core/normal/menu.c
> > > @@ -698,7 +698,8 @@ run_menu (grub_menu_t menu, int nested, int
> > > *auto_boot)
> > >
> > > c = grub_getkey_noblock ();
> > >
> > > - if (c != GRUB_TERM_NO_KEY)
> > > + /* Negative values are returned on error. */
> > > + if ((c != GRUB_TERM_NO_KEY) && (c > 0))
> > > {
> > > if (timeout >= 0)
> > > {
> > > ///////// end of your patch d3a3543a /////////
> > >
> > > For what I understand, your patch seems to "only" address
> > > normal/menu
> > > timeout issue (but I might be wrong, I'm still very new to grub2
> > > code),
> > > but mine fixes the keyboard (if input is 'at_keyboard') wherever it
> > > is
> > > used in the normal/menu or in the grub terminal/shell or even with
> > > the
> > > cryptomount utility (typing passphrase was my issue at the
> > > beginning).
> > >
> > > To be clear, with the last checkout (commit 4e75b2ae3), that
> > > includes
> > > your commit d3a3543a, I have my keyboard not working reliably (as
> > > described previously), and with my patch it works perfectly
> > > (meaning
> > > in the grub terminal/shell I can type commands seamlessly and even
> > > use different keyboard layouts).
> >
> > You are correct. Thank you for the analysis. Could you please create
> > a
> > “proper” commit with a commit message (most can be taken from your
> > mail), and send that to this list?
>
> The "proper" commit :
>
> ///////// show 261dd4318 /////////
> at_keyboard: fix unreliable key presses
>
> This patch fixes an issue that prevented the 'at_keyboard' module to work
> (for me).
>
> The cause is a bad/wrong return value in the function
> 'grub_at_keyboard_getkey()' in file 'grub-core/term/at_keyboard.c' at line
> 237.
>
> My symptoms were to have an unresponsive keyboard: keys needed to be pressed
> 10x and more to
> effectively be printed, sometimes generating multiple key presses (after 1 or
> 2 sec of no printing).
> Very problematic for typing passphrase in early stage (with
> GRUB_ENABLE_CRYPTODISK).
> When switching to 'console' terminal input, keyboard works perfectly.
> It also worked great with grub 2.02 packaged by Debian (2.02+dfsg1-20).
> It was not an output issue, but an input one.
>
> I've managed to analyse the issue and found where it came from: the following
> commit:
> ---------- commit ----------
> Commit: 216950a4eea1a1ead1c28eaca94e34ea2ef2ad19
> Author: Vladimir Serbinenko <phco...@gmail.com>
> Date: Mon May 8 21:41:22 2017 +0200
>
> at_keyboard: Split protocol from controller code.
>
> On vexpress controller is different but protocol is the same, so reuse the
> code.
> ---------- end of commit ----------
>
> 3 lines where moved from the function 'fetch_key()' in file
> 'grub-core/term/at_keyboard.c', to the begining of function
> 'grub_at_keyboard_getkey()' (same file).
> But returning '-1' made sense when in function 'fetch_key()' but not anymore
> in function 'grub_at_keyboard_getkey()', which should return
> 'GRUB_TERM_NO_KEY'.
>
> I think it was just an incomplete cut-paste, missing a small manual
> correction.
> Better fix it.
> ///////// end of commit /////////
>
> Please, feel free to edit/cut the commit message as you wish.
>
> > By the way, do you only have this problem on one machine or all? Can
> > it
> > be reproduced in QEMU?
>
> Actually I only have that issue with my libvirt-qemu-kvm virtual
> machines (where I do grub tweaking and theming).
> I've never used qemu directly, only through libvirt and with a GUI
> (virt-manager).
>
> But I can provide you the log of libvirt containing the qemu command,
> and the dump of my VM configuration (and some host informations). See
> below.
>
> ///////// log of libvirt: /var/log/libvirt/qemu/debian9.log /////////
> 019-09-10 21:27:15.157+0000: starting up libvirt version: 4.10.0, package: 2
> (Guido Günther <a...@sigxcpu.org> Tue, 18 Dec 2018 12:55:10 +0100), qemu
> version: 2.12.0Debian 1:2.12+dfsg-3+b1, kernel: 4.9.0-8-amd64, hostname:
> debian-host
> LC_ALL=C PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
> QEMU_AUDIO_DRV=spice /usr/bin/kvm -name guest=debian9,debug-threads=on -S
> -object
> secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-1-debian9/master-key.aes
> -machine pc-i440fx-2.8,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu
> Nehalem -m 1024 -realtime mlock=off -smp 1,sockets=1,cores=1,threads=1 -uuid
> 360d8792-c34f-43d6-bc80-149de68ff11b -no-user-config -nodefaults -chardev
> socket,id=charmonitor,fd=26,server,nowait -mon
> chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew
> -global kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global
> PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot menu=on,strict=on
> -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 -device
> ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
> -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1
> -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2
> -device lsi,id=scsi0,bus=pci.0,addr=0x6 -device
> ahci,id=sata0,bus=pci.0,addr=0x7 -device
> virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive
> file=/var/lib/libvirt/images/debian-9.5.0-amd64-netinst.iso,format=raw,if=none,id=drive-ide0-0-0,readonly=on
> -device ide-cd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=2
> -drive
> file=/home/michael/VMs/test-btrfs1.qcow2,format=qcow2,if=none,id=drive-sata0-0-0
> -device ide-hd,bus=sata0.0,drive=drive-sata0-0-0,id=sata0-0-0,bootindex=1
> -drive
> file=/home/michael/VMs/test-btrfs2.qcow2,format=qcow2,if=none,id=drive-sata0-0-1
> -device ide-hd,bus=sata0.1,drive=drive-sata0-0-1,id=sata0-0-1 -netdev
> tap,fd=28,id=hostnet0,vhost=on,vhostfd=29 -device
> virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:68:f1:50,bus=pci.0,addr=0x3
> -chardev pty,id=charserial0 -device
> isa-serial,chardev=charserial0,id=serial0 -chardev
> socket,id=charchannel0,fd=30,server,nowait -device
> virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
> -chardev spicevmc,id=charchannel1,name=vdagent -device
> virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.0
> -device usb-tablet,id=input0,bus=usb.0,port=1 -spice
> port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on
> -device
> qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
> -chardev spicevmc,id=charredir0,name=usbredir -device
> usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 -chardev
> spicevmc,id=charredir1,name=usbredir -device
> usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 -device
> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 -sandbox
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg
> timestamp=on
>
> 2019-09-10T21:27:15.760474Z qemu-system-x86_64: -chardev pty,id=charserial0:
> char device redirected to /dev/pts/0 (label charserial0)
> ///////// end of log /////////
>
>
> ///////// XML output of command: sudo virsh dumpxml debian9 /////////
> <domain type='kvm' id='1'>
> <name>debian9</name>
> <uuid>360d8792-c34f-43d6-bc80-149de68ff11b</uuid>
> <memory unit='KiB'>1048576</memory>
> <currentMemory unit='KiB'>1048576</currentMemory>
> <vcpu placement='static'>1</vcpu>
> <resource>
> <partition>/machine</partition>
> </resource>
> <os>
> <type arch='x86_64' machine='pc-i440fx-2.8'>hvm</type>
> <bootmenu enable='yes'/>
> </os>
> <features>
> <acpi/>
> <apic/>
> <vmport state='off'/>
> </features>
> <cpu mode='custom' match='exact' check='full'>
> <model fallback='forbid'>Nehalem</model>
> <feature policy='require' name='vme'/>
> <feature policy='require' name='x2apic'/>
> <feature policy='require' name='hypervisor'/>
> </cpu>
> <clock offset='utc'>
> <timer name='rtc' tickpolicy='catchup'/>
> <timer name='pit' tickpolicy='delay'/>
> <timer name='hpet' present='no'/>
> </clock>
> <on_poweroff>destroy</on_poweroff>
> <on_reboot>restart</on_reboot>
> <on_crash>restart</on_crash>
> <pm>
> <suspend-to-mem enabled='no'/>
> <suspend-to-disk enabled='no'/>
> </pm>
> <devices>
> <emulator>/usr/bin/kvm</emulator>
> <disk type='file' device='cdrom'>
> <driver name='qemu' type='raw'/>
> <source file='/var/lib/libvirt/images/debian-9.5.0-amd64-
> netinst.iso'/>
> <backingStore/>
> <target dev='hda' bus='ide'/>
> <readonly/>
> <boot order='2'/>
> <alias name='ide0-0-0'/>
> <address type='drive' controller='0' bus='0' target='0'
> unit='0'/>
> </disk>
> <disk type='file' device='disk'>
> <driver name='qemu' type='qcow2'/>
> <source file='/home/michael/VMs/test-btrfs1.qcow2'/>
> <backingStore/>
> <target dev='sda' bus='sata'/>
> <boot order='1'/>
> <alias name='sata0-0-0'/>
> <address type='drive' controller='0' bus='0' target='0'
> unit='0'/>
> </disk>
> <disk type='file' device='disk'>
> <driver name='qemu' type='qcow2'/>
> <source file='/home/michael/VMs/test-btrfs2.qcow2'/>
> <backingStore/>
> <target dev='sdb' bus='sata'/>
> <alias name='sata0-0-1'/>
> <address type='drive' controller='0' bus='0' target='0'
> unit='1'/>
> </disk>
> <controller type='usb' index='0' model='ich9-ehci1'>
> <alias name='usb'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x7'/>
> </controller>
> <controller type='usb' index='0' model='ich9-uhci1'>
> <alias name='usb'/>
> <master startport='0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x0' multifunction='on'/>
> </controller>
> <controller type='usb' index='0' model='ich9-uhci2'>
> <alias name='usb'/>
> <master startport='2'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x1'/>
> </controller>
> <controller type='usb' index='0' model='ich9-uhci3'>
> <alias name='usb'/>
> <master startport='4'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x2'/>
> </controller>
> <controller type='pci' index='0' model='pci-root'>
> <alias name='pci.0'/>
> </controller>
> <controller type='ide' index='0'>
> <alias name='ide'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> function='0x1'/>
> </controller>
> <controller type='virtio-serial' index='0'>
> <alias name='virtio-serial0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x05'
> function='0x0'/>
> </controller>
> <controller type='sata' index='0'>
> <alias name='sata0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x07'
> function='0x0'/>
> </controller>
> <controller type='scsi' index='0' model='lsilogic'>
> <alias name='scsi0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x06'
> function='0x0'/>
> </controller>
> <interface type='network'>
> <mac address='52:54:00:68:f1:50'/>
> <source network='default' bridge='virbr0'/>
> <target dev='vnet0'/>
> <model type='virtio'/>
> <alias name='net0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
> </interface>
> <serial type='pty'>
> <source path='/dev/pts/0'/>
> <target type='isa-serial' port='0'>
> <model name='isa-serial'/>
> </target>
> <alias name='serial0'/>
> </serial>
> <console type='pty' tty='/dev/pts/0'>
> <source path='/dev/pts/0'/>
> <target type='serial' port='0'/>
> <alias name='serial0'/>
> </console>
> <channel type='unix'>
> <source mode='bind'
> path='/var/lib/libvirt/qemu/channel/target/domain-1-
> debian9/org.qemu.guest_agent.0'/>
> <target type='virtio' name='org.qemu.guest_agent.0'
> state='disconnected'/>
> <alias name='channel0'/>
> <address type='virtio-serial' controller='0' bus='0' port='1'/>
> </channel>
> <channel type='spicevmc'>
> <target type='virtio' name='com.redhat.spice.0'
> state='disconnected'/>
> <alias name='channel1'/>
> <address type='virtio-serial' controller='0' bus='0' port='2'/>
> </channel>
> <input type='tablet' bus='usb'>
> <alias name='input0'/>
> <address type='usb' bus='0' port='1'/>
> </input>
> <input type='mouse' bus='ps2'>
> <alias name='input1'/>
> </input>
> <input type='keyboard' bus='ps2'>
> <alias name='input2'/>
> </input>
> <graphics type='spice' port='5900' autoport='yes'
> listen='127.0.0.1'>
> <listen type='address' address='127.0.0.1'/>
> <image compression='off'/>
> </graphics>
> <video>
> <model type='qxl' ram='65536' vram='65536' vgamem='16384'
> heads='1' primary='yes'/>
> <alias name='video0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
> function='0x0'/>
> </video>
> <redirdev bus='usb' type='spicevmc'>
> <alias name='redir0'/>
> <address type='usb' bus='0' port='2'/>
> </redirdev>
> <redirdev bus='usb' type='spicevmc'>
> <alias name='redir1'/>
> <address type='usb' bus='0' port='3'/>
> </redirdev>
> <memballoon model='virtio'>
> <stats period='5'/>
> <alias name='balloon0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x08'
> function='0x0'/>
> </memballoon>
> </devices>
> <seclabel type='dynamic' model='dac' relabel='yes'>
> <label>+64055:+64055</label>
> <imagelabel>+64055:+64055</imagelabel>
> </seclabel>
> </domain>
> ///////// XML end of output /////////
>
> Hope this helps.
> If you need more informations, I'll be glad to dig.
>
> > > PS: there is a typo in my introduction of my patch: it is at the
> > > line
> > > 237 and not 234 as mentioned. Do you think I should answer to my
> > > first
> > > email to say it (so people not reading this sub-thread will know)?
> >
> > You can fix that in the commit message.
> >
> > Please ask if you need further help.
>
> Thank you very much for your help and kind support.
>
> > Kind regards,
> >
> > Paul
> >
> >
> > > > [1]:
> > > > https://git.savannah.gnu.org/cgit/grub.git/commit/?id=d3a3543a5666c1dd180ae6027948ca753dcffc18
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
