Re: [Xen-devel] [PATCH 27/57] ARM: new VGIC: Add data structure definitions

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 03/06/2018 06:01 PM, Andre Przywara wrote:
> On 06/03/18 17:46, Julien Grall wrote:
> > On 05/03/18 16:03, Andre Przywara wrote:
> > > Add a new header file for the new and improved GIC implementation.
> > > The big change is that we now have a struct vgic_irq per IRQ instead
> > > of spreading all the information over various bitmaps in the ranks.
> > >
> > > We include this new header conditionally from within the old header
> > > file for the time being to avoid touching all the users.
> > >
> > > This is based on Linux commit b18b57787f5e, written by Christoffer Dall.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> > > ---
> > > Changelog RFC ... v1:
> > > - rename header file to new_vgic.h
> > > - drop unneeded data structures (vgic_its, vgic_v<x>_cpu_if)
> > > - reorder members in vgic_irq to avoid padding
> > > - move flags members into bool bitfields
> > > - drop prototypes
> > > - use unsigned and uint<x>_t data types
> > > - keep arch_vcpu member name as "vgic"
> > >
> > >    xen/include/asm-arm/new_vgic.h | 198
> > > +++++++++++++++++++++++++++++++++++++++++
> > >    xen/include/asm-arm/vgic.h     |   6 ++
> > >    2 files changed, 204 insertions(+)
> > >    create mode 100644 xen/include/asm-arm/new_vgic.h
> > >
> > > diff --git a/xen/include/asm-arm/new_vgic.h
> > > b/xen/include/asm-arm/new_vgic.h
> > > new file mode 100644
> > > index 0000000000..54be5aa3eb
> > > --- /dev/null
> > > +++ b/xen/include/asm-arm/new_vgic.h
> > > @@ -0,0 +1,198 @@
> > > +/*
> > > + * 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/>.
> > > + */
> > > +#ifndef __ASM_ARM_NEW_VGIC_H
> > > +#define __ASM_ARM_NEW_VGIC_H
> > > +
> > > +#include <asm/atomic.h>
> > > +#include <asm/mmio.h>
> > > +#include <xen/list.h>
> > > +#include <xen/mm.h>
> > > +#include <xen/spinlock.h>
> > > +
> > > +#define VGIC_V3_MAX_CPUS        255
> > > +#define VGIC_V2_MAX_CPUS        8
> > > +#define VGIC_NR_SGIS            16
> > > +#define VGIC_NR_PPIS            16
> > > +#define VGIC_NR_PRIVATE_IRQS    (VGIC_NR_SGIS + VGIC_NR_PPIS)
> > > +#define VGIC_MAX_PRIVATE        (VGIC_NR_PRIVATE_IRQS - 1)
> > > +#define VGIC_MAX_SPI            1019
> > > +#define VGIC_MAX_RESERVED       1023
> > > +#define VGIC_MIN_LPI            8192
> > > +
> > > +#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) <
> > > VGIC_NR_PRIVATE_IRQS)
> > > +#define irq_is_spi(irq) ((irq) >= VGIC_NR_PRIVATE_IRQS && \
> > > +                         (irq) <= VGIC_MAX_SPI)
> > > +
> > > +enum vgic_type {
> > > +    VGIC_V2,        /* Good ol' GICv2 */
> > > +    VGIC_V3,        /* New fancy GICv3 */
> > > +};
> > > +
> > > +#define VGIC_V2_MAX_LRS         (1 << 6)
> > > +#define VGIC_V3_MAX_LRS         16
> > > +#define VGIC_V3_LR_INDEX(lr)    (VGIC_V3_MAX_LRS - 1 - lr)
> > > +
> > > +enum vgic_irq_config {
> > > +    VGIC_CONFIG_EDGE = 0,
> > Again, I don't think it is necessary to set to 0 here as IIRC an enum
> > always start at 0 if not specified.
> But I consider this more readable, especially since this is a bool now.
> So it's immediately obvious that EDGE is 0.
Fair enough, I probably know to much the C spec :).

> Shall I replace this with:
> #define VGIC_CONFIG_EDGE false
> #define VGIC_CONFIG_LEVEL true
> now? Or using 0 and 1?
>
> The enum vgic_irq_config type is now not used anywhere.
I don't mind to keep the enum with a comment above. If you switch to 
define, then I would prefer to use false/true.
Cheers,

--
Julien Grall

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