[RFC v2 00/19] vITS save/restore

[RFC v2 00/19] vITS save/restore

[Eric Auger]

This series specifies and implements an API aimed at saving and restoring
the state of the in-kernel emulated ITS device.

The ITS is programmed through registers and tables. Those latter
are allocated by the guest. Their base address is programmed in
registers or table entries before the ITS is enabled.

The ITS is free to use some of them to flush its internal caches. This
is likely to be used when entering low power state.

Therefore, for save/restore use case, it looks natural to use this
guest RAM allocated space to save the table related data. However,
currently, The ITS in-kernel emulated device does not use all of those
tables and for those it uses, it does not always sync them with its
cached data. Additional sync must happen for:
- the collection table
- the device table
- the per-device translation tables
- the LPI pending tables.

The LPI configation table and the command queues do not need extra
syncs.

An alternative to flushing the tables into guest RAM could have been
to flush them into a separate user-space buffer. However the drawback
of this alternative is that the virtualizer would allocate dedicated
buffers to store the data that should normally be laid out in guest
RAM. It would also be obliged to re-compute their size from
register/table content.

So saving the tables in guest RAM better fit the ITS programming
model and optimizes the memory usage. The drawback of this solution
is it brings additional challenges at user-space level to make sure
the guest RAM is frozen after table sync.

The series applies on top of Vijaya's series:
- [PATCH v11 0/8] arm/arm64: vgic: Implement API for vGICv3 live
  migration
  https://www.spinics.net/lists/arm-kernel/msg558046.html

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/kvmarm-queue-vgic-mig-v11-its-rfcv2

applied on kvmarm queue.

* Testing:
- on Cavium ThunderX using a virtio-net-pci guest (x3 vNIC),
  virsh save/restore commands and virt-manager live migration.
  Tested with 1 and 2 stage device table.

History:

v1 -> v2:
- rebased on Vijaya's v11
- all entries now are 8 byte large
- devid/eventid indexing for device table and ITT
- support 2 stage device table
- common infra to read indexed tables
- add cpu <-> le64 conversions
- itte renamed into ite
- do not care anymore about pending table 1st KB
  (not needed at the moment for coarse mapping)

RFC v1
- creation


Eric Auger (19):
  KVM: arm/arm64: Add vITS save/restore API documentation
  KVM: arm/arm64: rename itte into ite
  arm/arm64: vgic: turn vgic_find_mmio_region into public
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_REGS group
  KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access
  KVM: arm64: ITS: Implement vgic_mmio_uaccess_write_its_creadr
  KVM: arm64: ITS: Report the ITE size in GITS_TYPER
  KVM: arm64: ITS: Interpret MAPD Size field and check related errors
  KVM: arm64: ITS: Interpret MAPD ITT_addr field
  KVM: arm64: ITS: Check the device id matches TYPER DEVBITS range
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_TABLES group
  KVM: arm64: ITS: vgic_its_alloc_ite/device
  KVM: arm64: ITS: Sort the device and ITE lists
  KVM: arm64: ITS: Add infrastructure for table lookup
  KVM: arm64: ITS: Collection table save/restore
  KVM: arm64: ITS: vgic_its_check_id returns the entry's GPA
  KVM: arm64: ITS: ITT flush and restore
  KVM: arm64: ITS: Device table save/restore
  KVM: arm64: ITS: Pending table save/restore

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |   78 ++
 arch/arm/include/uapi/asm/kvm.h                    |    2 +
 arch/arm64/include/uapi/asm/kvm.h                  |    2 +
 include/linux/irqchip/arm-gic-v3.h                 |    3 +
 virt/kvm/arm/vgic/vgic-its.c                       | 1031 ++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c                      |    3 +-
 virt/kvm/arm/vgic/vgic-mmio.h                      |   14 +-
 7 files changed, 1022 insertions(+), 111 deletions(-)

-- 
2.5.5

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Andre Przywara]

On 08/02/17 11:43, Eric Auger wrote:

Salut Eric,

one minor thing below, but first a general question:
I take it that the state of the ITS (enabled/disabled) shouldn't matter
when it comes to reading/writing registers, right? Because this is
totally under guest control and userland shouldn't mess with it?

Maybe this is handled somewhere in the next patches, but how are we
supposed to write CBASER and the BASERs, for instance, if the handler
bails out early when the ITS is already enabled?

> This patch implements vgic_its_has_attr_regs and vgic_its_attr_regs_access
> upon the MMIO framework. VGIC ITS KVM device KVM_DEV_ARM_VGIC_GRP_ITS_REGS
> group becomes functional.
> 
> At least GITS_CREADR requires to differentiate a guest write action from a
> user access. As such let's introduce a new uaccess_its_write
> vgic_register_region callback.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  virt/kvm/arm/vgic/vgic-its.c  | 74 ++++++++++++++++++++++++++++++++++++-------
>  virt/kvm/arm/vgic/vgic-mmio.h |  9 ++++--
>  2 files changed, 69 insertions(+), 14 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 43bb17e..e9c8f9f 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1287,13 +1287,14 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  	*regptr = reg;
>  }
>  
> -#define REGISTER_ITS_DESC(off, rd, wr, length, acc)		\
> +#define REGISTER_ITS_DESC(off, rd, wr, uwr, length, acc)		\
>  {								\
>  	.reg_offset = off,					\
>  	.len = length,						\
>  	.access_flags = acc,					\
>  	.its_read = rd,						\
>  	.its_write = wr,					\
> +	.uaccess_its_write = uwr,					\
>  }
>  
>  static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
> @@ -1304,28 +1305,28 @@ static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>  
>  static struct vgic_register_region its_registers[] = {
>  	REGISTER_ITS_DESC(GITS_CTLR,
> -		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4,
> +		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, NULL, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_ITS_DESC(GITS_IIDR,
> -		vgic_mmio_read_its_iidr, its_mmio_write_wi, 4,
> +		vgic_mmio_read_its_iidr, its_mmio_write_wi, NULL, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_ITS_DESC(GITS_TYPER,
> -		vgic_mmio_read_its_typer, its_mmio_write_wi, 8,
> +		vgic_mmio_read_its_typer, its_mmio_write_wi, NULL, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>  	REGISTER_ITS_DESC(GITS_CBASER,
> -		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, 8,
> +		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, NULL, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>  	REGISTER_ITS_DESC(GITS_CWRITER,
> -		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, 8,
> -		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> +		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, NULL,
> +		8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>  	REGISTER_ITS_DESC(GITS_CREADR,
> -		vgic_mmio_read_its_creadr, its_mmio_write_wi, 8,
> +		vgic_mmio_read_its_creadr, its_mmio_write_wi, NULL, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>  	REGISTER_ITS_DESC(GITS_BASER,
> -		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, 0x40,
> +		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, NULL, 0x40,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>  	REGISTER_ITS_DESC(GITS_IDREGS_BASE,
> -		vgic_mmio_read_its_idregs, its_mmio_write_wi, 0x30,
> +		vgic_mmio_read_its_idregs, its_mmio_write_wi, NULL, 0x30,
>  		VGIC_ACCESS_32bit),
>  };
>  
> @@ -1448,14 +1449,63 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
>  int vgic_its_has_attr_regs(struct kvm_device *dev,
>  			   struct kvm_device_attr *attr)
>  {
> -	return -ENXIO;
> +	const struct vgic_register_region *region;
> +	struct vgic_io_device iodev = {
> +		.regions = its_registers,
> +		.nr_regions = ARRAY_SIZE(its_registers),
> +	};
> +	gpa_t offset;
> +
> +	offset = attr->attr;
> +
> +	region = vgic_find_mmio_region(iodev.regions,
> +				       iodev.nr_regions,
> +				       offset);
> +	if (!region)
> +		return -ENXIO;
> +	return 0;
>  }
>  
>  int vgic_its_attr_regs_access(struct kvm_device *dev,
>  			      struct kvm_device_attr *attr,
>  			      u64 *reg, bool is_write)
>  {
> -	return -ENXIO;
> +	const struct vgic_register_region *region;
> +	struct vgic_io_device iodev = {
> +		.regions = its_registers,
> +		.nr_regions = ARRAY_SIZE(its_registers),
> +	};
> +	struct vgic_its *its = dev->private;
> +	gpa_t addr, offset;
> +	unsigned int len;
> +
> +	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
> +		return -ENXIO;
> +
> +	offset = attr->attr;
> +	if (offset & 0x7)
> +		return -EINVAL;

Isn't GITS_IIDR only 32-bit aligned?
Is it expected to just avoid reading this from userland?
If yes, it deserves a comment here, I guess.

Cheers,
Andre.

> +
> +	addr = its->vgic_its_base + offset;
> +
> +	region = vgic_find_mmio_region(iodev.regions,
> +				       iodev.nr_regions,
> +				       offset);
> +	if (!region)
> +		return -ENXIO;
> +
> +	len = region->access_flags & VGIC_ACCESS_64bit ? 8 : 4;
> +
> +	if (is_write) {
> +		if (region->uaccess_its_write)
> +			region->uaccess_its_write(dev->kvm, its, addr,
> +						  len, *reg);
> +		else
> +			region->its_write(dev->kvm, its, addr, len, *reg);
> +	} else {
> +		*reg = region->its_read(dev->kvm, its, addr, len);
> +	}
> +	return 0;
>  }
>  
>  static int vgic_its_has_attr(struct kvm_device *dev,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 055ad42..ad8a585 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -36,8 +36,13 @@ struct vgic_register_region {
>  	};
>  	unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
>  				      unsigned int len);
> -	void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
> -			      unsigned int len, unsigned long val);
> +	union {
> +		void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
> +				      unsigned int len, unsigned long val);
> +		void (*uaccess_its_write)(struct kvm *kvm, struct vgic_its *its,
> +					  gpa_t addr, unsigned int len,
> +					  unsigned long val);
> +	};
>  };
>  
>  extern struct kvm_io_device_ops kvm_io_gic_ops;
> 

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Auger Eric]

Hi Andre,

On 10/02/2017 12:57, Andre Przywara wrote:
> On 08/02/17 11:43, Eric Auger wrote:
> 
> Salut Eric,
> 
> one minor thing below, but first a general question:
> I take it that the state of the ITS (enabled/disabled) shouldn't matter
> when it comes to reading/writing registers, right? Because this is
> totally under guest control and userland shouldn't mess with it?
> 
> Maybe this is handled somewhere in the next patches, but how are we
> supposed to write CBASER and the BASERs, for instance, if the handler
> bails out early when the ITS is already enabled?
Well I mentioned in the KVM ITS device documentation that userspace
accesses to those registers have the same behavior as guest accesses. As
such it is not possible to write into CBASER and BASERs when the ITS is
already enabled. But isn't it correct to forbid such userspace accesses
at this moment? What is the use case?
> 
>> This patch implements vgic_its_has_attr_regs and vgic_its_attr_regs_access
>> upon the MMIO framework. VGIC ITS KVM device KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>> group becomes functional.
>>
>> At least GITS_CREADR requires to differentiate a guest write action from a
>> user access. As such let's introduce a new uaccess_its_write
>> vgic_register_region callback.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c  | 74 ++++++++++++++++++++++++++++++++++++-------
>>  virt/kvm/arm/vgic/vgic-mmio.h |  9 ++++--
>>  2 files changed, 69 insertions(+), 14 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 43bb17e..e9c8f9f 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1287,13 +1287,14 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>>  	*regptr = reg;
>>  }
>>  
>> -#define REGISTER_ITS_DESC(off, rd, wr, length, acc)		\
>> +#define REGISTER_ITS_DESC(off, rd, wr, uwr, length, acc)		\
>>  {								\
>>  	.reg_offset = off,					\
>>  	.len = length,						\
>>  	.access_flags = acc,					\
>>  	.its_read = rd,						\
>>  	.its_write = wr,					\
>> +	.uaccess_its_write = uwr,					\
>>  }
>>  
>>  static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>> @@ -1304,28 +1305,28 @@ static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>>  
>>  static struct vgic_register_region its_registers[] = {
>>  	REGISTER_ITS_DESC(GITS_CTLR,
>> -		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4,
>> +		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, NULL, 4,
>>  		VGIC_ACCESS_32bit),
>>  	REGISTER_ITS_DESC(GITS_IIDR,
>> -		vgic_mmio_read_its_iidr, its_mmio_write_wi, 4,
>> +		vgic_mmio_read_its_iidr, its_mmio_write_wi, NULL, 4,
>>  		VGIC_ACCESS_32bit),
>>  	REGISTER_ITS_DESC(GITS_TYPER,
>> -		vgic_mmio_read_its_typer, its_mmio_write_wi, 8,
>> +		vgic_mmio_read_its_typer, its_mmio_write_wi, NULL, 8,
>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>  	REGISTER_ITS_DESC(GITS_CBASER,
>> -		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, 8,
>> +		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, NULL, 8,
>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>  	REGISTER_ITS_DESC(GITS_CWRITER,
>> -		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, 8,
>> -		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>> +		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, NULL,
>> +		8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>  	REGISTER_ITS_DESC(GITS_CREADR,
>> -		vgic_mmio_read_its_creadr, its_mmio_write_wi, 8,
>> +		vgic_mmio_read_its_creadr, its_mmio_write_wi, NULL, 8,
>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>  	REGISTER_ITS_DESC(GITS_BASER,
>> -		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, 0x40,
>> +		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, NULL, 0x40,
>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>  	REGISTER_ITS_DESC(GITS_IDREGS_BASE,
>> -		vgic_mmio_read_its_idregs, its_mmio_write_wi, 0x30,
>> +		vgic_mmio_read_its_idregs, its_mmio_write_wi, NULL, 0x30,
>>  		VGIC_ACCESS_32bit),
>>  };
>>  
>> @@ -1448,14 +1449,63 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
>>  int vgic_its_has_attr_regs(struct kvm_device *dev,
>>  			   struct kvm_device_attr *attr)
>>  {
>> -	return -ENXIO;
>> +	const struct vgic_register_region *region;
>> +	struct vgic_io_device iodev = {
>> +		.regions = its_registers,
>> +		.nr_regions = ARRAY_SIZE(its_registers),
>> +	};
>> +	gpa_t offset;
>> +
>> +	offset = attr->attr;
>> +
>> +	region = vgic_find_mmio_region(iodev.regions,
>> +				       iodev.nr_regions,
>> +				       offset);
>> +	if (!region)
>> +		return -ENXIO;
>> +	return 0;
>>  }
>>  
>>  int vgic_its_attr_regs_access(struct kvm_device *dev,
>>  			      struct kvm_device_attr *attr,
>>  			      u64 *reg, bool is_write)
>>  {
>> -	return -ENXIO;
>> +	const struct vgic_register_region *region;
>> +	struct vgic_io_device iodev = {
>> +		.regions = its_registers,
>> +		.nr_regions = ARRAY_SIZE(its_registers),
>> +	};
>> +	struct vgic_its *its = dev->private;
>> +	gpa_t addr, offset;
>> +	unsigned int len;
>> +
>> +	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>> +		return -ENXIO;
>> +
>> +	offset = attr->attr;
>> +	if (offset & 0x7)
>> +		return -EINVAL;
> 
> Isn't GITS_IIDR only 32-bit aligned?
> Is it expected to just avoid reading this from userland?
> If yes, it deserves a comment here, I guess.
No it was not made on purpose :-( I will fix that.

Thanks!

Eric
> 
> Cheers,
> Andre.
> 
>> +
>> +	addr = its->vgic_its_base + offset;
>> +
>> +	region = vgic_find_mmio_region(iodev.regions,
>> +				       iodev.nr_regions,
>> +				       offset);
>> +	if (!region)
>> +		return -ENXIO;
>> +
>> +	len = region->access_flags & VGIC_ACCESS_64bit ? 8 : 4;
>> +
>> +	if (is_write) {
>> +		if (region->uaccess_its_write)
>> +			region->uaccess_its_write(dev->kvm, its, addr,
>> +						  len, *reg);
>> +		else
>> +			region->its_write(dev->kvm, its, addr, len, *reg);
>> +	} else {
>> +		*reg = region->its_read(dev->kvm, its, addr, len);
>> +	}
>> +	return 0;
>>  }
>>  
>>  static int vgic_its_has_attr(struct kvm_device *dev,
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
>> index 055ad42..ad8a585 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.h
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
>> @@ -36,8 +36,13 @@ struct vgic_register_region {
>>  	};
>>  	unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
>>  				      unsigned int len);
>> -	void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
>> -			      unsigned int len, unsigned long val);
>> +	union {
>> +		void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
>> +				      unsigned int len, unsigned long val);
>> +		void (*uaccess_its_write)(struct kvm *kvm, struct vgic_its *its,
>> +					  gpa_t addr, unsigned int len,
>> +					  unsigned long val);
>> +	};
>>  };
>>  
>>  extern struct kvm_io_device_ops kvm_io_gic_ops;
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Andre Przywara]

Hi,

On 10/02/17 12:26, Auger Eric wrote:
> Hi Andre,
> 
> On 10/02/2017 12:57, Andre Przywara wrote:
>> On 08/02/17 11:43, Eric Auger wrote:
>>
>> Salut Eric,
>>
>> one minor thing below, but first a general question:
>> I take it that the state of the ITS (enabled/disabled) shouldn't matter
>> when it comes to reading/writing registers, right? Because this is
>> totally under guest control and userland shouldn't mess with it?
>>
>> Maybe this is handled somewhere in the next patches, but how are we
>> supposed to write CBASER and the BASERs, for instance, if the handler
>> bails out early when the ITS is already enabled?
> Well I mentioned in the KVM ITS device documentation 

Oh, I am one of those guys who read the documentation at the very end
;-) Sorry, my bad.

> that userspace
> accesses to those registers have the same behavior as guest accesses. As
> such it is not possible to write into CBASER and BASERs when the ITS is
> already enabled. But isn't it correct to forbid such userspace accesses
> at this moment? What is the use case?

So the idea is to observe an order when restoring the registers? And
relying on the ITS being disabled upon creation, so that we can write to
those registers? Then restoring CTLR as the very last to get things going?

I think we need some special handling for CWRITER as well, but I just
see that we have some bug in our ITS emulation (processing commands
while the ITS being disabled and not triggering command processing upon
ITS enablement). Waiting for Marc's verdict on this.
I think the patch I came up with should fix this particular issue here.

But apart from that it should work, I think.

Cheers,
Andre.

>>> This patch implements vgic_its_has_attr_regs and vgic_its_attr_regs_access
>>> upon the MMIO framework. VGIC ITS KVM device KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>>> group becomes functional.
>>>
>>> At least GITS_CREADR requires to differentiate a guest write action from a
>>> user access. As such let's introduce a new uaccess_its_write
>>> vgic_register_region callback.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-its.c  | 74 ++++++++++++++++++++++++++++++++++++-------
>>>  virt/kvm/arm/vgic/vgic-mmio.h |  9 ++++--
>>>  2 files changed, 69 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>> index 43bb17e..e9c8f9f 100644
>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>> @@ -1287,13 +1287,14 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>>>  	*regptr = reg;
>>>  }
>>>  
>>> -#define REGISTER_ITS_DESC(off, rd, wr, length, acc)		\
>>> +#define REGISTER_ITS_DESC(off, rd, wr, uwr, length, acc)		\
>>>  {								\
>>>  	.reg_offset = off,					\
>>>  	.len = length,						\
>>>  	.access_flags = acc,					\
>>>  	.its_read = rd,						\
>>>  	.its_write = wr,					\
>>> +	.uaccess_its_write = uwr,					\
>>>  }
>>>  
>>>  static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>>> @@ -1304,28 +1305,28 @@ static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>>>  
>>>  static struct vgic_register_region its_registers[] = {
>>>  	REGISTER_ITS_DESC(GITS_CTLR,
>>> -		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4,
>>> +		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, NULL, 4,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_ITS_DESC(GITS_IIDR,
>>> -		vgic_mmio_read_its_iidr, its_mmio_write_wi, 4,
>>> +		vgic_mmio_read_its_iidr, its_mmio_write_wi, NULL, 4,
>>>  		VGIC_ACCESS_32bit),
>>>  	REGISTER_ITS_DESC(GITS_TYPER,
>>> -		vgic_mmio_read_its_typer, its_mmio_write_wi, 8,
>>> +		vgic_mmio_read_its_typer, its_mmio_write_wi, NULL, 8,
>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>  	REGISTER_ITS_DESC(GITS_CBASER,
>>> -		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, 8,
>>> +		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, NULL, 8,
>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>  	REGISTER_ITS_DESC(GITS_CWRITER,
>>> -		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, 8,
>>> -		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>> +		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, NULL,
>>> +		8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>  	REGISTER_ITS_DESC(GITS_CREADR,
>>> -		vgic_mmio_read_its_creadr, its_mmio_write_wi, 8,
>>> +		vgic_mmio_read_its_creadr, its_mmio_write_wi, NULL, 8,
>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>  	REGISTER_ITS_DESC(GITS_BASER,
>>> -		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, 0x40,
>>> +		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, NULL, 0x40,
>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>  	REGISTER_ITS_DESC(GITS_IDREGS_BASE,
>>> -		vgic_mmio_read_its_idregs, its_mmio_write_wi, 0x30,
>>> +		vgic_mmio_read_its_idregs, its_mmio_write_wi, NULL, 0x30,
>>>  		VGIC_ACCESS_32bit),
>>>  };
>>>  
>>> @@ -1448,14 +1449,63 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
>>>  int vgic_its_has_attr_regs(struct kvm_device *dev,
>>>  			   struct kvm_device_attr *attr)
>>>  {
>>> -	return -ENXIO;
>>> +	const struct vgic_register_region *region;
>>> +	struct vgic_io_device iodev = {
>>> +		.regions = its_registers,
>>> +		.nr_regions = ARRAY_SIZE(its_registers),
>>> +	};
>>> +	gpa_t offset;
>>> +
>>> +	offset = attr->attr;
>>> +
>>> +	region = vgic_find_mmio_region(iodev.regions,
>>> +				       iodev.nr_regions,
>>> +				       offset);
>>> +	if (!region)
>>> +		return -ENXIO;
>>> +	return 0;
>>>  }
>>>  
>>>  int vgic_its_attr_regs_access(struct kvm_device *dev,
>>>  			      struct kvm_device_attr *attr,
>>>  			      u64 *reg, bool is_write)
>>>  {
>>> -	return -ENXIO;
>>> +	const struct vgic_register_region *region;
>>> +	struct vgic_io_device iodev = {
>>> +		.regions = its_registers,
>>> +		.nr_regions = ARRAY_SIZE(its_registers),
>>> +	};
>>> +	struct vgic_its *its = dev->private;
>>> +	gpa_t addr, offset;
>>> +	unsigned int len;
>>> +
>>> +	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>>> +		return -ENXIO;
>>> +
>>> +	offset = attr->attr;
>>> +	if (offset & 0x7)
>>> +		return -EINVAL;
>>
>> Isn't GITS_IIDR only 32-bit aligned?
>> Is it expected to just avoid reading this from userland?
>> If yes, it deserves a comment here, I guess.
> No it was not made on purpose :-( I will fix that.
> 
> Thanks!
> 
> Eric
>>
>> Cheers,
>> Andre.
>>
>>> +
>>> +	addr = its->vgic_its_base + offset;
>>> +
>>> +	region = vgic_find_mmio_region(iodev.regions,
>>> +				       iodev.nr_regions,
>>> +				       offset);
>>> +	if (!region)
>>> +		return -ENXIO;
>>> +
>>> +	len = region->access_flags & VGIC_ACCESS_64bit ? 8 : 4;
>>> +
>>> +	if (is_write) {
>>> +		if (region->uaccess_its_write)
>>> +			region->uaccess_its_write(dev->kvm, its, addr,
>>> +						  len, *reg);
>>> +		else
>>> +			region->its_write(dev->kvm, its, addr, len, *reg);
>>> +	} else {
>>> +		*reg = region->its_read(dev->kvm, its, addr, len);
>>> +	}
>>> +	return 0;
>>>  }
>>>  
>>>  static int vgic_its_has_attr(struct kvm_device *dev,
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
>>> index 055ad42..ad8a585 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio.h
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
>>> @@ -36,8 +36,13 @@ struct vgic_register_region {
>>>  	};
>>>  	unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
>>>  				      unsigned int len);
>>> -	void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
>>> -			      unsigned int len, unsigned long val);
>>> +	union {
>>> +		void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
>>> +				      unsigned int len, unsigned long val);
>>> +		void (*uaccess_its_write)(struct kvm *kvm, struct vgic_its *its,
>>> +					  gpa_t addr, unsigned int len,
>>> +					  unsigned long val);
>>> +	};
>>>  };
>>>  
>>>  extern struct kvm_io_device_ops kvm_io_gic_ops;
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Auger Eric]

Hi Andre,

On 10/02/2017 18:06, Andre Przywara wrote:
> Hi,
> 
> On 10/02/17 12:26, Auger Eric wrote:
>> Hi Andre,
>>
>> On 10/02/2017 12:57, Andre Przywara wrote:
>>> On 08/02/17 11:43, Eric Auger wrote:
>>>
>>> Salut Eric,
>>>
>>> one minor thing below, but first a general question:
>>> I take it that the state of the ITS (enabled/disabled) shouldn't matter
>>> when it comes to reading/writing registers, right? Because this is
>>> totally under guest control and userland shouldn't mess with it?
>>>
>>> Maybe this is handled somewhere in the next patches, but how are we
>>> supposed to write CBASER and the BASERs, for instance, if the handler
>>> bails out early when the ITS is already enabled?
>> Well I mentioned in the KVM ITS device documentation 
> 
> Oh, I am one of those guys who read the documentation at the very end
> ;-) Sorry, my bad.
> 
>> that userspace
>> accesses to those registers have the same behavior as guest accesses. As
>> such it is not possible to write into CBASER and BASERs when the ITS is
>> already enabled. But isn't it correct to forbid such userspace accesses
>> at this moment? What is the use case?
> 
> So the idea is to observe an order when restoring the registers? And
> relying on the ITS being disabled upon creation, so that we can write to
> those registers? Then restoring CTLR as the very last to get things going?
Yes that's what I currently do on QEMU side.
> 
> I think we need some special handling for CWRITER as well, but I just
> see that we have some bug in our ITS emulation (processing commands
> while the ITS being disabled and not triggering command processing upon
> ITS enablement). Waiting for Marc's verdict on this.
> I think the patch I came up with should fix this particular issue here.
OK Looking forward to reviewing it.

Thanks

Eric
> 
> But apart from that it should work, I think.
> 
> Cheers,
> Andre.
> 
>>>> This patch implements vgic_its_has_attr_regs and vgic_its_attr_regs_access
>>>> upon the MMIO framework. VGIC ITS KVM device KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>>>> group becomes functional.
>>>>
>>>> At least GITS_CREADR requires to differentiate a guest write action from a
>>>> user access. As such let's introduce a new uaccess_its_write
>>>> vgic_register_region callback.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  virt/kvm/arm/vgic/vgic-its.c  | 74 ++++++++++++++++++++++++++++++++++++-------
>>>>  virt/kvm/arm/vgic/vgic-mmio.h |  9 ++++--
>>>>  2 files changed, 69 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>>> index 43bb17e..e9c8f9f 100644
>>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>>> @@ -1287,13 +1287,14 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>>>>  	*regptr = reg;
>>>>  }
>>>>  
>>>> -#define REGISTER_ITS_DESC(off, rd, wr, length, acc)		\
>>>> +#define REGISTER_ITS_DESC(off, rd, wr, uwr, length, acc)		\
>>>>  {								\
>>>>  	.reg_offset = off,					\
>>>>  	.len = length,						\
>>>>  	.access_flags = acc,					\
>>>>  	.its_read = rd,						\
>>>>  	.its_write = wr,					\
>>>> +	.uaccess_its_write = uwr,					\
>>>>  }
>>>>  
>>>>  static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>>>> @@ -1304,28 +1305,28 @@ static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
>>>>  
>>>>  static struct vgic_register_region its_registers[] = {
>>>>  	REGISTER_ITS_DESC(GITS_CTLR,
>>>> -		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4,
>>>> +		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, NULL, 4,
>>>>  		VGIC_ACCESS_32bit),
>>>>  	REGISTER_ITS_DESC(GITS_IIDR,
>>>> -		vgic_mmio_read_its_iidr, its_mmio_write_wi, 4,
>>>> +		vgic_mmio_read_its_iidr, its_mmio_write_wi, NULL, 4,
>>>>  		VGIC_ACCESS_32bit),
>>>>  	REGISTER_ITS_DESC(GITS_TYPER,
>>>> -		vgic_mmio_read_its_typer, its_mmio_write_wi, 8,
>>>> +		vgic_mmio_read_its_typer, its_mmio_write_wi, NULL, 8,
>>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>>  	REGISTER_ITS_DESC(GITS_CBASER,
>>>> -		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, 8,
>>>> +		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, NULL, 8,
>>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>>  	REGISTER_ITS_DESC(GITS_CWRITER,
>>>> -		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, 8,
>>>> -		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>> +		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, NULL,
>>>> +		8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>>  	REGISTER_ITS_DESC(GITS_CREADR,
>>>> -		vgic_mmio_read_its_creadr, its_mmio_write_wi, 8,
>>>> +		vgic_mmio_read_its_creadr, its_mmio_write_wi, NULL, 8,
>>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>>  	REGISTER_ITS_DESC(GITS_BASER,
>>>> -		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, 0x40,
>>>> +		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, NULL, 0x40,
>>>>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>>>>  	REGISTER_ITS_DESC(GITS_IDREGS_BASE,
>>>> -		vgic_mmio_read_its_idregs, its_mmio_write_wi, 0x30,
>>>> +		vgic_mmio_read_its_idregs, its_mmio_write_wi, NULL, 0x30,
>>>>  		VGIC_ACCESS_32bit),
>>>>  };
>>>>  
>>>> @@ -1448,14 +1449,63 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
>>>>  int vgic_its_has_attr_regs(struct kvm_device *dev,
>>>>  			   struct kvm_device_attr *attr)
>>>>  {
>>>> -	return -ENXIO;
>>>> +	const struct vgic_register_region *region;
>>>> +	struct vgic_io_device iodev = {
>>>> +		.regions = its_registers,
>>>> +		.nr_regions = ARRAY_SIZE(its_registers),
>>>> +	};
>>>> +	gpa_t offset;
>>>> +
>>>> +	offset = attr->attr;
>>>> +
>>>> +	region = vgic_find_mmio_region(iodev.regions,
>>>> +				       iodev.nr_regions,
>>>> +				       offset);
>>>> +	if (!region)
>>>> +		return -ENXIO;
>>>> +	return 0;
>>>>  }
>>>>  
>>>>  int vgic_its_attr_regs_access(struct kvm_device *dev,
>>>>  			      struct kvm_device_attr *attr,
>>>>  			      u64 *reg, bool is_write)
>>>>  {
>>>> -	return -ENXIO;
>>>> +	const struct vgic_register_region *region;
>>>> +	struct vgic_io_device iodev = {
>>>> +		.regions = its_registers,
>>>> +		.nr_regions = ARRAY_SIZE(its_registers),
>>>> +	};
>>>> +	struct vgic_its *its = dev->private;
>>>> +	gpa_t addr, offset;
>>>> +	unsigned int len;
>>>> +
>>>> +	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>>>> +		return -ENXIO;
>>>> +
>>>> +	offset = attr->attr;
>>>> +	if (offset & 0x7)
>>>> +		return -EINVAL;
>>>
>>> Isn't GITS_IIDR only 32-bit aligned?
>>> Is it expected to just avoid reading this from userland?
>>> If yes, it deserves a comment here, I guess.
>> No it was not made on purpose :-( I will fix that.
>>
>> Thanks!
>>
>> Eric
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> +
>>>> +	addr = its->vgic_its_base + offset;
>>>> +
>>>> +	region = vgic_find_mmio_region(iodev.regions,
>>>> +				       iodev.nr_regions,
>>>> +				       offset);
>>>> +	if (!region)
>>>> +		return -ENXIO;
>>>> +
>>>> +	len = region->access_flags & VGIC_ACCESS_64bit ? 8 : 4;
>>>> +
>>>> +	if (is_write) {
>>>> +		if (region->uaccess_its_write)
>>>> +			region->uaccess_its_write(dev->kvm, its, addr,
>>>> +						  len, *reg);
>>>> +		else
>>>> +			region->its_write(dev->kvm, its, addr, len, *reg);
>>>> +	} else {
>>>> +		*reg = region->its_read(dev->kvm, its, addr, len);
>>>> +	}
>>>> +	return 0;
>>>>  }
>>>>  
>>>>  static int vgic_its_has_attr(struct kvm_device *dev,
>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
>>>> index 055ad42..ad8a585 100644
>>>> --- a/virt/kvm/arm/vgic/vgic-mmio.h
>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
>>>> @@ -36,8 +36,13 @@ struct vgic_register_region {
>>>>  	};
>>>>  	unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
>>>>  				      unsigned int len);
>>>> -	void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
>>>> -			      unsigned int len, unsigned long val);
>>>> +	union {
>>>> +		void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
>>>> +				      unsigned int len, unsigned long val);
>>>> +		void (*uaccess_its_write)(struct kvm *kvm, struct vgic_its *its,
>>>> +					  gpa_t addr, unsigned int len,
>>>> +					  unsigned long val);
>>>> +	};
>>>>  };
>>>>  
>>>>  extern struct kvm_io_device_ops kvm_io_gic_ops;
>>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[RFC v2 18/19] KVM: arm64: ITS: Device table save/restore

[Auger Eric]

Hi all,
On 08/02/2017 12:43, Eric Auger wrote:
> This patch flushes the device table entries into guest RAM.
> Both flat table and 2 stage tables are supported.  DeviceId
> indexing is used.
> 
> For each device listed in the device table, we also flush
> the translation table using the vgic_its_flush/restore_itt
> routines.
> 
> On restore, devices are re-allocated and their itte are
> re-built.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - use 8 byte format for DTE and ITE
> - support 2 stage format
> - remove kvm parameter
> - ITT flush/restore moved in a separate patch
> - use deviceid indexing
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index be9e8ed..c1ae85b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1814,6 +1814,7 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>  		return ret;
>  
>  	update_affinity_ite(kvm, ite);
> +	return 0;
>  }
>  
>  static int vgic_its_flush_itt(struct vgic_its *its, struct its_device *device)
> @@ -1848,12 +1849,137 @@ static int vgic_its_restore_itt(struct vgic_its *its,
>  }
>  
>  /**
> + * vgic_its_flush_dte - Flush a device table entry at a given GPA
> + *
> + * @its: ITS handle
> + * @dev: ITS device
> + * @ptr: GPA
> + */
> +static int vgic_its_flush_dte(struct vgic_its *its,
> +			      struct its_device *dev, gpa_t ptr)
> +{
> +	struct kvm *kvm = its->dev->kvm;
> +	u64 val, itt_addr_field;
> +	int ret;
> +	u32 next_offset;
> +
> +	itt_addr_field = dev->itt_addr >> 8;
> +	next_offset = compute_next_devid_offset(&its->device_list, dev);
> +	val = (((u64)next_offset << 45) | (itt_addr_field << 5) |
> +		(dev->nb_eventid_bits - 1));
> +	val = cpu_to_le64(val);
> +	ret = kvm_write_guest(kvm, ptr, &val, 8);
> +	return ret;
> +}
> +
> +/**
> + * vgic_its_restore_dte - restore a device table entry
> + *
> + * @its: its handle
> + * @id: device id the DTE corresponds to
> + * @ptr: kernel VA where the 8 byte DTE is located
> + * @opaque: unused
> + * @next: offset to the next valid device id
> + *
> + * Return: < 0 on error, 0 otherwise
> + */
> +static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> +				void *ptr, void *opaque, u32 *next)
> +{
> +	struct its_device *dev;
> +	gpa_t itt_addr;
> +	size_t size;
> +	u64 val, *p = (u64 *)ptr;
> +	int ret;
> +
> +	val = *p;
> +	val = le64_to_cpu(val);
> +
> +	size = val & GENMASK_ULL(4, 0);
> +	itt_addr = (val & GENMASK_ULL(48, 5)) >> 5;
I Just discovered a bug above. 48 should be replaced by 44. With my LPI
setup the restore phase was failing.

I will send a formal fix soon. Please apologize for the inconvenience.

Best Regards

Eric
> +	*next = 1;
> +
> +	if (!itt_addr)
> +		return 0;
> +
> +	/* dte entry is valid */
> +	*next = (val & GENMASK_ULL(63, 45)) >> 45;
> +
> +	ret = vgic_its_alloc_device(its, &dev, id,
> +				    itt_addr, size);
> +	if (ret)
> +		return ret;
> +	ret = vgic_its_restore_itt(its, dev);
> +
> +	return ret;
> +}
> +
> +/**
>   * vgic_its_flush_device_tables - flush the device table and all ITT
>   * into guest RAM
>   */
>  static int vgic_its_flush_device_tables(struct vgic_its *its)
>  {
> -	return -ENXIO;
> +	struct its_device *dev;
> +	u64 baser;
> +
> +	baser = its->baser_device_table;
> +
> +	list_for_each_entry(dev, &its->device_list, dev_list) {
> +		int ret;
> +		gpa_t eaddr;
> +
> +		if (!vgic_its_check_id(its, baser,
> +				       dev->device_id, &eaddr))
> +			return -EINVAL;
> +
> +		ret = vgic_its_flush_itt(its, dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = vgic_its_flush_dte(its, dev, eaddr);
> +		if (ret)
> +			return ret;
> +		}
> +	return 0;
> +}
> +
> +/**
> + * handle_l1_entry - callback used for L1 entries (2 stage case)
> + *
> + * @its: its handle
> + * @id: id
> + * @addr: kernel VA
> + * @opaque: unused
> + * @next_offset: offset to the next L1 entry: 0 if the last element
> + * was found, 1 otherwise
> + */
> +static int handle_l1_entry(struct vgic_its *its, u32 id, void *addr,
> +			   void *opaque, u32 *next_offset)
> +{
> +	u64 *pe = addr;
> +	gpa_t gpa;
> +	int l2_start_id = id * (SZ_64K / 8);
> +	int ret;
> +
> +	*pe = le64_to_cpu(*pe);
> +	*next_offset = 1;
> +
> +	if (!(*pe & BIT_ULL(63)))
> +		return 0;
> +
> +	gpa = *pe & GENMASK_ULL(51, 16);
> +
> +	ret = lookup_table(its, gpa, SZ_64K, 8,
> +			    l2_start_id, vgic_its_restore_dte, NULL);
> +
> +	if (ret == 1) {
> +		/* last entry was found in this L2 table */
> +		*next_offset = 0;
> +		ret = 0;
> +	}
> +
> +	return ret;
>  }
>  
>  /**
> @@ -1862,7 +1988,22 @@ static int vgic_its_flush_device_tables(struct vgic_its *its)
>   */
>  static int vgic_its_restore_device_tables(struct vgic_its *its)
>  {
> -	return -ENXIO;
> +	u64 baser = its->baser_device_table;
> +	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
> +	int l1_esz = GITS_BASER_ENTRY_SIZE(baser);
> +	gpa_t l1_gpa;
> +
> +	l1_gpa = BASER_ADDRESS(baser);
> +	if (!l1_gpa)
> +		return 0;
> +
> +	if (!(baser & GITS_BASER_INDIRECT))
> +		return lookup_table(its, l1_gpa, l1_tbl_size, l1_esz,
> +				    0, vgic_its_restore_dte, NULL);
> +
> +	/* two stage table */
> +	return lookup_table(its, l1_gpa, l1_tbl_size, 8, 0,
> +			    handle_l1_entry, NULL);
>  }
>  
>  static int vgic_its_flush_cte(struct vgic_its *its,
>