[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 30 Apr 2021 15:37:12 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=ehLA4x5CNAUoyPN7SOlV8lVmFAdHvptm88ghkQ8wJhQ=; b=RPE21E9NPxxpAW086DvA/1ZI3Do2PrGY81gJooEUXEm3kvNaRNT5FwiNwyEDDYltt5oA7/yyXOkSzCpCLZ8lYfcyaqAtttUqnwNAVlgfCMKnUdivc/FRuHQWcvhm8eLuURAiXdxUdB0XHHfyBYPp5CWxxqqt3iOgQg7jKxY7YWSu0H7L8CqjjRPccx76b/oloOvlt0+5zFLLIF1DNwSZ2/WIf+IU7KtwTiUt5BShyrqEw/383D05KTaPGQGFjpW0rozHYxEsVTesBm6v+r27iV9TD3OfICE/QNsLdPCIyjGUaV6mtlHBAfwthmQmBi6yNmOnQ/9F3saftQn4CgpNyw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KdaXs7meDncAoWT4N/Fuk82j9DDGdLwXj+X6bjfv2aSCsLpC0Hhr1udXFdgu8+KbbHHRlVTepUrRoBpHRm8sUaNtb3skVlnuSWFUnXLz8WpUvM8qcMl7zahiRNv6um0FoLzyvFPmZibNzlMzDvt6JkzlPVxPbng/bHoXqWsnggFQCMPqaQy60pZiM1jiul8jPr3DZo/Zj8+gYr+KE2AJpLXN630tj5MJUlE7uQTlp7GHWbbehD3AQ3bPVs8mkDAfidKBXhQndEpI2BKYSO5IBve/gBIEtx27nTu+OO+z8rXttQPh7W3OFQFc/+iefu9/weBxiQw1BkC4gcnER1aM9g==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 30 Apr 2021 14:37:37 +0000
  • Ironport-hdrordr: A9a23:lPZVWq4u8Vur5BoeaQPXwTOBI+orLtY04lQ7vn1ZYQBJc8Ceis CllOka0xixszoKRHQ8g7m7VZWoa3Xa6JJz/M0tLa6vNTOW31eAAaNDyc/ZwzPmEzDj7eI178 1dWoV3FdGYNzdHpOng5g3QKbgd6fmBtJulnOLPi0pqJDsaCJ1IyydcJkKlHlZtRA9AbKBJcK a0wsZcvTKvdTA2Q62AZkUtZOTIq93VmJ+OW3dvayIP0wWAgSil77T3CXGjr3UjeghC3Ks49i z9mxH5j5/Jj9iA1hTe22XPhq4m/efJ990rPq2xo/lQEBrAoEKCZINtW7qN1QpF3N2H2RIRv/ Tn5zslN8R3wXvNcm+yuguF4Xie7B8er0XM5HXdrXz/odHoZD9SMbs+uatpNiH3xmBlnNZg3L lF12iU3qAnfC/orWDGyPXjEzRJ/3DEx0YKoKoooFF0FbY6Uvt3q7cS+UtEea1wZh7S2cQcP8 RFSP3H6O0+SyLiU1np+lNB7faLRXoJEhKPUiE5y7Go+gkTpnx/wkcCrfZv5ksoxdY4Q5lA0e zOLr5lorFIVtMXdqJwHo46MLCKNlA=
  • Ironport-sdr: DZbleYGSfGGOvS4orvI060mpejXleVr7Pl4M2V3MkenXlgE1eANX5/ElKyW/K9yZnt+7IY3HZL N6rK7ldxbHkLfYC4Sz0kJPwK1c14iHOJ/0OSg2otyewTkLpz4lirs4mvoIQA0Z24++AO/8RHLm a9Gmw84wKfuvoyM/h7QKijvMo3jcAJ7Z5FKS/PRXCnGFKvcUDX0F3UJ3ZLbv8yEQi7SWqnKWn8 s5VaTm7j1mt20Ha5hKjGkjM9hzdkF4OIwQ4/XWeBNb7R+ftiJU4726UA2J3ywOM6Lc+XJaM785 hB8=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.