Re: [edk2-devel] [PATCH v1 1/2] ArmVirtPkg/PlatformCI: Add EDKII CI support for Kvmtool

["Sean" <spbrogan@outlook.com>]

Sami,

Looking over this.
The root of the issue is that when I setup the PlatformBuild.py file i 
chose to split the objects.
	PlatformBuilder is the UefiBuilder and BuildSettingsManager
	SettingsManager is the Update, Setup, and PrEval object.

This means that you need to add that argument to both objects.
I made a few changes based on your v1 (i really prefer not to dup the 
PlatformBuild.py unless there are major differences).

https://github.com/spbrogan/edk2/tree/spbrogan_kvm_feedback

I just submitted the PR here: https://github.com/tianocore/edk2/pull/1460


One concern is to make PR evaluation reliable it requires a change in 
the platform build steps template as build flags are not sent to the 
PR_EVAL step and thus the wrong DSC would be picked up for your platform.

https://github.com/spbrogan/edk2/commit/6b7b405db248356df8ec080add839e6652f180a5

This should probably land first and then your addition of this platform.

Finally,
My other question is this is adding 6 more builds for each PR.
Do you think this platform provides enough differientation that it is 
worth taking the overhead?  We (edk2 community) may need to rethink 
platform CI as it can eat up a lot of resources.


Thanks
Sean





