[RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Antonios Motakis]

On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
handle the MMIO access through any registered read/write callbacks. This
is a dependency for eventfd support (ioeventfd and irqfd).

However, accesses to the VGIC are still left implemented independently,
since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic.c |  5 ++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index 4cb5a93..1d17831 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	return 0;
 }
 
+/**
+ * kvm_handle_mmio - handle an in-kernel MMIO access
+ * @vcpu:	pointer to the vcpu performing the access
+ * @run:	pointer to the kvm_run structure
+ * @mmio:	pointer to the data describing the access
+ *
+ * returns true if the MMIO access has been performed in kernel space,
+ * and false if it needs to be emulated in user space.
+ */
+static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
+		struct kvm_exit_mmio *mmio)
+{
+	int ret;
+	if (mmio->is_write) {
+		ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
+				mmio->len, &mmio->data);
+
+	} else {
+		ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
+				mmio->len, &mmio->data);
+	}
+	if (!ret) {
+		kvm_prepare_mmio(run, mmio);
+		kvm_handle_mmio_return(vcpu, run);
+	}
+
+	return !ret;
+}
+
 int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		 phys_addr_t fault_ipa)
 {
@@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	if (vgic_handle_mmio(vcpu, run, &mmio))
 		return 1;
 
+	if (handle_kernel_mmio(vcpu, run, &mmio))
+		return 1;
+
 	kvm_prepare_mmio(run, &mmio);
 	return 0;
 }
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 8ca405c..afdecc3 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -849,13 +849,16 @@ struct mmio_range *find_matching_range(const struct mmio_range *ranges,
 }
 
 /**
- * vgic_handle_mmio - handle an in-kernel MMIO access
+ * vgic_handle_mmio - handle an in-kernel vgic MMIO access
  * @vcpu:	pointer to the vcpu performing the access
  * @run:	pointer to the kvm_run structure
  * @mmio:	pointer to the data describing the access
  *
  * returns true if the MMIO access has been performed in kernel space,
  * and false if it needs to be emulated in user space.
+ *
+ * This is handled outside of kvm_handle_mmio because the kvm_io_bus only
+ * passes the VM pointer, while we need the VCPU performing the access.
  */
 bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		      struct kvm_exit_mmio *mmio)
-- 
1.8.3.2

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Christoffer Dall]

On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote:
> On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
> handle the MMIO access through any registered read/write callbacks. This
> is a dependency for eventfd support (ioeventfd and irqfd).
> 
> However, accesses to the VGIC are still left implemented independently,
> since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---
>  arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic.c |  5 ++++-
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 4cb5a93..1d17831 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	return 0;
>  }
>  
> +/**
> + * kvm_handle_mmio - handle an in-kernel MMIO access
> + * @vcpu:	pointer to the vcpu performing the access
> + * @run:	pointer to the kvm_run structure
> + * @mmio:	pointer to the data describing the access
> + *
> + * returns true if the MMIO access has been performed in kernel space,
> + * and false if it needs to be emulated in user space.
> + */
> +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> +		struct kvm_exit_mmio *mmio)
> +{
> +	int ret;
> +	if (mmio->is_write) {
> +		ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
> +				mmio->len, &mmio->data);
> +
> +	} else {
> +		ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
> +				mmio->len, &mmio->data);
> +	}
> +	if (!ret) {
> +		kvm_prepare_mmio(run, mmio);
> +		kvm_handle_mmio_return(vcpu, run);
> +	}
> +
> +	return !ret;
> +}
> +
>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		 phys_addr_t fault_ipa)
>  {
> @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  	if (vgic_handle_mmio(vcpu, run, &mmio))
>  		return 1;
>  
> +	if (handle_kernel_mmio(vcpu, run, &mmio))
> +		return 1;
> +

this special-casing of the vgic is now really terrible.  Is there
anything holding you back from doing the necessary restructure of the
kvm_bus_io_*() API instead?  That would allow us to get rid of the ugly
Fix it! in the vgic driver as well.

-Christoffer

>  	kvm_prepare_mmio(run, &mmio);
>  	return 0;
>  }
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 8ca405c..afdecc3 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -849,13 +849,16 @@ struct mmio_range *find_matching_range(const struct mmio_range *ranges,
>  }
>  
>  /**
> - * vgic_handle_mmio - handle an in-kernel MMIO access
> + * vgic_handle_mmio - handle an in-kernel vgic MMIO access
>   * @vcpu:	pointer to the vcpu performing the access
>   * @run:	pointer to the kvm_run structure
>   * @mmio:	pointer to the data describing the access
>   *
>   * returns true if the MMIO access has been performed in kernel space,
>   * and false if it needs to be emulated in user space.
> + *
> + * This is handled outside of kvm_handle_mmio because the kvm_io_bus only
> + * passes the VM pointer, while we need the VCPU performing the access.
>   */
>  bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		      struct kvm_exit_mmio *mmio)
> -- 
> 1.8.3.2
> 

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Nikolay Nikolaev]

Hello,

On Fri, Mar 28, 2014 at 9:09 PM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
>
> On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote:
> > On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
> > handle the MMIO access through any registered read/write callbacks. This
> > is a dependency for eventfd support (ioeventfd and irqfd).
> >
> > However, accesses to the VGIC are still left implemented independently,
> > since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > ---
> >  arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
> >  virt/kvm/arm/vgic.c |  5 ++++-
> >  2 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> > index 4cb5a93..1d17831 100644
> > --- a/arch/arm/kvm/mmio.c
> > +++ b/arch/arm/kvm/mmio.c
> > @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >       return 0;
> >  }
> >
> > +/**
> > + * kvm_handle_mmio - handle an in-kernel MMIO access
> > + * @vcpu:    pointer to the vcpu performing the access
> > + * @run:     pointer to the kvm_run structure
> > + * @mmio:    pointer to the data describing the access
> > + *
> > + * returns true if the MMIO access has been performed in kernel space,
> > + * and false if it needs to be emulated in user space.
> > + */
> > +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > +             struct kvm_exit_mmio *mmio)
> > +{
> > +     int ret;
> > +     if (mmio->is_write) {
> > +             ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
> > +                             mmio->len, &mmio->data);
> > +
> > +     } else {
> > +             ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
> > +                             mmio->len, &mmio->data);
> > +     }
> > +     if (!ret) {
> > +             kvm_prepare_mmio(run, mmio);
> > +             kvm_handle_mmio_return(vcpu, run);
> > +     }
> > +
> > +     return !ret;
> > +}
> > +
> >  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >                phys_addr_t fault_ipa)
> >  {
> > @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >       if (vgic_handle_mmio(vcpu, run, &mmio))
> >               return 1;
> >
> > +     if (handle_kernel_mmio(vcpu, run, &mmio))
> > +             return 1;
> > +


We're reconsidering ioeventfds patchseries and we tried to evaluate
what you suggested here.

>
> this special-casing of the vgic is now really terrible.  Is there
> anything holding you back from doing the necessary restructure of the
> kvm_bus_io_*() API instead?

Restructuring the kvm_io_bus_ API is not a big thing (we actually did
it), but is not directly related to the these patches.
Of course it can be justified if we do it in the context of removing
vgic_handle_mmio and leaving only handle_kernel_mmio.

>
> That would allow us to get rid of the ugly
> Fix it! in the vgic driver as well.

Going through the vgic_handle_mmio we see that it will require large
refactoring:
 - there are 15 MMIO ranges for the vgic now - each should be
registered as a separate device
 - the handler of each range should be split into read and write
 - all handlers take 'struct kvm_exit_mmio', and pass it to
'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'

To sum up - if we do this refactoring of vgic's MMIO handling +
kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
vgic code and as a bonus we'll get 'ioeventfd' capabilities.

We have 3 questions:
 - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
architectures too?
 - is this huge vgic MMIO handling redesign acceptable/desired (it
touches a lot of code)?
 - is there a way that ioeventfd is accepted leaving vgic.c in it's
current state?

regards,
Nikolay NIkolaev
Virtual Open Systems

>
> -Christoffer
>
> >       kvm_prepare_mmio(run, &mmio);
> >       return 0;
> >  }
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 8ca405c..afdecc3 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -849,13 +849,16 @@ struct mmio_range *find_matching_range(const struct mmio_range *ranges,
> >  }
> >
> >  /**
> > - * vgic_handle_mmio - handle an in-kernel MMIO access
> > + * vgic_handle_mmio - handle an in-kernel vgic MMIO access
> >   * @vcpu:    pointer to the vcpu performing the access
> >   * @run:     pointer to the kvm_run structure
> >   * @mmio:    pointer to the data describing the access
> >   *
> >   * returns true if the MMIO access has been performed in kernel space,
> >   * and false if it needs to be emulated in user space.
> > + *
> > + * This is handled outside of kvm_handle_mmio because the kvm_io_bus only
> > + * passes the VM pointer, while we need the VCPU performing the access.
> >   */
> >  bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >                     struct kvm_exit_mmio *mmio)
> > --
> > 1.8.3.2
> >

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Christoffer Dall]

