Re: [PATCH v10 09/17] KVM: arm64: introduce ITS emulation file with MMIO framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 18/07/2016 11:43, Marc Zyngier wrote:
> On 18/07/16 10:18, Auger Eric wrote:
>> Hi Andre, Marc,
>>
>> On 15/07/2016 13:43, 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>
>>> ---
>>>  include/kvm/arm_vgic.h           |  22 ++++++++-
>>>  virt/kvm/arm/vgic/vgic-its.c     | 103 +++++++++++++++++++++++++++++++++++++++
>>>  virt/kvm/arm/vgic/vgic-mmio-v3.c |  40 ++++++++++++++-
>>>  virt/kvm/arm/vgic/vgic-mmio.c    |  37 +++++++++++---
>>>  virt/kvm/arm/vgic/vgic-mmio.h    |  17 +++++--
>>>  virt/kvm/arm/vgic/vgic.h         |   7 +++
>>>  6 files changed, 213 insertions(+), 13 deletions(-)
>>>  create mode 100644 virt/kvm/arm/vgic/vgic-its.c
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index df2dec5..685f339 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -108,15 +108,35 @@ struct vgic_irq {
>>>  };
>>>  
>>>  struct vgic_register_region;
>>> +struct vgic_its;
>>> +
>>> +enum iodev_type {
>>> +	IODEV_CPUIF,
>>> +	IODEV_DIST,
>>> +	IODEV_REDIST,
>>> +	IODEV_ITS
>>> +};
>>>  
>>>  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;
>>> +	enum iodev_type iodev_type;
>>>  	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;
>>> +};
>>> +
>>>  struct vgic_dist {
>>>  	bool			in_kernel;
>>>  	bool			ready;
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>> new file mode 100644
>>> index 0000000..4654d6e
>>> --- /dev/null
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -0,0 +1,103 @@
>>> +/*
>>> + * 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,					\
>>> +	.its_read = rd,						\
>>> +	.its_write = wr,					\
>>> +}
>>> +
>>> +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 */
>>> +}
>>> +
>>> +static struct vgic_register_region its_registers[] = {
>>> +	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),
>>> +};
>>> +
>>> +static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
>>> +{
>>> +	struct vgic_io_device *iodev = &its->iodev;
>>> +	int ret;
>>> +
>>> +	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>>> +		return -ENXIO;
>>> +
>>> +	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->iodev_type = IODEV_ITS;
>>> +	iodev->its = its;
>>> +	mutex_lock(&kvm->slots_lock);
>>> +	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, iodev->base_addr,
>>> +				      KVM_VGIC_V3_ITS_SIZE, &iodev->dev);
>>> +	mutex_unlock(&kvm->slots_lock);
>>> +
>>> +	return ret;
>>> +}
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index 278bfbb..b92b7d6 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)
>>>  {
>>> @@ -132,6 +142,32 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,
>>>  	vgic_put_irq(vcpu->kvm, 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)
>>>  {
>>> @@ -372,7 +408,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,
>>> @@ -450,6 +486,7 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
>>>  
>>>  		kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
>>>  		rd_dev->base_addr = rd_base;
>>> +		rd_dev->iodev_type = IODEV_REDIST;
>>>  		rd_dev->regions = vgic_v3_rdbase_registers;
>>>  		rd_dev->nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
>>>  		rd_dev->redist_vcpu = vcpu;
>>> @@ -464,6 +501,7 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
>>>  
>>>  		kvm_iodevice_init(&sgi_dev->dev, &kvm_io_gic_ops);
>>>  		sgi_dev->base_addr = sgi_base;
>>> +		sgi_dev->iodev_type = IODEV_REDIST;
>>>  		sgi_dev->regions = vgic_v3_sgibase_registers;
>>>  		sgi_dev->nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers);
>>>  		sgi_dev->redist_vcpu = vcpu;
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>> index 5e79e01..26be827 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>> @@ -473,8 +473,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);
>>> @@ -483,8 +482,20 @@ 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);
>>> +	switch (iodev->iodev_type) {
>>> +	case IODEV_CPUIF:
>>> +		return 1;
>> This change causes a regression on AMD Seattle with QEMU (abort with
>> message "restoring 288 IRQs, but kernel supports max 32").
>> Shoudn't we have
>> 		data = region->read(vcpu, addr, len);
>> 		break;
>> This path is exercised by vgic_v2_cpuif_uaccess/vgic_uaccess
> 
> Bloody hell. Andre, have you even tested this?
> 
>>> +	case IODEV_DIST:
>>> +		data = region->read(vcpu, addr, len);
>>> +		break;
>>> +	case IODEV_REDIST:
>>> +		data = region->read(iodev->redist_vcpu, addr, len);
>>> +		break;
>>> +	case IODEV_ITS:
>>> +		data = region->its_read(vcpu->kvm, iodev->its, addr, len);
>>> +		break;
>>> +	}
>>> +
>>>  	vgic_data_host_to_mmio_bus(val, len, data);
>>>  	return 0;
>>>  }
>>> @@ -494,7 +505,6 @@ static int dispatch_mmio_write(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 = vgic_data_mmio_bus_to_host(val, len);
>>>  
>>>  	region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
>>> @@ -505,8 +515,20 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>>>  	if (!check_region(region, addr, len))
>>>  		return 0;
>>>  
>>> -	r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
>>> -	region->write(r_vcpu, addr, len, data);
>>> +	switch (iodev->iodev_type) {
>>> +	case IODEV_CPUIF:
>>> +		break;
>> same here
>> region->write(vcpu, addr, len, data);
>>
>> The above modifications fix the issue.
> 
> Eric, can you please whip up a quick patch patch that I can stick on
> top? Thanks,
Yes, no worries.

Eric
> 
> 	M.
> 
--
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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux