Re: [Xen-devel] [PATCH v2 24/29] Ovmf/Xen: add Xen PV console SerialPortLib driver

2015-02-02 Thread Laszlo Ersek
Unless I'm wrong, please:

On 01/26/15 20:03, Ard Biesheuvel wrote:
 This implements a SerialPortLib instance that wires up to the
 PV console ring used by domU guests. Also imports the required
 upstream Xen io/console.h header.
 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
 ---
  OvmfPkg/Include/IndustryStandard/Xen/io/console.h   |  51 
 ++
  OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c   | 147 
 ++
  OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf |  34 
 +
  3 files changed, 232 insertions(+)
 
 diff --git a/OvmfPkg/Include/IndustryStandard/Xen/io/console.h 
 b/OvmfPkg/Include/IndustryStandard/Xen/io/console.h
 new file mode 100644
 index ..f1caa9765bcf
 --- /dev/null
 +++ b/OvmfPkg/Include/IndustryStandard/Xen/io/console.h
 @@ -0,0 +1,51 @@
 +/**
 + * console.h
 + *
 + * Console I/O interface for Xen guest OSes.
 + *
 + * Permission is hereby granted, free of charge, to any person obtaining a 
 copy
 + * of this software and associated documentation files (the Software), to
 + * deal in the Software without restriction, including without limitation the
 + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
 and/or
 + * sell copies of the Software, and to permit persons to whom the Software is
 + * furnished to do so, subject to the following conditions:
 + *
 + * The above copyright notice and this permission notice shall be included in
 + * all copies or substantial portions of the Software.
 + *
 + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
 THE
 + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 + * DEALINGS IN THE SOFTWARE.
 + *
 + * Copyright (c) 2005, Keir Fraser
 + */
 +
 +#ifndef __XEN_PUBLIC_IO_CONSOLE_H__
 +#define __XEN_PUBLIC_IO_CONSOLE_H__
 +
 +typedef UINT32 XENCONS_RING_IDX;
 +
 +#define MASK_XENCONS_IDX(idx, ring) ((idx)  (sizeof(ring)-1))
 +
 +struct xencons_interface {
 +char in[1024];
 +char out[2048];
 +XENCONS_RING_IDX in_cons, in_prod;
 +XENCONS_RING_IDX out_cons, out_prod;
 +};
 +
 +#endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */
 +
 +/*
 + * Local variables:
 + * mode: C
 + * c-file-style: BSD
 + * c-basic-offset: 4
 + * tab-width: 4
 + * indent-tabs-mode: nil
 + * End:
 + */
 diff --git 
 a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c 
 b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
 new file mode 100644
 index ..97344dc4efb0
 --- /dev/null
 +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
 @@ -0,0 +1,147 @@
 +/** @file
 +  Xen console SerialPortLib instance
 +
 +  Copyright (c) 2015, Linaro Ltd. All rights reserved.BR
 +
 +  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
 +  http://opensource.org/licenses/bsd-license.php
 +
 +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN AS IS BASIS,
 +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
 IMPLIED.
 +
 +**/
 +
 +#include Base.h
 +#include Uefi/UefiBaseType.h
 +
 +#include Library/BaseLib.h
 +#include Library/SerialPortLib.h
 +#include Library/XenHypercallLib.h
 +
 +#include IndustryStandard/Xen/io/console.h
 +#include IndustryStandard/Xen/hvm/params.h
 +#include IndustryStandard/Xen/event_channel.h
 +
 +STATIC evtchn_send_t  mXenConsoleEventChain;
 +STATIC struct xencons_interface   *mXenConsoleInterface;

add a big fat warning above these variables with static storage
duration. You didn't restrict the UEFI phases in which this library is
usable, hence I'm thinking you'll use it in PEIMs and probably even in
SEC. Writable globals in SEC  PEI depend on those phases running out of
DRAM, not flash, which makes this library specific to PrePi. I think the
variables above merit a comment about this.

No other comments from me for this patch (the R-b from Stefano should
suffice).

