Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Marc Zyngier]

On 10/11/17 08:37, Christoffer Dall wrote:
> On Wed, Nov 08, 2017 at 03:08:36PM +, Marc Zyngier wrote:
>> On 07/11/17 21:28, Auger Eric wrote:
>>> Hi Marc,
>>>
>>> On 27/10/2017 16:28, Marc Zyngier wrote:
 Upon updating a property, we propagate it all the way to the physical
 ITS, and ask for an INV command to be executed there.

 Acked-by: Christoffer Dall 
 Signed-off-by: Marc Zyngier 
 ---
  virt/kvm/arm/vgic/vgic-its.c | 3 +++
  1 file changed, 3 insertions(+)

 diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
 index 0b7e648e7a0c..2e77c7c83942 100644
 --- a/virt/kvm/arm/vgic/vgic-its.c
 +++ b/virt/kvm/arm/vgic/vgic-its.c
 @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
 vgic_irq *irq,
spin_unlock(>irq_lock);
}
  
 +  if (irq->hw)
 +  return its_prop_update_vlpi(irq->host_irq, prop, true);
 +
return 0;
  }
>>> I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
>>> needed in hw mode?
>>
>> It's not. I guess we could bypass this altogether and take a short cut
>> after having updated the priority and enabled fields.
>>
> 
> I can apply this on top of the series as well if you're happy with it:
> 
> commit b54fba93b1330803a59ca75f3a5102e22cadc871 (HEAD -> next-gicv4)
> Author: Christoffer Dall 
> Date:   Fri Nov 10 09:34:54 2017 +0100
> 
> KVM: arm/arm64: Don't queue VLPIs on INV/INVALL
> 
> Since VLPIs are injected directly by the hardware there's no need to
> mark these as pending in software and queue them on the AP list.
> 
> Signed-off-by: Christoffer Dall 
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index c93ecd4a903b..a3754ec719c4 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -292,11 +292,14 @@ static int update_lpi_config(struct kvm *kvm, struct 
> vgic_irq *irq,
>   irq->priority = LPI_PROP_PRIORITY(prop);
>   irq->enabled = LPI_PROP_ENABLE_BIT(prop);
>  
> - vgic_queue_irq_unlock(kvm, irq, flags);
> - } else {
> - spin_unlock_irqrestore(>irq_lock, flags);
> + if (!irq->hw) {
> + vgic_queue_irq_unlock(kvm, irq, flags);
> + return 0;
> + }
>   }
>  
> + spin_unlock_irqrestore(>irq_lock, flags);
> +
>   if (irq->hw)
>   return its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
>  

Reviewed-by: Marc Zyngier 

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Marc Zyngier]

On 10/11/17 08:37, Christoffer Dall wrote:
> On Wed, Nov 08, 2017 at 03:08:36PM +, Marc Zyngier wrote:
>> On 07/11/17 21:28, Auger Eric wrote:
>>> Hi Marc,
>>>
>>> On 27/10/2017 16:28, Marc Zyngier wrote:
 Upon updating a property, we propagate it all the way to the physical
 ITS, and ask for an INV command to be executed there.

 Acked-by: Christoffer Dall 
 Signed-off-by: Marc Zyngier 
 ---
  virt/kvm/arm/vgic/vgic-its.c | 3 +++
  1 file changed, 3 insertions(+)

 diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
 index 0b7e648e7a0c..2e77c7c83942 100644
 --- a/virt/kvm/arm/vgic/vgic-its.c
 +++ b/virt/kvm/arm/vgic/vgic-its.c
 @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
 vgic_irq *irq,
spin_unlock(>irq_lock);
}
  
 +  if (irq->hw)
 +  return its_prop_update_vlpi(irq->host_irq, prop, true);
 +
return 0;
  }
>>> I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
>>> needed in hw mode?
>>
>> It's not. I guess we could bypass this altogether and take a short cut
>> after having updated the priority and enabled fields.
>>
> 
> I can apply this on top of the series as well if you're happy with it:
> 
> commit b54fba93b1330803a59ca75f3a5102e22cadc871 (HEAD -> next-gicv4)
> Author: Christoffer Dall 
> Date:   Fri Nov 10 09:34:54 2017 +0100
> 
> KVM: arm/arm64: Don't queue VLPIs on INV/INVALL
> 
> Since VLPIs are injected directly by the hardware there's no need to
> mark these as pending in software and queue them on the AP list.
> 
> Signed-off-by: Christoffer Dall 
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index c93ecd4a903b..a3754ec719c4 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -292,11 +292,14 @@ static int update_lpi_config(struct kvm *kvm, struct 
> vgic_irq *irq,
>   irq->priority = LPI_PROP_PRIORITY(prop);
>   irq->enabled = LPI_PROP_ENABLE_BIT(prop);
>  
> - vgic_queue_irq_unlock(kvm, irq, flags);
> - } else {
> - spin_unlock_irqrestore(>irq_lock, flags);
> + if (!irq->hw) {
> + vgic_queue_irq_unlock(kvm, irq, flags);
> + return 0;
> + }
>   }
>  
> + spin_unlock_irqrestore(>irq_lock, flags);
> +
>   if (irq->hw)
>   return its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
>  

