| Message ID | 20180315203050.19791-41-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 03/15/2018 08:30 PM, Andre Przywara wrote: > diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c > index 002fec57e6..4b9664f313 100644 > --- a/xen/arch/arm/vgic/vgic.c > +++ b/xen/arch/arm/vgic/vgic.c > @@ -946,6 +946,28 @@ void vgic_sync_hardware_irq(struct domain *d, > spin_unlock_irqrestore(&desc->lock, flags); > } > > +unsigned int vgic_max_vcpus(const struct domain *d) > +{ > + unsigned int vgic_vcpu_limit; > + > + switch ( d->arch.vgic.version ) > + { > +#ifdef CONFIG_HAS_GICV3 > + case GIC_V3: > + vgic_vcpu_limit = VGIC_V3_MAX_CPUS; > + break; > +#endif It is a bit strange that you handle GICV3 here but don't in domain_vgic_register. > + case GIC_V2: > + vgic_vcpu_limit = VGIC_V2_MAX_CPUS; > + break; > + default: > + vgic_vcpu_limit = MAX_VIRT_CPUS; I feel this is a bit odd. We only support GICv2 and GICv3 and the enum has two values. Likely GCC will complain if CONFIG_HAS_GICV3 is set because default label is not used. Lastly, I can't see how you handle the corner case mentioned in the current vGIC: /* * Since evtchn_init would call domain_max_vcpus for poll_mask * allocation when the vgic_ops haven't been initialised yet, * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null. */ The comment in the code would also be very useful as the reason to call vgic_max_vcpus before the full initialization is not that straightforward. Cheers,
Hi, On 20/03/18 01:17, Julien Grall wrote: > Hi Andre, > > On 03/15/2018 08:30 PM, Andre Przywara wrote: >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >> index 002fec57e6..4b9664f313 100644 >> --- a/xen/arch/arm/vgic/vgic.c >> +++ b/xen/arch/arm/vgic/vgic.c >> @@ -946,6 +946,28 @@ void vgic_sync_hardware_irq(struct domain *d, >> spin_unlock_irqrestore(&desc->lock, flags); >> } >> +unsigned int vgic_max_vcpus(const struct domain *d) >> +{ >> + unsigned int vgic_vcpu_limit; >> + >> + switch ( d->arch.vgic.version ) >> + { >> +#ifdef CONFIG_HAS_GICV3 >> + case GIC_V3: >> + vgic_vcpu_limit = VGIC_V3_MAX_CPUS; >> + break; >> +#endif > > It is a bit strange that you handle GICV3 here but don't in > domain_vgic_register. Fair enough. >> + case GIC_V2: >> + vgic_vcpu_limit = VGIC_V2_MAX_CPUS; >> + break; >> + default: >> + vgic_vcpu_limit = MAX_VIRT_CPUS; > > I feel this is a bit odd. We only support GICv2 and GICv3 and the enum > has two values. Likely GCC will complain if CONFIG_HAS_GICV3 is set > because default label is not used. AFAICT (and have tested) at least my GCC never complains about "default:", even if every enum member has already been used. I think it's good style to catch those cases should the enum get extended for some reason. Plus we have this already in arch_domain_create() (in switch get_hw_version()). > Lastly, I can't see how you handle the corner case mentioned in the > current vGIC: > > /* > * Since evtchn_init would call domain_max_vcpus for poll_mask > * allocation when the vgic_ops haven't been initialised yet, > * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null. > */ Do we need this still with Andrew's latest patch? > The comment in the code would also be very useful as the reason to call > vgic_max_vcpus before the full initialization is not that straightforward. Otherwise this smells like we need to have enum gic_version extended, to reserve the 0 case? enum gic_version {GIC_INVALID, GIC_V2, GIC_V3};? That should cover the not-yet-initialised case, shouldn't it? Cheers, Andre.
On 03/20/2018 05:11 PM, Andre Przywara wrote: > Hi, > > On 20/03/18 01:17, Julien Grall wrote: >> Hi Andre, >> >> On 03/15/2018 08:30 PM, Andre Przywara wrote: >>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>> index 002fec57e6..4b9664f313 100644 >>> --- a/xen/arch/arm/vgic/vgic.c >>> +++ b/xen/arch/arm/vgic/vgic.c >>> @@ -946,6 +946,28 @@ void vgic_sync_hardware_irq(struct domain *d, >>> spin_unlock_irqrestore(&desc->lock, flags); >>> } >>> +unsigned int vgic_max_vcpus(const struct domain *d) >>> +{ >>> + unsigned int vgic_vcpu_limit; >>> + >>> + switch ( d->arch.vgic.version ) >>> + { >>> +#ifdef CONFIG_HAS_GICV3 >>> + case GIC_V3: >>> + vgic_vcpu_limit = VGIC_V3_MAX_CPUS; >>> + break; >>> +#endif >> >> It is a bit strange that you handle GICV3 here but don't in >> domain_vgic_register. > > Fair enough. > >>> + case GIC_V2: >>> + vgic_vcpu_limit = VGIC_V2_MAX_CPUS; >>> + break; >>> + default: >>> + vgic_vcpu_limit = MAX_VIRT_CPUS; >> >> I feel this is a bit odd. We only support GICv2 and GICv3 and the enum >> has two values. Likely GCC will complain if CONFIG_HAS_GICV3 is set >> because default label is not used. > > AFAICT (and have tested) at least my GCC never complains about > "default:", even if every enum member has already been used. I think > it's good style to catch those cases should the enum get extended for > some reason. Well, the compiler will usually tell you if you miss one case. In that circumstance you put a value that may not make sense for that new item in the enum and you will have some trouble to catch it. So the default should really be a BUG() or ASSERT_UNREACHABLE(). > Plus we have this already in arch_domain_create() (in switch > get_hw_version()). See my point above. > >> Lastly, I can't see how you handle the corner case mentioned in the >> current vGIC: >> >> /* >> * Since evtchn_init would call domain_max_vcpus for poll_mask >> * allocation when the vgic_ops haven't been initialised yet, >> * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null. >> */ > > Do we need this still with Andrew's latest patch? Well depends which series is going first. If it is yours, then you still need it and Andrew has to fix it. > >> The comment in the code would also be very useful as the reason to call >> vgic_max_vcpus before the full initialization is not that straightforward. > > Otherwise this smells like we need to have enum gic_version extended, to > reserve the 0 case? enum gic_version {GIC_INVALID, GIC_V2, GIC_V3};? > > That should cover the not-yet-initialised case, shouldn't it? I think so. But see above. Cheers,
diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c new file mode 100644 index 0000000000..d091c92ed0 --- /dev/null +++ b/xen/arch/arm/vgic/vgic-init.c @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2015, 2016 ARM Ltd. + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <xen/sched.h> +#include <asm/new_vgic.h> + +#include "vgic.h" + +/* CREATION */ + +/** + * domain_vgic_register: create a virtual GIC + * @d: domain pointer + * @mmio_count: pointer to add number of required MMIO regions + * + * was: kvm_vgic_create + */ +int domain_vgic_register(struct domain *d, int *mmio_count) +{ + switch ( d->arch.vgic.version ) + { + case GIC_V2: + *mmio_count = 1; + break; + default: + BUG(); + } + + if ( d->max_vcpus > domain_max_vcpus(d) ) + return -E2BIG; + + d->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF; + d->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF; + d->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF; + + return 0; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c index 002fec57e6..4b9664f313 100644 --- a/xen/arch/arm/vgic/vgic.c +++ b/xen/arch/arm/vgic/vgic.c @@ -946,6 +946,28 @@ void vgic_sync_hardware_irq(struct domain *d, spin_unlock_irqrestore(&desc->lock, flags); } +unsigned int vgic_max_vcpus(const struct domain *d) +{ + unsigned int vgic_vcpu_limit; + + switch ( d->arch.vgic.version ) + { +#ifdef CONFIG_HAS_GICV3 + case GIC_V3: + vgic_vcpu_limit = VGIC_V3_MAX_CPUS; + break; +#endif + case GIC_V2: + vgic_vcpu_limit = VGIC_V2_MAX_CPUS; + break; + default: + vgic_vcpu_limit = MAX_VIRT_CPUS; + break; + } + + return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit); +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h index 2feed9615f..deefbb3ef7 100644 --- a/xen/arch/arm/vgic/vgic.h +++ b/xen/arch/arm/vgic/vgic.h @@ -21,6 +21,9 @@ #define VARIANT_ID_XEN 0x01 #define IMPLEMENTER_ARM 0x43b +#define VGIC_ADDR_UNDEF INVALID_PADDR +#define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF) + #define VGIC_PRI_BITS 5 #define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS)
This patch implements the function which is called by Xen when it wants to register the virtual GIC. This also implements vgic_max_vcpus() for the new VGIC, which reports back the maximum number of VCPUs a certain GIC model supports. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/vgic/vgic-init.c | 60 +++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vgic/vgic.c | 22 ++++++++++++++++ xen/arch/arm/vgic/vgic.h | 3 +++ 3 files changed, 85 insertions(+) create mode 100644 xen/arch/arm/vgic/vgic-init.c