On Mon, Nov 10, 2014 at 05:09:07PM +0200, Nikolay Nikolaev wrote:
> Hello,
> 
> On Fri, Mar 28, 2014 at 9:09 PM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> >
> > On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote:
> > > On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
> > > handle the MMIO access through any registered read/write callbacks. This
> > > is a dependency for eventfd support (ioeventfd and irqfd).
> > >
> > > However, accesses to the VGIC are still left implemented independently,
> > > since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.
> > >
> > > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > > ---
> > >  arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
> > >  virt/kvm/arm/vgic.c |  5 ++++-
> > >  2 files changed, 36 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> > > index 4cb5a93..1d17831 100644
> > > --- a/arch/arm/kvm/mmio.c
> > > +++ b/arch/arm/kvm/mmio.c
> > > @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> > >       return 0;
> > >  }
> > >
> > > +/**
> > > + * kvm_handle_mmio - handle an in-kernel MMIO access
> > > + * @vcpu:    pointer to the vcpu performing the access
> > > + * @run:     pointer to the kvm_run structure
> > > + * @mmio:    pointer to the data describing the access
> > > + *
> > > + * returns true if the MMIO access has been performed in kernel space,
> > > + * and false if it needs to be emulated in user space.
> > > + */
> > > +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > > +             struct kvm_exit_mmio *mmio)
> > > +{
> > > +     int ret;
> > > +     if (mmio->is_write) {
> > > +             ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
> > > +                             mmio->len, &mmio->data);
> > > +
> > > +     } else {
> > > +             ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
> > > +                             mmio->len, &mmio->data);
> > > +     }
> > > +     if (!ret) {
> > > +             kvm_prepare_mmio(run, mmio);
> > > +             kvm_handle_mmio_return(vcpu, run);
> > > +     }
> > > +
> > > +     return !ret;
> > > +}
> > > +
> > >  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > >                phys_addr_t fault_ipa)
> > >  {
> > > @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > >       if (vgic_handle_mmio(vcpu, run, &mmio))
> > >               return 1;
> > >
> > > +     if (handle_kernel_mmio(vcpu, run, &mmio))
> > > +             return 1;
> > > +
> 
> 
> We're reconsidering ioeventfds patchseries and we tried to evaluate
> what you suggested here.
> 
> >
> > this special-casing of the vgic is now really terrible.  Is there
> > anything holding you back from doing the necessary restructure of the
> > kvm_bus_io_*() API instead?
> 
> Restructuring the kvm_io_bus_ API is not a big thing (we actually did
> it), but is not directly related to the these patches.
> Of course it can be justified if we do it in the context of removing
> vgic_handle_mmio and leaving only handle_kernel_mmio.
> 
> >
> > That would allow us to get rid of the ugly
> > Fix it! in the vgic driver as well.
> 
> Going through the vgic_handle_mmio we see that it will require large
> refactoring:
>  - there are 15 MMIO ranges for the vgic now - each should be
> registered as a separate device
>  - the handler of each range should be split into read and write
>  - all handlers take 'struct kvm_exit_mmio', and pass it to
> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
> 
> To sum up - if we do this refactoring of vgic's MMIO handling +
> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
> 
> We have 3 questions:
>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
> architectures too?
>  - is this huge vgic MMIO handling redesign acceptable/desired (it
> touches a lot of code)?
>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
> current state?
> 
Not sure how the latter question is relevant to this, but check with
Andre who recently looked at this as well and decided that for GICv3 the
only sane thing was to remove that comment for the gic.

I don't recall the details of what you were trying to accomplish here
(it's been 8 months or so) but the surely the vgic handling code should
*somehow* be integrated into the handle_kernel_mmio (like Paolo
suggested), unless you come back and tell me that that would involve a
complete rewrite of the vgic code.

-Christoffer

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Nikolay Nikolaev]

On Mon, Nov 10, 2014 at 6:27 PM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Mon, Nov 10, 2014 at 05:09:07PM +0200, Nikolay Nikolaev wrote:
>> Hello,
>>
>> On Fri, Mar 28, 2014 at 9:09 PM, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>> >
>> > On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote:
>> > > On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
>> > > handle the MMIO access through any registered read/write callbacks. This
>> > > is a dependency for eventfd support (ioeventfd and irqfd).
>> > >
>> > > However, accesses to the VGIC are still left implemented independently,
>> > > since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.
>> > >
>> > > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> > > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>> > > ---
>> > >  arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
>> > >  virt/kvm/arm/vgic.c |  5 ++++-
>> > >  2 files changed, 36 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>> > > index 4cb5a93..1d17831 100644
>> > > --- a/arch/arm/kvm/mmio.c
>> > > +++ b/arch/arm/kvm/mmio.c
>> > > @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>> > >       return 0;
>> > >  }
>> > >
>> > > +/**
>> > > + * kvm_handle_mmio - handle an in-kernel MMIO access
>> > > + * @vcpu:    pointer to the vcpu performing the access
>> > > + * @run:     pointer to the kvm_run structure
>> > > + * @mmio:    pointer to the data describing the access
>> > > + *
>> > > + * returns true if the MMIO access has been performed in kernel space,
>> > > + * and false if it needs to be emulated in user space.
>> > > + */
>> > > +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>> > > +             struct kvm_exit_mmio *mmio)
>> > > +{
>> > > +     int ret;
>> > > +     if (mmio->is_write) {
>> > > +             ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>> > > +                             mmio->len, &mmio->data);
>> > > +
>> > > +     } else {
>> > > +             ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>> > > +                             mmio->len, &mmio->data);
>> > > +     }
>> > > +     if (!ret) {
>> > > +             kvm_prepare_mmio(run, mmio);
>> > > +             kvm_handle_mmio_return(vcpu, run);
>> > > +     }
>> > > +
>> > > +     return !ret;
>> > > +}
>> > > +
>> > >  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>> > >                phys_addr_t fault_ipa)
>> > >  {
>> > > @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>> > >       if (vgic_handle_mmio(vcpu, run, &mmio))
>> > >               return 1;
>> > >
>> > > +     if (handle_kernel_mmio(vcpu, run, &mmio))
>> > > +             return 1;
>> > > +
>>
>>
>> We're reconsidering ioeventfds patchseries and we tried to evaluate
>> what you suggested here.
>>
>> >
>> > this special-casing of the vgic is now really terrible.  Is there
>> > anything holding you back from doing the necessary restructure of the
>> > kvm_bus_io_*() API instead?
>>
>> Restructuring the kvm_io_bus_ API is not a big thing (we actually did
>> it), but is not directly related to the these patches.
>> Of course it can be justified if we do it in the context of removing
>> vgic_handle_mmio and leaving only handle_kernel_mmio.
>>
>> >
>> > That would allow us to get rid of the ugly
>> > Fix it! in the vgic driver as well.
>>
>> Going through the vgic_handle_mmio we see that it will require large
>> refactoring:
>>  - there are 15 MMIO ranges for the vgic now - each should be
>> registered as a separate device
>>  - the handler of each range should be split into read and write
>>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>>
>> To sum up - if we do this refactoring of vgic's MMIO handling +
>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>>
>> We have 3 questions:
>>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>> architectures too?
>>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>> touches a lot of code)?
>>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>> current state?
>>
> Not sure how the latter question is relevant to this, but check with
> Andre who recently looked at this as well and decided that for GICv3 the
> only sane thing was to remove that comment for the gic.
@Andre - what's your experience with the GICv3 and MMIO handling,
anything specific?
>
> I don't recall the details of what you were trying to accomplish here
> (it's been 8 months or so) but the surely the vgic handling code should
> *somehow* be integrated into the handle_kernel_mmio (like Paolo
> suggested), unless you come back and tell me that that would involve a
> complete rewrite of the vgic code.
I'm experimenting now - it's not exactly rewrite of whole vgic code,
but it will touch a lot of it  - all MMIO access handlers and the
supporting functions.
We're ready to spend the effort. My question is  - is this acceptable?

regards,
Nikolay Nikolaev
Virtual Open Systems
>
> -Christoffer

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Christoffer Dall]

