Re: [PATCH] x86: Always have CR4.PKE set in HVM context

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/04/2021 10:08, Jan Beulich wrote:
> On 30.04.2021 00:12, Andrew Cooper wrote:
>> The sole user of read_pkru() is the emulated pagewalk, and guarded behind
>> guest_pku_enabled() which restricts the path to HVM (hap, even) context only.
>>
>> The commentary in read_pkru() concerning _PAGE_GNTTAB overlapping with
>> _PAGE_PKEY_BITS is only applicable to PV guests.
>>
>> The context switch path, via write_ptbase() unconditionally writes CR4 on any
>> context switch.
>>
>> Therefore, we can guarantee to separate CR4.PKE between PV and HVM context at
>> no extra cost.  Set PKE in mmu_cr4_features on boot, so it becomes set in HVM
>> context, and clear it in pv_make_cr4().
>>
>> Rename read_pkru() to rdpkru() now that it is a simple wrapper around the
>> instruction.  This saves two CR4 writes on every pagewalk, which typically
>> occur more than one per emulation.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> The changes looks perfectly fine to me, but I still feel urged to make
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> conditional upon my "x86emul: support RDPKRU/WRPKRU" (submitted exactly
> half a year ago) going in first, unless there were to be review comments
> making extensive rework necessary. In the absence of such review
> feedback, I consider it pretty inappropriate for me to do rebasing work
> (no matter that this would be largely mechanical afaics) here for a
> patch which has been pending and effectively ignored for so long. (The
> main thing that immediately struck me as odd was "The sole user of
> read_pkru() is ...".)

So the observation about "sole user" occurred to me too, right after I
sent this.  Then I thought "surely Jan has spotted this and done a patch
for it".

Presumably you're talking about "x86emul: support RDPKRU/WRPKRU" from
Oct 30th ?  I found that via the archives, but I literally don't have a
copy in my inbox.


If I do the rebase, are you happy for this patch to stay as it is (so
the complicated change concerning context switching doesn't get more
complicated), and so we're not knowingly adding new constructs which
need immediate changes?

~Andrew