Re: [Xen-devel] [RFC PATCH 25/49] ARM: new VGIC: Add GICv2 world switch backend

[Julien Grall <julien.grall@xxxxxxx>]

On 02/26/2018 03:16 PM, Andre Przywara wrote:
> Hi,
Hi,

> forgot to mention:
>
> On 13/02/18 14:31, Julien Grall wrote:
> > Hi,
> >
> > On 09/02/18 14:39, Andre Przywara wrote:
> > > Processing maintenance interrupts and accessing the list registers
> > > are dependent on the host's GIC version.
> > > Introduce vgic-v2.c to contain GICv2 specific functions.
> > > Implement the GICv2 specific code for syncing the emulation state
> > > into the VGIC registers.
> > > This also adds the hook to let Xen setup the host GIC addresses.
> > >
> > > This is based on Linux commit 140b086dd197, written by Marc Zyngier.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> > > ---
> > >    xen/arch/arm/vgic/vgic-v2.c | 261
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >    xen/arch/arm/vgic/vgic.c    |  20 ++++
> > >    xen/arch/arm/vgic/vgic.h    |   8 ++
> > >    3 files changed, 289 insertions(+)
> > >    create mode 100644 xen/arch/arm/vgic/vgic-v2.c
> > >
> > > diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> > > new file mode 100644
> > > index 0000000000..10fc467ffa
> > > --- /dev/null
> > > +++ b/xen/arch/arm/vgic/vgic-v2.c
> ....
>
> > > +void vgic_v2_save_state(struct vcpu *vcpu)
> > > +{
> > > +    u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs;
> > > +
> > > +    if ( used_lrs )
> > > +    {
> > > +        save_lrs(vcpu, gic_v2_hw_data.hbase);
> > > +        writel_relaxed(0, gic_v2_hw_data.hbase + GICH_HCR);
> > > +    }
> > > +}
> > I am not entirely convinced that have a separate function to save the
> > LRs is necessary. This could be done in fold_lr_state().
> >
> > > +
> > > +void vgic_v2_restore_state(struct vcpu *vcpu)
> > > +{
> > > +    struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;,
> > > +    u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs;
> > > +    int i;
> > > +
> > > +    if ( used_lrs )
> > > +    {
> > > +        writel_relaxed(cpu_if->vgic_hcr,
> > > +                       gic_v2_hw_data.hbase + GICH_HCR);
> > > +        for ( i = 0; i < used_lrs; i++ )
> > > +            writel_relaxed(cpu_if->vgic_lr[i],
> > > +                           gic_v2_hw_data.hbase + GICH_LR0 + (i * 4));
> > > +    }
> > Same here but with populate_lr_state(). This would make the code easier
> > to follow and also avoid a lot ifery in the vgic.c code.
> This is mostly due to KVM's inability to directly access the GICv3 LRs
> when running in EL1. I will take a look whether what it would take to
> merge this. Sounds tempting, but there might be side effects.
I am not sure what would be the side effects. You basically
call save_state and right after fold_lr_state. This would streamline a 
bit more the code.
Cheers,

--
Julien Grall

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