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

[Marc Zyngier <maz@kernel.org>]

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.