Re: [edk2-devel] [PATCH v3 06/15] ArmVirtPkg: Add Kvmtool NOR flash lib

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 6/25/20 1:19 PM, Ard Biesheuvel wrote:
> On 6/25/20 10:45 AM, Philippe Mathieu-Daudé via groups.io wrote:
>> Hi Sami,
>>
>> On 6/24/20 3:34 PM, Sami Mujawar wrote:
>>> Kvmtool places the base address of the CFI flash in
>>> the device tree it passes to UEFI. This library
>>> parses the kvmtool device tree to read the CFI base
>>> address and initialise the PCDs use by the NOR flash
>>> driver and the variable storage.
>>>
>>> UEFI takes ownership of the CFI flash hardware, and
>>> exposes its functionality through the UEFI Runtime
>>> Variable Service. Therefore, disable the device tree
>>> node for the CFI flash used for storing the UEFI
>>> variables, to prevent the OS from attaching its device
>>> driver as well.
>>>
>>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>      v3:
>>>        - ASSERT is sufficient to test Locating                     [Ard]
>>>          gFdtClientProtocolGuid as DEPEX ensures that this is
>>>          guaranteed to succeed.
>>>        - Removed additional error handling based on review        
>>> [Sami]
>>>          feedback.
>>>        - Fix confusion caused by use of macro MAX_FLASH_BANKS.    
>>> [Philippe]
>>>        - Renamed MAX_FLASH_BANKS to MAX_FLASH_DEVICES.            
>>> [Sami]
>>>        - Use macro to define block size for flash.                
>>> [Philippe]
>>>        - Defined macro KVMTOOL_NOR_BLOCK_SIZE and also configured 
>>> [Sami]
>>>          to reflect the correct block size 64KB.
>>>        - Disable the DT flash node used for UEFI variable storage 
>>> [Sami]
>>>          as UEFI takes ownership of the flash device.
>>>          Ref: https://edk2.groups.io/g/devel/topic/74200914#60341
>>>           v2:
>>>        - Library to read CFI flash base address from DT and
>>> initialise [Sami]
>>>          PCDs used for NOR flash variables.
>>>
>>>   ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c      | 330
>>> ++++++++++++++++++++
>>>   ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf |  49 +++
>>>   2 files changed, 379 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
>>> b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
>>> new file mode 100644
>>> index
>>> 0000000000000000000000000000000000000000..8e9dcf31691b4b12b9c7bac1ad4ba8d3a534a1d8
>>>
>>> --- /dev/null
>>> +++ b/ArmVirtPkg/Library/NorFlashKvmtoolLib/NorFlashKvmtool.c
>>> @@ -0,0 +1,330 @@
>>> +/** @file
>>> +   An instance of the NorFlashPlatformLib for Kvmtool platform.
>>> +
>>> + Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
>>> +
>>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> + **/
>>> +
>>> +#include <Library/BaseLib.h>
>>> +#include <Library/DebugLib.h>
>>> +#include <Library/NorFlashPlatformLib.h>
>>> +#include <Library/UefiBootServicesTableLib.h>
>>> +#include <Protocol/FdtClient.h>
>>> +
>>> +/** Macro defining the NOR block size configured in Kvmtool.
>>> +*/
>>> +#define KVMTOOL_NOR_BLOCK_SIZE  SIZE_64KB
>>> +
>>> +/** Macro defining the maximum number of Flash devices.
>>> +*/
>>> +#define MAX_FLASH_DEVICES       4
>>
>> I am sorry but I am still confused...
>>
>> This is about the QEMU Virt machine, right?
>>
>> This machine was supposed to have 1 single flash, see QEMU commit
>> f5fdcd6e58 ("hw/arm: Add 'virt' platform") from Nov 2013:
>>
>>    /* Addresses and sizes of our components.
>>     * 0..128MB is space for a flash device so we can run bootrom code
>> such as UEFI.
>>     ...
>>
>> Due to limitations in the QEMU cfi-flash model, instead of using
>> a single flash device (with proper sector/bank protection), two
>> devices were added in QEMU commit acf82361c6 ("hw/arm/virt: Provide
>> flash devices for boot ROMs") Sep 2014:
>>
>>      Add two flash devices to the virt board, so that it can be used for
>>      running guests which want a bootrom image such as UEFI. We provide
>>      two flash devices to make it more convenient to provide both a
>>      read-only UEFI image and a read-write place to store guest-set
>>      UEFI config variables. The '-bios' command line option is set up
>>      to provide an image for the first of the two flash devices.
>>
>> What do you declare maximum 4 devices?
>>
> 
> Kvmtool != QEMU, so whatever QEMU does is irrelevant for this series.
> 

Ah, I feel relieved, thanks :)