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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
