Re: [PATCH v2 REPOST 11/12] x86/hvm/ioreq: defer mapping gfns until they are actually requsted

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

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 24 August 2017 18:21
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 REPOST 11/12] x86/hvm/ioreq: defer
> mapping gfns until they are actually requsted
> 
> On Tue, Aug 22, 2017 at 03:51:05PM +0100, Paul Durrant wrote:
> > A subsequent patch will introduce a new scheme to allow an emulator to
> > map IOREQ server pages directly from Xen rather than the guest P2M.
> >
> > This patch lays the groundwork for that change by deferring mapping of
> > gfns until their values are requested by an emulator. To that end, the
> > pad field of the xen_dm_op_get_ioreq_server_info structure is re-
> purposed
> > to a flags field and new flag, XEN_DMOP_no_gfns, defined which modifies
> the
> > behaviour of XEN_DMOP_get_ioreq_server_info to allow the caller to
> avoid
> > requesting the gfn values.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > ---
> >  tools/libs/devicemodel/core.c                   |  8 +++++
> >  tools/libs/devicemodel/include/xendevicemodel.h |  6 ++--
> >  xen/arch/x86/hvm/dm.c                           |  9 +++--
> >  xen/arch/x86/hvm/ioreq.c                        | 44 ++++++++++++++-----------
> >  xen/include/asm-x86/hvm/domain.h                |  2 +-
> >  xen/include/public/hvm/dm_op.h                  | 32 ++++++++++--------
> >  6 files changed, 61 insertions(+), 40 deletions(-)
> >
> > diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> > index fcb260d29b..907c894e77 100644
> > --- a/tools/libs/devicemodel/core.c
> > +++ b/tools/libs/devicemodel/core.c
> > @@ -188,6 +188,14 @@ int xendevicemodel_get_ioreq_server_info(
> >
> >      data->id = id;
> >
> > +    /*
> > +     * If the caller is not requesting gfn values then instruct the
> > +     * hypercall not to retrieve them as this may cause them to be
> > +     * mapped.
> > +     */
> > +    if (!ioreq_gfn && !bufioreq_gfn)
> > +        data->flags = XEN_DMOP_no_gfns;
> 
> Since this is memset to 0 I would use |= here, in case someone adds a
> new flag further up.

Ok.

> 
> Also, seeing the code below, don't you need to retry on error in case
> you are dealing with an old hypervisor version? (that will choke on pad
> being set).
>

This is tools code so there should be no possibility of a mismatch with the hypervisor.
 
> > diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-
> x86/hvm/domain.h
> > index 7b93d10209..b8bcd559a5 100644
> > --- a/xen/include/asm-x86/hvm/domain.h
> > +++ b/xen/include/asm-x86/hvm/domain.h
> > @@ -70,7 +70,7 @@ struct hvm_ioreq_server {
> >      evtchn_port_t          bufioreq_evtchn;
> >      struct rangeset        *range[NR_IO_RANGE_TYPES];
> 
> I would place bufioreq_handling here in order to have a more compact
> structure layout.

Ok.

> 
> >      bool                   enabled;
> > -    bool                   bufioreq_atomic;
> > +    int                    bufioreq_handling;
> >      bool                   is_default;
> >  };
> >
> > diff --git a/xen/include/public/hvm/dm_op.h
> b/xen/include/public/hvm/dm_op.h
> > index 6bbab5fca3..9677bd74e7 100644
> > --- a/xen/include/public/hvm/dm_op.h
> > +++ b/xen/include/public/hvm/dm_op.h
> > @@ -79,28 +79,34 @@ struct xen_dm_op_create_ioreq_server {
> >   * XEN_DMOP_get_ioreq_server_info: Get all the information necessary
> to
> >   *                                 access IOREQ Server <id>.
> >   *
> > - * The emulator needs to map the synchronous ioreq structures and
> buffered
> > - * ioreq ring (if it exists) that Xen uses to request emulation. These are
> > - * hosted in the target domain's gmfns <ioreq_gfn> and <bufioreq_gfn>
> > - * respectively. In addition, if the IOREQ Server is handling buffered
> > - * emulation requests, the emulator needs to bind to event channel
> > - * <bufioreq_port> to listen for them. (The event channels used for
> > - * synchronous emulation requests are specified in the per-CPU ioreq
> > - * structures in <ioreq_gfn>).
> > - * If the IOREQ Server is not handling buffered emulation requests then
> the
> > - * values handed back in <bufioreq_gfn> and <bufioreq_port> will both be
> 0.
> > + * If the IOREQ Server is handling buffered emulation requests, the
> > + * emulator needs to bind to event channel <bufioreq_port> to listen for
> > + * them. (The event channels used for synchronous emulation requests
> are
> > + * specified in the per-CPU ioreq structures).
> > + * In addition, if the XENMEM_acquire_resource memory op cannot be
> used,
> > + * the emulator will need to map the synchronous ioreq structures and
> > + * buffered ioreq ring (if it exists) from guest memory. If <flags> does
> > + * not contain XEN_DMOP_no_gfns then these pages will be made
> available and
> > + * the frame numbers passed back in gfns <ioreq_gfn> and
> <bufioreq_gfn>
> > + * respectively. (If the IOREQ Server is not handling buffered emulation
> > + * only <ioreq_gfn> will be valid).
> >   */
> >  #define XEN_DMOP_get_ioreq_server_info 2
> >
> >  struct xen_dm_op_get_ioreq_server_info {
> >      /* IN - server id */
> >      ioservid_t id;
> > -    uint16_t pad;
> > +    /* IN - flags */
> > +    uint16_t flags;
> 
> Don't you need to bump some version or similar to let the consumers
> know this field is now available? Or that's done using a compile test?
> 

No. Again this is tools code so the caller should maintain compat based on the version of Xen.

Cheers,

  Paul

> > +#define _XEN_DMOP_no_gfns 0
> > +#define XEN_DMOP_no_gfns (1u << _XEN_DMOP_no_gfns)
> > +
> >      /* OUT - buffered ioreq port */
> >      evtchn_port_t bufioreq_port;
> > -    /* OUT - sync ioreq gfn */
> > +    /* OUT - sync ioreq gfn (see block comment above) */
> >      uint64_aligned_t ioreq_gfn;
> > -    /* OUT - buffered ioreq gfn */
> > +    /* OUT - buffered ioreq gfn (see block comment above)*/
> >      uint64_aligned_t bufioreq_gfn;
> >  };
> >
> > --
> > 2.11.0
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel

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