Re: [Xen-devel] [RFC PATCH 44/49] ARM: new VGIC: vgic-init: register VGIC

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 19/02/18 12:39, Julien Grall wrote:
> Hi Andre,
> 
> On 09/02/18 14:39, Andre Przywara wrote:
>> This patch implements the function which is called by Xen when it wants
>> to register the virtual GIC.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/vgic/vgic-init.c | 62
>> +++++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/arm/vgic/vgic.h      |  3 +++
>>   2 files changed, 65 insertions(+)
>>   create mode 100644 xen/arch/arm/vgic/vgic-init.c
>>
>> diff --git a/xen/arch/arm/vgic/vgic-init.c
>> b/xen/arch/arm/vgic/vgic-init.c
>> new file mode 100644
>> index 0000000000..b5f1183a50
>> --- /dev/null
>> +++ b/xen/arch/arm/vgic/vgic-init.c
>> @@ -0,0 +1,62 @@
>> +/*
>> + * Copyright (C) 2015, 2016 ARM Ltd.
>> + *
>> + * 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/arm_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)
> 
> mmio_count should be set to the number of I/O region you will register.
> 
>> +{
>> +    switch ( d->arch.vgic.version )
>> +    {
>> +#ifdef CONFIG_HAS_GICV3
>> +    case GIC_V3:
>> +        d->arch.max_vcpus = VGIC_V3_MAX_CPUS;
>> +        break;
>> +#endif
>> +    case GIC_V2:
>> +        d->arch.max_vcpus = VGIC_V2_MAX_CPUS;
>> +        break;
>> +    }
>> +
>> +    if ( d->max_vcpus > d->arch.max_vcpus )
>> +        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;
> 
> Is there any reason to store an address rather than a frame? The latter
> would add a be more safety.

Possibly, but the existing VGIC doesn't do it as well, which means that
at the moment we deal with addresses (and not frame numbers) everywhere
(for instance GUEST_GICD_BASE).
Changing this for the distributor internally already (as requested for
some other patch) caused more changes than I liked, and at the moment I
am trying to make the TODO list smaller, not bigger ;-)

Cheers,
Andre.

>> +
>> +    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.h b/xen/arch/arm/vgic/vgic.h
>> index b104f8e964..205ce10ffa 100644
>> --- a/xen/arch/arm/vgic/vgic.h
>> +++ b/xen/arch/arm/vgic/vgic.h
>> @@ -20,6 +20,9 @@
>>   #define PRODUCT_ID_KVM      0x4b    /* ASCII code K */
>>   #define IMPLEMENTER_ARM     0x43b
>>   +#define VGIC_ADDR_UNDEF     (-1)
> 
> Please use INVALID_PADDR here.
> 
>> +#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)
>>
> 
> Cheers,
> 

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