[v3] KVM: arm64: Avoid serializing LPI get() / put()

[PATCH v3 04/10] KVM: arm64: vgic-its: Walk the LPI xarray in vgic_copy_lpi_list()

Posted by Oliver Upton 1 year, 11 months ago

Start iterating the LPI xarray in anticipation of removing the LPI
linked-list.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index fb2d3c356984..9ce2edfadd11 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -335,6 +335,7 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
 int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
+	XA_STATE(xas, &dist->lpi_xa, GIC_LPI_OFFSET);
 	struct vgic_irq *irq;
 	unsigned long flags;
 	u32 *intids;
@@ -353,7 +354,9 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
 		return -ENOMEM;
 
 	raw_spin_lock_irqsave(&dist->lpi_list_lock, flags);
-	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
+	rcu_read_lock();
+
+	xas_for_each(&xas, irq, INTERRUPT_ID_BITS_ITS) {
 		if (i == irq_count)
 			break;
 		/* We don't need to "get" the IRQ, as we hold the list lock. */
@@ -361,6 +364,8 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
 			continue;
 		intids[i++] = irq->intid;
 	}
+
+	rcu_read_unlock();
 	raw_spin_unlock_irqrestore(&dist->lpi_list_lock, flags);
 
 	*intid_ptr = intids;
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH v3 04/10] KVM: arm64: vgic-its: Walk the LPI xarray in vgic_copy_lpi_list()

Posted by Zenghui Yu 1 year, 11 months ago

On 2024/2/17 2:41, Oliver Upton wrote:
> Start iterating the LPI xarray in anticipation of removing the LPI
> linked-list.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index fb2d3c356984..9ce2edfadd11 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -335,6 +335,7 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
>  int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
> +	XA_STATE(xas, &dist->lpi_xa, GIC_LPI_OFFSET);
>  	struct vgic_irq *irq;
>  	unsigned long flags;
>  	u32 *intids;
> @@ -353,7 +354,9 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
>  		return -ENOMEM;
>  
>  	raw_spin_lock_irqsave(&dist->lpi_list_lock, flags);
> -	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
> +	rcu_read_lock();
> +
> +	xas_for_each(&xas, irq, INTERRUPT_ID_BITS_ITS) {

We should use '1 << INTERRUPT_ID_BITS_ITS - 1' to represent the maximum
LPI interrupt ID.

>  		if (i == irq_count)
>  			break;
>  		/* We don't need to "get" the IRQ, as we hold the list lock. */
> @@ -361,6 +364,8 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
>  			continue;
>  		intids[i++] = irq->intid;
>  	}
> +
> +	rcu_read_unlock();
>  	raw_spin_unlock_irqrestore(&dist->lpi_list_lock, flags);
>  
>  	*intid_ptr = intids;

Thanks,
Zenghui

Re: [PATCH v3 04/10] KVM: arm64: vgic-its: Walk the LPI xarray in vgic_copy_lpi_list()

Posted by Marc Zyngier 1 year, 11 months ago

On Sun, 18 Feb 2024 08:46:53 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> On 2024/2/17 2:41, Oliver Upton wrote:
> > Start iterating the LPI xarray in anticipation of removing the LPI
> > linked-list.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index fb2d3c356984..9ce2edfadd11 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -335,6 +335,7 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
> >  int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
> >  {
> >  	struct vgic_dist *dist = &kvm->arch.vgic;
> > +	XA_STATE(xas, &dist->lpi_xa, GIC_LPI_OFFSET);
> >  	struct vgic_irq *irq;
> >  	unsigned long flags;
> >  	u32 *intids;
> > @@ -353,7 +354,9 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
> >  		return -ENOMEM;
> >   	raw_spin_lock_irqsave(&dist->lpi_list_lock, flags);
> > -	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
> > +	rcu_read_lock();
> > +
> > +	xas_for_each(&xas, irq, INTERRUPT_ID_BITS_ITS) {
> 
> We should use '1 << INTERRUPT_ID_BITS_ITS - 1' to represent the maximum
> LPI interrupt ID.

Huh, well caught! I'm not even sure how it works, as that's way
smaller than the start of the walk (8192). Probably doesn't.

An alternative would be to use max_lpis_propbaser(), but I'm not sure
we always have a valid PROPBASER value set when we start using this
function. Worth investigating though.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 04/10] KVM: arm64: vgic-its: Walk the LPI xarray in vgic_copy_lpi_list()

Posted by Oliver Upton 1 year, 11 months ago

On Sun, Feb 18, 2024 at 10:28:19AM +0000, Marc Zyngier wrote:
> On Sun, 18 Feb 2024 08:46:53 +0000,
> Zenghui Yu <yuzenghui@huawei.com> wrote:
> > 
> > On 2024/2/17 2:41, Oliver Upton wrote:
> > > Start iterating the LPI xarray in anticipation of removing the LPI
> > > linked-list.
> > > 
> > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > > ---
> > >  arch/arm64/kvm/vgic/vgic-its.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > > index fb2d3c356984..9ce2edfadd11 100644
> > > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > > @@ -335,6 +335,7 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
> > >  int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
> > >  {
> > >  	struct vgic_dist *dist = &kvm->arch.vgic;
> > > +	XA_STATE(xas, &dist->lpi_xa, GIC_LPI_OFFSET);
> > >  	struct vgic_irq *irq;
> > >  	unsigned long flags;
> > >  	u32 *intids;
> > > @@ -353,7 +354,9 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
> > >  		return -ENOMEM;
> > >   	raw_spin_lock_irqsave(&dist->lpi_list_lock, flags);
> > > -	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
> > > +	rcu_read_lock();
> > > +
> > > +	xas_for_each(&xas, irq, INTERRUPT_ID_BITS_ITS) {
> > 
> > We should use '1 << INTERRUPT_ID_BITS_ITS - 1' to represent the maximum
> > LPI interrupt ID.

/facepalm

Thanks Zenghui!

> Huh, well caught! I'm not even sure how it works, as that's way
> smaller than the start of the walk (8192). Probably doesn't.
> 
> An alternative would be to use max_lpis_propbaser(), but I'm not sure
> we always have a valid PROPBASER value set when we start using this
> function. Worth investigating though.

Given the plans to eventually replace this with xarray marks, I'd vote
for doing the lazy thing and deciding this at compile time.

I can squash this in when I apply the series if the rest of it isn't
offensive, otherwise respin with the change.

-- 
Thanks,
Oliver

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index d84cb7618c59..f6025886071c 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -316,6 +316,8 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
 	return 0;
 }
 
+#define GIC_LPI_MAX_INTID	((1 << INTERRUPT_ID_BITS_ITS) - 1)
+
 /*
  * Create a snapshot of the current LPIs targeting @vcpu, so that we can
  * enumerate those LPIs without holding any lock.
@@ -345,7 +347,7 @@ int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr)
 	raw_spin_lock_irqsave(&dist->lpi_list_lock, flags);
 	rcu_read_lock();
 
-	xas_for_each(&xas, irq, INTERRUPT_ID_BITS_ITS) {
+	xas_for_each(&xas, irq, GIC_LPI_MAX_INTID) {
 		if (i == irq_count)
 			break;
 		/* We don't need to "get" the IRQ, as we hold the list lock. */