On Thu, Nov 13, 2014 at 12:45:42PM +0200, Nikolay Nikolaev wrote:

[...]

> >>
> >> Going through the vgic_handle_mmio we see that it will require large
> >> refactoring:
> >>  - there are 15 MMIO ranges for the vgic now - each should be
> >> registered as a separate device
> >>  - the handler of each range should be split into read and write
> >>  - all handlers take 'struct kvm_exit_mmio', and pass it to
> >> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
> >>
> >> To sum up - if we do this refactoring of vgic's MMIO handling +
> >> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
> >> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
> >>
> >> We have 3 questions:
> >>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
> >> architectures too?
> >>  - is this huge vgic MMIO handling redesign acceptable/desired (it
> >> touches a lot of code)?
> >>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
> >> current state?
> >>
> > Not sure how the latter question is relevant to this, but check with
> > Andre who recently looked at this as well and decided that for GICv3 the
> > only sane thing was to remove that comment for the gic.
> @Andre - what's your experience with the GICv3 and MMIO handling,
> anything specific?
> >
> > I don't recall the details of what you were trying to accomplish here
> > (it's been 8 months or so) but the surely the vgic handling code should
> > *somehow* be integrated into the handle_kernel_mmio (like Paolo
> > suggested), unless you come back and tell me that that would involve a
> > complete rewrite of the vgic code.
> I'm experimenting now - it's not exactly rewrite of whole vgic code,
> but it will touch a lot of it  - all MMIO access handlers and the
> supporting functions.
> We're ready to spend the effort. My question is  - is this acceptable?
> 
I certainly appreciate the offer to do this work, but it's hard to say
at this point if it is worth it.

What I was trying to say above is that Andre looked at this, and came to
the conclusion that it is not worth it.

Marc, what are your thoughts?

-Christoffer

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Christoffer Dall]

[resending to Andre's actual e-mail address]

On Thu, Nov 13, 2014 at 12:20 PM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Thu, Nov 13, 2014 at 12:45:42PM +0200, Nikolay Nikolaev wrote:
>
> [...]
>
>> >>
>> >> Going through the vgic_handle_mmio we see that it will require large
>> >> refactoring:
>> >>  - there are 15 MMIO ranges for the vgic now - each should be
>> >> registered as a separate device
>> >>  - the handler of each range should be split into read and write
>> >>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>> >> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>> >>
>> >> To sum up - if we do this refactoring of vgic's MMIO handling +
>> >> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>> >> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>> >>
>> >> We have 3 questions:
>> >>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>> >> architectures too?
>> >>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>> >> touches a lot of code)?
>> >>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>> >> current state?
>> >>
>> > Not sure how the latter question is relevant to this, but check with
>> > Andre who recently looked at this as well and decided that for GICv3 the
>> > only sane thing was to remove that comment for the gic.
>> @Andre - what's your experience with the GICv3 and MMIO handling,
>> anything specific?
>> >
>> > I don't recall the details of what you were trying to accomplish here
>> > (it's been 8 months or so) but the surely the vgic handling code should
>> > *somehow* be integrated into the handle_kernel_mmio (like Paolo
>> > suggested), unless you come back and tell me that that would involve a
>> > complete rewrite of the vgic code.
>> I'm experimenting now - it's not exactly rewrite of whole vgic code,
>> but it will touch a lot of it  - all MMIO access handlers and the
>> supporting functions.
>> We're ready to spend the effort. My question is  - is this acceptable?
>>
> I certainly appreciate the offer to do this work, but it's hard to say
> at this point if it is worth it.
>
> What I was trying to say above is that Andre looked at this, and came to
> the conclusion that it is not worth it.
>
> Marc, what are your thoughts?
>
> -Christoffer

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Andre Przywara]

Hi Nikolay,

On 13/11/14 11:37, Marc Zyngier wrote:
> [fixing Andre's email address]
> 
> On 13/11/14 11:20, Christoffer Dall wrote:
>> On Thu, Nov 13, 2014 at 12:45:42PM +0200, Nikolay Nikolaev wrote:
>>
>> [...]
>>
>>>>>
>>>>> Going through the vgic_handle_mmio we see that it will require large
>>>>> refactoring:
>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>>>> registered as a separate device
>>>>>  - the handler of each range should be split into read and write
>>>>>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>>>>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>>>>>
>>>>> To sum up - if we do this refactoring of vgic's MMIO handling +
>>>>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>>>>> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>>>>>
>>>>> We have 3 questions:
>>>>>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>>>>> architectures too?
>>>>>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>>>>> touches a lot of code)?
>>>>>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>>>>> current state?
>>>>>
>>>> Not sure how the latter question is relevant to this, but check with
>>>> Andre who recently looked at this as well and decided that for GICv3 the
>>>> only sane thing was to remove that comment for the gic.
>>> @Andre - what's your experience with the GICv3 and MMIO handling,
>>> anything specific?
>>>>
>>>> I don't recall the details of what you were trying to accomplish here
>>>> (it's been 8 months or so) but the surely the vgic handling code should
>>>> *somehow* be integrated into the handle_kernel_mmio (like Paolo
>>>> suggested), unless you come back and tell me that that would involve a
>>>> complete rewrite of the vgic code.
>>> I'm experimenting now - it's not exactly rewrite of whole vgic code,
>>> but it will touch a lot of it  - all MMIO access handlers and the
>>> supporting functions.
>>> We're ready to spend the effort. My question is  - is this acceptable?
>>>
>> I certainly appreciate the offer to do this work, but it's hard to say
>> at this point if it is worth it.
>>
>> What I was trying to say above is that Andre looked at this, and came to
>> the conclusion that it is not worth it.
>>
>> Marc, what are your thoughts?
> 
> Same here, I rely on Andre's view that it was not very useful. Now, it
> would be good to see a mock-up of the patches and find out:

Seconded, can you send a pointer to the VGIC rework patches mentioned?

> - if it is a major improvement for the general quality of the code
> - if that allow us to *delete* a lot of code (if it is just churn, I'm
> not really interested)
> - if it helps or hinders further developments that are currently in the
> pipeline
> 
> Andre, can you please share your findings? I don't remember the
> specifics of the discussion we had a few months ago...

1) Given the date in the reply I sense that your patches are from March
this year or earlier. So this is based on VGIC code from March, which
predates Marc's vgic_dyn changes that just went in 3.18-rc1? His patches
introduced another member to struct mmio_range to check validity of
accesses with a reduced number of SPIs supported (.bits_per_irq).
So is this covered in your rework?

2)
>>>  - there are 15 MMIO ranges for the vgic now - each should be

Well, the GICv3 emulation adds 41 new ranges. Not sure if this still fits.

>>> registered as a separate device

