Date   

Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

Tyler J Erickson <tyler.j.erickson@...>
 

Hi Hao,

Sorry for the late reply.
I tested and can confirm that if the NSID of the command and the provided NamespaceID as an input match, the commands do go through on the EDK2 driver. This is not the case with a driver built into a motherboard that I have here, as it only accepts values for the current NSID and nothing else. This is ok since I can load the EDK2 driver in place instead.

Since this is unclear in the documentation for the passthru function, can we make a documentation change instead?

Thanks,
-Tyler


On Wed, Sep 4, 2019 at 8:07 PM Wu, Hao A <hao.a.wu@...> wrote:

Hello Tyler,

 

Some inline comments below.

 

Best Regards,

Hao Wu

 

From: Tyler J Erickson [mailto:tyler.j.erickson@...]
Sent: Wednesday, September 04, 2019 11:07 PM
To: Wu, Hao A
Cc: devel@edk2.groups.io; Ni, Ray
Subject: Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands

 

Hi Hao,

 

I tried making both the NamespaceID and NSID values the same when calling the passthru function for these admin commands and it didn't work. I think that is due to another place in the passthru code filtering the NamespaceId input values on line 517-520.

The NamespaceId parameter is being checked to make sure it isn't greater than the number of supported namespaces by the controller and it makes sure it isn't set to (UINT32)-1 (All F's). 

 

 

[Hao]: I think the check on line 517-520 blocks ‘NamespaceId’ with value greater than the number of namespace. But it allows the value to be MAX_UINT32, which means the command takes effect on all namespaces.

 

 

I think this is correct since the NamespaceId input is what is discovered when calling the EFI_NVM_EXPRESS_PASS_THRU_GET_NEXT_NAMESPACE function. This is what I used to get the available namespaces available in the system.

 

In my case I'm sending some commands in my application with the NSID set to UINT32_MAX in order to request controller wide SMART/Health log data among other things. There are some commands where setting the NSID to another value may also be useful. For example, DST can use the NSID in the command to change between testing only the controller (0), testing all namespaces (UINT32_MAX), and a specific namespace.

 

 

[Hao]: Yes. Just as you said, commands like DST or Get Log Page will have different target when different Namespace values are provided.

I think for your case, you can:

1.       Set both the 'NamespaceId' parameter and 'Nsid' field to 0 if the target is the controller;

2.       Set both of them MAX_UINT32 if the target is all the namespaces;

3.       Set both of them to a corresponding ID if the target is a specific namespace.

 

As you can see from the current implementation of NvmExpressPassThru() function, the 'NamespaceId' parameter does not affect the construct of the submission queue item.

The 'NamespaceId' parameter only serves as a valid check for the 'Nsid' field in the EFI_NVM_EXPRESS_COMMAND structure.

 

 

The modification I made was done where the passthru command's NSID was actually being checked, which seemed like the best place to add this exception for admin commands.


-Tyler

 

 

On Tue, Sep 3, 2019 at 9:39 PM Wu, Hao A <hao.a.wu@...> wrote:

> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, September 04, 2019 11:39 AM
> To: devel@edk2.groups.io; Tyler Erickson
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Repost the mail to the list.
>
> Best Regards,
> Hao Wu
>
> -----Original Message-----
> From: Tyler Erickson [mailto:tyler.j.erickson@...]
> Sent: Tuesday, September 03, 2019 9:55 PM
> To: edk2-devel@...
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Cc: Hao A Wu <hao.a.wu@...>
> Cc: Ray Ni <ray.ni@...>
> Signed-off-by: Tyler Erickson <tyler.j.erickson@...>
> ---
>  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index 8e721379466a..78a3c663ded4 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -561,7 +561,7 @@ NvmExpressPassThru (
>    Sq  = Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt;
>    Cq  = Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh;
>
> -  if (Packet->NvmeCmd->Nsid != NamespaceId) {
> +  if (Packet->QueueType != NVME_ADMIN_QUEUE && Packet->NvmeCmd-
> >Nsid != NamespaceId) {


Hello,

Per my understanding to the codes, I think the:

* Input parameter 'NamespaceId' for the PassThru() service
* The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND

are of the same meaning.

Do you think setting these 2 values identical when calling the PassThru()
service can resolve the issue you met?

Best Regards,
Hao Wu


>      return EFI_INVALID_PARAMETER;
>    }
>
> --
> 2.17.1


Re: [External] Re: [PATCH v1 1/1] Drivers/DisplayLink/DisplayLinkPkg DisplayLinkGop

Andy Hayes
 

That’s right, the only (current) request was index 0 – that is why it didn’t show up. It was a refactoring error.

 

It was picked up when we ported some of the changes back into our “closed source” version of the driver and the unit tests failed.

 

Thanks for pushing this.

 

From: Leif Lindholm <leif.lindholm@...>
Sent: 17 September 2019 16:28
To: Andy Hayes <andy.hayes@...>
Cc: devel@edk2.groups.io; Ard Biesheuvel <ard.biesheuvel@...>
Subject: [External] Re: [PATCH v1 1/1] Drivers/DisplayLink/DisplayLinkPkg DisplayLinkGop

 

On Wed, Sep 11, 2019 at 07:42:03AM +0000, Andy Hayes wrote:
> Corrected initialisation of one of data structures used to transmit USB
> control messages. Mistake had no practical effects but fixing to be on safe
> side.

So, was the only request used index 0? Or why didn't this cause an
issue? Nevertheless, a clear fix.

> Cc: Leif Lindholm <leif.lindholm@...>
> Cc: Ard Biesheuvel <ard.biesheuvel@...>
> Signed-off-by: Andy Hayes <andy.hayes@...>

Reviewed-by: Leif Lindholm <leif.lindholm@...>
Pushed as 958aaf600728.

/
Leif

> ---
> Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c b/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
> index 252293da39d4..9871ab0378ce 100644
> --- a/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
> +++ b/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
> @@ -107,7 +107,7 @@ DlUsbSendControlWriteMessage (
> UINT32 UsbStatus;
> EFI_USB_DEVICE_REQUEST UsbRequest;
>
> - ZeroMem (&Request, sizeof (Request));
> + ZeroMem (&UsbRequest, sizeof (UsbRequest));
> UsbRequest.RequestType = USB_REQ_TYPE_VENDOR | USB_TARGET_INTERFACE;
> UsbRequest.Index = Device->InterfaceDescriptor.InterfaceNumber;
> UsbRequest.Request = Request;
> --
> 1.8.3.1
>


Re: [PATCH v1 1/1] Drivers/DisplayLink/DisplayLinkPkg DisplayLinkGop

Leif Lindholm
 

On Wed, Sep 11, 2019 at 07:42:03AM +0000, Andy Hayes wrote:
Corrected initialisation of one of data structures used to transmit USB
control messages. Mistake had no practical effects but fixing to be on safe
side.
So, was the only request used index 0? Or why didn't this cause an
issue? Nevertheless, a clear fix.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Andy Hayes <andy.hayes@displaylink.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as 958aaf600728.

/
Leif

---
Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c b/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
index 252293da39d4..9871ab0378ce 100644
--- a/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
+++ b/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c
@@ -107,7 +107,7 @@ DlUsbSendControlWriteMessage (
UINT32 UsbStatus;
EFI_USB_DEVICE_REQUEST UsbRequest;

- ZeroMem (&Request, sizeof (Request));
+ ZeroMem (&UsbRequest, sizeof (UsbRequest));
UsbRequest.RequestType = USB_REQ_TYPE_VENDOR | USB_TARGET_INTERFACE;
UsbRequest.Index = Device->InterfaceDescriptor.InterfaceNumber;
UsbRequest.Request = Request;
--
1.8.3.1


Upcoming Event: TianoCore Design / Bug Triage - EMEA - Wed, 09/18/2019 8:00am-9:00am #cal-reminder

devel@edk2.groups.io Calendar <devel@...>
 

Reminder: TianoCore Design / Bug Triage - EMEA

When: Wednesday, 18 September 2019, 8:00am to 9:00am, (GMT-07:00) America/Los Angeles

Where:https://zoom.us/j/695893389

View Event

Organizer: Stephano Cetola stephano.cetola@...

Description:

Join Zoom Meeting

https://zoom.us/j/695893389

 

One tap mobile

+17207072699,,695893389# US

+16465588656,,695893389# US (New York)

 

Dial by your location

        +1 720 707 2699 US

        +1 646 558 8656 US (New York)

Meeting ID: 695 893 389

Find your local number: https://zoom.us/u/abOtdJckxL

 


Re: [PATCH v2 0/2] *** Add VS2019 Support ***

Liming Gao
 

Ching:
The change is good. With this change, have you verified VS2017 tool chain?
I want to make sure there is no impact on VS2017.

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Cheng, Ching JenX
Sent: Tuesday, September 17, 2019 11:23 AM
To: devel@edk2.groups.io; Cheng, Ching JenX <ching.jenx.cheng@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 0/2] *** Add VS2019 Support ***

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2182

In order to support VS2019 on EDK2, the following patches was modified def and batch files 1. Add VS2019 x86/64 definitions on
tools_def.template 2. Add VS2019 support on toolsetup batches, and add version check with command vswhere
Because VS2019 and VS2017 using the same vswhere to get the InstallationPath

v2: In 01/02, add ARM/AARCH64/EBC Definitions, Combine VS2017_HOST and VS2019_HOST to VS_HOST

Ching JenX Cheng (2):
Add VS2019 Toolchain def
Add VS2019 Support on ToolSetup Batches

BaseTools/Conf/tools_def.template | 220
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------
BaseTools/get_vsvars.bat | 37 ++++++++++++++++++++++++++++++-------
BaseTools/set_vsprefix_envs.bat | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
BaseTools/toolsetup.bat | 16 +++++++++++++---
edksetup.bat | 6 ++++--
5 files changed, 313 insertions(+), 36 deletions(-)

--
2.21.0.windows.1






Re: [PATCH] MdePkg:Include: Update SmBios header file

Liming Gao
 

Abner:
I add my comments.

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif Lindholm
Sent: Tuesday, September 17, 2019 9:34 PM
To: Abner Chang <abner.chang@hpe.com>
Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Gilbert Chen
<gilbert.chen@hpe.com>
Subject: Re: [edk2-devel] [PATCH] MdePkg:Include: Update SmBios header file

On Tue, Sep 17, 2019 at 02:24:30PM +0800, Abner Chang wrote:
Update SmBios header file to conform with SMBIOS v3.3.0.
Ah, I note SMBIOS 3.3 has not yet been released - so this can not be
merged in edk2 master at this point. I did not realise this when I
requested you send the patch.
Please submit BZ https://bugzilla.tianocore.org/ for this update. This is a new feature.

However, you can carry this in your edk2-staging branch, and once the
specification gets released we can take it into edk2.

(After code review, a couple of minor comments below.)

The major update is to add definitions of SMBIOS Type 44h record.

Signed-off-by: Abner Chang <abner.chang@hpe.com>

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Gilbert Chen <gilbert.chen@hpe.com>

---
MdePkg/Include/IndustryStandard/SmBios.h | 74 +++++++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/IndustryStandard/SmBios.h b/MdePkg/Include/IndustryStandard/SmBios.h
index f3b6f18..ebf0ceb 100644
--- a/MdePkg/Include/IndustryStandard/SmBios.h
+++ b/MdePkg/Include/IndustryStandard/SmBios.h
@@ -3,6 +3,7 @@

Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
(C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP<BR>
+(C) Copyright 2015 - 2019 Hewlett Packard Enterprise Development LP<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
File header should be updated.

Industry Standard Definitions of SMBIOS Table Specification v3.2.0.
==>
Industry Standard Definitions of SMBIOS Table Specification v3.3.0.

And, please refer to https://bugzilla.tianocore.org/show_bug.cgi?id=1099 for 3.2 update,
prepare all changes for SmBios 3.3 update.

Thanks
Liming

**/
@@ -46,7 +47,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define SMBIOS_3_0_TABLE_MAX_LENGTH 0xFFFFFFFF

//
-// SMBIOS type macros which is according to SMBIOS 2.7 specification.
+// SMBIOS type macros which is according to SMBIOS 3.3.0 specification.
//
#define SMBIOS_TYPE_BIOS_INFORMATION 0
#define SMBIOS_TYPE_SYSTEM_INFORMATION 1
@@ -92,6 +93,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define SMBIOS_TYPE_ONBOARD_DEVICES_EXTENDED_INFORMATION 41
#define SMBIOS_TYPE_MANAGEMENT_CONTROLLER_HOST_INTERFACE 42
#define SMBIOS_TYPE_TPM_DEVICE 43
+#define SMBIOS_TYPE_PROCESSOR_ADDITIONAL_INFORMATION 44

///
/// Inactive type is added from SMBIOS 2.2. Reference SMBIOS 2.6, chapter 3.3.43.
@@ -727,7 +729,10 @@ typedef enum {
ProcessorFamilyMII = 0x012E,
ProcessorFamilyWinChip = 0x0140,
ProcessorFamilyDSP = 0x015E,
- ProcessorFamilyVideoProcessor = 0x01F4
+ ProcessorFamilyVideoProcessor = 0x01F4,
+ ProcessorFamilyRiscvRV32 = 0x0200, ///< SMBIOS spec 3.3.0 added
Please drop the "///< SMBIOS spec 3.3.0 added" comment. Here and below.

+ ProcessorFamilyRiscVRV64 = 0x0201, ///< SMBIOS spec 3.3.0 added
+ ProcessorFamilyRiscVRV128 = 0x0202 ///< SMBIOS spec 3.3.0 added
No further comments from me (MdePkg maintainers may have some).

/
Leif

} PROCESSOR_FAMILY2_DATA;

///
@@ -857,6 +862,19 @@ typedef struct {
} PROCESSOR_FEATURE_FLAGS;

typedef struct {
+ UINT32 ProcessorReserved1 :1;
+ UINT32 ProcessorUnknown :1;
+ UINT32 Processor64BitCapble :1;
+ UINT32 ProcessorMultiCore :1;
+ UINT32 ProcessorHardwareThread :1;
+ UINT32 ProcessorExecuteProtection :1;
+ UINT32 ProcessorEnhancedVirtulization :1;
+ UINT32 ProcessorPowerPerformanceCtrl :1;
+ UINT32 Processor128bitCapble :1;
+ UINT32 ProcessorReserved2 :7;
+} PROCESSOR_CHARACTERISTIC_FLAGS;
+
+typedef struct {
PROCESSOR_SIGNATURE Signature;
PROCESSOR_FEATURE_FLAGS FeatureFlags;
} PROCESSOR_ID_DATA;
@@ -2508,6 +2526,57 @@ typedef struct {
UINT8 InterfaceTypeSpecificData[4]; ///< This field has a minimum of four bytes
} SMBIOS_TABLE_TYPE42;

+
+///
+/// Processor Specific Block - Processor Architecture Type
+///
+typedef enum{
+ ProcessorSpecificBlockArchTypeReserved = 0x00,
+ ProcessorSpecificBlockArchTypeIa32 = 0x01,
+ ProcessorSpecificBlockArchTypeX64 = 0x02,
+ ProcessorSpecificBlockArchTypeItanium = 0x03,
+ ProcessorSpecificBlockArchTypeAarch32 = 0x04,
+ ProcessorSpecificBlockArchTypeAarch64 = 0x05,
+ ProcessorSpecificBlockArchTypeRiscVRV32 = 0x06,
+ ProcessorSpecificBlockArchTypeRiscVRV64 = 0x07,
+ ProcessorSpecificBlockArchTypeRiscVRV128 = 0x08
+} PROCESSOR_SPECIFIC_BLOCK_ARCH_TYPE;
+
+///
+/// Processor Specific Block is the standard container of processor-specific data.
+///
+typedef struct {
+ UINT8 Length;
+ UINT8 ProcessorArchType;
+ ///
+ /// Below followed by Processor-specific data
+ ///
+ ///
+} PROCESSOR_SPECIFIC_BLOCK;
+
+///
+/// Processor Additional Information(Type 44).
+///
+/// The information in this structure defines the processor additional information in case
+/// SMBIOS type 4 is not sufficient to describe processor characteristics.
+/// The SMBIOS type 44 structure has a reference handle field to link back to the related
+/// SMBIOS type 4 structure. There may be multiple SMBIOS type 44 structures linked to the
+/// same SMBIOS type 4 structure. For example, when cores are not identical in a processor,
+/// SMBIOS type 44 structures describe different core-specific information.
+///
+/// SMBIOS type 44 defines the standard header for the processor-specific block, while the
+/// contents of processor-specific data are maintained by processor
+/// architecture workgroups or vendors in separate documents.
+///
+typedef struct {
+ SMBIOS_STRUCTURE Hdr;
+ SMBIOS_HANDLE RefHandle; ///< This field refer to associated SMBIOS type 4
+ ///
+ /// Below followed by Processor-specific block
+ ///
+ PROCESSOR_SPECIFIC_BLOCK ProcessorSpecificBlock;
+} SMBIOS_TABLE_TYPE44;
+
///
/// TPM Device (Type 43).
///
@@ -2586,6 +2655,7 @@ typedef union {
SMBIOS_TABLE_TYPE41 *Type41;
SMBIOS_TABLE_TYPE42 *Type42;
SMBIOS_TABLE_TYPE43 *Type43;
+ SMBIOS_TABLE_TYPE44 *Type44;
SMBIOS_TABLE_TYPE126 *Type126;
SMBIOS_TABLE_TYPE127 *Type127;
UINT8 *Raw;
--
2.7.4


[staging/branch]: CdePkg - C Development Environment Package

Minnow Ware
 

Hi UEFI community,

 

I’d like to introduce the CdePkg to edk2-staging.

 

The package is not yet completed but ready to demonstrate it’s power, probably also for modernFW.

 

A couple of years ago, after an UEFI BIOS project on AMD platform I decided to write my own ANSI C Library for UEFI Shell and POST.

 

My design goals were:

  1. to rewrite the whole thing from scratch, without using any public source code from GNU, BSD, Watcom or Intel EDK2 / tiano core
  2. completeness: full blown C90 + C95 support, no C99, no non-specified extensions at all , e.g. itoa(), stricmp()...
  3. small code size, for UEFI-POST-driver uses a C-Library-Driver, that contains core/worker functions for realloc() ==  malloc() and free(),

entire printf-family, entire scanf-family.

UEFI-POST-driver just uses small wrapper functions to run the C-Library-Driver code.

  1. stable, exact, chipset independent (w/o ACPI timer) "clock()” with CLOCKS_PER_SEC == 1000
  2. complete set of the Microsoft C-compiler intrinsic functions
  3. ROM-able! Runs with stack but w/o any static storage duration in .data segment, e.g. for rand(), strtok(), tmpfile()

This is required for early PEI before memory sizing, when PEI-images run directly out of flash.

  1. Microsoft bug compatible (as far as possible)
    1. to save my lifetime writing a documentation  https://github.com/JoaquinConoBolillo/torito-C-Library/blob/master/implemented.md
    2. use original Microsoft header files for UEFI Shell Apps created in VS2017/19
    3. “debug”-mode for UEFI Shell executable in VS2017/19, that truly runs on Windows (that works

when using library functions only, no HW access, not UEFI-API use) to debug the library

itself – but this just links the same .OBJ module with the WinNT-EntryPoint instead of UEFI-EntryPoint

(The entry point module pulls in the appropriate OS-interface branch dispatcher)

  1. all that in one single C-Library CdeLib.lib

 

The Readme.MD is here: https://github.com/MinnowWare/CdePkg#cdepkg

 

CdePkg shall be adjusted to other compilers/tool chains too, once it is feature complete and accepted by the UEFI community,

as long as it is for Microsoft VS2017/19 only.

 

The CdePkg is integrated into the “vUDK2018”-EDK2, which in turn runs in a MinnowBoard build.

It can be emulated in the Nt32Pkg, since EmulatorPkg in “vUDK2018” doesn’t support Windows…

 

I would like to move the “vUDK2018”-EDK2 to the edk2-staging branch CdePkg, but need to have access granted.

 

Can anyone kindly grant access rights to me?

 

Best Regards,

Kilian

 

 


Re: [PATCH V2 0/3] MdeModulePkg/TerminalConsole: Extend the support terminal types

Liming Gao
 

Leif:

-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Tuesday, September 17, 2019 5:15 PM
To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
Cc: ard.biesheuvel@linaro.org; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>;
Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 0/3] MdeModulePkg/TerminalConsole: Extend the support terminal types

On Tue, Sep 17, 2019 at 07:17:27AM +0000, Zhang, Shenglei wrote:
That's my mistake to push the broken patch(0d85e67714e31e0dbe4241ab2ebb7c423aba174d).
This patch only updates the file guid, which I thought has no risk. So I didn’t check the build result.
I should double check the new guid used in the file.
Determining what affects build and not is something humans are very
bad at and computers are very good at.

So you should build check every patch you submit to the list, no
matter how trivial.

In normal circumstances, so should the maintainers before pushing the
patch.
I push this change. I should double confirm its test result. I will improve my rule.


We now have a commit in the tree known to break the build of pretty
much all ARM/AARCH64 platforms. This will be very unpleasant for
future bisect.
I agree the build break is the big impact. I expect we can speed up to enable EDK II Continuous Integration.
If so, we can avoid such break again.

Thanks
Liming

Best Regards,

Leif


Re: [edk2-staging/RISC-V-V2 PATCH v1 11/22]: BaseTools: BaseTools changes for RISC-V platform.

Leif Lindholm
 

On Tue, Sep 17, 2019 at 02:08:53PM +0100, Leif Lindholm wrote:
Please add the requisite support to the GCC5 profile instead. (Which
is not actually for gcc 5, but is effectively GCC5+ - we are still
successfully using it with gcc 9.)
I can try to use GCC5 profile but the toolchain still has to be
stick on https://github.com/riscv/riscv-gnu-toolchain @64879b24. We
got problem on the version higher than this, system hangs at SEC to
PEI transition if use GCC version higher than @64879b24.

We will figure it out later.
I suppose this is fine as long as this is specifically on the
edk2-staging branch. We will need to resolve it before we bring the
port to edk2 master.

I would still like this support to be tweaked to the point where I can
build with either the Fedora or the Debian packaged cross compiler.

So how can I mention this restrictions in tool_def?
I would suggest the following:
- The above information in the top-level Readme.md on the staging branch.
- A single line in the "GCC5 RISCV64 definitions" comment block.
- Adding the above to the commit message.
Oh, and please add information to that Readme.md on which toolchains
have successfully built this toolchain. Debian's gcc8 fails.

Best Regards,

Leif


Re: [edk2-staging/RISC-V-V2 PATCH v1 13/22]: MdePkg/Include: Update SmBios header file.

Leif Lindholm
 

On Mon, Sep 16, 2019 at 07:01:50AM +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Friday, September 6, 2019 12:17 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 13/22]:
MdePkg/Include: Update SmBios header file.

On Wed, Sep 04, 2019 at 06:43:08PM +0800, Abner Chang wrote:
Update SmBios header file to conform with SMBIOS v3.3.0.
The major update is to add definitions of SMBIOS Type 44h record.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Abner Chang <abner.chang@hpe.com>
This would be really useful to get straight into edk2 - could you submit it
straight for inclusion in edk2 master? We can then cherry-pick that back to
the edk2-staging branch.
Forgive me that I don't want to increase the complexity to RISC-V
edk2 submittal. We can send SMBIOS patch apart from RISC-V patches
with specific subject for SMBIOS change.
Yes, sorry, that was exactly what I meant :)
Thank you for doing that.

Best Regards,

Leif


Re: [edk2-staging/RISC-V-V2 PATCH v1 07/22]: MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.

Leif Lindholm
 

On Mon, Sep 16, 2019 at 05:37:51AM +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Thursday, September 5, 2019 10:28 PM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 07/22]:
MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.

On Wed, Sep 04, 2019 at 06:43:02PM +0800, Abner Chang wrote:
RISC-V MMIO library instance. RISC-V only supports memory map I/O.
However the first implementation of RISC-V EDK2 port uses PC/AT as the
RISC-V platform spec. We have to keep the I/O functions as the temporary
solution.

Can you expand on the I/O port situation?
Since the architecture doesn't support it, what do these functions do?

For the pure MMIO ops using compliant C, we really don't need yet another
implementation pretending it's architecture specific. We should just have a
single IoLibMmio.c and an IoLibMmioNonCompliant.c if the x86 folks want to
keep their current one.
Hmm. That was for the old RISC-V PC/AT QEUM version back to 2016. We
pulled in some X86 peripherals to build up RISC-V PC/AT like
platform . will remove this.
Excellent, thanks.

/
Leif


Re: [edk2-staging/RISC-V-V2 PATCH v1 04/22]: MdePkg/Include: RISC-V definitions.

Leif Lindholm
 

On Mon, Sep 16, 2019 at 05:31:40AM +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Thursday, September 5, 2019 4:40 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 04/22]:
MdePkg/Include: RISC-V definitions.

On Wed, Sep 04, 2019 at 06:42:59PM +0800, Abner Chang wrote:
Add RISC-V processor related definitions.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Abner Chang <abner.chang@hpe.com>
---
MdePkg/Include/IndustryStandard/PeImage.h | 14 +-
MdePkg/Include/Library/BaseLib.h | 67 ++++++
MdePkg/Include/Protocol/DebugSupport.h | 55 +++++
MdePkg/Include/Protocol/PxeBaseCode.h | 8 +
MdePkg/Include/RiscV64/ProcessorBind.h | 336
++++++++++++++++++++++++++++++
MdePkg/Include/Uefi/UefiBaseType.h | 25 +++
MdePkg/Include/Uefi/UefiSpec.h | 11 +
7 files changed, 513 insertions(+), 3 deletions(-) create mode
100644 MdePkg/Include/RiscV64/ProcessorBind.h

diff --git a/MdePkg/Include/IndustryStandard/PeImage.h
b/MdePkg/Include/IndustryStandard/PeImage.h
index 720bb08..47796b2 100644
--- a/MdePkg/Include/IndustryStandard/PeImage.h
+++ b/MdePkg/Include/IndustryStandard/PeImage.h
@@ -9,6 +9,8 @@

Copyright (c) 2006 - 2018, Intel Corporation. All rights
reserved.<BR> Portions copyright (c) 2008 - 2009, Apple Inc. All
rights reserved.<BR>
+Portions Copyright (c) 2016, Hewlett Packard Enterprise Development
+LP. All rights reserved.<BR>
+
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -34,6 +36,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define IMAGE_FILE_MACHINE_X64 0x8664
#define IMAGE_FILE_MACHINE_ARMTHUMB_MIXED 0x01c2
#define IMAGE_FILE_MACHINE_ARM64 0xAA64
+#define IMAGE_FILE_MACHINE_RISCV32 0x5032
+#define IMAGE_FILE_MACHINE_RISCV64 0x5064
+#define IMAGE_FILE_MACHINE_RISCV128 0x5128

//
// EXE file formats
@@ -478,9 +483,9 @@ typedef struct {
///
#define EFI_IMAGE_SIZEOF_BASE_RELOCATION 8

-//
-// Based relocation types.
-//
+///
+/// Based relocation types.
+///
I don't know if this change to the comment block is a wonky rebase or
whatever, but please drop it.
No, I revised it to three back slash because most of comment block
use three back slash.
It is always appreciated when people provide style fixes, but they
should be provided as separate patches.

In this case I don't see the value in doing that however, since // is
the comment format specified by the coding standars.

A separate patch fixing the existing incorrect ones would be
preferable (but not important).

Best Regards,

Leif


Re: [edk2-staging/RISC-V-V2 PATCH v1 01/22]: RiscVPkg: RISC-V processor package.

Leif Lindholm
 

On Mon, Sep 16, 2019 at 05:15:08AM +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Thursday, September 5, 2019 1:51 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 01/22]:
RiscVPkg: RISC-V processor package.

Hi Abner,

On Wed, Sep 04, 2019 at 06:42:56PM +0800, Abner Chang wrote:
- Add RiscVPkg package which provides RISC-V processor related drivers
and libraries.
- Support RISC-V OpenSBI and RISC-V platforms

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Abner Chang <abner.chang@hpe.com>
---
RiscVPkg/RiscVPkg.dec | 57
+++++++++++++++++++++++++++++++++++++++++++++
RiscVPkg/RiscVPkg.uni | Bin 0 -> 1718 bytes
RiscVPkg/RiscVPkgExtra.uni | Bin 0 -> 1374 bytes
3 files changed, 57 insertions(+)
create mode 100644 RiscVPkg/RiscVPkg.dec create mode 100644
RiscVPkg/RiscVPkg.uni create mode 100644 RiscVPkg/RiscVPkgExtra.uni

diff --git a/RiscVPkg/RiscVPkg.dec b/RiscVPkg/RiscVPkg.dec new file
mode 100644 index 0000000..acf71fe
--- /dev/null
+++ b/RiscVPkg/RiscVPkg.dec
@@ -0,0 +1,57 @@
+## @file RiscVPkg.dec
+# This Package provides UEFI RISC-V modules and libraries.
+#
+# Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development
+LP. 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 #
+INVALID URI REMOVED
3A__opensource.org_li
+censes_bsd-
2Dlicense.php&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN
+4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=1PSVwg69_Y8lpR9wdv1TN7
kg2brsZYR
+sj5F_hpyPrv4&s=USJlvms7O9ZDAsM0U-
FGng8i0uJkAMNbDEp1S_C4p0A&e=
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
+BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
EITHER EXPRESS OR IMPLIED.
+#
+##
+
+[Defines]
+ DEC_SPECIFICATION = 0x00010005
+ PACKAGE_NAME = RiscVPkg
+ PACKAGE_UNI_FILE = RiscVPkg.uni
+ PACKAGE_GUID = 993C7CAC-C87C-4F08-A2CF-AD3AABA859D1
+ PACKAGE_VERSION = 0.1
+
+[Includes]
+ Include
+ opensbi/include
+ opensbi/lib/utils/libfdt
This one is something we need to sort out (together). Having multiple copies
of libfdt in the tree is not on.

I personally think we need a longer-term encapsulation of libfdt that doesn't
mess up the coding style. But until then, I would be much happier if you used
the half measure we have in EmbeddedPkg:
EmbeddedPkg/Library/FdtLib/ and EmbeddedPkg/Include/.
We may not go this way due to everything is from OpenSBI and we
don't want to maintain the difference to open source OpenSBI. Just
take what OpenSBI provides.
If libfdt was a very quickly changing project, I might agree with you.
But it is not. It is a very simple piece of code that performs a small
set of operations on a very well defined structured encapsulation
format.

So please use the one from EmbeddedPkg. On the very unlikely
occurrence that you require functionality not provided by the version
in there, we can update it.

/
Leif


Re: [edk2-staging/RISC-V-V2 PATCH v1 02/22]: RiscVPkg/Include: Add header files of RISC-V CPU package

Leif Lindholm
 

On Mon, Sep 16, 2019 at 04:02:10AM +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Leif Lindholm
Sent: Thursday, September 5, 2019 2:56 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 02/22]:
RiscVPkg/Include: Add header files of RISC-V CPU package

