[PATCH] KVM: arm/arm64: GICv4: Do not perform an map to a mapped vLPI

[PATCH] KVM: arm/arm64: GICv4: Do not perform an map to a mapped vLPI

[Kunkun Jiang]

Before performing an unmap, let's check whether the vLPI has been
mapped. This corresponds to checking whether a vLPI is valid before
unmap it.

Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-v4.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index 339a55194b2c..824f4baf50ee 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -436,6 +436,11 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 	if (ret)
 		goto out;
 
+	if (irq->hw) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	/*
 	 * Emit the mapping request. If it fails, the ITS probably
 	 * isn't v4 compatible, so let's silently bail out. Holding
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm/arm64: GICv4: Do not perform an map to a mapped vLPI

[Marc Zyngier]

On Thu, 16 Nov 2023 12:52:15 +0000,
Kunkun Jiang <jiangkunkun@huawei.com> wrote:
> 
> Before performing an unmap, let's check whether the vLPI has been
> mapped. This corresponds to checking whether a vLPI is valid before
> unmap it.
>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  arch/arm64/kvm/vgic/vgic-v4.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
> index 339a55194b2c..824f4baf50ee 100644
> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> @@ -436,6 +436,11 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>  	if (ret)
>  		goto out;
>  
> +	if (irq->hw) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +

So this code affects the mapping side, not the unmapping. Even more
confusingly, the subject of the patch doesn't match the commit
message.

Furthermore, I'm not sure we want to return an error here. Userspace
has no knowledge of GICv4, and there are numerous other cases where we
don't return any error. This is certainly a userspace visible change.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm/arm64: GICv4: Do not perform an map to a mapped vLPI

[Kunkun Jiang]

Hi Marc，

On 2023/11/16 22:10, Marc Zyngier wrote:
> On Thu, 16 Nov 2023 12:52:15 +0000,
> Kunkun Jiang <jiangkunkun@huawei.com> wrote:
>> Before performing an unmap, let's check whether the vLPI has been
s/unmap/map
>> mapped. This corresponds to checking whether a vLPI is valid before
>> unmap it.
>>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   arch/arm64/kvm/vgic/vgic-v4.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>> index 339a55194b2c..824f4baf50ee 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>> @@ -436,6 +436,11 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>>   	if (ret)
>>   		goto out;
>>   
>> +	if (irq->hw) {
>> +		ret = -EBUSY;
>> +		goto out;
>> +	}
>> +
> So this code affects the mapping side, not the unmapping. Even more
> confusingly, the subject of the patch doesn't match the commit
> message.
>
> Furthermore, I'm not sure we want to return an error here. Userspace
> has no knowledge of GICv4, and there are numerous other cases where we
> don't return any error. This is certainly a userspace visible change.
Yes, Userspace has no knowledge of GICv4. In the current implementation,
this error will not be presented to userspace. Only a log will be printed
in kvm_irqfd_assign().
 > #ifdef CONFIG_HAVE_KVM_IRQ_BYPASS
 >         if (kvm_arch_has_irq_bypass()) {
 >                 irqfd->consumer.token = (void *)irqfd->eventfd;
 >                 irqfd->consumer.add_producer = 
kvm_arch_irq_bypass_add_producer;
 >                 irqfd->consumer.del_producer = 
kvm_arch_irq_bypass_del_producer;
 >                 irqfd->consumer.stop = kvm_arch_irq_bypass_stop;
 >                 irqfd->consumer.start = kvm_arch_irq_bypass_start;
 >                 ret = irq_bypass_register_consumer(&irqfd->consumer);
 >                 if (ret)
 >                         pr_info("irq bypass consumer (token %p) 
registration fails: %d\n",
 >                                 irqfd->consumer.token, ret);
 >         }
 > #endif

If you think it is better not to return an error, I will modify it in
the next version.

Looking forwarding to your reply.

Thanks,
Kunkun Jiang


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm/arm64: GICv4: Do not perform an map to a mapped vLPI

[Marc Zyngier]

On Fri, 17 Nov 2023 09:54:37 +0000,
Kunkun Jiang <jiangkunkun@huawei.com> wrote:
> 
> Hi Marc，
> 
> On 2023/11/16 22:10, Marc Zyngier wrote:
> > On Thu, 16 Nov 2023 12:52:15 +0000,
> > Kunkun Jiang <jiangkunkun@huawei.com> wrote:
> >> Before performing an unmap, let's check whether the vLPI has been
> s/unmap/map
> >> mapped. This corresponds to checking whether a vLPI is valid before
> >> unmap it.
> >> 
> >> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> >> ---
> >>   arch/arm64/kvm/vgic/vgic-v4.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
> >> index 339a55194b2c..824f4baf50ee 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> >> @@ -436,6 +436,11 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
> >>   	if (ret)
> >>   		goto out;
> >>   +	if (irq->hw) {
> >> +		ret = -EBUSY;
> >> +		goto out;
> >> +	}
> >> +
> > So this code affects the mapping side, not the unmapping. Even more
> > confusingly, the subject of the patch doesn't match the commit
> > message.
> > 
> > Furthermore, I'm not sure we want to return an error here. Userspace
> > has no knowledge of GICv4, and there are numerous other cases where we
> > don't return any error. This is certainly a userspace visible change.
> Yes, Userspace has no knowledge of GICv4. In the current implementation,
> this error will not be presented to userspace. Only a log will be printed
> in kvm_irqfd_assign().
> > #ifdef CONFIG_HAVE_KVM_IRQ_BYPASS
> >         if (kvm_arch_has_irq_bypass()) {
> >                 irqfd->consumer.token = (void *)irqfd->eventfd;
> >                 irqfd->consumer.add_producer =
> kvm_arch_irq_bypass_add_producer;
> >                 irqfd->consumer.del_producer =
> kvm_arch_irq_bypass_del_producer;
> >                 irqfd->consumer.stop = kvm_arch_irq_bypass_stop;
> >                 irqfd->consumer.start = kvm_arch_irq_bypass_start;
> >                 ret = irq_bypass_register_consumer(&irqfd->consumer);
> >                 if (ret)
> >                         pr_info("irq bypass consumer (token %p)
> registration fails: %d\n",
> >                                 irqfd->consumer.token, ret);
> >         }
> > #endif
> 
> If you think it is better not to return an error, I will modify it in
> the next version.

I don't think returning an error is worth it, given that it is not
actionable from userspace. The pr_info() itself is ugly and should
probably be at least ratelimited, but that's for another patch.

Please resend this with a 0 return value, a fixed commit message, and
a Fixes: tag.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel