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

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

Hi,

On 26/02/18 15:59, Julien Grall wrote:
> 
> 
> 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@linaro.org>
>>>> ---
>>>>    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.

The possible side effects are that we actually now have a shadow copy of
the LRs in our data structures. I have the gut feeling we don't need
this in Xen, but need to check more thoroughly.

Cheers,
Andre.

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