From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas Lengyel Subject: Re: [PATCH v2] xen/p2m: Fix p2m_flush_table for non-nested cases Date: Wed, 8 Feb 2017 14:42:46 -0700 Message-ID: References: <1486574546-10818-1-git-send-email-george.dunlap@citrix.com> <20170208172925.GC60904@deinos.phlegethon.org> <9e271c59-0d6c-8b53-1448-16afe64e3b2b@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8025172434985219626==" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cba0n-0005Zv-GH for xen-devel@lists.xenproject.org; Wed, 08 Feb 2017 21:42:49 +0000 Received: by mail-wm0-f66.google.com with SMTP id r18so223005wmd.3 for ; Wed, 08 Feb 2017 13:42:47 -0800 (PST) In-Reply-To: <9e271c59-0d6c-8b53-1448-16afe64e3b2b@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: George Dunlap Cc: Xen-devel , Tim Deegan , Jan Beulich , Andrew Cooper List-Id: xen-devel@lists.xenproject.org --===============8025172434985219626== Content-Type: multipart/alternative; boundary=001a113fb18ad8532305480bbe28 --001a113fb18ad8532305480bbe28 Content-Type: text/plain; charset=UTF-8 On Wed, Feb 8, 2017 at 11:12 AM, George Dunlap wrote: > On 08/02/17 17:29, Tim Deegan wrote: > > At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote: > >> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushing of > >> nested p2m tables whenever the host p2m table changed. Unfortunately > >> in the process, it added a filter to p2m_flush_table() function so > >> that the p2m would only be flushed if it was being used as a nested > >> p2m. This meant that the p2m was not being flushed at all for altp2m > >> callers. > >> > >> Only check np2m_base if p2m_class is set to p2m_nested. > >> > >> NB that this is not a security issue: The only time this codepath is > >> called is in cases where either nestedp2m or altp2m is enabled, and > >> neither of them are in security support. > >> > >> Reported-by: Matt Leinhos > >> Signed-off-by: George Dunlap > >> CC: Tamas K Lengyel > >> --- > >> xen/arch/x86/mm/p2m.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > >> index 6548e9f..0af2ec1 100644 > >> --- a/xen/arch/x86/mm/p2m.c > >> +++ b/xen/arch/x86/mm/p2m.c > >> @@ -1627,7 +1627,9 @@ p2m_flush_table(struct p2m_domain *p2m) > >> ASSERT(page_list_empty(&p2m->pod.super)); > >> ASSERT(page_list_empty(&p2m->pod.single)); > >> > >> - if ( p2m->np2m_base == P2M_BASE_EADDR ) > >> + /* No need to flush if it's already empty */ > >> + if ( p2m->p2m_class == p2m_nested && > >> + p2m->np2m_base == P2M_BASE_EADDR ) > > > > Looks like p2m_is_nestedp2m(p2m) is the usual idiom. Either way: > > > > Reviewed-by: Tim Deegan > > Right -- I'll change it to the usual idiom and check it in after I've > gotten a test report (or waited a reasonable amount of time). Tested-by: Tamas K Lengyel --001a113fb18ad8532305480bbe28 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Wed, Feb 8, 2017 at 11:12 AM, George Dunlap <george.dunlap@c= itrix.com> wrote:
On 08/02/17 17:29, Tim Deegan wrote:
> At 17:22 +0000 on 08 Feb (1486574546), George Dunlap wrote:
>> Commit 71bb7304e7a7a35ea6df4b0cedebc35028e4c159 added flushin= g of
>> nested p2m tables whenever the host p2m table changed.=C2=A0 Unfor= tunately
>> in the process, it added a filter to p2m_flush_table() function so=
>> that the p2m would only be flushed if it was being used as a neste= d
>> p2m.=C2=A0 This meant that the p2m was not being flushed at all fo= r altp2m
>> callers.
>>
>> Only check np2m_base if p2m_class is set to p2m_nested.
>>
>> NB that this is not a security issue: The only time this codepath = is
>> called is in cases where either nestedp2m or altp2m is enabled, an= d
>> neither of them are in security support.
>>
>> Reported-by: Matt Leinhos <m= att@starlab.io>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> ---
>>=C2=A0 xen/arch/x86/mm/p2m.c | 4 +++-
>>=C2=A0 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 6548e9f..0af2ec1 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1627,7 +1627,9 @@ p2m_flush_table(struct p2m_domain *p2m)
>>=C2=A0 =C2=A0 =C2=A0 ASSERT(page_list_empty(&p2m->pod.s= uper));
>>=C2=A0 =C2=A0 =C2=A0 ASSERT(page_list_empty(&p2m->pod.s= ingle));
>>
>> -=C2=A0 =C2=A0 if ( p2m->np2m_base =3D=3D P2M_BASE_EADDR )
>> +=C2=A0 =C2=A0 /* No need to flush if it's already empty */ >> +=C2=A0 =C2=A0 if ( p2m->p2m_class =3D=3D p2m_nested &&=
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p2m->np2m_base =3D=3D P2M_BA= SE_EADDR )
>
> Looks like p2m_is_nestedp2m(p2m) is the usual idiom.=C2=A0 Either way:=
>
> Reviewed-by: Tim Deegan <tim@xen.org= >

Right -- I'll change it to the usual idiom and check it in = after I've
gotten a test report (or waited a reasonable amount of time).
<= div>
Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

--001a113fb18ad8532305480bbe28-- --===============8025172434985219626== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============8025172434985219626==--