I found this fact a show-stopper when looking at this a month ago.
Somehow it feels wrong to register a bunch of pseudo-devices. I could go
with registering a small number of regions (one distributor, two
redistributor regions for instance), but not handling every single of
the 41 + 15 register "groups" as a device.

Also I wasn't sure if we had to expose some of the vGIC structures to
the other KVM code layers.

But I am open to any suggestions (as long as they go in _after_ my
vGICv3 series ;-)  - so looking forward to some repo to see how it looks
like.

Cheers,
Andre.

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Nikolay Nikolaev]

On Thu, Nov 13, 2014 at 1:52 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi Nikolay,
>
> On 13/11/14 11:37, Marc Zyngier wrote:
>> [fixing Andre's email address]
>>
>> On 13/11/14 11:20, Christoffer Dall wrote:
>>> On Thu, Nov 13, 2014 at 12:45:42PM +0200, Nikolay Nikolaev wrote:
>>>
>>> [...]
>>>
>>>>>>
>>>>>> Going through the vgic_handle_mmio we see that it will require large
>>>>>> refactoring:
>>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>>>>> registered as a separate device
>>>>>>  - the handler of each range should be split into read and write
>>>>>>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>>>>>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>>>>>>
>>>>>> To sum up - if we do this refactoring of vgic's MMIO handling +
>>>>>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>>>>>> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>>>>>>
>>>>>> We have 3 questions:
>>>>>>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>>>>>> architectures too?
>>>>>>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>>>>>> touches a lot of code)?
>>>>>>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>>>>>> current state?
>>>>>>
>>>>> Not sure how the latter question is relevant to this, but check with
>>>>> Andre who recently looked at this as well and decided that for GICv3 the
>>>>> only sane thing was to remove that comment for the gic.
>>>> @Andre - what's your experience with the GICv3 and MMIO handling,
>>>> anything specific?
>>>>>
>>>>> I don't recall the details of what you were trying to accomplish here
>>>>> (it's been 8 months or so) but the surely the vgic handling code should
>>>>> *somehow* be integrated into the handle_kernel_mmio (like Paolo
>>>>> suggested), unless you come back and tell me that that would involve a
>>>>> complete rewrite of the vgic code.
>>>> I'm experimenting now - it's not exactly rewrite of whole vgic code,
>>>> but it will touch a lot of it  - all MMIO access handlers and the
>>>> supporting functions.
>>>> We're ready to spend the effort. My question is  - is this acceptable?
>>>>
>>> I certainly appreciate the offer to do this work, but it's hard to say
>>> at this point if it is worth it.
>>>
>>> What I was trying to say above is that Andre looked at this, and came to
>>> the conclusion that it is not worth it.
>>>
>>> Marc, what are your thoughts?
>>
>> Same here, I rely on Andre's view that it was not very useful. Now, it
>> would be good to see a mock-up of the patches and find out:
>
> Seconded, can you send a pointer to the VGIC rework patches mentioned?
They are still in WiP state - not exactly working. I'm still exploring
what the status is.

Our major target is having ioeventfd suport in ARM. For this we need
to support kvm_io_bus_ mechanisms for MMIO access (cause ioevent fd
device is registered this way). Then this subject of integrating vgic
with the kvm_io_bus_ APIs came up.
My personal opinion - they should be able to coexist in peace.

>
>> - if it is a major improvement for the general quality of the code
>> - if that allow us to *delete* a lot of code (if it is just churn, I'm
>> not really interested)
>> - if it helps or hinders further developments that are currently in the
>> pipeline
>>
>> Andre, can you please share your findings? I don't remember the
>> specifics of the discussion we had a few months ago...
>
> 1) Given the date in the reply I sense that your patches are from March
> this year or earlier. So this is based on VGIC code from March, which
> predates Marc's vgic_dyn changes that just went in 3.18-rc1? His patches
> introduced another member to struct mmio_range to check validity of
> accesses with a reduced number of SPIs supported (.bits_per_irq).
> So is this covered in your rework?
Still no (rebased to 3.17) - didn't see it, but should not be an issue.
>
> 2)
>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>
> Well, the GICv3 emulation adds 41 new ranges. Not sure if this still fits.
>
>>>> registered as a separate device
>
> I found this fact a show-stopper when looking at this a month ago.
> Somehow it feels wrong to register a bunch of pseudo-devices. I could go
> with registering a small number of regions (one distributor, two
> redistributor regions for instance), but not handling every single of
> the 41 + 15 register "groups" as a device.
Do you sense performance issues, or just "it's not right"?
Maybe kvm_io_bus_ needs some extesion to hanlde a device with multiple regions?

>
> Also I wasn't sure if we had to expose some of the vGIC structures to
> the other KVM code layers.
I don't see such a need. Can you point some example?
>
> But I am open to any suggestions (as long as they go in _after_ my
> vGICv3 series ;-)  - so looking forward to some repo to see how it looks
> like.
There is still nothing much to show - but if there is interest we may
prepare something that shows the idea.
BTW, where is your repo (sorry I don't follow so close) with the vGICv3?

>
> Cheers,
> Andre.

regards,
Nikolay Nikolaev

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Andre Przywara]

Hi Nikolay,

On 13/11/14 12:29, Nikolay Nikolaev wrote:
> On Thu, Nov 13, 2014 at 1:52 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> Hi Nikolay,
>>
>> On 13/11/14 11:37, Marc Zyngier wrote:
>>> [fixing Andre's email address]
>>>
>>> On 13/11/14 11:20, Christoffer Dall wrote:
>>>> On Thu, Nov 13, 2014 at 12:45:42PM +0200, Nikolay Nikolaev wrote:
>>>>
>>>> [...]
>>>>
>>>>>>>
>>>>>>> Going through the vgic_handle_mmio we see that it will require large
>>>>>>> refactoring:
>>>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>>>>>> registered as a separate device
>>>>>>>  - the handler of each range should be split into read and write
>>>>>>>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>>>>>>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>>>>>>>
>>>>>>> To sum up - if we do this refactoring of vgic's MMIO handling +
>>>>>>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>>>>>>> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>>>>>>>
>>>>>>> We have 3 questions:
>>>>>>>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>>>>>>> architectures too?
>>>>>>>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>>>>>>> touches a lot of code)?
>>>>>>>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>>>>>>> current state?
>>>>>>>
>>>>>> Not sure how the latter question is relevant to this, but check with
>>>>>> Andre who recently looked at this as well and decided that for GICv3 the
>>>>>> only sane thing was to remove that comment for the gic.
>>>>> @Andre - what's your experience with the GICv3 and MMIO handling,
>>>>> anything specific?
>>>>>>
>>>>>> I don't recall the details of what you were trying to accomplish here
>>>>>> (it's been 8 months or so) but the surely the vgic handling code should
>>>>>> *somehow* be integrated into the handle_kernel_mmio (like Paolo
>>>>>> suggested), unless you come back and tell me that that would involve a
>>>>>> complete rewrite of the vgic code.
>>>>> I'm experimenting now - it's not exactly rewrite of whole vgic code,
>>>>> but it will touch a lot of it  - all MMIO access handlers and the
>>>>> supporting functions.
>>>>> We're ready to spend the effort. My question is  - is this acceptable?
>>>>>
>>>> I certainly appreciate the offer to do this work, but it's hard to say
>>>> at this point if it is worth it.
>>>>
>>>> What I was trying to say above is that Andre looked at this, and came to
>>>> the conclusion that it is not worth it.
>>>>
>>>> Marc, what are your thoughts?
>>>
>>> Same here, I rely on Andre's view that it was not very useful. Now, it
>>> would be good to see a mock-up of the patches and find out:
>>
>> Seconded, can you send a pointer to the VGIC rework patches mentioned?
> They are still in WiP state - not exactly working. I'm still exploring
> what the status is.
> 
> Our major target is having ioeventfd suport in ARM. For this we need
> to support kvm_io_bus_ mechanisms for MMIO access (cause ioevent fd
> device is registered this way). Then this subject of integrating vgic
> with the kvm_io_bus_ APIs came up.
> My personal opinion - they should be able to coexist in peace.
> 
>>
>>> - if it is a major improvement for the general quality of the code
>>> - if that allow us to *delete* a lot of code (if it is just churn, I'm
>>> not really interested)
>>> - if it helps or hinders further developments that are currently in the
>>> pipeline
>>>
>>> Andre, can you please share your findings? I don't remember the
>>> specifics of the discussion we had a few months ago...
>>
>> 1) Given the date in the reply I sense that your patches are from March
>> this year or earlier. So this is based on VGIC code from March, which
>> predates Marc's vgic_dyn changes that just went in 3.18-rc1? His patches
>> introduced another member to struct mmio_range to check validity of
>> accesses with a reduced number of SPIs supported (.bits_per_irq).
>> So is this covered in your rework?
> Still no (rebased to 3.17) - didn't see it, but should not be an issue.
>>
>> 2)
>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>
>> Well, the GICv3 emulation adds 41 new ranges. Not sure if this still fits.
>>
>>>>> registered as a separate device
>>
>> I found this fact a show-stopper when looking at this a month ago.
>> Somehow it feels wrong to register a bunch of pseudo-devices. I could go
>> with registering a small number of regions (one distributor, two
>> redistributor regions for instance), but not handling every single of
>> the 41 + 15 register "groups" as a device.
> Do you sense performance issues, or just "it's not right"?

Just "not right", since they are no 15 devices for a GICv2 emulation.

> Maybe kvm_io_bus_ needs some extesion to hanlde a device with multiple regions?

Well, maybe a simple rename could fix this, but I am not sure it still
fits then. I am just afraid we end up with quite some code duplication
in each handler function. Also if we needed to split-up read and write
this ends up with much more code. Currently this is cleverly handled in
one function without looking messy (great job, Marc, btw!)

Also handling private GICv3 interrupts is no longer done via a single
MMIO offset banked by the accessing (v)CPU, but by per-CPU MMIO regions.
Would we need to register separate devices for each VCPU then?
Which would multiply the 41 "devices" by the number of VCPUs?

>> Also I wasn't sure if we had to expose some of the vGIC structures to
>> the other KVM code layers.
> I don't see such a need. Can you point some example?

No, was just a feeling. Currently we are happily confined to
virt/kvm/arm and the interface to the generic and arch KVM code is
pretty small. I was just afraid that would have to be extended. But if
you say it's fine, then it's fine.

>> But I am open to any suggestions (as long as they go in _after_ my
>> vGICv3 series ;-)  - so looking forward to some repo to see how it looks
>> like.
> There is still nothing much to show - but if there is interest we may
> prepare something that shows the idea.

Yeah, just some dump of the vgic.c would suffice. Or maybe even an
example implementation of one or two registers to see how it compares to
the current code.

I just get the feeling that GICv2 emulation would be fine with this
refactoring, but it wouldn't fit anymore for GICv3 without hacks.

> BTW, where is your repo (sorry I don't follow so close) with the vGICv3?

It's on: http://www.linux-arm.org/git?p=linux-ap.git
Check the latest kvm-gicv3 branch.

Thanks,
Andre.

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Nikolay Nikolaev]

On Thu, Nov 13, 2014 at 4:23 PM, Eric Auger <eric.auger@linaro.org> wrote:
> On 11/13/2014 03:16 PM, Eric Auger wrote:
>> On 11/13/2014 11:45 AM, Nikolay Nikolaev wrote:
>>> On Mon, Nov 10, 2014 at 6:27 PM, Christoffer Dall
>>> <christoffer.dall@linaro.org> wrote:
>>>> On Mon, Nov 10, 2014 at 05:09:07PM +0200, Nikolay Nikolaev wrote:
>>>>> Hello,
>>>>>
>>>>> On Fri, Mar 28, 2014 at 9:09 PM, Christoffer Dall
>>>>> <christoffer.dall@linaro.org> wrote:
>>>>>>
>>>>>> On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote:
>>>>>>> On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
>>>>>>> handle the MMIO access through any registered read/write callbacks. This
>>>>>>> is a dependency for eventfd support (ioeventfd and irqfd).
>>>>>>>
>>>>>>> However, accesses to the VGIC are still left implemented independently,
>>>>>>> since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.
>>>>>>>
>>>>>>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>>>>>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>>>>>> ---
>>>>>>>  arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>>  virt/kvm/arm/vgic.c |  5 ++++-
>>>>>>>  2 files changed, 36 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>>>>> index 4cb5a93..1d17831 100644
>>>>>>> --- a/arch/arm/kvm/mmio.c
>>>>>>> +++ b/arch/arm/kvm/mmio.c
>>>>>>> @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>>>>       return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * kvm_handle_mmio - handle an in-kernel MMIO access
>>>>>>> + * @vcpu:    pointer to the vcpu performing the access
>>>>>>> + * @run:     pointer to the kvm_run structure
>>>>>>> + * @mmio:    pointer to the data describing the access
>>>>>>> + *
>>>>>>> + * returns true if the MMIO access has been performed in kernel space,
>>>>>>> + * and false if it needs to be emulated in user space.
>>>>>>> + */
>>>>>>> +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>> +             struct kvm_exit_mmio *mmio)
>>>>>>> +{
>>>>>>> +     int ret;
>>>>>>> +     if (mmio->is_write) {
>>>>>>> +             ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>>>>>>> +                             mmio->len, &mmio->data);
>>>>>>> +
>>>>>>> +     } else {
>>>>>>> +             ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>>>>>>> +                             mmio->len, &mmio->data);
>>>>>>> +     }
>>>>>>> +     if (!ret) {
>>>>>>> +             kvm_prepare_mmio(run, mmio);
>>>>>>> +             kvm_handle_mmio_return(vcpu, run);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     return !ret;
>>>>>>> +}
>>>>>>> +
>>>>>>>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>                phys_addr_t fault_ipa)
>>>>>>>  {
>>>>>>> @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>       if (vgic_handle_mmio(vcpu, run, &mmio))
>>>>>>>               return 1;
>>>>>>>
>>>>>>> +     if (handle_kernel_mmio(vcpu, run, &mmio))
>>>>>>> +             return 1;
>>>>>>> +
>>>>>
>>>>>
>>>>> We're reconsidering ioeventfds patchseries and we tried to evaluate
>>>>> what you suggested here.
>>>>>
>>>>>>
>>>>>> this special-casing of the vgic is now really terrible.  Is there
>>>>>> anything holding you back from doing the necessary restructure of the
>>>>>> kvm_bus_io_*() API instead?
>>>>>
>>>>> Restructuring the kvm_io_bus_ API is not a big thing (we actually did
>>>>> it), but is not directly related to the these patches.
>>>>> Of course it can be justified if we do it in the context of removing
>>>>> vgic_handle_mmio and leaving only handle_kernel_mmio.
>>>>>
>>>>>>
>>>>>> That would allow us to get rid of the ugly
>>>>>> Fix it! in the vgic driver as well.
>>>>>
>>>>> Going through the vgic_handle_mmio we see that it will require large
>>>>> refactoring:
>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>>>> registered as a separate device
> Re-correcting Andre's address, sorry:
> Hi Nikolay, Andre,
>
> what does mandate to register 15 devices? Isn't possible to register a
> single kvm_io_device covering the whole distributor range [base, base +
> KVM_VGIC_V2_DIST_SIZE] (current code) and in associated
> kvm_io_device_ops read/write locate the addressed range and do the same
> as what is done in current vgic_handle_mmio? Isn't it done that way for

Well, then we'll actually get slower mmio processing. Instead of calling
vgic_handle_mmio in io_mem_abort, we'll be calling kvm_io_bus_write.
This just adds another level of translation (i.e. find the kvm_io_ device)
and the underlying vgic code will remain almost the same.

> the ioapic? what do I miss?
I looked quickly in the ioapic code, and if I get it right there are no "ranges'
like what we have with the GIC. They have this regselect/regwindow concept
and they seem to have much less "registers" to handle. GIC seems a lot more
complex in terms of MMIO interface.

regards,
Nikolay Nikolaev

>
> Thanks
>
> Best Regards
>
> Eric
>>>>>  - the handler of each range should be split into read and write
>>>>>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>>>>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>>>>>
>>>>> To sum up - if we do this refactoring of vgic's MMIO handling +
>>>>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>>>>> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>>>>>
>>>>> We have 3 questions:
>>>>>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>>>>> architectures too?
>>>>>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>>>>> touches a lot of code)?
>>>>>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>>>>> current state?
>>>>>
>>>> Not sure how the latter question is relevant to this, but check with
>>>> Andre who recently looked at this as well and decided that for GICv3 the
>>>> only sane thing was to remove that comment for the gic.
>>> @Andre - what's your experience with the GICv3 and MMIO handling,
>>> anything specific?
>>>>
>>>> I don't recall the details of what you were trying to accomplish here
>>>> (it's been 8 months or so) but the surely the vgic handling code should
>>>> *somehow* be integrated into the handle_kernel_mmio (like Paolo
>>>> suggested), unless you come back and tell me that that would involve a
>>>> complete rewrite of the vgic code.
>>> I'm experimenting now - it's not exactly rewrite of whole vgic code,
>>> but it will touch a lot of it  - all MMIO access handlers and the
>>> supporting functions.
>>> We're ready to spend the effort. My question is  - is this acceptable?
>>>
>>> regards,
>>> Nikolay Nikolaev
>>> Virtual Open Systems
>>>>
>>>> -Christoffer
>>> _______________________________________________
>>> kvmarm mailing list
>>> kvmarm@lists.cs.columbia.edu
>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>
>>
>

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Christoffer Dall]

Hi Nikolay,

On Thu, Nov 13, 2014 at 4:02 PM, Nikolay Nikolaev
<n.nikolaev@virtualopensystems.com> wrote:
[...]

>>>>>>
>>>>>>
>>>>>> We're reconsidering ioeventfds patchseries and we tried to evaluate
>>>>>> what you suggested here.
>>>>>>
>>>>>>>
>>>>>>> this special-casing of the vgic is now really terrible.  Is there
>>>>>>> anything holding you back from doing the necessary restructure of the
>>>>>>> kvm_bus_io_*() API instead?
>>>>>>
>>>>>> Restructuring the kvm_io_bus_ API is not a big thing (we actually did
>>>>>> it), but is not directly related to the these patches.
>>>>>> Of course it can be justified if we do it in the context of removing
>>>>>> vgic_handle_mmio and leaving only handle_kernel_mmio.
>>>>>>
>>>>>>>
>>>>>>> That would allow us to get rid of the ugly
>>>>>>> Fix it! in the vgic driver as well.
>>>>>>
>>>>>> Going through the vgic_handle_mmio we see that it will require large
>>>>>> refactoring:
>>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>>>>> registered as a separate device
>> Re-correcting Andre's address, sorry:
>> Hi Nikolay, Andre,
>>
>> what does mandate to register 15 devices? Isn't possible to register a
>> single kvm_io_device covering the whole distributor range [base, base +
>> KVM_VGIC_V2_DIST_SIZE] (current code) and in associated
>> kvm_io_device_ops read/write locate the addressed range and do the same
>> as what is done in current vgic_handle_mmio? Isn't it done that way for
>
> Well, then we'll actually get slower mmio processing. Instead of calling
> vgic_handle_mmio in io_mem_abort, we'll be calling kvm_io_bus_write.
> This just adds another level of translation (i.e. find the kvm_io_ device)
> and the underlying vgic code will remain almost the same.
>

Define slower please.  Have you measured this?  With my ideas about
where we are spending overhead on a world-switch in this system,
looping through a few ranges is going to be infinitesimal, but as I
said, we would need to measure it before using it as an argument to
structure the code in a certain way, unless of course we're obviously
doing O(n^2) operations or something idiotic like that.

-Christoffer

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Andre Przywara]

On 13/11/14 15:02, Nikolay Nikolaev wrote:
> On Thu, Nov 13, 2014 at 4:23 PM, Eric Auger <eric.auger@linaro.org> wrote:
>> On 11/13/2014 03:16 PM, Eric Auger wrote:
>>> On 11/13/2014 11:45 AM, Nikolay Nikolaev wrote:
>>>> On Mon, Nov 10, 2014 at 6:27 PM, Christoffer Dall
>>>> <christoffer.dall@linaro.org> wrote:
>>>>> On Mon, Nov 10, 2014 at 05:09:07PM +0200, Nikolay Nikolaev wrote:
>>>>>> Hello,
>>>>>>
>>>>>> On Fri, Mar 28, 2014 at 9:09 PM, Christoffer Dall
>>>>>> <christoffer.dall@linaro.org> wrote:
>>>>>>>
>>>>>>> On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote:
>>>>>>>> On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
>>>>>>>> handle the MMIO access through any registered read/write callbacks. This
>>>>>>>> is a dependency for eventfd support (ioeventfd and irqfd).
>>>>>>>>
>>>>>>>> However, accesses to the VGIC are still left implemented independently,
>>>>>>>> since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.
>>>>>>>>
>>>>>>>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>>>>>>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>>>>>>> ---
>>>>>>>>  arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>>>  virt/kvm/arm/vgic.c |  5 ++++-
>>>>>>>>  2 files changed, 36 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>>>>>> index 4cb5a93..1d17831 100644
>>>>>>>> --- a/arch/arm/kvm/mmio.c
>>>>>>>> +++ b/arch/arm/kvm/mmio.c
>>>>>>>> @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>>>>>       return 0;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * kvm_handle_mmio - handle an in-kernel MMIO access
>>>>>>>> + * @vcpu:    pointer to the vcpu performing the access
>>>>>>>> + * @run:     pointer to the kvm_run structure
>>>>>>>> + * @mmio:    pointer to the data describing the access
>>>>>>>> + *
>>>>>>>> + * returns true if the MMIO access has been performed in kernel space,
>>>>>>>> + * and false if it needs to be emulated in user space.
>>>>>>>> + */
>>>>>>>> +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>> +             struct kvm_exit_mmio *mmio)
>>>>>>>> +{
>>>>>>>> +     int ret;
>>>>>>>> +     if (mmio->is_write) {
>>>>>>>> +             ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>>>>>>>> +                             mmio->len, &mmio->data);
>>>>>>>> +
>>>>>>>> +     } else {
>>>>>>>> +             ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>>>>>>>> +                             mmio->len, &mmio->data);
>>>>>>>> +     }
>>>>>>>> +     if (!ret) {
>>>>>>>> +             kvm_prepare_mmio(run, mmio);
>>>>>>>> +             kvm_handle_mmio_return(vcpu, run);
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     return !ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>>                phys_addr_t fault_ipa)
>>>>>>>>  {
>>>>>>>> @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>>       if (vgic_handle_mmio(vcpu, run, &mmio))
>>>>>>>>               return 1;
>>>>>>>>
>>>>>>>> +     if (handle_kernel_mmio(vcpu, run, &mmio))
>>>>>>>> +             return 1;
>>>>>>>> +
>>>>>>
>>>>>>
>>>>>> We're reconsidering ioeventfds patchseries and we tried to evaluate
>>>>>> what you suggested here.
>>>>>>
>>>>>>>
>>>>>>> this special-casing of the vgic is now really terrible.  Is there
>>>>>>> anything holding you back from doing the necessary restructure of the
>>>>>>> kvm_bus_io_*() API instead?
>>>>>>
>>>>>> Restructuring the kvm_io_bus_ API is not a big thing (we actually did
>>>>>> it), but is not directly related to the these patches.
>>>>>> Of course it can be justified if we do it in the context of removing
>>>>>> vgic_handle_mmio and leaving only handle_kernel_mmio.
>>>>>>
>>>>>>>
>>>>>>> That would allow us to get rid of the ugly
>>>>>>> Fix it! in the vgic driver as well.
>>>>>>
>>>>>> Going through the vgic_handle_mmio we see that it will require large
>>>>>> refactoring:
>>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>>>>> registered as a separate device
>> Re-correcting Andre's address, sorry:
>> Hi Nikolay, Andre,
>>
>> what does mandate to register 15 devices? Isn't possible to register a
>> single kvm_io_device covering the whole distributor range [base, base +
>> KVM_VGIC_V2_DIST_SIZE] (current code) and in associated
>> kvm_io_device_ops read/write locate the addressed range and do the same
>> as what is done in current vgic_handle_mmio? Isn't it done that way for
> 
> Well, then we'll actually get slower mmio processing. Instead of calling
> vgic_handle_mmio in io_mem_abort, we'll be calling kvm_io_bus_write.
> This just adds another level of translation (i.e. find the kvm_io_ device)
> and the underlying vgic code will remain almost the same.

Agreed. That was one possibility I came around also, but I think it
defeats the purpose of the rework, which is mostly to get rid of the
GIC's private MMIO dispatching code, right?

But honestly I would happily sacrifice "performance" for easier VGIC
code - especially if one thinks about security for instance. Though I
think that another memory reference doesn't really matter in this
context ;-)