Reviewed-by: Marc Zyngier 

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Christoffer Dall]

On Wed, Nov 08, 2017 at 03:08:36PM +, Marc Zyngier wrote:
> On 07/11/17 21:28, Auger Eric wrote:
> > Hi Marc,
> > 
> > On 27/10/2017 16:28, Marc Zyngier wrote:
> >> Upon updating a property, we propagate it all the way to the physical
> >> ITS, and ask for an INV command to be executed there.
> >>
> >> Acked-by: Christoffer Dall 
> >> Signed-off-by: Marc Zyngier 
> >> ---
> >>  virt/kvm/arm/vgic/vgic-its.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 0b7e648e7a0c..2e77c7c83942 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
> >> vgic_irq *irq,
> >>spin_unlock(>irq_lock);
> >>}
> >>  
> >> +  if (irq->hw)
> >> +  return its_prop_update_vlpi(irq->host_irq, prop, true);
> >> +
> >>return 0;
> >>  }
> > I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
> > needed in hw mode?
> 
> It's not. I guess we could bypass this altogether and take a short cut
> after having updated the priority and enabled fields.
> 

I can apply this on top of the series as well if you're happy with it:

commit b54fba93b1330803a59ca75f3a5102e22cadc871 (HEAD -> next-gicv4)
Author: Christoffer Dall 
Date:   Fri Nov 10 09:34:54 2017 +0100

KVM: arm/arm64: Don't queue VLPIs on INV/INVALL

Since VLPIs are injected directly by the hardware there's no need to
mark these as pending in software and queue them on the AP list.

Signed-off-by: Christoffer Dall 

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index c93ecd4a903b..a3754ec719c4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -292,11 +292,14 @@ static int update_lpi_config(struct kvm *kvm, struct 
vgic_irq *irq,
irq->priority = LPI_PROP_PRIORITY(prop);
irq->enabled = LPI_PROP_ENABLE_BIT(prop);
 
-   vgic_queue_irq_unlock(kvm, irq, flags);
-   } else {
-   spin_unlock_irqrestore(>irq_lock, flags);
+   if (!irq->hw) {
+   vgic_queue_irq_unlock(kvm, irq, flags);
+   return 0;
+   }
}
 
+   spin_unlock_irqrestore(>irq_lock, flags);
+
if (irq->hw)
return its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
 


Thanks,
-Christoffer

Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Christoffer Dall]

On Wed, Nov 08, 2017 at 03:08:36PM +, Marc Zyngier wrote:
> On 07/11/17 21:28, Auger Eric wrote:
> > Hi Marc,
> > 
> > On 27/10/2017 16:28, Marc Zyngier wrote:
> >> Upon updating a property, we propagate it all the way to the physical
> >> ITS, and ask for an INV command to be executed there.
> >>
> >> Acked-by: Christoffer Dall 
> >> Signed-off-by: Marc Zyngier 
> >> ---
> >>  virt/kvm/arm/vgic/vgic-its.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 0b7e648e7a0c..2e77c7c83942 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
> >> vgic_irq *irq,
> >>spin_unlock(>irq_lock);
> >>}
> >>  
> >> +  if (irq->hw)
> >> +  return its_prop_update_vlpi(irq->host_irq, prop, true);
> >> +
> >>return 0;
> >>  }
> > I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
> > needed in hw mode?
> 
> It's not. I guess we could bypass this altogether and take a short cut
> after having updated the priority and enabled fields.
> 

I can apply this on top of the series as well if you're happy with it:

commit b54fba93b1330803a59ca75f3a5102e22cadc871 (HEAD -> next-gicv4)
Author: Christoffer Dall 
Date:   Fri Nov 10 09:34:54 2017 +0100

KVM: arm/arm64: Don't queue VLPIs on INV/INVALL

Since VLPIs are injected directly by the hardware there's no need to
mark these as pending in software and queue them on the AP list.

Signed-off-by: Christoffer Dall 

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index c93ecd4a903b..a3754ec719c4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -292,11 +292,14 @@ static int update_lpi_config(struct kvm *kvm, struct 
vgic_irq *irq,
irq->priority = LPI_PROP_PRIORITY(prop);
irq->enabled = LPI_PROP_ENABLE_BIT(prop);
 
-   vgic_queue_irq_unlock(kvm, irq, flags);
-   } else {
-   spin_unlock_irqrestore(>irq_lock, flags);
+   if (!irq->hw) {
+   vgic_queue_irq_unlock(kvm, irq, flags);
+   return 0;
+   }
}
 
+   spin_unlock_irqrestore(>irq_lock, flags);
+
if (irq->hw)
return its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
 


Thanks,
-Christoffer

Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Marc Zyngier]

On 07/11/17 21:28, Auger Eric wrote:
> Hi Marc,
> 
> On 27/10/2017 16:28, Marc Zyngier wrote:
>> Upon updating a property, we propagate it all the way to the physical
>> ITS, and ask for an INV command to be executed there.
>>
>> Acked-by: Christoffer Dall 
>> Signed-off-by: Marc Zyngier 
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 0b7e648e7a0c..2e77c7c83942 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
>> vgic_irq *irq,
>>  spin_unlock(>irq_lock);
>>  }
>>  
>> +if (irq->hw)
>> +return its_prop_update_vlpi(irq->host_irq, prop, true);
>> +
>>  return 0;
>>  }
> I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
> needed in hw mode?

It's not. I guess we could bypass this altogether and take a short cut
after having updated the priority and enabled fields.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Marc Zyngier]

On 07/11/17 21:28, Auger Eric wrote:
> Hi Marc,
> 
> On 27/10/2017 16:28, Marc Zyngier wrote:
>> Upon updating a property, we propagate it all the way to the physical
>> ITS, and ask for an INV command to be executed there.
>>
>> Acked-by: Christoffer Dall 
>> Signed-off-by: Marc Zyngier 
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 0b7e648e7a0c..2e77c7c83942 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
>> vgic_irq *irq,
>>  spin_unlock(>irq_lock);
>>  }
>>  
>> +if (irq->hw)
>> +return its_prop_update_vlpi(irq->host_irq, prop, true);
>> +
>>  return 0;
>>  }
> I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
> needed in hw mode?

It's not. I guess we could bypass this altogether and take a short cut
after having updated the priority and enabled fields.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Auger Eric]

Hi Marc,

On 27/10/2017 16:28, Marc Zyngier wrote:
> Upon updating a property, we propagate it all the way to the physical
> ITS, and ask for an INV command to be executed there.
> 
> Acked-by: Christoffer Dall 
> Signed-off-by: Marc Zyngier 
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 0b7e648e7a0c..2e77c7c83942 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
> vgic_irq *irq,
>   spin_unlock(>irq_lock);
>   }
>  
> + if (irq->hw)
> + return its_prop_update_vlpi(irq->host_irq, prop, true);
> +
>   return 0;
>  }
I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
needed in hw mode?

Thanks

Eric

>  
> 

Re: [PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Auger Eric]

Hi Marc,

On 27/10/2017 16:28, Marc Zyngier wrote:
> Upon updating a property, we propagate it all the way to the physical
> ITS, and ask for an INV command to be executed there.
> 
> Acked-by: Christoffer Dall 
> Signed-off-by: Marc Zyngier 
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 0b7e648e7a0c..2e77c7c83942 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
> vgic_irq *irq,
>   spin_unlock(>irq_lock);
>   }
>  
> + if (irq->hw)
> + return its_prop_update_vlpi(irq->host_irq, prop, true);
> +
>   return 0;
>  }
I am confused by the vgic_queue_irq_unlock() on the "hw" path. Why is it
needed in hw mode?

Thanks

Eric

>  
> 

[PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Marc Zyngier]

Upon updating a property, we propagate it all the way to the physical
ITS, and ask for an INV command to be executed there.

Acked-by: Christoffer Dall 
Signed-off-by: Marc Zyngier 
---
 virt/kvm/arm/vgic/vgic-its.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 0b7e648e7a0c..2e77c7c83942 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
vgic_irq *irq,
spin_unlock(>irq_lock);
}
 
+   if (irq->hw)
+   return its_prop_update_vlpi(irq->host_irq, prop, true);
+
return 0;
 }
 
-- 
2.11.0

[PATCH v5 16/26] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs

[Marc Zyngier]

Upon updating a property, we propagate it all the way to the physical
ITS, and ask for an INV command to be executed there.

Acked-by: Christoffer Dall 
Signed-off-by: Marc Zyngier 
---
 virt/kvm/arm/vgic/vgic-its.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 0b7e648e7a0c..2e77c7c83942 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -296,6 +296,9 @@ static int update_lpi_config(struct kvm *kvm, struct 
vgic_irq *irq,
spin_unlock(>irq_lock);
}
 
+   if (irq->hw)
+   return its_prop_update_vlpi(irq->host_irq, prop, true);
+
return 0;
 }
 
-- 
2.11.0