| Message ID | 1466293521-32746-7-git-send-email-shankerd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Shanker, On 19/06/16 00:45, Shanker Donthineni wrote: > Split code that installs mmio handlers for GICD and Re-distributor > regions to a new function. The intension of this separation is to defer > steps that registers vgic_v2/v3 mmio handlers. Looking at this patch and the follow-up ones, I don't think this is the right way to go. You differ the registration of the IO handlers just because you are unable to find the size of the handlers array. I am wondering if the array for the handlers is the best solution here. On another side, it would be possible to find the maximum of handlers before hand. > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 5df5f01..5b39e0d 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -151,9 +151,12 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis) > for ( i = 0; i < NR_GIC_SGI; i++ ) > set_bit(i, d->arch.vgic.allocated_irqs); > > + d->arch.vgic.handler->domain_register_mmio(d); > + > return 0; > } > > + Spurious change. > void register_vgic_ops(struct domain *d, const struct vgic_ops *ops) > { > d->arch.vgic.handler = ops; > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index fbb763a..8fe65b4 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -132,6 +132,8 @@ struct vgic_ops { > void (*domain_free)(struct domain *d); > /* vGIC sysreg emulation */ > int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr); > + /* Register mmio handlers */ > + void (*domain_register_mmio)(struct domain *d); > /* Maximum number of vCPU supported */ > const unsigned int max_vcpus; > }; > Regards,
On 06/21/2016 05:49 AM, Julien Grall wrote: > Hello Shanker, > > On 19/06/16 00:45, Shanker Donthineni wrote: >> Split code that installs mmio handlers for GICD and Re-distributor >> regions to a new function. The intension of this separation is to defer >> steps that registers vgic_v2/v3 mmio handlers. > > Looking at this patch and the follow-up ones, I don't think this is > the right way to go. You differ the registration of the IO handlers > just because you are unable to find the size of the handlers array. > Is there any better approach? > I am wondering if the array for the handlers is the best solution > here. On another side, it would be possible to find the maximum of > handlers before hand. > The purpose of this change is to limit size of 'struct domain' less than PAGE_SIZE. I can think of second approach split vgic_init() into two stages, one for vgic registration and the second one for vgic_init(). This also requires a few lines of code changes to vgic_v2/v3_init() and vgic_init(). int arch_domain_create(struct domain *d, unsigned int domcr_flags, struct xen_arch_domainconfig *config) ... domain_vgic_register(d)); domain_io_init(d, mmio_count); domain_vgic_init(d, config->nr_spis)); >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 5df5f01..5b39e0d 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -151,9 +151,12 @@ int domain_vgic_init(struct domain *d, unsigned int > nr_spis) >> for ( i = 0; i < NR_GIC_SGI; i++ ) >> set_bit(i, d->arch.vgic.allocated_irqs); >> >> + d->arch.vgic.handler->domain_register_mmio(d); >> + >> return 0; >> } >> >> + > > Spurious change. > >> void register_vgic_ops(struct domain *d, const struct vgic_ops *ops) >> { >> d->arch.vgic.handler = ops; >> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h >> index fbb763a..8fe65b4 100644 >> --- a/xen/include/asm-arm/vgic.h >> +++ b/xen/include/asm-arm/vgic.h >> @@ -132,6 +132,8 @@ struct vgic_ops { >> void (*domain_free)(struct domain *d); >> /* vGIC sysreg emulation */ >> int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr); >> + /* Register mmio handlers */ >> + void (*domain_register_mmio)(struct domain *d); >> /* Maximum number of vCPU supported */ >> const unsigned int max_vcpus; >> }; >> > > Regards, >
On 21/06/16 15:36, Shanker Donthineni wrote: > > > On 06/21/2016 05:49 AM, Julien Grall wrote: >> Hello Shanker, >> >> On 19/06/16 00:45, Shanker Donthineni wrote: >>> Split code that installs mmio handlers for GICD and Re-distributor >>> regions to a new function. The intension of this separation is to defer >>> steps that registers vgic_v2/v3 mmio handlers. >> >> Looking at this patch and the follow-up ones, I don't think this is >> the right way to go. You differ the registration of the IO handlers >> just because you are unable to find the size of the handlers array. >> > Is there any better approach? Possibly using a different data structure. >> I am wondering if the array for the handlers is the best solution >> here. On another side, it would be possible to find the maximum of >> handlers before hand. >> > The purpose of this change is to limit size of 'struct domain' less than > PAGE_SIZE. I can think of second approach split vgic_init() into two > stages, one for vgic registration and the second one for vgic_init(). > This also requires a few lines of code changes to vgic_v2/v3_init() and > vgic_init(). I am fine as long as vgic_register_ does not do more than counting the number of IO handlers. You could re-use vgic_init_v{2,3} for this purpose. Regards,
Hi Julien, On 06/21/2016 09:48 AM, Julien Grall wrote: > > > On 21/06/16 15:36, Shanker Donthineni wrote: >> >> >> On 06/21/2016 05:49 AM, Julien Grall wrote: >>> Hello Shanker, >>> >>> On 19/06/16 00:45, Shanker Donthineni wrote: >>>> Split code that installs mmio handlers for GICD and Re-distributor >>>> regions to a new function. The intension of this separation is to >>>> defer >>>> steps that registers vgic_v2/v3 mmio handlers. >>> >>> Looking at this patch and the follow-up ones, I don't think this is >>> the right way to go. You differ the registration of the IO handlers >>> just because you are unable to find the size of the handlers array. >>> >> Is there any better approach? > > Possibly using a different data structure. > >>> I am wondering if the array for the handlers is the best solution >>> here. On another side, it would be possible to find the maximum of >>> handlers before hand. >>> >> The purpose of this change is to limit size of 'struct domain' less than >> PAGE_SIZE. I can think of second approach split vgic_init() into two >> stages, one for vgic registration and the second one for vgic_init(). >> This also requires a few lines of code changes to vgic_v2/v3_init() and >> vgic_init(). > > I am fine as long as vgic_register_ does not do more than counting the > number of IO handlers. You could re-use vgic_init_v{2,3} for this > purpose. > The way we are doing vgic_init() initialization has to be cleaned-up and rearrange a few lines of code for retrieving the number mmio handlers that are required dom0/domU domain. > Regards, >
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index f5778e6..d42b408 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -645,6 +645,12 @@ static int vgic_v2_vcpu_init(struct vcpu *v) return 0; } +static void vgic_v2_domain_register_mmio(struct domain *d) +{ + register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase, + PAGE_SIZE, NULL); +} + static int vgic_v2_domain_init(struct domain *d) { int ret; @@ -693,9 +699,6 @@ static int vgic_v2_domain_init(struct domain *d) if ( ret ) return ret; - register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase, - PAGE_SIZE, NULL); - return 0; } @@ -708,6 +711,7 @@ static const struct vgic_ops vgic_v2_ops = { .vcpu_init = vgic_v2_vcpu_init, .domain_init = vgic_v2_domain_init, .domain_free = vgic_v2_domain_free, + .domain_register_mmio = vgic_v2_domain_register_mmio, .max_vcpus = 8, }; diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index e877e9e..3a5aeb6 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1391,6 +1391,28 @@ static int vgic_v3_vcpu_init(struct vcpu *v) return 0; } +static void vgic_v3_domain_register_mmio(struct domain *d) +{ + int i; + + /* Register mmio handle for the Distributor */ + register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase, + SZ_64K, NULL); + + /* + * Register mmio handler per contiguous region occupied by the + * redistributors. The handler will take care to choose which + * redistributor is targeted. + */ + for ( i = 0; i < d->arch.vgic.nr_regions; i++ ) + { + struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i]; + + register_mmio_handler(d, &vgic_rdistr_mmio_handler, + region->base, region->size, region); + } +} + static int vgic_v3_domain_init(struct domain *d) { struct vgic_rdist_region *rdist_regions; @@ -1455,23 +1477,6 @@ static int vgic_v3_domain_init(struct domain *d) d->arch.vgic.rdist_regions[0].first_cpu = 0; } - /* Register mmio handle for the Distributor */ - register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase, - SZ_64K, NULL); - - /* - * Register mmio handler per contiguous region occupied by the - * redistributors. The handler will take care to choose which - * redistributor is targeted. - */ - for ( i = 0; i < d->arch.vgic.nr_regions; i++ ) - { - struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i]; - - register_mmio_handler(d, &vgic_rdistr_mmio_handler, - region->base, region->size, region); - } - d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT; return 0; @@ -1487,6 +1492,7 @@ static const struct vgic_ops v3_ops = { .domain_init = vgic_v3_domain_init, .domain_free = vgic_v3_domain_free, .emulate_sysreg = vgic_v3_emulate_sysreg, + .domain_register_mmio = vgic_v3_domain_register_mmio, /* * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU * that can be supported is up to 4096(==256*16) in theory. diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 5df5f01..5b39e0d 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -151,9 +151,12 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis) for ( i = 0; i < NR_GIC_SGI; i++ ) set_bit(i, d->arch.vgic.allocated_irqs); + d->arch.vgic.handler->domain_register_mmio(d); + return 0; } + void register_vgic_ops(struct domain *d, const struct vgic_ops *ops) { d->arch.vgic.handler = ops; diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index fbb763a..8fe65b4 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -132,6 +132,8 @@ struct vgic_ops { void (*domain_free)(struct domain *d); /* vGIC sysreg emulation */ int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr); + /* Register mmio handlers */ + void (*domain_register_mmio)(struct domain *d); /* Maximum number of vCPU supported */ const unsigned int max_vcpus; };
Split code that installs mmio handlers for GICD and Re-distributor regions to a new function. The intension of this separation is to defer steps that registers vgic_v2/v3 mmio handlers. Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org> --- xen/arch/arm/vgic-v2.c | 10 +++++++--- xen/arch/arm/vgic-v3.c | 40 +++++++++++++++++++++++----------------- xen/arch/arm/vgic.c | 3 +++ xen/include/asm-arm/vgic.h | 2 ++ 4 files changed, 35 insertions(+), 20 deletions(-)