Re: [edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

2018-09-11 Thread Zeng, Star 
Reviewed-by: Star Zeng 

-Original Message-
From: Ni, Ruiyu 
Sent: Monday, September 10, 2018 6:05 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A ; Kinney, Michael D 
; Zeng, Star 
Subject: [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

Today's implementation does an exact device path match to check whether the 
device path of a console is in ConIn/ConOut/ErrOut.
But that doesn't work for the USB keyboard.
Because when a platform have multiple USB port, ConIn needs to carry all device 
paths corresponding to each port.
Even worse, today's BDS core logic removes the device path from 
ConIn/ConOut/ErrOut when the connection to that device path fails.
So if user switches the USB keyboard from one port to another across boot, the 
USB keyboard doesn't work in the second boot.

ConPlatform driver solved this problem by adding the
IsHotPlugDevice() function. So that for USB keyboard, ConPlatform doesn't care 
whether its device path is in ConIn or not.
But the rule is too loose, and now causes platform BDS cannot control whether 
to enable USB keyboard as an active console.

The patch changes ConPlatform to support USB short-form device path when 
checking whether the device path of a console is in ConIn/ConOut/ErrOut.

The logic to always accept USB/PCCARD device as active console is removed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Hao A Wu 
Cc: Michael D Kinney 
Cc: Star Zeng 
---
 .../Universal/Console/ConPlatformDxe/ConPlatform.c | 513 ++--- 
 .../Universal/Console/ConPlatformDxe/ConPlatform.h |  24 +-
 .../Console/ConPlatformDxe/ConPlatformDxe.inf  |   1 +
 3 files changed, 339 insertions(+), 199 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c 
b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
index 5fa7facfca..27df8a4b56 100644
--- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
+++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
@@ -202,8 +202,7 @@ ConPlatformDriverBindingSupported (
   Start this driver on ControllerHandle by opening Simple Text Input Protocol,
   reading Device Path, and installing Console In Devcice GUID on 
ControllerHandle.
 
-  If this devcie is not one hot-plug devce, append its device path into the
-  console environment variables ConInDev.
+  Append its device path into the console environment variables ConInDev.
 
   @param  This Protocol instance pointer.
   @param  ControllerHandle Handle of device to bind driver to
@@ -270,58 +269,32 @@ ConPlatformTextInDriverBindingStart (
   }
 
   //
-  // Check the device path, if it is a hot plug device,
-  // do not put the device path into ConInDev, and install
-  // gEfiConsoleInDeviceGuid to the device handle directly.
-  // The policy is, make hot plug device plug in and play immediately.
+  // Append the device path to the ConInDev environment variable
   //
-  if (IsHotPlugDevice (DevicePath)) {
+  ConPlatformUpdateDeviceVariable (
+L"ConInDev",
+DevicePath,
+Append
+);
+
+  //
+  // If the device path is an instance in the ConIn environment 
+ variable,  // then install EfiConsoleInDeviceGuid onto 
+ ControllerHandle  //  if (IsInConInVariable) {
 gBS->InstallMultipleProtocolInterfaces (
,
,
NULL,
NULL
);
-//
-// Append the device path to ConInDev only if it is in ConIn variable.
-//
-if (IsInConInVariable) {
-  ConPlatformUpdateDeviceVariable (
-L"ConInDev",
-DevicePath,
-Append
-);
-}
   } else {
-//
-// If it is not a hot-plug device, append the device path to the
-// ConInDev environment variable
-//
-ConPlatformUpdateDeviceVariable (
-  L"ConInDev",
-  DevicePath,
-  Append
-  );
-
-//
-// If the device path is an instance in the ConIn environment variable,
-// then install EfiConsoleInDeviceGuid onto ControllerHandle
-//
-if (IsInConInVariable) {
-  gBS->InstallMultipleProtocolInterfaces (
- ,
- ,
- NULL,
- NULL
- );
-} else {
-  gBS->CloseProtocol (
- ControllerHandle,
- ,
- This->DriverBindingHandle,
- ControllerHandle
- );
-}
+gBS->CloseProtocol (
+   ControllerHandle,
+   ,
+   This->DriverBindingHandle,
+   ControllerHandle
+   );
   }
 
   return EFI_SUCCESS;
@@ -334,8 +307,7 @@ ConPlatformTextInDriverBindingStart (
   reading Device Path, and installing Console Out Devcic GUID, Standard Error
   Device GUID on ControllerHandle.
 
-  If this devcie is not one hot-plug devce, append its device path into the
-  console environment variables ConOutDev, ErrOutDev.
+  Append its device path into the console environment variables ConOutDev, 
ErrOutDev.
 
   @param

[edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

2018-09-10 Thread Ruiyu Ni 
Today's implementation does an exact device path match to check
whether the device path of a console is in ConIn/ConOut/ErrOut.
But that doesn't work for the USB keyboard.
Because when a platform have multiple USB port, ConIn needs to
carry all device paths corresponding to each port.
Even worse, today's BDS core logic removes the device path from
ConIn/ConOut/ErrOut when the connection to that device path fails.
So if user switches the USB keyboard from one port to another across
boot, the USB keyboard doesn't work in the second boot.

ConPlatform driver solved this problem by adding the
IsHotPlugDevice() function. So that for USB keyboard, ConPlatform
doesn't care whether its device path is in ConIn or not.
But the rule is too loose, and now causes platform BDS cannot control
whether to enable USB keyboard as an active console.

The patch changes ConPlatform to support USB short-form device path
when checking whether the device path of a console is in
ConIn/ConOut/ErrOut.

The logic to always accept USB/PCCARD device as active console is
removed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Hao A Wu 
Cc: Michael D Kinney 
Cc: Star Zeng 
---
 .../Universal/Console/ConPlatformDxe/ConPlatform.c | 513 ++---
 .../Universal/Console/ConPlatformDxe/ConPlatform.h |  24 +-
 .../Console/ConPlatformDxe/ConPlatformDxe.inf  |   1 +
 3 files changed, 339 insertions(+), 199 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c 
b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
index 5fa7facfca..27df8a4b56 100644
--- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
+++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
@@ -202,8 +202,7 @@ ConPlatformDriverBindingSupported (
   Start this driver on ControllerHandle by opening Simple Text Input Protocol,
   reading Device Path, and installing Console In Devcice GUID on 
ControllerHandle.
 
-  If this devcie is not one hot-plug devce, append its device path into the
-  console environment variables ConInDev.
+  Append its device path into the console environment variables ConInDev.
 
   @param  This Protocol instance pointer.
   @param  ControllerHandle Handle of device to bind driver to
@@ -270,58 +269,32 @@ ConPlatformTextInDriverBindingStart (
   }
 
   //
-  // Check the device path, if it is a hot plug device,
-  // do not put the device path into ConInDev, and install
-  // gEfiConsoleInDeviceGuid to the device handle directly.
-  // The policy is, make hot plug device plug in and play immediately.
+  // Append the device path to the ConInDev environment variable
   //
-  if (IsHotPlugDevice (DevicePath)) {
+  ConPlatformUpdateDeviceVariable (
+L"ConInDev",
+DevicePath,
+Append
+);
+
+  //
+  // If the device path is an instance in the ConIn environment variable,
+  // then install EfiConsoleInDeviceGuid onto ControllerHandle
+  //
+  if (IsInConInVariable) {
 gBS->InstallMultipleProtocolInterfaces (
,
,
NULL,
NULL
);
-//
-// Append the device path to ConInDev only if it is in ConIn variable.
-//
-if (IsInConInVariable) {
-  ConPlatformUpdateDeviceVariable (
-L"ConInDev",
-DevicePath,
-Append
-);
-}
   } else {
-//
-// If it is not a hot-plug device, append the device path to the
-// ConInDev environment variable
-//
-ConPlatformUpdateDeviceVariable (
-  L"ConInDev",
-  DevicePath,
-  Append
-  );
-
-//
-// If the device path is an instance in the ConIn environment variable,
-// then install EfiConsoleInDeviceGuid onto ControllerHandle
-//
-if (IsInConInVariable) {
-  gBS->InstallMultipleProtocolInterfaces (
- ,
- ,
- NULL,
- NULL
- );
-} else {
-  gBS->CloseProtocol (
- ControllerHandle,
- ,
- This->DriverBindingHandle,
- ControllerHandle
- );
-}
+gBS->CloseProtocol (
+   ControllerHandle,
+   ,
+   This->DriverBindingHandle,
+   ControllerHandle
+   );
   }
 
   return EFI_SUCCESS;
@@ -334,8 +307,7 @@ ConPlatformTextInDriverBindingStart (
   reading Device Path, and installing Console Out Devcic GUID, Standard Error
   Device GUID on ControllerHandle.
 
-  If this devcie is not one hot-plug devce, append its device path into the
-  console environment variables ConOutDev, ErrOutDev.
+  Append its device path into the console environment variables ConOutDev, 
ErrOutDev.
 
   @param  This Protocol instance pointer.
   @param  ControllerHandle Handle of device to bind driver to
@@ -416,95 +388,59 @@ ConPlatformTextOutDriverBindingStart (
   }
 
   //
-  // Check the device path, if it is a hot plug device,
-  // do not put the device

Re: [edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

2018-05-02 Thread Ni, Ruiyu 


Thanks/Ray

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, April 26, 2018 3:05 PM
> To: Ni, Ruiyu ; edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Kinney, Michael D
> ; Zeng, Star 
> Subject: RE: [PATCH] MdeModulePkg/ConPlatform: Support short-form USB
> device path
> 
> Ray,
> 
> Some minor comments below.
> 
> 1. How MSG_USB_DP and HW_PCCARD_DP be handled? We see they are
> checked in original IsHotPlugDevice()?
Original implementation to always accept USB/PCCARD device as active console is 
wrong.
So these code is just removed.

> 
> 2. gEfiUsbIoProtocolGuid needs be stated in ConPlatformDxe.inf?
Thanks. I will fix it.

> 
> 3. The comment " If it is not a hot-plug device, append the device path to " 
> in
> ConPlatformTextInDriverBindingStart()/ConPlatformTextOutDriverBindingSt
> art() needs be updated accordingly since IsHotPlugDevice() will be removed?
Thanks. I will fix it in v2 patch.

> 
> 
> Thanks,
> Star
> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, April 25, 2018 1:44 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Kinney, Michael D
> ; Zeng, Star 
> Subject: [PATCH] MdeModulePkg/ConPlatform: Support short-form USB
> device path
> 
> Today's implementation does an exact device path match to check
> whether the device path of a console is in ConIn/ConOut/ErrOut.
> But that doesn't work for the USB keyboard.
> Because when a platform have multiple USB port, ConIn needs to
> carry all device paths corresponding to each port.
> Even worse, today's BDS core logic removes the device path from
> ConIn/ConOut/ErrOut when the connection to that device path fails.
> So if user switches the USB keyboard from one port to another across
> boot, the USB keyboard doesn't work in the second boot.
> 
> ConPlatform driver solved this problem by adding the
> IsHotPlugDevice() function. So that for USB keyboard, ConPlatform
> doesn't care whether its device path is in ConIn or not.
> But the rule is too loose, and now causes platform BDS cannot control
> whether to enable USB keyboard as an active console.
> 
> The patch changes ConPlatform to support USB short-form device path
> when checking whether the device path of a console is in
> ConIn/ConOut/ErrOut.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Hao A Wu 
> Cc: Michael D Kinney 
> Cc: Star Zeng 
> ---
>  .../Universal/Console/ConPlatformDxe/ConPlatform.c | 526
> ++---
>  .../Universal/Console/ConPlatformDxe/ConPlatform.h |  20 +-
>  2 files changed, 353 insertions(+), 193 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
> b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
> index 6b53e8ac74..a509d0e3a5 100644
> --- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
> +++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
> @@ -2,7 +2,7 @@
>Console Platform DXE Driver, install Console Device Guids and update
> Console
>Environment Variables.
> 
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
> @@ -270,58 +270,33 @@ ConPlatformTextInDriverBindingStart (
>}
> 
>//
> -  // Check the device path, if it is a hot plug device,
> -  // do not put the device path into ConInDev, and install
> -  // gEfiConsoleInDeviceGuid to the device handle directly.
> -  // The policy is, make hot plug device plug in and play immediately.
> +  // If it is not a hot-plug device, append the device path to the
> +  // ConInDev environment variable
>//
> -  if (IsHotPlugDevice (DevicePath)) {
> +  ConPlatformUpdateDeviceVariable (
> +L"ConInDev",
> +DevicePath,
> +Append
> +);
> +
> +  //
> +  // If the device path is an instance in the ConIn environment variable,
> +  // then install EfiConsoleInDeviceGuid onto ControllerHandle
> +  //
> +  if (IsInConInVariable) {
>  gBS->InstallMultipleProtocolInterfaces (
> ,
> ,
> NULL,
> NULL
> );
> -//
> -// Append the device path to ConInDev only if it is in ConIn variable.
> -//
> -if (IsInConInVariable) {
> -  ConPlatformUpdateDeviceVariable (
> -L"ConInDev",
> -DevicePath,
> -Append
> -);
> -}
>} else {
> -//
> -// If it is not a hot-plug device, append the device path to the
> -// ConInDev environment variable
> -//
> -

Re: [edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

2018-04-26 Thread Zeng, Star 
Ray,

Some minor comments below.

1. How MSG_USB_DP and HW_PCCARD_DP be handled? We see they are checked in 
original IsHotPlugDevice()?

2. gEfiUsbIoProtocolGuid needs be stated in ConPlatformDxe.inf?

3. The comment " If it is not a hot-plug device, append the device path to " in 
ConPlatformTextInDriverBindingStart()/ConPlatformTextOutDriverBindingStart() 
needs be updated accordingly since IsHotPlugDevice() will be removed?


Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Wednesday, April 25, 2018 1:44 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A ; Kinney, Michael D 
; Zeng, Star 
Subject: [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

Today's implementation does an exact device path match to check
whether the device path of a console is in ConIn/ConOut/ErrOut.
But that doesn't work for the USB keyboard.
Because when a platform have multiple USB port, ConIn needs to
carry all device paths corresponding to each port.
Even worse, today's BDS core logic removes the device path from
ConIn/ConOut/ErrOut when the connection to that device path fails.
So if user switches the USB keyboard from one port to another across
boot, the USB keyboard doesn't work in the second boot.

ConPlatform driver solved this problem by adding the
IsHotPlugDevice() function. So that for USB keyboard, ConPlatform
doesn't care whether its device path is in ConIn or not.
But the rule is too loose, and now causes platform BDS cannot control
whether to enable USB keyboard as an active console.

The patch changes ConPlatform to support USB short-form device path
when checking whether the device path of a console is in
ConIn/ConOut/ErrOut.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Hao A Wu 
Cc: Michael D Kinney 
Cc: Star Zeng 
---
 .../Universal/Console/ConPlatformDxe/ConPlatform.c | 526 ++---
 .../Universal/Console/ConPlatformDxe/ConPlatform.h |  20 +-
 2 files changed, 353 insertions(+), 193 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c 
b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
index 6b53e8ac74..a509d0e3a5 100644
--- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
+++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
@@ -2,7 +2,7 @@
   Console Platform DXE Driver, install Console Device Guids and update Console
   Environment Variables.
 
-Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -270,58 +270,33 @@ ConPlatformTextInDriverBindingStart (
   }
 
   //
-  // Check the device path, if it is a hot plug device,
-  // do not put the device path into ConInDev, and install
-  // gEfiConsoleInDeviceGuid to the device handle directly.
-  // The policy is, make hot plug device plug in and play immediately.
+  // If it is not a hot-plug device, append the device path to the
+  // ConInDev environment variable
   //
-  if (IsHotPlugDevice (DevicePath)) {
+  ConPlatformUpdateDeviceVariable (
+L"ConInDev",
+DevicePath,
+Append
+);
+
+  //
+  // If the device path is an instance in the ConIn environment variable,
+  // then install EfiConsoleInDeviceGuid onto ControllerHandle
+  //
+  if (IsInConInVariable) {
 gBS->InstallMultipleProtocolInterfaces (
,
,
NULL,
NULL
);
-//
-// Append the device path to ConInDev only if it is in ConIn variable.
-//
-if (IsInConInVariable) {
-  ConPlatformUpdateDeviceVariable (
-L"ConInDev",
-DevicePath,
-Append
-);
-}
   } else {
-//
-// If it is not a hot-plug device, append the device path to the
-// ConInDev environment variable
-//
-ConPlatformUpdateDeviceVariable (
-  L"ConInDev",
-  DevicePath,
-  Append
-  );
-
-//
-// If the device path is an instance in the ConIn environment variable,
-// then install EfiConsoleInDeviceGuid onto ControllerHandle
-//
-if (IsInConInVariable) {
-  gBS->InstallMultipleProtocolInterfaces (
- ,
- ,
- NULL,
- NULL
- );
-} else {
-  gBS->CloseProtocol (
- ControllerHandle,
- ,
- This->DriverBindingHandle,
- ControllerHandle
- );
-}
+gBS->CloseProtocol (
+   ControllerHandle,
+   ,
+   This->DriverBindingHandle,
+   ControllerHandle
+   );
   }
 
   return EFI_SUCCESS;
@@

Re: [edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

2018-04-26 Thread Ni, Ruiyu 

> -Original Message-
> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Laszlo
> Ersek
> Sent: Wednesday, April 25, 2018 10:16 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>
> Cc: Wu, Hao A <hao.a...@intel.com>; Kinney, Michael D
> <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Zeng, Star
> <star.z...@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-
> form USB device path
> 
> Hello Ray,
> 
> On 04/25/18 07:44, Ruiyu Ni wrote:
> > Today's implementation does an exact device path match to check
> > whether the device path of a console is in ConIn/ConOut/ErrOut.
> > But that doesn't work for the USB keyboard.
> > Because when a platform have multiple USB port, ConIn needs to carry
> > all device paths corresponding to each port.
> > Even worse, today's BDS core logic removes the device path from
> > ConIn/ConOut/ErrOut when the connection to that device path fails.
> > So if user switches the USB keyboard from one port to another across
> > boot, the USB keyboard doesn't work in the second boot.
> >
> > ConPlatform driver solved this problem by adding the
> > IsHotPlugDevice() function. So that for USB keyboard, ConPlatform
> > doesn't care whether its device path is in ConIn or not.
> > But the rule is too loose, and now causes platform BDS cannot control
> > whether to enable USB keyboard as an active console.
> >
> > The patch changes ConPlatform to support USB short-form device path
> > when checking whether the device path of a console is in
> > ConIn/ConOut/ErrOut.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
> > Cc: Hao A Wu <hao.a...@intel.com>
> > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > Cc: Star Zeng <star.z...@intel.com>
> > ---
> >  .../Universal/Console/ConPlatformDxe/ConPlatform.c | 526
> > ++---
> > .../Universal/Console/ConPlatformDxe/ConPlatform.h |  20 +-
> >  2 files changed, 353 insertions(+), 193 deletions(-)
> 
> just a quick question: in OvmfPkg and ArmVirtPkg (more precisely:
> ArmVirtQemu*), we add a "wild card" USB keyboard to ConIn:
> 
> > STATIC PLATFORM_USB_KEYBOARD mUsbKeyboard = {
> >   //
> >   // USB_CLASS_DEVICE_PATH Keyboard
> >   //
> >   {
> > {
> >   MESSAGING_DEVICE_PATH, MSG_USB_CLASS_DP,
> >   DP_NODE_LEN (USB_CLASS_DEVICE_PATH)
> > },
> > 0x, // VendorId: any
> > 0x, // ProductId: any
> > 3,  // DeviceClass: HID
> > 1,  // DeviceSubClass: boot
> > 1   // DeviceProtocol: keyboard
> >   },
> >
> >   //
> >   // EFI_DEVICE_PATH_PROTOCOL End
> >   //
> >   {
> > END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > DP_NODE_LEN (EFI_DEVICE_PATH_PROTOCOL)
> >   }
> > };
> >
> > [...]
> >
> >   //
> >   // Add the hardcoded short-form USB keyboard device path to ConIn.
> >   //
> >   EfiBootManagerUpdateConsoleVariable (ConIn,
> > (EFI_DEVICE_PATH_PROTOCOL *), NULL);
> 
> The idea is to connect any USB keyboard(s) that the virtual machine might
> have.
> 
> This patch for ConPlatformDxe will keep that working, right?
Right!

> 
> Thanks!
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

2018-04-25 Thread Laszlo Ersek 
Hello Ray,

On 04/25/18 07:44, Ruiyu Ni wrote:
> Today's implementation does an exact device path match to check
> whether the device path of a console is in ConIn/ConOut/ErrOut.
> But that doesn't work for the USB keyboard.
> Because when a platform have multiple USB port, ConIn needs to
> carry all device paths corresponding to each port.
> Even worse, today's BDS core logic removes the device path from
> ConIn/ConOut/ErrOut when the connection to that device path fails.
> So if user switches the USB keyboard from one port to another across
> boot, the USB keyboard doesn't work in the second boot.
>
> ConPlatform driver solved this problem by adding the
> IsHotPlugDevice() function. So that for USB keyboard, ConPlatform
> doesn't care whether its device path is in ConIn or not.
> But the rule is too loose, and now causes platform BDS cannot control
> whether to enable USB keyboard as an active console.
>
> The patch changes ConPlatform to support USB short-form device path
> when checking whether the device path of a console is in
> ConIn/ConOut/ErrOut.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Hao A Wu 
> Cc: Michael D Kinney 
> Cc: Star Zeng 
> ---
>  .../Universal/Console/ConPlatformDxe/ConPlatform.c | 526 
> ++---
>  .../Universal/Console/ConPlatformDxe/ConPlatform.h |  20 +-
>  2 files changed, 353 insertions(+), 193 deletions(-)

just a quick question: in OvmfPkg and ArmVirtPkg (more precisely:
ArmVirtQemu*), we add a "wild card" USB keyboard to ConIn:

> STATIC PLATFORM_USB_KEYBOARD mUsbKeyboard = {
>   //
>   // USB_CLASS_DEVICE_PATH Keyboard
>   //
>   {
> {
>   MESSAGING_DEVICE_PATH, MSG_USB_CLASS_DP,
>   DP_NODE_LEN (USB_CLASS_DEVICE_PATH)
> },
> 0x, // VendorId: any
> 0x, // ProductId: any
> 3,  // DeviceClass: HID
> 1,  // DeviceSubClass: boot
> 1   // DeviceProtocol: keyboard
>   },
>
>   //
>   // EFI_DEVICE_PATH_PROTOCOL End
>   //
>   {
> END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> DP_NODE_LEN (EFI_DEVICE_PATH_PROTOCOL)
>   }
> };
>
> [...]
>
>   //
>   // Add the hardcoded short-form USB keyboard device path to ConIn.
>   //
>   EfiBootManagerUpdateConsoleVariable (ConIn,
> (EFI_DEVICE_PATH_PROTOCOL *), NULL);

The idea is to connect any USB keyboard(s) that the virtual machine
might have.

This patch for ConPlatformDxe will keep that working, right?

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] MdeModulePkg/ConPlatform: Support short-form USB device path

2018-04-24 Thread Ruiyu Ni 
Today's implementation does an exact device path match to check
whether the device path of a console is in ConIn/ConOut/ErrOut.
But that doesn't work for the USB keyboard.
Because when a platform have multiple USB port, ConIn needs to
carry all device paths corresponding to each port.
Even worse, today's BDS core logic removes the device path from
ConIn/ConOut/ErrOut when the connection to that device path fails.
So if user switches the USB keyboard from one port to another across
boot, the USB keyboard doesn't work in the second boot.

ConPlatform driver solved this problem by adding the
IsHotPlugDevice() function. So that for USB keyboard, ConPlatform
doesn't care whether its device path is in ConIn or not.
But the rule is too loose, and now causes platform BDS cannot control
whether to enable USB keyboard as an active console.

The patch changes ConPlatform to support USB short-form device path
when checking whether the device path of a console is in
ConIn/ConOut/ErrOut.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Hao A Wu 
Cc: Michael D Kinney 
Cc: Star Zeng 
---
 .../Universal/Console/ConPlatformDxe/ConPlatform.c | 526 ++---
 .../Universal/Console/ConPlatformDxe/ConPlatform.h |  20 +-
 2 files changed, 353 insertions(+), 193 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c 
b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
index 6b53e8ac74..a509d0e3a5 100644
--- a/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
+++ b/MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatform.c
@@ -2,7 +2,7 @@
   Console Platform DXE Driver, install Console Device Guids and update Console
   Environment Variables.
 
-Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -270,58 +270,33 @@ ConPlatformTextInDriverBindingStart (
   }
 
   //
-  // Check the device path, if it is a hot plug device,
-  // do not put the device path into ConInDev, and install
-  // gEfiConsoleInDeviceGuid to the device handle directly.
-  // The policy is, make hot plug device plug in and play immediately.
+  // If it is not a hot-plug device, append the device path to the
+  // ConInDev environment variable
   //
-  if (IsHotPlugDevice (DevicePath)) {
+  ConPlatformUpdateDeviceVariable (
+L"ConInDev",
+DevicePath,
+Append
+);
+
+  //
+  // If the device path is an instance in the ConIn environment variable,
+  // then install EfiConsoleInDeviceGuid onto ControllerHandle
+  //
+  if (IsInConInVariable) {
 gBS->InstallMultipleProtocolInterfaces (
,
,
NULL,
NULL
);
-//
-// Append the device path to ConInDev only if it is in ConIn variable.
-//
-if (IsInConInVariable) {
-  ConPlatformUpdateDeviceVariable (
-L"ConInDev",
-DevicePath,
-Append
-);
-}
   } else {
-//
-// If it is not a hot-plug device, append the device path to the
-// ConInDev environment variable
-//
-ConPlatformUpdateDeviceVariable (
-  L"ConInDev",
-  DevicePath,
-  Append
-  );
-
-//
-// If the device path is an instance in the ConIn environment variable,
-// then install EfiConsoleInDeviceGuid onto ControllerHandle
-//
-if (IsInConInVariable) {
-  gBS->InstallMultipleProtocolInterfaces (
- ,
- ,
- NULL,
- NULL
- );
-} else {
-  gBS->CloseProtocol (
- ControllerHandle,
- ,
- This->DriverBindingHandle,
- ControllerHandle
- );
-}
+gBS->CloseProtocol (
+   ControllerHandle,
+   ,
+   This->DriverBindingHandle,
+   ControllerHandle
+   );
   }
 
   return EFI_SUCCESS;
@@ -416,95 +391,60 @@ ConPlatformTextOutDriverBindingStart (
   }
 
   //
-  // Check the device path, if it is a hot plug device,
-  // do not put the device path into ConOutDev and ErrOutDev,
-  // and install gEfiConsoleOutDeviceGuid to the device handle directly.
-  // The policy is, make hot plug device plug in and play immediately.
+  // If it is not a hot-plug device, append the device path to 
+  // the ConOutDev and ErrOutDev environment variable.
+  // For GOP device path, append the sibling device path as well.
+  //
+  if (!ConPlatformUpdateGopCandidate (DevicePath)) {
+ConPlatformUpdateDeviceVariable (
+  L"ConOutDev",
+  DevicePath,
+  Append
+  );
+//
+// Then append the device path to the ErrOutDev environment