[Virtio-fs] [edk2 PATCH 00/48] ArmVirtPkg, OvmfPkg: virtio filesystem driver
Ard Biesheuvel
ard.biesheuvel at arm.com
Mon Dec 21 10:10:12 UTC 2020
On 12/21/20 2:46 AM, Laszlo Ersek wrote:
> 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 at 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 at redhat.com>
> Message-Id: <20201216211125.19496-6-lersek at redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel at arm.com>
> + [lersek at 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 at redhat.com>
> Message-Id: <20201216211125.19496-47-lersek at redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel at arm.com>
> + [lersek at 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.)
>
No problem.
--
Ard.
More information about the Virtio-fs
mailing list