[edk2-devel] [edk2 PATCH 00/48] ArmVirtPkg, OvmfPkg: virtio filesystem driver
Laszlo Ersek
lersek at redhat.com
Mon Dec 21 01:46:40 UTC 2020
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.)
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69286): https://edk2.groups.io/g/devel/message/69286
Mute This Topic: https://groups.io/mt/79022474/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list