Thanks
Laszlo

 +
 +RETURN_STATUS
 +EFIAPI
 +SerialPortInitialize (
 +  VOID
 +  )
 +{
 +  mXenConsoleEventChain.port = (UINT32)XenHypercallHvmGetParam 
 (HVM_PARAM_CONSOLE_EVTCHN);
 +  mXenConsoleInterface = (struct xencons_interface *)(UINTN)
 +(XenHypercallHvmGetParam (HVM_PARAM_CONSOLE_PFN)  EFI_PAGE_SHIFT);
 +
 +  //
 +  // No point in ASSERT'ing here as 

Re: [Xen-devel] [PATCH v2 24/29] Ovmf/Xen: add Xen PV console SerialPortLib driver

2015-01-27 Thread Julien Grall
Hi Ard,

On 26/01/15 19:03, Ard Biesheuvel wrote:
 +RETURN_STATUS
 +EFIAPI
 +SerialPortInitialize (
 +  VOID
 +  )
 +{
 +  mXenConsoleEventChain.port = (UINT32)XenHypercallHvmGetParam 
 (HVM_PARAM_CONSOLE_EVTCHN);
 +  mXenConsoleInterface = (struct xencons_interface *)(UINTN)
 +(XenHypercallHvmGetParam (HVM_PARAM_CONSOLE_PFN)  EFI_PAGE_SHIFT);

IIRC, x86 PVH is using a different way to get the console PFN.

It might be good to add a comment stating we only support x86 HVM and
ARM guest here.

Regards,

-- 
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel


Re: [Xen-devel] [PATCH v2 24/29] Ovmf/Xen: add Xen PV console SerialPortLib driver

2015-01-27 Thread Stefano Stabellini
On Mon, 26 Jan 2015, Ard Biesheuvel wrote:
 This implements a SerialPortLib instance that wires up to the
 PV console ring used by domU guests. Also imports the required
 upstream Xen io/console.h header.
 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org

Reviewed-by: Stefano Stabellini stefano.stabell...@eu.citrix.com


  OvmfPkg/Include/IndustryStandard/Xen/io/console.h   |  51 
 ++
  OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c   | 147 
 ++
  OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf |  34 
 +
  3 files changed, 232 insertions(+)
 
 diff --git a/OvmfPkg/Include/IndustryStandard/Xen/io/console.h 
 b/OvmfPkg/Include/IndustryStandard/Xen/io/console.h
 new file mode 100644
 index ..f1caa9765bcf
 --- /dev/null
 +++ b/OvmfPkg/Include/IndustryStandard/Xen/io/console.h
 @@ -0,0 +1,51 @@
 +/**
 + * console.h
 + *
 + * Console I/O interface for Xen guest OSes.
 + *
 + * Permission is hereby granted, free of charge, to any person obtaining a 
 copy
 + * of this software and associated documentation files (the Software), to
 + * deal in the Software without restriction, including without limitation the
 + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
 and/or
 + * sell copies of the Software, and to permit persons to whom the Software is
 + * furnished to do so, subject to the following conditions:
 + *
 + * The above copyright notice and this permission notice shall be included in
 + * all copies or substantial portions of the Software.
 + *
 + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
 THE
 + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
 + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 + * DEALINGS IN THE SOFTWARE.
 + *
 + * Copyright (c) 2005, Keir Fraser
 + */
 +
 +#ifndef __XEN_PUBLIC_IO_CONSOLE_H__
 +#define __XEN_PUBLIC_IO_CONSOLE_H__
 +
 +typedef UINT32 XENCONS_RING_IDX;
 +
 +#define MASK_XENCONS_IDX(idx, ring) ((idx)  (sizeof(ring)-1))
 +
 +struct xencons_interface {
 +char in[1024];
 +char out[2048];
 +XENCONS_RING_IDX in_cons, in_prod;
 +XENCONS_RING_IDX out_cons, out_prod;
 +};
 +
 +#endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */
 +
 +/*
 + * Local variables:
 + * mode: C
 + * c-file-style: BSD
 + * c-basic-offset: 4
 + * tab-width: 4
 + * indent-tabs-mode: nil
 + * End:
 + */
 diff --git 
 a/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c 
 b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
 new file mode 100644
 index ..97344dc4efb0
 --- /dev/null
 +++ b/OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
 @@ -0,0 +1,147 @@
 +/** @file
 +  Xen console SerialPortLib instance
 +
 +  Copyright (c) 2015, Linaro Ltd. All rights reserved.BR
 +
 +  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
 +  http://opensource.org/licenses/bsd-license.php
 +
 +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN AS IS BASIS,
 +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
 IMPLIED.
 +
 +**/
 +
 +#include Base.h
 +#include Uefi/UefiBaseType.h
 +
 +#include Library/BaseLib.h
 +#include Library/SerialPortLib.h
 +#include Library/XenHypercallLib.h
 +
 +#include IndustryStandard/Xen/io/console.h
 +#include IndustryStandard/Xen/hvm/params.h
 +#include IndustryStandard/Xen/event_channel.h
 +
 +STATIC evtchn_send_t  mXenConsoleEventChain;
 +STATIC struct xencons_interface   *mXenConsoleInterface;
 +
 +RETURN_STATUS
 +EFIAPI
 +SerialPortInitialize (
 +  VOID
 +  )
 +{
 +  mXenConsoleEventChain.port = (UINT32)XenHypercallHvmGetParam 
 (HVM_PARAM_CONSOLE_EVTCHN);
 +  mXenConsoleInterface = (struct xencons_interface *)(UINTN)
 +(XenHypercallHvmGetParam (HVM_PARAM_CONSOLE_PFN)  EFI_PAGE_SHIFT);
 +
 +  //
 +  // No point in ASSERT'ing here as we won't be seeing the output
 +  //
 +  return RETURN_SUCCESS;
 +}
 +
 +/**
 +  Write data to serial device.
 +
 +  @param  Buffer   Point of data buffer which need to be written.
 +  @param  NumberOfBytesNumber of output bytes which are cached in Buffer.
 +
 +  @retval 0Write data failed.
 +  @retval !0   Actual number of bytes written to serial device.
 +
 +**/
 +UINTN
 +EFIAPI
 +SerialPortWrite (
 +  IN