[Xen-devel,04/11] xen/arm: vpl011: Enable vpl011 emulation for a domain in Xen

Message ID 1487676368-22356-5-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • pl011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur Feb. 21, 2017, 11:26 a.m.
Based on one of the domain creation flags, the vpl011 emulation will be enabled for
the domain.

This flag is currently set always but finally it needs to be controlled by the user
through some configuration option.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 xen/arch/arm/domain.c   | 8 ++++++++
 xen/common/domctl.c     | 2 ++
 xen/include/xen/sched.h | 4 ++++
 3 files changed, 14 insertions(+)

Comments

Konrad Rzeszutek Wilk March 3, 2017, 9:47 p.m. | #1
On Tue, Feb 21, 2017 at 04:56:01PM +0530, Bhupinder Thakur wrote:
> Based on one of the domain creation flags, the vpl011 emulation will be enabled for
> the domain.
> 
> This flag is currently set always but finally it needs to be controlled by the user
> through some configuration option.

"some"? Could you be more specific please.

Perhaps mention the title of the patch that adds this configuration
knob.

> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/arch/arm/domain.c   | 8 ++++++++
>  xen/common/domctl.c     | 2 ++
>  xen/include/xen/sched.h | 4 ++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7e43691..6fee541 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -36,6 +36,9 @@
>  #include <asm/platform.h>
>  #include "vtimer.h"
>  #include "vuart.h"
> +#ifdef CONFIG_VPL011_CONSOLE
> +#include "vpl011.h"
> +#endif
>  
>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>  
> @@ -626,6 +629,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>      if ( (rc = domain_vtimer_init(d, config)) != 0 )
>          goto fail;
>  
> +#ifdef CONFIG_VPL011_CONSOLE
> +    if ( domcr_flags & DOMCRF_vpl011_console )
> +        if ( (rc = domain_vpl011_init(d)) != 0 )
> +            goto fail;
> +#endif
>      update_domain_wallclock_time(d);
>  
>      /*
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 12cf4a9..fafc506 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -551,6 +551,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
>              domcr_flags |= DOMCRF_xs_domain;
>  
> +        domcr_flags |= DOMCRF_vpl011_console;
> +
>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
>                            &op->u.createdomain.config);
>          if ( IS_ERR(d) )
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 063efe6..9e03a60 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -555,6 +555,10 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>  #define _DOMCRF_xs_domain       6
>  #define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
>  
> + /* DOMCRF_vpl011_console: create domain with pl011 emulation enabled */

Perhaps:

/* Enable pl011 emulation. Preferred on ARM but not so much on x86. */ ?
> +#define _DOMCRF_vpl011_console  7
> +#define DOMCRF_vpl011_console   (1U<<_DOMCRF_vpl011_console)
> +
>  /*
>   * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>   * This is the preferred function if the returned domain reference
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
Julien Grall March 5, 2017, 12:46 p.m. | #2
(CC the REST maintainer)

Hi Bhupinder,

On 02/21/2017 11:26 AM, Bhupinder Thakur wrote:
> Based on one of the domain creation flags, the vpl011 emulation will be enabled for
> the domain.
>
> This flag is currently set always but finally it needs to be controlled by the user
> through some configuration option.

We really don't want the pl011 emulation by default. You should provide 
a libxl option to enable pl011 emulation. Also without the next patch, 
this will break guest boot as SPIs are not correctly initialized.

You likely want to re-order the two.

>
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  xen/arch/arm/domain.c   | 8 ++++++++
>  xen/common/domctl.c     | 2 ++
>  xen/include/xen/sched.h | 4 ++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7e43691..6fee541 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -36,6 +36,9 @@
>  #include <asm/platform.h>
>  #include "vtimer.h"
>  #include "vuart.h"
> +#ifdef CONFIG_VPL011_CONSOLE
> +#include "vpl011.h"
> +#endif
>
>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>
> @@ -626,6 +629,11 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>      if ( (rc = domain_vtimer_init(d, config)) != 0 )
>          goto fail;
>
> +#ifdef CONFIG_VPL011_CONSOLE
> +    if ( domcr_flags & DOMCRF_vpl011_console )
> +        if ( (rc = domain_vpl011_init(d)) != 0 )
> +            goto fail;
> +#endif

You likely want to return an error if the toolstack request the pl011 
emulation and it is not present in Xen.

So I would add a stub for domain_vpl011_init to return -ENOSYS when the 
pl011 emulation does not exist in Xen.

>      update_domain_wallclock_time(d);
>
>      /*
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 12cf4a9..fafc506 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -551,6 +551,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
>              domcr_flags |= DOMCRF_xs_domain;
>
> +        domcr_flags |= DOMCRF_vpl011_console;
> +

Please don't do that. See my suggestion above.


>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
>                            &op->u.createdomain.config);
>          if ( IS_ERR(d) )
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 063efe6..9e03a60 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -555,6 +555,10 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>  #define _DOMCRF_xs_domain       6
>  #define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
>
> + /* DOMCRF_vpl011_console: create domain with pl011 emulation enabled */
> +#define _DOMCRF_vpl011_console  7
> +#define DOMCRF_vpl011_console   (1U<<_DOMCRF_vpl011_console)
> +
>  /*
>   * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
>   * This is the preferred function if the returned domain reference
>

Cheers,
Jan Beulich March 6, 2017, 8:27 a.m. | #3
>>> On 05.03.17 at 13:46, <julien.grall@arm.com> wrote:
> On 02/21/2017 11:26 AM, Bhupinder Thakur wrote:
>> Based on one of the domain creation flags, the vpl011 emulation will be enabled for
>> the domain.
>>
>> This flag is currently set always but finally it needs to be controlled by the user
>> through some configuration option.
> 
> We really don't want the pl011 emulation by default. You should provide 
> a libxl option to enable pl011 emulation.

Furthermore I don't think emulation of individual devices should be
set up via DOMCRF_*, these should be used only for more general
aspects of domain creation.

Jan

Patch hide | download patch | download mbox

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7e43691..6fee541 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -36,6 +36,9 @@ 
 #include <asm/platform.h>
 #include "vtimer.h"
 #include "vuart.h"
+#ifdef CONFIG_VPL011_CONSOLE
+#include "vpl011.h"
+#endif
 
 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
 
@@ -626,6 +629,11 @@  int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     if ( (rc = domain_vtimer_init(d, config)) != 0 )
         goto fail;
 
+#ifdef CONFIG_VPL011_CONSOLE
+    if ( domcr_flags & DOMCRF_vpl011_console )
+        if ( (rc = domain_vpl011_init(d)) != 0 )
+            goto fail;
+#endif
     update_domain_wallclock_time(d);
 
     /*
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 12cf4a9..fafc506 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -551,6 +551,8 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain )
             domcr_flags |= DOMCRF_xs_domain;
 
+        domcr_flags |= DOMCRF_vpl011_console;
+
         d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
                           &op->u.createdomain.config);
         if ( IS_ERR(d) )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 063efe6..9e03a60 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -555,6 +555,10 @@  struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
 #define _DOMCRF_xs_domain       6
 #define DOMCRF_xs_domain        (1U<<_DOMCRF_xs_domain)
 
+ /* DOMCRF_vpl011_console: create domain with pl011 emulation enabled */
+#define _DOMCRF_vpl011_console  7
+#define DOMCRF_vpl011_console   (1U<<_DOMCRF_vpl011_console)
+
 /*
  * rcu_lock_domain_by_id() is more efficient than get_domain_by_id().
  * This is the preferred function if the returned domain reference