| Message ID | 1406818852-31856-9-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 31 Jul 2014, Julien Grall wrote: > The virtual GIC may differ between each guest (number of SPIs, emulate GIC > version...). Those informations may not be know when the domain is created > (for instance in case of migration). Therefore, move the VGIC initialization > in a separate function. > > Introduce a new DOMCTL for ARM to configure the domain. This has to be called > before setting the maximum number of VCPUs for the guest. > > For the moment, only configure the number SPIs. New informations could be > added later. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Jan Beulich <jbeulich@suse.com> > > --- > Changes in v2: > - Patch added > --- > tools/libxc/xc_domain.c | 12 ++++++++++++ > tools/libxc/xenctrl.h | 4 ++++ > tools/libxl/libxl_arch.h | 3 +++ > tools/libxl/libxl_arm.c | 19 +++++++++++++++++++ > tools/libxl/libxl_dom.c | 4 ++++ > tools/libxl/libxl_x86.c | 7 +++++++ > xen/arch/arm/domain.c | 28 ++++++++++++++++++++++------ > xen/arch/arm/domctl.c | 11 +++++++++++ > xen/arch/arm/setup.c | 10 ++++++++-- > xen/arch/arm/vgic.c | 10 +++++----- > xen/include/asm-arm/domain.h | 6 ++++++ > xen/include/asm-arm/vgic.h | 4 +++- > xen/include/public/domctl.h | 14 ++++++++++++++ > xen/xsm/flask/hooks.c | 3 +++ > xen/xsm/flask/policy/access_vectors | 2 ++ > 15 files changed, 123 insertions(+), 14 deletions(-) > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > index 27fe3b6..1348905 100644 > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -48,6 +48,18 @@ int xc_domain_create(xc_interface *xch, > return 0; > } > > +#if defined(__arm__) || defined(__arch64__) > +int xc_domain_configure(xc_interface *xch, uint32_t domid, > + uint32_t nr_spis) Given that we'll likely add new fields to xen_domctl_configuredomain, I think it is best if we pass a struct domain_configure to xc_domain_configure instead of nr_spis. The struct we pass to xc_domain_configure doesn't have to be identical to struct xen_domctl_configuredomain, but at the moment it would be. > +{ > + DECLARE_DOMCTL; > + domctl.cmd = XEN_DOMCTL_configure_domain; > + domctl.domain = (domid_t)domid; > + domctl.u.configuredomain.nr_spis = nr_spis; > + return do_domctl(xch, &domctl); > +} > +#endif > + > int xc_domain_cacheflush(xc_interface *xch, uint32_t domid, > xen_pfn_t start_pfn, xen_pfn_t nr_pfns) > { > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index 5beb846..cdda4e5 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -482,6 +482,10 @@ int xc_domain_create(xc_interface *xch, > uint32_t flags, > uint32_t *pdomid); > > +#if defined(__arm__) || defined(__aarch64__) > +int xc_domain_configure(xc_interface *xch, uint32_t domid, > + uint32_t nr_spis); > +#endif > > /* Functions to produce a dump of a given domain > * xc_domain_dumpcore - produces a dump to a specified file > diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h > index d3bc136..454f8db 100644 > --- a/tools/libxl/libxl_arch.h > +++ b/tools/libxl/libxl_arch.h > @@ -16,6 +16,9 @@ > #define LIBXL_ARCH_H > > /* arch specific internal domain creation function */ > +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config *d_config, > + libxl__domain_build_state *state, > + uint32_t domid); > int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, > uint32_t domid); > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index e19e2f4..b0491c3 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -23,6 +23,25 @@ > #define DT_IRQ_TYPE_LEVEL_HIGH 0x00000004 > #define DT_IRQ_TYPE_LEVEL_LOW 0x00000008 > > +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config *d_config, > + libxl__domain_build_state *state, > + uint32_t domid) > +{ > + uint32_t nr_spis = 0; > + > + nr_spis += d_config->b_info.num_irqs; > + > + LOG(DEBUG, "Allocate %u SPIs\n", nr_spis); > + > + if (xc_domain_configure(CTX->xch, domid, nr_spis) != 0) { > + LOG(ERROR, "Couldn't configure the domain"); > + return ERROR_FAIL; > + } > + > + return 0; > +} > + > + > int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, > uint32_t domid) > { > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index c944804..a0e4e34 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -235,6 +235,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid, > char *xs_domid, *con_domid; > int rc; > > + rc = libxl__arch_domain_create_pre(gc, d_config, state, domid); > + if (rc != 0) > + return rc; > + > if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) { > LOG(ERROR, "Couldn't set max vcpu count"); > return ERROR_FAIL; > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c > index 7589060..0abc1aa 100644 > --- a/tools/libxl/libxl_x86.c > +++ b/tools/libxl/libxl_x86.c > @@ -244,6 +244,13 @@ static int libxl__e820_alloc(libxl__gc *gc, uint32_t domid, > return 0; > } > > +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config *d_config, > + libxl__domain_build_state *state, > + uint32_t domid) > +{ > + return 0; > +} > + > int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, > uint32_t domid) > { > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index aa4a8d7..fc97f8c 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -472,6 +472,13 @@ int vcpu_initialise(struct vcpu *v) > if ( is_idle_vcpu(v) ) > return rc; > > + /* We can't initialize the VCPU if the VGIC has not been correctly > + * initialized. > + */ > + rc = -ENXIO; > + if ( !domain_vgic_is_initialized(v->domain) ) > + goto fail; > + > v->arch.sctlr = SCTLR_GUEST_INIT; > > /* > @@ -534,12 +541,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) > if ( (rc = p2m_alloc_table(d)) != 0 ) > goto fail; > > - if ( (rc = gicv_setup(d)) != 0 ) > - goto fail; > - > - if ( (rc = domain_vgic_init(d)) != 0 ) > - goto fail; > - > if ( (rc = domain_vtimer_init(d)) != 0 ) > goto fail; > > @@ -580,6 +581,21 @@ void arch_domain_destroy(struct domain *d) > free_xenheap_page(d->shared_info); > } > > +int domain_configure_vgic(struct domain *d, unsigned int nr_spis) > +{ > + int rc; > + > + if ( (rc = gicv_setup(d)) != 0 ) > + return rc; > + > + if ( (rc = domain_vgic_init(d, nr_spis)) != 0 ) > + return rc; > + > + d->arch.vgic.initialized = 1; > + > + return 0; > +} > + > static int is_guest_pv32_psr(uint32_t psr) > { > switch (psr & PSR_MODE_MASK) > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c > index 45974e7..bab92b2 100644 > --- a/xen/arch/arm/domctl.c > +++ b/xen/arch/arm/domctl.c > @@ -30,6 +30,17 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d, > > return p2m_cache_flush(d, s, e); > } > + case XEN_DOMCTL_configure_domain: > + { > + if ( domain_vgic_is_initialized(d) ) > + return -EBUSY; Given that XEN_DOMCTL_configure_domain should be called exactly once at domain creation, instead of introducing domain_vgic_is_initialized, I would make sure that XEN_DOMCTL_configure_domain hasn't been called for this domain before. In other words, I would make this check more generic, rather than vgic specific. > + /* Sanity check on the number of SPIs */ > + if ( domctl->u.configuredomain.nr_spis > (gic_number_lines() - 32) ) > + return -EINVAL; > + > + return domain_configure_vgic(d, domctl->u.configuredomain.nr_spis); > + } > > default: > return subarch_do_domctl(domctl, d, u_domctl); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 446b4dc..2be7dd1 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -840,8 +840,14 @@ void __init start_xen(unsigned long boot_phys_offset, > > /* Create initial domain 0. */ > dom0 = domain_create(0, 0, 0); > - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) > - panic("Error creating domain 0"); > + if ( IS_ERR(dom0) ) > + panic("Error creating domain 0"); > + > + if ( domain_configure_vgic(dom0, gic_number_lines() - 32) ) > + panic("Error configure the vgic for domain 0"); > + > + if ( alloc_dom0_vcpu0(dom0) == NULL ) > + panic("Error create vcpu0 for domain 0"); > > dom0->is_privileged = 1; > dom0->target = NULL; > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 17cde7a..a037ecc 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -60,16 +60,16 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned virq) > p->irq = virq; > } > > -int domain_vgic_init(struct domain *d) > +int domain_vgic_init(struct domain *d, unsigned int nr_spis) > { > int i; > > d->arch.vgic.ctlr = 0; > > - if ( is_hardware_domain(d) ) > - d->arch.vgic.nr_spis = gic_number_lines() - 32; > - else > - d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */ > + /* The number of SPIs as to be aligned to 32 see > + * GICD_TYPER.ITLinesNumber definition > + */ > + d->arch.vgic.nr_spis = ROUNDUP(nr_spis, 32); > > switch ( gic_hw_version() ) > { > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 5719fe5..44727b2 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -76,6 +76,10 @@ struct arch_domain > } virt_timer_base; > > struct { > + /* The VGIC initialization is defered to let the toolstack > + * configure the emulated GIC (such as number of SPIs, version...) > + */ > + bool_t initialized; > /* GIC HW version specific vGIC driver handler */ > const struct vgic_ops *handler; > /* > @@ -241,6 +245,8 @@ struct arch_vcpu > void vcpu_show_execution_state(struct vcpu *); > void vcpu_show_registers(const struct vcpu *); > > +int domain_configure_vgic(struct domain *d, unsigned int spis_number); > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 5ddc681..84ae441 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -148,6 +148,8 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset) > *reg |= var; > } > > +#define domain_vgic_is_initialized(d) ((d)->arch.vgic.initialized) > + > enum gic_sgi_mode; > > /* > @@ -158,7 +160,7 @@ enum gic_sgi_mode; > > #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32) > > -extern int domain_vgic_init(struct domain *d); > +extern int domain_vgic_init(struct domain *d, unsigned int nr_spis); > extern void domain_vgic_free(struct domain *d); > extern int vcpu_vgic_init(struct vcpu *v); > extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq); > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 5b11bbf..b5f2ed7 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -67,6 +67,16 @@ struct xen_domctl_createdomain { > typedef struct xen_domctl_createdomain xen_domctl_createdomain_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_createdomain_t); > > +#if defined(__arm__) || defined(__aarch64__) > +/* XEN_DOMCTL_configure_domain */ > +struct xen_domctl_configuredomain { > + /* IN parameters */ > + uint32_t nr_spis; > +}; > +typedef struct xen_domctl_configuredomain xen_domctl_configuredomain_t; > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_configuredomain_t); > +#endif > + > /* XEN_DOMCTL_getdomaininfo */ > struct xen_domctl_getdomaininfo { > /* OUT variables. */ > @@ -1008,6 +1018,7 @@ struct xen_domctl { > #define XEN_DOMCTL_cacheflush 71 > #define XEN_DOMCTL_get_vcpu_msrs 72 > #define XEN_DOMCTL_set_vcpu_msrs 73 > +#define XEN_DOMCTL_configure_domain 74 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -1016,6 +1027,9 @@ struct xen_domctl { > domid_t domain; > union { > struct xen_domctl_createdomain createdomain; > +#if defined(__arm__) || defined(__aarch64__) > + struct xen_domctl_configuredomain configuredomain; > +#endif > struct xen_domctl_getdomaininfo getdomaininfo; > struct xen_domctl_getmemlist getmemlist; > struct xen_domctl_getpageframeinfo getpageframeinfo; > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index f2f59ea..4759461 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -715,6 +715,9 @@ static int flask_domctl(struct domain *d, int cmd) > case XEN_DOMCTL_cacheflush: > return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CACHEFLUSH); > > + case XEN_DOMCTL_configure_domain: > + return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CONFIGURE_DOMAIN); > + > default: > printk("flask_domctl: Unknown op %d\n", cmd); > return -EPERM; > diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors > index 32371a9..33eec66 100644 > --- a/xen/xsm/flask/policy/access_vectors > +++ b/xen/xsm/flask/policy/access_vectors > @@ -200,6 +200,8 @@ class domain2 > cacheflush > # Creation of the hardware domain when it is not dom0 > create_hardware_domain > +# XEN_DOMCTL_configure_domain > + configure_domain > } > > # Similar to class domain, but primarily contains domctls related to HVM domains > -- > 1.7.10.4 >
On 07/31/2014 11:00 AM, Julien Grall wrote: > The virtual GIC may differ between each guest (number of SPIs, emulate GIC > version...). Those informations may not be know when the domain is created > (for instance in case of migration). Therefore, move the VGIC initialization > in a separate function. > > Introduce a new DOMCTL for ARM to configure the domain. This has to be called > before setting the maximum number of VCPUs for the guest. > > For the moment, only configure the number SPIs. New informations could be > added later. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Jan Beulich <jbeulich@suse.com> For the XSM parts: Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Hi Julien, If I need to use 1 to 1 IRQ mapping in domU, I got an error in the following code: > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index e19e2f4..b0491c3 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -23,6 +23,25 @@ > #define DT_IRQ_TYPE_LEVEL_HIGH 0x00000004 > #define DT_IRQ_TYPE_LEVEL_LOW 0x00000008 > > +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config *d_config, > + libxl__domain_build_state *state, > + uint32_t domid) > +{ > + uint32_t nr_spis = 0; > + > + nr_spis += d_config->b_info.num_irqs; I have 4 IRQS for domU, they numbers are 53, 173, 174. With this change there will be 3 SPIs for domU, and below code will configure vgic for 32 lines. > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 17cde7a..a037ecc 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -60,16 +60,16 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned virq) > p->irq = virq; > } > > -int domain_vgic_init(struct domain *d) > +int domain_vgic_init(struct domain *d, unsigned int nr_spis) > { > int i; > > d->arch.vgic.ctlr = 0; > > - if ( is_hardware_domain(d) ) > - d->arch.vgic.nr_spis = gic_number_lines() - 32; > - else > - d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */ > + /* The number of SPIs as to be aligned to 32 see > + * GICD_TYPER.ITLinesNumber definition > + */ > + d->arch.vgic.nr_spis = ROUNDUP(nr_spis, 32); Here I got 32 lines for 3 SPIs And this will break checking for IRQ validity in xen/arch/arm/irq.c:route_irq_to_guest() call. I will not be able to use IRQ with number greater than 63. Can this be handled somehow? Or maybe it is already handled and I missed something ? Regards, Andrii
On 29/08/14 09:09, Andrii Tseglytskyi wrote: > Hi Julien, Hi Andrii, > Here I got 32 lines for 3 SPIs > > And this will break checking for IRQ validity in > xen/arch/arm/irq.c:route_irq_to_guest() call. I will not be able to > use IRQ with number greater than 63. > > Can this be handled somehow? Or maybe it is already handled and I > missed something ? You will have to modify the function for your purpose. If you plan to PIRQ == VIRQ every time, you could do smth like: d->arch.vgic.nr_spis = gic_number_lines() - 32; I assume, you already modified vgic_allocate_irq to return directly the pirq, and do nothing in vgic_free_irq. Regards,
On 29/08/14 15:49, Andrii Tseglytskyi wrote: > On Fri, Aug 29, 2014 at 9:57 PM, Julien Grall <julien.grall@linaro.org> wrote: >> You will have to modify the function for your purpose. If you plan to PIRQ >> == VIRQ every time, you could do smth like: >> >> >> d->arch.vgic.nr_spis = gic_number_lines() - 32; >> >> I assume, you already modified vgic_allocate_irq to return directly the >> pirq, and do nothing in vgic_free_irq. > > Yes, locally I did the same as you mentioned, but I'm thinking about > solution which will allow me to have 1 to 1 irq mapping and which will > be acceptable for Xen mainline. IHMO, the best solution would adding a boolean in the new DOMCTL to specify if we want to use PIRQ == VIRQ or not. Then you will have to modify vgic_allocate_irq to return the physical IRQ if the domain as this new option enabled. > My local diff is: > > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c > index 9333aa0..50ff100 100644 > --- a/xen/arch/arm/physdev.c > +++ b/xen/arch/arm/physdev.c > @@ -46,16 +46,13 @@ static int physdev_map_pirq(domid_t domid, int > type, int index, int *pirq_p) > > virq = vgic_allocate_virq(d, irq); > ret = -EMFILE; > if ( virq == -1 ) > goto free_domain; > > - ret = route_irq_to_guest(d, virq, irq, "routed IRQ"); > + ret = route_irq_to_guest(d, irq, irq, "routed IRQ"); With this change you may have some issue when unmap_pirq is used. I would modify vgic_allocate_irq to always return the physical IRQ number. The code will be also more clean.
Hi Julien, On Fri, Aug 29, 2014 at 11:04 PM, Julien Grall <julien.grall@linaro.org> wrote: > > > On 29/08/14 15:49, Andrii Tseglytskyi wrote: >> >> On Fri, Aug 29, 2014 at 9:57 PM, Julien Grall <julien.grall@linaro.org> >> wrote: >>> >>> You will have to modify the function for your purpose. If you plan to >>> PIRQ >>> == VIRQ every time, you could do smth like: >>> >>> >>> d->arch.vgic.nr_spis = gic_number_lines() - 32; >>> >>> I assume, you already modified vgic_allocate_irq to return directly the >>> pirq, and do nothing in vgic_free_irq. >> >> >> Yes, locally I did the same as you mentioned, but I'm thinking about >> solution which will allow me to have 1 to 1 irq mapping and which will >> be acceptable for Xen mainline. > > > IHMO, the best solution would adding a boolean in the new DOMCTL to specify > if we want to use PIRQ == VIRQ or not. > > Then you will have to modify vgic_allocate_irq to return the physical IRQ if > the domain as this new option enabled. This sounds good. > > >> My local diff is: >> >> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c >> index 9333aa0..50ff100 100644 >> --- a/xen/arch/arm/physdev.c >> +++ b/xen/arch/arm/physdev.c >> @@ -46,16 +46,13 @@ static int physdev_map_pirq(domid_t domid, int >> type, int index, int *pirq_p) >> >> virq = vgic_allocate_virq(d, irq); >> ret = -EMFILE; >> if ( virq == -1 ) >> goto free_domain; >> >> - ret = route_irq_to_guest(d, virq, irq, "routed IRQ"); >> + ret = route_irq_to_guest(d, irq, irq, "routed IRQ"); > > > With this change you may have some issue when unmap_pirq is used. I would > modify vgic_allocate_irq to always return the physical IRQ number. > > The code will be also more clean. Sure, anyway this won't be needed in case of new bool option for domain Regards, Andrii
Hi Julien, On 31 July 2014 20:30, Julien Grall <julien.grall@linaro.org> wrote: > The virtual GIC may differ between each guest (number of SPIs, emulate GIC > version...). Those informations may not be know when the domain is created > (for instance in case of migration). Therefore, move the VGIC > initialization > in a separate function. > > Introduce a new DOMCTL for ARM to configure the domain. This has to be > called > before setting the maximum number of VCPUs for the guest. > > For the moment, only configure the number SPIs. New informations could be > added later. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Jan Beulich <jbeulich@suse.com> > > --- > Changes in v2: > - Patch added > --- > tools/libxc/xc_domain.c | 12 ++++++++++++ > tools/libxc/xenctrl.h | 4 ++++ > tools/libxl/libxl_arch.h | 3 +++ > tools/libxl/libxl_arm.c | 19 +++++++++++++++++++ > tools/libxl/libxl_dom.c | 4 ++++ > tools/libxl/libxl_x86.c | 7 +++++++ > xen/arch/arm/domain.c | 28 ++++++++++++++++++++++------ > xen/arch/arm/domctl.c | 11 +++++++++++ > xen/arch/arm/setup.c | 10 ++++++++-- > xen/arch/arm/vgic.c | 10 +++++----- > xen/include/asm-arm/domain.h | 6 ++++++ > xen/include/asm-arm/vgic.h | 4 +++- > xen/include/public/domctl.h | 14 ++++++++++++++ > xen/xsm/flask/hooks.c | 3 +++ > xen/xsm/flask/policy/access_vectors | 2 ++ > 15 files changed, 123 insertions(+), 14 deletions(-) > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > index 27fe3b6..1348905 100644 > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -48,6 +48,18 @@ int xc_domain_create(xc_interface *xch, > return 0; > } > > +#if defined(__arm__) || defined(__arch64__) > Please change __arch64__ to __aarch64__ , otherwise it will break on arm64. Thanks, Pranav > +int xc_domain_configure(xc_interface *xch, uint32_t domid, > + uint32_t nr_spis) > +{ > + DECLARE_DOMCTL; > + domctl.cmd = XEN_DOMCTL_configure_domain; > + domctl.domain = (domid_t)domid; > + domctl.u.configuredomain.nr_spis = nr_spis; > + return do_domctl(xch, &domctl); > +} > +#endif > + > int xc_domain_cacheflush(xc_interface *xch, uint32_t domid, > xen_pfn_t start_pfn, xen_pfn_t nr_pfns) > { > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index 5beb846..cdda4e5 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -482,6 +482,10 @@ int xc_domain_create(xc_interface *xch, > uint32_t flags, > uint32_t *pdomid); > > +#if defined(__arm__) || defined(__aarch64__) > +int xc_domain_configure(xc_interface *xch, uint32_t domid, > + uint32_t nr_spis); > +#endif > > /* Functions to produce a dump of a given domain > * xc_domain_dumpcore - produces a dump to a specified file > diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h > index d3bc136..454f8db 100644 > --- a/tools/libxl/libxl_arch.h > +++ b/tools/libxl/libxl_arch.h > @@ -16,6 +16,9 @@ > #define LIBXL_ARCH_H > > /* arch specific internal domain creation function */ > +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config > *d_config, > + libxl__domain_build_state *state, > + uint32_t domid); > int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config > *d_config, > uint32_t domid); > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index e19e2f4..b0491c3 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -23,6 +23,25 @@ > #define DT_IRQ_TYPE_LEVEL_HIGH 0x00000004 > #define DT_IRQ_TYPE_LEVEL_LOW 0x00000008 > > +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config > *d_config, > + libxl__domain_build_state *state, > + uint32_t domid) > +{ > + uint32_t nr_spis = 0; > + > + nr_spis += d_config->b_info.num_irqs; > + > + LOG(DEBUG, "Allocate %u SPIs\n", nr_spis); > + > + if (xc_domain_configure(CTX->xch, domid, nr_spis) != 0) { > + LOG(ERROR, "Couldn't configure the domain"); > + return ERROR_FAIL; > + } > + > + return 0; > +} > + > + > int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config > *d_config, > uint32_t domid) > { > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index c944804..a0e4e34 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -235,6 +235,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid, > char *xs_domid, *con_domid; > int rc; > > + rc = libxl__arch_domain_create_pre(gc, d_config, state, domid); > + if (rc != 0) > + return rc; > + > if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) { > LOG(ERROR, "Couldn't set max vcpu count"); > return ERROR_FAIL; > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c > index 7589060..0abc1aa 100644 > --- a/tools/libxl/libxl_x86.c > +++ b/tools/libxl/libxl_x86.c > @@ -244,6 +244,13 @@ static int libxl__e820_alloc(libxl__gc *gc, uint32_t > domid, > return 0; > } > > +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config > *d_config, > + libxl__domain_build_state *state, > + uint32_t domid) > +{ > + return 0; > +} > + > int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config > *d_config, > uint32_t domid) > { > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index aa4a8d7..fc97f8c 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -472,6 +472,13 @@ int vcpu_initialise(struct vcpu *v) > if ( is_idle_vcpu(v) ) > return rc; > > + /* We can't initialize the VCPU if the VGIC has not been correctly > + * initialized. > + */ > + rc = -ENXIO; > + if ( !domain_vgic_is_initialized(v->domain) ) > + goto fail; > + > v->arch.sctlr = SCTLR_GUEST_INIT; > > /* > @@ -534,12 +541,6 @@ int arch_domain_create(struct domain *d, unsigned int > domcr_flags) > if ( (rc = p2m_alloc_table(d)) != 0 ) > goto fail; > > - if ( (rc = gicv_setup(d)) != 0 ) > - goto fail; > - > - if ( (rc = domain_vgic_init(d)) != 0 ) > - goto fail; > - > if ( (rc = domain_vtimer_init(d)) != 0 ) > goto fail; > > @@ -580,6 +581,21 @@ void arch_domain_destroy(struct domain *d) > free_xenheap_page(d->shared_info); > } > > +int domain_configure_vgic(struct domain *d, unsigned int nr_spis) > +{ > + int rc; > + > + if ( (rc = gicv_setup(d)) != 0 ) > + return rc; > + > + if ( (rc = domain_vgic_init(d, nr_spis)) != 0 ) > + return rc; > + > + d->arch.vgic.initialized = 1; > + > + return 0; > +} > + > static int is_guest_pv32_psr(uint32_t psr) > { > switch (psr & PSR_MODE_MASK) > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c > index 45974e7..bab92b2 100644 > --- a/xen/arch/arm/domctl.c > +++ b/xen/arch/arm/domctl.c > @@ -30,6 +30,17 @@ long arch_do_domctl(struct xen_domctl *domctl, struct > domain *d, > > return p2m_cache_flush(d, s, e); > } > + case XEN_DOMCTL_configure_domain: > + { > + if ( domain_vgic_is_initialized(d) ) > + return -EBUSY; > + > + /* Sanity check on the number of SPIs */ > + if ( domctl->u.configuredomain.nr_spis > (gic_number_lines() - > 32) ) > + return -EINVAL; > + > + return domain_configure_vgic(d, > domctl->u.configuredomain.nr_spis); > + } > > default: > return subarch_do_domctl(domctl, d, u_domctl); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 446b4dc..2be7dd1 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -840,8 +840,14 @@ void __init start_xen(unsigned long boot_phys_offset, > > /* Create initial domain 0. */ > dom0 = domain_create(0, 0, 0); > - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) > - panic("Error creating domain 0"); > + if ( IS_ERR(dom0) ) > + panic("Error creating domain 0"); > + > + if ( domain_configure_vgic(dom0, gic_number_lines() - 32) ) > + panic("Error configure the vgic for domain 0"); > + > + if ( alloc_dom0_vcpu0(dom0) == NULL ) > + panic("Error create vcpu0 for domain 0"); > > dom0->is_privileged = 1; > dom0->target = NULL; > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 17cde7a..a037ecc 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -60,16 +60,16 @@ static void vgic_init_pending_irq(struct pending_irq > *p, unsigned virq) > p->irq = virq; > } > > -int domain_vgic_init(struct domain *d) > +int domain_vgic_init(struct domain *d, unsigned int nr_spis) > { > int i; > > d->arch.vgic.ctlr = 0; > > - if ( is_hardware_domain(d) ) > - d->arch.vgic.nr_spis = gic_number_lines() - 32; > - else > - d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */ > + /* The number of SPIs as to be aligned to 32 see > + * GICD_TYPER.ITLinesNumber definition > + */ > + d->arch.vgic.nr_spis = ROUNDUP(nr_spis, 32); > > switch ( gic_hw_version() ) > { > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 5719fe5..44727b2 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -76,6 +76,10 @@ struct arch_domain > } virt_timer_base; > > struct { > + /* The VGIC initialization is defered to let the toolstack > + * configure the emulated GIC (such as number of SPIs, version...) > + */ > + bool_t initialized; > /* GIC HW version specific vGIC driver handler */ > const struct vgic_ops *handler; > /* > @@ -241,6 +245,8 @@ struct arch_vcpu > void vcpu_show_execution_state(struct vcpu *); > void vcpu_show_registers(const struct vcpu *); > > +int domain_configure_vgic(struct domain *d, unsigned int spis_number); > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 5ddc681..84ae441 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -148,6 +148,8 @@ static inline void vgic_byte_write(uint32_t *reg, > uint32_t var, int offset) > *reg |= var; > } > > +#define domain_vgic_is_initialized(d) ((d)->arch.vgic.initialized) > + > enum gic_sgi_mode; > > /* > @@ -158,7 +160,7 @@ enum gic_sgi_mode; > > #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32) > > -extern int domain_vgic_init(struct domain *d); > +extern int domain_vgic_init(struct domain *d, unsigned int nr_spis); > extern void domain_vgic_free(struct domain *d); > extern int vcpu_vgic_init(struct vcpu *v); > extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int > irq); > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 5b11bbf..b5f2ed7 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -67,6 +67,16 @@ struct xen_domctl_createdomain { > typedef struct xen_domctl_createdomain xen_domctl_createdomain_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_createdomain_t); > > +#if defined(__arm__) || defined(__aarch64__) > +/* XEN_DOMCTL_configure_domain */ > +struct xen_domctl_configuredomain { > + /* IN parameters */ > + uint32_t nr_spis; > +}; > +typedef struct xen_domctl_configuredomain xen_domctl_configuredomain_t; > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_configuredomain_t); > +#endif > + > /* XEN_DOMCTL_getdomaininfo */ > struct xen_domctl_getdomaininfo { > /* OUT variables. */ > @@ -1008,6 +1018,7 @@ struct xen_domctl { > #define XEN_DOMCTL_cacheflush 71 > #define XEN_DOMCTL_get_vcpu_msrs 72 > #define XEN_DOMCTL_set_vcpu_msrs 73 > +#define XEN_DOMCTL_configure_domain 74 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -1016,6 +1027,9 @@ struct xen_domctl { > domid_t domain; > union { > struct xen_domctl_createdomain createdomain; > +#if defined(__arm__) || defined(__aarch64__) > + struct xen_domctl_configuredomain configuredomain; > +#endif > struct xen_domctl_getdomaininfo getdomaininfo; > struct xen_domctl_getmemlist getmemlist; > struct xen_domctl_getpageframeinfo getpageframeinfo; > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index f2f59ea..4759461 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -715,6 +715,9 @@ static int flask_domctl(struct domain *d, int cmd) > case XEN_DOMCTL_cacheflush: > return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CACHEFLUSH); > > + case XEN_DOMCTL_configure_domain: > + return current_has_perm(d, SECCLASS_DOMAIN2, > DOMAIN2__CONFIGURE_DOMAIN); > + > default: > printk("flask_domctl: Unknown op %d\n", cmd); > return -EPERM; > diff --git a/xen/xsm/flask/policy/access_vectors > b/xen/xsm/flask/policy/access_vectors > index 32371a9..33eec66 100644 > --- a/xen/xsm/flask/policy/access_vectors > +++ b/xen/xsm/flask/policy/access_vectors > @@ -200,6 +200,8 @@ class domain2 > cacheflush > # Creation of the hardware domain when it is not dom0 > create_hardware_domain > +# XEN_DOMCTL_configure_domain > + configure_domain > } > > # Similar to class domain, but primarily contains domctls related to HVM > domains > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
On Fri, 2014-08-29 at 16:04 -0400, Julien Grall wrote: > > On 29/08/14 15:49, Andrii Tseglytskyi wrote: > > On Fri, Aug 29, 2014 at 9:57 PM, Julien Grall <julien.grall@linaro.org> wrote: > >> You will have to modify the function for your purpose. If you plan to PIRQ > >> == VIRQ every time, you could do smth like: > >> > >> > >> d->arch.vgic.nr_spis = gic_number_lines() - 32; > >> > >> I assume, you already modified vgic_allocate_irq to return directly the > >> pirq, and do nothing in vgic_free_irq. > > > > Yes, locally I did the same as you mentioned, but I'm thinking about > > solution which will allow me to have 1 to 1 irq mapping and which will > > be acceptable for Xen mainline. > > IHMO, the best solution would adding a boolean in the new DOMCTL to > specify if we want to use PIRQ == VIRQ or not. Shouldn't it be up to the toolstack to pass in a suitably large nr_spis, i.e. at least as large as the maximum irq it wasn't to map 1:1? So with Andrii's set of 53, 173, 174 the toolstack would have to pass 174. Ian. > Then you will have to modify vgic_allocate_irq to return the physical > IRQ if the domain as this new option enabled. > > > My local diff is: > > > > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c > > index 9333aa0..50ff100 100644 > > --- a/xen/arch/arm/physdev.c > > +++ b/xen/arch/arm/physdev.c > > @@ -46,16 +46,13 @@ static int physdev_map_pirq(domid_t domid, int > > type, int index, int *pirq_p) > > > > virq = vgic_allocate_virq(d, irq); > > ret = -EMFILE; > > if ( virq == -1 ) > > goto free_domain; > > > > - ret = route_irq_to_guest(d, virq, irq, "routed IRQ"); > > + ret = route_irq_to_guest(d, irq, irq, "routed IRQ"); > > With this change you may have some issue when unmap_pirq is used. I > would modify vgic_allocate_irq to always return the physical IRQ number. > > The code will be also more clean. >
On Wed, 2014-08-06 at 16:35 +0100, Stefano Stabellini wrote: > > +#if defined(__arm__) || defined(__arch64__) > > +int xc_domain_configure(xc_interface *xch, uint32_t domid, > > + uint32_t nr_spis) > > Given that we'll likely add new fields to xen_domctl_configuredomain, I > think it is best if we pass a struct domain_configure to > xc_domain_configure instead of nr_spis. The struct we pass to > xc_domain_configure doesn't have to be identical to struct > xen_domctl_configuredomain, but at the moment it would be. In that case you may as well inline the domctl call into arch_domain_create_pre, which I don't think is a bad idea. Ian
On Thu, 2014-07-31 at 16:00 +0100, Julien Grall wrote:
> + /* The number of SPIs as to be aligned to 32 see
"has to be".
Ian
Hi Ian, On 09/09/14 06:35, Ian Campbell wrote: > On Wed, 2014-08-06 at 16:35 +0100, Stefano Stabellini wrote: > >>> +#if defined(__arm__) || defined(__arch64__) >>> +int xc_domain_configure(xc_interface *xch, uint32_t domid, >>> + uint32_t nr_spis) >> >> Given that we'll likely add new fields to xen_domctl_configuredomain, I >> think it is best if we pass a struct domain_configure to >> xc_domain_configure instead of nr_spis. The struct we pass to >> xc_domain_configure doesn't have to be identical to struct >> xen_domctl_configuredomain, but at the moment it would be. > > In that case you may as well inline the domctl call into > arch_domain_create_pre, which I don't think is a bad idea. It will be hard. Libxl doesn't have access to the facility for create bounce buffer... Regards,
Hi Ian, On 09/09/14 06:33, Ian Campbell wrote: > On Fri, 2014-08-29 at 16:04 -0400, Julien Grall wrote: >> >> On 29/08/14 15:49, Andrii Tseglytskyi wrote: >>> On Fri, Aug 29, 2014 at 9:57 PM, Julien Grall <julien.grall@linaro.org> wrote: >>>> You will have to modify the function for your purpose. If you plan to PIRQ >>>> == VIRQ every time, you could do smth like: >>>> >>>> >>>> d->arch.vgic.nr_spis = gic_number_lines() - 32; >>>> >>>> I assume, you already modified vgic_allocate_irq to return directly the >>>> pirq, and do nothing in vgic_free_irq. >>> >>> Yes, locally I did the same as you mentioned, but I'm thinking about >>> solution which will allow me to have 1 to 1 irq mapping and which will >>> be acceptable for Xen mainline. >> >> IHMO, the best solution would adding a boolean in the new DOMCTL to >> specify if we want to use PIRQ == VIRQ or not. > > Shouldn't it be up to the toolstack to pass in a suitably large nr_spis, > i.e. at least as large as the maximum irq it wasn't to map 1:1? For the moment, Xen always choose the VIRQ. Looking again to the hypercall, I think we can let the toolstack asking for a specific VIRQ. For now, I don't handle this case. I will rework my series to do it. Hence, it will make patch #20 less uglier. Also it would help Andrii use case and avoid him to carry a bigger patch :). Regards,
On Tue, Sep 9, 2014 at 10:11 PM, Julien Grall <julien.grall@linaro.org> wrote: > Hi Ian, > > On 09/09/14 06:33, Ian Campbell wrote: > >> On Fri, 2014-08-29 at 16:04 -0400, Julien Grall wrote: >> >>> >>> On 29/08/14 15:49, Andrii Tseglytskyi wrote: >>> >>>> On Fri, Aug 29, 2014 at 9:57 PM, Julien Grall <julien.grall@linaro.org> >>>> wrote: >>>> >>>>> You will have to modify the function for your purpose. If you plan to >>>>> PIRQ >>>>> == VIRQ every time, you could do smth like: >>>>> >>>>> >>>>> d->arch.vgic.nr_spis = gic_number_lines() - 32; >>>>> >>>>> I assume, you already modified vgic_allocate_irq to return directly the >>>>> pirq, and do nothing in vgic_free_irq. >>>>> >>>> >>>> Yes, locally I did the same as you mentioned, but I'm thinking about >>>> solution which will allow me to have 1 to 1 irq mapping and which will >>>> be acceptable for Xen mainline. >>>> >>> >>> IHMO, the best solution would adding a boolean in the new DOMCTL to >>> specify if we want to use PIRQ == VIRQ or not. >>> >> >> Shouldn't it be up to the toolstack to pass in a suitably large nr_spis, >> i.e. at least as large as the maximum irq it wasn't to map 1:1? >> > > For the moment, Xen always choose the VIRQ. Looking again to the > hypercall, I think we can let the toolstack asking for a specific VIRQ. > > For now, I don't handle this case. I will rework my series to do it. > Hence, it will make patch #20 less uglier. > > Also it would help Andrii use case and avoid him to carry a bigger patch > :). > > This sounds really great. I think I will not need any additional local patch if you handle this. Thank you Regards, Andrii > Regards, > > -- > Julien Grall >
On Tue, 2014-09-09 at 11:57 -0700, Julien Grall wrote: > Hi Ian, > > On 09/09/14 06:35, Ian Campbell wrote: > > On Wed, 2014-08-06 at 16:35 +0100, Stefano Stabellini wrote: > > > >>> +#if defined(__arm__) || defined(__arch64__) > >>> +int xc_domain_configure(xc_interface *xch, uint32_t domid, > >>> + uint32_t nr_spis) > >> > >> Given that we'll likely add new fields to xen_domctl_configuredomain, I > >> think it is best if we pass a struct domain_configure to > >> xc_domain_configure instead of nr_spis. The struct we pass to > >> xc_domain_configure doesn't have to be identical to struct > >> xen_domctl_configuredomain, but at the moment it would be. > > > > In that case you may as well inline the domctl call into > > arch_domain_create_pre, which I don't think is a bad idea. > > It will be hard. Libxl doesn't have access to the facility for create > bounce buffer... Ah, nevermind then. Passing the struct as a ptr as Stefano suggests would probably be good though. Ian.
Hi Stefano, Sorry, I forgot to answer to this mail. On 06/08/14 08:35, Stefano Stabellini wrote: > On Thu, 31 Jul 2014, Julien Grall wrote: >> The virtual GIC may differ between each guest (number of SPIs, emulate GIC >> version...). Those informations may not be know when the domain is created >> (for instance in case of migration). Therefore, move the VGIC initialization >> in a separate function. >> >> Introduce a new DOMCTL for ARM to configure the domain. This has to be called >> before setting the maximum number of VCPUs for the guest. >> >> For the moment, only configure the number SPIs. New informations could be >> added later. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> Cc: Jan Beulich <jbeulich@suse.com> >> >> --- >> Changes in v2: >> - Patch added >> --- >> tools/libxc/xc_domain.c | 12 ++++++++++++ >> tools/libxc/xenctrl.h | 4 ++++ >> tools/libxl/libxl_arch.h | 3 +++ >> tools/libxl/libxl_arm.c | 19 +++++++++++++++++++ >> tools/libxl/libxl_dom.c | 4 ++++ >> tools/libxl/libxl_x86.c | 7 +++++++ >> xen/arch/arm/domain.c | 28 ++++++++++++++++++++++------ >> xen/arch/arm/domctl.c | 11 +++++++++++ >> xen/arch/arm/setup.c | 10 ++++++++-- >> xen/arch/arm/vgic.c | 10 +++++----- >> xen/include/asm-arm/domain.h | 6 ++++++ >> xen/include/asm-arm/vgic.h | 4 +++- >> xen/include/public/domctl.h | 14 ++++++++++++++ >> xen/xsm/flask/hooks.c | 3 +++ >> xen/xsm/flask/policy/access_vectors | 2 ++ >> 15 files changed, 123 insertions(+), 14 deletions(-) >> >> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c >> index 27fe3b6..1348905 100644 >> --- a/tools/libxc/xc_domain.c >> +++ b/tools/libxc/xc_domain.c >> @@ -48,6 +48,18 @@ int xc_domain_create(xc_interface *xch, >> return 0; >> } >> >> +#if defined(__arm__) || defined(__arch64__) >> +int xc_domain_configure(xc_interface *xch, uint32_t domid, >> + uint32_t nr_spis) > > Given that we'll likely add new fields to xen_domctl_configuredomain, I > think it is best if we pass a struct domain_configure to > xc_domain_configure instead of nr_spis. The struct we pass to > xc_domain_configure doesn't have to be identical to struct > xen_domctl_configuredomain, but at the moment it would be. Ok. I will do. [..] >> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c >> index 45974e7..bab92b2 100644 >> --- a/xen/arch/arm/domctl.c >> +++ b/xen/arch/arm/domctl.c >> @@ -30,6 +30,17 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d, >> >> return p2m_cache_flush(d, s, e); >> } >> + case XEN_DOMCTL_configure_domain: >> + { >> + if ( domain_vgic_is_initialized(d) ) >> + return -EBUSY; > > Given that XEN_DOMCTL_configure_domain should be called exactly once at > domain creation, instead of introducing domain_vgic_is_initialized, I > would make sure that XEN_DOMCTL_configure_domain hasn't been called for > this domain before. In other words, I would make this check more > generic, rather than vgic specific. The VGIC initialization may have fail because there is not enough memory. So it would be valid, even if it's stupid, to call this DOMCTL twice. domain_vgic_is_initialized is also used in vcpu_initialise to check that the VGIC has effectively been initialized I would keep this check for now.
On Thu, 2014-09-11 at 16:01 -0700, Julien Grall wrote: > >> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c > >> index 45974e7..bab92b2 100644 > >> --- a/xen/arch/arm/domctl.c > >> +++ b/xen/arch/arm/domctl.c > >> @@ -30,6 +30,17 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d, > >> > >> return p2m_cache_flush(d, s, e); > >> } > >> + case XEN_DOMCTL_configure_domain: > >> + { > >> + if ( domain_vgic_is_initialized(d) ) > >> + return -EBUSY; > > > > Given that XEN_DOMCTL_configure_domain should be called exactly once at > > domain creation, instead of introducing domain_vgic_is_initialized, I > > would make sure that XEN_DOMCTL_configure_domain hasn't been called for > > this domain before. In other words, I would make this check more > > generic, rather than vgic specific. > > The VGIC initialization may have fail because there is not enough > memory. So it would be valid, even if it's stupid, to call this DOMCTL > twice. I think there was an implied "successfully" in what Stefano said. That said it's hard to imagine that a toolstack would retry on failure of such a hypercall, rather than aborting the build entirely (and perhaps trying again at that level) > domain_vgic_is_initialized is also used in vcpu_initialise to check that > the VGIC has effectively been initialized > > I would keep this check for now. >
On 09/04/2014 10:02 AM, Pranavkumar Sawargaonkar wrote: > Hi Julien, Hi Pranav, > On 31 July 2014 20:30, Julien Grall <julien.grall@linaro.org > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > index 27fe3b6..1348905 100644 > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -48,6 +48,18 @@ int xc_domain_create(xc_interface *xch, > return 0; > } > > +#if defined(__arm__) || defined(__arch64__) > > > Please change __arch64__ to __aarch64__ , otherwise it will break on arm64. Thanks for spotting this error. I will update the patch for the next version. Regards,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c index 27fe3b6..1348905 100644 --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -48,6 +48,18 @@ int xc_domain_create(xc_interface *xch, return 0; } +#if defined(__arm__) || defined(__arch64__) +int xc_domain_configure(xc_interface *xch, uint32_t domid, + uint32_t nr_spis) +{ + DECLARE_DOMCTL; + domctl.cmd = XEN_DOMCTL_configure_domain; + domctl.domain = (domid_t)domid; + domctl.u.configuredomain.nr_spis = nr_spis; + return do_domctl(xch, &domctl); +} +#endif + int xc_domain_cacheflush(xc_interface *xch, uint32_t domid, xen_pfn_t start_pfn, xen_pfn_t nr_pfns) { diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h index 5beb846..cdda4e5 100644 --- a/tools/libxc/xenctrl.h +++ b/tools/libxc/xenctrl.h @@ -482,6 +482,10 @@ int xc_domain_create(xc_interface *xch, uint32_t flags, uint32_t *pdomid); +#if defined(__arm__) || defined(__aarch64__) +int xc_domain_configure(xc_interface *xch, uint32_t domid, + uint32_t nr_spis); +#endif /* Functions to produce a dump of a given domain * xc_domain_dumpcore - produces a dump to a specified file diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h index d3bc136..454f8db 100644 --- a/tools/libxl/libxl_arch.h +++ b/tools/libxl/libxl_arch.h @@ -16,6 +16,9 @@ #define LIBXL_ARCH_H /* arch specific internal domain creation function */ +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config *d_config, + libxl__domain_build_state *state, + uint32_t domid); int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid); diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index e19e2f4..b0491c3 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -23,6 +23,25 @@ #define DT_IRQ_TYPE_LEVEL_HIGH 0x00000004 #define DT_IRQ_TYPE_LEVEL_LOW 0x00000008 +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config *d_config, + libxl__domain_build_state *state, + uint32_t domid) +{ + uint32_t nr_spis = 0; + + nr_spis += d_config->b_info.num_irqs; + + LOG(DEBUG, "Allocate %u SPIs\n", nr_spis); + + if (xc_domain_configure(CTX->xch, domid, nr_spis) != 0) { + LOG(ERROR, "Couldn't configure the domain"); + return ERROR_FAIL; + } + + return 0; +} + + int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid) { diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index c944804..a0e4e34 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -235,6 +235,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid, char *xs_domid, *con_domid; int rc; + rc = libxl__arch_domain_create_pre(gc, d_config, state, domid); + if (rc != 0) + return rc; + if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) { LOG(ERROR, "Couldn't set max vcpu count"); return ERROR_FAIL; diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c index 7589060..0abc1aa 100644 --- a/tools/libxl/libxl_x86.c +++ b/tools/libxl/libxl_x86.c @@ -244,6 +244,13 @@ static int libxl__e820_alloc(libxl__gc *gc, uint32_t domid, return 0; } +int libxl__arch_domain_create_pre(libxl__gc *gc, libxl_domain_config *d_config, + libxl__domain_build_state *state, + uint32_t domid) +{ + return 0; +} + int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid) { diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index aa4a8d7..fc97f8c 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -472,6 +472,13 @@ int vcpu_initialise(struct vcpu *v) if ( is_idle_vcpu(v) ) return rc; + /* We can't initialize the VCPU if the VGIC has not been correctly + * initialized. + */ + rc = -ENXIO; + if ( !domain_vgic_is_initialized(v->domain) ) + goto fail; + v->arch.sctlr = SCTLR_GUEST_INIT; /* @@ -534,12 +541,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) if ( (rc = p2m_alloc_table(d)) != 0 ) goto fail; - if ( (rc = gicv_setup(d)) != 0 ) - goto fail; - - if ( (rc = domain_vgic_init(d)) != 0 ) - goto fail; - if ( (rc = domain_vtimer_init(d)) != 0 ) goto fail; @@ -580,6 +581,21 @@ void arch_domain_destroy(struct domain *d) free_xenheap_page(d->shared_info); } +int domain_configure_vgic(struct domain *d, unsigned int nr_spis) +{ + int rc; + + if ( (rc = gicv_setup(d)) != 0 ) + return rc; + + if ( (rc = domain_vgic_init(d, nr_spis)) != 0 ) + return rc; + + d->arch.vgic.initialized = 1; + + return 0; +} + static int is_guest_pv32_psr(uint32_t psr) { switch (psr & PSR_MODE_MASK) diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c index 45974e7..bab92b2 100644 --- a/xen/arch/arm/domctl.c +++ b/xen/arch/arm/domctl.c @@ -30,6 +30,17 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d, return p2m_cache_flush(d, s, e); } + case XEN_DOMCTL_configure_domain: + { + if ( domain_vgic_is_initialized(d) ) + return -EBUSY; + + /* Sanity check on the number of SPIs */ + if ( domctl->u.configuredomain.nr_spis > (gic_number_lines() - 32) ) + return -EINVAL; + + return domain_configure_vgic(d, domctl->u.configuredomain.nr_spis); + } default: return subarch_do_domctl(domctl, d, u_domctl); diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 446b4dc..2be7dd1 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -840,8 +840,14 @@ void __init start_xen(unsigned long boot_phys_offset, /* Create initial domain 0. */ dom0 = domain_create(0, 0, 0); - if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) - panic("Error creating domain 0"); + if ( IS_ERR(dom0) ) + panic("Error creating domain 0"); + + if ( domain_configure_vgic(dom0, gic_number_lines() - 32) ) + panic("Error configure the vgic for domain 0"); + + if ( alloc_dom0_vcpu0(dom0) == NULL ) + panic("Error create vcpu0 for domain 0"); dom0->is_privileged = 1; dom0->target = NULL; diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 17cde7a..a037ecc 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -60,16 +60,16 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned virq) p->irq = virq; } -int domain_vgic_init(struct domain *d) +int domain_vgic_init(struct domain *d, unsigned int nr_spis) { int i; d->arch.vgic.ctlr = 0; - if ( is_hardware_domain(d) ) - d->arch.vgic.nr_spis = gic_number_lines() - 32; - else - d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */ + /* The number of SPIs as to be aligned to 32 see + * GICD_TYPER.ITLinesNumber definition + */ + d->arch.vgic.nr_spis = ROUNDUP(nr_spis, 32); switch ( gic_hw_version() ) { diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 5719fe5..44727b2 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -76,6 +76,10 @@ struct arch_domain } virt_timer_base; struct { + /* The VGIC initialization is defered to let the toolstack + * configure the emulated GIC (such as number of SPIs, version...) + */ + bool_t initialized; /* GIC HW version specific vGIC driver handler */ const struct vgic_ops *handler; /* @@ -241,6 +245,8 @@ struct arch_vcpu void vcpu_show_execution_state(struct vcpu *); void vcpu_show_registers(const struct vcpu *); +int domain_configure_vgic(struct domain *d, unsigned int spis_number); + #endif /* __ASM_DOMAIN_H__ */ /* diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 5ddc681..84ae441 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -148,6 +148,8 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset) *reg |= var; } +#define domain_vgic_is_initialized(d) ((d)->arch.vgic.initialized) + enum gic_sgi_mode; /* @@ -158,7 +160,7 @@ enum gic_sgi_mode; #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32) -extern int domain_vgic_init(struct domain *d); +extern int domain_vgic_init(struct domain *d, unsigned int nr_spis); extern void domain_vgic_free(struct domain *d); extern int vcpu_vgic_init(struct vcpu *v); extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq); diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 5b11bbf..b5f2ed7 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -67,6 +67,16 @@ struct xen_domctl_createdomain { typedef struct xen_domctl_createdomain xen_domctl_createdomain_t; DEFINE_XEN_GUEST_HANDLE(xen_domctl_createdomain_t); +#if defined(__arm__) || defined(__aarch64__) +/* XEN_DOMCTL_configure_domain */ +struct xen_domctl_configuredomain { + /* IN parameters */ + uint32_t nr_spis; +}; +typedef struct xen_domctl_configuredomain xen_domctl_configuredomain_t; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_configuredomain_t); +#endif + /* XEN_DOMCTL_getdomaininfo */ struct xen_domctl_getdomaininfo { /* OUT variables. */ @@ -1008,6 +1018,7 @@ struct xen_domctl { #define XEN_DOMCTL_cacheflush 71 #define XEN_DOMCTL_get_vcpu_msrs 72 #define XEN_DOMCTL_set_vcpu_msrs 73 +#define XEN_DOMCTL_configure_domain 74 #define XEN_DOMCTL_gdbsx_guestmemio 1000 #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 @@ -1016,6 +1027,9 @@ struct xen_domctl { domid_t domain; union { struct xen_domctl_createdomain createdomain; +#if defined(__arm__) || defined(__aarch64__) + struct xen_domctl_configuredomain configuredomain; +#endif struct xen_domctl_getdomaininfo getdomaininfo; struct xen_domctl_getmemlist getmemlist; struct xen_domctl_getpageframeinfo getpageframeinfo; diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index f2f59ea..4759461 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -715,6 +715,9 @@ static int flask_domctl(struct domain *d, int cmd) case XEN_DOMCTL_cacheflush: return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CACHEFLUSH); + case XEN_DOMCTL_configure_domain: + return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__CONFIGURE_DOMAIN); + default: printk("flask_domctl: Unknown op %d\n", cmd); return -EPERM; diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index 32371a9..33eec66 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -200,6 +200,8 @@ class domain2 cacheflush # Creation of the hardware domain when it is not dom0 create_hardware_domain +# XEN_DOMCTL_configure_domain + configure_domain } # Similar to class domain, but primarily contains domctls related to HVM domains
The virtual GIC may differ between each guest (number of SPIs, emulate GIC version...). Those informations may not be know when the domain is created (for instance in case of migration). Therefore, move the VGIC initialization in a separate function. Introduce a new DOMCTL for ARM to configure the domain. This has to be called before setting the maximum number of VCPUs for the guest. For the moment, only configure the number SPIs. New informations could be added later. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> Cc: Jan Beulich <jbeulich@suse.com> --- Changes in v2: - Patch added --- tools/libxc/xc_domain.c | 12 ++++++++++++ tools/libxc/xenctrl.h | 4 ++++ tools/libxl/libxl_arch.h | 3 +++ tools/libxl/libxl_arm.c | 19 +++++++++++++++++++ tools/libxl/libxl_dom.c | 4 ++++ tools/libxl/libxl_x86.c | 7 +++++++ xen/arch/arm/domain.c | 28 ++++++++++++++++++++++------ xen/arch/arm/domctl.c | 11 +++++++++++ xen/arch/arm/setup.c | 10 ++++++++-- xen/arch/arm/vgic.c | 10 +++++----- xen/include/asm-arm/domain.h | 6 ++++++ xen/include/asm-arm/vgic.h | 4 +++- xen/include/public/domctl.h | 14 ++++++++++++++ xen/xsm/flask/hooks.c | 3 +++ xen/xsm/flask/policy/access_vectors | 2 ++ 15 files changed, 123 insertions(+), 14 deletions(-)