Re: [PATCH] xen: Restore p2m_access_t enum order to allow bitmask semantics

From: George Dunlap <george.dunlap@citrix.com>
To: Malcolm Crossley <malcolm.crossley@citrix.com>,
	george.dunlap@eu.citrix.com, keir@xen.org, jbeulich@suse.com,
	andrew.cooper3@citrix.com
Cc: "Lengyel, Tamas" <tlengyel@novetta.com>, xen-devel@lists.xen.org
Subject: Re: [PATCH] xen: Restore p2m_access_t enum order to allow bitmask semantics
Date: Wed, 9 Mar 2016 16:30:50 +0000	[thread overview]
Message-ID: <56E04FBA.2030404@citrix.com> (raw)
In-Reply-To: <1457451028-3808-1-git-send-email-malcolm.crossley@citrix.com>

On 08/03/16 15:30, Malcolm Crossley wrote:
> Nested hap code assumed implict bitmask semantics of the p2m_access_t
> enum prior to C/S 4c63692d7c38c5ac414fe97f8ef37b66e05abe5c
> 
> The change to the enum ordering broke this assumption and caused functional
> problems for the nested hap code. As it may be error prone to audit and find
> all other p2m_access users assuming bitmask semantics, instead restore the
> previous enum order and make it explict that bitmask semantics are to be
> preserved for the read, write and execute access types.
> 
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>

Looks good; but following up Jan's point, could you do a brief survey of
the places where the p2m_access values are used, and confirm that none
of them now implicitly assume that p2m_access_rwx is zero?

(Or Tamas, can you say that you're reasonably certain nothing has now
come to depend on the value of p2m_access_rwx being zero?)

Thanks,
 -George

> ---
>  xen/arch/x86/mm/hap/nested_hap.c |  2 +-
>  xen/include/xen/p2m-common.h     | 17 +++++++++--------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
> index 0dbae13..9cee5a0 100644
> --- a/xen/arch/x86/mm/hap/nested_hap.c
> +++ b/xen/arch/x86/mm/hap/nested_hap.c
> @@ -263,7 +263,7 @@ nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t *L2_gpa,
>  
>      switch ( p2ma_10 )
>      {
> -    case p2m_access_rwx ... p2m_access_n:
> +    case p2m_access_n ... p2m_access_rwx:
>          break;
>      case p2m_access_rx2rw:
>          p2ma_10 = p2m_access_rx;
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 8b70459..6374a5b 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -15,14 +15,15 @@
>   * default.
>   */
>  typedef enum {
> -    p2m_access_rwx   = 0, /* The default access type when not used. */
> -    p2m_access_wx    = 1,
> -    p2m_access_rx    = 2,
> -    p2m_access_x     = 3,
> -    p2m_access_rw    = 4,
> -    p2m_access_w     = 5,
> -    p2m_access_r     = 6,
> -    p2m_access_n     = 7, /* No access allowed. */
> +    /* Code uses bottom three bits with bitmask semantics */
> +    p2m_access_n     = 0, /* No access allowed. */
> +    p2m_access_r     = 1 << 0,
> +    p2m_access_w     = 1 << 1,
> +    p2m_access_x     = 1 << 2,
> +    p2m_access_rw    = p2m_access_r | p2m_access_w,
> +    p2m_access_rx    = p2m_access_r | p2m_access_x,
> +    p2m_access_wx    = p2m_access_w | p2m_access_x,
> +    p2m_access_rwx   = p2m_access_r | p2m_access_w | p2m_access_x,
>  
>      p2m_access_rx2rw = 8, /* Special: page goes from RX to RW on write */
>      p2m_access_n2rwx = 9, /* Special: page goes from N to RWX on access, *
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel