Re: [Xen-devel] [PATCH 06/25 v7] xen/arm: vpl011: Add a new domctl API to initialize vpl011

[Julien Grall <julien.grall@xxxxxxx>]

Hi Bhupinder,

On 07/08/17 09:52, Bhupinder Thakur wrote:
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index 5e1fc60..784ec7f 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -44,6 +44,13 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>                                        libxl_domain_build_info *info,
>                                        struct xc_dom_image *dom);
>
> +/* perform any pending hardware initialization */
> +_hidden
> +int libxl__arch_build_dom_finish(libxl__gc *gc,
> +                                 libxl_domain_build_info *info,
> +                                 struct xc_dom_image *dom,
> +                                 libxl__domain_build_state *state);
> +
>  /* build vNUMA vmemrange with arch specific information */
>  _hidden
>  int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d88..a33d3c9 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -1038,6 +1038,27 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc 
> *gc,
>      return 0;
>  }
>
> +int libxl__arch_build_dom_finish(libxl__gc *gc,
> +                                 libxl_domain_build_info *info,
> +                                 struct xc_dom_image *dom,
> +                                 libxl__domain_build_state *state)
> +{
> +    int ret = 0;
> +
> +    if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
NIT: You could avoid on level of indentation if you do:

if ( info->arch_arm.vuart != LIBXL_VUART_TYPE_SBSA_UART )
  return 0;

....

> +        ret = xc_dom_vuart_init(CTX->xch,
> +                                XEN_DOMCTL_VUART_TYPE_VPL011,
> +                                dom->guest_domid,
> +                                dom->console_domid,
> +                                dom->vuart_gfn,
> +                                &state->vuart_port);
> +        if (ret < 0)
> +            LOG(ERROR, "xc_dom_vuart_init failed\n");
> +    }
> +
> +    return ret;
> +}
> +
>  int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
>                                        uint32_t domid,
>                                        libxl_domain_build_info *info,
[...]

> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index db6838d..c7f650e 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -5,9 +5,11 @@
>   */
>
>  #include <xen/errno.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 <xen/types.h>
>  #include <xsm/xsm.h>
> @@ -119,6 +121,46 @@ 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:
> +
> +            if ( !d->creation_finished )
> +            {
> +                if (vuart_op->type == XEN_DOMCTL_VUART_TYPE_VPL011)
Coding style.

> +                {
> +                        struct vpl011_init_info info;
The indentation is wrong.

> +
> +                        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 = -EINVAL;
I think this one should be -EOPNOTSUPP.

> +            }
> +            else
> +                rc = - EPERM;
Can we please avoid the number of nested if? Maybe by introducing a 
function to handle the domctl.
> +
> +            break;
> +
> +        default:
> +            rc = -EINVAL;
Same here.

> +            break;
> +        }
> +
> +        return rc;
> +    }
>      default:
>      {
>          int rc;
Cheers,

--
Julien Grall

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