Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: George Dunlap <george.dunlap@citrix.com>, xen-devel@lists.xenproject.org
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Tamas K Lengyel <tamas.lengyel@zentific.com>,
	Tim Deegan <tim@xen.org>, Paul Durrant <paul.durrant@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Isaila Alexandru <aisaila@bitdefender.com>
Subject: Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
Date: Wed, 26 Sep 2018 18:22:56 +0100	[thread overview]
Message-ID: <6c2e77c5-fe94-3644-8b33-ac8785e4d065@citrix.com> (raw)
In-Reply-To: <20180926164741.13405-2-george.dunlap@citrix.com>

On 26/09/18 17:47, George Dunlap wrote:
> From: Isaila Alexandru <aisaila@bitdefender.com>
>
> This patch adds access control for NPT mode.
>
> There aren’t enough extra bits to store the access rights in the NPT p2m
> table, so we add a radix tree to store extra information.

I'm sorry to re-open this argument, but why?

ISTR there being some argument based on pagetable sharing with the
IOMMU, but that doesn't work at the moment and can't reasonably be made
to work.  For one, attempting to use pt sharing will break as soon as
you try and DMA to a mapped grant.

I'm disinclined to let a broken vestigial feature get in the way of real
improvements.

Beyond that, an NPT PTE has basically the same number of software
available bits as an EPT PTE.

Am I missing anything?

>
> For efficiency:
>  - Only allocate this radix tree when we first store "non-default"
>    extra information
>
>  - Remove entires which match the default extra information rather
>    than continuing to store them
>
>  - For superpages, only store an entry for the first gfn in the
>    superpage.  Use the order of the p2m entry being read to determine
>    the proper place to look in the radix table.
>
> Modify p2m_type_to_flags() to accept and interpret an access value,
> parallel to the ept code.
>
> Add a set_default_access() method to the p2m-pt and p2m-ept versions
> of the p2m rather than setting it directly, to deal with different
> default permitted access values.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> NB, this is compile-tested only.
>
> cc'ing Paul because this is functionality he may want at some point in
> the future.
>
> I'm not sure why we only allow 'int' to be stored in the radix tree,
> but that throws away 30-some bits we could otherwise use.  We might
> consider revising this if we run out of bits here.

Probably because we have a old fork of the Linux radix tree
functionality, rather than any specific reason to waste 30-some bits.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel