Arm virtio keyboard driver

[elkoniu]

Description

This set of commits introduces virtio based keyboard driver. The main idea is to have a keyboard driver which is more "light" than standard USB based driver. This may be better for some small ARM based platforms with limited resources.

• [ ] Breaking change?
  • Breaking change - Does this PR cause a break in build or boot behavior?
  • Examples: Does it add a new library class or move a module to a different repo.
• [ ] Impacts security?
  • Security - Does this PR have a direct security impact?
  • Examples: Crypto algorithm change or buffer overflow fix.
• [ ] Includes tests?
  • Tests - Does this PR include any explicit test code?
  • Examples: Unit tests or integration tests.

How This Was Tested

I have tested this manually with RPi5 ARM platform and qemu. For test I have navigated over efi shell, config and grub bootloader window running from Fedora ISO.

CODE=/home/koniu/RedHat/git/virt/edk2/Build/Ovmf3264/DEBUG_GCC5/FV/OVMF_CODE.fd VARS=/home/koniu/RedHat/git/virt/edk2/Build/Ovmf3264/DEBUG_GCC5/FV/OVMF_VARS.fd

qemu-system-x86_64
-blockdev node-name=code,driver=file,filename=${CODE},read-only=on
-drive if=none,id=vars,format=raw,file=${VARS},snapshot=on
-machine q35,kernel-irqchip=on,smm=on,pflash0=code,pflash1=vars
-accel kvm -m 4G -cpu host -smp cores=2,threads=1
-chardev stdio,id=fwlog
-device isa-debugcon,iobase=0x402,chardev=fwlog
-net none
-device virtio-keyboard-pci
-boot d -cdrom ~/Pobrane/Fedora.iso

Integration Instructions

N/A

[elkoniu]

@kraxel @osteffenrh - following latest EDK2 development guidelines, instead of mailing list I have created PR here for the easier review and discussion :)

[osteffenrh]

CI found some errors:

---

WARNING - Uncrustify found 3 files with formatting errors
CRITICAL - Visit the following instructions to learn how to find the detailed formatting errors in Azure DevOps CI: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting#how-to-find-uncrustify-formatting-errors-in-continuous-integration-ci
INFO - Calculating file diffs. This might take a while...
INFO - Formatting errors in Library/PlatformBootManagerLib/BdsPlatform.c
INFO - Formatting errors in Include/IndustryStandard/Virtio10.h
INFO - Formatting errors in Include/IndustryStandard/input-event-codes.h
ERROR - --->Test Failed: Uncrustify Coding Standard Test NO-TARGET returned 3

---

52/96 ./ArmVirtPkg/ArmVirtQemu.dsc 112.01ms X
INFO - /__w/1/s/ArmVirtPkg/ArmVirtQemu.dsc:34:10 - Unknown word (CAVIUM)
INFO - /__w/1/s/ArmVirtPkg/ArmVirtQemu.dsc:128:7 - Unknown word (CAVIUM)
INFO - /__w/1/s/ArmVirtPkg/ArmVirtQemu.dsc:129:31 - Unknown word (DCAVIUM)

Strange that the spell check complains about that. This is code and not regular text:

[BuildOptions]
!if $(CAVIUM_ERRATUM_27456) == TRUE
  GCC:*_*_AARCH64_PP_FLAGS = -DCAVIUM_ERRATUM_27456
!endif

---

ERROR - Invalid license in: OvmfPkg/Include/IndustryStandard/input-event-codes.h Hint: Only BSD-2-Clause-Patent is accepted.
ERROR - --->Test Failed: License Check Test NO-TARGET returned 1

[kraxel]

We cannot incorporate GPL sources. Why do we need this?

The GPL file is a straight copy from the linux kernel source tree (include/uapi/linux/input-event-codes.h). Needed because of the keycodes (the KEY_* defines). It only defines an API, it isn't actual code, so I think copying this over should be fine license-wise. Nevertheless CI is probably unhappy because of the SPDX tag in there. Suggestions how to move forward best?

The driver is useful to have keyboard support without depending either PS/2 (which is x86-only) or USB.

[elkoniu]

I can eventually copy over keycodes I am using but it may not be the "cleanest" solution to this problem. Yet it would satisfy the CI.

[kraxel]

It is possible to exclude files from licence checks via OvmfPkg.ci.yaml. See https://github.com/tianocore/edk2/pull/5698 where the same is done for a header file copied over from Xen.

[ardbiesheuvel]

It is possible to exclude files from licence checks via OvmfPkg.ci.yaml. See #5698 where the same is done for a header file copied over from Xen.

This presupposes that it is actually fine to incorporate this code, and I am not convinced about that.

cc @leiflindholm @mdkinney

[leiflindholm]

It is possible to exclude files from licence checks via OvmfPkg.ci.yaml. See #5698 where the same is done for a header file copied over from Xen.

This presupposes that it is actually fine to incorporate this code, and I am not convinced about that.

cc @leiflindholm @mdkinney

No, we can't have GPL in this repo. I'm not super happy about the MIT situation either (because that loses the explicit patent grant, which perhaps could be resolved by an SPDX "and" statement?), but clearly we dropped the ball there.

If the argument is that there is no copyright in effect on this header, then the submitter should be able to change the license outright and still comply with the DCO. Are we happy there is no copyright? (I'm not entirely convinced.)

[elkoniu]

What if I will just copy needed definitions directly into the driver code / header? It would solve the issue but eventually in the future we could go out of sync with the original header.

[leiflindholm]

What if I will just copy needed definitions directly into the driver code / header? It would solve the issue but eventually in the future we could go out of sync with the original header.

TL;DR: yes, that works for me.

Given that this header is (unfortunately) referenced as the canonical definition by the virtio spec, it wouldn't seem to make sense to use any other source.

But the following statement in the original header https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input-event-codes.h?h=v6.11-rc6#n67: "Most of the keys/buttons are modeled after USB HUT 1.12 (see http://www.usb.org/developers/hidpage)." makes me more comfortable about copying the needed definitions (as opposed to the whole file).

[kraxel]

What if I will just copy needed definitions directly into the driver code / header? It would solve the issue but eventually in the future we could go out of sync with the original header.

I don't think we hve to worry too much about things running out of sync. Even if there are new keys added down the road (such as microsoft defining a new copilot key) it is highly unlikely that supporting them makes sense for the firmware.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

[github-actions[bot]]

This pull request has been automatically been closed because it did not have any activity in 60 days and no follow up within 7 days after being marked stale. Thank you for your contributions.

[elkoniu]

I missed the fact that GH closed this PR and I cant overwrite this. I have addressed review comments and opened new PR: https://github.com/tianocore/edk2/pull/6444 @kraxel @osteffenrh @leiflindholm