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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@citrix.com]
> 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-
> devel@lists.xen.org; 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, <Paul.Durrant@citrix.com> wrote:
> >>>>  -----Original Message-----
> >>>> From: Yu, Zhang [mailto:yu.c.zhang@linux.intel.com]
> >>>> Sent: 25 April 2016 16:22
> >>>> To: Paul Durrant; George Dunlap
> >>>> Cc: xen-devel@lists.xen.org; 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: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf
> Of
> >>>>>> George Dunlap
> >>>>>> Sent: 25 April 2016 14:39
> >>>>>> To: Yu Zhang
> >>>>>> Cc: xen-devel@lists.xen.org; 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
> >>>> <yu.c.zhang@linux.intel.com>
> >>>>>> 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 <paul.durrant@citrix.com>
> >>>>>>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> >>>>>>> Acked-by: Tim Deegan <tim@xen.org>
> >>>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>>>>> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> >>>>>>> Cc: Keir Fraser <keir@xen.org>
> >>>>>>> Cc: Jan Beulich <jbeulich@suse.com>
> >>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>>>>> Cc: Jun Nakajima <jun.nakajima@intel.com>
> >>>>>>> Cc: Kevin Tian <kevin.tian@intel.com>
> >>>>>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> >>>>>>> Cc: Tim Deegan <tim@xen.org>
> >>>>>>> ---
> >>>>>>>  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

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