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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/04/2021 11:42, Jan Beulich wrote:
> On 30.04.2021 12:21, Andrew Cooper wrote:
>> 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.
> Odd. Was that then the time of your severe email issues, and were your
> IT folks not even able to make sure pending email got delivered
> afterwards (or at least enumerate what emails got discarded)? If I had
> got a reply saying the mail couldn't be delivered, I surely would have
> resent it.
>
> Just to be sure - you seem to have got a copy of "x86emul: de-duplicate
> scatters to the same linear address", as I seem to recall you responding
> there, albeit not with an ack or anything I could actually act upon (and
> this might have been in irc). That was sent just a few days later, and
> suffers a similar stall. And while in a ping I did say I would commit it
> without ack, I'm really hesitant to do so there. I've put it on next
> week's community meeting's agenda, just in case.

I do recall that patch.

>
>> 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?
> Well, the answer is not just "yes", but in reality I wouldn't mind
> doing the rebasing myself, if only I knew it wasn't for the purpose of
> waiting another half year for an ack (or otherwise).

The patch itself looks entirely fine.  Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>

The only observation I've got is that the other instructions in Grp7
probably want a blanket conversion from generate_exception_if(vex.pfx,
EXC_UD); to use the unimplemented_insn path instead, but that's clearly
further work.

I'll commit this patch, and the rebase delta on yours ought to just be
the naming of the helpers.

~Andrew