>> the ioapic? what do I miss?
> I looked quickly in the ioapic code, and if I get it right there are no "ranges'
> like what we have with the GIC. They have this regselect/regwindow concept
> and they seem to have much less "registers" to handle. GIC seems a lot more
> complex in terms of MMIO interface.

Right, that was my impression, too. IOAPIC isn't really comparable to
the GIC in this respect. That's why I was going away from this rework,
since I thought that the kvm_io_bus API wasn't really meant for such
beasts as the GIC.

Cheers,
Andre.

> 
> regards,
> Nikolay Nikolaev
> 
>>
>> Thanks
>>
>> Best Regards
>>
>> Eric
>>>>>>  - the handler of each range should be split into read and write
>>>>>>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>>>>>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>>>>>>
>>>>>> To sum up - if we do this refactoring of vgic's MMIO handling +
>>>>>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>>>>>> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>>>>>>
>>>>>> We have 3 questions:
>>>>>>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>>>>>> architectures too?
>>>>>>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>>>>>> touches a lot of code)?
>>>>>>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>>>>>> current state?
>>>>>>
>>>>> Not sure how the latter question is relevant to this, but check with
>>>>> Andre who recently looked at this as well and decided that for GICv3 the
>>>>> only sane thing was to remove that comment for the gic.
>>>> @Andre - what's your experience with the GICv3 and MMIO handling,
>>>> anything specific?
>>>>>
>>>>> I don't recall the details of what you were trying to accomplish here
>>>>> (it's been 8 months or so) but the surely the vgic handling code should
>>>>> *somehow* be integrated into the handle_kernel_mmio (like Paolo
>>>>> suggested), unless you come back and tell me that that would involve a
>>>>> complete rewrite of the vgic code.
>>>> I'm experimenting now - it's not exactly rewrite of whole vgic code,
>>>> but it will touch a lot of it  - all MMIO access handlers and the
>>>> supporting functions.
>>>> We're ready to spend the effort. My question is  - is this acceptable?
>>>>
>>>> regards,
>>>> Nikolay Nikolaev
>>>> Virtual Open Systems
>>>>>
>>>>> -Christoffer
>>>> _______________________________________________
>>>> kvmarm mailing list
>>>> kvmarm@lists.cs.columbia.edu
>>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>>
>>>
>>
> 

Re: [RFC PATCH 1/4] ARM: KVM: on unhandled IO mem abort, route the call to the KVM MMIO bus

[Eric Auger]

On 11/13/2014 04:31 PM, Andre Przywara wrote:
> 
> 
> On 13/11/14 15:02, Nikolay Nikolaev wrote:
>> On Thu, Nov 13, 2014 at 4:23 PM, Eric Auger <eric.auger@linaro.org> wrote:
>>> On 11/13/2014 03:16 PM, Eric Auger wrote:
>>>> On 11/13/2014 11:45 AM, Nikolay Nikolaev wrote:
>>>>> On Mon, Nov 10, 2014 at 6:27 PM, Christoffer Dall
>>>>> <christoffer.dall@linaro.org> wrote:
>>>>>> On Mon, Nov 10, 2014 at 05:09:07PM +0200, Nikolay Nikolaev wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> On Fri, Mar 28, 2014 at 9:09 PM, Christoffer Dall
>>>>>>> <christoffer.dall@linaro.org> wrote:
>>>>>>>>
>>>>>>>> On Thu, Mar 13, 2014 at 04:57:26PM +0100, Antonios Motakis wrote:
>>>>>>>>> On an unhandled IO memory abort, use the kvm_io_bus_* API in order to
>>>>>>>>> handle the MMIO access through any registered read/write callbacks. This
>>>>>>>>> is a dependency for eventfd support (ioeventfd and irqfd).
>>>>>>>>>
>>>>>>>>> However, accesses to the VGIC are still left implemented independently,
>>>>>>>>> since the kvm_io_bus_* API doesn't pass the VCPU pointer doing the access.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>>>>>>>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/kvm/mmio.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>>>>  virt/kvm/arm/vgic.c |  5 ++++-
>>>>>>>>>  2 files changed, 36 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>>>>>>> index 4cb5a93..1d17831 100644
>>>>>>>>> --- a/arch/arm/kvm/mmio.c
>>>>>>>>> +++ b/arch/arm/kvm/mmio.c
>>>>>>>>> @@ -162,6 +162,35 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>>>>>>       return 0;
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> +/**
>>>>>>>>> + * kvm_handle_mmio - handle an in-kernel MMIO access
>>>>>>>>> + * @vcpu:    pointer to the vcpu performing the access
>>>>>>>>> + * @run:     pointer to the kvm_run structure
>>>>>>>>> + * @mmio:    pointer to the data describing the access
>>>>>>>>> + *
>>>>>>>>> + * returns true if the MMIO access has been performed in kernel space,
>>>>>>>>> + * and false if it needs to be emulated in user space.
>>>>>>>>> + */
>>>>>>>>> +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>>> +             struct kvm_exit_mmio *mmio)
>>>>>>>>> +{
>>>>>>>>> +     int ret;
>>>>>>>>> +     if (mmio->is_write) {
>>>>>>>>> +             ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>>>>>>>>> +                             mmio->len, &mmio->data);
>>>>>>>>> +
>>>>>>>>> +     } else {
>>>>>>>>> +             ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, mmio->phys_addr,
>>>>>>>>> +                             mmio->len, &mmio->data);
>>>>>>>>> +     }
>>>>>>>>> +     if (!ret) {
>>>>>>>>> +             kvm_prepare_mmio(run, mmio);
>>>>>>>>> +             kvm_handle_mmio_return(vcpu, run);
>>>>>>>>> +     }
>>>>>>>>> +
>>>>>>>>> +     return !ret;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>>>                phys_addr_t fault_ipa)
>>>>>>>>>  {
>>>>>>>>> @@ -200,6 +229,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>>>>>       if (vgic_handle_mmio(vcpu, run, &mmio))
>>>>>>>>>               return 1;
>>>>>>>>>
>>>>>>>>> +     if (handle_kernel_mmio(vcpu, run, &mmio))
>>>>>>>>> +             return 1;
>>>>>>>>> +
>>>>>>>
>>>>>>>
>>>>>>> We're reconsidering ioeventfds patchseries and we tried to evaluate
>>>>>>> what you suggested here.
>>>>>>>
>>>>>>>>
>>>>>>>> this special-casing of the vgic is now really terrible.  Is there
>>>>>>>> anything holding you back from doing the necessary restructure of the
>>>>>>>> kvm_bus_io_*() API instead?
>>>>>>>
>>>>>>> Restructuring the kvm_io_bus_ API is not a big thing (we actually did
>>>>>>> it), but is not directly related to the these patches.
>>>>>>> Of course it can be justified if we do it in the context of removing
>>>>>>> vgic_handle_mmio and leaving only handle_kernel_mmio.
>>>>>>>
>>>>>>>>
>>>>>>>> That would allow us to get rid of the ugly
>>>>>>>> Fix it! in the vgic driver as well.
>>>>>>>
>>>>>>> Going through the vgic_handle_mmio we see that it will require large
>>>>>>> refactoring:
>>>>>>>  - there are 15 MMIO ranges for the vgic now - each should be
>>>>>>> registered as a separate device
>>> Re-correcting Andre's address, sorry:
>>> Hi Nikolay, Andre,
>>>
>>> what does mandate to register 15 devices? Isn't possible to register a
>>> single kvm_io_device covering the whole distributor range [base, base +
>>> KVM_VGIC_V2_DIST_SIZE] (current code) and in associated
>>> kvm_io_device_ops read/write locate the addressed range and do the same
>>> as what is done in current vgic_handle_mmio? Isn't it done that way for
>>
>> Well, then we'll actually get slower mmio processing. Instead of calling
>> vgic_handle_mmio in io_mem_abort, we'll be calling kvm_io_bus_write.
>> This just adds another level of translation (i.e. find the kvm_io_ device)
>> and the underlying vgic code will remain almost the same.
> 
> Agreed. That was one possibility I came around also, but I think it
> defeats the purpose of the rework, which is mostly to get rid of the
> GIC's private MMIO dispatching code, right?
Hi Andre, Nikolay,

Well my feeling is one of your primary goal was to split in a clean way
v2 and v3 MMIO regions. This may be achieved by creating either
io_device. Also in the future don't we have ITS &  redistributors
showing up that could match io_devices too - but all that stuff still
are very fuzzy for me ;-) - ?

