From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A5C93208AE2D4 for ; Wed, 20 Feb 2019 01:44:33 -0800 (PST) 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 mx1.redhat.com (Postfix) with ESMTPS id 1C765C068BF5; Wed, 20 Feb 2019 09:44:33 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-220.rdu2.redhat.com [10.10.120.220]) by smtp.corp.redhat.com (Postfix) with ESMTP id 19530662D6; Wed, 20 Feb 2019 09:44:31 +0000 (UTC) To: Jordan Justen , edk2-devel@lists.01.org Cc: Anthony Perard , Peter Fang References: <20190218101015.23399-1-jordan.l.justen@intel.com> <155060636442.7367.9770376016776133854@jljusten-skl> From: Laszlo Ersek Message-ID: <569df551-8b8d-0543-2c47-021596e98109@redhat.com> Date: Wed, 20 Feb 2019 10:44:31 +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: <155060636442.7367.9770376016776133854@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 20 Feb 2019 09:44:33 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg/Sec: Clear the Cache Disable flag in the CR0 register X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Feb 2019 09:44:33 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/19/19 20:59, Jordan Justen wrote: > On 2019-02-18 05:23:28, Laszlo Ersek wrote: >> generic comment (applies to all NASM usage I guess): >> >> On 02/18/19 11:10, Jordan Justen wrote: >> >>> + mov eax, cr0 >>> + and eax, ~(1 << 30) >>> + mov cr0, eax >> >>> + mov rax, cr0 >>> + and eax, ~(1 << 30) >>> + mov cr0, rax >> >> I've read up on the << and ~ operators in the NASM documentation, and I >> think the above build-time calculations of the masks are well-defined >> and correct. >> >> - bit shifts are always unsigned >> - given bit position 30, ~(1 << 30) will be a value with 32 bits >> - bit-neg simply flips bits (one's complement) >> >> On the other hand, I find these NASM specifics counter-intuitive. The >> expression ~(1 << 30) looks like valid C, but in C, it means a quite >> different thing. > > Can you elaborate? I guess there might be something subtly different, > but for the most part it means the same thing, right? > >> I think calculating the mask with "strict dword" somehow (not exactly >> sure how) would make this more readable; > > Oh, are you saying that (1 << 30) doesn't necessarily mean we are > operating on a 32-bit value? I think the code is fine as-is, in the technical sense; it is just confusing/concerning to read for someone with a background in C. The expression *looks* like C, but doesn't *behave* like in C. Ultimately, the actual behavior is *right* for the assembly, but the code is still difficult to read. (To me anyway.) >> or else the BTR instruction would. > > Yeah, I guess this works. > >> Opinions? (Again, pertaining to all NASM usage in edk2.) > > As always, my opinion is to avoid writing assembly code. :) > > We actually had a version that set this just before the decompress in > SecMain.c. Then I noted that we were initializing temp-ram here, so I > moved it, even though the memory init doesn't take a significant > amount of time compared to the decompress. Maybe we should just do > that instead? I think I would prefer that, yes, if the performance on ACRN remains tolerable like that. In fact... can you identify ACRN through AsmCpuidEx() or similar, and restrict the CR0.CD manipulation to ACRN? Thanks, Laszlo