Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.11.14 at 08:59, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2838,7 +2838,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>       * to the mmio handler.
>       */
>      if ( (p2mt == p2m_mmio_dm) || 
> -         (npfec.write_access && (p2mt == p2m_ram_ro)) )
> +         (npfec.write_access &&
> +             ((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )

Why are you corrupting indentation here?

Furthermore the code you modify here suggests that p2m_ram_ro
already has the needed semantics - writes get passed to the DM.
None of the other changes you make, and none of the other uses
of p2m_ram_ro appear to be in conflict with your intentions, so
you'd really need to explain better why you need the new type.

> @@ -5922,6 +5923,8 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>                  a.mem_type =  HVMMEM_ram_rw;
>              else if ( p2m_is_grant(t) )
>                  a.mem_type =  HVMMEM_ram_rw;
> +            else if ( t == p2m_mmio_write_dm )
> +                a.mem_type = HVMMEM_mmio_write_dm;
>              else
>                  a.mem_type =  HVMMEM_mmio_dm;
>              rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> @@ -5941,7 +5944,8 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          static const p2m_type_t memtype[] = {
>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>              [HVMMEM_ram_ro]  = p2m_ram_ro,
> -            [HVMMEM_mmio_dm] = p2m_mmio_dm
> +            [HVMMEM_mmio_dm] = p2m_mmio_dm,
> +            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>          };
>  
>          if ( copy_from_guest(&a, arg, 1) )
> @@ -5987,14 +5991,17 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>                  rc = -EAGAIN;
>                  goto param_fail4;
>              }
> +

Stray addition of a blank line?

>              if ( !p2m_is_ram(t) &&
> -                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) )
> +                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
> +                 t != p2m_mmio_write_dm )

Do you really want to permit e.g. transitions between mmio_dm and
mmio_write_dm? We should be as restrictive as possible here to not
open up paths to security problems.

>              {
>                  put_gfn(d, pfn);
>                  goto param_fail4;
>              }
>  
>              rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
> +
>              put_gfn(d, pfn);
>              if ( rc )
>                  goto param_fail4;

Another stray newline addition.

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -72,6 +72,7 @@ typedef enum {
>      p2m_ram_shared = 12,          /* Shared or sharable memory */
>      p2m_ram_broken = 13,          /* Broken page, access cause domain crash 
> */
>      p2m_map_foreign  = 14,        /* ram pages from foreign domain */
> +    p2m_mmio_write_dm = 15,       /* Read-only; writes go to the device 
> model */
>  } p2m_type_t;
>  
>  /* Modifiers to the query */
> 

If the new type is really needed, shouldn't this get added to
P2M_RO_TYPES?

Jan


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