From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::243; helo=mail-wr0-x243.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x243.google.com (mail-wr0-x243.google.com [IPv6:2a00:1450:400c:c0c::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 11516223CCEF2 for ; Fri, 2 Feb 2018 05:22:58 -0800 (PST) Received: by mail-wr0-x243.google.com with SMTP id v15so22561404wrb.8 for ; Fri, 02 Feb 2018 05:28:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=nUYR7BNLv9J32Y/bmSBeI5INr02XPV7Pztt/GASf0Cw=; b=FmafLu1L9GVR2sfFNzYfzfZJW1xPj72m++377UzqLKUveDEKeyN4fNct6F+zZotIbj OBlRS83MipDaHu/1E0YVfCsq2bsDXHhsXsn8fzRclgJtaU4pj2DLSB4jBwNy2Pi2RdBQ 9J+OeV11PmGETTqZ+Vkyz7Eg07vzaMXg7B9CI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=nUYR7BNLv9J32Y/bmSBeI5INr02XPV7Pztt/GASf0Cw=; b=AmUK3j0fEzj5k8JYfGpYfdyntddui8WVsjNNnq28AA4hnxNERxY7lVz5Zsl52t7AbS 97yU/RSpB3ddN21mMljp/uZuHP5sFkktnlm+/S+rleaZ27iiJEZFJ5BFAoS87yNUYijt QcovmlDIsZ0VyoLvS1cbNnPcA7xQh9JcnctUWj8a5U1Y6nxTDUj/lCotoFiA7bJ2uzmF kgBpHoaJ0hrIJ62WqRxfAMFyTCc5xUQrUPYYexdCugujbzF7SjxAGZyrBbH6P2Di80Lo ec6V8afiU8uuq4QHxz5WkSCYC3HXTamwbLZs1qRU/UTlAhz5ZF8oZwMktpWxrMjUMLlE pB5A== X-Gm-Message-State: AKwxyteu3K18HiUmgE4qrmcfUrI6imQaPAkY3s+/Q5upqZX/H40OyLTS YciHKfmXefx2CnmTYcsZugu+HQ== X-Google-Smtp-Source: AH8x227M+lBb2vMiPY3LMIj0MRGu0tW+PCpRLlZpm1e7QS/ZdU0MRj3ynjtttybCG7t8fOA3vfk33g== X-Received: by 10.223.176.211 with SMTP id j19mr25232465wra.60.1517578115687; Fri, 02 Feb 2018 05:28:35 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id z41sm5499719wrb.50.2018.02.02.05.28.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 02 Feb 2018 05:28:34 -0800 (PST) Date: Fri, 2 Feb 2018 13:28:32 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: Laszlo Ersek , "Kinney, Michael D" , edk2-devel-01 , "Ni, Ruiyu" , Paolo Bonzini , "Yao, Jiewen" , "Dong, Eric" Message-ID: <20180202132832.h37jcf3ksi2sdnxl@bivouac.eciton.net> References: <20180130153348.31992-1-lersek@redhat.com> <20180130153348.31992-2-lersek@redhat.com> <31138ce7-0637-a755-ec57-e36ab812f259@redhat.com> <17c44add-ca8e-c346-8cc8-7e94b694a7e1@redhat.com> <352efa04-a5c3-af45-2da7-8e9e0043aee9@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Feb 2018 13:22:59 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Feb 02, 2018 at 10:06:07AM +0000, Ard Biesheuvel wrote: > On 31 January 2018 at 10:40, Laszlo Ersek wrote: > > On 01/30/18 23:25, Kinney, Michael D wrote: > >> Laszlo, > >> > >> I agree that the function is better than a macro. > >> > >> I thought of the alignment issues as well. CopyMem() > >> is a good solution. We could also consider > >> WriteUnalignedxx() functions in BaseLib. > > > > IMO, the WriteUnalignedxx functions are a bit pointless in the exact > > form they are declared (this was discussed earlier esp. with regard to > > aarch64). The functions take pointers to objects that already have the > > target type, such as > > > > UINT32 > > EFIAPI > > WriteUnaligned32 ( > > OUT UINT32 *Buffer, > > IN UINT32 Value > > ) > > > > Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise, > > the undefined behavior (due to mis-alignment) surfaces as soon as the > > function is called with an unaligned pointer (i.e. before the target > > area is actually written). > > > >> I was originally thinking this functionality would go > >> into BaseLib. But with the use of CopyMem(), we can't > >> do that. > > > > Can we put it in BaseMemoryLib instead (which is where CopyMem() is > > from)? That library class is still low-level enough. And, while I count > > 9 library instances, PatchAssembly() is not a large function, we could > > tolerate adding it to all 9 instances, identically. > > > > Let me also ask the opposite question: should we perhaps make the > > PatchAssembly() API *less* abstract? (Also suggested by your naming of > > the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64 > > doesn't lend itself to such patching (= expressed through the address > > right after the instruction), then even BaseMemoryLib may be too generic > > for the API. > > > >> Maybe we should use WriteUnalignedxx() and > >> add some ASSERT() checks. > >> > >> VOID > >> PatchAssembly ( > >> VOID *BufferEnd, > >> UINT64 PatchValue, > >> UINTN ValueSize > >> ) > >> { > >> ASSERT ((UINTN)BufferEnd > ValueSize); > >> switch (ValueSize) { > >> case 1: > >> ASSERT (PatchValue <= MAX_UINT8); > >> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue; > >> case 2: > >> ASSERT (PatchValue <= MAX_UINT16); > >> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue)); > >> break; > >> case 4: > >> ASSERT (PatchValue <= MAX_UINT32); > >> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue)); > >> break; > >> case 8: > >> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue)); > >> break; > >> default: > >> ASSERT (FALSE); > >> } > >> } > > > > In my opinion: > > > > - If Ard and Leif say that PatchAssembly() API makes sense for AARCH64, > > then I think we can go with the above generic implementation (for > > BaseLib). > > > > Code patching on ARM/AARCH64 has some hoops to jump through, i.e., > clean the D-cache to the point of unification, invalidate the I-cache, > probably some barriers in case the patching function happened to end > up in the same cache line as the patchee Not just the same cache line. Prefetching can happen whenever, for whatever reason. > (which may not be a concern > for this specific use case, but it does need to be taken into account > if this is turned into a patch-any-assembly-anywhere function) > > So if the PatchAssembly() prototype does end up in a generic library > class, we'd have to provide ARM and AARCH64 specific implementations > anyway, and given that I don't see any use for this on ARM/AARCH64 in > the first place, I think this should belong in an IA32/X64 specific > package. I also don't see a specific use for this on ARM* at the moment. But if this is going to become more widespread, it would be useful to introduce a higher-level layer with more portable semantics (I don't know RISC-V, but could imagine they require similar). However, at that point, we would probably want something buffer-oriented rather than instruction-oriented, since we'd like to keep the overhead down if writing more than one register's worth. / Leif