On Wed, Sep 04, 2019 at 06:42:57PM +0800, Abner Chang wrote:
RISC-V package library definitions.

RiscV.h
-Add RiscV.h which conform with RISC-V Privilege Spec v1.10.

sbi.h
sbi_bits.h
sbi_types.h
- Add definitions for RISC-V OpenSBI EDK2 port.
A web search suggests this refers to the RISC-V Open Source Supervisor
Binary Interface. It would be helpful to expand it on first use.
https://github.com/riscv/opensbi/?
Is this expected to fluctuate much?
Yes it does change often, the community keeps adding new features to openSBI.
OK. I got some more intro to this at Linux Plumbers Conference last week.

I ask for two reasons:
1) Because if it is not, I would much prefer to see the
files/directories renamed to conform the the coding style.
If it is, I would like for us to consider implementing this as a
git submodule instead.
Yes. Please use submodule. Don't touch the open source from openSBI to avoid maintenance effort to edk2.
Sounds good.

...

diff --git a/RiscVPkg/Include/sbi/sbi_bits.h
b/RiscVPkg/Include/sbi/sbi_bits.h
new file mode 100644
index 0000000..4116ee6
--- /dev/null
+++ b/RiscVPkg/Include/sbi/sbi_bits.h
@@ -0,0 +1,23 @@
+/** @file
+ RISC-V OpesbSBI header file reference.
+
+ Copyright (c) 2019, Hewlett Packard Enterprise Development LP. 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
+ INVALID URI REMOVED
3A__opensource.org_licenses_bsd-
2Dlicense.php&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4V
gi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=iwfkW8MQjzEkixp0gv3xsvh20ei
odo7hGcTLXEL_I0o&s=mLKjYgrdQ6MuAN9UVYQeCDB0pNA44m9yBOylxW-
Koiw&e=
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
+
+**/
+#ifndef __EDK2_SBI_BITS_H__
+#define __EDK2_SBI_BITS_H__
+
+#undef MAX
+#undef MIN
Why?
OpebSBI sbi_bits.h has its own MAX/MIN definitions which are
duplicated with edk2 ones. OpenSBI is the implementation of RISC-V
sbi spec which is similar to edk2 for UEFI, the duplicate macros are
expected. This is the wrapper file to OpenSBI because of we don't
want to touch OpenSBI code.
I think we should look at refactoring this in OpenSBI instead.
Especially with us using this as effectively a library, we would need
to be actively monitoring (well, on every update, but you suggested
they may be frequent) whether any new clashes developed.

The guys who attended Plumbers suggested thy would be quite flexible
to restructure code in ways that makes the project more consumable.

I am OK with this being here while it is on the edk2-staging branch.


+
+#include "../opensbi/include/sbi/sbi_bits.h"
No relative includes. Let's figure out a way to expose the interface properly.
Can be fixed by RiscVPkg.dec
Sounds good.

+
+#endif
\ No newline at end of file
diff --git a/RiscVPkg/Include/sbi/sbi_types.h
b/RiscVPkg/Include/sbi/sbi_types.h
new file mode 100644
index 0000000..fe877f2
--- /dev/null
+++ b/RiscVPkg/Include/sbi/sbi_types.h
@@ -0,0 +1,24 @@
+/** @file
+ RISC-V OpesbSBI header file reference.
+
+ Copyright (c) 2019, Hewlett Packard Enterprise Development LP. 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
+ INVALID URI REMOVED
3A__opensource.org_licenses_bsd-
2Dlicense.php&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4V
gi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=iwfkW8MQjzEkixp0gv3xsvh20ei
odo7hGcTLXEL_I0o&s=mLKjYgrdQ6MuAN9UVYQeCDB0pNA44m9yBOylxW-
Koiw&e=
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
+
+**/
+#ifndef __EDK2_SBI_TYPES_H__
+#define __EDK2_SBI_TYPES_H__
+
+#undef TRUE
+#undef FALSE
+#undef NULL
Why?
Same reason as above.
OK, same response as above.