with respect to vcpu, we could add an opaque void * in kvm_io_device and
pass the vcpu when calling kvm_iodevice_init().

I also think we must resume integration of ioeventfd, whatever the
chosen solution all the more so there seems to be some momentum on this now.

Best Regards

Eric
> 
> But honestly I would happily sacrifice "performance" for easier VGIC
> code - especially if one thinks about security for instance. Though I
> think that another memory reference doesn't really matter in this
> context ;-)
> 
>>> the ioapic? what do I miss?
>> I looked quickly in the ioapic code, and if I get it right there are no "ranges'
>> like what we have with the GIC. They have this regselect/regwindow concept
>> and they seem to have much less "registers" to handle. GIC seems a lot more
>> complex in terms of MMIO interface.
> 
> Right, that was my impression, too. IOAPIC isn't really comparable to
> the GIC in this respect. That's why I was going away from this rework,
> since I thought that the kvm_io_bus API wasn't really meant for such
> beasts as the GIC.
> 
> Cheers,
> Andre.
> 
>>
>> regards,
>> Nikolay Nikolaev
>>
>>>
>>> Thanks
>>>
>>> Best Regards
>>>
>>> Eric
>>>>>>>  - the handler of each range should be split into read and write
>>>>>>>  - all handlers take 'struct kvm_exit_mmio', and pass it to
>>>>>>> 'vgic_reg_access', 'mmio_data_read' and 'mmio_data_read'
>>>>>>>
>>>>>>> To sum up - if we do this refactoring of vgic's MMIO handling +
>>>>>>> kvm_io_bus_ API getting 'vcpu" argument we'll get a 'much' cleaner
>>>>>>> vgic code and as a bonus we'll get 'ioeventfd' capabilities.
>>>>>>>
>>>>>>> We have 3 questions:
>>>>>>>  - is the kvm_io_bus_ getting 'vcpu' argument acceptable for the other
>>>>>>> architectures too?
>>>>>>>  - is this huge vgic MMIO handling redesign acceptable/desired (it
>>>>>>> touches a lot of code)?
>>>>>>>  - is there a way that ioeventfd is accepted leaving vgic.c in it's
>>>>>>> current state?
>>>>>>>
>>>>>> Not sure how the latter question is relevant to this, but check with
>>>>>> Andre who recently looked at this as well and decided that for GICv3 the
>>>>>> only sane thing was to remove that comment for the gic.
>>>>> @Andre - what's your experience with the GICv3 and MMIO handling,
>>>>> anything specific?
>>>>>>
>>>>>> I don't recall the details of what you were trying to accomplish here
>>>>>> (it's been 8 months or so) but the surely the vgic handling code should
>>>>>> *somehow* be integrated into the handle_kernel_mmio (like Paolo
>>>>>> suggested), unless you come back and tell me that that would involve a
>>>>>> complete rewrite of the vgic code.
>>>>> I'm experimenting now - it's not exactly rewrite of whole vgic code,
>>>>> but it will touch a lot of it  - all MMIO access handlers and the
>>>>> supporting functions.
>>>>> We're ready to spend the effort. My question is  - is this acceptable?
>>>>>
>>>>> regards,
>>>>> Nikolay Nikolaev
>>>>> Virtual Open Systems
>>>>>>
>>>>>> -Christoffer
>>>>> _______________________________________________
>>>>> kvmarm mailing list
>>>>> kvmarm@lists.cs.columbia.edu
>>>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>>>
>>>>
>>>
>>

[RFC PATCH 2/4] KVM: irqfd should depend on CONFIG_HAVE_KVM_IRQ_ROUTING

[Antonios Motakis]

With an ARM VGIC we have an IRQ chip in the kernel (using the hardware
support), but the irqfd feature is still not available due to a dependency
on IRQ routing. The prototypes of kvm_irqfd and kvm_irqfd_release should
be conditionally compiled depending on HAVE_KVM_IRQ_ROUTING instead of
HAVE_KVM_IRQCHIP.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 include/linux/kvm_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b8e9a43..8728159 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -936,7 +936,7 @@ static inline void kvm_free_irq_routing(struct kvm *kvm) {}
 void kvm_eventfd_init(struct kvm *kvm);
 int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args);
 
-#ifdef CONFIG_HAVE_KVM_IRQCHIP
+#ifdef CONFIG_HAVE_KVM_IRQ_ROUTING
 int kvm_irqfd(struct kvm *kvm, struct kvm_irqfd *args);
 void kvm_irqfd_release(struct kvm *kvm);
 void kvm_irq_routing_update(struct kvm *, struct kvm_irq_routing_table *);
-- 
1.8.3.2

[RFC PATCH 3/4] ARM: KVM: enable linking against eventfd

[Antonios Motakis]

This enables and compiles the ioeventfd capability of KVM on ARM. The
irqfd feature will not be included in a build, due to the lack of IRQ
routing in the VGIC implementation of KVM.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/Kconfig  | 1 +
 arch/arm/kvm/Makefile | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 466bd29..a4b0312 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -20,6 +20,7 @@ config KVM
 	bool "Kernel-based Virtual Machine (KVM) support"
 	select PREEMPT_NOTIFIERS
 	select ANON_INODES
+	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_CPU_RELAX_INTERCEPT
 	select KVM_MMIO
 	select KVM_ARM_HOST
diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index 789bca9..2fa2f82 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -15,7 +15,7 @@ AFLAGS_init.o := -Wa,-march=armv7-a$(plus_virt)
 AFLAGS_interrupts.o := -Wa,-march=armv7-a$(plus_virt)
 
 KVM := ../../../virt/kvm
-kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o
 
 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
-- 
1.8.3.2

[RFC PATCH 4/4] ARM: KVM: enable KVM_CAP_IOEVENTFD

[Antonios Motakis]

KVM on ARM can now advertise support for the ioeventfd capability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/arm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index bd18bb8..08cd89b 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -211,6 +211,9 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_MAX_VCPUS:
 		r = KVM_MAX_VCPUS;
 		break;
+	case KVM_CAP_IOEVENTFD:
+		r = 1;
+		break;
 	default:
 		r = kvm_arch_dev_ioctl_check_extension(ext);
 		break;
-- 
1.8.3.2