Re: Question on guest enable msi fail when using GICv4/4.1

From: Marc Zyngier <maz@kernel.org>
To: Jason Wang <jasowang@redhat.com>
Cc: Zhu Lingshan <lingshan.zhu@intel.com>,
	Shaokun Zhang <zhangshaokun@hisilicon.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org,
	linux-pci@vger.kernel.org,
	Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Nianyao Tang <tangnianyao@huawei.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Eric Auger <eric.auger@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: Question on guest enable msi fail when using GICv4/4.1
Date: Sat, 08 May 2021 10:15:36 +0100	[thread overview]
Message-ID: <874kfdvb5z.wl-maz@kernel.org> (raw)
In-Reply-To: <373c70d3-eda3-8e84-d138-2f90d4e55217@redhat.com>

On Sat, 08 May 2021 02:51:39 +0100,
Jason Wang <jasowang@redhat.com> wrote:
> 
> 
> 在 2021/5/8 上午1:36, Marc Zyngier 写道:

[...]

> > Adding Zhu, Jason, MST to the party. It all seems to be caused by this
> > commit:
> > 
> > commit a979a6aa009f3c99689432e0cdb5402a4463fb88
> > Author: Zhu Lingshan <lingshan.zhu@intel.com>
> > Date:   Fri Jul 31 14:55:33 2020 +0800
> > 
> >      irqbypass: do not start cons/prod when failed connect
> >           If failed to connect, there is no need to start consumer
> > nor
> >      producer.
> >           Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >      Suggested-by: Jason Wang <jasowang@redhat.com>
> >      Link: https://lore.kernel.org/r/20200731065533.4144-7-lingshan.zhu@intel.com
> >      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > 
> > Zhu, I'd really like to understand why you think it is OK not to
> > restart consumer and producers when a connection has failed to be
> > established between the two?
> 
> 
> My bad, I didn't check ARM code but it's not easy to infer that the
> cons->start/stop is not a per consumer specific operation but a global
> one like VM halting/resuming.

I don't disagree that it is a bit of an odd behaviour, and maybe we
can eventually relax this. However, my rule of thumb for error
handling is to try and put things back in the state you found them.

It is also unfortunate that this same commit introduces an interesting
bug by unconditionally calling del_producer(), even if the
producer/consumer connection has succeeded. I guess it is a good thing
that nobody seem to implement any of the producer callbacks.

> > In the case of KVM/arm64, this results in the guest being forever
> > suspended and never resumed. That's obviously not an acceptable
> > regression, as there is a number of benign reasons for a connect to
> > fail.
> 
> 
> Let's revert this commit.

Thanks,

	M.

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