[v3,1/3] x86/ioreq server(patch for 4.7): Rename p2m_mmio_write_dm to p2m_ioreq_server.

Commit Message

Yu Zhang April 25, 2016, 10:35 a.m. UTC

Previously p2m type p2m_mmio_write_dm was introduced for write-
protected memory pages whose write operations are supposed to be
forwarded to and emulated by an ioreq server. Yet limitations of
rangeset restrict the number of guest pages to be write-protected.

This patch replaces the p2m type p2m_mmio_write_dm with a new name:
p2m_ioreq_server, which means this p2m type can be claimed by one
ioreq server, instead of being tracked inside the rangeset of ioreq
server. Patches following up will add the related hvmop handling
code which map/unmap type p2m_ioreq_server to/from an ioreq server.

changes in v3:
  - According to Jan & George's comments, keep HVMMEM_mmio_write_dm
    for old xen interface versions, and replace it with HVMMEM_unused
    for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
    enum - HVMMEM_ioreq_server is introduced for the get/set mem type
    interfaces;
  - Add George's Reviewed-by and Acked-by from Tim & Andrew.

changes in v2:
  - According to George Dunlap's comments, only rename the p2m type,
    with no behavior changes.

Signed-off-by: Paul Durrant <[e-mail]>
Signed-off-by: Yu Zhang <[e-mail]>
Acked-by: Tim Deegan <[e-mail]>
Acked-by: Andrew Cooper <[e-mail]>
Reviewed-by: George Dunlap <[e-mail]>
Cc: Keir Fraser <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: Jun Nakajima <[e-mail]>
Cc: Kevin Tian <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Tim Deegan <[e-mail]>
---
 xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
 xen/arch/x86/mm/p2m-ept.c       |  2 +-
 xen/arch/x86/mm/p2m-pt.c        |  2 +-
 xen/arch/x86/mm/shadow/multi.c  |  2 +-
 xen/include/asm-x86/p2m.h       |  4 ++--
 xen/include/public/hvm/hvm_op.h |  8 +++++++-
 6 files changed, 20 insertions(+), 12 deletions(-)

Comments

Jan Beulich April 25, 2016, 12:12 p.m. UTC | #1

>>> On 25.04.16 at 12:35, <[e-mail]> wrote:
> Previously p2m type p2m_mmio_write_dm was introduced for write-
> protected memory pages whose write operations are supposed to be
> forwarded to and emulated by an ioreq server. Yet limitations of
> rangeset restrict the number of guest pages to be write-protected.
> 
> This patch replaces the p2m type p2m_mmio_write_dm with a new name:
> p2m_ioreq_server, which means this p2m type can be claimed by one
> ioreq server, instead of being tracked inside the rangeset of ioreq
> server. Patches following up will add the related hvmop handling
> code which map/unmap type p2m_ioreq_server to/from an ioreq server.
> 
> changes in v3:
>   - According to Jan & George's comments, keep HVMMEM_mmio_write_dm
>     for old xen interface versions, and replace it with HVMMEM_unused
>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>     enum - HVMMEM_ioreq_server is introduced for the get/set mem type
>     interfaces;
>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
> 
> changes in v2:
>   - According to George Dunlap's comments, only rename the p2m type,
>     with no behavior changes.
> 
> Signed-off-by: Paul Durrant <[e-mail]>
> Signed-off-by: Yu Zhang <[e-mail]>
> Acked-by: Tim Deegan <[e-mail]>
> Acked-by: Andrew Cooper <[e-mail]>
> Reviewed-by: George Dunlap <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>

I hope the other three tags above are still being considered
applicable by their originators.

Jan

Wei Liu April 25, 2016, 1:30 p.m. UTC | #2

On Mon, Apr 25, 2016 at 06:12:34AM -0600, Jan Beulich wrote:
> >>> On 25.04.16 at 12:35, <[e-mail]> wrote:
> > Previously p2m type p2m_mmio_write_dm was introduced for write-
> > protected memory pages whose write operations are supposed to be
> > forwarded to and emulated by an ioreq server. Yet limitations of
> > rangeset restrict the number of guest pages to be write-protected.
> > 
> > This patch replaces the p2m type p2m_mmio_write_dm with a new name:
> > p2m_ioreq_server, which means this p2m type can be claimed by one
> > ioreq server, instead of being tracked inside the rangeset of ioreq
> > server. Patches following up will add the related hvmop handling
> > code which map/unmap type p2m_ioreq_server to/from an ioreq server.
> > 
> > changes in v3:
> >   - According to Jan & George's comments, keep HVMMEM_mmio_write_dm
> >     for old xen interface versions, and replace it with HVMMEM_unused
> >     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
> >     enum - HVMMEM_ioreq_server is introduced for the get/set mem type
> >     interfaces;
> >   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
> > 
> > changes in v2:
> >   - According to George Dunlap's comments, only rename the p2m type,
> >     with no behavior changes.
> > 
> > Signed-off-by: Paul Durrant <[e-mail]>
> > Signed-off-by: Yu Zhang <[e-mail]>
> > Acked-by: Tim Deegan <[e-mail]>
> > Acked-by: Andrew Cooper <[e-mail]>
> > Reviewed-by: George Dunlap <[e-mail]>
> 
> Reviewed-by: Jan Beulich <[e-mail]>
> 
> I hope the other three tags above are still being considered
> applicable by their originators.
> 

Subject to confirmation from the originators:

Release-acked-by: Wei Liu <[e-mail]>

> Jan
>

George Dunlap April 25, 2016, 1:39 p.m. UTC | #3

On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang <[e-mail]> wrote:
> Previously p2m type p2m_mmio_write_dm was introduced for write-
> protected memory pages whose write operations are supposed to be
> forwarded to and emulated by an ioreq server. Yet limitations of
> rangeset restrict the number of guest pages to be write-protected.
>
> This patch replaces the p2m type p2m_mmio_write_dm with a new name:
> p2m_ioreq_server, which means this p2m type can be claimed by one
> ioreq server, instead of being tracked inside the rangeset of ioreq
> server. Patches following up will add the related hvmop handling
> code which map/unmap type p2m_ioreq_server to/from an ioreq server.
>
> changes in v3:
>   - According to Jan & George's comments, keep HVMMEM_mmio_write_dm
>     for old xen interface versions, and replace it with HVMMEM_unused
>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>     enum - HVMMEM_ioreq_server is introduced for the get/set mem type
>     interfaces;
>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.

Unfortunately these rather contradict each other -- I consider
Reviewed-by to only stick if the code I've specified hasn't changed
(or has only changed trivially).

Also...

>
> changes in v2:
>   - According to George Dunlap's comments, only rename the p2m type,
>     with no behavior changes.
>
> Signed-off-by: Paul Durrant <[e-mail]>
> Signed-off-by: Yu Zhang <[e-mail]>
> Acked-by: Tim Deegan <[e-mail]>
> Acked-by: Andrew Cooper <[e-mail]>
> Reviewed-by: George Dunlap <[e-mail]>
> Cc: Keir Fraser <[e-mail]>
> Cc: Jan Beulich <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>
> Cc: Jun Nakajima <[e-mail]>
> Cc: Kevin Tian <[e-mail]>
> Cc: George Dunlap <[e-mail]>
> Cc: Tim Deegan <[e-mail]>
> ---
>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>  xen/include/asm-x86/p2m.h       |  4 ++--
>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>  6 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f24126d..874cb0f 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>       */
>      if ( (p2mt == p2m_mmio_dm) ||
>           (npfec.write_access &&
> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>      {
>          __put_gfn(p2m, gfn);
>          if ( ap2m_active )
> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              get_gfn_query_unlocked(d, a.pfn, &t);
>              if ( p2m_is_mmio(t) )
>                  a.mem_type =  HVMMEM_mmio_dm;
> -            else if ( t == p2m_mmio_write_dm )
> -                a.mem_type = HVMMEM_mmio_write_dm;
> +            else if ( t == p2m_ioreq_server )
> +                a.mem_type = HVMMEM_ioreq_server;
>              else if ( p2m_is_readonly(t) )
>                  a.mem_type =  HVMMEM_ram_ro;
>              else if ( p2m_is_ram(t) )
> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
> +            [HVMMEM_unused] = p2m_invalid,
> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>          };
>
>          if ( copy_from_guest(&a, arg, 1) )
> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto setmemtype_fail;
>
> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>              goto setmemtype_fail;
>
>          while ( a.nr > start_iter )
> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              }
>              if ( !p2m_is_ram(t) &&
>                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
> -                 (t != p2m_mmio_write_dm || a.hvmmem_type != HVMMEM_ram_rw) )
> +                 (t != p2m_ioreq_server || a.hvmmem_type != HVMMEM_ram_rw) )
>              {
>                  put_gfn(d, pfn);
>                  goto setmemtype_fail;
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 3cb6868..380ec25 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>              break;
>          case p2m_grant_map_ro:
> -        case p2m_mmio_write_dm:
> +        case p2m_ioreq_server:
>              entry->r = 1;
>              entry->w = entry->x = 0;
>              entry->a = !!cpu_has_vmx_ept_ad;
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 3d80612..eabd2e3 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -94,7 +94,7 @@ static unsigned long p2m_type_to_flags(p2m_type_t t, mfn_t mfn,
>      default:
>          return flags | _PAGE_NX_BIT;
>      case p2m_grant_map_ro:
> -    case p2m_mmio_write_dm:
> +    case p2m_ioreq_server:
>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>      case p2m_ram_ro:
>      case p2m_ram_logdirty:
> diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
> index e5c8499..c81302a 100644
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>
>      /* Need to hand off device-model MMIO to the device model */
>      if ( p2mt == p2m_mmio_dm
> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>      {
>          gpa = guest_walk_to_gpa(&gw);
>          goto mmio;
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 5392eb0..ee2ea9c 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -71,7 +71,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_ioreq_server = 15,
>  } p2m_type_t;
>
>  /* Modifiers to the query */
> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>                        | p2m_to_mask(p2m_ram_ro)         \
>                        | p2m_to_mask(p2m_grant_map_ro)   \
>                        | p2m_to_mask(p2m_ram_shared)     \
> -                      | p2m_to_mask(p2m_mmio_write_dm))
> +                      | p2m_to_mask(p2m_ioreq_server))
>
>  /* Write-discard types, which should discard the write operations */
>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 1606185..b3e45cf 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -83,7 +83,13 @@ typedef enum {
>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>      HVMMEM_mmio_dm,            /* Reads and write go to the device model */
> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device model */
> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device model */
> +#else
> +    HVMMEM_unused,             /* Placeholder; setting memory to this type
> +                                  will fail for code after 4.7.0 */
> +#endif
> +    HVMMEM_ioreq_server

Also, I don't think we've had a convincing argument for why this patch
needs to be in 4.7.  The p2m name changes are internal only, and so
don't need to be made at all; and the old functionality will work as
well as it ever did.  Furthermore, the whole reason we're in this
situation is that we checked in a publicly-visible change to the
interface before it was properly ready; I think we should avoid making
the same mistake this time.

So personally I'd just leave this patch entirely for 4.8; but if Paul
and/or Jan have strong opinions, then I would say check in only a
patch to do the #if/#else/#endif, and leave both the p2m type changes
and the new HVMMEM_ioreq_server enum for when the 4.8 development
window opens.

 -George

Paul Durrant April 25, 2016, 2:01 p.m. UTC | #4

> -----Original Message-----

> From: [e-mail] [mailto:[e-mail]] On Behalf Of

> George Dunlap

> Sent: 25 April 2016 14:39

> To: Yu Zhang

> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;

> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan Beulich; Wei Liu

> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> 

> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang <[e-mail]>

> wrote:

> > Previously p2m type p2m_mmio_write_dm was introduced for write-

> > protected memory pages whose write operations are supposed to be

> > forwarded to and emulated by an ioreq server. Yet limitations of

> > rangeset restrict the number of guest pages to be write-protected.

> >

> > This patch replaces the p2m type p2m_mmio_write_dm with a new name:

> > p2m_ioreq_server, which means this p2m type can be claimed by one

> > ioreq server, instead of being tracked inside the rangeset of ioreq

> > server. Patches following up will add the related hvmop handling

> > code which map/unmap type p2m_ioreq_server to/from an ioreq server.

> >

> > changes in v3:

> >   - According to Jan & George's comments, keep

> HVMMEM_mmio_write_dm

> >     for old xen interface versions, and replace it with HVMMEM_unused

> >     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new

> >     enum - HVMMEM_ioreq_server is introduced for the get/set mem type

> >     interfaces;

> >   - Add George's Reviewed-by and Acked-by from Tim & Andrew.

> 

> Unfortunately these rather contradict each other -- I consider

> Reviewed-by to only stick if the code I've specified hasn't changed

> (or has only changed trivially).

> 

> Also...

> 

> >

> > changes in v2:

> >   - According to George Dunlap's comments, only rename the p2m type,

> >     with no behavior changes.

> >

> > Signed-off-by: Paul Durrant <[e-mail]>

> > Signed-off-by: Yu Zhang <[e-mail]>

> > Acked-by: Tim Deegan <[e-mail]>

> > Acked-by: Andrew Cooper <[e-mail]>

> > Reviewed-by: George Dunlap <[e-mail]>

> > Cc: Keir Fraser <[e-mail]>

> > Cc: Jan Beulich <[e-mail]>

> > Cc: Andrew Cooper <[e-mail]>

> > Cc: Jun Nakajima <[e-mail]>

> > Cc: Kevin Tian <[e-mail]>

> > Cc: George Dunlap <[e-mail]>

> > Cc: Tim Deegan <[e-mail]>

> > ---

> >  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------

> >  xen/arch/x86/mm/p2m-ept.c       |  2 +-

> >  xen/arch/x86/mm/p2m-pt.c        |  2 +-

> >  xen/arch/x86/mm/shadow/multi.c  |  2 +-

> >  xen/include/asm-x86/p2m.h       |  4 ++--

> >  xen/include/public/hvm/hvm_op.h |  8 +++++++-

> >  6 files changed, 20 insertions(+), 12 deletions(-)

> >

> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c

> > index f24126d..874cb0f 100644

> > --- a/xen/arch/x86/hvm/hvm.c

> > +++ b/xen/arch/x86/hvm/hvm.c

> > @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,

> unsigned long gla,

> >       */

> >      if ( (p2mt == p2m_mmio_dm) ||

> >           (npfec.write_access &&

> > -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )

> > +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )

> >      {

> >          __put_gfn(p2m, gfn);

> >          if ( ap2m_active )

> > @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,

> XEN_GUEST_HANDLE_PARAM(void) arg)

> >              get_gfn_query_unlocked(d, a.pfn, &t);

> >              if ( p2m_is_mmio(t) )

> >                  a.mem_type =  HVMMEM_mmio_dm;

> > -            else if ( t == p2m_mmio_write_dm )

> > -                a.mem_type = HVMMEM_mmio_write_dm;

> > +            else if ( t == p2m_ioreq_server )

> > +                a.mem_type = HVMMEM_ioreq_server;

> >              else if ( p2m_is_readonly(t) )

> >                  a.mem_type =  HVMMEM_ram_ro;

> >              else if ( p2m_is_ram(t) )

> > @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,

> XEN_GUEST_HANDLE_PARAM(void) arg)

> >              [HVMMEM_ram_rw]  = p2m_ram_rw,

> >              [HVMMEM_ram_ro]  = p2m_ram_ro,

> >              [HVMMEM_mmio_dm] = p2m_mmio_dm,

> > -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm

> > +            [HVMMEM_unused] = p2m_invalid,

> > +            [HVMMEM_ioreq_server] = p2m_ioreq_server

> >          };

> >

> >          if ( copy_from_guest(&a, arg, 1) )

> > @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,

> XEN_GUEST_HANDLE_PARAM(void) arg)

> >               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )

> >              goto setmemtype_fail;

> >

> > -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )

> > +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||

> > +             unlikely(a.hvmmem_type == HVMMEM_unused) )

> >              goto setmemtype_fail;

> >

> >          while ( a.nr > start_iter )

> > @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,

> XEN_GUEST_HANDLE_PARAM(void) arg)

> >              }

> >              if ( !p2m_is_ram(t) &&

> >                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm)

> &&

> > -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=

> HVMMEM_ram_rw) )

> > +                 (t != p2m_ioreq_server || a.hvmmem_type !=

> HVMMEM_ram_rw) )

> >              {

> >                  put_gfn(d, pfn);

> >                  goto setmemtype_fail;

> > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c

> > index 3cb6868..380ec25 100644

> > --- a/xen/arch/x86/mm/p2m-ept.c

> > +++ b/xen/arch/x86/mm/p2m-ept.c

> > @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct

> p2m_domain *p2m, ept_entry_t *entry,

> >              entry->a = entry->d = !!cpu_has_vmx_ept_ad;

> >              break;

> >          case p2m_grant_map_ro:

> > -        case p2m_mmio_write_dm:

> > +        case p2m_ioreq_server:

> >              entry->r = 1;

> >              entry->w = entry->x = 0;

> >              entry->a = !!cpu_has_vmx_ept_ad;

> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c

> > index 3d80612..eabd2e3 100644

> > --- a/xen/arch/x86/mm/p2m-pt.c

> > +++ b/xen/arch/x86/mm/p2m-pt.c

> > @@ -94,7 +94,7 @@ static unsigned long p2m_type_to_flags(p2m_type_t

> t, mfn_t mfn,

> >      default:

> >          return flags | _PAGE_NX_BIT;

> >      case p2m_grant_map_ro:

> > -    case p2m_mmio_write_dm:

> > +    case p2m_ioreq_server:

> >          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;

> >      case p2m_ram_ro:

> >      case p2m_ram_logdirty:

> > diff --git a/xen/arch/x86/mm/shadow/multi.c

> b/xen/arch/x86/mm/shadow/multi.c

> > index e5c8499..c81302a 100644

> > --- a/xen/arch/x86/mm/shadow/multi.c

> > +++ b/xen/arch/x86/mm/shadow/multi.c

> > @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,

> >

> >      /* Need to hand off device-model MMIO to the device model */

> >      if ( p2mt == p2m_mmio_dm

> > -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )

> > +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )

> >      {

> >          gpa = guest_walk_to_gpa(&gw);

> >          goto mmio;

> > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h

> > index 5392eb0..ee2ea9c 100644

> > --- a/xen/include/asm-x86/p2m.h

> > +++ b/xen/include/asm-x86/p2m.h

> > @@ -71,7 +71,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_ioreq_server = 15,

> >  } p2m_type_t;

> >

> >  /* Modifiers to the query */

> > @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;

> >                        | p2m_to_mask(p2m_ram_ro)         \

> >                        | p2m_to_mask(p2m_grant_map_ro)   \

> >                        | p2m_to_mask(p2m_ram_shared)     \

> > -                      | p2m_to_mask(p2m_mmio_write_dm))

> > +                      | p2m_to_mask(p2m_ioreq_server))

> >

> >  /* Write-discard types, which should discard the write operations */

> >  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \

> > diff --git a/xen/include/public/hvm/hvm_op.h

> b/xen/include/public/hvm/hvm_op.h

> > index 1606185..b3e45cf 100644

> > --- a/xen/include/public/hvm/hvm_op.h

> > +++ b/xen/include/public/hvm/hvm_op.h