+
+#include "../opensbi/include/sbi/sbi_types.h"
No relative includes.
Can be fixed by RiscVPkg.dec
OK.

/
Leif


Re: [PATCH] MdePkg:Include: Update SmBios header file

Leif Lindholm
 

On Tue, Sep 17, 2019 at 02:24:30PM +0800, Abner Chang wrote:
Update SmBios header file to conform with SMBIOS v3.3.0.
Ah, I note SMBIOS 3.3 has not yet been released - so this can not be
merged in edk2 master at this point. I did not realise this when I
requested you send the patch.

However, you can carry this in your edk2-staging branch, and once the
specification gets released we can take it into edk2.

(After code review, a couple of minor comments below.)

The major update is to add definitions of SMBIOS Type 44h record.

Signed-off-by: Abner Chang <abner.chang@hpe.com>

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Gilbert Chen <gilbert.chen@hpe.com>

---
MdePkg/Include/IndustryStandard/SmBios.h | 74 +++++++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/IndustryStandard/SmBios.h b/MdePkg/Include/IndustryStandard/SmBios.h
index f3b6f18..ebf0ceb 100644
--- a/MdePkg/Include/IndustryStandard/SmBios.h
+++ b/MdePkg/Include/IndustryStandard/SmBios.h
@@ -3,6 +3,7 @@

Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
(C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP<BR>
+(C) Copyright 2015 - 2019 Hewlett Packard Enterprise Development LP<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -46,7 +47,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define SMBIOS_3_0_TABLE_MAX_LENGTH 0xFFFFFFFF

//
-// SMBIOS type macros which is according to SMBIOS 2.7 specification.
+// SMBIOS type macros which is according to SMBIOS 3.3.0 specification.
//
#define SMBIOS_TYPE_BIOS_INFORMATION 0
#define SMBIOS_TYPE_SYSTEM_INFORMATION 1
@@ -92,6 +93,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define SMBIOS_TYPE_ONBOARD_DEVICES_EXTENDED_INFORMATION 41
#define SMBIOS_TYPE_MANAGEMENT_CONTROLLER_HOST_INTERFACE 42
#define SMBIOS_TYPE_TPM_DEVICE 43
+#define SMBIOS_TYPE_PROCESSOR_ADDITIONAL_INFORMATION 44

///
/// Inactive type is added from SMBIOS 2.2. Reference SMBIOS 2.6, chapter 3.3.43.
@@ -727,7 +729,10 @@ typedef enum {
ProcessorFamilyMII = 0x012E,
ProcessorFamilyWinChip = 0x0140,
ProcessorFamilyDSP = 0x015E,
- ProcessorFamilyVideoProcessor = 0x01F4
+ ProcessorFamilyVideoProcessor = 0x01F4,
+ ProcessorFamilyRiscvRV32 = 0x0200, ///< SMBIOS spec 3.3.0 added
Please drop the "///< SMBIOS spec 3.3.0 added" comment. Here and below.

+ ProcessorFamilyRiscVRV64 = 0x0201, ///< SMBIOS spec 3.3.0 added
+ ProcessorFamilyRiscVRV128 = 0x0202 ///< SMBIOS spec 3.3.0 added
No further comments from me (MdePkg maintainers may have some).

/
Leif

} PROCESSOR_FAMILY2_DATA;

///
@@ -857,6 +862,19 @@ typedef struct {
} PROCESSOR_FEATURE_FLAGS;

typedef struct {
+ UINT32 ProcessorReserved1 :1;
+ UINT32 ProcessorUnknown :1;
+ UINT32 Processor64BitCapble :1;
+ UINT32 ProcessorMultiCore :1;
+ UINT32 ProcessorHardwareThread :1;
+ UINT32 ProcessorExecuteProtection :1;
+ UINT32 ProcessorEnhancedVirtulization :1;
+ UINT32 ProcessorPowerPerformanceCtrl :1;
+ UINT32 Processor128bitCapble :1;
+ UINT32 ProcessorReserved2 :7;
+} PROCESSOR_CHARACTERISTIC_FLAGS;
+
+typedef struct {
PROCESSOR_SIGNATURE Signature;
PROCESSOR_FEATURE_FLAGS FeatureFlags;
} PROCESSOR_ID_DATA;
@@ -2508,6 +2526,57 @@ typedef struct {
UINT8 InterfaceTypeSpecificData[4]; ///< This field has a minimum of four bytes
} SMBIOS_TABLE_TYPE42;

+
+///
+/// Processor Specific Block - Processor Architecture Type
+///
+typedef enum{
+ ProcessorSpecificBlockArchTypeReserved = 0x00,
+ ProcessorSpecificBlockArchTypeIa32 = 0x01,
+ ProcessorSpecificBlockArchTypeX64 = 0x02,
+ ProcessorSpecificBlockArchTypeItanium = 0x03,
+ ProcessorSpecificBlockArchTypeAarch32 = 0x04,
+ ProcessorSpecificBlockArchTypeAarch64 = 0x05,
+ ProcessorSpecificBlockArchTypeRiscVRV32 = 0x06,
+ ProcessorSpecificBlockArchTypeRiscVRV64 = 0x07,
+ ProcessorSpecificBlockArchTypeRiscVRV128 = 0x08
+} PROCESSOR_SPECIFIC_BLOCK_ARCH_TYPE;
+
+///
+/// Processor Specific Block is the standard container of processor-specific data.
+///
+typedef struct {
+ UINT8 Length;
+ UINT8 ProcessorArchType;
+ ///
+ /// Below followed by Processor-specific data
+ ///
+ ///
+} PROCESSOR_SPECIFIC_BLOCK;
+
+///
+/// Processor Additional Information(Type 44).
+///
+/// The information in this structure defines the processor additional information in case
+/// SMBIOS type 4 is not sufficient to describe processor characteristics.
+/// The SMBIOS type 44 structure has a reference handle field to link back to the related
+/// SMBIOS type 4 structure. There may be multiple SMBIOS type 44 structures linked to the
+/// same SMBIOS type 4 structure. For example, when cores are not identical in a processor,
+/// SMBIOS type 44 structures describe different core-specific information.
+///
+/// SMBIOS type 44 defines the standard header for the processor-specific block, while the
+/// contents of processor-specific data are maintained by processor
+/// architecture workgroups or vendors in separate documents.
+///
+typedef struct {
+ SMBIOS_STRUCTURE Hdr;
+ SMBIOS_HANDLE RefHandle; ///< This field refer to associated SMBIOS type 4
+ ///
+ /// Below followed by Processor-specific block
+ ///
+ PROCESSOR_SPECIFIC_BLOCK ProcessorSpecificBlock;
+} SMBIOS_TABLE_TYPE44;
+
///
/// TPM Device (Type 43).
///
@@ -2586,6 +2655,7 @@ typedef union {
SMBIOS_TABLE_TYPE41 *Type41;
SMBIOS_TABLE_TYPE42 *Type42;
SMBIOS_TABLE_TYPE43 *Type43;
+ SMBIOS_TABLE_TYPE44 *Type44;
SMBIOS_TABLE_TYPE126 *Type126;
SMBIOS_TABLE_TYPE127 *Type127;
UINT8 *Raw;
--
2.7.4


Re: [PATCH] q35: lpc: allow to lock down 128K RAM at default SMBASE address

Igor Mammedov
 

On Wed, 11 Sep 2019 19:30:46 +0200
"Laszlo Ersek" <lersek@redhat.com> wrote:

On 09/10/19 17:58, Igor Mammedov wrote:
On Mon, 9 Sep 2019 21:15:44 +0200
Laszlo Ersek <lersek@redhat.com> wrote:
[...]

It looks like fwcfg smi feature negotiation isn't reusable in this case.
I'd prefer not to add another device just for another SMI feature
negotiation/activation.
I thought it could be a register on the new CPU hotplug controller that
we're going to need anyway (if I understand correctly, at least).
If we don't have to 'park' hotplugged CPUs, then I don't see a need for
an extra controller.


But:

How about stealing reserved register from pci-host similar to your
extended TSEG commit (2f295167 q35/mch: implement extended TSEG sizes)?
(Looking into spec can (ab)use 0x58 or 0x59 register)
Yes, that should work.

In fact, I had considered 0x58 / 0x59 when looking for unused registers
for extended TSEG configuration:

http://mid.mail-archive.com/d8802612-0b11-776f-b209-53bbdaf67515@redhat.com

So I'm OK with this, thank you.
Thanks for the tip!
... patches with a stolen register are on the way to mail-list.


Re: [edk2-staging/RISC-V-V2 PATCH v1 11/22]: BaseTools: BaseTools changes for RISC-V platform.

Leif Lindholm
 

On Mon, Sep 16, 2019 at 07:46:26AM +0000, Abner Chang wrote:
-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
Sent: Monday, September 9, 2019 7:37 PM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v1 11/22]:
BaseTools: BaseTools changes for RISC-V platform.

Hi Abner,

Having actually tried to build things, I have come across a bunch of
issues with this patch I missed on my (very cursory) ocular review.

On Wed, Sep 04, 2019 at 06:43:06PM +0800, Abner Chang wrote:
BaseTools changes for building EDK2 RISC-V platform.
The changes made to build_rule.template is to avoid build errors cause by
GCC711RISCV tool chain.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Abner Chang <abner.chang@hpe.com>
---
BaseTools/Conf/build_rule.template | 23 +-
BaseTools/Conf/tools_def.template | 108 +-
BaseTools/Source/C/Common/BasePeCoff.c | 19 +-
BaseTools/Source/C/Common/PeCoffLoaderEx.c | 96 ++
BaseTools/Source/C/GenFv/GenFvInternalLib.c | 281 ++++-
BaseTools/Source/C/GenFw/Elf32Convert.c | 6 +-
BaseTools/Source/C/GenFw/Elf64Convert.c | 273 ++++-
BaseTools/Source/C/GenFw/elf_common.h | 63 ++
.../Source/C/Include/IndustryStandard/PeImage.h | 10 +
BaseTools/Source/Python/Common/DataType.py | 1075
++++++++++----------
10 files changed, 1393 insertions(+), 561 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template
b/BaseTools/Conf/tools_def.template
index 8f0e6cb..36a301a 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3,7 +3,7 @@
# Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
# Portions copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
# Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR>
-# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
+# (C) Copyright 2016-2019 Hewlett Packard Enterprise Development
LP<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -114,6 +114,12 @@ DEFINE GCC49_X64_PREFIX = ENV(GCC49_BIN)
DEFINE GCC5_IA32_PREFIX = ENV(GCC5_BIN)
DEFINE GCC5_X64_PREFIX = ENV(GCC5_BIN)
DEFINE GCC_HOST_PREFIX = ENV(GCC_HOST_BIN)
+#
+# RISC-V GCC toolchain
+# This is the default directory used when install official riscv-tools.
+#
+DEFINE GCCRISCV_RISCV32_PREFIX = ENV(GCC_RISCV32_BIN)
+DEFINE GCCRISCV_RISCV64_PREFIX = ENV(GCC_RISCV64_BIN)
If at all possible, I would strongly recommend *not* following the x86
_BIN example, and instead using ENV(<toolchain>_RISCV64_PREFIX) like
the ARM/AARCH64 profiles.

DEFINE UNIX_IASL_BIN = ENV(IASL_PREFIX)iasl
DEFINE WIN_IASL_BIN = ENV(IASL_PREFIX)iasl.exe
@@ -236,6 +242,15 @@ DEFINE DTC_BIN = ENV(DTC_PREFIX)dtc
# Required to build platforms or ACPI tables:
# Intel(r) ACPI Compiler from
# https://acpica.org/downloads
+# GCCRISCV - Linux - Requires:
+# RISC-V official release of RISC-V GNU toolchain,
+# https://github.com/riscv/riscv-gnu-toolchain @64879b24
+# The commit ID 64879b24 is the one can build RISC-V
platform and boo to EFI shell.
+# Follow the instructions mentioned in README.md to
build RISC-V tool change.
+# Set below environment variables to the RISC-V tool chain
binaries before building RISC-V EDK2 port.
+# - GCC_RISCV32_BIN
+# - GCC_RISCV64_BIN
+#
# CLANG35 -Linux,Windows- Requires:
# Clang v3.5 or later, and GNU binutils targeting aarch64-
linux-gnu or arm-linux-gnueabi
# Optional:
@@ -1806,6 +1821,26 @@ DEFINE GCC5_ARM_ASLDLINK_FLAGS =
DEF(GCC49_ARM_ASLDLINK_FLAGS)
DEFINE GCC5_AARCH64_ASLDLINK_FLAGS =
DEF(GCC49_AARCH64_ASLDLINK_FLAGS)
DEFINE GCC5_ASLCC_FLAGS = DEF(GCC49_ASLCC_FLAGS) -fno-lto

+DEFINE GCC_RISCV_ALL_CC_FLAGS = -g -fshort-wchar -fno-
strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-
sections -c -include AutoGen.h -fno-common -
DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
+DEFINE GCC_RISCV_ALL_DLINK_COMMON = -nostdlib -n -q --gc-
sections -z common-page-size=0x40
+DEFINE GCC_RISCV_ALL_DLINK_FLAGS =
DEF(GCC_RISCV_ALL_DLINK_COMMON) --entry $(IMAGE_ENTRY_POINT) -u
$(IMAGE_ENTRY_POINT) -Map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
+DEFINE GCC_RISCV_ALL_DLINK2_FLAGS = --
defsym=PECOFF_HEADER_SIZE=0x220 --
script=$(EDK_TOOLS_PATH)/Scripts/GccBaseRiscV.lds
+DEFINE GCC_RISCV_ALL_ASM_FLAGS = -c -x assembler -imacros
$(DEST_DIR_DEBUG)/AutoGen.h
+DEFINE GCC_RISCV_RISCV32_DLINK2_FLAGS = --
defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
+
+DEFINE GCCRISCV_RISCV32_ARCH = rv32imafdc
+DEFINE GCCRISCV_RISCV64_ARCH = rv64imafdc
+DEFINE GCCRISCV_CC_FLAGS_WARNING_DISABLE = -Wno-
tautological-compare -Wno-pointer-compare
+DEFINE GCCRISCV_RISCV32_CC_FLAGS =
DEF(GCC_RISCV_ALL_CC_FLAGS)
DEF(GCCRISCV_CC_FLAGS_WARNING_DISABLE) -
march=DEF(GCCRISCV_RISCV32_ARCH) -malign-double -fno-stack-protector
-D EFI32 -fno-asynchronous-unwind-tables -Wno-address -Wno-unused-but-
set-variable -fpack-struct=8
+DEFINE GCCRISCV_RISCV64_CC_FLAGS =
DEF(GCC_RISCV_ALL_CC_FLAGS)
DEF(GCCRISCV_CC_FLAGS_WARNING_DISABLE) -
march=DEF(GCCRISCV_RISCV64_ARCH) -fno-builtin -fno-builtin-memcpy -
fno-stack-protector -Wno-address -fno-asynchronous-unwind-tables -Wno-
unused-but-set-variable -fpack-struct=8 -mcmodel=medany -mabi=lp64
+DEFINE GCCRISCV_RISCV32_RISCV64_DLINK_COMMON = -nostdlib -n -q
--gc-sections -z common-page-size=0x40
+DEFINE GCCRISCV_RISCV32_RISCV64_ASLDLINK_FLAGS =
DEF(GCC_RISCV_ALL_DLINK_COMMON) --entry ReferenceAcpiTable -u
ReferenceAcpiTable
+DEFINE GCCRISCV_RISCV32_RISCV64_DLINK_FLAGS =
DEF(GCC_RISCV_ALL_DLINK_COMMON) --entry $(IMAGE_ENTRY_POINT) -u
$(IMAGE_ENTRY_POINT) -Map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
+DEFINE GCCRISCV_RISCV32_DLINK2_FLAGS =
DEF(GCC_RISCV_RISCV32_DLINK2_FLAGS)
+DEFINE GCCRISCV_RISCV64_DLINK_FLAGS =
DEF(GCC_RISCV_ALL_DLINK_FLAGS) -melf64lriscv --oformat=elf64-littleriscv
--no-relax
+DEFINE GCCRISCV_RISCV64_DLINK2_FLAGS =
DEF(GCC_RISCV_ALL_DLINK2_FLAGS)
+DEFINE GCCRISCV_ASM_FLAGS =
DEF(GCC_RISCV_ALL_ASM_FLAGS) -march=DEF(GCCRISCV_RISCV64_ARCH)
-mcmodel=medany -mabi=lp64
+
##########################################################
##########################
#
# GCC 4.8 - This configuration is used to compile under Linux to produce
@@ -2247,6 +2282,77 @@ RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z
common-page-size=0x20
NOOPT_GCC5_AARCH64_DLINK_FLAGS =
DEF(GCC5_AARCH64_DLINK_FLAGS) -O0
NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
-O0

+#########################################################
##########################
+#########################################################
###########################
+#
+# GCC RISC-V This configuration is used to compile under Linux to produce
+# PE/COFF binaries using GCC RISC-V tool chain
+# https://github.com/riscv/riscv-gnu-toolchain @64879b24
+# The commit ID 64879b24 is the one can build RISC-V platform and boo to
EFI shell.
+#
+#########################################################
###########################

Please don't do this. This mistake was made for the ARM port, and it
caused us nothing but pain.

Please add the requisite support to the GCC5 profile instead. (Which
is not actually for gcc 5, but is effectively GCC5+ - we are still
successfully using it with gcc 9.)
I can try to use GCC5 profile but the toolchain still has to be
stick on https://github.com/riscv/riscv-gnu-toolchain @64879b24. We
got problem on the version higher than this, system hangs at SEC to
PEI transition if use GCC version higher than @64879b24.

We will figure it out later.
I suppose this is fine as long as this is specifically on the
edk2-staging branch. We will need to resolve it before we bring the
port to edk2 master.

I would still like this support to be tweaked to the point where I can
build with either the Fedora or the Debian packaged cross compiler.

So how can I mention this restrictions in tool_def?
I would suggest the following:
- The above information in the top-level Readme.md on the staging branch.
- A single line in the "GCC5 RISCV64 definitions" comment block.
- Adding the above to the commit message.

Best Regards,

Leif


[PATCH 2/2] tests: q35: MCH: add default SMBASE SMRAM lock test

Igor Mammedov
 

test lockable SMRAM at default SMBASE feature introduced by
commit "q35: implement 128K SMRAM at default SMBASE address"

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
tests/q35-test.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)

