Re: [edk2 PATCH 00/48] ArmVirtPkg, OvmfPkg: virtio filesystem driver

["Laszlo Ersek" <lersek@redhat.com>]

On 12/20/20 11:15, Ard Biesheuvel wrote:
> On 12/20/20 1:09 AM, Laszlo Ersek wrote:

>> The only thing I couldn't test that way was (obviously) building on
>> Windows. So clearly now that I've tried merging the series at
>> <https://github.com/tianocore/edk2/pull/1248>, that's what fails.
>> Because, this is the first time that EmbeddedPkg/TimeBaseLib is seen
>> by the Visual Studio compiler, and Visual Studio complains:
>>
>>> ERROR - Compiler #2220 from
>>> d:\a\1\s\EmbeddedPkg\Library\TimeBaseLib\TimeBaseLib.c(111):   the
>>> following warning is treated as an error
>>> WARNING - Compiler #4244 from
>>> d:\a\1\s\EmbeddedPkg\Library\TimeBaseLib\TimeBaseLib.c(111):   '=':
>>> conversion from 'UINTN' to 'UINT32', possible loss of data
>>
>> It happens to be correct:
>>
>>     99  /**
>>    100    Converts EFI_TIME to Epoch seconds (elapsed since 1970 JANUARY 01, 00:00:00 UTC)
>>    101   **/
>>    102  UINT32
>>    103  EFIAPI
>>    104  EfiTimeToEpoch (
>>    105    IN  EFI_TIME  *Time
>>    106    )
>>    107  {
>>    108    UINT32 EpochDays;   // Number of days elapsed since EPOCH_JULIAN_DAY
>>    109    UINT32 EpochSeconds;
>>    110
>>    111    EpochDays = EfiGetEpochDays (Time);
>>    112
>>    113    EpochSeconds = (EpochDays * SEC_PER_DAY) + ((UINTN)Time->Hour * SEC_PER_HOUR) + (Time->Minute * SEC_PER_MIN) + Time->Second;
>>    114
>>    115    return EpochSeconds;
>>    116  }
>>
>> This was discussed on the list earlier, when the function was duplicated
>> for the HTTP dynamic command:
>>
>> - https://edk2.groups.io/g/devel/message/64933
>> - https://edk2.groups.io/g/devel/message/65186
>>
>> Compare EfiTimeToEpoch() in
>> "ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c".
>>
>>    430  STATIC
>>    431  UINTN
>>    432  EFIAPI
>>    433  EfiTimeToEpoch (
>>    434    IN  EFI_TIME  *Time
>>    435    )
>>    436  {
>>    437    //
>>    438    // Number of days elapsed since EPOCH_JULIAN_DAY.
>>    439    //
>>    440    UINTN EpochDays;
>>    441    UINTN EpochSeconds;
>>    442
>>    443    EpochDays = EfiGetEpochDays (Time);
>>    444
>>    445    EpochSeconds = (EpochDays * SEC_PER_DAY) +
>>    446                   ((UINTN)Time->Hour * SEC_PER_HOUR) +
>>    447                   (Time->Minute * SEC_PER_MIN) + Time->Second;
>>    448
>>    449    return EpochSeconds;
>>    450  }
>>
>> So, in order to merge this series, I'll first have to fix
>> EfiTimeToEpoch() in EmbeddedPkg :(
>>
>> I wish the fixed version in
>> "ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c" had been contributed
>> back to EmbeddedPkg.
>>
>> Anyway, that is for 2021.

(given the fantastic opportunities provided by the COVID-19 pandemic, to
catch up with family and friends around Christmas /s, I might as well
treat the discussion of this patch series during my PTO as something
welcome that takes my mind off of how much I miss the people I can't
meet this year)

> Thanks for clearing up the outstanding questions.
>
> For the EmbeddedPkg change from UINT32 to UINTN
>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
> although I suppose you will have to preserve the prototype, so adding
> a (UINT32) cast to line 111 is probably the best approach here.

I ended up installing a brand new Windows 10 + VS2019 virtual machine,
as I got fed up with the nasty surprises in the PRs (and I refuse to
publish my work-in-progress branch just for the sake of setting off CI
on it).

Two consequences:

(1) I'll squash the following trivial updates into two of the patches,
for my next merge request attempt:

 5:  bb254f89067a !  7:  17a76bbec317 OvmfPkg/VirtioFsDxe: add a scatter-gather list data type
    @@ -20,6 +20,8 @@
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
         Message-Id: <20201216211125.19496-6-lersek@redhat.com>
         Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
    +    [lersek@redhat.com: suppress useless VS2019 warning about signed/unsigned
    +     comparison in VirtioFsSgListsValidate()]

     diff --git a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
     --- a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
    @@ -213,7 +215,7 @@
     +    // can be added to the virtio queue, after the other descriptors added
     +    // previously.
     +    //
    -+    if (SgList->NumVec > MAX_UINT16 - DescriptorsNeeded ||
    ++    if (SgList->NumVec > (UINTN)(MAX_UINT16 - DescriptorsNeeded) ||
     +        DescriptorsNeeded + SgList->NumVec > VirtioFs->QueueSize) {
     +      return EFI_UNSUPPORTED;
     +    }

and

46:  4f42ecc2d9bb ! 48:  b807d3c0b54b OvmfPkg/VirtioFsDxe: add helper for determining access time updates
    @@ -12,6 +12,8 @@
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
         Message-Id: <20201216211125.19496-47-lersek@redhat.com>
         Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
    +    [lersek@redhat.com: suppress bogus VS2019 warning about lack of
    +     initialization for ZeroTime]

     diff --git a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
     --- a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
    @@ -92,7 +94,7 @@
     +  EFI_TIME              *Time[3];
     +  EFI_TIME              *NewTime[ARRAY_SIZE (Time)];
     +  UINTN                 Idx;
    -+  STATIC CONST EFI_TIME ZeroTime;
    ++  STATIC CONST EFI_TIME ZeroTime = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
     +  BOOLEAN               Change[ARRAY_SIZE (Time)];
     +  UINT64                Seconds[ARRAY_SIZE (Time)];
     +

(2) I've written three patches in total for fixing EfiTimeToEpoch(),
including the prototype:

(2a) edk2-platforms:

  1  Silicon/Marvell/RealTimeClockLib: make EpochSeconds, WakeupSeconds UINTN

     Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c | 14 ++++++++++----
     1 file changed, 10 insertions(+), 4 deletions(-)

(other RealTimeClockLib instances / EfiTimeToEpoch() callers in
edk2-platforms need no updates; furthermore, edk2-non-osi contains no
EfiTimeToEpoch() calls at all)

(2b) edk2:

  1  ArmPlatformPkg/PL031RealTimeClockLib: cast EfiTimeToEpoch() val. to UINT32

     ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)

  2  EmbeddedPkg/TimeBaseLib: remove useless truncation to 32-bit

     EmbeddedPkg/Include/Library/TimeBaseLib.h     | 2 +-
     EmbeddedPkg/Library/TimeBaseLib/TimeBaseLib.c | 6 +++---
     2 files changed, 4 insertions(+), 4 deletions(-)

If you're reading this before 2021, please let me know if you'd tolerate
receiving these patches for approval still in 2020. (The edk2-platforms
patch theoretically belongs to Leif and Marcin, but if Leif has stopped
consuming work email (which we all should have by now, I guess...), then
I believe you could ACK that patch in Leif's stead.)

Thanks,
Laszlo