> > @@ -83,7 +83,13 @@ typedef enum {

> >      HVMMEM_ram_rw,             /* Normal read/write guest RAM */

> >      HVMMEM_ram_ro,             /* Read-only; writes are discarded */

> >      HVMMEM_mmio_dm,            /* Reads and write go to the device model

> */

> > -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device

> model */

> > +#if __XEN_INTERFACE_VERSION__ < 0x00040700

> > +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device

> model */

> > +#else

> > +    HVMMEM_unused,             /* Placeholder; setting memory to this type

> > +                                  will fail for code after 4.7.0 */

> > +#endif

> > +    HVMMEM_ioreq_server

> 

> Also, I don't think we've had a convincing argument for why this patch

> needs to be in 4.7.  The p2m name changes are internal only, and so

> don't need to be made at all; and the old functionality will work as

> well as it ever did.  Furthermore, the whole reason we're in this

> situation is that we checked in a publicly-visible change to the

> interface before it was properly ready; I think we should avoid making

> the same mistake this time.

> 

> So personally I'd just leave this patch entirely for 4.8; but if Paul

> and/or Jan have strong opinions, then I would say check in only a

> patch to do the #if/#else/#endif, and leave both the p2m type changes

> and the new HVMMEM_ioreq_server enum for when the 4.8 development

> window opens.

> 


If the whole series is going in then I think this patch is ok. If this the only patch that is going in for 4.7 then I thing we need the patch to hvm_op.h to deprecate the old type and that's it. We definitely should not introduce an implementation of the type HVMMEM_ioreq_server that has the same hardcoded semantics as the old type and then change it.
The p2m type changes are also wrong. That type needs to be left alone, presumably, so that anything using HVMMEM_mmio_write_dm and compiled to the old interface version continues to function. I think HVMMEM_ioreq_server needs to map to a new p2m type which should be introduced in patch #3.

  Paul


>  -George

Jan Beulich April 25, 2016, 2:14 p.m. UTC | #5

>>> On 25.04.16 at 15:39, <[e-mail]> wrote:
> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang <[e-mail]> wrote:
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -83,7 +83,13 @@ typedef enum {
>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>>      HVMMEM_mmio_dm,            /* Reads and write go to the device model */
>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device model */
>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device model */
>> +#else
>> +    HVMMEM_unused,             /* Placeholder; setting memory to this type
>> +                                  will fail for code after 4.7.0 */
>> +#endif
>> +    HVMMEM_ioreq_server
> 
> Also, I don't think we've had a convincing argument for why this patch
> needs to be in 4.7.  The p2m name changes are internal only, and so
> don't need to be made at all; and the old functionality will work as
> well as it ever did.  Furthermore, the whole reason we're in this
> situation is that we checked in a publicly-visible change to the
> interface before it was properly ready; I think we should avoid making
> the same mistake this time.

Good point.

> So personally I'd just leave this patch entirely for 4.8; but if Paul
> and/or Jan have strong opinions, then I would say check in only a
> patch to do the #if/#else/#endif, and leave both the p2m type changes
> and the new HVMMEM_ioreq_server enum for when the 4.8 development
> window opens.

Doing that alone would break the build - it would need to be a
little more than that.

Jan

George Dunlap April 25, 2016, 2:15 p.m. UTC | #6

On 25/04/16 15:01, Paul Durrant wrote:
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>> George Dunlap
>> Sent: 25 April 2016 14:39
>> To: Yu Zhang
>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan Beulich; Wei Liu
>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>
>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang <[e-mail]>
>> wrote:
>>> Previously p2m type p2m_mmio_write_dm was introduced for write-
>>> protected memory pages whose write operations are supposed to be
>>> forwarded to and emulated by an ioreq server. Yet limitations of
>>> rangeset restrict the number of guest pages to be write-protected.
>>>
>>> This patch replaces the p2m type p2m_mmio_write_dm with a new name:
>>> p2m_ioreq_server, which means this p2m type can be claimed by one
>>> ioreq server, instead of being tracked inside the rangeset of ioreq
>>> server. Patches following up will add the related hvmop handling
>>> code which map/unmap type p2m_ioreq_server to/from an ioreq server.
>>>
>>> changes in v3:
>>>   - According to Jan & George's comments, keep
>> HVMMEM_mmio_write_dm
>>>     for old xen interface versions, and replace it with HVMMEM_unused
>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem type
>>>     interfaces;
>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>>
>> Unfortunately these rather contradict each other -- I consider
>> Reviewed-by to only stick if the code I've specified hasn't changed
>> (or has only changed trivially).
>>
>> Also...
>>
>>>
>>> changes in v2:
>>>   - According to George Dunlap's comments, only rename the p2m type,
>>>     with no behavior changes.
>>>
>>> Signed-off-by: Paul Durrant <[e-mail]>
>>> Signed-off-by: Yu Zhang <[e-mail]>
>>> Acked-by: Tim Deegan <[e-mail]>
>>> Acked-by: Andrew Cooper <[e-mail]>
>>> Reviewed-by: George Dunlap <[e-mail]>
>>> Cc: Keir Fraser <[e-mail]>
>>> Cc: Jan Beulich <[e-mail]>
>>> Cc: Andrew Cooper <[e-mail]>
>>> Cc: Jun Nakajima <[e-mail]>
>>> Cc: Kevin Tian <[e-mail]>
>>> Cc: George Dunlap <[e-mail]>
>>> Cc: Tim Deegan <[e-mail]>
>>> ---
>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>>>  xen/include/asm-x86/p2m.h       |  4 ++--
>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>>>  6 files changed, 20 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index f24126d..874cb0f 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>> unsigned long gla,
>>>       */
>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>           (npfec.write_access &&
>>> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
>>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>>>      {
>>>          __put_gfn(p2m, gfn);
>>>          if ( ap2m_active )
>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              get_gfn_query_unlocked(d, a.pfn, &t);
>>>              if ( p2m_is_mmio(t) )
>>>                  a.mem_type =  HVMMEM_mmio_dm;
>>> -            else if ( t == p2m_mmio_write_dm )
>>> -                a.mem_type = HVMMEM_mmio_write_dm;
>>> +            else if ( t == p2m_ioreq_server )
>>> +                a.mem_type = HVMMEM_ioreq_server;
>>>              else if ( p2m_is_readonly(t) )
>>>                  a.mem_type =  HVMMEM_ram_ro;
>>>              else if ( p2m_is_ram(t) )
>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>> +            [HVMMEM_unused] = p2m_invalid,
>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>>>          };
>>>
>>>          if ( copy_from_guest(&a, arg, 1) )
>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>>              goto setmemtype_fail;
>>>
>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>>>              goto setmemtype_fail;
>>>
>>>          while ( a.nr > start_iter )
>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              }
>>>              if ( !p2m_is_ram(t) &&
>>>                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm)
>> &&
>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>> HVMMEM_ram_rw) )
>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>> HVMMEM_ram_rw) )
>>>              {
>>>                  put_gfn(d, pfn);
>>>                  goto setmemtype_fail;
>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>>> index 3cb6868..380ec25 100644
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>> p2m_domain *p2m, ept_entry_t *entry,
>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>>              break;
>>>          case p2m_grant_map_ro:
>>> -        case p2m_mmio_write_dm:
>>> +        case p2m_ioreq_server:
>>>              entry->r = 1;
>>>              entry->w = entry->x = 0;
>>>              entry->a = !!cpu_has_vmx_ept_ad;
>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>> index 3d80612..eabd2e3 100644
>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>> @@ -94,7 +94,7 @@ static unsigned long p2m_type_to_flags(p2m_type_t
>> t, mfn_t mfn,
>>>      default:
>>>          return flags | _PAGE_NX_BIT;
>>>      case p2m_grant_map_ro:
>>> -    case p2m_mmio_write_dm:
>>> +    case p2m_ioreq_server:
>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>      case p2m_ram_ro:
>>>      case p2m_ram_logdirty:
>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>> b/xen/arch/x86/mm/shadow/multi.c
>>> index e5c8499..c81302a 100644
>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>>
>>>      /* Need to hand off device-model MMIO to the device model */
>>>      if ( p2mt == p2m_mmio_dm
>>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>>>      {
>>>          gpa = guest_walk_to_gpa(&gw);
>>>          goto mmio;
>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>> index 5392eb0..ee2ea9c 100644
>>> --- a/xen/include/asm-x86/p2m.h
>>> +++ b/xen/include/asm-x86/p2m.h
>>> @@ -71,7 +71,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_ioreq_server = 15,
>>>  } p2m_type_t;
>>>
>>>  /* Modifiers to the query */
>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>>>                        | p2m_to_mask(p2m_ram_ro)         \
>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>>>                        | p2m_to_mask(p2m_ram_shared)     \
>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>>> +                      | p2m_to_mask(p2m_ioreq_server))
>>>
>>>  /* Write-discard types, which should discard the write operations */
>>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
>>> diff --git a/xen/include/public/hvm/hvm_op.h
>> b/xen/include/public/hvm/hvm_op.h
>>> index 1606185..b3e45cf 100644
>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>> @@ -83,7 +83,13 @@ typedef enum {
>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device model
>> */
>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
>> model */
>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device
>> model */
>>> +#else
>>> +    HVMMEM_unused,             /* Placeholder; setting memory to this type
>>> +                                  will fail for code after 4.7.0 */
>>> +#endif
>>> +    HVMMEM_ioreq_server
>>
>> Also, I don't think we've had a convincing argument for why this patch
>> needs to be in 4.7.  The p2m name changes are internal only, and so
>> don't need to be made at all; and the old functionality will work as
>> well as it ever did.  Furthermore, the whole reason we're in this
>> situation is that we checked in a publicly-visible change to the
>> interface before it was properly ready; I think we should avoid making
>> the same mistake this time.
>>
>> So personally I'd just leave this patch entirely for 4.8; but if Paul
>> and/or Jan have strong opinions, then I would say check in only a
>> patch to do the #if/#else/#endif, and leave both the p2m type changes
>> and the new HVMMEM_ioreq_server enum for when the 4.8 development
>> window opens.
>>
> 
> If the whole series is going in then I think this patch is ok. If this the only patch that is going in for 4.7 then I thing we need the patch to hvm_op.h to deprecate the old type and that's it. We definitely should not introduce an implementation of the type HVMMEM_ioreq_server that has the same hardcoded semantics as the old type and then change it.
> The p2m type changes are also wrong. That type needs to be left alone, presumably, so that anything using HVMMEM_mmio_write_dm and compiled to the old interface version continues to function. I think HVMMEM_ioreq_server needs to map to a new p2m type which should be introduced in patch #3.

Well yes, if the whole series is going in the patch is OK; but I assumed
that since it's a new feature that missed the hard deadline we were at
this point only talking about how to fix up the interface for the 4.7
release.

I think for 4.8 it should return -EINVAL until someone complains that
it's not working, but that's something we can discuss when the
development window opens.

Thanks
 -George

Jan Beulich April 25, 2016, 2:16 p.m. UTC | #7

>>> On 25.04.16 at 16:01, <[e-mail]> wrote:
> The p2m type changes are also wrong. That type needs to be left alone, 
> presumably, so that anything using HVMMEM_mmio_write_dm and compiled to the 
> old interface version continues to function. I think HVMMEM_ioreq_server 
> needs to map to a new p2m type which should be introduced in patch #3.

I don't understand this part: I thought it was agreed that the old
p2m type needs to go away (not the least because we're tight on
these), and use of the old HVMMEM_* type needs to result in
errors.

Jan

Paul Durrant April 25, 2016, 2:19 p.m. UTC | #8

> -----Original Message-----
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 25 April 2016 15:16
> To: Paul Durrant
> Cc: Andrew Cooper; George Dunlap; Wei Liu; Jun Nakajima; Kevin Tian;
> Zhiyuan Lv; Yu Zhang; [e-mail]; Keir (Xen.org); Tim (Xen.org)
> Subject: RE: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
> Rename p2m_mmio_write_dm to p2m_ioreq_server.
> 
> >>> On 25.04.16 at 16:01, <[e-mail]> wrote:
> > The p2m type changes are also wrong. That type needs to be left alone,
> > presumably, so that anything using HVMMEM_mmio_write_dm and
> compiled to the
> > old interface version continues to function. I think HVMMEM_ioreq_server
> > needs to map to a new p2m type which should be introduced in patch #3.
> 
> I don't understand this part: I thought it was agreed that the old
> p2m type needs to go away (not the least because we're tight on
> these), and use of the old HVMMEM_* type needs to result in
> errors.
> 

I may have misunderstood. I thought we'd back-tracked on that because there's a concern that we also need to keep anything compiled against the old header working. If not then this patch should also remove that p2m type, not rename it.

  Paul

> Jan

George Dunlap April 25, 2016, 2:28 p.m. UTC | #9

On Mon, Apr 25, 2016 at 3:19 PM, Paul Durrant <[e-mail]> wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:[e-mail]]
>> Sent: 25 April 2016 15:16
>> To: Paul Durrant
>> Cc: Andrew Cooper; George Dunlap; Wei Liu; Jun Nakajima; Kevin Tian;
>> Zhiyuan Lv; Yu Zhang; [e-mail]; Keir (Xen.org); Tim (Xen.org)
>> Subject: RE: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>
>> >>> On 25.04.16 at 16:01, <[e-mail]> wrote:
>> > The p2m type changes are also wrong. That type needs to be left alone,
>> > presumably, so that anything using HVMMEM_mmio_write_dm and
>> compiled to the
>> > old interface version continues to function. I think HVMMEM_ioreq_server
>> > needs to map to a new p2m type which should be introduced in patch #3.
>>
>> I don't understand this part: I thought it was agreed that the old
>> p2m type needs to go away (not the least because we're tight on
>> these), and use of the old HVMMEM_* type needs to result in
>> errors.
>>
>
> I may have misunderstood. I thought we'd back-tracked on that because there's a concern that we also need to keep anything compiled against the old header working. If not then this patch should also remove that p2m type, not rename it.

You mean remove the old HVMMEM type?

There are two issues:
1. Whether old code should continue to compile
2. How old code should act on new hypervisors

I think we've determined that we definitely cannot allow code compiled
against old hypervisors to accidentally use a different p2m type; so
we certainly need to "burn" an enum here.

I'd personally prefer we just straight-up rename it to HVMMEM_unused,
so nobody continues to think that the HVMMEM_mmio_write_dm
functionality might still actually work; I think Jan thinks that's not
allowed.

Honestly I don't see the point of letting it compile and then return
-EINVAL when we run it.  If people complain that it doesn't work
anymore we should either make it compile *and* maintain the
functionality, or say "Sorry, just use an older version" and make it
neither compile nor maintain the functionality.

But I sort of assumed this discussion on what support looked like had
already been had and Jan was just enforcing it.

(Maybe we should have had a talk about this in person at the Hackathon...)

 -George

Paul Durrant April 25, 2016, 2:34 p.m. UTC | #10

> -----Original Message-----

> From: George Dunlap

> Sent: 25 April 2016 15:28

> To: Paul Durrant

> Cc: Jan Beulich; Kevin Tian; Wei Liu; Andrew Cooper; Tim (Xen.org); xen-

> [e-mail]; Yu Zhang; Zhiyuan Lv; Jun Nakajima; Keir (Xen.org)

> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> 

> On Mon, Apr 25, 2016 at 3:19 PM, Paul Durrant <[e-mail]>

> wrote:

> >> -----Original Message-----

> >> From: Jan Beulich [mailto:[e-mail]]

> >> Sent: 25 April 2016 15:16

> >> To: Paul Durrant

> >> Cc: Andrew Cooper; George Dunlap; Wei Liu; Jun Nakajima; Kevin Tian;

> >> Zhiyuan Lv; Yu Zhang; [e-mail]; Keir (Xen.org); Tim

> (Xen.org)

> >> Subject: RE: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> >> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> >>

> >> >>> On 25.04.16 at 16:01, <[e-mail]> wrote:

> >> > The p2m type changes are also wrong. That type needs to be left alone,

> >> > presumably, so that anything using HVMMEM_mmio_write_dm and

> >> compiled to the

> >> > old interface version continues to function. I think

> HVMMEM_ioreq_server

> >> > needs to map to a new p2m type which should be introduced in patch

> #3.

> >>

> >> I don't understand this part: I thought it was agreed that the old

> >> p2m type needs to go away (not the least because we're tight on

> >> these), and use of the old HVMMEM_* type needs to result in

> >> errors.

> >>

> >

> > I may have misunderstood. I thought we'd back-tracked on that because

> there's a concern that we also need to keep anything compiled against the

> old header working. If not then this patch should also remove that p2m type,

> not rename it.

> 

> You mean remove the old HVMMEM type?

> 

> There are two issues:

> 1. Whether old code should continue to compile

> 2. How old code should act on new hypervisors

> 

> I think we've determined that we definitely cannot allow code compiled

> against old hypervisors to accidentally use a different p2m type; so

> we certainly need to "burn" an enum here.

> 

> I'd personally prefer we just straight-up rename it to HVMMEM_unused,

> so nobody continues to think that the HVMMEM_mmio_write_dm

> functionality might still actually work; I think Jan thinks that's not

> allowed.

> 

> Honestly I don't see the point of letting it compile and then return

> -EINVAL when we run it.  If people complain that it doesn't work

> anymore we should either make it compile *and* maintain the

> functionality, or say "Sorry, just use an older version" and make it

> neither compile nor maintain the functionality.

> 

> But I sort of assumed this discussion on what support looked like had

> already been had and Jan was just enforcing it.

> 

> (Maybe we should have had a talk about this in person at the Hackathon...)

> 


I'm now confused as to what was agreed, if anything.

The fact of the matter is though that the old type escaped into the wild. I wanted it to go away but because it escaped I guess that may just not be possible.

  Paul

>  -George

Yu Zhang April 25, 2016, 3:21 p.m. UTC | #11

On 4/25/2016 10:01 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>> George Dunlap
>> Sent: 25 April 2016 14:39
>> To: Yu Zhang
>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan Beulich; Wei Liu
>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>
>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang <[e-mail]>
>> wrote:
>>> Previously p2m type p2m_mmio_write_dm was introduced for write-
>>> protected memory pages whose write operations are supposed to be
>>> forwarded to and emulated by an ioreq server. Yet limitations of
>>> rangeset restrict the number of guest pages to be write-protected.
>>>
>>> This patch replaces the p2m type p2m_mmio_write_dm with a new name:
>>> p2m_ioreq_server, which means this p2m type can be claimed by one
>>> ioreq server, instead of being tracked inside the rangeset of ioreq
>>> server. Patches following up will add the related hvmop handling
>>> code which map/unmap type p2m_ioreq_server to/from an ioreq server.
>>>
>>> changes in v3:
>>>   - According to Jan & George's comments, keep
>> HVMMEM_mmio_write_dm
>>>     for old xen interface versions, and replace it with HVMMEM_unused
>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem type
>>>     interfaces;
>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>>
>> Unfortunately these rather contradict each other -- I consider
>> Reviewed-by to only stick if the code I've specified hasn't changed
>> (or has only changed trivially).
>>
>> Also...
>>
>>>
>>> changes in v2:
>>>   - According to George Dunlap's comments, only rename the p2m type,
>>>     with no behavior changes.
>>>
>>> Signed-off-by: Paul Durrant <[e-mail]>
>>> Signed-off-by: Yu Zhang <[e-mail]>
>>> Acked-by: Tim Deegan <[e-mail]>
>>> Acked-by: Andrew Cooper <[e-mail]>
>>> Reviewed-by: George Dunlap <[e-mail]>
>>> Cc: Keir Fraser <[e-mail]>
>>> Cc: Jan Beulich <[e-mail]>
>>> Cc: Andrew Cooper <[e-mail]>
>>> Cc: Jun Nakajima <[e-mail]>
>>> Cc: Kevin Tian <[e-mail]>
>>> Cc: George Dunlap <[e-mail]>
>>> Cc: Tim Deegan <[e-mail]>
>>> ---
>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>>>  xen/include/asm-x86/p2m.h       |  4 ++--
>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>>>  6 files changed, 20 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index f24126d..874cb0f 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>> unsigned long gla,
>>>       */
>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>           (npfec.write_access &&
>>> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
>>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>>>      {
>>>          __put_gfn(p2m, gfn);
>>>          if ( ap2m_active )
>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              get_gfn_query_unlocked(d, a.pfn, &t);
>>>              if ( p2m_is_mmio(t) )
>>>                  a.mem_type =  HVMMEM_mmio_dm;
>>> -            else if ( t == p2m_mmio_write_dm )
>>> -                a.mem_type = HVMMEM_mmio_write_dm;
>>> +            else if ( t == p2m_ioreq_server )
>>> +                a.mem_type = HVMMEM_ioreq_server;
>>>              else if ( p2m_is_readonly(t) )
>>>                  a.mem_type =  HVMMEM_ram_ro;
>>>              else if ( p2m_is_ram(t) )
>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>> +            [HVMMEM_unused] = p2m_invalid,
>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>>>          };
>>>
>>>          if ( copy_from_guest(&a, arg, 1) )
>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>>              goto setmemtype_fail;
>>>
>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>>>              goto setmemtype_fail;
>>>
>>>          while ( a.nr > start_iter )
>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              }
>>>              if ( !p2m_is_ram(t) &&
>>>                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm)
>> &&
>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>> HVMMEM_ram_rw) )
>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>> HVMMEM_ram_rw) )
>>>              {
>>>                  put_gfn(d, pfn);
>>>                  goto setmemtype_fail;
>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>>> index 3cb6868..380ec25 100644
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>> p2m_domain *p2m, ept_entry_t *entry,
>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>>              break;
>>>          case p2m_grant_map_ro:
>>> -        case p2m_mmio_write_dm:
>>> +        case p2m_ioreq_server:
>>>              entry->r = 1;
>>>              entry->w = entry->x = 0;
>>>              entry->a = !!cpu_has_vmx_ept_ad;
>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>> index 3d80612..eabd2e3 100644
>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>> @@ -94,7 +94,7 @@ static unsigned long p2m_type_to_flags(p2m_type_t
>> t, mfn_t mfn,
>>>      default:
>>>          return flags | _PAGE_NX_BIT;
>>>      case p2m_grant_map_ro:
>>> -    case p2m_mmio_write_dm:
>>> +    case p2m_ioreq_server:
>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>      case p2m_ram_ro:
>>>      case p2m_ram_logdirty:
>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>> b/xen/arch/x86/mm/shadow/multi.c
>>> index e5c8499..c81302a 100644
>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>>
>>>      /* Need to hand off device-model MMIO to the device model */
>>>      if ( p2mt == p2m_mmio_dm
>>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>>>      {
>>>          gpa = guest_walk_to_gpa(&gw);
>>>          goto mmio;
>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>> index 5392eb0..ee2ea9c 100644
>>> --- a/xen/include/asm-x86/p2m.h
>>> +++ b/xen/include/asm-x86/p2m.h
>>> @@ -71,7 +71,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_ioreq_server = 15,
>>>  } p2m_type_t;
>>>
>>>  /* Modifiers to the query */
>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>>>                        | p2m_to_mask(p2m_ram_ro)         \
>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>>>                        | p2m_to_mask(p2m_ram_shared)     \
>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>>> +                      | p2m_to_mask(p2m_ioreq_server))
>>>
>>>  /* Write-discard types, which should discard the write operations */
>>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
>>> diff --git a/xen/include/public/hvm/hvm_op.h
>> b/xen/include/public/hvm/hvm_op.h
>>> index 1606185..b3e45cf 100644
>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>> @@ -83,7 +83,13 @@ typedef enum {
>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device model
>> */
>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
>> model */
>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device
>> model */
>>> +#else
>>> +    HVMMEM_unused,             /* Placeholder; setting memory to this type
>>> +                                  will fail for code after 4.7.0 */
>>> +#endif
>>> +    HVMMEM_ioreq_server
>>
>> Also, I don't think we've had a convincing argument for why this patch
>> needs to be in 4.7.  The p2m name changes are internal only, and so
>> don't need to be made at all; and the old functionality will work as
>> well as it ever did.  Furthermore, the whole reason we're in this
>> situation is that we checked in a publicly-visible change to the
>> interface before it was properly ready; I think we should avoid making
>> the same mistake this time.
>>
>> So personally I'd just leave this patch entirely for 4.8; but if Paul
>> and/or Jan have strong opinions, then I would say check in only a
>> patch to do the #if/#else/#endif, and leave both the p2m type changes
>> and the new HVMMEM_ioreq_server enum for when the 4.8 development
>> window opens.
>>
>
> If the whole series is going in then I think this patch is ok. If this the only patch that is going in for 4.7 then I thing we need the patch to hvm_op.h to deprecate the old type and that's it. We definitely should not introduce an implementation of the type HVMMEM_ioreq_server that has the same hardcoded semantics as the old type and then change it.
> The p2m type changes are also wrong. That type needs to be left alone, presumably, so that anything using HVMMEM_mmio_write_dm and compiled to the old interface version continues to function. I think HVMMEM_ioreq_server needs to map to a new p2m type which should be introduced in patch #3.
>

Sorry, I'm also confused now. :(

Do we really want to introduce a new p2m type? Why?
My understanding of the previous agreement is that:
1> We need the interface to work on old hypervisor for
HVMMEM_mmio_write_dm;
2> We need the interface to return -EINVAL for new hypervisor
for HVMMEM_mmio_write_dm;
3> We need the new type HVMMEM_ioreq_server to work on new
hypervisor;

Did I miss something? Or I totally misunderstood?

B.R.
Yu

Paul Durrant April 25, 2016, 3:29 p.m. UTC | #12

> -----Original Message-----

> From: Yu, Zhang [mailto:[e-mail]]

> Sent: 25 April 2016 16:22

> To: Paul Durrant; George Dunlap

> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;

> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu

> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> 

> 

> 

> On 4/25/2016 10:01 PM, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: [e-mail] [mailto:[e-mail]] On Behalf Of

> >> George Dunlap

> >> Sent: 25 April 2016 14:39

> >> To: Yu Zhang

> >> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;

> >> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan Beulich; Wei

> Liu

> >> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> >> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> >>

> >> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang

> <[e-mail]>

> >> wrote:

> >>> Previously p2m type p2m_mmio_write_dm was introduced for write-

> >>> protected memory pages whose write operations are supposed to be

> >>> forwarded to and emulated by an ioreq server. Yet limitations of

> >>> rangeset restrict the number of guest pages to be write-protected.

> >>>

> >>> This patch replaces the p2m type p2m_mmio_write_dm with a new

> name:

> >>> p2m_ioreq_server, which means this p2m type can be claimed by one

> >>> ioreq server, instead of being tracked inside the rangeset of ioreq

> >>> server. Patches following up will add the related hvmop handling

> >>> code which map/unmap type p2m_ioreq_server to/from an ioreq

> server.

> >>>

> >>> changes in v3:

> >>>   - According to Jan & George's comments, keep

> >> HVMMEM_mmio_write_dm

> >>>     for old xen interface versions, and replace it with HVMMEM_unused

> >>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new

> >>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem

> type

> >>>     interfaces;

> >>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.

> >>

> >> Unfortunately these rather contradict each other -- I consider

> >> Reviewed-by to only stick if the code I've specified hasn't changed

> >> (or has only changed trivially).

> >>

> >> Also...

> >>

> >>>

> >>> changes in v2:

> >>>   - According to George Dunlap's comments, only rename the p2m type,

> >>>     with no behavior changes.

> >>>

> >>> Signed-off-by: Paul Durrant <[e-mail]>

> >>> Signed-off-by: Yu Zhang <[e-mail]>

> >>> Acked-by: Tim Deegan <[e-mail]>

> >>> Acked-by: Andrew Cooper <[e-mail]>

> >>> Reviewed-by: George Dunlap <[e-mail]>

> >>> Cc: Keir Fraser <[e-mail]>

> >>> Cc: Jan Beulich <[e-mail]>

> >>> Cc: Andrew Cooper <[e-mail]>

> >>> Cc: Jun Nakajima <[e-mail]>

> >>> Cc: Kevin Tian <[e-mail]>

> >>> Cc: George Dunlap <[e-mail]>

> >>> Cc: Tim Deegan <[e-mail]>

> >>> ---

> >>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------

> >>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-

> >>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-

> >>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-

> >>>  xen/include/asm-x86/p2m.h       |  4 ++--

> >>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-

> >>>  6 files changed, 20 insertions(+), 12 deletions(-)

> >>>

> >>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c

> >>> index f24126d..874cb0f 100644

> >>> --- a/xen/arch/x86/hvm/hvm.c

> >>> +++ b/xen/arch/x86/hvm/hvm.c

> >>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,

> >> unsigned long gla,

> >>>       */

> >>>      if ( (p2mt == p2m_mmio_dm) ||

> >>>           (npfec.write_access &&

> >>> -          (p2m_is_discard_write(p2mt) || (p2mt ==

> p2m_mmio_write_dm))) )

> >>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )

> >>>      {

> >>>          __put_gfn(p2m, gfn);

> >>>          if ( ap2m_active )

> >>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,

> >> XEN_GUEST_HANDLE_PARAM(void) arg)

> >>>              get_gfn_query_unlocked(d, a.pfn, &t);

> >>>              if ( p2m_is_mmio(t) )

> >>>                  a.mem_type =  HVMMEM_mmio_dm;

> >>> -            else if ( t == p2m_mmio_write_dm )

> >>> -                a.mem_type = HVMMEM_mmio_write_dm;

> >>> +            else if ( t == p2m_ioreq_server )

> >>> +                a.mem_type = HVMMEM_ioreq_server;

> >>>              else if ( p2m_is_readonly(t) )

> >>>                  a.mem_type =  HVMMEM_ram_ro;

> >>>              else if ( p2m_is_ram(t) )

> >>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,

> >> XEN_GUEST_HANDLE_PARAM(void) arg)

> >>>              [HVMMEM_ram_rw]  = p2m_ram_rw,

> >>>              [HVMMEM_ram_ro]  = p2m_ram_ro,

> >>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,

> >>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm

> >>> +            [HVMMEM_unused] = p2m_invalid,

> >>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server

> >>>          };

> >>>

> >>>          if ( copy_from_guest(&a, arg, 1) )

> >>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,

> >> XEN_GUEST_HANDLE_PARAM(void) arg)

> >>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )

> >>>              goto setmemtype_fail;

> >>>

> >>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )

> >>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||

> >>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )

> >>>              goto setmemtype_fail;

> >>>

> >>>          while ( a.nr > start_iter )

> >>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,

> >> XEN_GUEST_HANDLE_PARAM(void) arg)

> >>>              }

> >>>              if ( !p2m_is_ram(t) &&

> >>>                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm)

> >> &&

> >>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=

> >> HVMMEM_ram_rw) )

> >>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=

> >> HVMMEM_ram_rw) )

> >>>              {

> >>>                  put_gfn(d, pfn);

> >>>                  goto setmemtype_fail;

> >>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-

> ept.c

> >>> index 3cb6868..380ec25 100644

> >>> --- a/xen/arch/x86/mm/p2m-ept.c

> >>> +++ b/xen/arch/x86/mm/p2m-ept.c

> >>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct

> >> p2m_domain *p2m, ept_entry_t *entry,

> >>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;

> >>>              break;

> >>>          case p2m_grant_map_ro:

> >>> -        case p2m_mmio_write_dm:

> >>> +        case p2m_ioreq_server:

> >>>              entry->r = 1;

> >>>              entry->w = entry->x = 0;

> >>>              entry->a = !!cpu_has_vmx_ept_ad;

> >>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c

> >>> index 3d80612..eabd2e3 100644

> >>> --- a/xen/arch/x86/mm/p2m-pt.c

> >>> +++ b/xen/arch/x86/mm/p2m-pt.c

> >>> @@ -94,7 +94,7 @@ static unsigned long

> p2m_type_to_flags(p2m_type_t

> >> t, mfn_t mfn,

> >>>      default:

> >>>          return flags | _PAGE_NX_BIT;

> >>>      case p2m_grant_map_ro:

> >>> -    case p2m_mmio_write_dm:

> >>> +    case p2m_ioreq_server:

> >>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;

> >>>      case p2m_ram_ro:

> >>>      case p2m_ram_logdirty:

> >>> diff --git a/xen/arch/x86/mm/shadow/multi.c

> >> b/xen/arch/x86/mm/shadow/multi.c

> >>> index e5c8499..c81302a 100644

> >>> --- a/xen/arch/x86/mm/shadow/multi.c

> >>> +++ b/xen/arch/x86/mm/shadow/multi.c

> >>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,

> >>>

> >>>      /* Need to hand off device-model MMIO to the device model */

> >>>      if ( p2mt == p2m_mmio_dm

> >>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )

> >>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )

> >>>      {

> >>>          gpa = guest_walk_to_gpa(&gw);

> >>>          goto mmio;

> >>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h

> >>> index 5392eb0..ee2ea9c 100644

> >>> --- a/xen/include/asm-x86/p2m.h

> >>> +++ b/xen/include/asm-x86/p2m.h

> >>> @@ -71,7 +71,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_ioreq_server = 15,

> >>>  } p2m_type_t;

> >>>

> >>>  /* Modifiers to the query */

> >>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;

> >>>                        | p2m_to_mask(p2m_ram_ro)         \

> >>>                        | p2m_to_mask(p2m_grant_map_ro)   \

> >>>                        | p2m_to_mask(p2m_ram_shared)     \

> >>> -                      | p2m_to_mask(p2m_mmio_write_dm))

> >>> +                      | p2m_to_mask(p2m_ioreq_server))

> >>>

> >>>  /* Write-discard types, which should discard the write operations */

> >>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \

> >>> diff --git a/xen/include/public/hvm/hvm_op.h

> >> b/xen/include/public/hvm/hvm_op.h

> >>> index 1606185..b3e45cf 100644

> >>> --- a/xen/include/public/hvm/hvm_op.h

> >>> +++ b/xen/include/public/hvm/hvm_op.h

> >>> @@ -83,7 +83,13 @@ typedef enum {

> >>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */

> >>>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */

> >>>      HVMMEM_mmio_dm,            /* Reads and write go to the device

> model

> >> */

> >>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device

> >> model */

> >>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700

> >>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device

> >> model */

> >>> +#else

> >>> +    HVMMEM_unused,             /* Placeholder; setting memory to this

> type

> >>> +                                  will fail for code after 4.7.0 */

> >>> +#endif

> >>> +    HVMMEM_ioreq_server

> >>

> >> Also, I don't think we've had a convincing argument for why this patch

> >> needs to be in 4.7.  The p2m name changes are internal only, and so

> >> don't need to be made at all; and the old functionality will work as

> >> well as it ever did.  Furthermore, the whole reason we're in this

> >> situation is that we checked in a publicly-visible change to the

> >> interface before it was properly ready; I think we should avoid making

> >> the same mistake this time.

> >>

> >> So personally I'd just leave this patch entirely for 4.8; but if Paul

> >> and/or Jan have strong opinions, then I would say check in only a

> >> patch to do the #if/#else/#endif, and leave both the p2m type changes

> >> and the new HVMMEM_ioreq_server enum for when the 4.8

> development

> >> window opens.

> >>

> >

> > If the whole series is going in then I think this patch is ok. If this the only

> patch that is going in for 4.7 then I thing we need the patch to hvm_op.h to

> deprecate the old type and that's it. We definitely should not introduce an

> implementation of the type HVMMEM_ioreq_server that has the same

> hardcoded semantics as the old type and then change it.

> > The p2m type changes are also wrong. That type needs to be left alone,

> presumably, so that anything using HVMMEM_mmio_write_dm and

> compiled to the old interface version continues to function. I think

> HVMMEM_ioreq_server needs to map to a new p2m type which should be

> introduced in patch #3.

> >

> 

> Sorry, I'm also confused now. :(

> 

> Do we really want to introduce a new p2m type? Why?

> My understanding of the previous agreement is that:

> 1> We need the interface to work on old hypervisor for

> HVMMEM_mmio_write_dm;

> 2> We need the interface to return -EINVAL for new hypervisor

> for HVMMEM_mmio_write_dm;

> 3> We need the new type HVMMEM_ioreq_server to work on new

> hypervisor;

> 

> Did I miss something? Or I totally misunderstood?

> 


I don't know. I'm confused too. What we definitely don't want to do is add a new HVMMEM type and have it map to the old behaviour, otherwise we're no better off.

The question I'm not clear on the answer to is what happens to old code:

Should it continue to compile? If so, should it continue to run.

  Paul

> B.R.

> Yu

Jan Beulich April 25, 2016, 3:38 p.m. UTC | #13

>>> On 25.04.16 at 17:29, <[e-mail]> wrote:
>>  -----Original Message-----
>> From: Yu, Zhang [mailto:[e-mail]]
>> Sent: 25 April 2016 16:22
>> To: Paul Durrant; George Dunlap
>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu
>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>> 
>> 
>> 
>> On 4/25/2016 10:01 PM, Paul Durrant wrote:
>> >> -----Original Message-----
>> >> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>> >> George Dunlap
>> >> Sent: 25 April 2016 14:39
>> >> To: Yu Zhang
>> >> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>> >> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan Beulich; Wei
>> Liu
>> >> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> >> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>> >>
>> >> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang
>> <[e-mail]>
>> >> wrote:
>> >>> Previously p2m type p2m_mmio_write_dm was introduced for write-
>> >>> protected memory pages whose write operations are supposed to be
>> >>> forwarded to and emulated by an ioreq server. Yet limitations of
>> >>> rangeset restrict the number of guest pages to be write-protected.
>> >>>
>> >>> This patch replaces the p2m type p2m_mmio_write_dm with a new
>> name:
>> >>> p2m_ioreq_server, which means this p2m type can be claimed by one
>> >>> ioreq server, instead of being tracked inside the rangeset of ioreq
>> >>> server. Patches following up will add the related hvmop handling
>> >>> code which map/unmap type p2m_ioreq_server to/from an ioreq
>> server.
>> >>>
>> >>> changes in v3:
>> >>>   - According to Jan & George's comments, keep
>> >> HVMMEM_mmio_write_dm
>> >>>     for old xen interface versions, and replace it with HVMMEM_unused
>> >>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>> >>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem
>> type
>> >>>     interfaces;
>> >>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>> >>
>> >> Unfortunately these rather contradict each other -- I consider
>> >> Reviewed-by to only stick if the code I've specified hasn't changed
>> >> (or has only changed trivially).
>> >>
>> >> Also...
>> >>
>> >>>
>> >>> changes in v2:
>> >>>   - According to George Dunlap's comments, only rename the p2m type,
>> >>>     with no behavior changes.
>> >>>
>> >>> Signed-off-by: Paul Durrant <[e-mail]>
>> >>> Signed-off-by: Yu Zhang <[e-mail]>
>> >>> Acked-by: Tim Deegan <[e-mail]>
>> >>> Acked-by: Andrew Cooper <[e-mail]>
>> >>> Reviewed-by: George Dunlap <[e-mail]>
>> >>> Cc: Keir Fraser <[e-mail]>
>> >>> Cc: Jan Beulich <[e-mail]>
>> >>> Cc: Andrew Cooper <[e-mail]>
>> >>> Cc: Jun Nakajima <[e-mail]>
>> >>> Cc: Kevin Tian <[e-mail]>
>> >>> Cc: George Dunlap <[e-mail]>
>> >>> Cc: Tim Deegan <[e-mail]>
>> >>> ---
>> >>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>> >>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>> >>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>> >>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>> >>>  xen/include/asm-x86/p2m.h       |  4 ++--
>> >>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>> >>>  6 files changed, 20 insertions(+), 12 deletions(-)
>> >>>
>> >>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> >>> index f24126d..874cb0f 100644
>> >>> --- a/xen/arch/x86/hvm/hvm.c
>> >>> +++ b/xen/arch/x86/hvm/hvm.c
>> >>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>> >> unsigned long gla,
>> >>>       */
>> >>>      if ( (p2mt == p2m_mmio_dm) ||
>> >>>           (npfec.write_access &&
>> >>> -          (p2m_is_discard_write(p2mt) || (p2mt ==
>> p2m_mmio_write_dm))) )
>> >>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>> >>>      {
>> >>>          __put_gfn(p2m, gfn);
>> >>>          if ( ap2m_active )
>> >>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>> >> XEN_GUEST_HANDLE_PARAM(void) arg)
>> >>>              get_gfn_query_unlocked(d, a.pfn, &t);
>> >>>              if ( p2m_is_mmio(t) )
>> >>>                  a.mem_type =  HVMMEM_mmio_dm;
>> >>> -            else if ( t == p2m_mmio_write_dm )
>> >>> -                a.mem_type = HVMMEM_mmio_write_dm;
>> >>> +            else if ( t == p2m_ioreq_server )
>> >>> +                a.mem_type = HVMMEM_ioreq_server;
>> >>>              else if ( p2m_is_readonly(t) )
>> >>>                  a.mem_type =  HVMMEM_ram_ro;
>> >>>              else if ( p2m_is_ram(t) )
>> >>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>> >> XEN_GUEST_HANDLE_PARAM(void) arg)
>> >>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>> >>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>> >>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>> >>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>> >>> +            [HVMMEM_unused] = p2m_invalid,
>> >>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>> >>>          };
>> >>>
>> >>>          if ( copy_from_guest(&a, arg, 1) )
>> >>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>> >> XEN_GUEST_HANDLE_PARAM(void) arg)
>> >>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>> >>>              goto setmemtype_fail;
>> >>>
>> >>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>> >>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>> >>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>> >>>              goto setmemtype_fail;
>> >>>
>> >>>          while ( a.nr > start_iter )
>> >>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>> >> XEN_GUEST_HANDLE_PARAM(void) arg)
>> >>>              }
>> >>>              if ( !p2m_is_ram(t) &&
>> >>>                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm)
>> >> &&
>> >>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>> >> HVMMEM_ram_rw) )
>> >>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>> >> HVMMEM_ram_rw) )
>> >>>              {
>> >>>                  put_gfn(d, pfn);
>> >>>                  goto setmemtype_fail;
>> >>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
>> ept.c
>> >>> index 3cb6868..380ec25 100644
>> >>> --- a/xen/arch/x86/mm/p2m-ept.c
>> >>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> >>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>> >> p2m_domain *p2m, ept_entry_t *entry,
>> >>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>> >>>              break;
>> >>>          case p2m_grant_map_ro:
>> >>> -        case p2m_mmio_write_dm:
>> >>> +        case p2m_ioreq_server:
>> >>>              entry->r = 1;
>> >>>              entry->w = entry->x = 0;
>> >>>              entry->a = !!cpu_has_vmx_ept_ad;
>> >>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>> >>> index 3d80612..eabd2e3 100644
>> >>> --- a/xen/arch/x86/mm/p2m-pt.c
>> >>> +++ b/xen/arch/x86/mm/p2m-pt.c
>> >>> @@ -94,7 +94,7 @@ static unsigned long
>> p2m_type_to_flags(p2m_type_t
>> >> t, mfn_t mfn,
>> >>>      default:
>> >>>          return flags | _PAGE_NX_BIT;
>> >>>      case p2m_grant_map_ro:
>> >>> -    case p2m_mmio_write_dm:
>> >>> +    case p2m_ioreq_server:
>> >>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>> >>>      case p2m_ram_ro:
>> >>>      case p2m_ram_logdirty:
>> >>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>> >> b/xen/arch/x86/mm/shadow/multi.c
>> >>> index e5c8499..c81302a 100644
>> >>> --- a/xen/arch/x86/mm/shadow/multi.c
>> >>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> >>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>> >>>
>> >>>      /* Need to hand off device-model MMIO to the device model */
>> >>>      if ( p2mt == p2m_mmio_dm
>> >>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
>> >>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>> >>>      {
>> >>>          gpa = guest_walk_to_gpa(&gw);
>> >>>          goto mmio;
>> >>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> >>> index 5392eb0..ee2ea9c 100644
>> >>> --- a/xen/include/asm-x86/p2m.h
>> >>> +++ b/xen/include/asm-x86/p2m.h
>> >>> @@ -71,7 +71,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_ioreq_server = 15,
>> >>>  } p2m_type_t;
>> >>>
>> >>>  /* Modifiers to the query */
>> >>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>> >>>                        | p2m_to_mask(p2m_ram_ro)         \
>> >>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>> >>>                        | p2m_to_mask(p2m_ram_shared)     \
>> >>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>> >>> +                      | p2m_to_mask(p2m_ioreq_server))
>> >>>
>> >>>  /* Write-discard types, which should discard the write operations */
>> >>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
>> >>> diff --git a/xen/include/public/hvm/hvm_op.h
>> >> b/xen/include/public/hvm/hvm_op.h
>> >>> index 1606185..b3e45cf 100644
>> >>> --- a/xen/include/public/hvm/hvm_op.h
>> >>> +++ b/xen/include/public/hvm/hvm_op.h
>> >>> @@ -83,7 +83,13 @@ typedef enum {
>> >>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>> >>>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>> >>>      HVMMEM_mmio_dm,            /* Reads and write go to the device
>> model
>> >> */
>> >>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
>> >> model */
>> >>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>> >>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device
>> >> model */
>> >>> +#else
>> >>> +    HVMMEM_unused,             /* Placeholder; setting memory to this
>> type
>> >>> +                                  will fail for code after 4.7.0 */
>> >>> +#endif
>> >>> +    HVMMEM_ioreq_server
>> >>
>> >> Also, I don't think we've had a convincing argument for why this patch
>> >> needs to be in 4.7.  The p2m name changes are internal only, and so
>> >> don't need to be made at all; and the old functionality will work as
>> >> well as it ever did.  Furthermore, the whole reason we're in this
>> >> situation is that we checked in a publicly-visible change to the
>> >> interface before it was properly ready; I think we should avoid making
>> >> the same mistake this time.
>> >>
>> >> So personally I'd just leave this patch entirely for 4.8; but if Paul
>> >> and/or Jan have strong opinions, then I would say check in only a
>> >> patch to do the #if/#else/#endif, and leave both the p2m type changes
>> >> and the new HVMMEM_ioreq_server enum for when the 4.8
>> development
>> >> window opens.
>> >>
>> >
>> > If the whole series is going in then I think this patch is ok. If this the 
> only
>> patch that is going in for 4.7 then I thing we need the patch to hvm_op.h to
>> deprecate the old type and that's it. We definitely should not introduce an
>> implementation of the type HVMMEM_ioreq_server that has the same
>> hardcoded semantics as the old type and then change it.
>> > The p2m type changes are also wrong. That type needs to be left alone,
>> presumably, so that anything using HVMMEM_mmio_write_dm and
>> compiled to the old interface version continues to function. I think
>> HVMMEM_ioreq_server needs to map to a new p2m type which should be
>> introduced in patch #3.
>> >
>> 
>> Sorry, I'm also confused now. :(
>> 
>> Do we really want to introduce a new p2m type? Why?
>> My understanding of the previous agreement is that:
>> 1> We need the interface to work on old hypervisor for
>> HVMMEM_mmio_write_dm;
>> 2> We need the interface to return -EINVAL for new hypervisor
>> for HVMMEM_mmio_write_dm;
>> 3> We need the new type HVMMEM_ioreq_server to work on new
>> hypervisor;
>> 
>> Did I miss something? Or I totally misunderstood?
>> 
> 
> I don't know. I'm confused too. What we definitely don't want to do is add a 
> new HVMMEM type and have it map to the old behaviour, otherwise we're no 
> better off.
> 
> The question I'm not clear on the answer to is what happens to old code:
> 
> Should it continue to compile? If so, should it continue to run.

We only need to be concerned about the "get type" functionality,
as that's the only thing an arbitrary guest can use. If the
hypercall simply never returns the old type, then old code will
still work (it'll just have some dead code on new Xen), and hence
it compiling against the older interface is fine (and, from general
considerations, a requirement).

Jan

Yu Zhang April 25, 2016, 3:49 p.m. UTC | #14

On 4/25/2016 11:29 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Yu, Zhang [mailto:[e-mail]]
>> Sent: 25 April 2016 16:22
>> To: Paul Durrant; George Dunlap
>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu
>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>
>>
>>
>> On 4/25/2016 10:01 PM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>>>> George Dunlap
>>>> Sent: 25 April 2016 14:39
>>>> To: Yu Zhang
>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>>>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan Beulich; Wei
>> Liu
>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>
>>>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang
>> <[e-mail]>
>>>> wrote:
>>>>> Previously p2m type p2m_mmio_write_dm was introduced for write-
>>>>> protected memory pages whose write operations are supposed to be
>>>>> forwarded to and emulated by an ioreq server. Yet limitations of
>>>>> rangeset restrict the number of guest pages to be write-protected.
>>>>>
>>>>> This patch replaces the p2m type p2m_mmio_write_dm with a new
>> name:
>>>>> p2m_ioreq_server, which means this p2m type can be claimed by one
>>>>> ioreq server, instead of being tracked inside the rangeset of ioreq
>>>>> server. Patches following up will add the related hvmop handling
>>>>> code which map/unmap type p2m_ioreq_server to/from an ioreq
>> server.
>>>>>
>>>>> changes in v3:
>>>>>   - According to Jan & George's comments, keep
>>>> HVMMEM_mmio_write_dm
>>>>>     for old xen interface versions, and replace it with HVMMEM_unused
>>>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>>>>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem
>> type
>>>>>     interfaces;
>>>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>>>>
>>>> Unfortunately these rather contradict each other -- I consider
>>>> Reviewed-by to only stick if the code I've specified hasn't changed
>>>> (or has only changed trivially).
>>>>
>>>> Also...
>>>>
>>>>>
>>>>> changes in v2:
>>>>>   - According to George Dunlap's comments, only rename the p2m type,
>>>>>     with no behavior changes.
>>>>>
>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>> Signed-off-by: Yu Zhang <[e-mail]>
>>>>> Acked-by: Tim Deegan <[e-mail]>
>>>>> Acked-by: Andrew Cooper <[e-mail]>
>>>>> Reviewed-by: George Dunlap <[e-mail]>
>>>>> Cc: Keir Fraser <[e-mail]>
>>>>> Cc: Jan Beulich <[e-mail]>
>>>>> Cc: Andrew Cooper <[e-mail]>
>>>>> Cc: Jun Nakajima <[e-mail]>
>>>>> Cc: Kevin Tian <[e-mail]>
>>>>> Cc: George Dunlap <[e-mail]>
>>>>> Cc: Tim Deegan <[e-mail]>
>>>>> ---
>>>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>>>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>>>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>>>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>>>>>  xen/include/asm-x86/p2m.h       |  4 ++--
>>>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>>>>>  6 files changed, 20 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>> index f24126d..874cb0f 100644
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>>>> unsigned long gla,
>>>>>       */
>>>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>>>           (npfec.write_access &&
>>>>> -          (p2m_is_discard_write(p2mt) || (p2mt ==
>> p2m_mmio_write_dm))) )
>>>>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>>>>>      {
>>>>>          __put_gfn(p2m, gfn);
>>>>>          if ( ap2m_active )
>>>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>              get_gfn_query_unlocked(d, a.pfn, &t);
>>>>>              if ( p2m_is_mmio(t) )
>>>>>                  a.mem_type =  HVMMEM_mmio_dm;
>>>>> -            else if ( t == p2m_mmio_write_dm )
>>>>> -                a.mem_type = HVMMEM_mmio_write_dm;
>>>>> +            else if ( t == p2m_ioreq_server )
>>>>> +                a.mem_type = HVMMEM_ioreq_server;
>>>>>              else if ( p2m_is_readonly(t) )
>>>>>                  a.mem_type =  HVMMEM_ram_ro;
>>>>>              else if ( p2m_is_ram(t) )
>>>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>>>> +            [HVMMEM_unused] = p2m_invalid,
>>>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>>>>>          };
>>>>>
>>>>>          if ( copy_from_guest(&a, arg, 1) )
>>>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>>>>              goto setmemtype_fail;
>>>>>
>>>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>>>>>              goto setmemtype_fail;
>>>>>
>>>>>          while ( a.nr > start_iter )
>>>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>              }
>>>>>              if ( !p2m_is_ram(t) &&
>>>>>                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm)
>>>> &&
>>>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>>>> HVMMEM_ram_rw) )
>>>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>>>> HVMMEM_ram_rw) )
>>>>>              {
>>>>>                  put_gfn(d, pfn);
>>>>>                  goto setmemtype_fail;
>>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
>> ept.c
>>>>> index 3cb6868..380ec25 100644
>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>>>> p2m_domain *p2m, ept_entry_t *entry,
>>>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>>>>              break;
>>>>>          case p2m_grant_map_ro:
>>>>> -        case p2m_mmio_write_dm:
>>>>> +        case p2m_ioreq_server:
>>>>>              entry->r = 1;
>>>>>              entry->w = entry->x = 0;
>>>>>              entry->a = !!cpu_has_vmx_ept_ad;
>>>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>>>> index 3d80612..eabd2e3 100644
>>>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>>>> @@ -94,7 +94,7 @@ static unsigned long
>> p2m_type_to_flags(p2m_type_t
>>>> t, mfn_t mfn,
>>>>>      default:
>>>>>          return flags | _PAGE_NX_BIT;
>>>>>      case p2m_grant_map_ro:
>>>>> -    case p2m_mmio_write_dm:
>>>>> +    case p2m_ioreq_server:
>>>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>>>      case p2m_ram_ro:
>>>>>      case p2m_ram_logdirty:
>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>>> index e5c8499..c81302a 100644
>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>>>>
>>>>>      /* Need to hand off device-model MMIO to the device model */
>>>>>      if ( p2mt == p2m_mmio_dm
>>>>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
>>>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>>>>>      {
>>>>>          gpa = guest_walk_to_gpa(&gw);
>>>>>          goto mmio;
>>>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>>>> index 5392eb0..ee2ea9c 100644
>>>>> --- a/xen/include/asm-x86/p2m.h
>>>>> +++ b/xen/include/asm-x86/p2m.h
>>>>> @@ -71,7 +71,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_ioreq_server = 15,
>>>>>  } p2m_type_t;
>>>>>
>>>>>  /* Modifiers to the query */
>>>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>>>>>                        | p2m_to_mask(p2m_ram_ro)         \
>>>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>>>>>                        | p2m_to_mask(p2m_ram_shared)     \
>>>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>>>>> +                      | p2m_to_mask(p2m_ioreq_server))
>>>>>
>>>>>  /* Write-discard types, which should discard the write operations */
>>>>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
>>>>> diff --git a/xen/include/public/hvm/hvm_op.h
>>>> b/xen/include/public/hvm/hvm_op.h
>>>>> index 1606185..b3e45cf 100644
>>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>>> @@ -83,7 +83,13 @@ typedef enum {
>>>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>>>>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>>>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device
>> model
>>>> */
>>>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
>>>> model */
>>>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>>>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device
>>>> model */
>>>>> +#else
>>>>> +    HVMMEM_unused,             /* Placeholder; setting memory to this
>> type
>>>>> +                                  will fail for code after 4.7.0 */
>>>>> +#endif
>>>>> +    HVMMEM_ioreq_server
>>>>
>>>> Also, I don't think we've had a convincing argument for why this patch
>>>> needs to be in 4.7.  The p2m name changes are internal only, and so
>>>> don't need to be made at all; and the old functionality will work as
>>>> well as it ever did.  Furthermore, the whole reason we're in this
>>>> situation is that we checked in a publicly-visible change to the
>>>> interface before it was properly ready; I think we should avoid making
>>>> the same mistake this time.
>>>>
>>>> So personally I'd just leave this patch entirely for 4.8; but if Paul
>>>> and/or Jan have strong opinions, then I would say check in only a
>>>> patch to do the #if/#else/#endif, and leave both the p2m type changes
>>>> and the new HVMMEM_ioreq_server enum for when the 4.8
>> development
>>>> window opens.
>>>>
>>>
>>> If the whole series is going in then I think this patch is ok. If this the only
>> patch that is going in for 4.7 then I thing we need the patch to hvm_op.h to
>> deprecate the old type and that's it. We definitely should not introduce an
>> implementation of the type HVMMEM_ioreq_server that has the same
>> hardcoded semantics as the old type and then change it.
>>> The p2m type changes are also wrong. That type needs to be left alone,
>> presumably, so that anything using HVMMEM_mmio_write_dm and
>> compiled to the old interface version continues to function. I think
>> HVMMEM_ioreq_server needs to map to a new p2m type which should be
>> introduced in patch #3.
>>>
>>
>> Sorry, I'm also confused now. :(
>>
>> Do we really want to introduce a new p2m type? Why?
>> My understanding of the previous agreement is that:
>> 1> We need the interface to work on old hypervisor for
>> HVMMEM_mmio_write_dm;
>> 2> We need the interface to return -EINVAL for new hypervisor
>> for HVMMEM_mmio_write_dm;
>> 3> We need the new type HVMMEM_ioreq_server to work on new
>> hypervisor;
>>
>> Did I miss something? Or I totally misunderstood?
>>
>
> I don't know. I'm confused too. What we definitely don't want to do is add a new HVMMEM type and have it map to the old behaviour, otherwise we're no better off.
>

Thanks for your reply, Paul.

Well, but the old HEMMEM type does not exist anymore, and it is not old
behavior with patch3 accepted. Will it be more reasonable if all 3 these
patches are accepted in next version, 4.8 or 4.7.1 if there's one?

One reason I hesitate to remove the old p2m_mmio_write_dm (which is 0xf
IIRC), and define p2m_ioreq_server another value(say, 0x10), is that
what if another future p2m type is introduced with this value(0xf)?
That would also be weird…

> The question I'm not clear on the answer to is what happens to old code:
>
> Should it continue to compile? If so, should it continue to run.
>

By "old code", you mean old interface on new hypervisor, or both?
>   Paul
>
>> B.R.
>> Yu

Yu

Yu Zhang April 25, 2016, 3:53 p.m. UTC | #15

On 4/25/2016 11:38 PM, Jan Beulich wrote:
>>>> On 25.04.16 at 17:29, <[e-mail]> wrote:
>>>  -----Original Message-----
>>> From: Yu, Zhang [mailto:[e-mail]]
>>> Sent: 25 April 2016 16:22
>>> To: Paul Durrant; George Dunlap
>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>>> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu
>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>
>>>
>>>
>>> On 4/25/2016 10:01 PM, Paul Durrant wrote:
>>>>> -----Original Message-----
>>>>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>>>>> George Dunlap
>>>>> Sent: 25 April 2016 14:39
>>>>> To: Yu Zhang
>>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>>>>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan Beulich; Wei
>>> Liu
>>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>>
>>>>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang
>>> <[e-mail]>
>>>>> wrote:
>>>>>> Previously p2m type p2m_mmio_write_dm was introduced for write-
>>>>>> protected memory pages whose write operations are supposed to be
>>>>>> forwarded to and emulated by an ioreq server. Yet limitations of
>>>>>> rangeset restrict the number of guest pages to be write-protected.
>>>>>>
>>>>>> This patch replaces the p2m type p2m_mmio_write_dm with a new
>>> name:
>>>>>> p2m_ioreq_server, which means this p2m type can be claimed by one
>>>>>> ioreq server, instead of being tracked inside the rangeset of ioreq
>>>>>> server. Patches following up will add the related hvmop handling
>>>>>> code which map/unmap type p2m_ioreq_server to/from an ioreq
>>> server.
>>>>>>
>>>>>> changes in v3:
>>>>>>   - According to Jan & George's comments, keep
>>>>> HVMMEM_mmio_write_dm
>>>>>>     for old xen interface versions, and replace it with HVMMEM_unused
>>>>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>>>>>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem
>>> type
>>>>>>     interfaces;
>>>>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>>>>>
>>>>> Unfortunately these rather contradict each other -- I consider
>>>>> Reviewed-by to only stick if the code I've specified hasn't changed
>>>>> (or has only changed trivially).
>>>>>
>>>>> Also...
>>>>>
>>>>>>
>>>>>> changes in v2:
>>>>>>   - According to George Dunlap's comments, only rename the p2m type,
>>>>>>     with no behavior changes.
>>>>>>
>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>>> Signed-off-by: Yu Zhang <[e-mail]>
>>>>>> Acked-by: Tim Deegan <[e-mail]>
>>>>>> Acked-by: Andrew Cooper <[e-mail]>
>>>>>> Reviewed-by: George Dunlap <[e-mail]>
>>>>>> Cc: Keir Fraser <[e-mail]>
>>>>>> Cc: Jan Beulich <[e-mail]>
>>>>>> Cc: Andrew Cooper <[e-mail]>
>>>>>> Cc: Jun Nakajima <[e-mail]>
>>>>>> Cc: Kevin Tian <[e-mail]>
>>>>>> Cc: George Dunlap <[e-mail]>
>>>>>> Cc: Tim Deegan <[e-mail]>
>>>>>> ---
>>>>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>>>>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>>>>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>>>>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>>>>>>  xen/include/asm-x86/p2m.h       |  4 ++--
>>>>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>>>>>>  6 files changed, 20 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>>> index f24126d..874cb0f 100644
>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>>>>> unsigned long gla,
>>>>>>       */
>>>>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>>>>           (npfec.write_access &&
>>>>>> -          (p2m_is_discard_write(p2mt) || (p2mt ==
>>> p2m_mmio_write_dm))) )
>>>>>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>>>>>>      {
>>>>>>          __put_gfn(p2m, gfn);
>>>>>>          if ( ap2m_active )
>>>>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>              get_gfn_query_unlocked(d, a.pfn, &t);
>>>>>>              if ( p2m_is_mmio(t) )
>>>>>>                  a.mem_type =  HVMMEM_mmio_dm;
>>>>>> -            else if ( t == p2m_mmio_write_dm )
>>>>>> -                a.mem_type = HVMMEM_mmio_write_dm;
>>>>>> +            else if ( t == p2m_ioreq_server )
>>>>>> +                a.mem_type = HVMMEM_ioreq_server;
>>>>>>              else if ( p2m_is_readonly(t) )
>>>>>>                  a.mem_type =  HVMMEM_ram_ro;
>>>>>>              else if ( p2m_is_ram(t) )
>>>>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>>>>> +            [HVMMEM_unused] = p2m_invalid,
>>>>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>>>>>>          };
>>>>>>
>>>>>>          if ( copy_from_guest(&a, arg, 1) )
>>>>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>>>>>              goto setmemtype_fail;
>>>>>>
>>>>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>>>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>>>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>>>>>>              goto setmemtype_fail;
>>>>>>
>>>>>>          while ( a.nr > start_iter )
>>>>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>              }
>>>>>>              if ( !p2m_is_ram(t) &&
>>>>>>                   (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm)
>>>>> &&
>>>>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>>>>> HVMMEM_ram_rw) )
>>>>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>>>>> HVMMEM_ram_rw) )
>>>>>>              {
>>>>>>                  put_gfn(d, pfn);
>>>>>>                  goto setmemtype_fail;
>>>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
>>> ept.c
>>>>>> index 3cb6868..380ec25 100644
>>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>>>>> p2m_domain *p2m, ept_entry_t *entry,
>>>>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>>>>>              break;
>>>>>>          case p2m_grant_map_ro:
>>>>>> -        case p2m_mmio_write_dm:
>>>>>> +        case p2m_ioreq_server:
>>>>>>              entry->r = 1;
>>>>>>              entry->w = entry->x = 0;
>>>>>>              entry->a = !!cpu_has_vmx_ept_ad;
>>>>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>>>>> index 3d80612..eabd2e3 100644
>>>>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>>>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>>>>> @@ -94,7 +94,7 @@ static unsigned long
>>> p2m_type_to_flags(p2m_type_t
>>>>> t, mfn_t mfn,
>>>>>>      default:
>>>>>>          return flags | _PAGE_NX_BIT;
>>>>>>      case p2m_grant_map_ro:
>>>>>> -    case p2m_mmio_write_dm:
>>>>>> +    case p2m_ioreq_server:
>>>>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>>>>      case p2m_ram_ro:
>>>>>>      case p2m_ram_logdirty:
>>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>>>> index e5c8499..c81302a 100644
>>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>>>>>
>>>>>>      /* Need to hand off device-model MMIO to the device model */
>>>>>>      if ( p2mt == p2m_mmio_dm
>>>>>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
>>>>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>>>>>>      {
>>>>>>          gpa = guest_walk_to_gpa(&gw);
>>>>>>          goto mmio;
>>>>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>>>>> index 5392eb0..ee2ea9c 100644
>>>>>> --- a/xen/include/asm-x86/p2m.h
>>>>>> +++ b/xen/include/asm-x86/p2m.h
>>>>>> @@ -71,7 +71,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_ioreq_server = 15,
>>>>>>  } p2m_type_t;
>>>>>>
>>>>>>  /* Modifiers to the query */
>>>>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>>>>>>                        | p2m_to_mask(p2m_ram_ro)         \
>>>>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>>>>>>                        | p2m_to_mask(p2m_ram_shared)     \
>>>>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>>>>>> +                      | p2m_to_mask(p2m_ioreq_server))
>>>>>>
>>>>>>  /* Write-discard types, which should discard the write operations */
>>>>>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
>>>>>> diff --git a/xen/include/public/hvm/hvm_op.h
>>>>> b/xen/include/public/hvm/hvm_op.h
>>>>>> index 1606185..b3e45cf 100644
>>>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>>>> @@ -83,7 +83,13 @@ typedef enum {
>>>>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>>>>>      HVMMEM_ram_ro,             /* Read-only; writes are discarded */
>>>>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device
>>> model
>>>>> */
>>>>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device
>>>>> model */
>>>>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>>>>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device
>>>>> model */
>>>>>> +#else
>>>>>> +    HVMMEM_unused,             /* Placeholder; setting memory to this
>>> type
>>>>>> +                                  will fail for code after 4.7.0 */
>>>>>> +#endif
>>>>>> +    HVMMEM_ioreq_server
>>>>>
>>>>> Also, I don't think we've had a convincing argument for why this patch
>>>>> needs to be in 4.7.  The p2m name changes are internal only, and so
>>>>> don't need to be made at all; and the old functionality will work as
>>>>> well as it ever did.  Furthermore, the whole reason we're in this
>>>>> situation is that we checked in a publicly-visible change to the
>>>>> interface before it was properly ready; I think we should avoid making
>>>>> the same mistake this time.
>>>>>
>>>>> So personally I'd just leave this patch entirely for 4.8; but if Paul
>>>>> and/or Jan have strong opinions, then I would say check in only a
>>>>> patch to do the #if/#else/#endif, and leave both the p2m type changes
>>>>> and the new HVMMEM_ioreq_server enum for when the 4.8
>>> development
>>>>> window opens.
>>>>>
>>>>
>>>> If the whole series is going in then I think this patch is ok. If this the
>> only
>>> patch that is going in for 4.7 then I thing we need the patch to hvm_op.h to
>>> deprecate the old type and that's it. We definitely should not introduce an
>>> implementation of the type HVMMEM_ioreq_server that has the same
>>> hardcoded semantics as the old type and then change it.
>>>> The p2m type changes are also wrong. That type needs to be left alone,
>>> presumably, so that anything using HVMMEM_mmio_write_dm and
>>> compiled to the old interface version continues to function. I think
>>> HVMMEM_ioreq_server needs to map to a new p2m type which should be
>>> introduced in patch #3.
>>>>
>>>
>>> Sorry, I'm also confused now. :(
>>>
>>> Do we really want to introduce a new p2m type? Why?
>>> My understanding of the previous agreement is that:
>>> 1> We need the interface to work on old hypervisor for
>>> HVMMEM_mmio_write_dm;
>>> 2> We need the interface to return -EINVAL for new hypervisor
>>> for HVMMEM_mmio_write_dm;
>>> 3> We need the new type HVMMEM_ioreq_server to work on new
>>> hypervisor;
>>>
>>> Did I miss something? Or I totally misunderstood?
>>>
>>
>> I don't know. I'm confused too. What we definitely don't want to do is add a
>> new HVMMEM type and have it map to the old behaviour, otherwise we're no
>> better off.
>>
>> The question I'm not clear on the answer to is what happens to old code:
>>
>> Should it continue to compile? If so, should it continue to run.
>
> We only need to be concerned about the "get type" functionality,
> as that's the only thing an arbitrary guest can use. If the
> hypercall simply never returns the old type, then old code will
> still work (it'll just have some dead code on new Xen), and hence
> it compiling against the older interface is fine (and, from general
> considerations, a requirement).
>

Thanks Jan. And I think the answer is yes. The new hypervisor will
only return HVMMEM_ioreq_server, which is a different value now.

> Jan
>

Yu

George Dunlap April 25, 2016, 4:15 p.m. UTC | #16

On 25/04/16 16:53, Yu, Zhang wrote:
> 
> 
> On 4/25/2016 11:38 PM, Jan Beulich wrote:
>>>>> On 25.04.16 at 17:29, <[e-mail]> wrote:
>>>>  -----Original Message-----
>>>> From: Yu, Zhang [mailto:[e-mail]]
>>>> Sent: 25 April 2016 16:22
>>>> To: Paul Durrant; George Dunlap
>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>>>> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu
>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
>>>> 4.7):
>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>
>>>>
>>>>
>>>> On 4/25/2016 10:01 PM, Paul Durrant wrote:
>>>>>> -----Original Message-----
>>>>>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>>>>>> George Dunlap
>>>>>> Sent: 25 April 2016 14:39
>>>>>> To: Yu Zhang
>>>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun
>>>>>> Nakajima;
>>>>>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan
>>>>>> Beulich; Wei
>>>> Liu
>>>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
>>>>>> 4.7):
>>>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>>>
>>>>>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang
>>>> <[e-mail]>
>>>>>> wrote:
>>>>>>> Previously p2m type p2m_mmio_write_dm was introduced for write-
>>>>>>> protected memory pages whose write operations are supposed to be
>>>>>>> forwarded to and emulated by an ioreq server. Yet limitations of
>>>>>>> rangeset restrict the number of guest pages to be write-protected.
>>>>>>>
>>>>>>> This patch replaces the p2m type p2m_mmio_write_dm with a new
>>>> name:
>>>>>>> p2m_ioreq_server, which means this p2m type can be claimed by one
>>>>>>> ioreq server, instead of being tracked inside the rangeset of ioreq
>>>>>>> server. Patches following up will add the related hvmop handling
>>>>>>> code which map/unmap type p2m_ioreq_server to/from an ioreq
>>>> server.
>>>>>>>
>>>>>>> changes in v3:
>>>>>>>   - According to Jan & George's comments, keep
>>>>>> HVMMEM_mmio_write_dm
>>>>>>>     for old xen interface versions, and replace it with
>>>>>>> HVMMEM_unused
>>>>>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>>>>>>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem
>>>> type
>>>>>>>     interfaces;
>>>>>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>>>>>>
>>>>>> Unfortunately these rather contradict each other -- I consider
>>>>>> Reviewed-by to only stick if the code I've specified hasn't changed
>>>>>> (or has only changed trivially).
>>>>>>
>>>>>> Also...
>>>>>>
>>>>>>>
>>>>>>> changes in v2:
>>>>>>>   - According to George Dunlap's comments, only rename the p2m type,
>>>>>>>     with no behavior changes.
>>>>>>>
>>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>>>> Signed-off-by: Yu Zhang <[e-mail]>
>>>>>>> Acked-by: Tim Deegan <[e-mail]>
>>>>>>> Acked-by: Andrew Cooper <[e-mail]>
>>>>>>> Reviewed-by: George Dunlap <[e-mail]>
>>>>>>> Cc: Keir Fraser <[e-mail]>
>>>>>>> Cc: Jan Beulich <[e-mail]>
>>>>>>> Cc: Andrew Cooper <[e-mail]>
>>>>>>> Cc: Jun Nakajima <[e-mail]>
>>>>>>> Cc: Kevin Tian <[e-mail]>
>>>>>>> Cc: George Dunlap <[e-mail]>
>>>>>>> Cc: Tim Deegan <[e-mail]>
>>>>>>> ---
>>>>>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>>>>>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>>>>>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>>>>>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>>>>>>>  xen/include/asm-x86/p2m.h       |  4 ++--
>>>>>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>>>>>>>  6 files changed, 20 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>>>> index f24126d..874cb0f 100644
>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>>>>>> unsigned long gla,
>>>>>>>       */
>>>>>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>>>>>           (npfec.write_access &&
>>>>>>> -          (p2m_is_discard_write(p2mt) || (p2mt ==
>>>> p2m_mmio_write_dm))) )
>>>>>>> +          (p2m_is_discard_write(p2mt) || (p2mt ==
>>>>>>> p2m_ioreq_server))) )
>>>>>>>      {
>>>>>>>          __put_gfn(p2m, gfn);
>>>>>>>          if ( ap2m_active )
>>>>>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>              get_gfn_query_unlocked(d, a.pfn, &t);
>>>>>>>              if ( p2m_is_mmio(t) )
>>>>>>>                  a.mem_type =  HVMMEM_mmio_dm;
>>>>>>> -            else if ( t == p2m_mmio_write_dm )
>>>>>>> -                a.mem_type = HVMMEM_mmio_write_dm;
>>>>>>> +            else if ( t == p2m_ioreq_server )
>>>>>>> +                a.mem_type = HVMMEM_ioreq_server;
>>>>>>>              else if ( p2m_is_readonly(t) )
>>>>>>>                  a.mem_type =  HVMMEM_ram_ro;
>>>>>>>              else if ( p2m_is_ram(t) )
>>>>>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>>>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>>>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>>>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>>>>>> +            [HVMMEM_unused] = p2m_invalid,
>>>>>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>>>>>>>          };
>>>>>>>
>>>>>>>          if ( copy_from_guest(&a, arg, 1) )
>>>>>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>               ((a.first_pfn + a.nr - 1) >
>>>>>>> domain_get_maximum_gpfn(d)) )
>>>>>>>              goto setmemtype_fail;
>>>>>>>
>>>>>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>>>>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>>>>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>>>>>>>              goto setmemtype_fail;
>>>>>>>
>>>>>>>          while ( a.nr > start_iter )
>>>>>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>              }
>>>>>>>              if ( !p2m_is_ram(t) &&
>>>>>>>                   (!p2m_is_hole(t) || a.hvmmem_type !=
>>>>>>> HVMMEM_mmio_dm)
>>>>>> &&
>>>>>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>>>>>> HVMMEM_ram_rw) )
>>>>>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>>>>>> HVMMEM_ram_rw) )
>>>>>>>              {
>>>>>>>                  put_gfn(d, pfn);
>>>>>>>                  goto setmemtype_fail;
>>>>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
>>>> ept.c
>>>>>>> index 3cb6868..380ec25 100644
>>>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>>>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>>>>>> p2m_domain *p2m, ept_entry_t *entry,
>>>>>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>>>>>>              break;
>>>>>>>          case p2m_grant_map_ro:
>>>>>>> -        case p2m_mmio_write_dm:
>>>>>>> +        case p2m_ioreq_server:
>>>>>>>              entry->r = 1;
>>>>>>>              entry->w = entry->x = 0;
>>>>>>>              entry->a = !!cpu_has_vmx_ept_ad;
>>>>>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>>>>>> index 3d80612..eabd2e3 100644
>>>>>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>>>>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>>>>>> @@ -94,7 +94,7 @@ static unsigned long
>>>> p2m_type_to_flags(p2m_type_t
>>>>>> t, mfn_t mfn,
>>>>>>>      default:
>>>>>>>          return flags | _PAGE_NX_BIT;
>>>>>>>      case p2m_grant_map_ro:
>>>>>>> -    case p2m_mmio_write_dm:
>>>>>>> +    case p2m_ioreq_server:
>>>>>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>>>>>      case p2m_ram_ro:
>>>>>>>      case p2m_ram_logdirty:
>>>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>>>>> index e5c8499..c81302a 100644
>>>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>>>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>>>>>>
>>>>>>>      /* Need to hand off device-model MMIO to the device model */
>>>>>>>      if ( p2mt == p2m_mmio_dm
>>>>>>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
>>>>>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>>>>>>>      {
>>>>>>>          gpa = guest_walk_to_gpa(&gw);
>>>>>>>          goto mmio;
>>>>>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>>>>>> index 5392eb0..ee2ea9c 100644
>>>>>>> --- a/xen/include/asm-x86/p2m.h
>>>>>>> +++ b/xen/include/asm-x86/p2m.h
>>>>>>> @@ -71,7 +71,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_ioreq_server = 15,
>>>>>>>  } p2m_type_t;
>>>>>>>
>>>>>>>  /* Modifiers to the query */
>>>>>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>>>>>>>                        | p2m_to_mask(p2m_ram_ro)         \
>>>>>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>>>>>>>                        | p2m_to_mask(p2m_ram_shared)     \
>>>>>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>>>>>>> +                      | p2m_to_mask(p2m_ioreq_server))
>>>>>>>
>>>>>>>  /* Write-discard types, which should discard the write
>>>>>>> operations */
>>>>>>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
>>>>>>> diff --git a/xen/include/public/hvm/hvm_op.h
>>>>>> b/xen/include/public/hvm/hvm_op.h
>>>>>>> index 1606185..b3e45cf 100644
>>>>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>>>>> @@ -83,7 +83,13 @@ typedef enum {
>>>>>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>>>>>>      HVMMEM_ram_ro,             /* Read-only; writes are
>>>>>>> discarded */
>>>>>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device
>>>> model
>>>>>> */
>>>>>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the
>>>>>>> device
>>>>>> model */
>>>>>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>>>>>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the
>>>>>>> device
>>>>>> model */
>>>>>>> +#else
>>>>>>> +    HVMMEM_unused,             /* Placeholder; setting memory to
>>>>>>> this
>>>> type
>>>>>>> +                                  will fail for code after 4.7.0 */
>>>>>>> +#endif
>>>>>>> +    HVMMEM_ioreq_server
>>>>>>
>>>>>> Also, I don't think we've had a convincing argument for why this
>>>>>> patch
>>>>>> needs to be in 4.7.  The p2m name changes are internal only, and so
>>>>>> don't need to be made at all; and the old functionality will work as
>>>>>> well as it ever did.  Furthermore, the whole reason we're in this
>>>>>> situation is that we checked in a publicly-visible change to the
>>>>>> interface before it was properly ready; I think we should avoid
>>>>>> making
>>>>>> the same mistake this time.
>>>>>>
>>>>>> So personally I'd just leave this patch entirely for 4.8; but if Paul
>>>>>> and/or Jan have strong opinions, then I would say check in only a
>>>>>> patch to do the #if/#else/#endif, and leave both the p2m type changes
>>>>>> and the new HVMMEM_ioreq_server enum for when the 4.8
>>>> development
>>>>>> window opens.
>>>>>>
>>>>>
>>>>> If the whole series is going in then I think this patch is ok. If
>>>>> this the
>>> only
>>>> patch that is going in for 4.7 then I thing we need the patch to
>>>> hvm_op.h to
>>>> deprecate the old type and that's it. We definitely should not
>>>> introduce an
>>>> implementation of the type HVMMEM_ioreq_server that has the same
>>>> hardcoded semantics as the old type and then change it.
>>>>> The p2m type changes are also wrong. That type needs to be left alone,
>>>> presumably, so that anything using HVMMEM_mmio_write_dm and
>>>> compiled to the old interface version continues to function. I think
>>>> HVMMEM_ioreq_server needs to map to a new p2m type which should be
>>>> introduced in patch #3.
>>>>>
>>>>
>>>> Sorry, I'm also confused now. :(
>>>>
>>>> Do we really want to introduce a new p2m type? Why?
>>>> My understanding of the previous agreement is that:
>>>> 1> We need the interface to work on old hypervisor for
>>>> HVMMEM_mmio_write_dm;
>>>> 2> We need the interface to return -EINVAL for new hypervisor
>>>> for HVMMEM_mmio_write_dm;
>>>> 3> We need the new type HVMMEM_ioreq_server to work on new
>>>> hypervisor;
>>>>
>>>> Did I miss something? Or I totally misunderstood?
>>>>
>>>
>>> I don't know. I'm confused too. What we definitely don't want to do
>>> is add a
>>> new HVMMEM type and have it map to the old behaviour, otherwise we're no
>>> better off.
>>>
>>> The question I'm not clear on the answer to is what happens to old code:
>>>
>>> Should it continue to compile? If so, should it continue to run.
>>
>> We only need to be concerned about the "get type" functionality,
>> as that's the only thing an arbitrary guest can use. If the
>> hypercall simply never returns the old type, then old code will
>> still work (it'll just have some dead code on new Xen), and hence
>> it compiling against the older interface is fine (and, from general
>> considerations, a requirement).
>>
> 
> Thanks Jan. And I think the answer is yes. The new hypervisor will
> only return HVMMEM_ioreq_server, which is a different value now.

Right -- but we can't check in this entire series for 4.7; however, Paul
would like to make it clear that HVMMEM_mmio_write_dm is deprecated; so
we need a patch which does the #ifdef/#else/#endif clause, and then
everything else necessary to make sure that things compile properly
either way, but no p2m changes or additional functionality.

 -George

Yu Zhang April 25, 2016, 4:20 p.m. UTC | #17

On 4/26/2016 12:15 AM, George Dunlap wrote:
> On 25/04/16 16:53, Yu, Zhang wrote:
>>
>>
>> On 4/25/2016 11:38 PM, Jan Beulich wrote:
>>>>>> On 25.04.16 at 17:29, <[e-mail]> wrote:
>>>>>  -----Original Message-----
>>>>> From: Yu, Zhang [mailto:[e-mail]]
>>>>> Sent: 25 April 2016 16:22
>>>>> To: Paul Durrant; George Dunlap
>>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>>>>> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu
>>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
>>>>> 4.7):
>>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>>
>>>>>
>>>>>
>>>>> On 4/25/2016 10:01 PM, Paul Durrant wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>>>>>>> George Dunlap
>>>>>>> Sent: 25 April 2016 14:39
>>>>>>> To: Yu Zhang
>>>>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun
>>>>>>> Nakajima;
>>>>>>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan
>>>>>>> Beulich; Wei
>>>>> Liu
>>>>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
>>>>>>> 4.7):
>>>>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>>>>
>>>>>>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang
>>>>> <[e-mail]>
>>>>>>> wrote:
>>>>>>>> Previously p2m type p2m_mmio_write_dm was introduced for write-
>>>>>>>> protected memory pages whose write operations are supposed to be
>>>>>>>> forwarded to and emulated by an ioreq server. Yet limitations of
>>>>>>>> rangeset restrict the number of guest pages to be write-protected.
>>>>>>>>
>>>>>>>> This patch replaces the p2m type p2m_mmio_write_dm with a new
>>>>> name:
>>>>>>>> p2m_ioreq_server, which means this p2m type can be claimed by one
>>>>>>>> ioreq server, instead of being tracked inside the rangeset of ioreq
>>>>>>>> server. Patches following up will add the related hvmop handling
>>>>>>>> code which map/unmap type p2m_ioreq_server to/from an ioreq
>>>>> server.
>>>>>>>>
>>>>>>>> changes in v3:
>>>>>>>>   - According to Jan & George's comments, keep
>>>>>>> HVMMEM_mmio_write_dm
>>>>>>>>     for old xen interface versions, and replace it with
>>>>>>>> HVMMEM_unused
>>>>>>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a new
>>>>>>>>     enum - HVMMEM_ioreq_server is introduced for the get/set mem
>>>>> type
>>>>>>>>     interfaces;
>>>>>>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>>>>>>>
>>>>>>> Unfortunately these rather contradict each other -- I consider
>>>>>>> Reviewed-by to only stick if the code I've specified hasn't changed
>>>>>>> (or has only changed trivially).
>>>>>>>
>>>>>>> Also...
>>>>>>>
>>>>>>>>
>>>>>>>> changes in v2:
>>>>>>>>   - According to George Dunlap's comments, only rename the p2m type,
>>>>>>>>     with no behavior changes.
>>>>>>>>
>>>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>>>>> Signed-off-by: Yu Zhang <[e-mail]>
>>>>>>>> Acked-by: Tim Deegan <[e-mail]>
>>>>>>>> Acked-by: Andrew Cooper <[e-mail]>
>>>>>>>> Reviewed-by: George Dunlap <[e-mail]>
>>>>>>>> Cc: Keir Fraser <[e-mail]>
>>>>>>>> Cc: Jan Beulich <[e-mail]>
>>>>>>>> Cc: Andrew Cooper <[e-mail]>
>>>>>>>> Cc: Jun Nakajima <[e-mail]>
>>>>>>>> Cc: Kevin Tian <[e-mail]>
>>>>>>>> Cc: George Dunlap <[e-mail]>
>>>>>>>> Cc: Tim Deegan <[e-mail]>
>>>>>>>> ---
>>>>>>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>>>>>>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>>>>>>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>>>>>>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>>>>>>>>  xen/include/asm-x86/p2m.h       |  4 ++--
>>>>>>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>>>>>>>>  6 files changed, 20 insertions(+), 12 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>>>>> index f24126d..874cb0f 100644
>>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>>>>>>> unsigned long gla,
>>>>>>>>       */
>>>>>>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>>>>>>           (npfec.write_access &&
>>>>>>>> -          (p2m_is_discard_write(p2mt) || (p2mt ==
>>>>> p2m_mmio_write_dm))) )
>>>>>>>> +          (p2m_is_discard_write(p2mt) || (p2mt ==
>>>>>>>> p2m_ioreq_server))) )
>>>>>>>>      {
>>>>>>>>          __put_gfn(p2m, gfn);
>>>>>>>>          if ( ap2m_active )
>>>>>>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>              get_gfn_query_unlocked(d, a.pfn, &t);
>>>>>>>>              if ( p2m_is_mmio(t) )
>>>>>>>>                  a.mem_type =  HVMMEM_mmio_dm;
>>>>>>>> -            else if ( t == p2m_mmio_write_dm )
>>>>>>>> -                a.mem_type = HVMMEM_mmio_write_dm;
>>>>>>>> +            else if ( t == p2m_ioreq_server )
>>>>>>>> +                a.mem_type = HVMMEM_ioreq_server;
>>>>>>>>              else if ( p2m_is_readonly(t) )
>>>>>>>>                  a.mem_type =  HVMMEM_ram_ro;
>>>>>>>>              else if ( p2m_is_ram(t) )
>>>>>>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>>>>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>>>>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>>>>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>>>>>>> +            [HVMMEM_unused] = p2m_invalid,
>>>>>>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>>>>>>>>          };
>>>>>>>>
>>>>>>>>          if ( copy_from_guest(&a, arg, 1) )
>>>>>>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>               ((a.first_pfn + a.nr - 1) >
>>>>>>>> domain_get_maximum_gpfn(d)) )
>>>>>>>>              goto setmemtype_fail;
>>>>>>>>
>>>>>>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>>>>>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>>>>>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>>>>>>>>              goto setmemtype_fail;
>>>>>>>>
>>>>>>>>          while ( a.nr > start_iter )
>>>>>>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>              }
>>>>>>>>              if ( !p2m_is_ram(t) &&
>>>>>>>>                   (!p2m_is_hole(t) || a.hvmmem_type !=
>>>>>>>> HVMMEM_mmio_dm)
>>>>>>> &&
>>>>>>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>>>>>>> HVMMEM_ram_rw) )
>>>>>>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>>>>>>> HVMMEM_ram_rw) )
>>>>>>>>              {
>>>>>>>>                  put_gfn(d, pfn);
>>>>>>>>                  goto setmemtype_fail;
>>>>>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-
>>>>> ept.c
>>>>>>>> index 3cb6868..380ec25 100644
>>>>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>>>>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>>>>>>> p2m_domain *p2m, ept_entry_t *entry,
>>>>>>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>>>>>>>              break;
>>>>>>>>          case p2m_grant_map_ro:
>>>>>>>> -        case p2m_mmio_write_dm:
>>>>>>>> +        case p2m_ioreq_server:
>>>>>>>>              entry->r = 1;
>>>>>>>>              entry->w = entry->x = 0;
>>>>>>>>              entry->a = !!cpu_has_vmx_ept_ad;
>>>>>>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
>>>>>>>> index 3d80612..eabd2e3 100644
>>>>>>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>>>>>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>>>>>>> @@ -94,7 +94,7 @@ static unsigned long
>>>>> p2m_type_to_flags(p2m_type_t
>>>>>>> t, mfn_t mfn,
>>>>>>>>      default:
>>>>>>>>          return flags | _PAGE_NX_BIT;
>>>>>>>>      case p2m_grant_map_ro:
>>>>>>>> -    case p2m_mmio_write_dm:
>>>>>>>> +    case p2m_ioreq_server:
>>>>>>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>>>>>>      case p2m_ram_ro:
>>>>>>>>      case p2m_ram_logdirty:
>>>>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>>>>>> index e5c8499..c81302a 100644
>>>>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>>>>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>>>>>>>
>>>>>>>>      /* Need to hand off device-model MMIO to the device model */
>>>>>>>>      if ( p2mt == p2m_mmio_dm
>>>>>>>> -         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
>>>>>>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>>>>>>>>      {
>>>>>>>>          gpa = guest_walk_to_gpa(&gw);
>>>>>>>>          goto mmio;
>>>>>>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>>>>>>>> index 5392eb0..ee2ea9c 100644
>>>>>>>> --- a/xen/include/asm-x86/p2m.h
>>>>>>>> +++ b/xen/include/asm-x86/p2m.h
>>>>>>>> @@ -71,7 +71,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_ioreq_server = 15,
>>>>>>>>  } p2m_type_t;
>>>>>>>>
>>>>>>>>  /* Modifiers to the query */
>>>>>>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>>>>>>>>                        | p2m_to_mask(p2m_ram_ro)         \
>>>>>>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>>>>>>>>                        | p2m_to_mask(p2m_ram_shared)     \
>>>>>>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>>>>>>>> +                      | p2m_to_mask(p2m_ioreq_server))
>>>>>>>>
>>>>>>>>  /* Write-discard types, which should discard the write
>>>>>>>> operations */
>>>>>>>>  #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
>>>>>>>> diff --git a/xen/include/public/hvm/hvm_op.h
>>>>>>> b/xen/include/public/hvm/hvm_op.h
>>>>>>>> index 1606185..b3e45cf 100644
>>>>>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>>>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>>>>>> @@ -83,7 +83,13 @@ typedef enum {
>>>>>>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>>>>>>>      HVMMEM_ram_ro,             /* Read-only; writes are
>>>>>>>> discarded */
>>>>>>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device
>>>>> model
>>>>>>> */
>>>>>>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the
>>>>>>>> device
>>>>>>> model */
>>>>>>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>>>>>>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the
>>>>>>>> device
>>>>>>> model */
>>>>>>>> +#else
>>>>>>>> +    HVMMEM_unused,             /* Placeholder; setting memory to
>>>>>>>> this
>>>>> type
>>>>>>>> +                                  will fail for code after 4.7.0 */
>>>>>>>> +#endif
>>>>>>>> +    HVMMEM_ioreq_server
>>>>>>>
>>>>>>> Also, I don't think we've had a convincing argument for why this
>>>>>>> patch
>>>>>>> needs to be in 4.7.  The p2m name changes are internal only, and so
>>>>>>> don't need to be made at all; and the old functionality will work as
>>>>>>> well as it ever did.  Furthermore, the whole reason we're in this
>>>>>>> situation is that we checked in a publicly-visible change to the
>>>>>>> interface before it was properly ready; I think we should avoid
>>>>>>> making
>>>>>>> the same mistake this time.
>>>>>>>
>>>>>>> So personally I'd just leave this patch entirely for 4.8; but if Paul
>>>>>>> and/or Jan have strong opinions, then I would say check in only a
>>>>>>> patch to do the #if/#else/#endif, and leave both the p2m type changes
>>>>>>> and the new HVMMEM_ioreq_server enum for when the 4.8
>>>>> development
>>>>>>> window opens.
>>>>>>>
>>>>>>
>>>>>> If the whole series is going in then I think this patch is ok. If
>>>>>> this the
>>>> only
>>>>> patch that is going in for 4.7 then I thing we need the patch to
>>>>> hvm_op.h to
>>>>> deprecate the old type and that's it. We definitely should not
>>>>> introduce an
>>>>> implementation of the type HVMMEM_ioreq_server that has the same
>>>>> hardcoded semantics as the old type and then change it.
>>>>>> The p2m type changes are also wrong. That type needs to be left alone,
>>>>> presumably, so that anything using HVMMEM_mmio_write_dm and
>>>>> compiled to the old interface version continues to function. I think
>>>>> HVMMEM_ioreq_server needs to map to a new p2m type which should be
>>>>> introduced in patch #3.
>>>>>>
>>>>>
>>>>> Sorry, I'm also confused now. :(
>>>>>
>>>>> Do we really want to introduce a new p2m type? Why?
>>>>> My understanding of the previous agreement is that:
>>>>> 1> We need the interface to work on old hypervisor for
>>>>> HVMMEM_mmio_write_dm;
>>>>> 2> We need the interface to return -EINVAL for new hypervisor
>>>>> for HVMMEM_mmio_write_dm;
>>>>> 3> We need the new type HVMMEM_ioreq_server to work on new
>>>>> hypervisor;
>>>>>
>>>>> Did I miss something? Or I totally misunderstood?
>>>>>
>>>>
>>>> I don't know. I'm confused too. What we definitely don't want to do
>>>> is add a
>>>> new HVMMEM type and have it map to the old behaviour, otherwise we're no
>>>> better off.
>>>>
>>>> The question I'm not clear on the answer to is what happens to old code:
>>>>
>>>> Should it continue to compile? If so, should it continue to run.
>>>
>>> We only need to be concerned about the "get type" functionality,
>>> as that's the only thing an arbitrary guest can use. If the
>>> hypercall simply never returns the old type, then old code will
>>> still work (it'll just have some dead code on new Xen), and hence
>>> it compiling against the older interface is fine (and, from general
>>> considerations, a requirement).
>>>
>>
>> Thanks Jan. And I think the answer is yes. The new hypervisor will
>> only return HVMMEM_ioreq_server, which is a different value now.
>
> Right -- but we can't check in this entire series for 4.7; however, Paul
> would like to make it clear that HVMMEM_mmio_write_dm is deprecated; so
> we need a patch which does the #ifdef/#else/#endif clause, and then
> everything else necessary to make sure that things compile properly
> either way, but no p2m changes or additional functionality.
>

Thanks, George.
So what is it good for, with only the #ifdef/#els/#endif clause? It
seems more reasonable to me that we hold all 3 patches till next
version, and at that time the __XEN_INTERFACE_VERSION__ should be
compared against 0x000408000 in the #ifdef I guess?

Yu

Paul Durrant April 25, 2016, 5:01 p.m. UTC | #18

> -----Original Message-----
> From: George Dunlap [mailto:[e-mail]]
> Sent: 25 April 2016 17:16
> To: Yu, Zhang; Jan Beulich; Paul Durrant
> Cc: Andrew Cooper; Wei Liu; Jun Nakajima; Kevin Tian; Zhiyuan Lv; xen-
> [e-mail]; Keir (Xen.org); Tim (Xen.org)
> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
> Rename p2m_mmio_write_dm to p2m_ioreq_server.
> 
> On 25/04/16 16:53, Yu, Zhang wrote:
> >
> >
> > On 4/25/2016 11:38 PM, Jan Beulich wrote:
> >>>>> On 25.04.16 at 17:29, <[e-mail]> wrote:
> >>>>  -----Original Message-----
> >>>> From: Yu, Zhang [mailto:[e-mail]]
> >>>> Sent: 25 April 2016 16:22
> >>>> To: Paul Durrant; George Dunlap
> >>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
> >>>> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu
> >>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
> >>>> 4.7):
> >>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
> >>>>
> >>>>
> >>>>
> >>>> On 4/25/2016 10:01 PM, Paul Durrant wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of
> >>>>>> George Dunlap
> >>>>>> Sent: 25 April 2016 14:39
> >>>>>> To: Yu Zhang
> >>>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun
> >>>>>> Nakajima;
> >>>>>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan
> >>>>>> Beulich; Wei
> >>>> Liu
> >>>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
> >>>>>> 4.7):
> >>>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
> >>>>>>
> >>>>>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang
> >>>> <[e-mail]>
> >>>>>> wrote:
> >>>>>>> Previously p2m type p2m_mmio_write_dm was introduced for
> write-
> >>>>>>> protected memory pages whose write operations are supposed to
> be
> >>>>>>> forwarded to and emulated by an ioreq server. Yet limitations of
> >>>>>>> rangeset restrict the number of guest pages to be write-protected.
> >>>>>>>
> >>>>>>> This patch replaces the p2m type p2m_mmio_write_dm with a new
> >>>> name:
> >>>>>>> p2m_ioreq_server, which means this p2m type can be claimed by
> one
> >>>>>>> ioreq server, instead of being tracked inside the rangeset of ioreq
> >>>>>>> server. Patches following up will add the related hvmop handling
> >>>>>>> code which map/unmap type p2m_ioreq_server to/from an ioreq
> >>>> server.
> >>>>>>>
> >>>>>>> changes in v3:
> >>>>>>>   - According to Jan & George's comments, keep
> >>>>>> HVMMEM_mmio_write_dm
> >>>>>>>     for old xen interface versions, and replace it with
> >>>>>>> HVMMEM_unused
> >>>>>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a
> new
> >>>>>>>     enum - HVMMEM_ioreq_server is introduced for the get/set
> mem
> >>>> type
> >>>>>>>     interfaces;
> >>>>>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
> >>>>>>
> >>>>>> Unfortunately these rather contradict each other -- I consider
> >>>>>> Reviewed-by to only stick if the code I've specified hasn't changed
> >>>>>> (or has only changed trivially).
> >>>>>>
> >>>>>> Also...
> >>>>>>
> >>>>>>>
> >>>>>>> changes in v2:
> >>>>>>>   - According to George Dunlap's comments, only rename the p2m
> type,
> >>>>>>>     with no behavior changes.
> >>>>>>>
> >>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
> >>>>>>> Signed-off-by: Yu Zhang <[e-mail]>
> >>>>>>> Acked-by: Tim Deegan <[e-mail]>
> >>>>>>> Acked-by: Andrew Cooper <[e-mail]>
> >>>>>>> Reviewed-by: George Dunlap <[e-mail]>
> >>>>>>> Cc: Keir Fraser <[e-mail]>
> >>>>>>> Cc: Jan Beulich <[e-mail]>
> >>>>>>> Cc: Andrew Cooper <[e-mail]>
> >>>>>>> Cc: Jun Nakajima <[e-mail]>
> >>>>>>> Cc: Kevin Tian <[e-mail]>
> >>>>>>> Cc: George Dunlap <[e-mail]>
> >>>>>>> Cc: Tim Deegan <[e-mail]>
> >>>>>>> ---
> >>>>>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
> >>>>>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
> >>>>>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
> >>>>>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
> >>>>>>>  xen/include/asm-x86/p2m.h       |  4 ++--
> >>>>>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
> >>>>>>>  6 files changed, 20 insertions(+), 12 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> >>>>>>> index f24126d..874cb0f 100644
> >>>>>>> --- a/xen/arch/x86/hvm/hvm.c
> >>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
> >>>>>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t
> gpa,
> >>>>>> unsigned long gla,
> >>>>>>>       */
> >>>>>>>      if ( (p2mt == p2m_mmio_dm) ||
> >>>>>>>           (npfec.write_access &&
> >>>>>>> -          (p2m_is_discard_write(p2mt) || (p2mt ==
> >>>> p2m_mmio_write_dm))) )
> >>>>>>> +          (p2m_is_discard_write(p2mt) || (p2mt ==
> >>>>>>> p2m_ioreq_server))) )
> >>>>>>>      {
> >>>>>>>          __put_gfn(p2m, gfn);
> >>>>>>>          if ( ap2m_active )
> >>>>>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
> >>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>>>              get_gfn_query_unlocked(d, a.pfn, &t);
> >>>>>>>              if ( p2m_is_mmio(t) )
> >>>>>>>                  a.mem_type =  HVMMEM_mmio_dm;
> >>>>>>> -            else if ( t == p2m_mmio_write_dm )
> >>>>>>> -                a.mem_type = HVMMEM_mmio_write_dm;
> >>>>>>> +            else if ( t == p2m_ioreq_server )
> >>>>>>> +                a.mem_type = HVMMEM_ioreq_server;
> >>>>>>>              else if ( p2m_is_readonly(t) )
> >>>>>>>                  a.mem_type =  HVMMEM_ram_ro;
> >>>>>>>              else if ( p2m_is_ram(t) )
> >>>>>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
> >>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
> >>>>>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
> >>>>>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
> >>>>>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
> >>>>>>> +            [HVMMEM_unused] = p2m_invalid,
> >>>>>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
> >>>>>>>          };
> >>>>>>>
> >>>>>>>          if ( copy_from_guest(&a, arg, 1) )
> >>>>>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
> >>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>>>               ((a.first_pfn + a.nr - 1) >
> >>>>>>> domain_get_maximum_gpfn(d)) )
> >>>>>>>              goto setmemtype_fail;
> >>>>>>>
> >>>>>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> >>>>>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
> >>>>>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
> >>>>>>>              goto setmemtype_fail;
> >>>>>>>
> >>>>>>>          while ( a.nr > start_iter )
> >>>>>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
> >>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>>>              }
> >>>>>>>              if ( !p2m_is_ram(t) &&
> >>>>>>>                   (!p2m_is_hole(t) || a.hvmmem_type !=
> >>>>>>> HVMMEM_mmio_dm)
> >>>>>> &&
> >>>>>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
> >>>>>> HVMMEM_ram_rw) )
> >>>>>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
> >>>>>> HVMMEM_ram_rw) )
> >>>>>>>              {
> >>>>>>>                  put_gfn(d, pfn);
> >>>>>>>                  goto setmemtype_fail;
> >>>>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c
> b/xen/arch/x86/mm/p2m-
> >>>> ept.c
> >>>>>>> index 3cb6868..380ec25 100644
> >>>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
> >>>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
> >>>>>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
> >>>>>> p2m_domain *p2m, ept_entry_t *entry,
> >>>>>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
> >>>>>>>              break;
> >>>>>>>          case p2m_grant_map_ro:
> >>>>>>> -        case p2m_mmio_write_dm:
> >>>>>>> +        case p2m_ioreq_server:
> >>>>>>>              entry->r = 1;
> >>>>>>>              entry->w = entry->x = 0;
> >>>>>>>              entry->a = !!cpu_has_vmx_ept_ad;
> >>>>>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-
> pt.c
> >>>>>>> index 3d80612..eabd2e3 100644
> >>>>>>> --- a/xen/arch/x86/mm/p2m-pt.c
> >>>>>>> +++ b/xen/arch/x86/mm/p2m-pt.c
> >>>>>>> @@ -94,7 +94,7 @@ static unsigned long
> >>>> p2m_type_to_flags(p2m_type_t
> >>>>>> t, mfn_t mfn,
> >>>>>>>      default:
> >>>>>>>          return flags | _PAGE_NX_BIT;
> >>>>>>>      case p2m_grant_map_ro:
> >>>>>>> -    case p2m_mmio_write_dm:
> >>>>>>> +    case p2m_ioreq_server:
> >>>>>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
> >>>>>>>      case p2m_ram_ro:
> >>>>>>>      case p2m_ram_logdirty:
> >>>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
> >>>>>> b/xen/arch/x86/mm/shadow/multi.c
> >>>>>>> index e5c8499..c81302a 100644
> >>>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
> >>>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
> >>>>>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
> >>>>>>>
> >>>>>>>      /* Need to hand off device-model MMIO to the device model */
> >>>>>>>      if ( p2mt == p2m_mmio_dm
> >>>>>>> -         || (p2mt == p2m_mmio_write_dm && ft ==
> ft_demand_write) )
> >>>>>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
> >>>>>>>      {
> >>>>>>>          gpa = guest_walk_to_gpa(&gw);
> >>>>>>>          goto mmio;
> >>>>>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-
> x86/p2m.h
> >>>>>>> index 5392eb0..ee2ea9c 100644
> >>>>>>> --- a/xen/include/asm-x86/p2m.h
> >>>>>>> +++ b/xen/include/asm-x86/p2m.h
> >>>>>>> @@ -71,7 +71,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_ioreq_server = 15,
> >>>>>>>  } p2m_type_t;
> >>>>>>>
> >>>>>>>  /* Modifiers to the query */
> >>>>>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
> >>>>>>>                        | p2m_to_mask(p2m_ram_ro)         \
> >>>>>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
> >>>>>>>                        | p2m_to_mask(p2m_ram_shared)     \
> >>>>>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
> >>>>>>> +                      | p2m_to_mask(p2m_ioreq_server))
> >>>>>>>
> >>>>>>>  /* Write-discard types, which should discard the write
> >>>>>>> operations */
> >>>>>>>  #define P2M_DISCARD_WRITE_TYPES
> (p2m_to_mask(p2m_ram_ro)     \
> >>>>>>> diff --git a/xen/include/public/hvm/hvm_op.h
> >>>>>> b/xen/include/public/hvm/hvm_op.h
> >>>>>>> index 1606185..b3e45cf 100644
> >>>>>>> --- a/xen/include/public/hvm/hvm_op.h
> >>>>>>> +++ b/xen/include/public/hvm/hvm_op.h
> >>>>>>> @@ -83,7 +83,13 @@ typedef enum {
> >>>>>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
> >>>>>>>      HVMMEM_ram_ro,             /* Read-only; writes are
> >>>>>>> discarded */
> >>>>>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device
> >>>> model
> >>>>>> */
> >>>>>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the
> >>>>>>> device
> >>>>>> model */
> >>>>>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
> >>>>>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the
> >>>>>>> device
> >>>>>> model */
> >>>>>>> +#else
> >>>>>>> +    HVMMEM_unused,             /* Placeholder; setting memory to
> >>>>>>> this
> >>>> type
> >>>>>>> +                                  will fail for code after 4.7.0 */
> >>>>>>> +#endif
> >>>>>>> +    HVMMEM_ioreq_server
> >>>>>>
> >>>>>> Also, I don't think we've had a convincing argument for why this
> >>>>>> patch
> >>>>>> needs to be in 4.7.  The p2m name changes are internal only, and so
> >>>>>> don't need to be made at all; and the old functionality will work as
> >>>>>> well as it ever did.  Furthermore, the whole reason we're in this
> >>>>>> situation is that we checked in a publicly-visible change to the
> >>>>>> interface before it was properly ready; I think we should avoid
> >>>>>> making
> >>>>>> the same mistake this time.
> >>>>>>
> >>>>>> So personally I'd just leave this patch entirely for 4.8; but if Paul
> >>>>>> and/or Jan have strong opinions, then I would say check in only a
> >>>>>> patch to do the #if/#else/#endif, and leave both the p2m type
> changes
> >>>>>> and the new HVMMEM_ioreq_server enum for when the 4.8
> >>>> development
> >>>>>> window opens.
> >>>>>>
> >>>>>
> >>>>> If the whole series is going in then I think this patch is ok. If
> >>>>> this the
> >>> only
> >>>> patch that is going in for 4.7 then I thing we need the patch to
> >>>> hvm_op.h to
> >>>> deprecate the old type and that's it. We definitely should not
> >>>> introduce an
> >>>> implementation of the type HVMMEM_ioreq_server that has the same
> >>>> hardcoded semantics as the old type and then change it.
> >>>>> The p2m type changes are also wrong. That type needs to be left
> alone,
> >>>> presumably, so that anything using HVMMEM_mmio_write_dm and
> >>>> compiled to the old interface version continues to function. I think
> >>>> HVMMEM_ioreq_server needs to map to a new p2m type which
> should be
> >>>> introduced in patch #3.
> >>>>>
> >>>>
> >>>> Sorry, I'm also confused now. :(
> >>>>
> >>>> Do we really want to introduce a new p2m type? Why?
> >>>> My understanding of the previous agreement is that:
> >>>> 1> We need the interface to work on old hypervisor for
> >>>> HVMMEM_mmio_write_dm;
> >>>> 2> We need the interface to return -EINVAL for new hypervisor
> >>>> for HVMMEM_mmio_write_dm;
> >>>> 3> We need the new type HVMMEM_ioreq_server to work on new
> >>>> hypervisor;
> >>>>
> >>>> Did I miss something? Or I totally misunderstood?
> >>>>
> >>>
> >>> I don't know. I'm confused too. What we definitely don't want to do
> >>> is add a
> >>> new HVMMEM type and have it map to the old behaviour, otherwise
> we're no
> >>> better off.
> >>>
> >>> The question I'm not clear on the answer to is what happens to old code:
> >>>
> >>> Should it continue to compile? If so, should it continue to run.
> >>
> >> We only need to be concerned about the "get type" functionality,
> >> as that's the only thing an arbitrary guest can use. If the
> >> hypercall simply never returns the old type, then old code will
> >> still work (it'll just have some dead code on new Xen), and hence
> >> it compiling against the older interface is fine (and, from general
> >> considerations, a requirement).
> >>
> >
> > Thanks Jan. And I think the answer is yes. The new hypervisor will
> > only return HVMMEM_ioreq_server, which is a different value now.
> 
> Right -- but we can't check in this entire series for 4.7; however, Paul
> would like to make it clear that HVMMEM_mmio_write_dm is deprecated;
> so
> we need a patch which does the #ifdef/#else/#endif clause, and then
> everything else necessary to make sure that things compile properly
> either way, but no p2m changes or additional functionality.
> 

For clarity, do you expect any existing use of HVMMEM_mmio_write_dm to continue to *function*? I agree that things should continue to build, but if they don't need to function then the now redundant p2m type should be removed IMO and any attempt to set a page to HVMMEM_mmio_write_dm (or the new HVMMEM_unused) name should result in -EINVAL. What is your position on this?

  Paul

>  -George

Yu Zhang April 26, 2016, 8:23 a.m. UTC | #19

On 4/26/2016 1:01 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: George Dunlap [mailto:[e-mail]]
>> Sent: 25 April 2016 17:16
>> To: Yu, Zhang; Jan Beulich; Paul Durrant
>> Cc: Andrew Cooper; Wei Liu; Jun Nakajima; Kevin Tian; Zhiyuan Lv; xen-
>> [e-mail]; Keir (Xen.org); Tim (Xen.org)
>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>
>> On 25/04/16 16:53, Yu, Zhang wrote:
>>>
>>>
>>> On 4/25/2016 11:38 PM, Jan Beulich wrote:
>>>>>>> On 25.04.16 at 17:29, <[e-mail]> wrote:
>>>>>>  -----Original Message-----
>>>>>> From: Yu, Zhang [mailto:[e-mail]]
>>>>>> Sent: 25 April 2016 16:22
>>>>>> To: Paul Durrant; George Dunlap
>>>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun Nakajima;
>>>>>> Andrew Cooper; Tim (Xen.org); Lv, Zhiyuan; Jan Beulich; Wei Liu
>>>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
>>>>>> 4.7):
>>>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/25/2016 10:01 PM, Paul Durrant wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: [e-mail] [mailto:[e-mail]] On Behalf
>> Of
>>>>>>>> George Dunlap
>>>>>>>> Sent: 25 April 2016 14:39
>>>>>>>> To: Yu Zhang
>>>>>>>> Cc: [e-mail]; Kevin Tian; Keir (Xen.org); Jun
>>>>>>>> Nakajima;
>>>>>>>> Andrew Cooper; Tim (Xen.org); Paul Durrant; Lv, Zhiyuan; Jan
>>>>>>>> Beulich; Wei
>>>>>> Liu
>>>>>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for
>>>>>>>> 4.7):
>>>>>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>>>>>
>>>>>>>> On Mon, Apr 25, 2016 at 11:35 AM, Yu Zhang
>>>>>> <[e-mail]>
>>>>>>>> wrote:
>>>>>>>>> Previously p2m type p2m_mmio_write_dm was introduced for
>> write-
>>>>>>>>> protected memory pages whose write operations are supposed to
>> be
>>>>>>>>> forwarded to and emulated by an ioreq server. Yet limitations of
>>>>>>>>> rangeset restrict the number of guest pages to be write-protected.
>>>>>>>>>
>>>>>>>>> This patch replaces the p2m type p2m_mmio_write_dm with a new
>>>>>> name:
>>>>>>>>> p2m_ioreq_server, which means this p2m type can be claimed by
>> one
>>>>>>>>> ioreq server, instead of being tracked inside the rangeset of ioreq
>>>>>>>>> server. Patches following up will add the related hvmop handling
>>>>>>>>> code which map/unmap type p2m_ioreq_server to/from an ioreq
>>>>>> server.
>>>>>>>>>
>>>>>>>>> changes in v3:
>>>>>>>>>   - According to Jan & George's comments, keep
>>>>>>>> HVMMEM_mmio_write_dm
>>>>>>>>>     for old xen interface versions, and replace it with
>>>>>>>>> HVMMEM_unused
>>>>>>>>>     for xen interfaces newer than 4.7.0; For p2m_ioreq_server, a
>> new
>>>>>>>>>     enum - HVMMEM_ioreq_server is introduced for the get/set
>> mem
>>>>>> type
>>>>>>>>>     interfaces;
>>>>>>>>>   - Add George's Reviewed-by and Acked-by from Tim & Andrew.
>>>>>>>>
>>>>>>>> Unfortunately these rather contradict each other -- I consider
>>>>>>>> Reviewed-by to only stick if the code I've specified hasn't changed
>>>>>>>> (or has only changed trivially).
>>>>>>>>
>>>>>>>> Also...
>>>>>>>>
>>>>>>>>>
>>>>>>>>> changes in v2:
>>>>>>>>>   - According to George Dunlap's comments, only rename the p2m
>> type,
>>>>>>>>>     with no behavior changes.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Paul Durrant <[e-mail]>
>>>>>>>>> Signed-off-by: Yu Zhang <[e-mail]>
>>>>>>>>> Acked-by: Tim Deegan <[e-mail]>
>>>>>>>>> Acked-by: Andrew Cooper <[e-mail]>
>>>>>>>>> Reviewed-by: George Dunlap <[e-mail]>
>>>>>>>>> Cc: Keir Fraser <[e-mail]>
>>>>>>>>> Cc: Jan Beulich <[e-mail]>
>>>>>>>>> Cc: Andrew Cooper <[e-mail]>
>>>>>>>>> Cc: Jun Nakajima <[e-mail]>
>>>>>>>>> Cc: Kevin Tian <[e-mail]>
>>>>>>>>> Cc: George Dunlap <[e-mail]>
>>>>>>>>> Cc: Tim Deegan <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++------
>>>>>>>>>  xen/arch/x86/mm/p2m-ept.c       |  2 +-
>>>>>>>>>  xen/arch/x86/mm/p2m-pt.c        |  2 +-
>>>>>>>>>  xen/arch/x86/mm/shadow/multi.c  |  2 +-
>>>>>>>>>  xen/include/asm-x86/p2m.h       |  4 ++--
>>>>>>>>>  xen/include/public/hvm/hvm_op.h |  8 +++++++-
>>>>>>>>>  6 files changed, 20 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>>>>>> index f24126d..874cb0f 100644
>>>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>>>> @@ -1857,7 +1857,7 @@ int hvm_hap_nested_page_fault(paddr_t
>> gpa,
>>>>>>>> unsigned long gla,
>>>>>>>>>       */
>>>>>>>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>>>>>>>           (npfec.write_access &&
>>>>>>>>> -          (p2m_is_discard_write(p2mt) || (p2mt ==
>>>>>> p2m_mmio_write_dm))) )
>>>>>>>>> +          (p2m_is_discard_write(p2mt) || (p2mt ==
>>>>>>>>> p2m_ioreq_server))) )
>>>>>>>>>      {
>>>>>>>>>          __put_gfn(p2m, gfn);
>>>>>>>>>          if ( ap2m_active )
>>>>>>>>> @@ -5499,8 +5499,8 @@ long do_hvm_op(unsigned long op,
>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>>              get_gfn_query_unlocked(d, a.pfn, &t);
>>>>>>>>>              if ( p2m_is_mmio(t) )
>>>>>>>>>                  a.mem_type =  HVMMEM_mmio_dm;
>>>>>>>>> -            else if ( t == p2m_mmio_write_dm )
>>>>>>>>> -                a.mem_type = HVMMEM_mmio_write_dm;
>>>>>>>>> +            else if ( t == p2m_ioreq_server )
>>>>>>>>> +                a.mem_type = HVMMEM_ioreq_server;
>>>>>>>>>              else if ( p2m_is_readonly(t) )
>>>>>>>>>                  a.mem_type =  HVMMEM_ram_ro;
>>>>>>>>>              else if ( p2m_is_ram(t) )
>>>>>>>>> @@ -5531,7 +5531,8 @@ long do_hvm_op(unsigned long op,
>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>>>>>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>>>>>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>>>>>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>>>>>>>> +            [HVMMEM_unused] = p2m_invalid,
>>>>>>>>> +            [HVMMEM_ioreq_server] = p2m_ioreq_server
>>>>>>>>>          };
>>>>>>>>>
>>>>>>>>>          if ( copy_from_guest(&a, arg, 1) )
>>>>>>>>> @@ -5555,7 +5556,8 @@ long do_hvm_op(unsigned long op,
>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>>               ((a.first_pfn + a.nr - 1) >
>>>>>>>>> domain_get_maximum_gpfn(d)) )
>>>>>>>>>              goto setmemtype_fail;
>>>>>>>>>
>>>>>>>>> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>>>>>>>>> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>>>>>>>>> +             unlikely(a.hvmmem_type == HVMMEM_unused) )
>>>>>>>>>              goto setmemtype_fail;
>>>>>>>>>
>>>>>>>>>          while ( a.nr > start_iter )
>>>>>>>>> @@ -5579,7 +5581,7 @@ long do_hvm_op(unsigned long op,
>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>>              }
>>>>>>>>>              if ( !p2m_is_ram(t) &&
>>>>>>>>>                   (!p2m_is_hole(t) || a.hvmmem_type !=
>>>>>>>>> HVMMEM_mmio_dm)
>>>>>>>> &&
>>>>>>>>> -                 (t != p2m_mmio_write_dm || a.hvmmem_type !=
>>>>>>>> HVMMEM_ram_rw) )
>>>>>>>>> +                 (t != p2m_ioreq_server || a.hvmmem_type !=
>>>>>>>> HVMMEM_ram_rw) )
>>>>>>>>>              {
>>>>>>>>>                  put_gfn(d, pfn);
>>>>>>>>>                  goto setmemtype_fail;
>>>>>>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c
>> b/xen/arch/x86/mm/p2m-
>>>>>> ept.c
>>>>>>>>> index 3cb6868..380ec25 100644
>>>>>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>>>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>>>>>>> @@ -171,7 +171,7 @@ static void ept_p2m_type_to_flags(struct
>>>>>>>> p2m_domain *p2m, ept_entry_t *entry,
>>>>>>>>>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>>>>>>>>              break;
>>>>>>>>>          case p2m_grant_map_ro:
>>>>>>>>> -        case p2m_mmio_write_dm:
>>>>>>>>> +        case p2m_ioreq_server:
>>>>>>>>>              entry->r = 1;
>>>>>>>>>              entry->w = entry->x = 0;
>>>>>>>>>              entry->a = !!cpu_has_vmx_ept_ad;
>>>>>>>>> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-
>> pt.c
>>>>>>>>> index 3d80612..eabd2e3 100644
>>>>>>>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>>>>>>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>>>>>>>> @@ -94,7 +94,7 @@ static unsigned long
>>>>>> p2m_type_to_flags(p2m_type_t
>>>>>>>> t, mfn_t mfn,
>>>>>>>>>      default:
>>>>>>>>>          return flags | _PAGE_NX_BIT;
>>>>>>>>>      case p2m_grant_map_ro:
>>>>>>>>> -    case p2m_mmio_write_dm:
>>>>>>>>> +    case p2m_ioreq_server:
>>>>>>>>>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>>>>>>>      case p2m_ram_ro:
>>>>>>>>>      case p2m_ram_logdirty:
>>>>>>>>> diff --git a/xen/arch/x86/mm/shadow/multi.c
>>>>>>>> b/xen/arch/x86/mm/shadow/multi.c
>>>>>>>>> index e5c8499..c81302a 100644
>>>>>>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>>>>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>>>>>>> @@ -3225,7 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>>>>>>>>
>>>>>>>>>      /* Need to hand off device-model MMIO to the device model */
>>>>>>>>>      if ( p2mt == p2m_mmio_dm
>>>>>>>>> -         || (p2mt == p2m_mmio_write_dm && ft ==
>> ft_demand_write) )
>>>>>>>>> +         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>>>>>>>>>      {
>>>>>>>>>          gpa = guest_walk_to_gpa(&gw);
>>>>>>>>>          goto mmio;
>>>>>>>>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-
>> x86/p2m.h
>>>>>>>>> index 5392eb0..ee2ea9c 100644
>>>>>>>>> --- a/xen/include/asm-x86/p2m.h
>>>>>>>>> +++ b/xen/include/asm-x86/p2m.h
>>>>>>>>> @@ -71,7 +71,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_ioreq_server = 15,
>>>>>>>>>  } p2m_type_t;
>>>>>>>>>
>>>>>>>>>  /* Modifiers to the query */
>>>>>>>>> @@ -112,7 +112,7 @@ typedef unsigned int p2m_query_t;
>>>>>>>>>                        | p2m_to_mask(p2m_ram_ro)         \
>>>>>>>>>                        | p2m_to_mask(p2m_grant_map_ro)   \
>>>>>>>>>                        | p2m_to_mask(p2m_ram_shared)     \
>>>>>>>>> -                      | p2m_to_mask(p2m_mmio_write_dm))
>>>>>>>>> +                      | p2m_to_mask(p2m_ioreq_server))
>>>>>>>>>
>>>>>>>>>  /* Write-discard types, which should discard the write
>>>>>>>>> operations */
>>>>>>>>>  #define P2M_DISCARD_WRITE_TYPES
>> (p2m_to_mask(p2m_ram_ro)     \
>>>>>>>>> diff --git a/xen/include/public/hvm/hvm_op.h
>>>>>>>> b/xen/include/public/hvm/hvm_op.h
>>>>>>>>> index 1606185..b3e45cf 100644
>>>>>>>>> --- a/xen/include/public/hvm/hvm_op.h
>>>>>>>>> +++ b/xen/include/public/hvm/hvm_op.h
>>>>>>>>> @@ -83,7 +83,13 @@ typedef enum {
>>>>>>>>>      HVMMEM_ram_rw,             /* Normal read/write guest RAM */
>>>>>>>>>      HVMMEM_ram_ro,             /* Read-only; writes are
>>>>>>>>> discarded */
>>>>>>>>>      HVMMEM_mmio_dm,            /* Reads and write go to the device
>>>>>> model
>>>>>>>> */
>>>>>>>>> -    HVMMEM_mmio_write_dm       /* Read-only; writes go to the
>>>>>>>>> device
>>>>>>>> model */
>>>>>>>>> +#if __XEN_INTERFACE_VERSION__ < 0x00040700
>>>>>>>>> +    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the
>>>>>>>>> device
>>>>>>>> model */
>>>>>>>>> +#else
>>>>>>>>> +    HVMMEM_unused,             /* Placeholder; setting memory to
>>>>>>>>> this
>>>>>> type
>>>>>>>>> +                                  will fail for code after 4.7.0 */
>>>>>>>>> +#endif
>>>>>>>>> +    HVMMEM_ioreq_server
>>>>>>>>
>>>>>>>> Also, I don't think we've had a convincing argument for why this
>>>>>>>> patch
>>>>>>>> needs to be in 4.7.  The p2m name changes are internal only, and so
>>>>>>>> don't need to be made at all; and the old functionality will work as
>>>>>>>> well as it ever did.  Furthermore, the whole reason we're in this
>>>>>>>> situation is that we checked in a publicly-visible change to the
>>>>>>>> interface before it was properly ready; I think we should avoid
>>>>>>>> making
>>>>>>>> the same mistake this time.
>>>>>>>>
>>>>>>>> So personally I'd just leave this patch entirely for 4.8; but if Paul
>>>>>>>> and/or Jan have strong opinions, then I would say check in only a
>>>>>>>> patch to do the #if/#else/#endif, and leave both the p2m type
>> changes
>>>>>>>> and the new HVMMEM_ioreq_server enum for when the 4.8
>>>>>> development
>>>>>>>> window opens.
>>>>>>>>
>>>>>>>
>>>>>>> If the whole series is going in then I think this patch is ok. If
>>>>>>> this the
>>>>> only
>>>>>> patch that is going in for 4.7 then I thing we need the patch to
>>>>>> hvm_op.h to
>>>>>> deprecate the old type and that's it. We definitely should not
>>>>>> introduce an
>>>>>> implementation of the type HVMMEM_ioreq_server that has the same
>>>>>> hardcoded semantics as the old type and then change it.
>>>>>>> The p2m type changes are also wrong. That type needs to be left
>> alone,
>>>>>> presumably, so that anything using HVMMEM_mmio_write_dm and
>>>>>> compiled to the old interface version continues to function. I think
>>>>>> HVMMEM_ioreq_server needs to map to a new p2m type which
>> should be
>>>>>> introduced in patch #3.
>>>>>>>
>>>>>>
>>>>>> Sorry, I'm also confused now. :(
>>>>>>
>>>>>> Do we really want to introduce a new p2m type? Why?
>>>>>> My understanding of the previous agreement is that:
>>>>>> 1> We need the interface to work on old hypervisor for
>>>>>> HVMMEM_mmio_write_dm;
>>>>>> 2> We need the interface to return -EINVAL for new hypervisor
>>>>>> for HVMMEM_mmio_write_dm;
>>>>>> 3> We need the new type HVMMEM_ioreq_server to work on new
>>>>>> hypervisor;
>>>>>>
>>>>>> Did I miss something? Or I totally misunderstood?
>>>>>>
>>>>>
>>>>> I don't know. I'm confused too. What we definitely don't want to do
>>>>> is add a
>>>>> new HVMMEM type and have it map to the old behaviour, otherwise
>> we're no
>>>>> better off.
>>>>>
>>>>> The question I'm not clear on the answer to is what happens to old code:
>>>>>
>>>>> Should it continue to compile? If so, should it continue to run.
>>>>
>>>> We only need to be concerned about the "get type" functionality,
>>>> as that's the only thing an arbitrary guest can use. If the
>>>> hypercall simply never returns the old type, then old code will
>>>> still work (it'll just have some dead code on new Xen), and hence
>>>> it compiling against the older interface is fine (and, from general
>>>> considerations, a requirement).
>>>>
>>>
>>> Thanks Jan. And I think the answer is yes. The new hypervisor will
>>> only return HVMMEM_ioreq_server, which is a different value now.
>>
>> Right -- but we can't check in this entire series for 4.7; however, Paul
>> would like to make it clear that HVMMEM_mmio_write_dm is deprecated;
>> so
>> we need a patch which does the #ifdef/#else/#endif clause, and then
>> everything else necessary to make sure that things compile properly
>> either way, but no p2m changes or additional functionality.
>>
>
> For clarity, do you expect any existing use of HVMMEM_mmio_write_dm to continue to *function*? I agree that things should continue to build, but if they don't need to function then the now redundant p2m type should be removed IMO and any attempt to set a page to HVMMEM_mmio_write_dm (or the new HVMMEM_unused) name should result in -EINVAL. What is your position on this?
>

Thanks, Paul.
My expectation is that HVMMEM_mmio_write_dm shall fail in new xen
version, but I do not think we need to remove the p2m type, just
rename it could be OK.

>   Paul
>
>>  -George

Yu

Paul Durrant April 26, 2016, 8:33 a.m. UTC | #20

> -----Original Message-----

[snip]
> >

> > For clarity, do you expect any existing use of HVMMEM_mmio_write_dm

> to continue to *function*? I agree that things should continue to build, but if

> they don't need to function then the now redundant p2m type should be

> removed IMO and any attempt to set a page to HVMMEM_mmio_write_dm

> (or the new HVMMEM_unused) name should result in -EINVAL. What is your

> position on this?

> >

> 

> Thanks, Paul.

> My expectation is that HVMMEM_mmio_write_dm shall fail in new xen

> version, but I do not think we need to remove the p2m type, just

> rename it could be OK.

> 


I think we need George's response before we can proceed.

  Paul

George Dunlap April 27, 2016, 2:12 p.m. UTC | #21

On Mon, Apr 25, 2016 at 6:01 PM, Paul Durrant <[e-mail]> wrote:
> For clarity, do you expect any existing use of HVMMEM_mmio_write_dm to continue to *function*? I agree that things should continue to build, but if they don't need to function then the now redundant p2m type should be removed IMO and any attempt to set a page to HVMMEM_mmio_write_dm (or the new HVMMEM_unused) name should result in -EINVAL. What is your position on this?

I sort of feel like we're playing some strange guessing game with the
color of this bike shed, where all 4 of us give a random combination
of constrants and then we have to figure out what the solution is. :-)

There are two issues: the interface (HVMMEM_*) and the internals.(p2m_*).

Jan says that code that calls HVMOP_get_mem_type has to continue to
compile and function.  "Functioning" is easy, as you just don't return
that value and you're done.  Compiling just means having the #ifdef.

It sounds like we all agree that HVMOP_set_mem_type with the current
HVMMEM_mmio_write_dm value should return -EINVAL.

Regarding the p2m type which now should be impossible to set -- I
don't think it's critical to remove from the release, since it's just
internal.  I'd normally say just leave it for now to reduce code
churn.

But mostly I think we just want to get this bike shed painted, so if
anyone thinks we should really remove the p2m type from this release,
then that's fine with me too (assuming it's OK with Wei).

Does this cover everything?

 -George

Paul Durrant April 27, 2016, 2:42 p.m. UTC | #22

> -----Original Message-----

> From: George Dunlap

> Sent: 27 April 2016 15:13

> To: Paul Durrant

> Cc: Yu, Zhang; Jan Beulich; Kevin Tian; Wei Liu; Andrew Cooper; Tim

> (Xen.org); [e-mail]; Zhiyuan Lv; Jun Nakajima; Keir (Xen.org)

> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> 

> On Mon, Apr 25, 2016 at 6:01 PM, Paul Durrant <[e-mail]>

> wrote:

> > For clarity, do you expect any existing use of HVMMEM_mmio_write_dm

> to continue to *function*? I agree that things should continue to build, but if

> they don't need to function then the now redundant p2m type should be

> removed IMO and any attempt to set a page to HVMMEM_mmio_write_dm

> (or the new HVMMEM_unused) name should result in -EINVAL. What is your

> position on this?

> 

> I sort of feel like we're playing some strange guessing game with the

> color of this bike shed, where all 4 of us give a random combination

> of constrants and then we have to figure out what the solution is. :-)

> 

> There are two issues: the interface (HVMMEM_*) and the

> internals.(p2m_*).

> 

> Jan says that code that calls HVMOP_get_mem_type has to continue to

> compile and function.  "Functioning" is easy, as you just don't return

> that value and you're done.  Compiling just means having the #ifdef.

> 

> It sounds like we all agree that HVMOP_set_mem_type with the current

> HVMMEM_mmio_write_dm value should return -EINVAL.

> 

> Regarding the p2m type which now should be impossible to set -- I

> don't think it's critical to remove from the release, since it's just

> internal.  I'd normally say just leave it for now to reduce code

> churn.

> 

> But mostly I think we just want to get this bike shed painted, so if

> anyone thinks we should really remove the p2m type from this release,

> then that's fine with me too (assuming it's OK with Wei).

