On 22/06/16 16:03, Andre Przywara wrote:
> Hi,
>
> On 22/06/16 15:48, Marc Zyngier wrote:
>> On 17/06/16 13:08, Andre Przywara wrote:
>>> The ARM GICv3 ITS emulation code goes into a separate file, but needs
>>> to be connected to the GICv3 emulation, of which it is an option.
>>> The ITS MMIO handlers require the respective ITS pointer to be passed in,
>>> so we amend the existing VGIC MMIO framework to let it cope with that.
>>> Also we introduce the basic ITS data structure and initialize it, but
>>> don't return any success yet, as we are not yet ready for the show.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>> ---
>>> arch/arm64/kvm/Makefile | 1 +
>>> include/kvm/vgic/vgic.h | 13 ++++-
>>> include/linux/irqchip/arm-gic-v3.h | 1 +
>>> virt/kvm/arm/vgic/vgic-its.c | 107 +++++++++++++++++++++++++++++++++++++
>>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 38 ++++++++++++-
>>> virt/kvm/arm/vgic/vgic-mmio.c | 25 ++++++---
>>> virt/kvm/arm/vgic/vgic-mmio.h | 4 ++
>>> virt/kvm/arm/vgic/vgic.h | 7 +++
>>> 8 files changed, 187 insertions(+), 9 deletions(-)
>>> create mode 100644 virt/kvm/arm/vgic/vgic-its.c
>>>
>>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>>> index a7a958c..2755d17 100644
>>> --- a/arch/arm64/kvm/Makefile
>>> +++ b/arch/arm64/kvm/Makefile
>>> @@ -30,6 +30,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v2.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>>> else
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic.o
>>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic-v2.o
>>> diff --git a/include/kvm/vgic/vgic.h b/include/kvm/vgic/vgic.h
>>> index dc7f2fd..a66ba9a 100644
>>> --- a/include/kvm/vgic/vgic.h
>>> +++ b/include/kvm/vgic/vgic.h
>>> @@ -111,12 +111,23 @@ struct vgic_register_region;
>>>
>>> struct vgic_io_device {
>>> gpa_t base_addr;
>>> - struct kvm_vcpu *redist_vcpu;
>>> + union {
>>> + struct kvm_vcpu *redist_vcpu;
>>> + struct vgic_its *its;
>>> + };
>>> const struct vgic_register_region *regions;
>>> int nr_regions;
>>> struct kvm_io_device dev;
>>> };
>>>
>>> +struct vgic_its {
>>> + /* The base address of the ITS control register frame */
>>> + gpa_t vgic_its_base;
>>> +
>>> + bool enabled;
>>> + struct vgic_io_device iodev;
>>> +};
>>
>> You may want to move this definition before struct vgic_io_device, some
>> compilers are more picky than some others.
>
> Mmh, I just see that this is actually circular here.
Ah, indeed.
> So shall I add an opaque declaration for struct vgic_its above?
Yes please.
>>
>>> +
>>> struct vgic_dist {
>>> bool in_kernel;
>>> bool ready;
>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>>> index 64e8c70..92d5da8 100644
>>> --- a/include/linux/irqchip/arm-gic-v3.h
>>> +++ b/include/linux/irqchip/arm-gic-v3.h
>>> @@ -176,6 +176,7 @@
>>> #define GITS_CWRITER 0x0088
>>> #define GITS_CREADR 0x0090
>>> #define GITS_BASER 0x0100
>>> +#define GITS_IDREGS_BASE 0xffd0
>>
>> Please make all the changes to the core GIC code in a separate patch,
>> which I can pick independently.
>
> Sure.
>
>>
>>> #define GITS_PIDR2 GICR_PIDR2
>>>
>>> #define GITS_TRANSLATER 0x10040
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>> new file mode 100644
>>> index 0000000..c02d9df
>>> --- /dev/null
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -0,0 +1,107 @@
>>> +/*
>>> + * GICv3 ITS emulation
>>> + *
>>> + * Copyright (C) 2015,2016 ARM Ltd.
>>> + * Author: Andre Przywara <andre.przywara@xxxxxxx>
>>> + *
>>> + * 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 <linux/cpu.h>
>>> +#include <linux/kvm.h>
>>> +#include <linux/kvm_host.h>
>>> +#include <linux/interrupt.h>
>>> +
>>> +#include <linux/irqchip/arm-gic-v3.h>
>>> +
>>> +#include <asm/kvm_emulate.h>
>>> +#include <asm/kvm_arm.h>
>>> +#include <asm/kvm_mmu.h>
>>> +
>>> +#include "vgic.h"
>>> +#include "vgic-mmio.h"
>>> +
>>> +#define REGISTER_ITS_DESC(off, rd, wr, length, acc) \
>>> +{ \
>>> + .reg_offset = off, \
>>> + .len = length, \
>>> + .access_flags = acc, \
>>> + .read = NULL, \
>>> + .write = NULL, \
>>> + .its_read = rd, \
>>> + .its_write = wr, \
>>
>> Meh. That's not very nice... Why can't they be a union?
>
> We can, but we would need another differentiator then (as you suggested
> below).
>
>>> +}
>>> +
>>> +static unsigned long its_mmio_read_raz(struct kvm *kvm, struct vgic_its *its,
>>> + gpa_t addr, unsigned int len)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>>> + gpa_t addr, unsigned int len, unsigned long val)
>>> +{
>>> + /* Ignore */
>>> +}
>>> +
>>> +struct vgic_register_region its_registers[] = {
>>
>> Shouldn't this be static?
>
> Indeed, looks like a rebase artefact (I had the registration in another
> file during some rework attempts).
>
>>
>>> + REGISTER_ITS_DESC(GITS_CTLR,
>>> + its_mmio_read_raz, its_mmio_write_wi, 4,
>>> + VGIC_ACCESS_32bit),
>>> + REGISTER_ITS_DESC(GITS_IIDR,
>>> + its_mmio_read_raz, its_mmio_write_wi, 4,
>>> + VGIC_ACCESS_32bit),
>>> + REGISTER_ITS_DESC(GITS_TYPER,
>>> + its_mmio_read_raz, its_mmio_write_wi, 8,
>>> + VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>> + REGISTER_ITS_DESC(GITS_CBASER,
>>> + its_mmio_read_raz, its_mmio_write_wi, 8,
>>> + VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>> + REGISTER_ITS_DESC(GITS_CWRITER,
>>> + its_mmio_read_raz, its_mmio_write_wi, 8,
>>> + VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>> + REGISTER_ITS_DESC(GITS_CREADR,
>>> + its_mmio_read_raz, its_mmio_write_wi, 8,
>>> + VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>> + REGISTER_ITS_DESC(GITS_BASER,
>>> + its_mmio_read_raz, its_mmio_write_wi, 0x40,
>>> + VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>> + REGISTER_ITS_DESC(GITS_IDREGS_BASE,
>>> + its_mmio_read_raz, its_mmio_write_wi, 0x30,
>>> + VGIC_ACCESS_32bit),
>>> +};
>>> +
>>> +int vits_register(struct kvm *kvm, struct vgic_its *its)
>>> +{
>>> + struct vgic_io_device *iodev = &its->iodev;
>>> + int ret;
>>> +
>>> + iodev->regions = its_registers;
>>> + iodev->nr_regions = ARRAY_SIZE(its_registers);
>>> + kvm_iodevice_init(&iodev->dev, &kvm_io_gic_ops);
>>> +
>>> + iodev->base_addr = its->vgic_its_base;
>>> + iodev->its = its;
>>> + mutex_lock(&kvm->slots_lock);
>>> + ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr,
>>> + SZ_64K, &iodev->dev);
>>> + mutex_unlock(&kvm->slots_lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +void vits_destroy(struct kvm *kvm, struct vgic_its *its)
>>> +{
>>> +
>>> + its->enabled = false;
>>> +}
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 8cd7190..4eee0d7 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> @@ -42,6 +42,16 @@ static u64 update_64bit_reg(u64 reg, unsigned int offset, unsigned int len,
>>> return reg | ((u64)val << lower);
>>> }
>>>
>>> +bool vgic_has_its(struct kvm *kvm)
>>> +{
>>> + struct vgic_dist *dist = &kvm->arch.vgic;
>>> +
>>> + if (dist->vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
>>> + return false;
>>> +
>>> + return false;
>>> +}
>>> +
>>> static unsigned long vgic_mmio_read_v3_misc(struct kvm_vcpu *vcpu,
>>> gpa_t addr, unsigned int len)
>>> {
>>> @@ -126,6 +136,32 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,
>>> vgic_put_irq(irq);
>>> }
>>>
>>> +static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
>>> + gpa_t addr, unsigned int len)
>>> +{
>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> +
>>> + return vgic_cpu->lpis_enabled ? GICR_CTLR_ENABLE_LPIS : 0;
>>> +}
>>> +
>>> +
>>> +static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
>>> + gpa_t addr, unsigned int len,
>>> + unsigned long val)
>>> +{
>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> + bool was_enabled = vgic_cpu->lpis_enabled;
>>> +
>>> + if (!vgic_has_its(vcpu->kvm))
>>> + return;
>>> +
>>> + vgic_cpu->lpis_enabled = val & GICR_CTLR_ENABLE_LPIS;
>>> +
>>> + if (!was_enabled && vgic_cpu->lpis_enabled) {
>>> + /* Eventually do something */
>>> + }
>>> +}
>>> +
>>> static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>>> gpa_t addr, unsigned int len)
>>> {
>>> @@ -325,7 +361,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
>>>
>>> static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
>>> REGISTER_DESC_WITH_LENGTH(GICR_CTLR,
>>> - vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
>>> + vgic_mmio_read_v3r_ctlr, vgic_mmio_write_v3r_ctlr, 4,
>>> VGIC_ACCESS_32bit),
>>> REGISTER_DESC_WITH_LENGTH(GICR_IIDR,
>>> vgic_mmio_read_v3r_iidr, vgic_mmio_write_wi, 4,
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>> index 4050c1c..c98a4fd 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>> @@ -461,8 +461,7 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>>> {
>>> struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
>>> const struct vgic_register_region *region;
>>> - struct kvm_vcpu *r_vcpu;
>>> - unsigned long data;
>>> + unsigned long data = 0;
>>>
>>> region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
>>> addr - iodev->base_addr);
>>> @@ -471,8 +470,15 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>>> return 0;
>>> }
>>>
>>> - r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
>>> - data = region->read(r_vcpu, addr, len);
>>> + if (region->read) {
>>> + struct kvm_vcpu *r_vcpu;
>>> +
>>> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
>>> + data = region->read(r_vcpu, addr, len);
>>> + } else if (region->its_read) {
>>> + data = region->its_read(vcpu->kvm, iodev->its, addr, len);
>>> + }
>>
>> I'd rather have a flag somewhere that allows us to identify the type of
>> device we're dealing with. You could have something like:
>>
>> enum iodev_type {
>> IODEV_VGIC_DISTRIBUTOR,
>> IODEV_VGIC_REDISTRIBUTOR,
>> IODEV_VGIC_ITS,
>> };
>>
>> assign the type to each region we have, and then have something like:
>>
>> switch (region->iodev_type) {
>> case IODEV_VGIC_DISTRIBUTOR:
>> data = region->read(vcpu, addr, len);
>> break;
>> case IODEV_VGIC_REDISTRIBUTOR:
>> data = region->read(iodev->redist_vcpu, addr, len);
>> break;
>> case IODEV_VGIC_ITS:
>> data = region->its_read(vcpu->kvm, iodev->its, addr, len);
>> break;
>> }
>>
>> It would make it extensible *and* readable.
>
> Agreed, will fix it.
Cool. Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
