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

[Julien Grall <julien.grall@xxxxxxx>]

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,

--
Julien Grall

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