> 

> Does this cover everything?

> 


I think so. Thanks for the clarification. 

Yu, are you happy to submit a patch with the #ifdef in the header, and that removes any ability to set the old type?

I guess leaving the p2m type in place to avoid code churn is reasonable at this stage, but anyone looking at the p2m code is probably going to question why it's there in 4.7.

  Paul

>  -George

Wei Liu April 27, 2016, 2:47 p.m. UTC | #23

On Wed, Apr 27, 2016 at 03:12:46PM +0100, George Dunlap wrote:
> On Mon, Apr 25, 2016 at 6:01 PM, Paul Durrant <[e-mail]> wrote:
> > For clarity, do you expect any existing use of HVMMEM_mmio_write_dm to continue to *function*? I agree that things should continue to build, but if they don't need to function then the now redundant p2m type should be removed IMO and any attempt to set a page to HVMMEM_mmio_write_dm (or the new HVMMEM_unused) name should result in -EINVAL. What is your position on this?
> 
> I sort of feel like we're playing some strange guessing game with the
> color of this bike shed, where all 4 of us give a random combination
> of constrants and then we have to figure out what the solution is. :-)
> 
> There are two issues: the interface (HVMMEM_*) and the internals.(p2m_*).
> 
> Jan says that code that calls HVMOP_get_mem_type has to continue to
> compile and function.  "Functioning" is easy, as you just don't return
> that value and you're done.  Compiling just means having the #ifdef.
> 
> It sounds like we all agree that HVMOP_set_mem_type with the current
> HVMMEM_mmio_write_dm value should return -EINVAL.
> 