diff --git a/tests/q35-test.c b/tests/q35-test.c
index a68183d513..dd02660303 100644
--- a/tests/q35-test.c
+++ b/tests/q35-test.c
@@ -186,6 +186,109 @@ static void test_tseg_size(const void *data)
qtest_quit(qts);
}

+#define SMBASE 0x30000
+#define SMRAM_TEST_PATTERN 0x32
+#define SMRAM_TEST_RESET_PATTERN 0x23
+
+static void test_smram_smbase_lock(void)
+{
+ QPCIBus *pcibus;
+ QPCIDevice *pcidev;
+ QDict *response;
+ QTestState *qts;
+ int i;
+
+ qts = qtest_init("-M q35");
+
+ pcibus = qpci_new_pc(qts, NULL);
+ g_assert(pcibus != NULL);
+
+ pcidev = qpci_device_find(pcibus, 0);
+ g_assert(pcidev != NULL);
+
+ /* check that SMRAM is not enabled by default */
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
+
+ /* check that writinng junk to 0x9c before before negotiating is ignorred */
+ for (i = 0; i < 0xff; i++) {
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
+ }
+
+ /* enable SMRAM at SMBASE */
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);
+ /* lock SMRAM at SMBASE */
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0x02);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x02);
+
+ /* check that SMRAM at SMBASE is locked and can't be unlocked */
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, 0xff);
+ for (i = 0; i <= 0xff; i++) {
+ /* make sure register is immutable */
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x02);
+
+ /* RAM access should go inot black hole */
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, 0xff);
+ }
+
+ /* reset */
+ response = qtest_qmp(qts, "{'execute': 'system_reset', 'arguments': {} }");
+ g_assert(response);
+ g_assert(!qdict_haskey(response, "error"));
+ qobject_unref(response);
+
+ /* check RAM at SMBASE is available after reset */
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_RESET_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_RESET_PATTERN);
+
+ g_free(pcidev);
+ qpci_free_pc(pcibus);
+
+ qtest_quit(qts);
+}
+
+static void test_without_smram_base(void)
+{
+ QPCIBus *pcibus;
+ QPCIDevice *pcidev;
+ QTestState *qts;
+ int i;
+
+ qts = qtest_init("-M pc-q35-4.1");
+
+ pcibus = qpci_new_pc(qts, NULL);
+ g_assert(pcibus != NULL);
+
+ pcidev = qpci_device_find(pcibus, 0);
+ g_assert(pcidev != NULL);
+
+ /* check that RAM accessible */
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
+
+ /* check that writing to 0x9c succeeds */
+ for (i = 0; i <= 0xff; i++) {
+ qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
+ g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == i);
+ }
+
+ /* check that RAM is still accessible */
+ qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN + 1);
+ g_assert_cmpint(qtest_readb(qts, SMBASE), ==, (SMRAM_TEST_PATTERN + 1));
+
+ g_free(pcidev);
+ qpci_free_pc(pcibus);
+
+ qtest_quit(qts);
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -197,5 +300,7 @@ int main(int argc, char **argv)
qtest_add_data_func("/q35/tseg-size/8mb", &tseg_8mb, test_tseg_size);
qtest_add_data_func("/q35/tseg-size/ext/16mb", &tseg_ext_16mb,
test_tseg_size);
+ qtest_add_func("/q35/smram/smbase_lock", test_smram_smbase_lock);
+ qtest_add_func("/q35/smram/legacy_smbase", test_without_smram_base);
return g_test_run();
}
--
2.18.1


[PATCH 1/2] q35: implement 128K SMRAM at default SMBASE address

Igor Mammedov
 

Use commit (2f295167e0 q35/mch: implement extended TSEG sizes) for
inspiration and (ab)use reserved register in config space at 0x9c
offset [*] to extend q35 pci-host with ability to use 128K at
0x30000 as SMRAM and hide it (like TSEG) from non-SMM context.

Usage:
1: write 0xff in the register
2: if the feature is supported, follow up read from the register
should return 0x01. At this point RAM at 0x30000 is still
available for SMI handler configuration from non-SMM context
3: writing 0x02 in the register, locks SMBASE area, making its contents
available only from SMM context. In non-SMM context, reads return
0xff and writes are ignored. Further writes into the register are
ignored until the system reset.

*) https://www.mail-archive.com/qemu-devel@nongnu.org/msg455991.html

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/hw/pci-host/q35.h | 10 +++++
hw/i386/pc.c | 4 +-
hw/pci-host/q35.c | 80 +++++++++++++++++++++++++++++++++++----
3 files changed, 86 insertions(+), 8 deletions(-)

diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index b3bcf2e632..976fbae599 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -32,6 +32,7 @@
#include "hw/acpi/ich9.h"
#include "hw/pci-host/pam.h"
#include "hw/i386/intel_iommu.h"
+#include "qemu/units.h"

