Re: [PATCH 07/12] ARM: VGIC: split gic.c to observe hardware/virtual GIC separation

[Andre Przywara <andre.przywara@linaro.org>]

Hi,

On 26/10/17 01:37, Stefano Stabellini wrote:
> On Thu, 19 Oct 2017, Andre Przywara wrote:
>> Currently gic.c holds code to handle hardware IRQs as well as code to
>> bridge VGIC requests to the GIC virtualization hardware.
> 
> That is true, however, I don't necessarely see "the code to bridge VGIC
> requests to the GIC virtualization hardware" as belonging to the VGIC. I
> think is a good abstraction to place in the GIC. That said, see below.
> 
> 
>> Despite being named gic.c, this file reaches into the VGIC and uses data
>> structures describing virtual IRQs.
>> To improve abstraction, move the VGIC functions into a separate file,
>> so that gic.c does what is says on the tin.
> 
> Splitting "the code to bridge VGIC requests to the GIC virtualization
> hardware" is harmless, so I am OK with this patch.
> 
> One cosmetic comment below.
> 
> 
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  xen/arch/arm/Makefile   |   1 +
>>  xen/arch/arm/gic-vgic.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/gic.c      | 348 +-----------------------------------------
>>  3 files changed, 398 insertions(+), 346 deletions(-)
>>  create mode 100644 xen/arch/arm/gic-vgic.c
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 30a2a6500a..41d7366527 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -16,6 +16,7 @@ obj-y += domain_build.o
>>  obj-y += domctl.o
>>  obj-$(EARLY_PRINTK) += early_printk.o
>>  obj-y += gic.o
>> +obj-y += gic-vgic.o
>>  obj-y += gic-v2.o
>>  obj-$(CONFIG_HAS_GICV3) += gic-v3.o
>>  obj-$(CONFIG_HAS_ITS) += gic-v3-its.o
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> new file mode 100644
>> index 0000000000..66cae21e82
>> --- /dev/null
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -0,0 +1,395 @@
>> +/*
>> + * xen/arch/arm/gic-vgic.c
>> + *
>> + * ARM Generic Interrupt Controller virtualization support
>> + *
>> + * Tim Deegan <tim@xen.org>
>> + * Copyright (c) 2011 Citrix Systems.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * 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.
>> + */
>> +
>> +#include <xen/lib.h>
>> +#include <xen/init.h>
>> +#include <xen/mm.h>
>> +#include <xen/irq.h>
>> +#include <xen/sched.h>
>> +#include <xen/errno.h>
>> +#include <xen/softirq.h>
>> +#include <xen/list.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/acpi.h>
>> +#include <asm/p2m.h>
>> +#include <asm/domain.h>
>> +#include <asm/platform.h>
>> +#include <asm/device.h>
>> +#include <asm/io.h>
>> +#include <asm/gic.h>
>> +#include <asm/vgic.h>
>> +#include <asm/acpi.h>
>> +
>> +extern uint64_t per_cpu__lr_mask;
> 
> This is a bit ugly. Would the macro "get_per_cpu_var" help?

So this could become:
extern uint64_t get_per_cpu_var(lr_mask);
though I am not sure if this is really nicer?

As you found out below yourself, this lr_mask is a bit nasty, as it's
used in both gic-vgic.c and gic.c. I don't think there is much we can do
about it, thus just sharing this variable was by far the easiest and
simplest solution.

>> +extern const struct gic_hw_operations *gic_hw_ops;
>> +
>> +#define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) - 1))
>> +

...


>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 58d69955fb..04e6d66b69 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -36,15 +36,11 @@
>>  #include <asm/vgic.h>
>>  #include <asm/acpi.h>
>>  
>> -static DEFINE_PER_CPU(uint64_t, lr_mask);
>> -
>> -#define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) - 1))
>> +DEFINE_PER_CPU(uint64_t, lr_mask);
> 
> I didn't look at what's left in gic.c, but would it be possible to move
> the definition of lr_mask to gic-vgic.c?

As mentioned above: not really, we need it in both files, unfortunately.

As in the other mail, I guess it's not worth trying to properly fix this
now, unless you have a really good reason.

Cheers,
Andre.

>>  #undef GIC_DEBUG
>>  
>> -static void gic_update_one_lr(struct vcpu *v, int i);
>> -
>> -static const struct gic_hw_operations *gic_hw_ops;
>> +const struct gic_hw_operations *gic_hw_ops;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel