[Xen-devel,06/14,v4] xen/arm: vpl011: Add a new domctl API to initialize vpl011

Message ID 1496769929-23355-7-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • PL011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur June 6, 2017, 5:25 p.m.
Add a new domctl API to initialize vpl011. It takes the GFN and console
backend domid as input and returns an event channel to be used for
sending and receiving events from Xen.

Xen will communicate with xenconsole using GFN as the ring buffer and
the event channel to transmit and receive pl011 data on the guest domain's
behalf.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
CC: ij
CC: wl
CC: ss
CC: jg

Changes since v3:
- Added a new arch specific function libxl__arch_domain_create_finish(), which
  calls the vpl011 initialization function. For x86 this function does not do
  anything.
- domain_vpl011_init() takes a pointer to a structure which contains all the 
  required information such as console_domid, gfn instead of passing parameters
  separately.
- Dropped a DOMCTL API defined for de-initializing vpl011 as that should be
  taken care when the domain is destroyed (and not dependent on userspace 
  libraries/applications).

Changes since v2:
- Replaced the DOMCTL APIs defined for get/set of event channel and GFN with 
  a set of DOMCTL APIs for initializing and de-initializing vpl011 emulation.

 tools/libxc/include/xenctrl.h | 17 +++++++++++++++++
 tools/libxc/xc_domain.c       | 23 ++++++++++++++++++++++
 tools/libxl/libxl_arch.h      |  7 +++++++
 tools/libxl/libxl_arm.c       | 19 +++++++++++++++++++
 tools/libxl/libxl_dom.c       |  6 +++++-
 tools/libxl/libxl_x86.c       |  8 ++++++++
 xen/arch/arm/domain.c         |  2 ++
 xen/arch/arm/domctl.c         | 44 ++++++++++++++++++++++++++++++++++++++++---
 xen/include/public/domctl.h   | 12 ++++++++++++
 9 files changed, 134 insertions(+), 4 deletions(-)

Comments

Stefano Stabellini June 6, 2017, 11:26 p.m. | #1
On Tue, 6 Jun 2017, Bhupinder Thakur wrote:
> Add a new domctl API to initialize vpl011. It takes the GFN and console
> backend domid as input and returns an event channel to be used for
> sending and receiving events from Xen.
> 
> Xen will communicate with xenconsole using GFN as the ring buffer and
> the event channel to transmit and receive pl011 data on the guest domain's
> behalf.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> CC: ij
> CC: wl
> CC: ss
> CC: jg
> 
> Changes since v3:
> - Added a new arch specific function libxl__arch_domain_create_finish(), which
>   calls the vpl011 initialization function. For x86 this function does not do
>   anything.
> - domain_vpl011_init() takes a pointer to a structure which contains all the 
>   required information such as console_domid, gfn instead of passing parameters
>   separately.
> - Dropped a DOMCTL API defined for de-initializing vpl011 as that should be
>   taken care when the domain is destroyed (and not dependent on userspace 
>   libraries/applications).
> 
> Changes since v2:
> - Replaced the DOMCTL APIs defined for get/set of event channel and GFN with 
>   a set of DOMCTL APIs for initializing and de-initializing vpl011 emulation.
> 
>  tools/libxc/include/xenctrl.h | 17 +++++++++++++++++
>  tools/libxc/xc_domain.c       | 23 ++++++++++++++++++++++
>  tools/libxl/libxl_arch.h      |  7 +++++++
>  tools/libxl/libxl_arm.c       | 19 +++++++++++++++++++
>  tools/libxl/libxl_dom.c       |  6 +++++-
>  tools/libxl/libxl_x86.c       |  8 ++++++++
>  xen/arch/arm/domain.c         |  2 ++
>  xen/arch/arm/domctl.c         | 44 ++++++++++++++++++++++++++++++++++++++++---
>  xen/include/public/domctl.h   | 12 ++++++++++++
>  9 files changed, 134 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1629f41..77425dd 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -884,6 +884,23 @@ int xc_vcpu_getcontext(xc_interface *xch,
>                         uint32_t domid,
>                         uint32_t vcpu,
>                         vcpu_guest_context_any_t *ctxt);
> +/**
> + * This function initializes the vpl011 emulation and returns
> + * the event to be used by the backend for communicating with
> + * the emulation code.
> + *
> + * @parm xch a handle to an open hypervisor interface
> + * @parm domid the domain to get information from
> + * @parm console_domid the domid of the backend console
> + * @parm gfn the guest pfn to be used as the ring buffer
> + * @parm evtchn the event channel to be used for events
> + * @return 0 on success, negative error on failure
> + */
> +int xc_dom_vpl011_init(xc_interface *xch,
> +                       uint32_t domid,
> +                       uint32_t console_domid,
> +                       xen_pfn_t gfn,
> +                       evtchn_port_t *evtchn);
>  
>  /**
>   * This function returns information about the XSAVE state of a particular
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 00909ad4..a8efd5e 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -343,6 +343,29 @@ int xc_domain_get_guest_width(xc_interface *xch, uint32_t domid,
>      return 0;
>  }
>  
> +int xc_dom_vpl011_init(xc_interface *xch,
> +                       uint32_t domid,
> +                       uint32_t console_domid,
> +                       xen_pfn_t gfn,
> +                       evtchn_port_t *evtchn)
> +{
> +    DECLARE_DOMCTL;
> +    int rc = 0;
> +
> +    domctl.cmd = XEN_DOMCTL_vuart_op;
> +    domctl.domain = (domid_t)domid;
> +    domctl.u.vuart_op.cmd = XEN_DOMCTL_VUART_OP_INIT_VPL011;
> +    domctl.u.vuart_op.console_domid = console_domid;
> +    domctl.u.vuart_op.gfn = gfn;
> +
> +    if ( (rc = do_domctl(xch, &domctl)) < 0 )
> +        return rc;
> +
> +    *evtchn = domctl.u.vuart_op.evtchn;
> +
> +    return rc;
> +}

It looks like this function should be in one of the arm specific files,
such as xc_dom_arm.c (otherwise it becomes available to x86 too).


>  int xc_domain_getinfo(xc_interface *xch,
>                        uint32_t first_domid,
>                        unsigned int max_doms,
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index 5e1fc60..d1ca9c6 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -32,6 +32,13 @@ _hidden
>  int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>                 uint32_t domid);
>  
> +/* arch specific internal domain creation finish function */
> +_hidden
> +int libxl__arch_domain_create_finish(libxl__gc *gc,
> +                                     libxl_domain_build_info *info,
> +                                     uint32_t domid,
> +                                     libxl__domain_build_state *state);
> +
>  /* setup arch specific hardware description, i.e. DTB on ARM */
>  _hidden
>  int libxl__arch_domain_init_hw_description(libxl__gc *gc,
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..b60dfa9 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -106,6 +106,25 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>      return 0;
>  }
>  
> +int libxl__arch_domain_create_finish(libxl__gc *gc,
> +                                     libxl_domain_build_info *info,
> +                                     uint32_t domid,
> +                                     libxl__domain_build_state *state)
> +{
> +    int ret = 0;
> +
> +    if ( info->arch_arm.vuart &&
> +         (ret = xc_dom_vpl011_init(CTX->xch,
> +                                   domid,
> +                                   state->console_domid,
> +                                   xc_get_vuart_gfn(),
> +                                   &state->vuart_port)) != 0 ) {
> +        LOG(ERROR, "xc_dom_vpl011_init failed\n");
> +    }
> +
> +    return ret;
> +}
> +
>  int libxl__arch_extra_memory(libxl__gc *gc,
>                               const libxl_domain_build_info *info,
>                               uint64_t *out)
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 5d914a5..187c5bd 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -587,7 +587,10 @@ retry_transaction:
>              goto retry_transaction;
>      xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port);
>      free(vm_path);
> -    return 0;
> +
> +    rc = libxl__arch_domain_create_finish(gc, info, domid, state);
> +
> +    return rc;
>  }
>  
>  static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
> @@ -788,6 +791,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>      if (xc_dom_translated(dom)) {
>          state->console_mfn = dom->console_pfn;
>          state->store_mfn = dom->xenstore_pfn;
> +        state->vuart_gfn = dom->vuart_gfn;
>      } else {
>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);