#define TYPE_Q35_HOST_DEVICE "q35-pcihost"
#define Q35_HOST_DEVICE(obj) \
@@ -54,6 +55,8 @@ typedef struct MCHPCIState {
MemoryRegion smram_region, open_high_smram;
MemoryRegion smram, low_smram, high_smram;
MemoryRegion tseg_blackhole, tseg_window;
+ MemoryRegion smbase_blackhole, smbase_window;
+ bool has_smram_at_smbase;
Range pci_hole;
uint64_t below_4g_mem_size;
uint64_t above_4g_mem_size;
@@ -97,6 +100,13 @@ typedef struct Q35PCIHost {
#define MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY 0xffff
#define MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_MAX 0xfff

+#define MCH_HOST_BRIDGE_SMBASE_SIZE (128 * KiB)
+#define MCH_HOST_BRIDGE_SMBASE_ADDR 0x30000
+#define MCH_HOST_BRIDGE_F_SMBASE 0x9c
+#define MCH_HOST_BRIDGE_F_SMBASE_QUERY 0xff
+#define MCH_HOST_BRIDGE_F_SMBASE_IN_RAM 0x01
+#define MCH_HOST_BRIDGE_F_SMBASE_LCK 0x02
+
#define MCH_HOST_BRIDGE_PCIEXBAR 0x60 /* 64bit register */
#define MCH_HOST_BRIDGE_PCIEXBAR_SIZE 8 /* 64bit register */
#define MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT 0xb0000000
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bad866fe44..288d28358a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -119,7 +119,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
/* Physical Address of PVH entry point read from kernel ELF NOTE */
static size_t pvh_start_addr;

-GlobalProperty pc_compat_4_1[] = {};
+GlobalProperty pc_compat_4_1[] = {
+ { "mch", "smbase-smram", "off" },
+};
const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);

GlobalProperty pc_compat_4_0[] = {};
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 158d270b9f..c1bd9f78ae 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -275,20 +275,20 @@ static const TypeInfo q35_host_info = {
* MCH D0:F0
*/

-static uint64_t tseg_blackhole_read(void *ptr, hwaddr reg, unsigned size)
+static uint64_t blackhole_read(void *ptr, hwaddr reg, unsigned size)
{
return 0xffffffff;
}

-static void tseg_blackhole_write(void *opaque, hwaddr addr, uint64_t val,
- unsigned width)
+static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned width)
{
/* nothing */
}

-static const MemoryRegionOps tseg_blackhole_ops = {
- .read = tseg_blackhole_read,
- .write = tseg_blackhole_write,
+static const MemoryRegionOps blackhole_ops = {
+ .read = blackhole_read,
+ .write = blackhole_write,
.endianness = DEVICE_NATIVE_ENDIAN,
.valid.min_access_size = 1,
.valid.max_access_size = 4,
@@ -430,6 +430,46 @@ static void mch_update_ext_tseg_mbytes(MCHPCIState *mch)
}
}

+static void mch_update_smbase_smram(MCHPCIState *mch)
+{
+ PCIDevice *pd = PCI_DEVICE(mch);
+ uint8_t *reg = pd->config + MCH_HOST_BRIDGE_F_SMBASE;
+ bool lck;
+
+ if (!mch->has_smram_at_smbase) {
+ return;
+ }
+
+ if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
+ pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
+ MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
+ return;
+ }
+
+ /*
+ * default/reset state, discard written value
+ * which will disable SMRAM balackhole at SMBASE
+ */
+ if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
+ *reg = 0x00;
+ }
+
+ memory_region_transaction_begin();
+ if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
+ /* disable all writes */
+ pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
+ ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ lck = true;
+ } else {
+ lck = false;
+ }
+ memory_region_set_enabled(&mch->smbase_blackhole, lck);
+ memory_region_set_enabled(&mch->smbase_window, lck);
+ memory_region_transaction_commit();
+}
+
static void mch_write_config(PCIDevice *d,
uint32_t address, uint32_t val, int len)
{
@@ -456,6 +496,10 @@ static void mch_write_config(PCIDevice *d,
MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_SIZE)) {
mch_update_ext_tseg_mbytes(mch);
}
+
+ if (ranges_overlap(address, len, MCH_HOST_BRIDGE_F_SMBASE, 1)) {
+ mch_update_smbase_smram(mch);
+ }
}

static void mch_update(MCHPCIState *mch)
@@ -464,6 +508,7 @@ static void mch_update(MCHPCIState *mch)
mch_update_pam(mch);
mch_update_smram(mch);
mch_update_ext_tseg_mbytes(mch);
+ mch_update_smbase_smram(mch);

/*
* pci hole goes from end-of-low-ram to io-apic.
@@ -514,6 +559,9 @@ static void mch_reset(DeviceState *qdev)
MCH_HOST_BRIDGE_EXT_TSEG_MBYTES_QUERY);
}

+ d->config[MCH_HOST_BRIDGE_F_SMBASE] = 0;
+ d->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0xff;
+
mch_update(mch);
}

@@ -563,7 +611,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
memory_region_add_subregion(&mch->smram, 0xfeda0000, &mch->high_smram);

memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch),
- &tseg_blackhole_ops, NULL,
+ &blackhole_ops, NULL,
"tseg-blackhole", 0);
memory_region_set_enabled(&mch->tseg_blackhole, false);
memory_region_add_subregion_overlap(mch->system_memory,
@@ -575,6 +623,23 @@ static void mch_realize(PCIDevice *d, Error **errp)
memory_region_set_enabled(&mch->tseg_window, false);
memory_region_add_subregion(&mch->smram, mch->below_4g_mem_size,
&mch->tseg_window);
+
+ memory_region_init_io(&mch->smbase_blackhole, OBJECT(mch), &blackhole_ops,
+ NULL, "smbase-blackhole",
+ MCH_HOST_BRIDGE_SMBASE_SIZE);
+ memory_region_set_enabled(&mch->smbase_blackhole, false);
+ memory_region_add_subregion_overlap(mch->system_memory,
+ MCH_HOST_BRIDGE_SMBASE_ADDR,
+ &mch->smbase_blackhole, 1);
+
+ memory_region_init_alias(&mch->smbase_window, OBJECT(mch),
+ "smbase-window", mch->ram_memory,
+ MCH_HOST_BRIDGE_SMBASE_ADDR,
+ MCH_HOST_BRIDGE_SMBASE_SIZE);
+ memory_region_set_enabled(&mch->smbase_window, false);
+ memory_region_add_subregion(&mch->smram, MCH_HOST_BRIDGE_SMBASE_ADDR,
+ &mch->smbase_window);
+
object_property_add_const_link(qdev_get_machine(), "smram",
OBJECT(&mch->smram), &error_abort);

@@ -601,6 +666,7 @@ uint64_t mch_mcfg_base(void)
static Property mch_props[] = {
DEFINE_PROP_UINT16("extended-tseg-mbytes", MCHPCIState, ext_tseg_mbytes,
16),
+ DEFINE_PROP_BOOL("smbase-smram", MCHPCIState, has_smram_at_smbase, true),
DEFINE_PROP_END_OF_LIST(),
};

--
2.18.1


[PATCH 0/2] q35: mch: allow to lock down 128K RAM at default SMBASE address

Igor Mammedov
 

Try #2 using PCI config space of MCH to negotiate/lock SMRAM
at 0x30000.

CC: yingwen.chen@intel.com
CC: devel@edk2.groups.io
CC: phillip.goerl@oracle.com
CC: alex.williamson@redhat.com
CC: jiewen.yao@intel.com
CC: jun.nakajima@intel.com
CC: michael.d.kinney@intel.com
CC: pbonzini@redhat.com
CC: boris.ostrovsky@oracle.com
CC: rfc@edk2.groups.io
CC: joao.m.martins@oracle.com
CC: lersek@redhat.com


Igor Mammedov (2):
q35: implement 128K SMRAM at default SMBASE address
tests: q35: MCH: add default SMBASE SMRAM lock test

include/hw/pci-host/q35.h | 10 ++++
hw/i386/pc.c | 4 +-
hw/pci-host/q35.c | 80 ++++++++++++++++++++++++++---
tests/q35-test.c | 105 ++++++++++++++++++++++++++++++++++++++
4 files changed, 191 insertions(+), 8 deletions(-)

--
2.18.1