This, is the most urgent issue at the moment AIUI.

> Regarding the p2m type which now should be impossible to set -- I
> don't think it's critical to remove from the release, since it's just
> internal.  I'd normally say just leave it for now to reduce code
> churn.
> 
> But mostly I think we just want to get this bike shed painted, so if
> anyone thinks we should really remove the p2m type from this release,
> then that's fine with me too (assuming it's OK with Wei).
> 

I would prefer as few churns as possible.

Wei.

> Does this cover everything?
> 
>  -George

Yu Zhang April 28, 2016, 2:47 a.m. UTC | #24

On 4/27/2016 10:42 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: George Dunlap
>> Sent: 27 April 2016 15:13
>> To: Paul Durrant
>> Cc: Yu, Zhang; Jan Beulich; Kevin Tian; Wei Liu; Andrew Cooper; Tim
>> (Xen.org); [e-mail]; Zhiyuan Lv; Jun Nakajima; Keir (Xen.org)
>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>
>> On Mon, Apr 25, 2016 at 6:01 PM, Paul Durrant <[e-mail]>
>> wrote:
>>> For clarity, do you expect any existing use of HVMMEM_mmio_write_dm
>> to continue to *function*? I agree that things should continue to build, but if
>> they don't need to function then the now redundant p2m type should be
>> removed IMO and any attempt to set a page to HVMMEM_mmio_write_dm
>> (or the new HVMMEM_unused) name should result in -EINVAL. What is your
>> position on this?
>>
>> I sort of feel like we're playing some strange guessing game with the
>> color of this bike shed, where all 4 of us give a random combination
>> of constrants and then we have to figure out what the solution is. :-)
>>
>> There are two issues: the interface (HVMMEM_*) and the
>> internals.(p2m_*).
>>
>> Jan says that code that calls HVMOP_get_mem_type has to continue to
>> compile and function.  "Functioning" is easy, as you just don't return
>> that value and you're done.  Compiling just means having the #ifdef.
>>
>> It sounds like we all agree that HVMOP_set_mem_type with the current
>> HVMMEM_mmio_write_dm value should return -EINVAL.
>>
>> Regarding the p2m type which now should be impossible to set -- I
>> don't think it's critical to remove from the release, since it's just
>> internal.  I'd normally say just leave it for now to reduce code
>> churn.
>>
>> But mostly I think we just want to get this bike shed painted, so if
>> anyone thinks we should really remove the p2m type from this release,
>> then that's fine with me too (assuming it's OK with Wei).
>>
>> Does this cover everything?
>>
>
> I think so. Thanks for the clarification.
>
> Yu, are you happy to submit a patch with the #ifdef in the header, and that removes any ability to set the old type?
>

I'm fine with this, and thanks. :)
So my understanding is that the only difference between the new
patch and this current one is we do not replace p2m_mmio_write_dm
with p2m_ioreq_server, hence no need to introduce HVMMEM_ioreq_server.
Is this understanding correct?

Besides, do you think it acceptable we just replace p2m_mmio_write_dm
with p2m_ioreq_server in next version, or should we remove this p2m
type and define p2m_ioreq_server with a different value? :)


> I guess leaving the p2m type in place to avoid code churn is reasonable at this stage, but anyone looking at the p2m code is probably going to question why it's there in 4.7.
>
>   Paul
>
>>  -George

B.R.
Yu

Yu Zhang April 28, 2016, 7:07 a.m. UTC | #25

On 4/28/2016 3:14 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Yu, Zhang [mailto:[e-mail]]
>> Sent: 28 April 2016 03:47
>> To: Paul Durrant; George Dunlap
>> Cc: Kevin Tian; Wei Liu; Jun Nakajima; Andrew Cooper; Tim (Xen.org); xen-
>> [e-mail]; Zhiyuan Lv; Jan Beulich; Keir (Xen.org)
>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>
>>
>>
>> On 4/27/2016 10:42 PM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: George Dunlap
>>>> Sent: 27 April 2016 15:13
>>>> To: Paul Durrant
>>>> Cc: Yu, Zhang; Jan Beulich; Kevin Tian; Wei Liu; Andrew Cooper; Tim
>>>> (Xen.org); [e-mail]; Zhiyuan Lv; Jun Nakajima; Keir
>> (Xen.org)
>>>> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
>>>> Rename p2m_mmio_write_dm to p2m_ioreq_server.
>>>>
>>>> On Mon, Apr 25, 2016 at 6:01 PM, Paul Durrant <[e-mail]>
>>>> wrote:
>>>>> For clarity, do you expect any existing use of
>> HVMMEM_mmio_write_dm
>>>> to continue to *function*? I agree that things should continue to build,
>> but if
>>>> they don't need to function then the now redundant p2m type should be
>>>> removed IMO and any attempt to set a page to
>> HVMMEM_mmio_write_dm
>>>> (or the new HVMMEM_unused) name should result in -EINVAL. What is
>> your
>>>> position on this?
>>>>
>>>> I sort of feel like we're playing some strange guessing game with the
>>>> color of this bike shed, where all 4 of us give a random combination
>>>> of constrants and then we have to figure out what the solution is. :-)
>>>>
>>>> There are two issues: the interface (HVMMEM_*) and the
>>>> internals.(p2m_*).
>>>>
>>>> Jan says that code that calls HVMOP_get_mem_type has to continue to
>>>> compile and function.  "Functioning" is easy, as you just don't return
>>>> that value and you're done.  Compiling just means having the #ifdef.
>>>>
>>>> It sounds like we all agree that HVMOP_set_mem_type with the current
>>>> HVMMEM_mmio_write_dm value should return -EINVAL.
>>>>
>>>> Regarding the p2m type which now should be impossible to set -- I
>>>> don't think it's critical to remove from the release, since it's just
>>>> internal.  I'd normally say just leave it for now to reduce code
>>>> churn.
>>>>
>>>> But mostly I think we just want to get this bike shed painted, so if
>>>> anyone thinks we should really remove the p2m type from this release,
>>>> then that's fine with me too (assuming it's OK with Wei).
>>>>
>>>> Does this cover everything?
>>>>
>>>
>>> I think so. Thanks for the clarification.
>>>
>>> Yu, are you happy to submit a patch with the #ifdef in the header, and that
>> removes any ability to set the old type?
>>>
>>
>> I'm fine with this, and thanks. :)
>> So my understanding is that the only difference between the new
>> patch and this current one is we do not replace p2m_mmio_write_dm
>> with p2m_ioreq_server, hence no need to introduce
>> HVMMEM_ioreq_server.
>> Is this understanding correct?
>>
>
> I believe so. The main points are that:
>
> a) HVMMEM_mmio_write_dm no longer exists with the new interface version and is replaced by HVMMEM_unused
> b) Any attempt to set a page to type HVMMEM_unused results in -EINVAL
>

Yep.

>> Besides, do you think it acceptable we just replace p2m_mmio_write_dm
>> with p2m_ioreq_server in next version, or should we remove this p2m
>> type and define p2m_ioreq_server with a different value? :)
>>
>
> p2m_ioreq_server becomes defunct after the aforementioned patch. With that patch applied there will be no way to set that type on a p2m entry so it should be ok to re-use the type.
>

Got it. Thank you, Paul.
Will send the patch out later. :)

>   Paul
>
>>
>>> I guess leaving the p2m type in place to avoid code churn is reasonable at
>> this stage, but anyone looking at the p2m code is probably going to question
>> why it's there in 4.7.
>>>
>>>   Paul
>>>
>>>>  -George
>>
>> B.R.
>> Yu
>

Yu

Paul Durrant April 28, 2016, 7:14 a.m. UTC | #26

> -----Original Message-----

> From: Yu, Zhang [mailto:[e-mail]]

> Sent: 28 April 2016 03:47

> To: Paul Durrant; George Dunlap

> Cc: Kevin Tian; Wei Liu; Jun Nakajima; Andrew Cooper; Tim (Xen.org); xen-

> [e-mail]; Zhiyuan Lv; Jan Beulich; Keir (Xen.org)

> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> 

> 

> 

> On 4/27/2016 10:42 PM, Paul Durrant wrote:

> >> -----Original Message-----

> >> From: George Dunlap

> >> Sent: 27 April 2016 15:13

> >> To: Paul Durrant

> >> Cc: Yu, Zhang; Jan Beulich; Kevin Tian; Wei Liu; Andrew Cooper; Tim

> >> (Xen.org); [e-mail]; Zhiyuan Lv; Jun Nakajima; Keir

> (Xen.org)

> >> Subject: Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):

> >> Rename p2m_mmio_write_dm to p2m_ioreq_server.

> >>

> >> On Mon, Apr 25, 2016 at 6:01 PM, Paul Durrant <[e-mail]>

> >> wrote:

> >>> For clarity, do you expect any existing use of

> HVMMEM_mmio_write_dm

> >> to continue to *function*? I agree that things should continue to build,

> but if

> >> they don't need to function then the now redundant p2m type should be

> >> removed IMO and any attempt to set a page to

> HVMMEM_mmio_write_dm

> >> (or the new HVMMEM_unused) name should result in -EINVAL. What is

> your

> >> position on this?

> >>

> >> I sort of feel like we're playing some strange guessing game with the

> >> color of this bike shed, where all 4 of us give a random combination

> >> of constrants and then we have to figure out what the solution is. :-)

> >>

> >> There are two issues: the interface (HVMMEM_*) and the

> >> internals.(p2m_*).

> >>

> >> Jan says that code that calls HVMOP_get_mem_type has to continue to

> >> compile and function.  "Functioning" is easy, as you just don't return

> >> that value and you're done.  Compiling just means having the #ifdef.

> >>

> >> It sounds like we all agree that HVMOP_set_mem_type with the current

> >> HVMMEM_mmio_write_dm value should return -EINVAL.

> >>

> >> Regarding the p2m type which now should be impossible to set -- I

> >> don't think it's critical to remove from the release, since it's just

> >> internal.  I'd normally say just leave it for now to reduce code

> >> churn.

> >>

> >> But mostly I think we just want to get this bike shed painted, so if

> >> anyone thinks we should really remove the p2m type from this release,

> >> then that's fine with me too (assuming it's OK with Wei).

> >>

> >> Does this cover everything?

> >>

> >

> > I think so. Thanks for the clarification.

> >

> > Yu, are you happy to submit a patch with the #ifdef in the header, and that

> removes any ability to set the old type?

> >

> 

> I'm fine with this, and thanks. :)

> So my understanding is that the only difference between the new

> patch and this current one is we do not replace p2m_mmio_write_dm

> with p2m_ioreq_server, hence no need to introduce

> HVMMEM_ioreq_server.

> Is this understanding correct?

> 


I believe so. The main points are that:

a) HVMMEM_mmio_write_dm no longer exists with the new interface version and is replaced by HVMMEM_unused
b) Any attempt to set a page to type HVMMEM_unused results in -EINVAL

> Besides, do you think it acceptable we just replace p2m_mmio_write_dm

> with p2m_ioreq_server in next version, or should we remove this p2m

> type and define p2m_ioreq_server with a different value? :)

> 


p2m_ioreq_server becomes defunct after the aforementioned patch. With that patch applied there will be no way to set that type on a p2m entry so it should be ok to re-use the type.

  Paul

> 

> > I guess leaving the p2m type in place to avoid code churn is reasonable at

> this stage, but anyone looking at the p2m code is probably going to question

> why it's there in 4.7.

> >

> >   Paul

> >

> >>  -George

> 

> B.R.

> Yu

Jan Beulich April 28, 2016, 10:02 a.m. UTC | #27

>>> On 28.04.16 at 09:14, <[e-mail]> wrote:
>> From: Yu, Zhang [mailto:[e-mail]]
>> Sent: 28 April 2016 03:47
>> Besides, do you think it acceptable we just replace p2m_mmio_write_dm
>> with p2m_ioreq_server in next version, or should we remove this p2m
>> type and define p2m_ioreq_server with a different value? :)
>> 
> 
> p2m_ioreq_server becomes defunct after the aforementioned patch. With that 
> patch applied there will be no way to set that type on a p2m entry so it 
> should be ok to re-use the type.

s/p2m_ioreq_server/p2m_mmio_write_dm/ ?

Jan

Paul Durrant April 28, 2016, 10:43 a.m. UTC | #28

> -----Original Message-----
> From: Jan Beulich [mailto:[e-mail]]
> Sent: 28 April 2016 11:03
> To: Paul Durrant
> Cc: Andrew Cooper; George Dunlap; Wei Liu; JunNakajima; Kevin Tian;
> Zhiyuan Lv; Zhang Yu; [e-mail]; Keir (Xen.org); Tim (Xen.org)
> Subject: RE: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
> Rename p2m_mmio_write_dm to p2m_ioreq_server.
> 
> >>> On 28.04.16 at 09:14, <[e-mail]> wrote:
> >> From: Yu, Zhang [mailto:[e-mail]]
> >> Sent: 28 April 2016 03:47
> >> Besides, do you think it acceptable we just replace p2m_mmio_write_dm
> >> with p2m_ioreq_server in next version, or should we remove this p2m
> >> type and define p2m_ioreq_server with a different value? :)
> >>
> >
> > p2m_ioreq_server becomes defunct after the aforementioned patch. With
> that
> > patch applied there will be no way to set that type on a p2m entry so it
> > should be ok to re-use the type.
> 
> s/p2m_ioreq_server/p2m_mmio_write_dm/ ?
> 

Yes, that's what I meant.

  Paul

> Jan

Patch

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f24126d..874cb0f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1857,7 +1857,7 @@  int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      */
     if ( (p2mt == p2m_mmio_dm) || 
          (npfec.write_access &&
-          (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
+          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
     {
         __put_gfn(p2m, gfn);
         if ( ap2m_active )
@@ -5499,8 +5499,8 @@  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             get_gfn_query_unlocked(d, a.pfn, &t);
             if ( p2m_is_mmio(t) )
                 a.mem_type =  HVMMEM_mmio_dm;
-            else if ( t == p2m_mmio_write_dm )
-                a.mem_type = HVMMEM_mmio_write_dm;
+            else if ( t == p2m_ioreq_server )
+                a.mem_type = HVMMEM_ioreq_server;
             else if ( p2m_is_readonly(t) )
                 a.mem_type =  HVMMEM_ram_ro;
             else if ( p2m_is_ram(t) )
@@ -5531,7 +5531,8 @@  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             [HVMMEM_ram_rw]  = p2m_ram_rw,
             [HVMMEM_ram_ro]  = p2m_ram_ro,
             [HVMMEM_mmio_dm] = p2m_mmio_dm,
-            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
+            [HVMMEM_unused] = p2m_invalid,
+            [HVMMEM_ioreq_server] = p2m_ioreq_server
         };
 
         if ( copy_from_guest(&a, arg, 1) )
@@ -5555,7 +5556,8 @@  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto setmemtype_fail;
             
-        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
+        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
+             unlikely(a.hvmmem_type == HVMMEM_unused) )
             goto setmemtype_fail;
 
         while ( a.nr > start_iter )
@@ -5579,7 +5581,7 @@  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             }
             if ( !p2m_is_ram(t) &&
                  (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
-                 (t != p2m_mmio_write_dm || a.hvmmem_type != HVMMEM_ram_rw) )
+                 (t != p2m_ioreq_server || a.hvmmem_type != HVMMEM_ram_rw) )
             {
                 put_gfn(d, pfn);
                 goto setmemtype_fail;
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 3cb6868..380ec25 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -171,7 +171,7 @@  static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
             entry->a = entry->d = !!cpu_has_vmx_ept_ad;
             break;
         case p2m_grant_map_ro:
-        case p2m_mmio_write_dm:
+        case p2m_ioreq_server:
             entry->r = 1;
             entry->w = entry->x = 0;
             entry->a = !!cpu_has_vmx_ept_ad;
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 3d80612..eabd2e3 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -94,7 +94,7 @@  static unsigned long p2m_type_to_flags(p2m_type_t t, mfn_t mfn,
     default:
         return flags | _PAGE_NX_BIT;
     case p2m_grant_map_ro:
-    case p2m_mmio_write_dm:
+    case p2m_ioreq_server:
         return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
     case p2m_ram_ro:
     case p2m_ram_logdirty:
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index e5c8499..c81302a 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3225,7 +3225,7 @@  static int sh_page_fault(struct vcpu *v,
 
     /* Need to hand off device-model MMIO to the device model */
     if ( p2mt == p2m_mmio_dm
-         || (p2mt == p2m_mmio_write_dm && ft == ft_demand_write) )
+         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
     {
         gpa = guest_walk_to_gpa(&gw);
         goto mmio;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 5392eb0..ee2ea9c 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -71,7 +71,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_ioreq_server = 15,
 } p2m_type_t;
 
 /* Modifiers to the query */
@@ -112,7 +112,7 @@  typedef unsigned int p2m_query_t;
                       | p2m_to_mask(p2m_ram_ro)         \
                       | p2m_to_mask(p2m_grant_map_ro)   \
                       | p2m_to_mask(p2m_ram_shared)     \
-                      | p2m_to_mask(p2m_mmio_write_dm))
+                      | p2m_to_mask(p2m_ioreq_server))
 
 /* Write-discard types, which should discard the write operations */
 #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 1606185..b3e45cf 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -83,7 +83,13 @@  typedef enum {
     HVMMEM_ram_rw,             /* Normal read/write guest RAM */
     HVMMEM_ram_ro,             /* Read-only; writes are discarded */
     HVMMEM_mmio_dm,            /* Reads and write go to the device model */
-    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device model */
+#if __XEN_INTERFACE_VERSION__ < 0x00040700
+    HVMMEM_mmio_write_dm,      /* Read-only; writes go to the device model */
+#else
+    HVMMEM_unused,             /* Placeholder; setting memory to this type
+                                  will fail for code after 4.7.0 */
+#endif
+    HVMMEM_ioreq_server
 } hvmmem_type_t;
 
 /* Following tools-only interfaces may change in future. */