| Message ID | 1493395284-18430-4-git-send-email-bhupinder.thakur@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 28 Apr 2017, Bhupinder Thakur wrote: > Vpl011 emulation is enabled for a guest domain in Xen only when it is > enabled through an option in libxl provided by the user through > guest configuration. > > The pl011 enable/disable knob in libxl is introduced in the following > patch: > > xen/arm: vpl011: Add support for vuart in libxl > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > xen/arch/arm/domain.c | 6 ++++++ > xen/common/domctl.c | 3 +++ > xen/include/public/domctl.h | 2 ++ > xen/include/xen/sched.h | 4 ++++ > 4 files changed, 15 insertions(+) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 76310ed..23baa81 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -36,6 +36,7 @@ > #include <asm/platform.h> > #include "vtimer.h" > #include "vuart.h" > +#include <xen/vpl011.h> > > DEFINE_PER_CPU(struct vcpu *, curr_vcpu); > > @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > if ( (rc = domain_vtimer_init(d, config)) != 0 ) > goto fail; > > + if ( domcr_flags & DOMCRF_vuart ) > + if ( (rc = domain_vpl011_init(d, config)) != 0 ) > + goto fail; > update_domain_wallclock_time(d); > > /* > @@ -665,6 +669,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/common/domctl.c b/xen/common/domctl.c > index 951a5dc..902dd71 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -501,6 +501,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > | XEN_DOMCTL_CDF_hap > | XEN_DOMCTL_CDF_s3_integrity > | XEN_DOMCTL_CDF_oos_off > + | XEN_DOMCTL_VUART_enable > | XEN_DOMCTL_CDF_xs_domain)) ) Please place XEN_DOMCTL_VUART_enable at last item. Aside from this: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > break; > > @@ -539,6 +540,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > domcr_flags |= DOMCRF_oos_off; > if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain ) > domcr_flags |= DOMCRF_xs_domain; > + if ( op->u.createdomain.flags & XEN_DOMCTL_VUART_enable ) > + domcr_flags |= DOMCRF_vuart; > > d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref, > &op->u.createdomain.config); > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 8bee0c3..c307013 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -63,6 +63,8 @@ struct xen_domctl_createdomain { > /* Is this a xenstore domain? */ > #define _XEN_DOMCTL_CDF_xs_domain 4 > #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) > +#define _XEN_DOMCTL_VUART_enable 6 > +#define XEN_DOMCTL_VUART_enable (1U<<_XEN_DOMCTL_VUART_enable) > uint32_t flags; > struct xen_arch_domainconfig config; > }; > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index 1127ca9..ee7dc7a 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 5 > #define DOMCRF_xs_domain (1U<<_DOMCRF_xs_domain) > > + /* DOMCRF_vuart: enable virtual uart emulation. Used for Aarch64. */ > +#define _DOMCRF_vuart 7 > +#define DOMCRF_vuart (1U<<_DOMCRF_vuart) > + > /* > * 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 >
>>> On 28.04.17 at 18:01, <bhupinder.thakur@linaro.org> wrote: > @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, > if ( (rc = domain_vtimer_init(d, config)) != 0 ) > goto fail; > > + if ( domcr_flags & DOMCRF_vuart ) > + if ( (rc = domain_vpl011_init(d, config)) != 0 ) > + goto fail; > update_domain_wallclock_time(d); This is ARM code, so my opinion may not mean much, but why two if()s instead of one, and why no blank line after your addition? > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -63,6 +63,8 @@ struct xen_domctl_createdomain { > /* Is this a xenstore domain? */ > #define _XEN_DOMCTL_CDF_xs_domain 4 > #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) > +#define _XEN_DOMCTL_VUART_enable 6 > +#define XEN_DOMCTL_VUART_enable (1U<<_XEN_DOMCTL_VUART_enable) As expressed before, I object to this addition, as doing things this way does not scale. I don't think I've seen any proper reply to my previous objection, and the patch description here certainly does not justify why an exception should be made in this case. As a side note, why bit 6 instead of the first available one (5)? Jan
Hi Jan, >> @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, >> if ( (rc = domain_vtimer_init(d, config)) != 0 ) >> goto fail; >> >> + if ( domcr_flags & DOMCRF_vuart ) >> + if ( (rc = domain_vpl011_init(d, config)) != 0 ) >> + goto fail; >> update_domain_wallclock_time(d); > I am planning to remove the usage of domain creation flag to check whether vuart is enabled/disabled. Please see my next comment. With that change, domain_vpl011_init() will be called always. The domain_vpl011_init() will check whether vuart is enabled or disabled in the config structure passed. If vuart is enabled then it will go ahead with vpl011 initialization else it will return without initializing vpl011. > This is ARM code, so my opinion may not mean much, but why two > if()s instead of one, and why no blank line after your addition? > >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -63,6 +63,8 @@ struct xen_domctl_createdomain { >> /* Is this a xenstore domain? */ >> #define _XEN_DOMCTL_CDF_xs_domain 4 >> #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) >> +#define _XEN_DOMCTL_VUART_enable 6 >> +#define XEN_DOMCTL_VUART_enable (1U<<_XEN_DOMCTL_VUART_enable) > > As expressed before, I object to this addition, as doing things this > way does not scale. I don't think I've seen any proper reply to my > previous objection, and the patch description here certainly does > not justify why an exception should be made in this case. As a > side note, why bit 6 instead of the first available one (5)? > Sorry I missed to address this review comment. I will pass the vuart enable/disable information through the configuration structure xc_domain_configuration_t, which is passed transparently to domain_vpl011_init() by the tool stack. With that change, we need not use domain creation flags. Regards, Bhupinder
Hi Bhupinder, On 02/05/17 16:20, Bhupinder Thakur wrote: > Hi Jan, > >>> @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, >>> if ( (rc = domain_vtimer_init(d, config)) != 0 ) >>> goto fail; >>> >>> + if ( domcr_flags & DOMCRF_vuart ) >>> + if ( (rc = domain_vpl011_init(d, config)) != 0 ) >>> + goto fail; >>> update_domain_wallclock_time(d); >> > I am planning to remove the usage of domain creation flag to check > whether vuart is enabled/disabled. Please see my next comment. With > that change, domain_vpl011_init() will be called always. The > domain_vpl011_init() will check whether vuart is enabled or disabled > in the config structure passed. If vuart is enabled then it will go > ahead with vpl011 initialization else it will return without > initializing vpl011. Please don't do that. The arch code decides whether domain_vpl011_init not the invert. I would much prefer if you do: if ( enable_vuart && ((rc = domain_vpl011_init(d, config) != 0) ) goto fail; or rc = (enable_vuart) ? domain_vpl011_init(d, config) : 0; if ( rc != 0 ) goto fail; Cheers,
On 02/05/17 16:23, Julien Grall wrote: > Hi Bhupinder, > > On 02/05/17 16:20, Bhupinder Thakur wrote: >> Hi Jan, >> >>>> @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, >>>> unsigned int domcr_flags, >>>> if ( (rc = domain_vtimer_init(d, config)) != 0 ) >>>> goto fail; >>>> >>>> + if ( domcr_flags & DOMCRF_vuart ) >>>> + if ( (rc = domain_vpl011_init(d, config)) != 0 ) >>>> + goto fail; >>>> update_domain_wallclock_time(d); >>> >> I am planning to remove the usage of domain creation flag to check >> whether vuart is enabled/disabled. Please see my next comment. With >> that change, domain_vpl011_init() will be called always. The >> domain_vpl011_init() will check whether vuart is enabled or disabled >> in the config structure passed. If vuart is enabled then it will go >> ahead with vpl011 initialization else it will return without >> initializing vpl011. > > Please don't do that. The arch code decides whether domain_vpl011_init > not the invert. I was wondering whether it would be better to defer the PL011 creation to a domctl. This could be called after the domain is created with all the information required (MMIO region, Console PFN...). This would also make the migration support more trivial as the we will not need to know in advance whether a UART is been used. Any opinions? Cheers,
>>> On 03.05.17 at 12:22, <julien.grall@arm.com> wrote: > > On 02/05/17 16:23, Julien Grall wrote: >> Hi Bhupinder, >> >> On 02/05/17 16:20, Bhupinder Thakur wrote: >>> Hi Jan, >>> >>>>> @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, >>>>> unsigned int domcr_flags, >>>>> if ( (rc = domain_vtimer_init(d, config)) != 0 ) >>>>> goto fail; >>>>> >>>>> + if ( domcr_flags & DOMCRF_vuart ) >>>>> + if ( (rc = domain_vpl011_init(d, config)) != 0 ) >>>>> + goto fail; >>>>> update_domain_wallclock_time(d); >>>> >>> I am planning to remove the usage of domain creation flag to check >>> whether vuart is enabled/disabled. Please see my next comment. With >>> that change, domain_vpl011_init() will be called always. The >>> domain_vpl011_init() will check whether vuart is enabled or disabled >>> in the config structure passed. If vuart is enabled then it will go >>> ahead with vpl011 initialization else it will return without >>> initializing vpl011. >> >> Please don't do that. The arch code decides whether domain_vpl011_init >> not the invert. > > I was wondering whether it would be better to defer the PL011 creation > to a domctl. This could be called after the domain is created with all > the information required (MMIO region, Console PFN...). > > This would also make the migration support more trivial as the we will > not need to know in advance whether a UART is been used. > > Any opinions? Wasn't that the suggestion given during v1 review? Jan
Hi Julien, >>> Hi Jan, >>> >>>>> @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, >>>>> unsigned int domcr_flags, >>>>> if ( (rc = domain_vtimer_init(d, config)) != 0 ) >>>>> goto fail; >>>>> >>>>> + if ( domcr_flags & DOMCRF_vuart ) >>>>> + if ( (rc = domain_vpl011_init(d, config)) != 0 ) >>>>> + goto fail; >>>>> update_domain_wallclock_time(d); >>>> >>>> >>> I am planning to remove the usage of domain creation flag to check >>> whether vuart is enabled/disabled. Please see my next comment. With >>> that change, domain_vpl011_init() will be called always. The >>> domain_vpl011_init() will check whether vuart is enabled or disabled >>> in the config structure passed. If vuart is enabled then it will go >>> ahead with vpl011 initialization else it will return without >>> initializing vpl011. >> >> >> Please don't do that. The arch code decides whether domain_vpl011_init >> not the invert. > > > I was wondering whether it would be better to defer the PL011 creation to a > domctl. This could be called after the domain is created with all the > information required (MMIO region, Console PFN...). > > This would also make the migration support more trivial as the we will not > need to know in advance whether a UART is been used. > > Any opinions? Would there be race condition where the guest tries to access the pl011 mmio region (as the domain has been created) but pl011 is not initialized yet as domctl is not called? What could be an appropriate place to call this domctl? It should be before xenstore is populated with vuart ring-ref/port information. Regards, Bhupinder
On 05/05/17 08:10, Bhupinder Thakur wrote: > Hi Julien, Hi Bhupinder, >>>> Hi Jan, >>>> >>>>>> @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, >>>>>> unsigned int domcr_flags, >>>>>> if ( (rc = domain_vtimer_init(d, config)) != 0 ) >>>>>> goto fail; >>>>>> >>>>>> + if ( domcr_flags & DOMCRF_vuart ) >>>>>> + if ( (rc = domain_vpl011_init(d, config)) != 0 ) >>>>>> + goto fail; >>>>>> update_domain_wallclock_time(d); >>>>> >>>>> >>>> I am planning to remove the usage of domain creation flag to check >>>> whether vuart is enabled/disabled. Please see my next comment. With >>>> that change, domain_vpl011_init() will be called always. The >>>> domain_vpl011_init() will check whether vuart is enabled or disabled >>>> in the config structure passed. If vuart is enabled then it will go >>>> ahead with vpl011 initialization else it will return without >>>> initializing vpl011. >>> >>> >>> Please don't do that. The arch code decides whether domain_vpl011_init >>> not the invert. >> >> >> I was wondering whether it would be better to defer the PL011 creation to a >> domctl. This could be called after the domain is created with all the >> information required (MMIO region, Console PFN...). >> >> This would also make the migration support more trivial as the we will not >> need to know in advance whether a UART is been used. >> >> Any opinions? > > Would there be race condition where the guest tries to access the > pl011 mmio region (as the domain has been created) but pl011 is not > initialized yet as domctl is not called? What could be an appropriate > place to call this domctl? It should be before xenstore is populated > with vuart ring-ref/port information. There are no race condition. The domain will only be started when everything has been created by calling XEN_DOMCTL_unpausedomain. The DOMCTL createdomain only initialize the basic structure for the domain, after the hypercall the domain is not in state to be run because, for instance, the vCPUs were not allocated (see XEN_DOMCTL_max_vcpus) and the guest RAM were not populated. I am not very familiar with the libxl code, but I think libxl__arch_domain_create should be a good candidate. I will let Ian and Wei confirm that. Cheers,
Hi, >>> I was wondering whether it would be better to defer the PL011 creation to >>> a >>> domctl. This could be called after the domain is created with all the >>> information required (MMIO region, Console PFN...). >>> >>> This would also make the migration support more trivial as the we will >>> not >>> need to know in advance whether a UART is been used. >>> >>> Any opinions? >> >> >> Would there be race condition where the guest tries to access the >> pl011 mmio region (as the domain has been created) but pl011 is not >> initialized yet as domctl is not called? What could be an appropriate >> place to call this domctl? It should be before xenstore is populated >> with vuart ring-ref/port information. > > > There are no race condition. The domain will only be started when everything > has been created by calling XEN_DOMCTL_unpausedomain. > > The DOMCTL createdomain only initialize the basic structure for the domain, > after the hypercall the domain is not in state to be run because, for > instance, the vCPUs were not allocated (see XEN_DOMCTL_max_vcpus) and the > guest RAM were not populated. > > I am not very familiar with the libxl code, but I think > libxl__arch_domain_create should be a good candidate. I will let Ian and Wei > confirm that. I have defined a new DOMCTL xc_dom_vpl011_init() which collects all the required information such as ring buffer GFN, console domid and returns the event channel. This function is being called from libxl__build_dom(), which is called from inside libxl__build_pv() at the last. This new function does away with the earlier set of domctls defined for get/set of gfn/eventchannels. It also does away with the tinkering of domain creation flags while creating a domain. Regards, Bhupinder
On Fri, May 05, 2017 at 02:43:40PM +0100, Julien Grall wrote: > On 05/05/17 08:10, Bhupinder Thakur wrote: > > Hi Julien, > > Hi Bhupinder, > > > > > > Hi Jan, > > > > > > > > > > > > @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, > > > > > > > unsigned int domcr_flags, > > > > > > > if ( (rc = domain_vtimer_init(d, config)) != 0 ) > > > > > > > goto fail; > > > > > > > > > > > > > > + if ( domcr_flags & DOMCRF_vuart ) > > > > > > > + if ( (rc = domain_vpl011_init(d, config)) != 0 ) > > > > > > > + goto fail; > > > > > > > update_domain_wallclock_time(d); > > > > > > > > > > > > > > > > > I am planning to remove the usage of domain creation flag to check > > > > > whether vuart is enabled/disabled. Please see my next comment. With > > > > > that change, domain_vpl011_init() will be called always. The > > > > > domain_vpl011_init() will check whether vuart is enabled or disabled > > > > > in the config structure passed. If vuart is enabled then it will go > > > > > ahead with vpl011 initialization else it will return without > > > > > initializing vpl011. > > > > > > > > > > > > Please don't do that. The arch code decides whether domain_vpl011_init > > > > not the invert. > > > > > > > > > I was wondering whether it would be better to defer the PL011 creation to a > > > domctl. This could be called after the domain is created with all the > > > information required (MMIO region, Console PFN...). > > > > > > This would also make the migration support more trivial as the we will not > > > need to know in advance whether a UART is been used. > > > > > > Any opinions? > > > > Would there be race condition where the guest tries to access the > > pl011 mmio region (as the domain has been created) but pl011 is not > > initialized yet as domctl is not called? What could be an appropriate > > place to call this domctl? It should be before xenstore is populated > > with vuart ring-ref/port information. > > There are no race condition. The domain will only be started when everything > has been created by calling XEN_DOMCTL_unpausedomain. > > The DOMCTL createdomain only initialize the basic structure for the domain, > after the hypercall the domain is not in state to be run because, for > instance, the vCPUs were not allocated (see XEN_DOMCTL_max_vcpus) and the > guest RAM were not populated. > > I am not very familiar with the libxl code, but I think > libxl__arch_domain_create should be a good candidate. I will let Ian and Wei > confirm that. > That sounds reasonable.
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 76310ed..23baa81 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -36,6 +36,7 @@ #include <asm/platform.h> #include "vtimer.h" #include "vuart.h" +#include <xen/vpl011.h> DEFINE_PER_CPU(struct vcpu *, curr_vcpu); @@ -631,6 +632,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, if ( (rc = domain_vtimer_init(d, config)) != 0 ) goto fail; + if ( domcr_flags & DOMCRF_vuart ) + if ( (rc = domain_vpl011_init(d, config)) != 0 ) + goto fail; update_domain_wallclock_time(d); /* @@ -665,6 +669,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/common/domctl.c b/xen/common/domctl.c index 951a5dc..902dd71 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -501,6 +501,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) | XEN_DOMCTL_CDF_hap | XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off + | XEN_DOMCTL_VUART_enable | XEN_DOMCTL_CDF_xs_domain)) ) break; @@ -539,6 +540,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) domcr_flags |= DOMCRF_oos_off; if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_xs_domain ) domcr_flags |= DOMCRF_xs_domain; + if ( op->u.createdomain.flags & XEN_DOMCTL_VUART_enable ) + domcr_flags |= DOMCRF_vuart; d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref, &op->u.createdomain.config); diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 8bee0c3..c307013 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -63,6 +63,8 @@ struct xen_domctl_createdomain { /* Is this a xenstore domain? */ #define _XEN_DOMCTL_CDF_xs_domain 4 #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) +#define _XEN_DOMCTL_VUART_enable 6 +#define XEN_DOMCTL_VUART_enable (1U<<_XEN_DOMCTL_VUART_enable) uint32_t flags; struct xen_arch_domainconfig config; }; diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 1127ca9..ee7dc7a 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 5 #define DOMCRF_xs_domain (1U<<_DOMCRF_xs_domain) + /* DOMCRF_vuart: enable virtual uart emulation. Used for Aarch64. */ +#define _DOMCRF_vuart 7 +#define DOMCRF_vuart (1U<<_DOMCRF_vuart) + /* * rcu_lock_domain_by_id() is more efficient than get_domain_by_id(). * This is the preferred function if the returned domain reference
Vpl011 emulation is enabled for a guest domain in Xen only when it is enabled through an option in libxl provided by the user through guest configuration. The pl011 enable/disable knob in libxl is introduced in the following patch: xen/arm: vpl011: Add support for vuart in libxl Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- xen/arch/arm/domain.c | 6 ++++++ xen/common/domctl.c | 3 +++ xen/include/public/domctl.h | 2 ++ xen/include/xen/sched.h | 4 ++++ 4 files changed, 15 insertions(+)