Re: [PATCH v2 0/3] Ovmf: Stop using ISA drivers within IntelFrameworkModulePkg

[Laszlo Ersek <lersek@redhat.com>]

On 03/26/19 03:49, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, March 25, 2019 7:29 PM
>> To: Wu, Hao A; edk2-devel@lists.01.org; Julien Grall; Anthony Perard
>> Cc: Justen, Jordan L; Ard Biesheuvel; Ni, Ray
>> Subject: Re: [PATCH v2 0/3] Ovmf: Stop using ISA drivers within
>> IntelFrameworkModulePkg
>>
>> On 03/25/19 11:58, Laszlo Ersek wrote:
>>> On 03/25/19 06:28, Hao Wu wrote:
>>>> The series is also available at:
>>>> https://github.com/hwu25/edk2/tree/ovmf_siobus_v2
>>>>
>>>> V2 changes:
>>>> * Introduce a static build flag 'USE_LEGACY_ISA_STACK' in OVMF DSC files
>>>>   for users to select between the ISA driver stacks.
>>>> * V1 patch 2/2 is split into 2 patches in V2. The first one will add the
>>>>   new OVMF SioBusDxe driver and list it in the DSC files. Then second one
>>>>   will add the whole new ISA stack in DSC/FDF files.
>>>>
>>>>
>>>> V1 history:
>>>>
>>>> This series will update the OVMF to stop using the ISA drivers within
>>>> IntelFrameworkModulePkg.
>>>>
>>>> As the replacement, a new OVMF Super I/O bus driver has been add which
>>>> will install the Super I/O protocol for ISA serial and PS2 keyboard
>>>> devices. By doing so, these devices can be managed by:
>>>>
>>>>   MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
>>>>   MdeModulePkg/Bus/Isa/Ps2KeyboardDxe/Ps2KeyboardDxe.inf
>>>>
>>>> respectively.
>>>>
>>>>
>>>> Tests done:
>>>> A. GCC5 & VS2015x86 tool chains build pass
>>>> B. Launch QEMU (2.4.50, Windows) with command:
>>>>    > qemu-system-x86_64.exe -pflash <SOME_PATH>\OVMF.fd -serial
>> file:1.txt -serial file:2.txt
>>>>
>>>>    Able to see the ISA COM1/COM2 UART and PS2Keyboard devices under
>> Shell
>>>>    using command 'devtree';
>>>>
>>>>    Both the serials and PS2 keyboard are working fine;
>>>
>>> Can you please confirm the following:
>>>
>>> (1) In the PrepareLpcBridgeDevicePath() function, in file
>>> "OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c", we add
>>> IsaKeyboard to ConIn, and IsaSerial to ConOut, ConIn, ErrOut.
>>>
>>> This function takes the "LPC Bridge device" handle from its caller,
>>> namely DetectAndPreparePlatformPciDevicePath(), and appends some
>>> constant device path nodes, from "BdsPlatform.h" / "PlatformData.c".
>>>
>>> Can you please confirm that the existing Platform BDS code described
>>> above is compatible with the new driver?
>>>
>>> In other words, do DetectAndPreparePlatformPciDevicePath() +
>>> PrepareLpcBridgeDevicePath() still add the proper device paths to
>>> ConIn/ConOut/ErrOut?
>>>
>>> (Note, they need not be identical to the previous device paths, but the
>>> *logic* must continue to work -- i.e. *some* device paths have to be
>>> added, and they should be correct.)
> 
> Hello Laszlo,
> 
> For (1), since I saw in function PrepareLpcBridgeDevicePath() there are
> already some codes to print out the device path for COM1/COM2. So I just
> add some logic to print the device path for the PS2 keyboard as well.
> 
> I ran the qemu with command:
> qemu-system-x86_64.exe -pflash <SOME_PATH>\OVMF.fd -debugcon file:boot.log -global isa-debugcon.iobase=0x402
> 
> For the new and origin ISA stack, their logs both show:
> Found LPC Bridge device
> BdsPlatform.c+585: PS2 Keyboard DevPath: PciRoot(0x0)/Pci(0x1,0x0)/Acpi(PNP0303,0x0)
> BdsPlatform.c+615: COM1 DevPath: PciRoot(0x0)/Pci(0x1,0x0)/Serial(0x0)/Uart(115200,8,N,1)/VenMsg(E0C14753-F9BE-11D2-9A0C-0090273FC14D)
> BdsPlatform.c+647: COM2 DevPath: PciRoot(0x0)/Pci(0x1,0x0)/Serial(0x1)/Uart(115200,8,N,1)/VenMsg(E0C14753-F9BE-11D2-9A0C-0090273FC14D)
> 
> Thus, I think the Platform BDS code behavior remains the same.
> 
>>>
>>> (2) Can you please confirm if the new build survives repeated
>>>
>>>   reconnect -r
>>>
>>> commands in the UEFI shell? Both the ISA keyboard and the serial console
>>> should resume working after "reconnect -r".
> 
> For (2), I also run the qemu with:
> qemu-system-x86_64.exe -pflash <SOME_PATH>\OVMF.fd -debugcon file:boot.log -global isa-debugcon.iobase=0x402
> 
> After running 'reconnect -r' under Shell, the PS2 keyboard works fine.
> Then I switch the display from 'VGA' to 'serial0', and the serial works
> fine. (I just re-run the 'reconnect -r' under 'serial0', after the command
> finishes, the keyboard is responding and there is still output on 'serial0'.)
> 
>>
>> (3) Hao, can you please verify the above steps on the "q35" machine type
>> as well?
>>
>> (The QEMU command line that you mention selects the "i440fx" machine
>> type. QEMU provides an ICH9 ISA controller with the q35 board, and a
>> PIIX4 one with the i440fx board. I think we should regression-test this
>> work with both.)
> 
> For (3), I run qemu with:
> qemu-system-x86_64.exe -machine q35 -pflash <SOME_PATH>\OVMF.fd -debugcon file:boot.log -global isa-debugcon.iobase=0x402
> 
> Both tests (1) and (2) passed in this case.
> 
> 
> Please help to let me know if there is anything not right in the above
> tests. Thanks.

All of the above sounds great, thank you for the thorough testing.

Please let's wait for (4) below, and also let's sort out the license
identifier scheduling.

Thanks!
Laszlo

>>
>> (4) Julien, Anthony: can you please fetch this series (github URL at the
>> top) and see if the PS/2 keyboard, and the serial port, still work on
>> Xen, to interact e.g. with the UEFI shell?
>>
>> Thanks!
>> Laszlo