Re: [Xen-devel] [RFC Patch v4 2/8] ioreq: bump the number of IOREQ page to 4 pages

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Chao Gao [mailto:chao.gao@xxxxxxxxx]
> Sent: 06 December 2017 07:50
> To: xen-devel@xxxxxxxxxxxxx
> Cc: Chao Gao <chao.gao@xxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>
> Subject: [RFC Patch v4 2/8] ioreq: bump the number of IOREQ page to 4
> pages
> 
> One 4K-byte page at most contains 128 'ioreq_t'. In order to remove the vcpu
> number constraint imposed by one IOREQ page, bump the number of IOREQ
> page to
> 4 pages. With this patch, multiple pages can be used as IOREQ page.
> 
> Basically, this patch extends 'ioreq' field in struct hvm_ioreq_server to an
> array. All accesses to 'ioreq' field such as 's->ioreq' are replaced with
> FOR_EACH_IOREQ_PAGE macro.
> 
> In order to access an IOREQ page, QEMU should get the gmfn and map this
> gmfn
> to its virtual address space.

No. There's no need to extend the 'legacy' mechanism of using magic page gfns. 
You should only handle the case where the mfns are allocated on demand (see the 
call to hvm_ioreq_server_alloc_pages() in hvm_get_ioreq_server_frame()). The 
number of guest vcpus is known at this point so the correct number of pages can 
be allocated. If the creator of the ioreq server attempts to use the legacy 
hvm_get_ioreq_server_info() and the guest has >128 vcpus then the call should 
fail.

  Paul

> Now there are several pages, to be compatible
> with previous QEMU, the interface to get the gmfn doesn't change. But
> newer
> QEMU needs to get the gmfn repeatly until a same gmfn is found. To
> implement
> this, an internal index is introduced: when QEMU queries the gmfn, the gmfn
> of
> IOREQ page referenced by the index is returned.  After each operation, the
> index increases by 1 and rewinds when it overflows.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v4:
>  - new
> ---
>  tools/libxc/include/xc_dom.h     |   2 +-
>  tools/libxc/xc_dom_x86.c         |   6 +-
>  xen/arch/x86/hvm/hvm.c           |   1 +
>  xen/arch/x86/hvm/ioreq.c         | 116
> ++++++++++++++++++++++++++++++---------
>  xen/include/asm-x86/hvm/domain.h |   6 +-
>  xen/include/public/hvm/ioreq.h   |   2 +
>  xen/include/public/hvm/params.h  |   8 ++-
>  7 files changed, 110 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 45c9d67..2f8b412 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -20,7 +20,7 @@
>  #include <xenguest.h>
> 
>  #define INVALID_PFN ((xen_pfn_t)-1)
> -#define X86_HVM_NR_SPECIAL_PAGES    8
> +#define X86_HVM_NR_SPECIAL_PAGES    11
>  #define X86_HVM_END_SPECIAL_REGION  0xff000u
> 
>  /* --- typedefs and structs ---------------------------------------- */
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index bff68a0..b316ebc 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -32,6 +32,7 @@
>  #include <xen/foreign/x86_32.h>
>  #include <xen/foreign/x86_64.h>
>  #include <xen/hvm/hvm_info_table.h>
> +#include <xen/hvm/ioreq.h>
>  #include <xen/arch-x86/hvm/start_info.h>
>  #include <xen/io/protocols.h>
> 
> @@ -57,8 +58,8 @@
>  #define SPECIALPAGE_BUFIOREQ 3
>  #define SPECIALPAGE_XENSTORE 4
>  #define SPECIALPAGE_IOREQ    5
> -#define SPECIALPAGE_IDENT_PT 6
> -#define SPECIALPAGE_CONSOLE  7
> +#define SPECIALPAGE_IDENT_PT (5 + MAX_IOREQ_PAGE)
> +#define SPECIALPAGE_CONSOLE  (SPECIALPAGE_IDENT_PT + 1)
>  #define special_pfn(x) \
>      (X86_HVM_END_SPECIAL_REGION - X86_HVM_NR_SPECIAL_PAGES + (x))
> 
> @@ -612,6 +613,7 @@ static int alloc_magic_pages_hvm(struct
> xc_dom_image *dom)
>                                 X86_HVM_NR_SPECIAL_PAGES) )
>              goto error_out;
> 
> +    xc_hvm_param_set(xch, domid, HVM_PARAM_IOREQ_PAGES,
> MAX_IOREQ_PAGE);
>      xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_PFN,
>                       special_pfn(SPECIALPAGE_XENSTORE));
>      xc_hvm_param_set(xch, domid, HVM_PARAM_BUFIOREQ_PFN,
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5d06767..0b3bd04 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4077,6 +4077,7 @@ static int hvm_allow_set_param(struct domain *d,
>      case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>      case HVM_PARAM_ALTP2M:
>      case HVM_PARAM_MCA_CAP:
> +    case HVM_PARAM_IOREQ_PAGES:
>          if ( value != 0 && a->value != value )
>              rc = -EEXIST;
>          break;
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index a879f20..0a36001 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -64,14 +64,24 @@ static struct hvm_ioreq_server
> *get_ioreq_server(const struct domain *d,
>              continue; \
>          else
> 
> +/* Iterate over all ioreq pages */
> +#define FOR_EACH_IOREQ_PAGE(s, i, iorp) \
> +    for ( (i) = 0, iorp = s->ioreq; (i) < (s)->ioreq_page_nr; (i)++, iorp++ )
> +
>  static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v)
>  {
> -    shared_iopage_t *p = s->ioreq.va;
> +    shared_iopage_t *p = s->ioreq[v->vcpu_id /
> IOREQ_NUM_PER_PAGE].va;
> 
>      ASSERT((v == current) || !vcpu_runnable(v));
>      ASSERT(p != NULL);
> 
> -    return &p->vcpu_ioreq[v->vcpu_id];
> +    return &p->vcpu_ioreq[v->vcpu_id % IOREQ_NUM_PER_PAGE];
> +}
> +
> +static ioreq_t *get_ioreq_fallible(struct hvm_ioreq_server *s, struct vcpu
> *v)
> +{
> +    return s->ioreq[v->vcpu_id / IOREQ_NUM_PER_PAGE].va ?
> +           get_ioreq(s, v) : NULL;
>  }
> 
>  bool hvm_io_pending(struct vcpu *v)
> @@ -252,10 +262,10 @@ static void hvm_unmap_ioreq_gfn(struct
> hvm_ioreq_server *s,
>      iorp->gfn = INVALID_GFN;
>  }
> 
> -static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
> +static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf,
> uint8_t i)
>  {
>      struct domain *d = s->domain;
> -    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq[i];
>      int rc;
> 
>      if ( iorp->page )
> @@ -277,7 +287,7 @@ static int hvm_map_ioreq_gfn(struct
> hvm_ioreq_server *s, bool buf)
>      if ( IS_DEFAULT(s) )
>          iorp->gfn = _gfn(buf ?
>                           d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN] :
> -                         d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN]);
> +                         d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN] + i);
>      else
>          iorp->gfn = hvm_alloc_ioreq_gfn(s);
> 
> @@ -366,7 +376,22 @@ bool is_ioreq_server_page(struct domain *d, const
> struct page_info *page)
> 
>      FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
> -        if ( (s->ioreq.page == page) || (s->bufioreq.page == page) )
> +        int i;
> +        const struct hvm_ioreq_page *iorp;
> +
> +        FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +        {
> +            if ( iorp->page == page )
> +            {
> +                found = true;
> +                break;
> +            }
> +        }
> +
> +        if ( found )
> +            break;
> +
> +        if ( s->bufioreq.page == page )
>          {
>              found = true;
>              break;
> @@ -415,14 +440,12 @@ static int hvm_add_ioreq_gfn(struct
> hvm_ioreq_server *s,
>  static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s,
>                                      struct hvm_ioreq_vcpu *sv)
>  {
> -    ASSERT(spin_is_locked(&s->lock));
> +    ioreq_t *p = get_ioreq_fallible(s, sv->vcpu);
> 
> -    if ( s->ioreq.va != NULL )
> -    {
> -        ioreq_t *p = get_ioreq(s, sv->vcpu);
> +    ASSERT(spin_is_locked(&s->lock));
> 
> +    if ( p )
>          p->vp_eport = sv->ioreq_evtchn;
> -    }
>  }
> 
>  #define HANDLE_BUFIOREQ(s) \
> @@ -540,44 +563,66 @@ static void
> hvm_ioreq_server_remove_all_vcpus(struct hvm_ioreq_server *s)
> 
>  static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s)
>  {
> -    int rc;
> +    int i, rc = -EINVAL;
> +    struct hvm_ioreq_page *iorp;
> 
> -    rc = hvm_map_ioreq_gfn(s, false);
> +    for ( i = 0; i < s->ioreq_page_nr; i++ )
> +    {
> +        rc = hvm_map_ioreq_gfn(s, false, i);
> +        if ( rc )
> +            break;
> +    }
> 
>      if ( !rc && HANDLE_BUFIOREQ(s) )
> -        rc = hvm_map_ioreq_gfn(s, true);
> +        rc = hvm_map_ioreq_gfn(s, true, 0);
> 
>      if ( rc )
> -        hvm_unmap_ioreq_gfn(s, &s->ioreq);
> +        FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +            hvm_unmap_ioreq_gfn(s, iorp);
> 
>      return rc;
>  }
> 
>  static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s)
>  {
> -    hvm_unmap_ioreq_gfn(s, &s->ioreq);
> +    int i;
> +    struct hvm_ioreq_page *iorp;
> +
> +    FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +        hvm_unmap_ioreq_gfn(s, iorp);
>      hvm_unmap_ioreq_gfn(s, &s->bufioreq);
>  }
> 
>  static int hvm_ioreq_server_alloc_pages(struct hvm_ioreq_server *s)
>  {
> -    int rc;
> +    int i, rc = -EINVAL;
> +    struct hvm_ioreq_page *iorp;
> 
> -    rc = hvm_alloc_ioreq_mfn(s, &s->ioreq);
> +    FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +    {
> +        rc = hvm_alloc_ioreq_mfn(s, iorp);
> +        if ( rc )
> +            break;
> +    }
> 
>      if ( !rc && (s->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF) )
>          rc = hvm_alloc_ioreq_mfn(s, &s->bufioreq);
> 
>      if ( rc )
> -        hvm_free_ioreq_mfn(s, &s->ioreq);
> +        FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +            hvm_free_ioreq_mfn(s, iorp);
> 
>      return rc;
>  }
> 
>  static void hvm_ioreq_server_free_pages(struct hvm_ioreq_server *s)
>  {
> +    int i;
> +    struct hvm_ioreq_page *iorp;
> +
>      hvm_free_ioreq_mfn(s, &s->bufioreq);
> -    hvm_free_ioreq_mfn(s, &s->ioreq);
> +    FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +        hvm_free_ioreq_mfn(s, iorp);
>  }
> 
>  static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s)
> @@ -638,13 +683,16 @@ static int hvm_ioreq_server_alloc_rangesets(struct
> hvm_ioreq_server *s,
>  static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
>  {
>      struct hvm_ioreq_vcpu *sv;
> +    struct hvm_ioreq_page *iorp;
> +    int i;
> 
>      spin_lock(&s->lock);
> 
>      if ( s->enabled )
>          goto done;
> 
> -    hvm_remove_ioreq_gfn(s, &s->ioreq);
> +    FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +        hvm_remove_ioreq_gfn(s, iorp);
>      hvm_remove_ioreq_gfn(s, &s->bufioreq);
> 
>      s->enabled = true;
> @@ -660,13 +708,17 @@ static void hvm_ioreq_server_enable(struct
> hvm_ioreq_server *s)
> 
>  static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
>  {
> +    struct hvm_ioreq_page *iorp;
> +    int i;
> +
>      spin_lock(&s->lock);
> 
>      if ( !s->enabled )
>          goto done;
> 
>      hvm_add_ioreq_gfn(s, &s->bufioreq);
> -    hvm_add_ioreq_gfn(s, &s->ioreq);
> +    FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +        hvm_add_ioreq_gfn(s, iorp);
> 
>      s->enabled = false;
> 
> @@ -679,7 +731,8 @@ static int hvm_ioreq_server_init(struct
> hvm_ioreq_server *s,
>                                   int bufioreq_handling, ioservid_t id)
>  {
>      struct vcpu *v;
> -    int rc;
> +    int rc, i;
> +    struct hvm_ioreq_page *iorp;
> 
>      s->domain = d;
>      s->domid = domid;
> @@ -688,8 +741,15 @@ static int hvm_ioreq_server_init(struct
> hvm_ioreq_server *s,
>      INIT_LIST_HEAD(&s->ioreq_vcpu_list);
>      spin_lock_init(&s->bufioreq_lock);
> 
> -    s->ioreq.gfn = INVALID_GFN;
> +    FOR_EACH_IOREQ_PAGE(s, i, iorp)
> +        iorp->gfn = INVALID_GFN;
>      s->bufioreq.gfn = INVALID_GFN;
> +    s->ioreq_page_nr = (d->max_vcpus + IOREQ_NUM_PER_PAGE - 1) /
> +                       IOREQ_NUM_PER_PAGE;
> +    if ( s->ioreq_page_nr > d-
> >arch.hvm_domain.params[HVM_PARAM_IOREQ_PAGES] )
> +        return -EINVAL;
> +
> +    s->ioreq_idx = 0;
> 
>      rc = hvm_ioreq_server_alloc_rangesets(s, id);
>      if ( rc )
> @@ -866,7 +926,10 @@ int hvm_get_ioreq_server_info(struct domain *d,
> ioservid_t id,
>      }
> 
>      if ( ioreq_gfn )
> -        *ioreq_gfn = gfn_x(s->ioreq.gfn);
> +    {
> +        *ioreq_gfn = gfn_x(s->ioreq[s->ioreq_idx].gfn);
> +        s->ioreq_idx = (s->ioreq_idx + 1) % s->ioreq_page_nr;
> +    }
> 
>      if ( HANDLE_BUFIOREQ(s) )
>      {
> @@ -916,7 +979,8 @@ int hvm_get_ioreq_server_frame(struct domain *d,
> ioservid_t id,
>          break;
> 
>      case XENMEM_resource_ioreq_server_frame_ioreq(0):
> -        *mfn = _mfn(page_to_mfn(s->ioreq.page));
> +        *mfn = _mfn(page_to_mfn(s->ioreq[s->ioreq_idx].page));
> +        s->ioreq_idx = (s->ioreq_idx + 1) % s->ioreq_page_nr;
>          rc = 0;
>          break;
> 
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-
> x86/hvm/domain.h
> index 87f7994..ff8c44d 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -51,6 +51,7 @@ struct hvm_ioreq_vcpu {
> 
>  #define NR_IO_RANGE_TYPES (XEN_DMOP_IO_RANGE_PCI + 1)
>  #define MAX_NR_IO_RANGES  256
> +#define IOREQ_NUM_PER_PAGE (PAGE_SIZE / sizeof(ioreq_t))
> 
>  struct hvm_ioreq_server {
>      struct list_head       list_entry;
> @@ -61,7 +62,10 @@ struct hvm_ioreq_server {
> 
>      /* Domain id of emulating domain */
>      domid_t                domid;
> -    struct hvm_ioreq_page  ioreq;
> +    /* Index and size of ioreq page array */
> +    uint8_t                ioreq_idx;
> +    uint8_t                ioreq_page_nr;
> +    struct hvm_ioreq_page  ioreq[MAX_IOREQ_PAGE];
>      struct list_head       ioreq_vcpu_list;
>      struct hvm_ioreq_page  bufioreq;
> 
> diff --git a/xen/include/public/hvm/ioreq.h
> b/xen/include/public/hvm/ioreq.h
> index d309d12..d628303 100644
> --- a/xen/include/public/hvm/ioreq.h
> +++ b/xen/include/public/hvm/ioreq.h
> @@ -71,6 +71,8 @@ struct shared_iopage {
>  };
>  typedef struct shared_iopage shared_iopage_t;
> 
> +#define MAX_IOREQ_PAGE 4
> +
>  struct buf_ioreq {
>      uint8_t  type;   /* I/O type                    */
>      uint8_t  pad:1;
> diff --git a/xen/include/public/hvm/params.h
> b/xen/include/public/hvm/params.h
> index 2ec2e7c..537799d 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -279,6 +279,12 @@
>  #define XEN_HVM_MCA_CAP_LMCE   (xen_mk_ullong(1) << 0)
>  #define XEN_HVM_MCA_CAP_MASK   XEN_HVM_MCA_CAP_LMCE
> 
> -#define HVM_NR_PARAMS 39
> +/*
> + * Number of pages that are reserved for default IOREQ server. The base
> PFN
> + * is set via HVM_PARAM_IOREQ_PFN.
> + */
> +#define HVM_PARAM_IOREQ_PAGES 39
> +
> +#define HVM_NR_PARAMS 40
> 
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
> --
> 1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel