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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

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

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