| Message ID | 20180209143937.28866-9-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi, On 02/09/2018 02:38 PM, Andre Przywara wrote: > The VGIC model used for a domain (GICv2 or GICv3) determines the maximum > number of VCPUs for that guest, as GICv2 can only handle 8 processors. > In the moment we carry this per-VGIC-model limit in the vgic_ops, > alongside the model specific functions. That makes some sense, but > exposes some current VGIC implementation details to generic Xen code. > Add a new arch specific field in our domain structure to hold this vcpu limit, > and initialize it when we set the ops. This allows us to plug in the new > VGIC later without also needing to carry some ops structure. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/domain.c | 5 ++--- > xen/arch/arm/vgic.c | 3 ++- > xen/include/asm-arm/domain.h | 1 + > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index a010443bfd..9ad4cd0a6e 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -975,11 +975,10 @@ unsigned int domain_max_vcpus(const struct domain *d) > * allocation when the vgic_ops haven't been initialised yet, > * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null. > */ > - if ( !d->arch.vgic.handler ) > + if ( !d->arch.max_vcpus ) > return MAX_VIRT_CPUS; > else > - return min_t(unsigned int, MAX_VIRT_CPUS, > - d->arch.vgic.handler->max_vcpus); > + return d->arch.max_vcpus; > } > > /* > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 9921769b15..5f47aa84a9 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -166,7 +166,8 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis) > > void register_vgic_ops(struct domain *d, const struct vgic_ops *ops) > { > - d->arch.vgic.handler = ops; > + d->arch.vgic.handler = ops; > + d->arch.max_vcpus = ops->max_vcpus; > } > > void domain_vgic_free(struct domain *d) > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 1dd9683d25..2fef32eaee 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -149,6 +149,7 @@ struct arch_domain > #ifdef CONFIG_SBSA_VUART_CONSOLE > struct vpl011 vpl011; > #endif > + unsigned int max_vcpus; Now, you have max_vcpus defined in both arch_domain and domain. Which makes the code very confusing to read. This is becoming apparent in the check if (d->arch.max_vcpus > d->max_vcpus). If you plan to ditch the ops, then I would prefer a check on the vGIC version. Even if it means carrying vGIC specific implementation in generic Xen code. This would also avoid to grow the struct domain just for a "constant field" used mostly at guest creation. Cheers,
Hi, On 09/02/18 19:27, Julien Grall wrote: > Hi, > > On 02/09/2018 02:38 PM, Andre Przywara wrote: >> The VGIC model used for a domain (GICv2 or GICv3) determines the maximum >> number of VCPUs for that guest, as GICv2 can only handle 8 processors. >> In the moment we carry this per-VGIC-model limit in the vgic_ops, >> alongside the model specific functions. That makes some sense, but >> exposes some current VGIC implementation details to generic Xen code. >> Add a new arch specific field in our domain structure to hold this >> vcpu limit, >> and initialize it when we set the ops. This allows us to plug in the new >> VGIC later without also needing to carry some ops structure. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/domain.c | 5 ++--- >> xen/arch/arm/vgic.c | 3 ++- >> xen/include/asm-arm/domain.h | 1 + >> 3 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index a010443bfd..9ad4cd0a6e 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -975,11 +975,10 @@ unsigned int domain_max_vcpus(const struct >> domain *d) >> * allocation when the vgic_ops haven't been initialised yet, >> * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null. >> */ >> - if ( !d->arch.vgic.handler ) >> + if ( !d->arch.max_vcpus ) >> return MAX_VIRT_CPUS; >> else >> - return min_t(unsigned int, MAX_VIRT_CPUS, >> - d->arch.vgic.handler->max_vcpus); >> + return d->arch.max_vcpus; >> } >> /* >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 9921769b15..5f47aa84a9 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -166,7 +166,8 @@ int domain_vgic_init(struct domain *d, unsigned >> int nr_spis) >> void register_vgic_ops(struct domain *d, const struct vgic_ops *ops) >> { >> - d->arch.vgic.handler = ops; >> + d->arch.vgic.handler = ops; >> + d->arch.max_vcpus = ops->max_vcpus; >> } >> void domain_vgic_free(struct domain *d) >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 1dd9683d25..2fef32eaee 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -149,6 +149,7 @@ struct arch_domain >> #ifdef CONFIG_SBSA_VUART_CONSOLE >> struct vpl011 vpl011; >> #endif >> + unsigned int max_vcpus; > > Now, you have max_vcpus defined in both arch_domain and domain. Which > makes the code very confusing to read. This is becoming apparent in the > check if (d->arch.max_vcpus > d->max_vcpus). True, though I was just copying the name from the vgic_ops ;-) I could suggest arch.vgic_vcpu_limit instead, but... > If you plan to ditch the ops, then I would prefer a check on the vGIC > version. Even if it means carrying vGIC specific implementation in > generic Xen code. So what about just providing per-VGIC implementations of domain_max_vcpus()? That seems to be the only user of this information, AFAICS? So I move this from xen/arch/arm/domain.c to xen/arch/arm{/vgic,}/vgic.c, where we can do all kind of internal VGIC tricks to learn this bit of information. > This would also avoid to grow the struct domain just for a "constant > field" used mostly at guest creation. "Here’s a nickel, kid. Get yourself some memory." ;-) Cheers, Andre.
Hi, On 02/28/2018 12:32 PM, Andre Przywara wrote: > Hi, > > On 09/02/18 19:27, Julien Grall wrote: >> Hi, >> >> On 02/09/2018 02:38 PM, Andre Przywara wrote: >>> The VGIC model used for a domain (GICv2 or GICv3) determines the maximum >>> number of VCPUs for that guest, as GICv2 can only handle 8 processors. >>> In the moment we carry this per-VGIC-model limit in the vgic_ops, >>> alongside the model specific functions. That makes some sense, but >>> exposes some current VGIC implementation details to generic Xen code. >>> Add a new arch specific field in our domain structure to hold this >>> vcpu limit, >>> and initialize it when we set the ops. This allows us to plug in the new >>> VGIC later without also needing to carry some ops structure. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>> --- >>> xen/arch/arm/domain.c | 5 ++--- >>> xen/arch/arm/vgic.c | 3 ++- >>> xen/include/asm-arm/domain.h | 1 + >>> 3 files changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>> index a010443bfd..9ad4cd0a6e 100644 >>> --- a/xen/arch/arm/domain.c >>> +++ b/xen/arch/arm/domain.c >>> @@ -975,11 +975,10 @@ unsigned int domain_max_vcpus(const struct >>> domain *d) >>> * allocation when the vgic_ops haven't been initialised yet, >>> * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null. >>> */ >>> - if ( !d->arch.vgic.handler ) >>> + if ( !d->arch.max_vcpus ) >>> return MAX_VIRT_CPUS; >>> else >>> - return min_t(unsigned int, MAX_VIRT_CPUS, >>> - d->arch.vgic.handler->max_vcpus); >>> + return d->arch.max_vcpus; >>> } >>> /* >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>> index 9921769b15..5f47aa84a9 100644 >>> --- a/xen/arch/arm/vgic.c >>> +++ b/xen/arch/arm/vgic.c >>> @@ -166,7 +166,8 @@ int domain_vgic_init(struct domain *d, unsigned >>> int nr_spis) >>> void register_vgic_ops(struct domain *d, const struct vgic_ops *ops) >>> { >>> - d->arch.vgic.handler = ops; >>> + d->arch.vgic.handler = ops; >>> + d->arch.max_vcpus = ops->max_vcpus; >>> } >>> void domain_vgic_free(struct domain *d) >>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >>> index 1dd9683d25..2fef32eaee 100644 >>> --- a/xen/include/asm-arm/domain.h >>> +++ b/xen/include/asm-arm/domain.h >>> @@ -149,6 +149,7 @@ struct arch_domain >>> #ifdef CONFIG_SBSA_VUART_CONSOLE >>> struct vpl011 vpl011; >>> #endif >>> + unsigned int max_vcpus; >> >> Now, you have max_vcpus defined in both arch_domain and domain. Which >> makes the code very confusing to read. This is becoming apparent in the >> check if (d->arch.max_vcpus > d->max_vcpus). > > True, though I was just copying the name from the vgic_ops ;-) > I could suggest arch.vgic_vcpu_limit instead, but... > >> If you plan to ditch the ops, then I would prefer a check on the vGIC >> version. Even if it means carrying vGIC specific implementation in >> generic Xen code. > > So what about just providing per-VGIC implementations of > domain_max_vcpus()? That seems to be the only user of this information, > AFAICS? So I move this from xen/arch/arm/domain.c to > xen/arch/arm{/vgic,}/vgic.c, where we can do all kind of internal VGIC > tricks to learn this bit of information. Sounds good to me. Cheers,
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index a010443bfd..9ad4cd0a6e 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -975,11 +975,10 @@ unsigned int domain_max_vcpus(const struct domain *d) * allocation when the vgic_ops haven't been initialised yet, * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null. */ - if ( !d->arch.vgic.handler ) + if ( !d->arch.max_vcpus ) return MAX_VIRT_CPUS; else - return min_t(unsigned int, MAX_VIRT_CPUS, - d->arch.vgic.handler->max_vcpus); + return d->arch.max_vcpus; } /* diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 9921769b15..5f47aa84a9 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -166,7 +166,8 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis) void register_vgic_ops(struct domain *d, const struct vgic_ops *ops) { - d->arch.vgic.handler = ops; + d->arch.vgic.handler = ops; + d->arch.max_vcpus = ops->max_vcpus; } void domain_vgic_free(struct domain *d) diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 1dd9683d25..2fef32eaee 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -149,6 +149,7 @@ struct arch_domain #ifdef CONFIG_SBSA_VUART_CONSOLE struct vpl011 vpl011; #endif + unsigned int max_vcpus; } __cacheline_aligned;
The VGIC model used for a domain (GICv2 or GICv3) determines the maximum number of VCPUs for that guest, as GICv2 can only handle 8 processors. In the moment we carry this per-VGIC-model limit in the vgic_ops, alongside the model specific functions. That makes some sense, but exposes some current VGIC implementation details to generic Xen code. Add a new arch specific field in our domain structure to hold this vcpu limit, and initialize it when we set the ops. This allows us to plug in the new VGIC later without also needing to carry some ops structure. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/domain.c | 5 ++--- xen/arch/arm/vgic.c | 3 ++- xen/include/asm-arm/domain.h | 1 + 3 files changed, 5 insertions(+), 4 deletions(-)