On 2/25/2021 6:39 AM, Sami Mujawar wrote:
> Hi All,
> 
> It appears that the --dsc parameter would fail in the stuart_setup stage when running in the upstream EDKII Core CI environment. For some reason it worked for me in the local CI builds.
> 
> I am testing a v2 version of my patch at https://github.com/samimujawar/edk2/tree/1596_kvmtool_ci_v2 and will submit it shortly.
> 
> Regards,
> 
> Sami Mujawar
> 
> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: 25 February 2021 02:31 PM
> To: devel@edk2.groups.io; spbrogan@outlook.com; Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: ardb+tianocore@kernel.org; leif@nuviainc.com; sean.brogan@microsoft.com; Bret.Barkelew@microsoft.com; michael.d.kinney@intel.com; lersek@redhat.com; Matteo Carlini <Matteo.Carlini@arm.com>; Ben Adderson <Ben.Adderson@arm.com>; nd <nd@arm.com>
> Subject: 回复: [edk2-devel] [PATCH v1 1/2] ArmVirtPkg/PlatformCI: Add EDKII CI support for Kvmtool
> 
> Sean:
> 
>> -----邮件原件-----
>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Sean
>> 发送时间: 2021年2月24日 6:32
>> 收件人: devel@edk2.groups.io; sami.mujawar@arm.com
>> 抄送: ardb+tianocore@kernel.org; leif@nuviainc.com;
>> sean.brogan@microsoft.com; Bret.Barkelew@microsoft.com;
>> michael.d.kinney@intel.com; gaoliming@byosoft.com.cn; lersek@redhat.com;
>> Matteo.Carlini@arm.com; Ben.Adderson@arm.com; nd@arm.com
>> 主题: Re: [edk2-devel] [PATCH v1 1/2] ArmVirtPkg/PlatformCI: Add EDKII CI
>> support for Kvmtool
>>
>> Sami,
>>
>> Do you have these in a PR or somewhere online that is already merged?
>> Obviously i can do that but usually developers already have that (either
>> edk2 PR for ci testing or on their fork).
>>
>> one comment below.
>>
>> Thanks
>> Sean
>>
>>
>> On 1/22/2021 9:19 AM, Sami Mujawar wrote:
>>> Kvmtool is a virtual machine manager that can be used to launch
>>> guest partitions. ArmVirtPkg already has UEFI (virtual/guest)
>>> firmware support for Kvmtool guest.
>>>
>>> Therefore, update the Platform CI script to add support for
>>> building the Kvmtool firmware.
>>>
>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>> ---
>>>    ArmVirtPkg/PlatformCI/PlatformBuild.py | 132 +++++++++++---------
>>>    ArmVirtPkg/PlatformCI/ReadMe.md        |  21 ++--
>>>    2 files changed, 88 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/PlatformCI/PlatformBuild.py
>> b/ArmVirtPkg/PlatformCI/PlatformBuild.py
>>> index
>> dff653e919eb42391fc56ec44b4043a75f79d162..473f7d58d15c3e26ef5a25e2
>> 10cb679679b28131 100644
>>> --- a/ArmVirtPkg/PlatformCI/PlatformBuild.py
>>> +++ b/ArmVirtPkg/PlatformCI/PlatformBuild.py
>>> @@ -2,6 +2,7 @@
>>>    # Script to Build ArmVirtPkg UEFI firmware
>>>    #
>>>    # Copyright (c) Microsoft Corporation.
>>> +# Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
>>>    # SPDX-License-Identifier: BSD-2-Clause-Patent
>>>    ##
>>>    import os
>>> @@ -139,7 +140,8 @@ class SettingsManager(UpdateSettingsManager,
>> SetupSettingsManager, PrEvalSetting
>>>
>>>            The tuple should be (<workspace relative path to dsc file>,
>> <input dictionary of dsc key value pairs>)
>>>            '''
>>
>> This doesn't look right.  When returning the dsc to use it should only
>> return 1 dsc file.  The second parameter of the tuple is for key=value
>> pairs to process the DSC file.
>>
> 
> If the second parameter is not DSC file, that means ArmVirtKvmTool.dsc is not used.
> So, this patch doesn't enable CI support for ArmVirtKvmTool. Right?
> 
> Thanks
> Liming
>>
>>> -        return (os.path.join("ArmVirtPkg", "ArmVirtQemu.dsc"), {})
>>> +        return (os.path.join("ArmVirtPkg", "ArmVirtQemu.dsc"),
>>> +                os.path.join("ArmVirtPkg", "ArmVirtKvmTool.dsc"), {})
>>>
>>>
>>>        #
>> ##############################################################
>> ######################### #
>>> @@ -150,11 +152,15 @@ class SettingsManager(UpdateSettingsManager,
>> SetupSettingsManager, PrEvalSetting
>>>    class PlatformBuilder(UefiBuilder, BuildSettingsManager):
>>>        def __init__(self):
>>>            UefiBuilder.__init__(self)
>>> +        self.PlatformList = [os.path.join("ArmVirtPkg",
>> "ArmVirtQemu.dsc"),
>>> +                        os.path.join("ArmVirtPkg",
>> "ArmVirtKvmTool.dsc")]
>>>
>>>        def AddCommandLineOptions(self, parserObj):
>>>            ''' Add command line options to the argparser '''
>>>            parserObj.add_argument('-a', "--arch", dest="build_arch",
>> type=str, default="AARCH64",
>>>                                   help="Optional - Architecture to
>> build.  Default = AARCH64")
>>> +        parserObj.add_argument('-d', "--dsc", dest="active_platform",
>> type=str, default=self.PlatformList[0],
>>> +                               help="Optional - Platform to build.
>> Default = " + self.PlatformList[0])
>>>
>>>        def RetrieveCommandLineOptions(self, args):
>>>            '''  Retrieve command line options from the argparser '''
>>> @@ -162,8 +168,12 @@ class PlatformBuilder(UefiBuilder,
>> BuildSettingsManager):
>>>            shell_environment.GetBuildVars().SetValue(
>>>                "TARGET_ARCH", args.build_arch.upper(), "From
>> CmdLine")
>>>
>>> -        shell_environment.GetBuildVars().SetValue(
>>> -            "ACTIVE_PLATFORM", "ArmVirtPkg/ArmVirtQemu.dsc",
>> "From CmdLine")
>>> +        if (args.active_platform == self.PlatformList[1]):
>>> +            shell_environment.GetBuildVars().SetValue(
>>> +                "ACTIVE_PLATFORM", self.PlatformList[1], "From
>> CmdLine")
>>> +        else:
>>> +            shell_environment.GetBuildVars().SetValue(
>>> +                "ACTIVE_PLATFORM", self.PlatformList[0], "From
>> CmdLine")
>>>
>>>        def GetWorkspaceRoot(self):
>>>            ''' get WorkspacePath '''
>>> @@ -207,9 +217,12 @@ class PlatformBuilder(UefiBuilder,
>> BuildSettingsManager):
>>>
>>>        def SetPlatformEnv(self):
>>>            logging.debug("PlatformBuilder SetPlatformEnv")
>>> -        self.env.SetValue("PRODUCT_NAME", "ArmVirtQemu", "Platform
>> Hardcoded")
>>>            self.env.SetValue("MAKE_STARTUP_NSH", "FALSE", "Default to
>> false")
>>>            self.env.SetValue("QEMU_HEADLESS", "FALSE", "Default to
>> false")
>>> +        if (self.env.GetValue("ACTIVE_PLATFORM") ==
>> self.PlatformList[1]):
>>> +            self.env.SetValue("PRODUCT_NAME", "ArmVirtKvmtool",
>> "Platform Hardcoded")
>>> +        else:
>>> +            self.env.SetValue("PRODUCT_NAME", "ArmVirtQemu",
>> "Platform Hardcoded")
>>>            return 0
>>>
>>>        def PlatformPreBuild(self):
>>> @@ -219,58 +232,61 @@ class PlatformBuilder(UefiBuilder,
>> BuildSettingsManager):
>>>            return 0
>>>
>>>        def FlashRomImage(self):
>>> -        VirtualDrive = os.path.join(self.env.GetValue(
>>> -            "BUILD_OUTPUT_BASE"), "VirtualDrive")
>>> -        os.makedirs(VirtualDrive, exist_ok=True)
>>> -        OutputPath_FV = os.path.join(
>>> -            self.env.GetValue("BUILD_OUTPUT_BASE"), "FV")
>>> -        Built_FV = os.path.join(OutputPath_FV, "QEMU_EFI.fd")
>>> -
>>> -        # pad fd to 64mb
>>> -        with open(Built_FV, "ab") as fvfile:
>>> -            fvfile.seek(0, os.SEEK_END)
>>> -            additional = b'\0' * ((64 * 1024 * 1024)-fvfile.tell())
>>> -            fvfile.write(additional)
>>> -
>>> -        # QEMU must be on that path
>>> -
>>> -        # Unique Command and Args parameters per ARCH
>>> -        if (self.env.GetValue("TARGET_ARCH").upper() == "AARCH64"):
>>> -            cmd = "qemu-system-aarch64"
>>> -            args = "-M virt"
>>> -            args += " -cpu cortex-a57"
>> # emulate cpu
>>> -        elif(self.env.GetValue("TARGET_ARCH").upper() == "ARM"):
>>> -            cmd = "qemu-system-arm"
>>> -            args = "-M virt"
>>> -            args += " -cpu cortex-a15"
>> # emulate cpu
>>> +        if (self.env.GetValue("ACTIVE_PLATFORM") ==
>> self.PlatformList[1]):
>>> +              return 0
>>>            else:
>>> -            raise NotImplementedError()
>>> -
>>> -        # Common Args
>>> -        args += " -pflash " + Built_FV
>> # path to fw
>>> -        args += " -m 1024"
>> # 1gb memory
>>> -        # turn off network
>>> -        args += " -net none"
>>> -        # Serial messages out
>>> -        args += " -serial stdio"
>>> -        # Mount disk with startup.nsh
>>> -        args += f" -drive
>> file=fat:rw:{VirtualDrive},format=raw,media=disk"
>>> -
>>> -        # Conditional Args
>>> -        if (self.env.GetValue("QEMU_HEADLESS").upper() == "TRUE"):
>>> -            args += " -display none"  # no graphics
>>> -
>>> -        if (self.env.GetValue("MAKE_STARTUP_NSH").upper() == "TRUE"):
>>> -            f = open(os.path.join(VirtualDrive, "startup.nsh"), "w")
>>> -            f.write("BOOT SUCCESS !!! \n")
>>> -            # add commands here
>>> -            f.write("reset -s\n")
>>> -            f.close()
>>> -
>>> -        ret = RunCmd(cmd, args)
>>> -
>>> -        if ret == 0xc0000005:
>>> -            # for some reason getting a c0000005 on successful return
>>> -            return 0
>>> -
>>> -        return ret
>>> +              VirtualDrive = os.path.join(self.env.GetValue(
>>> +                  "BUILD_OUTPUT_BASE"), "VirtualDrive")
>>> +              os.makedirs(VirtualDrive, exist_ok=True)
>>> +              OutputPath_FV = os.path.join(
>>> +                  self.env.GetValue("BUILD_OUTPUT_BASE"), "FV")
>>> +              Built_FV = os.path.join(OutputPath_FV, "QEMU_EFI.fd")
>>> +
>>> +              # pad fd to 64mb
>>> +              with open(Built_FV, "ab") as fvfile:
>>> +                  fvfile.seek(0, os.SEEK_END)
>>> +                  additional = b'\0' * ((64 * 1024 * 1024)-fvfile.tell())
>>> +                  fvfile.write(additional)
>>> +
>>> +              # QEMU must be on that path
>>> +
>>> +              # Unique Command and Args parameters per ARCH
>>> +              if (self.env.GetValue("TARGET_ARCH").upper() ==
>> "AARCH64"):
>>> +                  cmd = "qemu-system-aarch64"
>>> +                  args = "-M virt"
>>> +                  args += " -cpu cortex-a57"
>> # emulate cpu
>>> +              elif(self.env.GetValue("TARGET_ARCH").upper() ==
>> "ARM"):
>>> +                  cmd = "qemu-system-arm"
>>> +                  args = "-M virt"
>>> +                  args += " -cpu cortex-a15"
>> # emulate cpu
>>> +              else:
>>> +                  raise NotImplementedError()
>>> +
>>> +              # Common Args
>>> +              args += " -pflash " + Built_FV
>> # path to fw
>>> +              args += " -m 1024"
>> # 1gb memory
>>> +              # turn off network
>>> +              args += " -net none"
>>> +              # Serial messages out
>>> +              args += " -serial stdio"
>>> +              # Mount disk with startup.nsh
>>> +              args += f" -drive
>> file=fat:rw:{VirtualDrive},format=raw,media=disk"
>>> +
>>> +              # Conditional Args
>>> +              if (self.env.GetValue("QEMU_HEADLESS").upper() ==
>> "TRUE"):
>>> +                  args += " -display none"  # no graphics
>>> +
>>> +              if (self.env.GetValue("MAKE_STARTUP_NSH").upper() ==
>> "TRUE"):
>>> +                  f = open(os.path.join(VirtualDrive, "startup.nsh"),
>> "w")
>>> +                  f.write("BOOT SUCCESS !!! \n")
>>> +                  # add commands here
>>> +                  f.write("reset -s\n")
>>> +                  f.close()
>>> +
>>> +              ret = RunCmd(cmd, args)
>>> +
>>> +              if ret == 0xc0000005:
>>> +                  # for some reason getting a c0000005 on successful
>> return
>>> +                  return 0
>>> +
>>> +              return ret
>>> diff --git a/ArmVirtPkg/PlatformCI/ReadMe.md
>> b/ArmVirtPkg/PlatformCI/ReadMe.md
>>> index
>> 7c11d925f59ede4717d4b210df9d2b97f755ebd8..98a3ca91f40c075bf1a2069
>> edd99e9680a1252e9 100644
>>> --- a/ArmVirtPkg/PlatformCI/ReadMe.md
>>> +++ b/ArmVirtPkg/PlatformCI/ReadMe.md
>>> @@ -6,13 +6,14 @@ to use the same Pytools based build infrastructure
>> locally.
>>>    ## Supported Configuration Details
>>>
>>>    This solution for building and running ArmVirtPkg has only been validated
>> with Ubuntu
>>> -18.04 and the GCC5 toolchain. Two different firmware builds are supported
>> and are
>>> -described below.
>>> +18.04 and the GCC5 toolchain. The supported firmware builds are
>> described below.
>>>
>>> -| Configuration name      | Architecture       | DSC File
>> |Additional Flags |
>>> -| :----------             | :-----             | :-----
>> | :----           |
>>> -| AARCH64                 | AARCH64            |
>> ArmVirtQemu.dsc  | None            |
>>> -| ARM                     | ARM                |
>> ArmVirtQemu.dsc  | None            |
>>> +| Configuration name      | Architecture       | DSC File
>> |Additional Flags |
>>> +| :----------             | :-----             | :-----
>> | :----           |
>>> +| AARCH64                 | AARCH64            |
>> ArmVirtQemu.dsc     | None            |
>>> +| ARM                     | ARM                |
>> ArmVirtQemu.dsc     | None            |
>>> +| AARCH64                 | AARCH64            |
>> ArmVirtKvmTool.dsc  | None            |
>>> +| ARM                     | ARM                |
>> ArmVirtKvmTool.dsc  | None            |
>>>
>>>    ## EDK2 Developer environment
>>>
>>> @@ -79,7 +80,13 @@ Pytools build system.
>>>        ```
>>>
>>>        - use `stuart_build -c ArmVirtPkg/PlatformCI/PlatformBuild.py -h`
>> option to see additional
>>> -    options like `--clean`
>>> +    options like `--clean`, `--dsc`, etc.
>>> +
>>> +    Example: The `--dsc` option can be used to specify the platform to
>> build.
>>> +
>>> +      ``` bash
>>> +      stuart_build -c ArmVirtPkg/PlatformCI/PlatformBuild.py
>> TOOL_CHAIN_TAG=<TOOL_CHAIN_TAG> -a <TARGET_ARCH> --dsc
>> ArmVirtPkg/ArmVirtKvmTool.dsc
>>> +      ```
>>>
>>>    8. Running Emulator
>>>        - You can add `--FlashRom` to the end of your build command and
>> the emulator will run after the
>>>
>>
>>
>> 
>>
> 
> 
>