vuart_gfn was introduced in patch #4, why are we setting it only now?


> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 455f6f0..3544028 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -358,6 +358,14 @@ out:
>      return ret;
>  }
>  
> +int libxl__arch_domain_create_finish(libxl__gc *gc,
> +                                     libxl_domain_build_info *info,
> +                                     uint32_t domid,
> +                                     libxl__domain_build_state *state)
> +{
> +    return 0;
> +}
> +
>  int libxl__arch_extra_memory(libxl__gc *gc,
>                               const libxl_domain_build_info *info,
>                               uint64_t *out)
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 76310ed..9e150ba 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -665,6 +665,8 @@ fail:
>  
>  void arch_domain_destroy(struct domain *d)
>  {
> +    domain_vpl011_deinit(d);
> +
>      /* IOMMU page table is shared with P2M, always call
>       * iommu_domain_destroy() before p2m_teardown().
>       */

I cannot find the definition of domain_vpl011_deinit


> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 971caec..741679b 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -5,13 +5,15 @@
>   */
>  
>  #include <xen/types.h>
> -#include <xen/lib.h>
> +#include <public/domctl.h>
>  #include <xen/errno.h>
> -#include <xen/sched.h>
> +#include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <xen/iocap.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/sched.h>
>  #include <xsm/xsm.h>
> -#include <public/domctl.h>
>  
>  void arch_get_domain_info(const struct domain *d,
>                            struct xen_domctl_getdomaininfo *info)
> @@ -119,6 +121,42 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          d->disable_migrate = domctl->u.disable_migrate.disable;
>          return 0;
>  
> +    case XEN_DOMCTL_vuart_op:
> +    {
> +        int rc;
> +        struct xen_domctl_vuart_op *vuart_op = &domctl->u.vuart_op;
> +
> +        switch(vuart_op->cmd)
> +        {
> +        case XEN_DOMCTL_VUART_OP_INIT_VPL011:
> +
> +            if ( !d->creation_finished )
> +            {
> +                struct vpl011_init_info info;
> +
> +                info.console_domid = vuart_op->console_domid;
> +                info.gfn = _gfn(vuart_op->gfn);
> +
> +                rc = domain_vpl011_init(d, &info);
> +                if ( !rc )
> +                {
> +                    vuart_op->evtchn = info.evtchn;
> +                    rc = __copy_to_guest(u_domctl, domctl, 1);
> +                }
> +            }
> +            else
> +            {
> +                rc = - EPERM;
> +            }
> +            break;
> +
> +        default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        return rc;
> +    }
>      default:
>      {
>          int rc;
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index e6cf211..c6ff458 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -36,6 +36,7 @@
>  #include "grant_table.h"
>  #include "hvm/save.h"
>  #include "memory.h"
> +#include "event_channel.h"
>  
>  #define XEN_DOMCTL_INTERFACE_VERSION 0x0000000d
>  
> @@ -1138,6 +1139,15 @@ struct xen_domctl_psr_cat_op {
>      uint32_t target;    /* IN */
>      uint64_t data;      /* IN/OUT */
>  };
> +
> +struct xen_domctl_vuart_op {
> +#define XEN_DOMCTL_VUART_OP_INIT_VPL011  0
> +        uint32_t cmd;           /* XEN_DOMCTL_VUART_OP_* */
> +        uint32_t console_domid; /* IN */
> +        xen_pfn_t gfn;          /* IN */
> +        evtchn_port_t evtchn;   /* OUT */
> +};
> +
>  typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>  
> @@ -1218,6 +1228,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_monitor_op                    77
>  #define XEN_DOMCTL_psr_cat_op                    78
>  #define XEN_DOMCTL_soft_reset                    79
> +#define XEN_DOMCTL_vuart_op                      80
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -1280,6 +1291,7 @@ struct xen_domctl {
>          struct xen_domctl_psr_cmt_op        psr_cmt_op;
>          struct xen_domctl_monitor_op        monitor_op;
>          struct xen_domctl_psr_cat_op        psr_cat_op;
> +        struct xen_domctl_vuart_op          vuart_op;
>          uint8_t                             pad[128];
>      } u;
>  };
> -- 
> 2.7.4
>
Julien Grall June 9, 2017, 2:06 p.m. | #2
Hi Stefano,

On 07/06/17 00:26, Stefano Stabellini wrote:
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index 00909ad4..a8efd5e 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -343,6 +343,29 @@ int xc_domain_get_guest_width(xc_interface *xch, uint32_t domid,
>>      return 0;
>>  }
>>
>> +int xc_dom_vpl011_init(xc_interface *xch,
>> +                       uint32_t domid,
>> +                       uint32_t console_domid,
>> +                       xen_pfn_t gfn,
>> +                       evtchn_port_t *evtchn)
>> +{
>> +    DECLARE_DOMCTL;
>> +    int rc = 0;
>> +
>> +    domctl.cmd = XEN_DOMCTL_vuart_op;
>> +    domctl.domain = (domid_t)domid;
>> +    domctl.u.vuart_op.cmd = XEN_DOMCTL_VUART_OP_INIT_VPL011;
>> +    domctl.u.vuart_op.console_domid = console_domid;
>> +    domctl.u.vuart_op.gfn = gfn;
>> +
>> +    if ( (rc = do_domctl(xch, &domctl)) < 0 )
>> +        return rc;
>> +
>> +    *evtchn = domctl.u.vuart_op.evtchn;
>> +
>> +    return rc;
>> +}
>
> It looks like this function should be in one of the arm specific files,
> such as xc_dom_arm.c (otherwise it becomes available to x86 too).

AFAICT xc_dom_arm.c has a completely different purpose. Looking at other 
helpers, it seems the usage if too #ifdef helpers (see 
xc_vcpu_get_extstate or xc_domain_set_memory_map).

[...]

>> +int libxl__arch_domain_create_finish(libxl__gc *gc,
>> +                                     libxl_domain_build_info *info,
>> +                                     uint32_t domid,
>> +                                     libxl__domain_build_state *state)
>> +{
>> +    return 0;
>> +}
>> +
>>  int libxl__arch_extra_memory(libxl__gc *gc,
>>                               const libxl_domain_build_info *info,
>>                               uint64_t *out)
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 76310ed..9e150ba 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -665,6 +665,8 @@ fail:
>>
>>  void arch_domain_destroy(struct domain *d)
>>  {
>> +    domain_vpl011_deinit(d);
>> +
>>      /* IOMMU page table is shared with P2M, always call
>>       * iommu_domain_destroy() before p2m_teardown().
>>       */
>
> I cannot find the definition of domain_vpl011_deinit

See patch #3.

Cheers,
Julien Grall June 9, 2017, 2:13 p.m. | #3
Hi Bhupinder,

On 06/06/17 18:25, Bhupinder Thakur wrote:
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1629f41..77425dd 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -884,6 +884,23 @@ int xc_vcpu_getcontext(xc_interface *xch,
>                         uint32_t domid,
>                         uint32_t vcpu,
>                         vcpu_guest_context_any_t *ctxt);

Newline here please.

[...]

> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index 5e1fc60..d1ca9c6 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -32,6 +32,13 @@ _hidden
>  int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>                 uint32_t domid);
>
> +/* arch specific internal domain creation finish function */
> +_hidden
> +int libxl__arch_domain_create_finish(libxl__gc *gc,
> +                                     libxl_domain_build_info *info,
> +                                     uint32_t domid,
> +                                     libxl__domain_build_state *state);

Can you explain why you need a new arch helper rather than using the 
current one?

[...]

> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 76310ed..9e150ba 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -665,6 +665,8 @@ fail:
>
>  void arch_domain_destroy(struct domain *d)
>  {
> +    domain_vpl011_deinit(d);

Please add a comment explain where the initialization has been done (i.e 
via a DOMCTL). This would make easier to know what's going on.

> +
>      /* IOMMU page table is shared with P2M, always call
>       * iommu_domain_destroy() before p2m_teardown().
>       */
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 971caec..741679b 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -5,13 +5,15 @@
>   */
>
>  #include <xen/types.h>
> -#include <xen/lib.h>
> +#include <public/domctl.h>
>  #include <xen/errno.h>
> -#include <xen/sched.h>
> +#include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <xen/iocap.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/sched.h>
>  #include <xsm/xsm.h>
> -#include <public/domctl.h>

Why do you reshuffle the headers? Is it to use the alphabetical order? 
If so, this should really be done in a separate patch.

>
>  void arch_get_domain_info(const struct domain *d,
>                            struct xen_domctl_getdomaininfo *info)
> @@ -119,6 +121,42 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          d->disable_migrate = domctl->u.disable_migrate.disable;
>          return 0;
>
> +    case XEN_DOMCTL_vuart_op:
> +    {
> +        int rc;
> +        struct xen_domctl_vuart_op *vuart_op = &domctl->u.vuart_op;
> +
> +        switch(vuart_op->cmd)
> +        {
> +        case XEN_DOMCTL_VUART_OP_INIT_VPL011:
> +
> +            if ( !d->creation_finished )
> +            {
> +                struct vpl011_init_info info;
> +
> +                info.console_domid = vuart_op->console_domid;
> +                info.gfn = _gfn(vuart_op->gfn);
> +
> +                rc = domain_vpl011_init(d, &info);
> +                if ( !rc )
> +                {
> +                    vuart_op->evtchn = info.evtchn;
> +                    rc = __copy_to_guest(u_domctl, domctl, 1);
> +                }
> +            }
> +            else
> +            {
> +                rc = - EPERM;
> +            }

Unecessary {}.

> +            break;
> +
> +        default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +
> +        return rc;
> +    }
>      default:
>      {
>          int rc;

Cheers,
Stefano Stabellini June 9, 2017, 6:32 p.m. | #4
On Fri, 9 Jun 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/06/17 00:26, Stefano Stabellini wrote:
> > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > > index 00909ad4..a8efd5e 100644
> > > --- a/tools/libxc/xc_domain.c
> > > +++ b/tools/libxc/xc_domain.c
> > > @@ -343,6 +343,29 @@ int xc_domain_get_guest_width(xc_interface *xch,
> > > uint32_t domid,
> > >      return 0;
> > >  }
> > > 
> > > +int xc_dom_vpl011_init(xc_interface *xch,
> > > +                       uint32_t domid,
> > > +                       uint32_t console_domid,
> > > +                       xen_pfn_t gfn,
> > > +                       evtchn_port_t *evtchn)
> > > +{
> > > +    DECLARE_DOMCTL;
> > > +    int rc = 0;
> > > +
> > > +    domctl.cmd = XEN_DOMCTL_vuart_op;
> > > +    domctl.domain = (domid_t)domid;
> > > +    domctl.u.vuart_op.cmd = XEN_DOMCTL_VUART_OP_INIT_VPL011;
> > > +    domctl.u.vuart_op.console_domid = console_domid;
> > > +    domctl.u.vuart_op.gfn = gfn;
> > > +
> > > +    if ( (rc = do_domctl(xch, &domctl)) < 0 )
> > > +        return rc;
> > > +
> > > +    *evtchn = domctl.u.vuart_op.evtchn;
> > > +
> > > +    return rc;
> > > +}
> > 
> > It looks like this function should be in one of the arm specific files,
> > such as xc_dom_arm.c (otherwise it becomes available to x86 too).
> 
> AFAICT xc_dom_arm.c has a completely different purpose. Looking at other
> helpers, it seems the usage if too #ifdef helpers (see xc_vcpu_get_extstate or
> xc_domain_set_memory_map).

That's true. It's best to continue that pattern and use #ifdefs here.


> > > +int libxl__arch_domain_create_finish(libxl__gc *gc,
> > > +                                     libxl_domain_build_info *info,
> > > +                                     uint32_t domid,
> > > +                                     libxl__domain_build_state *state)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >  int libxl__arch_extra_memory(libxl__gc *gc,
> > >                               const libxl_domain_build_info *info,
> > >                               uint64_t *out)
> > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > index 76310ed..9e150ba 100644
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -665,6 +665,8 @@ fail:
> > > 
> > >  void arch_domain_destroy(struct domain *d)
> > >  {
> > > +    domain_vpl011_deinit(d);
> > > +
> > >      /* IOMMU page table is shared with P2M, always call
> > >       * iommu_domain_destroy() before p2m_teardown().
> > >       */
> > 
> > I cannot find the definition of domain_vpl011_deinit
> 
> See patch #3.

All right, thanks. Initially I thought it was weird for this change to
be here, but now I think it makes sense because this patch introduces
the call to domain_vpl011_init.
Bhupinder Thakur June 14, 2017, 7:35 a.m. | #5
Hi Stefano,

On 7 June 2017 at 04:56, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>  static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>> @@ -788,6 +791,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>      if (xc_dom_translated(dom)) {
>>          state->console_mfn = dom->console_pfn;
>>          state->store_mfn = dom->xenstore_pfn;
>> +        state->vuart_gfn = dom->vuart_gfn;
>>      } else {
>>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
>
> vuart_gfn was introduced in patch #4, why are we setting it only now?

I think this change can be moved to patch #5, which initializes
dom->vuart_gfn, which is required for this change.

Regards,
Bhupinder
Bhupinder Thakur June 14, 2017, 8:34 a.m. | #6
On 14 June 2017 at 13:05, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
> Hi Stefano,
>
> On 7 June 2017 at 04:56, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>  static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
>>> @@ -788,6 +791,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>>      if (xc_dom_translated(dom)) {
>>>          state->console_mfn = dom->console_pfn;
>>>          state->store_mfn = dom->xenstore_pfn;
>>> +        state->vuart_gfn = dom->vuart_gfn;
>>>      } else {
>>>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>>>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
>>
>> vuart_gfn was introduced in patch #4, why are we setting it only now?
>
> I think this change can be moved to patch #5, which initializes
> dom->vuart_gfn, which is required for this change.

I will move this change to patch #4 only and move patch #4 after patch #5.

Regards,
Bhupinder
Bhupinder Thakur June 14, 2017, 9:16 a.m. | #7
Hi Julien,


>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 1629f41..77425dd 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -884,6 +884,23 @@ int xc_vcpu_getcontext(xc_interface *xch,
>>                         uint32_t domid,
>>                         uint32_t vcpu,
>>                         vcpu_guest_context_any_t *ctxt);
>
>
> Newline here please.
>
ok.
> [...]
>
>> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
>> index 5e1fc60..d1ca9c6 100644
>> --- a/tools/libxl/libxl_arch.h
>> +++ b/tools/libxl/libxl_arch.h
>> @@ -32,6 +32,13 @@ _hidden
>>  int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config
>> *d_config,
>>                 uint32_t domid);
>>
>> +/* arch specific internal domain creation finish function */
>> +_hidden
>> +int libxl__arch_domain_create_finish(libxl__gc *gc,
>> +                                     libxl_domain_build_info *info,
>> +                                     uint32_t domid,
>> +                                     libxl__domain_build_state *state);
>
>
> Can you explain why you need a new arch helper rather than using the current
> one?

libxl__arch_domain_create() is called from libxl__build_pre(). This
function is called before libxl__build_pv(). By this time the domain
has not be created and I found that if I tried to initialize vpl011
from inside libxl__arch_domain_create() then initialization was
failing due to prepare_ring_for_helper() failing.

So I had to create another function which will be called from
libxl__build_post() after domain has been setup.

>
> [...]
>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 76310ed..9e150ba 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -665,6 +665,8 @@ fail:
>>
>>  void arch_domain_destroy(struct domain *d)
>>  {
>> +    domain_vpl011_deinit(d);
>
>
> Please add a comment explain where the initialization has been done (i.e via
> a DOMCTL). This would make easier to know what's going on.
ok.
>
>> +
>>      /* IOMMU page table is shared with P2M, always call
>>       * iommu_domain_destroy() before p2m_teardown().
>>       */
>> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
>> index 971caec..741679b 100644
>> --- a/xen/arch/arm/domctl.c
>> +++ b/xen/arch/arm/domctl.c
>> @@ -5,13 +5,15 @@
>>   */
>>
>>  #include <xen/types.h>
>> -#include <xen/lib.h>
>> +#include <public/domctl.h>
>>  #include <xen/errno.h>
>> -#include <xen/sched.h>
>> +#include <xen/guest_access.h>
>>  #include <xen/hypercall.h>
>>  #include <xen/iocap.h>
>> +#include <xen/lib.h>
>> +#include <xen/mm.h>
>> +#include <xen/sched.h>
>>  #include <xsm/xsm.h>
>> -#include <public/domctl.h>
>
>
> Why do you reshuffle the headers? Is it to use the alphabetical order? If
> so, this should really be done in a separate patch.
>
ok. I will introduce a new patch for header files reshuffling.

>
>>
>>  void arch_get_domain_info(const struct domain *d,
>>                            struct xen_domctl_getdomaininfo *info)
>> @@ -119,6 +121,42 @@ long arch_do_domctl(struct xen_domctl *domctl, struct
>> domain *d,
>>          d->disable_migrate = domctl->u.disable_migrate.disable;
>>          return 0;
>>
>> +    case XEN_DOMCTL_vuart_op:
>> +    {
>> +        int rc;
>> +        struct xen_domctl_vuart_op *vuart_op = &domctl->u.vuart_op;
>> +
>> +        switch(vuart_op->cmd)
>> +        {
>> +        case XEN_DOMCTL_VUART_OP_INIT_VPL011:
>> +
>> +            if ( !d->creation_finished )
>> +            {
>> +                struct vpl011_init_info info;
>> +
>> +                info.console_domid = vuart_op->console_domid;
>> +                info.gfn = _gfn(vuart_op->gfn);
>> +
>> +                rc = domain_vpl011_init(d, &info);
>> +                if ( !rc )
>> +                {
>> +                    vuart_op->evtchn = info.evtchn;
>> +                    rc = __copy_to_guest(u_domctl, domctl, 1);
>> +                }
>> +            }
>> +            else
>> +            {
>> +                rc = - EPERM;
>> +            }
>
>
> Unecessary {}.
>
ok.

Regards,
Bhupinder
Julien Grall June 14, 2017, 10:15 a.m. | #8
On 06/14/2017 10:16 AM, Bhupinder Thakur wrote:
> Hi Julien,

Hi Bhupinder,

> 
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index 1629f41..77425dd 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -884,6 +884,23 @@ int xc_vcpu_getcontext(xc_interface *xch,
>>>                          uint32_t domid,
>>>                          uint32_t vcpu,
>>>                          vcpu_guest_context_any_t *ctxt);
>>
>>
>> Newline here please.
>>
> ok.
>> [...]
>>
>>> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
>>> index 5e1fc60..d1ca9c6 100644
>>> --- a/tools/libxl/libxl_arch.h
>>> +++ b/tools/libxl/libxl_arch.h
>>> @@ -32,6 +32,13 @@ _hidden
>>>   int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config
>>> *d_config,
>>>                  uint32_t domid);
>>>
>>> +/* arch specific internal domain creation finish function */
>>> +_hidden
>>> +int libxl__arch_domain_create_finish(libxl__gc *gc,
>>> +                                     libxl_domain_build_info *info,
>>> +                                     uint32_t domid,
>>> +                                     libxl__domain_build_state *state);
>>
>>
>> Can you explain why you need a new arch helper rather than using the current
>> one?
> 
> libxl__arch_domain_create() is called from libxl__build_pre(). This
> function is called before libxl__build_pv(). By this time the domain
> has not be created and I found that if I tried to initialize vpl011
> from inside libxl__arch_domain_create() then initialization was
> failing due to prepare_ring_for_helper() failing.

What do you mean by the domain has not been created? The domain has 
already been created (you have a domid in hand) when you 
libxl__build_pre. So the problem is different.

Looking at the code, I guess the problem is because the vuart pfn will 
be allocated by xc_dom_build_image called by libxl_build_pv -> 
libxl__build_dom.

> 
> So I had to create another function which will be called from
> libxl__build_post() after domain has been setup.

It looks a bit odd to me to create the vpl011 UART that late in the 
process because when you read the code you would expect all the hardware 
to be setup after libxl__arch_domain_finalise_hw_descriptions is called.

But I understand it is not possible to do it as the ring has not yet 
been allocated. So is there a way to allocate the ring before?

Wei, Ian, do you have any opinions on what should the workflow in libxl?

Cheers,
Bhupinder Thakur June 15, 2017, 6:33 a.m. | #9
Hi Julien,


>>>> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
>>>> index 5e1fc60..d1ca9c6 100644
>>>> --- a/tools/libxl/libxl_arch.h
>>>> +++ b/tools/libxl/libxl_arch.h
>>>> @@ -32,6 +32,13 @@ _hidden
>>>>   int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config
>>>> *d_config,
>>>>                  uint32_t domid);
>>>>
>>>> +/* arch specific internal domain creation finish function */
>>>> +_hidden
>>>> +int libxl__arch_domain_create_finish(libxl__gc *gc,
>>>> +                                     libxl_domain_build_info *info,
>>>> +                                     uint32_t domid,
>>>> +                                     libxl__domain_build_state *state);
>>>
>>>
>>>
>>> Can you explain why you need a new arch helper rather than using the
>>> current
>>> one?
>>
>>
>> libxl__arch_domain_create() is called from libxl__build_pre(). This
>> function is called before libxl__build_pv(). By this time the domain
>> has not be created and I found that if I tried to initialize vpl011
>> from inside libxl__arch_domain_create() then initialization was
>> failing due to prepare_ring_for_helper() failing.
>
>
> What do you mean by the domain has not been created? The domain has already
> been created (you have a domid in hand) when you libxl__build_pre. So the
> problem is different.
>
> Looking at the code, I guess the problem is because the vuart pfn will be
> allocated by xc_dom_build_image called by libxl_build_pv ->
> libxl__build_dom.
>
>>
>> So I had to create another function which will be called from
>> libxl__build_post() after domain has been setup.
>
>
> It looks a bit odd to me to create the vpl011 UART that late in the process
> because when you read the code you would expect all the hardware to be setup
> after libxl__arch_domain_finalise_hw_descriptions is called.
>
> But I understand it is not possible to do it as the ring has not yet been
> allocated. So is there a way to allocate the ring before?
 >
> Wei, Ian, do you have any opinions on what should the workflow in libxl?

Actually, I had introduced an API xc_get_vuart_gfn() to get the pfn.
Since it is a fixed pfn, the API can
return it even before xc_build_dom_image() is called. I tried after
moving the vpl011_init function to libxl__arch_domain_create() and it
is working.

Regards,
Bhupinder
Bhupinder Thakur June 19, 2017, 10:59 a.m. | #10
Hi Julien,

I was mistaken in my earlier mail about vpl011 init working if it is
moved to libxl__arch_domain_create(). It is failing because as you
have mentioned vuart_pfn is allocated later in xc_dom_build_image().

Can we delay mapping of this page in Xen until the ring buffer is
actually required by the emulation code for reading/writing data. By
that time, the page would have been physically mapped.

Regards,
Bhupinder

On 15 June 2017 at 12:03, Bhupinder Thakur <bhupinder.thakur@linaro.org> wrote:
> Hi Julien,
>
>
>>>>> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
>>>>> index 5e1fc60..d1ca9c6 100644
>>>>> --- a/tools/libxl/libxl_arch.h
>>>>> +++ b/tools/libxl/libxl_arch.h
>>>>> @@ -32,6 +32,13 @@ _hidden
>>>>>   int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config
>>>>> *d_config,
>>>>>                  uint32_t domid);
>>>>>
>>>>> +/* arch specific internal domain creation finish function */
>>>>> +_hidden
>>>>> +int libxl__arch_domain_create_finish(libxl__gc *gc,
>>>>> +                                     libxl_domain_build_info *info,
>>>>> +                                     uint32_t domid,
>>>>> +                                     libxl__domain_build_state *state);
>>>>
>>>>
>>>>
>>>> Can you explain why you need a new arch helper rather than using the
>>>> current
>>>> one?
>>>
>>>
>>> libxl__arch_domain_create() is called from libxl__build_pre(). This
>>> function is called before libxl__build_pv(). By this time the domain
>>> has not be created and I found that if I tried to initialize vpl011
>>> from inside libxl__arch_domain_create() then initialization was
>>> failing due to prepare_ring_for_helper() failing.
>>
>>
>> What do you mean by the domain has not been created? The domain has already
>> been created (you have a domid in hand) when you libxl__build_pre. So the
>> problem is different.
>>
>> Looking at the code, I guess the problem is because the vuart pfn will be
>> allocated by xc_dom_build_image called by libxl_build_pv ->
>> libxl__build_dom.
>>
>>>
>>> So I had to create another function which will be called from
>>> libxl__build_post() after domain has been setup.
>>
>>
>> It looks a bit odd to me to create the vpl011 UART that late in the process
>> because when you read the code you would expect all the hardware to be setup
>> after libxl__arch_domain_finalise_hw_descriptions is called.
>>
>> But I understand it is not possible to do it as the ring has not yet been
>> allocated. So is there a way to allocate the ring before?
>  >
>> Wei, Ian, do you have any opinions on what should the workflow in libxl?
>
> Actually, I had introduced an API xc_get_vuart_gfn() to get the pfn.
> Since it is a fixed pfn, the API can
> return it even before xc_build_dom_image() is called. I tried after
> moving the vpl011_init function to libxl__arch_domain_create() and it
> is working.
>
> Regards,
> Bhupinder
Julien Grall June 19, 2017, 11:01 a.m. | #11
On 19/06/17 11:59, Bhupinder Thakur wrote:
> Hi Julien,
>
> I was mistaken in my earlier mail about vpl011 init working if it is
> moved to libxl__arch_domain_create(). It is failing because as you
> have mentioned vuart_pfn is allocated later in xc_dom_build_image().
>
> Can we delay mapping of this page in Xen until the ring buffer is
> actually required by the emulation code for reading/writing data. By
> that time, the page would have been physically mapped.

You would not be able to report an error if you fail to map it. But this 
looks like to me a workaround for a tool problem.

Anyway, as I said, I'd like feedback from the tools maintainers to see 
how we can proceed.

Cheers,
Wei Liu June 19, 2017, 11:47 a.m. | #12
On Mon, Jun 19, 2017 at 12:01:32PM +0100, Julien Grall wrote:
> 
> 
> On 19/06/17 11:59, Bhupinder Thakur wrote:
> > Hi Julien,
> > 
> > I was mistaken in my earlier mail about vpl011 init working if it is
> > moved to libxl__arch_domain_create(). It is failing because as you
> > have mentioned vuart_pfn is allocated later in xc_dom_build_image().
> > 
> > Can we delay mapping of this page in Xen until the ring buffer is
> > actually required by the emulation code for reading/writing data. By
> > that time, the page would have been physically mapped.
> 
> You would not be able to report an error if you fail to map it. But this
> looks like to me a workaround for a tool problem.
> 
> Anyway, as I said, I'd like feedback from the tools maintainers to see how
> we can proceed.
> 

Is there a summary of the problem, is there a particular email in this
thread I should look at? Sorry I'm swamped by emails and patches at the
moment.
Bhupinder Thakur June 19, 2017, 1:11 p.m. | #13
Hi Wei,

On 19 June 2017 at 17:17, Wei Liu <wei.liu2@citrix.com> wrote:
> On Mon, Jun 19, 2017 at 12:01:32PM +0100, Julien Grall wrote:
>>
>>
>> On 19/06/17 11:59, Bhupinder Thakur wrote:
>> > Hi Julien,
>> >
>> > I was mistaken in my earlier mail about vpl011 init working if it is
>> > moved to libxl__arch_domain_create(). It is failing because as you
>> > have mentioned vuart_pfn is allocated later in xc_dom_build_image().
>> >
>> > Can we delay mapping of this page in Xen until the ring buffer is
>> > actually required by the emulation code for reading/writing data. By
>> > that time, the page would have been physically mapped.
>>
>> You would not be able to report an error if you fail to map it. But this
>> looks like to me a workaround for a tool problem.
>>
>> Anyway, as I said, I'd like feedback from the tools maintainers to see how
>> we can proceed.
>>
>
> Is there a summary of the problem, is there a particular email in this
> thread I should look at? Sorry I'm swamped by emails and patches at the
> moment.

I will summarize the problem.

It was decided to call domain_vpl011_init() from inside
libxl__arch_domain_create() to initialize vpl011. However,
domain_vpl011_init() fails to map the the vuart GFN because it has not
been physically mapped yet by the toolstack.

The following call flows highlights the issue.

libxl__domain_build() ---> libxl__build_pv ---> libxl__build_dom()
----> xc_dom_build_image() ---> alloc_magic_pages() ----> vuart GFN
allocated/mapped here

libxl__domain_build() ----> libxl__build_pre()  ---->
libxl__arch_domain_create() ----> domain_vpl011_init() ---> this call
fails as the vuart GFN has not been physically mapped yet as shown in
the first call flow.

However, libxl__build_pv() is called after libxl__build_pre(). It
means that the domain_vpl011_init() is called before
alloc_magic_pages() is called and hence the initialization fails.

For that reason, I had introduced a new function
libxl__arch_domain_create_finish() which will be called from
libxl__build_post(). I moved the domain_vpl011_init() there. However,
Julien pointed out that vuart should be initialized early in
libxl__arch_domain_create() function.

So the issue is what is the right place to call domain_vpl011_init()?

I hope it clarifies the issue.

Regards,
Bhupinder
Julien Grall June 20, 2017, 11:16 a.m. | #14
On 06/19/2017 02:11 PM, Bhupinder Thakur wrote:
> Hi Wei,

Hi Bhupinder,

> On 19 June 2017 at 17:17, Wei Liu <wei.liu2@citrix.com> wrote:
>> On Mon, Jun 19, 2017 at 12:01:32PM +0100, Julien Grall wrote:
>>>
>>>
>>> On 19/06/17 11:59, Bhupinder Thakur wrote:
>>>> Hi Julien,
>>>>
>>>> I was mistaken in my earlier mail about vpl011 init working if it is
>>>> moved to libxl__arch_domain_create(). It is failing because as you
>>>> have mentioned vuart_pfn is allocated later in xc_dom_build_image().
>>>>
>>>> Can we delay mapping of this page in Xen until the ring buffer is
>>>> actually required by the emulation code for reading/writing data. By
>>>> that time, the page would have been physically mapped.
>>>
>>> You would not be able to report an error if you fail to map it. But this
>>> looks like to me a workaround for a tool problem.
>>>
>>> Anyway, as I said, I'd like feedback from the tools maintainers to see how
>>> we can proceed.
>>>
>>
>> Is there a summary of the problem, is there a particular email in this
>> thread I should look at? Sorry I'm swamped by emails and patches at the
>> moment.
> 
> I will summarize the problem.
> 
> It was decided to call domain_vpl011_init() from inside
> libxl__arch_domain_create() to initialize vpl011. However,
> domain_vpl011_init() fails to map the the vuart GFN because it has not
> been physically mapped yet by the toolstack.
> 
> The following call flows highlights the issue.
> 
> libxl__domain_build() ---> libxl__build_pv ---> libxl__build_dom()
> ----> xc_dom_build_image() ---> alloc_magic_pages() ----> vuart GFN
> allocated/mapped here
> 
> libxl__domain_build() ----> libxl__build_pre()  ---->
> libxl__arch_domain_create() ----> domain_vpl011_init() ---> this call
> fails as the vuart GFN has not been physically mapped yet as shown in
> the first call flow.
> 
> However, libxl__build_pv() is called after libxl__build_pre(). It
> means that the domain_vpl011_init() is called before
> alloc_magic_pages() is called and hence the initialization fails.
> 
> For that reason, I had introduced a new function
> libxl__arch_domain_create_finish() which will be called from
> libxl__build_post(). I moved the domain_vpl011_init() there. However,
> Julien pointed out that vuart should be initialized early in
> libxl__arch_domain_create() function. 

libxl__arch_domain_create could be a place or even 
libxl__arch_domain_finalise_hw_descriptions.

My point is it looks a bit odd to create the vpl011 UART very late in 
the process as from the code you would expect all the hardware to be 
setup after libxl__arch_domain_finialise_hw_descriptions is called.

> 
> So the issue is what is the right place to call domain_vpl011_init()?
> 
> I hope it clarifies the issue.

Cheers,
Wei Liu June 20, 2017, 11:42 a.m. | #15
On Tue, Jun 20, 2017 at 12:16:52PM +0100, Julien Grall wrote:
> On 06/19/2017 02:11 PM, Bhupinder Thakur wrote:
> > Hi Wei,
> 
> Hi Bhupinder,
> 
> > On 19 June 2017 at 17:17, Wei Liu <wei.liu2@citrix.com> wrote:
> > > On Mon, Jun 19, 2017 at 12:01:32PM +0100, Julien Grall wrote:
> > > > 
> > > > 
> > > > On 19/06/17 11:59, Bhupinder Thakur wrote:
> > > > > Hi Julien,
> > > > > 
> > > > > I was mistaken in my earlier mail about vpl011 init working if it is
> > > > > moved to libxl__arch_domain_create(). It is failing because as you
> > > > > have mentioned vuart_pfn is allocated later in xc_dom_build_image().
> > > > > 
> > > > > Can we delay mapping of this page in Xen until the ring buffer is
> > > > > actually required by the emulation code for reading/writing data. By
> > > > > that time, the page would have been physically mapped.
> > > > 
> > > > You would not be able to report an error if you fail to map it. But this
> > > > looks like to me a workaround for a tool problem.
> > > > 
> > > > Anyway, as I said, I'd like feedback from the tools maintainers to see how
> > > > we can proceed.
> > > > 
> > > 
> > > Is there a summary of the problem, is there a particular email in this
> > > thread I should look at? Sorry I'm swamped by emails and patches at the
> > > moment.
> > 
> > I will summarize the problem.
> > 
> > It was decided to call domain_vpl011_init() from inside
> > libxl__arch_domain_create() to initialize vpl011. However,
> > domain_vpl011_init() fails to map the the vuart GFN because it has not
> > been physically mapped yet by the toolstack.
> > 
> > The following call flows highlights the issue.
> > 
> > libxl__domain_build() ---> libxl__build_pv ---> libxl__build_dom()
> > ----> xc_dom_build_image() ---> alloc_magic_pages() ----> vuart GFN
> > allocated/mapped here
> > 
> > libxl__domain_build() ----> libxl__build_pre()  ---->
> > libxl__arch_domain_create() ----> domain_vpl011_init() ---> this call
> > fails as the vuart GFN has not been physically mapped yet as shown in
> > the first call flow.
> > 
> > However, libxl__build_pv() is called after libxl__build_pre(). It
> > means that the domain_vpl011_init() is called before
> > alloc_magic_pages() is called and hence the initialization fails.
> > 
> > For that reason, I had introduced a new function
> > libxl__arch_domain_create_finish() which will be called from
> > libxl__build_post(). I moved the domain_vpl011_init() there. However,
> > Julien pointed out that vuart should be initialized early in
> > libxl__arch_domain_create() function.
> 
> libxl__arch_domain_create could be a place or even
> libxl__arch_domain_finalise_hw_descriptions.
> 

libxl__arch_domain_finialise_hw_descriptions sounds like a good place to
me.
Bhupinder Thakur June 21, 2017, 10:18 a.m. | #16
Hi Julien,

On 20 June 2017 at 16:46, Julien Grall <julien.grall@arm.com> wrote:
> On 06/19/2017 02:11 PM, Bhupinder Thakur wrote:
>>
>> Hi Wei,
>
>
> Hi Bhupinder,
>
>
>> On 19 June 2017 at 17:17, Wei Liu <wei.liu2@citrix.com> wrote:
>>>
>>> On Mon, Jun 19, 2017 at 12:01:32PM +0100, Julien Grall wrote:
>>>>
>>>>
>>>>
>>>> On 19/06/17 11:59, Bhupinder Thakur wrote:
>>>>>
>>>>> Hi Julien,
>>>>>
>>>>> I was mistaken in my earlier mail about vpl011 init working if it is
>>>>> moved to libxl__arch_domain_create(). It is failing because as you
>>>>> have mentioned vuart_pfn is allocated later in xc_dom_build_image().
>>>>>
>>>>> Can we delay mapping of this page in Xen until the ring buffer is
>>>>> actually required by the emulation code for reading/writing data. By
>>>>> that time, the page would have been physically mapped.
>>>>
>>>>
>>>> You would not be able to report an error if you fail to map it. But this
>>>> looks like to me a workaround for a tool problem.
>>>>
>>>> Anyway, as I said, I'd like feedback from the tools maintainers to see
>>>> how
>>>> we can proceed.
>>>>
>>>
>>> Is there a summary of the problem, is there a particular email in this
>>> thread I should look at? Sorry I'm swamped by emails and patches at the
>>> moment.
>>
>>
>> I will summarize the problem.
>>
>> It was decided to call domain_vpl011_init() from inside
>> libxl__arch_domain_create() to initialize vpl011. However,
>> domain_vpl011_init() fails to map the the vuart GFN because it has not
>> been physically mapped yet by the toolstack.
>>
>> The following call flows highlights the issue.
>>
>> libxl__domain_build() ---> libxl__build_pv ---> libxl__build_dom()
>> ----> xc_dom_build_image() ---> alloc_magic_pages() ----> vuart GFN
>> allocated/mapped here
>>
>> libxl__domain_build() ----> libxl__build_pre()  ---->
>> libxl__arch_domain_create() ----> domain_vpl011_init() ---> this call
>> fails as the vuart GFN has not been physically mapped yet as shown in
>> the first call flow.
>>
>> However, libxl__build_pv() is called after libxl__build_pre(). It
>> means that the domain_vpl011_init() is called before
>> alloc_magic_pages() is called and hence the initialization fails.
>>
>> For that reason, I had introduced a new function
>> libxl__arch_domain_create_finish() which will be called from
>> libxl__build_post(). I moved the domain_vpl011_init() there. However,
>> Julien pointed out that vuart should be initialized early in
>> libxl__arch_domain_create() function.
>
>
> libxl__arch_domain_create could be a place or even
> libxl__arch_domain_finalise_hw_descriptions.
>
> My point is it looks a bit odd to create the vpl011 UART very late in the
> process as from the code you would expect all the hardware to be setup after
> libxl__arch_domain_finialise_hw_descriptions is called.
>

libxl__arch_domain_finalise_hw_descriptions() is called just before
xc_dom_build_image() and therefore the vuart gfn is still not
allocated. Maybe I can introduce a new arch specific
libxl__arch_domain_init_vpl011() function and call it from inside
libxl__build_dom() after call to xc_dom_build_image() so that the
vuart gfn is allocated.

Regards,
Bhupinder

Patch hide | download patch | download mbox

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 1629f41..77425dd 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -884,6 +884,23 @@  int xc_vcpu_getcontext(xc_interface *xch,
                        uint32_t domid,
                        uint32_t vcpu,
                        vcpu_guest_context_any_t *ctxt);
+/**
+ * This function initializes the vpl011 emulation and returns
+ * the event to be used by the backend for communicating with
+ * the emulation code.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid the domain to get information from
+ * @parm console_domid the domid of the backend console
+ * @parm gfn the guest pfn to be used as the ring buffer
+ * @parm evtchn the event channel to be used for events
+ * @return 0 on success, negative error on failure
+ */
+int xc_dom_vpl011_init(xc_interface *xch,
+                       uint32_t domid,
+                       uint32_t console_domid,
+                       xen_pfn_t gfn,
+                       evtchn_port_t *evtchn);
 
 /**
  * This function returns information about the XSAVE state of a particular
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 00909ad4..a8efd5e 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -343,6 +343,29 @@  int xc_domain_get_guest_width(xc_interface *xch, uint32_t domid,
     return 0;
 }
 
+int xc_dom_vpl011_init(xc_interface *xch,
+                       uint32_t domid,
+                       uint32_t console_domid,
+                       xen_pfn_t gfn,
+                       evtchn_port_t *evtchn)
+{
+    DECLARE_DOMCTL;
+    int rc = 0;
+
+    domctl.cmd = XEN_DOMCTL_vuart_op;
+    domctl.domain = (domid_t)domid;
+    domctl.u.vuart_op.cmd = XEN_DOMCTL_VUART_OP_INIT_VPL011;
+    domctl.u.vuart_op.console_domid = console_domid;
+    domctl.u.vuart_op.gfn = gfn;
+
+    if ( (rc = do_domctl(xch, &domctl)) < 0 )
+        return rc;
+
+    *evtchn = domctl.u.vuart_op.evtchn;
+
+    return rc;
+}
+
 int xc_domain_getinfo(xc_interface *xch,
                       uint32_t first_domid,
                       unsigned int max_doms,
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index 5e1fc60..d1ca9c6 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -32,6 +32,13 @@  _hidden
 int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
                uint32_t domid);
 
+/* arch specific internal domain creation finish function */
+_hidden
+int libxl__arch_domain_create_finish(libxl__gc *gc,
+                                     libxl_domain_build_info *info,
+                                     uint32_t domid,
+                                     libxl__domain_build_state *state);
+
 /* setup arch specific hardware description, i.e. DTB on ARM */
 _hidden
 int libxl__arch_domain_init_hw_description(libxl__gc *gc,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d88..b60dfa9 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -106,6 +106,25 @@  int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
     return 0;
 }
 
+int libxl__arch_domain_create_finish(libxl__gc *gc,
+                                     libxl_domain_build_info *info,
+                                     uint32_t domid,
+                                     libxl__domain_build_state *state)
+{
+    int ret = 0;
+
+    if ( info->arch_arm.vuart &&
+         (ret = xc_dom_vpl011_init(CTX->xch,
+                                   domid,
+                                   state->console_domid,
+                                   xc_get_vuart_gfn(),
+                                   &state->vuart_port)) != 0 ) {
+        LOG(ERROR, "xc_dom_vpl011_init failed\n");
+    }
+
+    return ret;
+}
+
 int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 5d914a5..187c5bd 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -587,7 +587,10 @@  retry_transaction:
             goto retry_transaction;
     xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port);
     free(vm_path);
-    return 0;
+
+    rc = libxl__arch_domain_create_finish(gc, info, domid, state);
+
+    return rc;
 }
 
 static int set_vnuma_info(libxl__gc *gc, uint32_t domid,
@@ -788,6 +791,7 @@  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     if (xc_dom_translated(dom)) {
         state->console_mfn = dom->console_pfn;
         state->store_mfn = dom->xenstore_pfn;
+        state->vuart_gfn = dom->vuart_gfn;
     } else {
         state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
         state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 455f6f0..3544028 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -358,6 +358,14 @@  out:
     return ret;
 }
 
+int libxl__arch_domain_create_finish(libxl__gc *gc,
+                                     libxl_domain_build_info *info,
+                                     uint32_t domid,
+                                     libxl__domain_build_state *state)
+{
+    return 0;
+}
+
 int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 76310ed..9e150ba 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -665,6 +665,8 @@  fail:
 
 void arch_domain_destroy(struct domain *d)
 {
+    domain_vpl011_deinit(d);
+
     /* IOMMU page table is shared with P2M, always call
      * iommu_domain_destroy() before p2m_teardown().
      */
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 971caec..741679b 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -5,13 +5,15 @@ 
  */
 
 #include <xen/types.h>
-#include <xen/lib.h>
+#include <public/domctl.h>
 #include <xen/errno.h>
-#include <xen/sched.h>
+#include <xen/guest_access.h>
 #include <xen/hypercall.h>
 #include <xen/iocap.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/sched.h>
 #include <xsm/xsm.h>
-#include <public/domctl.h>
 
 void arch_get_domain_info(const struct domain *d,
                           struct xen_domctl_getdomaininfo *info)
@@ -119,6 +121,42 @@  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         d->disable_migrate = domctl->u.disable_migrate.disable;
         return 0;
 
+    case XEN_DOMCTL_vuart_op:
+    {
+        int rc;
+        struct xen_domctl_vuart_op *vuart_op = &domctl->u.vuart_op;
+
+        switch(vuart_op->cmd)
+        {
+        case XEN_DOMCTL_VUART_OP_INIT_VPL011:
+
+            if ( !d->creation_finished )
+            {
+                struct vpl011_init_info info;
+
+                info.console_domid = vuart_op->console_domid;
+                info.gfn = _gfn(vuart_op->gfn);
+
+                rc = domain_vpl011_init(d, &info);
+                if ( !rc )
+                {
+                    vuart_op->evtchn = info.evtchn;
+                    rc = __copy_to_guest(u_domctl, domctl, 1);
+                }
+            }
+            else
+            {
+                rc = - EPERM;
+            }
+            break;
+
+        default:
+            rc = -EINVAL;
+            break;
+        }
+
+        return rc;
+    }
     default:
     {
         int rc;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index e6cf211..c6ff458 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -36,6 +36,7 @@ 
 #include "grant_table.h"
 #include "hvm/save.h"
 #include "memory.h"
+#include "event_channel.h"
 
 #define XEN_DOMCTL_INTERFACE_VERSION 0x0000000d
 
@@ -1138,6 +1139,15 @@  struct xen_domctl_psr_cat_op {
     uint32_t target;    /* IN */
     uint64_t data;      /* IN/OUT */
 };
+
+struct xen_domctl_vuart_op {
+#define XEN_DOMCTL_VUART_OP_INIT_VPL011  0
+        uint32_t cmd;           /* XEN_DOMCTL_VUART_OP_* */
+        uint32_t console_domid; /* IN */
+        xen_pfn_t gfn;          /* IN */
+        evtchn_port_t evtchn;   /* OUT */
+};
+
 typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
 
@@ -1218,6 +1228,7 @@  struct xen_domctl {
 #define XEN_DOMCTL_monitor_op                    77
 #define XEN_DOMCTL_psr_cat_op                    78
 #define XEN_DOMCTL_soft_reset                    79
+#define XEN_DOMCTL_vuart_op                      80
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1280,6 +1291,7 @@  struct xen_domctl {
         struct xen_domctl_psr_cmt_op        psr_cmt_op;
         struct xen_domctl_monitor_op        monitor_op;
         struct xen_domctl_psr_cat_op        psr_cat_op;
+        struct xen_domctl_vuart_op          vuart_op;
         uint8_t                             pad[128];
     } u;
 };