From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.3611.1608573743721148496 for ; Mon, 21 Dec 2020 10:02:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YOjh+O4C; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1608573742; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=65wFKxE+X0WGT8IFsILYOQaikrZWAI1JcpXhY8Nfu0o=; b=YOjh+O4C2vXfchQliakUM0vZ8qneqGdTrKpvi1WBudPmkVR4LQW8tBLgUVomtD9CpJMyCO izoB2ZW9U9c6kdDUmlqxMPICT0k7WmUQKI7D0CAdsBc77DGamGBI6PgecEfKSKDsfxSrq7 3BwSGg37X3tHcFT2ib2Ei94TpREtznU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-99-3VqrJGuaOtygjmyv2p4SfA-1; Mon, 21 Dec 2020 13:02:16 -0500 X-MC-Unique: 3VqrJGuaOtygjmyv2p4SfA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 97288B8103; Mon, 21 Dec 2020 18:02:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-71.ams2.redhat.com [10.36.114.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id F121C5C1D1; Mon, 21 Dec 2020 18:02:08 +0000 (UTC) Subject: Re: [edk2-devel] [edk2 PATCH 00/48] ArmVirtPkg, OvmfPkg: virtio filesystem driver To: devel@edk2.groups.io, ard.biesheuvel@arm.com, virtio-fs@redhat.com Cc: Jordan Justen , Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , "Liming Gao (Byosoft address)" References: <20201216211125.19496-1-lersek@redhat.com> <5f75d127-a110-f256-19ad-4aa120332d6c@redhat.com> <7d1d4694-8fe2-7a63-7679-a6c0a0287113@arm.com> From: "Laszlo Ersek" Message-ID: <1f510693-832b-8b57-fce8-e415aaf4e238@redhat.com> Date: Mon, 21 Dec 2020 19:02:07 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/21/20 11:10, Ard Biesheuvel wrote: > 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 >>>> , 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 >>> >>> 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 >> Message-Id: <20201216211125.19496-6-lersek@redhat.com> >> Acked-by: Ard Biesheuvel >> + [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 >> Message-Id: <20201216211125.19496-47-lersek@redhat.com> >> Acked-by: Ard Biesheuvel >> + [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.) >> > > No problem. > Merged as commit range c06635ea3f4b..35ed29f207fd, via . I've also added to . Thank you, Ard! Laszlo