subject:"Re\: \[PATCH v8 09\/17\] KVM\: arm64\: introduce ITS emulation file with MMIO framework"

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

2016-07-08 Thread André Przywara

On 08/07/16 14:34, Marc Zyngier wrote:
> On 05/07/16 12:23, 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 
>> ---
>>  arch/arm64/kvm/Makefile  |   1 +
>>  include/kvm/arm_vgic.h   |  14 +-
>>  virt/kvm/arm/vgic/vgic-its.c | 100 +
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  40 +++
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 104 
>> ++-
>>  virt/kvm/arm/vgic/vgic-mmio.c|  36 +++---
>>  virt/kvm/arm/vgic/vgic-mmio.h|  31 +---
>>  virt/kvm/arm/vgic/vgic.h |   7 +++
>>  8 files changed, 266 insertions(+), 67 deletions(-)
>>  create mode 100644 virt/kvm/arm/vgic/vgic-its.c
>>
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index f00b2cd..a5b9664 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -29,5 +29,6 @@ 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
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index f6f860d..f606641 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -108,15 +108,27 @@ struct vgic_irq {
>>  };
>>  
>>  struct vgic_register_region;
>> +struct vgic_its;
>>  
>>  struct vgic_io_device {
>>  gpa_t base_addr;
>> -struct kvm_vcpu *redist_vcpu;
>> +union {
>> +struct kvm_vcpu *redist_vcpu;
>> +struct vgic_its *its;
>> +};
> 
> The only question that springs to mind is...
> 
>>  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;
>> +
>> +boolenabled;
>> +struct vgic_io_device   iodev;
>> +};
>> +
>>  struct vgic_dist {
>>  boolin_kernel;
>>  boolready;
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> new file mode 100644
>> index 000..ab8d244
>> --- /dev/null
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * GICv3 ITS emulation
>> + *
>> + * Copyright (C) 2015,2016 ARM Ltd.
>> + * Author: Andre Przywara 
>> + *
>> + * 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 .
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "vgic.h"
>> +#include "vgic-mmio.h"
>> +
>> +#define REGISTER_ITS_DESC(off, rd, wr, length, acc) \
>> +{   \
>> +.reg_offset = off,  \
>> +.len = length,  \
>> +.access_flags = acc,\
>> +.iodev_type = IODEV_ITS,\
> 
> ... why isn't this at the device level? It doesn't make much sense to
> have it at the register level (we never access a register in isolation,
> we always access it relatively to a device).
> 
> And given that the *only* time you actually evaluate this flag is in
> dispatch_mmio_read/write, there is zero benefit in duplicating it all
> over the place.
> 
> Smaller structures, smaller patch. Am I missing something?

Looks possible. I think I tried something like this in the beginning and
hit some wall - possibly the one in my head ;-). Also I found it saner
to have the type associated with the declaration instead of adjusting
this in the

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

2016-07-08 Thread Marc Zyngier

On 08/07/16 14:34, Marc Zyngier wrote:
> On 05/07/16 12:23, 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 
>> ---
>>  arch/arm64/kvm/Makefile  |   1 +
>>  include/kvm/arm_vgic.h   |  14 +-
>>  virt/kvm/arm/vgic/vgic-its.c | 100 +
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  40 +++
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 104 
>> ++-
>>  virt/kvm/arm/vgic/vgic-mmio.c|  36 +++---
>>  virt/kvm/arm/vgic/vgic-mmio.h|  31 +---
>>  virt/kvm/arm/vgic/vgic.h |   7 +++
>>  8 files changed, 266 insertions(+), 67 deletions(-)
>>  create mode 100644 virt/kvm/arm/vgic/vgic-its.c
>>
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index f00b2cd..a5b9664 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -29,5 +29,6 @@ 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
>>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index f6f860d..f606641 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -108,15 +108,27 @@ struct vgic_irq {
>>  };
>>  
>>  struct vgic_register_region;
>> +struct vgic_its;
>>  
>>  struct vgic_io_device {
>>  gpa_t base_addr;
>> -struct kvm_vcpu *redist_vcpu;
>> +union {
>> +struct kvm_vcpu *redist_vcpu;
>> +struct vgic_its *its;
>> +};
> 
> The only question that springs to mind is...
> 
>>  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;
>> +
>> +boolenabled;
>> +struct vgic_io_device   iodev;
>> +};
>> +
>>  struct vgic_dist {
>>  boolin_kernel;
>>  boolready;
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> new file mode 100644
>> index 000..ab8d244
>> --- /dev/null
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * GICv3 ITS emulation
>> + *
>> + * Copyright (C) 2015,2016 ARM Ltd.
>> + * Author: Andre Przywara 
>> + *
>> + * 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 .
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "vgic.h"
>> +#include "vgic-mmio.h"
>> +
>> +#define REGISTER_ITS_DESC(off, rd, wr, length, acc) \
>> +{   \
>> +.reg_offset = off,  \
>> +.len = length,  \
>> +.access_flags = acc,\
>> +.iodev_type = IODEV_ITS,\
> 
> ... why isn't this at the device level? It doesn't make much sense to
> have it at the register level (we never access a register in isolation,
> we always access it relatively to a device).
> 
> And given that the *only* time you actually evaluate this flag is in
> dispatch_mmio_read/write, there is zero benefit in duplicating it all
> over the place.
> 
> Smaller structures, smaller patch. Am I missing something?

And for the record, here's what I've cooked on top of your patch:

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index c64db0f..95eab74 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -112,12

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

2016-07-08 Thread Marc Zyngier

On 05/07/16 12:23, 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 
> ---
>  arch/arm64/kvm/Makefile  |   1 +
>  include/kvm/arm_vgic.h   |  14 +-
>  virt/kvm/arm/vgic/vgic-its.c | 100 +
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  40 +++
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 104 
> ++-
>  virt/kvm/arm/vgic/vgic-mmio.c|  36 +++---
>  virt/kvm/arm/vgic/vgic-mmio.h|  31 +---
>  virt/kvm/arm/vgic/vgic.h |   7 +++
>  8 files changed, 266 insertions(+), 67 deletions(-)
>  create mode 100644 virt/kvm/arm/vgic/vgic-its.c
> 
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index f00b2cd..a5b9664 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -29,5 +29,6 @@ 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
>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f6f860d..f606641 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -108,15 +108,27 @@ struct vgic_irq {
>  };
>  
>  struct vgic_register_region;
> +struct vgic_its;
>  
>  struct vgic_io_device {
>   gpa_t base_addr;
> - struct kvm_vcpu *redist_vcpu;
> + union {
> + struct kvm_vcpu *redist_vcpu;
> + struct vgic_its *its;
> + };

The only question that springs to mind is...

>   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;
> +
> + boolenabled;
> + struct vgic_io_device   iodev;
> +};
> +
>  struct vgic_dist {
>   boolin_kernel;
>   boolready;
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> new file mode 100644
> index 000..ab8d244
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -0,0 +1,100 @@
> +/*
> + * GICv3 ITS emulation
> + *
> + * Copyright (C) 2015,2016 ARM Ltd.
> + * Author: Andre Przywara 
> + *
> + * 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 .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "vgic.h"
> +#include "vgic-mmio.h"
> +
> +#define REGISTER_ITS_DESC(off, rd, wr, length, acc)  \
> +{\
> + .reg_offset = off,  \
> + .len = length,  \
> + .access_flags = acc,\
> + .iodev_type = IODEV_ITS,\

... why isn't this at the device level? It doesn't make much sense to
have it at the register level (we never access a register in isolation,
we always access it relatively to a device).

And given that the *only* time you actually evaluate this flag is in
dispatch_mmio_read/write, there is zero benefit in duplicating it all
over the place.

Smaller structures, smaller patch. Am I missing something?

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information