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

2017-02-13 Thread 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 
 ---
  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,
 -

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

2017-02-10 Thread 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 
>> ---
>>  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 =

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

2017-02-10 Thread 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 
> ---
>  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

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

2017-02-08 Thread Eric Auger 
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 
---
 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;
+
+