From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.6476.1584042192722896612 for ; Thu, 12 Mar 2020 12:43:12 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UlLrDdps; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1584042191; 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=iVtSimwLPcIgUBFq4vV52PWDxWzOgCHFY27B8oxkacA=; b=UlLrDdpsCu6eeDInz2VGyeMwFIdnyatNO0+4xtZCw8YmE9pq7HwLrwVxuMgUDhxW1l0/Lp 4ru9Fo31hMEYJGn4ZnzeytwfMOONJl7jfPnRzsAJQFXnGeDggo3OEE/bAhFo4OGO0P6fCP JAo9+aQw9mVR2B2gzb9c2b5s4cVnm9E= 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-224-bE2_Jr4LPkeVtvXsumDW6w-1; Thu, 12 Mar 2020 15:43:07 -0400 X-MC-Unique: bE2_Jr4LPkeVtvXsumDW6w-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 966E31902ECA; Thu, 12 Mar 2020 19:43:05 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-74.ams2.redhat.com [10.36.117.74]) by smtp.corp.redhat.com (Postfix) with ESMTP id 89DA560BF1; Thu, 12 Mar 2020 19:42:59 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem instead of GUID assignment To: Leif Lindholm , "Chang, Abner (HPS SW/FW Technologist)" Cc: "Schaefer, Daniel (DualStudy)" , "devel@edk2.groups.io" , "Chen, Gilbert" , "afish@apple.com" , "michael.d.kinney@intel.com" , "pete@akeo.ie" , Ard Biesheuvel References: <20200302103238.25726-1-daniel.schaefer@hpe.com> <20200302103238.25726-4-daniel.schaefer@hpe.com> <20200312105528.GC23627@bivouac.eciton.net> <539c8673-786c-9c58-98cc-ab470b345740@hpe.com> <20200312140304.GF23627@bivouac.eciton.net> <20200312144452.GI23627@bivouac.eciton.net> From: "Laszlo Ersek" Message-ID: Date: Thu, 12 Mar 2020 20:42:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200312144452.GI23627@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/12/20 15:44, Leif Lindholm wrote: > And what would you propose we do the next time the RISC-V toolchain > generates a memcpy call based on some other completely valid change to > core code? We could choose to enable the intrinsics library for RISC-V at that point. IIUC, the CreateDeviceManagerForm() code in question did break an edk2 rule ("don't use structure assignment") *prior* to commit 64a228f5f893. The rule violation was in commit 32465d9ae7ee; RISC-V only exposed it. This doesn't seem uncharted territory. Thanks Laszlo > Doing this de-risks the RISC-V upstreaming effort. > It's also a trivial move/rename opetation - the only question is where > the library should live and be called. > > / > Leif > > On Thu, Mar 12, 2020 at 14:33:27 +0000, Chang, Abner (HPS SW/FW Technologist) wrote: >> I don't prefer to do this at this moment, Leif. I have no problem if >> we create a BZ for this and create BaseCompilerIntrinsicsLib in >> MdeModulePkg, but please don't bind it with RISC-V EDK2 port. You >> know my concern pretty well, we can't hold those RISC-V patches on >> hands like forever and address the code structure optimization. We >> can still work on BaseCompilerIntrinsicsLib but not part of this >> RISC-V submission. We can implement RISC-V variant if necessary >> after RISC-V edk2 get in edk2 master. >> >> Abner >> >>> -----Original Message----- >>> From: Leif Lindholm [mailto:leif@nuviainc.com] >>> Sent: Thursday, March 12, 2020 10:03 PM >>> To: Schaefer, Daniel (DualStudy) >>> Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist) >>> ; Chen, Gilbert ; >>> afish@apple.com; michael.d.kinney@intel.com; pete@akeo.ie; Ard >>> Biesheuvel ; Laszlo Ersek >>> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem >>> instead of GUID assignment >>> >>> +Ard, Laszlo. >>> >>> I think it would make sense to move it to MdeModulePkg (or MdePkg) and >>> rename it BaseCompilerIntrinsicsLib (it *is* a BASE library). >>> >>> As I alluded in my reply to Ray - x86 also have this problem, but to a lesser >>> extent, and ended up creating library functions to call instead of using plain C >>> for certain operations. >>> Which was probably the right decision when it was restricted to a very few >>> corner cases. >>> >>> / >>> Leif >>> >>> On Thu, Mar 12, 2020 at 13:24:30 +0000, Schaefer, Daniel (DualStudy) wrote: >>>> Hi Leif, >>>> >>>> you're right. If I revert my commit and include >>>> NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf >>>> without making any changes to it, the build succeeds. >>>> >>>> What do others think? (cc Michael, Pete, Andrew, Ard, who have made >>>> changes to this module) Is this a big hack or should we use it in RISC-V, too >>> and move the module to MdeModulePkg? >>>> Why isn't this a problem on x86? Was it fine on Itanium? >>>> >>>> - Daniel >>>> >>>> ________________________________ >>>> From: devel@edk2.groups.io >>>> on behalf of Leif >>>> Lindholm >>>> Sent: Thursday, March 12, 2020 11:55 >>>> To: devel@edk2.groups.io >>>> ; Schaefer, Daniel >>>> (DualStudy) >>> >>>> Cc: Chang, Abner (HPS SW/FW Technologist) >>>> ; Chen, Gilbert >>>> ; Dandan Bi >>>> ; Eric Dong >>>> >>>> Subject: Re: [edk2-devel] [PATCH v2 3/3] MdeModulePkg: Use CopyMem >>>> instead of GUID assignment >>>> >>>> Hi Daniel, >>>> >>>> There is nothing wrong with this patch that just went in (and I should >>>> have called out sooner if I wanted to stop it), but I think a better >>>> solution is to implement a RISC-V variant of >>>> ArmPkg/Library/CompilerIntrinsicsLib/. >>>> >>>> It is perfectly valid for the compiler to generate memcpy calls in >>>> response to struct operations that are perfectly valid C. >>>> >>>> In fact, we could consider moving the ArmPkg one over into >>>> MdeModulePkg. I have a feeling that including a >>>> NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf >>>> in your current build would be an alternative solution to your >>>> compilation error. >>>> >>>> / >>>> Leif >>>> >>>> On Mon, Mar 02, 2020 at 11:32:38 +0100, Daniel Schaefer wrote: >>>>> GCC translates a simple assignment to memcpy, which EDKII doesn't >>> provide. >>>>> See: >>>>> INVALID URI REMOVED >>> 2Darch >>>>> ive.com_edk2-2Ddevel- >>> 40lists.01.org_msg11928.html&d=DwIBAg&c=C5b8zRQ >>>>> >>> O1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3 >>> E&m=wjlf >>>>> >>> QYZsXfc5WWDFmywLOEYlCKvrpYrnaXMIpJcLK7U&s=426yv7VvgHTtgtYaR0f0X >>> NiaEA >>>>> LgqZkxektjywEwPco&e= >>>>> >>>>> REF:INVALID URI REMOVED >>>>> anocore.org_show-5Fbug.cgi-3Fid- >>> 3D2547&d=DwIBAg&c=C5b8zRQO1miGmBeVZ2 >>>>> >>> LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=wjlfQYZsX >>> fc5WWD >>>>> FmywLOEYlCKvrpYrnaXMIpJcLK7U&s=PjC_mIwh0GhNy5np2h7K8l4l- >>> pLLBWt1FRuLB >>>>> UbULpc&e= >>>>> >>>>> Signed-off-by: Daniel Schaefer >>>>> >>>>> Cc: Abner Chang >>> >>>>> Cc: Gilbert Chen >>> >>>>> Cc: Leif Lindholm >>>>> Cc: Dandan Bi >>>>> Cc: Eric Dong >>>>> --- >>>>> >>>>> Notes: >>>>> v2: >>>>> - Use CopyMem instead of CopyGuid [Dandan] >>>>> >>>>> MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git >>> a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>>> b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>>> index 5cc527679a78..0540e6fa8a44 100644 >>>>> --- a/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>>> +++ b/MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.c >>>>> @@ -619,7 +619,7 @@ CreateDeviceManagerForm( >>>>> TokenHelp = HiiSetString (HiiHandle, 0, String, NULL); >>>>> FreePool (String); >>>>> >>>>> - FormSetGuid = ((EFI_IFR_FORM_SET *)Ptr)->Guid; >>>>> + CopyMem (&FormSetGuid, &((EFI_IFR_FORM_SET *) Ptr)->Guid, >>>>> + sizeof (EFI_GUID)); >>>>> >>>>> // >>>>> // Network device process >>>>> -- >>>>> 2.25.0 >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >