Re: [PATCH v1 1/2] x86: add p2m_ram_wp

["Jan Beulich" <JBeulich@suse.com>]

>>> On 28.07.14 at 19:55, <wei.ye@intel.com> wrote:
>  xen/arch/x86/hvm/hvm.c    |    8 +++++++-
>  xen/arch/x86/mm/p2m-ept.c |    1 +
>  xen/include/asm-x86/p2m.h |    8 +++++++-
>  3 files changed, 15 insertions(+), 2 deletions(-)

This can't be complete: At the very least p2m-pt.c also needs a change
similar to the one to p2m-ept.c.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2738,7 +2738,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>       * If this GFN is emulated MMIO or marked as read-only, pass the fault
>       * to the mmio handler.
>       */
> -    if ( (p2mt == p2m_mmio_dm) || 
> +    if ( (p2mt == p2m_mmio_dm) ||
> +         (p2mt == p2m_ram_wp) || 
>           (access_w && (p2mt == p2m_ram_ro)) )

Comparing your change with the surrounding existing code, you
would pass even reads happening to fault (perhaps for another
reason) to the DM, other than done for p2m_ram_ro. I don't think
that's correct.

> @@ -3829,6 +3830,11 @@ static enum hvm_copy_result __hvm_copy(
>              put_page(page);
>              return HVMCOPY_unhandleable;
>          }
> +        if ( p2m_is_wp_ram(p2mt) )
> +        {
> +            put_page(page);
> +            return HVMCOPY_bad_gfn_to_mfn;
> +        }

And again this change can't be complete: __hvm_clear() would also
need a similar change.

> @@ -167,6 +169,9 @@ typedef unsigned int p2m_query_t;
>   * and must not be touched. */
>  #define P2M_BROKEN_TYPES (p2m_to_mask(p2m_ram_broken))
>  
> +/* Write protection types */
> +#define P2M_WP_TYPES (p2m_to_mask(p2m_ram_wp))
> +
>  /* Useful predicates */
>  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
>  #define p2m_is_hole(_t) (p2m_to_mask(_t) & P2M_HOLE_TYPES)
> @@ -191,6 +196,7 @@ typedef unsigned int p2m_query_t;
>  #define p2m_is_any_ram(_t)  (p2m_to_mask(_t) &                   \
>                               (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
>                                p2m_to_mask(p2m_map_foreign)))
> +#define p2m_is_wp_ram(_t)   (p2m_to_mask(_t) & P2M_WP_TYPES)

To me such single-type classes don't seem very useful. Agreed
there are a number of pre-existing ones, but for classes where
future extensions are rather hard to imagine I wouldn't needlessly
add classes right away. But Tim (whom you failed to Cc anyway)
will have the final say here.

Jan