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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 26/02/18 16:02, Julien Grall wrote:
> Hi Andre,
> 
> On 02/26/2018 03:13 PM, Andre Przywara wrote:
>> Hi,
>>
>> 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
>>>> @@ -0,0 +1,261 @@
>>>> +/*
>>>> + * Copyright (C) 2015, 2016 ARM Ltd.
>>>> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
>>>> + *
>>>> + * 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 <asm/arm_vgic.h>
>>>> +#include <asm/bug.h>
>>>> +#include <asm/io.h>
>>>> +#include <xen/sched.h>
>>>> +#include <xen/sizes.h>
>>>> +
>>>> +#include "vgic.h"
>>>> +
>>>> +#define GICH_ELRSR0                     0x30
>>>> +#define GICH_ELRSR1                     0x34
>>>> +#define GICH_LR0                        0x100
>>>> +
>>>> +#define GICH_LR_VIRTUALID               (0x3ff << 0)
>>>> +#define GICH_LR_PHYSID_CPUID_SHIFT      (10)
>>>> +#define GICH_LR_PHYSID_CPUID            (0x3ff <<
>>>> GICH_LR_PHYSID_CPUID_SHIFT)
>>>> +#define GICH_LR_PRIORITY_SHIFT          23
>>>> +#define GICH_LR_STATE                   (3 << 28)
>>>> +#define GICH_LR_PENDING_BIT             (1 << 28)
>>>> +#define GICH_LR_ACTIVE_BIT              (1 << 29)
>>>> +#define GICH_LR_EOI                     (1 << 19)
>>>> +#define GICH_LR_HW                      (1 << 31)
>>>
>>> Can we define them in either in gic.h or a new header gic-v2.h?
>>
>> Yes, but they clash with some ill-named GICv3 LR bits. So expect another
>> patch which renames GICH_LR_STATE_SHIFT to ICH_LR_STATE_SHIFT. Which is
>> the actual spec name for that system register in GICv3, there is no
>> GICH_LR_ with the GICv3 bit positions.
> 
> While this would be a nice clean-up. Wouldn't create a new gic-v2.h
> sufficient?

I don't think that would be right. We actually already have some GICH_
definitions in xen/include/asm-arm/gic.h, so just adding the missing
ones there sounds natural. I now remember that I just didn't do this
initially because of the clash and and at this time I just wanted to
make it compile ;-)

And since assigning GICH_ names to GICv3 ICH_ register bits sounds wrong
in the first place, I consider this a good opportunity to fix this.

Cheers,
Andre.

> 
>>
>>
>>>> +
>>>> +static struct {
>>>> +    bool enabled;
>>>> +    paddr_t dbase;          /* Distributor interface address */
>>>> +    paddr_t cbase;          /* CPU interface address & size */
>>>> +    paddr_t csize;
>>>> +    paddr_t vbase;          /* Virtual CPU interface address */
>>>> +    void __iomem *hbase;        /* Hypervisor control interface */
>>>> +
>>>> +    /* Offset to add to get an 8kB contiguous region if GIC is
>>>> aliased */
>>>> +    uint32_t aliased_offset;
>>>> +} gic_v2_hw_data;
>>>> +
>>>> +void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
>>>> +              paddr_t vbase, void __iomem *hbase,
>>>> +              uint32_t aliased_offset)
>>>> +{
>>>> +    gic_v2_hw_data.enabled = true;
>>>> +    gic_v2_hw_data.dbase = dbase;
>>>> +    gic_v2_hw_data.cbase = cbase;
>>>> +    gic_v2_hw_data.csize = csize;
>>>> +    gic_v2_hw_data.vbase = vbase;
>>>> +    gic_v2_hw_data.hbase = hbase;
>>>> +    gic_v2_hw_data.aliased_offset = aliased_offset;
>>>> +}
>>>> +
>>>> +void vgic_v2_set_underflow(struct vcpu *vcpu)
>>>> +{
>>>> +    gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
>>>> +}
>>>> +
>>>> +/*
>>>> + * transfer the content of the LRs back into the corresponding
>>>> ap_list:
>>>> + * - active bit is transferred as is
>>>> + * - pending bit is
>>>> + *   - transferred as is in case of edge sensitive IRQs
>>>> + *   - set to the line-level (resample time) for level sensitive IRQs
>>>> + */
>>>> +void vgic_v2_fold_lr_state(struct vcpu *vcpu)
>>>
>>> I am wondering how much we could share this code with
>>> vgic_v3_fold_lr_state.
>>
>> I think we discussed this and dismissed the idea:
>> - The actual LR encoding is much different between GICv3 and GICv2, up
>> to the point where we have some fields in one which are not in the
>> other. That really clutters the code.
>> - Originally this function was much shorter and didn't have that many
>> special cases. So the code duplication was really minimal.
>>
>> I see your point, but don't really want to go there now for two reasons:
>> - It is probably nasty to implement, since we always have to check which
>> GIC we are running on when masking the LR value.
>> - It would deviate further from the KVM implementation, in a core
>> function. For any bugs introduced we are on our own here.
>>
>> I will try to bring this up with the KVM people, to see whether it's
>> worth to revisit this decision. There is indeed quite some code
>> duplication these days.
>> But this may come as an optimization later.
> 
> Fine with me. It was mostly to avoid having to review twice the same
> hairy code.
> 
> Cheers,
> 

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