Question on guest enable msi fail when using GICv4/4.1

Question on guest enable msi fail when using GICv4/4.1

[Shaokun Zhang]

[This letter comes from Nianyao Tang]

Hi,

Using GICv4/4.1 and msi capability, guest vf driver requires 3 vectors and enable msi, will lead to
guest stuck. Qemu gets number of interrupts from Multiple Message Capable field set by guest. This
field is aligned to a power of 2(if a function requires 3 vectors, it initializes it to 2).
However, guest driver just sends 3 mapi-cmd to vits and 3 ite entries is recorded in host.
Vfio initializes msi interrupts using the number of interrupts 4 provide by qemu.
When it comes to the 4th msi without ite in vits, in irq_bypass_register_producer, producer
and consumer will __connect fail, due to find_ite fail, and do not resume guest.

Do we support this case, Guest function using msi interrupts number not aligned to a power of 2?
Or qemu should provide correct msi interrupts number?

Thanks,
Shaokun

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

[Marc Zyngier]

On Fri, 07 May 2021 06:57:04 +0100,
Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> 
> [This letter comes from Nianyao Tang]
> 
> Hi,
> 
> Using GICv4/4.1 and msi capability, guest vf driver requires 3
> vectors and enable msi, will lead to guest stuck.

Stuck how?

> Qemu gets number of interrupts from Multiple Message Capable field
> set by guest. This field is aligned to a power of 2(if a function
> requires 3 vectors, it initializes it to 2).

So I guess this is a MultiMSI device with 4 vectors, right?

> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
> entries is recorded in host.  Vfio initializes msi interrupts using
> the number of interrupts 4 provide by qemu.  When it comes to the
> 4th msi without ite in vits, in irq_bypass_register_producer,
> producer and consumer will __connect fail, due to find_ite fail, and
> do not resume guest.

Let me rephrase this to check that I understand it:
- The device has 4 vectors
- The guest only create mappings for 3 of them
- VFIO calls kvm_vgic_v4_set_forwarding() for each vector
- KVM doesn't have a mapping for the 4th vector and returns an error
- VFIO disable this 4th vector

Is that correct? If yes, I don't understand why that impacts the guest
at all. From what I can see, vfio_msi_set_vector_signal() just prints
a message on the console and carries on.

> Do we support this case, Guest function using msi interrupts number
> not aligned to a power of 2?  Or qemu should provide correct msi
> interrupts number?

QEMU cannot know how many vectors are in use, and the guest is free to
issue mappings for the exact number of vectors it wants to service.

Please describe what breaks the guest here.

Thanks,

	M.

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

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

[Shaokun Zhang]

Hi Marc,

Thanks for your quick reply.

On 2021/5/7 17:03, Marc Zyngier wrote:
> On Fri, 07 May 2021 06:57:04 +0100,
> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>
>> [This letter comes from Nianyao Tang]
>>
>> Hi,
>>
>> Using GICv4/4.1 and msi capability, guest vf driver requires 3
>> vectors and enable msi, will lead to guest stuck.
> 
> Stuck how?

Guest serial does not response anymore and guest network shutdown.

> 
>> Qemu gets number of interrupts from Multiple Message Capable field
>> set by guest. This field is aligned to a power of 2(if a function
>> requires 3 vectors, it initializes it to 2).
> 
> So I guess this is a MultiMSI device with 4 vectors, right?
> 

Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi.

>> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
>> entries is recorded in host.  Vfio initializes msi interrupts using
>> the number of interrupts 4 provide by qemu.  When it comes to the
>> 4th msi without ite in vits, in irq_bypass_register_producer,
>> producer and consumer will __connect fail, due to find_ite fail, and
>> do not resume guest.
> 
> Let me rephrase this to check that I understand it:
> - The device has 4 vectors
> - The guest only create mappings for 3 of them
> - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
> - KVM doesn't have a mapping for the 4th vector and returns an error
> - VFIO disable this 4th vector
> 
> Is that correct? If yes, I don't understand why that impacts the guest
> at all. From what I can see, vfio_msi_set_vector_signal() just prints
> a message on the console and carries on.
> 

function calls:
--> vfio_msi_set_vector_signal
   --> irq_bypass_register_producer
      -->__connect

in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding and fails to
get the 4th mapping. When add_producer fail, it does not call cons->start, calls
kvm_arch_irq_bypass_start and then kvm_arm_resume_guest.

Thanks,
Shaokun

>> Do we support this case, Guest function using msi interrupts number
>> not aligned to a power of 2?  Or qemu should provide correct msi
>> interrupts number?
> 
> QEMU cannot know how many vectors are in use, and the guest is free to
> issue mappings for the exact number of vectors it wants to service.
> 
> Please describe what breaks the guest here.
> 
> Thanks,
> 
> 	M.
> 

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

[Marc Zyngier]

On Fri, 07 May 2021 10:58:23 +0100,
Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> 
> Hi Marc,
> 
> Thanks for your quick reply.
> 
> On 2021/5/7 17:03, Marc Zyngier wrote:
> > On Fri, 07 May 2021 06:57:04 +0100,
> > Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> >>
> >> [This letter comes from Nianyao Tang]
> >>
> >> Hi,
> >>
> >> Using GICv4/4.1 and msi capability, guest vf driver requires 3
> >> vectors and enable msi, will lead to guest stuck.
> > 
> > Stuck how?
> 
> Guest serial does not response anymore and guest network shutdown.
> 
> > 
> >> Qemu gets number of interrupts from Multiple Message Capable field
> >> set by guest. This field is aligned to a power of 2(if a function
> >> requires 3 vectors, it initializes it to 2).
> > 
> > So I guess this is a MultiMSI device with 4 vectors, right?
> > 
> 
> Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi.
> 
> >> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
> >> entries is recorded in host.  Vfio initializes msi interrupts using
> >> the number of interrupts 4 provide by qemu.  When it comes to the
> >> 4th msi without ite in vits, in irq_bypass_register_producer,
> >> producer and consumer will __connect fail, due to find_ite fail, and
> >> do not resume guest.
> > 
> > Let me rephrase this to check that I understand it:
> > - The device has 4 vectors
> > - The guest only create mappings for 3 of them
> > - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
> > - KVM doesn't have a mapping for the 4th vector and returns an error
> > - VFIO disable this 4th vector
> > 
> > Is that correct? If yes, I don't understand why that impacts the guest
> > at all. From what I can see, vfio_msi_set_vector_signal() just prints
> > a message on the console and carries on.
> > 
> 
> function calls:
> --> vfio_msi_set_vector_signal
>    --> irq_bypass_register_producer
>       -->__connect
> 
> in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding
> and fails to get the 4th mapping. When add_producer fail, it does
> not call cons->start, calls kvm_arch_irq_bypass_start and then
> kvm_arm_resume_guest.

[+Eric, who wrote the irq_bypass infrastructure.]

Ah, so the guest is actually paused, not in a livelock situation
(which is how I interpreted "stuck").

I think we should handle this case gracefully, as there should be no
expectation that the guest will be using this interrupt. Given that
VFIO seems to be pretty unfazed when a producer fails, I'm temped to
do the same thing and restart the guest.

Also, __disconnect doesn't care about errors, so why should __connect
have this odd behaviour?

Can you please try this? It is completely untested (and I think the
del_consumer call is odd, which is why I've also dropped it).

Eric, what do you think?

Thanks,

	M.

diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index c9bb3957f58a..7e1865e15668 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -40,21 +40,14 @@ static int __connect(struct irq_bypass_producer *prod,
 	if (prod->add_consumer)
 		ret = prod->add_consumer(prod, cons);
 
-	if (ret)
-		goto err_add_consumer;
-
-	ret = cons->add_producer(cons, prod);
-	if (ret)
-		goto err_add_producer;
+	if (!ret)
+		ret = cons->add_producer(cons, prod);
 
 	if (cons->start)
 		cons->start(cons);
 	if (prod->start)
 		prod->start(prod);
-err_add_producer:
-	if (prod->del_consumer)
-		prod->del_consumer(prod, cons);
-err_add_consumer:
+
 	return ret;
 }
 

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

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

[Jason Wang]

在 2021/5/8 上午1:36, Marc Zyngier 写道:
> On Fri, 07 May 2021 12:02:57 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
>> On Fri, 07 May 2021 10:58:23 +0100,
>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>> Hi Marc,
>>>
>>> Thanks for your quick reply.
>>>
>>> On 2021/5/7 17:03, Marc Zyngier wrote:
>>>> On Fri, 07 May 2021 06:57:04 +0100,
>>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>>>> [This letter comes from Nianyao Tang]
>>>>>
>>>>> Hi,
>>>>>
>>>>> Using GICv4/4.1 and msi capability, guest vf driver requires 3
>>>>> vectors and enable msi, will lead to guest stuck.
>>>> Stuck how?
>>> Guest serial does not response anymore and guest network shutdown.
>>>
>>>>> Qemu gets number of interrupts from Multiple Message Capable field
>>>>> set by guest. This field is aligned to a power of 2(if a function
>>>>> requires 3 vectors, it initializes it to 2).
>>>> So I guess this is a MultiMSI device with 4 vectors, right?
>>>>
>>> Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi.
>>>
>>>>> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
>>>>> entries is recorded in host.  Vfio initializes msi interrupts using
>>>>> the number of interrupts 4 provide by qemu.  When it comes to the
>>>>> 4th msi without ite in vits, in irq_bypass_register_producer,
>>>>> producer and consumer will __connect fail, due to find_ite fail, and
>>>>> do not resume guest.
>>>> Let me rephrase this to check that I understand it:
>>>> - The device has 4 vectors
>>>> - The guest only create mappings for 3 of them
>>>> - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
>>>> - KVM doesn't have a mapping for the 4th vector and returns an error
>>>> - VFIO disable this 4th vector
>>>>
>>>> Is that correct? If yes, I don't understand why that impacts the guest
>>>> at all. From what I can see, vfio_msi_set_vector_signal() just prints
>>>> a message on the console and carries on.
>>>>
>>> function calls:
>>> --> vfio_msi_set_vector_signal
>>>     --> irq_bypass_register_producer
>>>        -->__connect
>>>
>>> in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding
>>> and fails to get the 4th mapping. When add_producer fail, it does
>>> not call cons->start, calls kvm_arch_irq_bypass_start and then
>>> kvm_arm_resume_guest.
>> [+Eric, who wrote the irq_bypass infrastructure.]
>>
>> Ah, so the guest is actually paused, not in a livelock situation
>> (which is how I interpreted "stuck").
>>
>> I think we should handle this case gracefully, as there should be no
>> expectation that the guest will be using this interrupt. Given that
>> VFIO seems to be pretty unfazed when a producer fails, I'm temped to
>> do the same thing and restart the guest.
>>
>> Also, __disconnect doesn't care about errors, so why should __connect
>> have this odd behaviour?
>>
>> Can you please try this? It is completely untested (and I think the
>> del_consumer call is odd, which is why I've also dropped it).
>>
>> Eric, what do you think?
> 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.


>
> 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


>
> Thanks,
>
> 	M.
>

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

[Zhu, Lingshan]

On 5/8/2021 9:51 AM, Jason Wang wrote:
>
> 在 2021/5/8 上午1:36, Marc Zyngier 写道:
>> On Fri, 07 May 2021 12:02:57 +0100,
>> Marc Zyngier <maz@kernel.org> wrote:
>>> On Fri, 07 May 2021 10:58:23 +0100,
>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>>> Hi Marc,
>>>>
>>>> Thanks for your quick reply.
>>>>
>>>> On 2021/5/7 17:03, Marc Zyngier wrote:
>>>>> On Fri, 07 May 2021 06:57:04 +0100,
>>>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>>>>> [This letter comes from Nianyao Tang]
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Using GICv4/4.1 and msi capability, guest vf driver requires 3
>>>>>> vectors and enable msi, will lead to guest stuck.
>>>>> Stuck how?
>>>> Guest serial does not response anymore and guest network shutdown.
>>>>
>>>>>> Qemu gets number of interrupts from Multiple Message Capable field
>>>>>> set by guest. This field is aligned to a power of 2(if a function
>>>>>> requires 3 vectors, it initializes it to 2).
>>>>> So I guess this is a MultiMSI device with 4 vectors, right?
>>>>>
>>>> Yes, it can support maximum of 32 msi interrupts, and vf driver 
>>>> only use 3 msi.
>>>>
>>>>>> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
>>>>>> entries is recorded in host.  Vfio initializes msi interrupts using
>>>>>> the number of interrupts 4 provide by qemu.  When it comes to the
>>>>>> 4th msi without ite in vits, in irq_bypass_register_producer,
>>>>>> producer and consumer will __connect fail, due to find_ite fail, and
>>>>>> do not resume guest.
>>>>> Let me rephrase this to check that I understand it:
>>>>> - The device has 4 vectors
>>>>> - The guest only create mappings for 3 of them
>>>>> - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
>>>>> - KVM doesn't have a mapping for the 4th vector and returns an error
>>>>> - VFIO disable this 4th vector
>>>>>
>>>>> Is that correct? If yes, I don't understand why that impacts the 
>>>>> guest
>>>>> at all. From what I can see, vfio_msi_set_vector_signal() just prints
>>>>> a message on the console and carries on.
>>>>>
>>>> function calls:
>>>> --> vfio_msi_set_vector_signal
>>>>     --> irq_bypass_register_producer
>>>>        -->__connect
>>>>
>>>> in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding
>>>> and fails to get the 4th mapping. When add_producer fail, it does
>>>> not call cons->start, calls kvm_arch_irq_bypass_start and then
>>>> kvm_arm_resume_guest.
>>> [+Eric, who wrote the irq_bypass infrastructure.]
>>>
>>> Ah, so the guest is actually paused, not in a livelock situation
>>> (which is how I interpreted "stuck").
>>>
>>> I think we should handle this case gracefully, as there should be no
>>> expectation that the guest will be using this interrupt. Given that
>>> VFIO seems to be pretty unfazed when a producer fails, I'm temped to
>>> do the same thing and restart the guest.
>>>
>>> Also, __disconnect doesn't care about errors, so why should __connect
>>> have this odd behaviour?
>>>
>>> Can you please try this? It is completely untested (and I think the
>>> del_consumer call is odd, which is why I've also dropped it).
>>>
>>> Eric, what do you think?
>> 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.
Hi Marc,

I will send out a patch to revert this commit as Jason suggested.

Thanks
>
>
>>
>> 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
>
>
>>
>> Thanks,
>>
>>     M.
>>
>

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

[Marc Zyngier]

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.

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

[Auger Eric]

Hi,
On 5/7/21 1:02 PM, Marc Zyngier wrote:
> On Fri, 07 May 2021 10:58:23 +0100,
> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>
>> Hi Marc,
>>
>> Thanks for your quick reply.
>>
>> On 2021/5/7 17:03, Marc Zyngier wrote:
>>> On Fri, 07 May 2021 06:57:04 +0100,
>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>>>
>>>> [This letter comes from Nianyao Tang]
>>>>
>>>> Hi,
>>>>
>>>> Using GICv4/4.1 and msi capability, guest vf driver requires 3
>>>> vectors and enable msi, will lead to guest stuck.
>>>
>>> Stuck how?
>>
>> Guest serial does not response anymore and guest network shutdown.
>>
>>>
>>>> Qemu gets number of interrupts from Multiple Message Capable field
>>>> set by guest. This field is aligned to a power of 2(if a function
>>>> requires 3 vectors, it initializes it to 2).
>>>
>>> So I guess this is a MultiMSI device with 4 vectors, right?
>>>
>>
>> Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi.
>>
>>>> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
>>>> entries is recorded in host.  Vfio initializes msi interrupts using
>>>> the number of interrupts 4 provide by qemu.  When it comes to the
>>>> 4th msi without ite in vits, in irq_bypass_register_producer,
>>>> producer and consumer will __connect fail, due to find_ite fail, and
>>>> do not resume guest.
>>>
>>> Let me rephrase this to check that I understand it:
>>> - The device has 4 vectors
>>> - The guest only create mappings for 3 of them
>>> - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
>>> - KVM doesn't have a mapping for the 4th vector and returns an error
>>> - VFIO disable this 4th vector
>>>
>>> Is that correct? If yes, I don't understand why that impacts the guest
>>> at all. From what I can see, vfio_msi_set_vector_signal() just prints
>>> a message on the console and carries on.
>>>
>>
>> function calls:
>> --> vfio_msi_set_vector_signal
>>    --> irq_bypass_register_producer
>>       -->__connect
>>
>> in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding
>> and fails to get the 4th mapping. When add_producer fail, it does
>> not call cons->start, calls kvm_arch_irq_bypass_start and then
>> kvm_arm_resume_guest.
> 
> [+Eric, who wrote the irq_bypass infrastructure.]
> 
> Ah, so the guest is actually paused, not in a livelock situation
> (which is how I interpreted "stuck").
> 
> I think we should handle this case gracefully, as there should be no
> expectation that the guest will be using this interrupt. Given that
> VFIO seems to be pretty unfazed when a producer fails, I'm temped to
> do the same thing and restart the guest.
> 
> Also, __disconnect doesn't care about errors, so why should __connect
> have this odd behaviour?

_disconnect() does not care as we should always succeed tearing off
things. del_* ops are void functions. On the opposite we can fail
setting up the bypass.

Effectively
a979a6aa009f ("irqbypass: do not start cons/prod when failed connect")
needs to be reverted.

I agree the kerneldoc comments in linux/irqbypass.h may be improved to
better explain the role of stop/start cbs and warn about their potential
global impact.

wrt the case above, "in __connect, add_producer finally calls
kvm_vgic_v4_set_forwarding and fails to get the 4th mapping", shouldn't
we succeed in that case?

Thanks

Eric

> 
> Can you please try this? It is completely untested (and I think the
> del_consumer call is odd, which is why I've also dropped it).
> 
> Eric, what do you think?
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
> index c9bb3957f58a..7e1865e15668 100644
> --- a/virt/lib/irqbypass.c
> +++ b/virt/lib/irqbypass.c
> @@ -40,21 +40,14 @@ static int __connect(struct irq_bypass_producer *prod,
>  	if (prod->add_consumer)
>  		ret = prod->add_consumer(prod, cons);
>  
> -	if (ret)
> -		goto err_add_consumer;
> -
> -	ret = cons->add_producer(cons, prod);
> -	if (ret)
> -		goto err_add_producer;
> +	if (!ret)
> +		ret = cons->add_producer(cons, prod);
>  
>  	if (cons->start)
>  		cons->start(cons);
>  	if (prod->start)
>  		prod->start(prod);
> -err_add_producer:
> -	if (prod->del_consumer)
> -		prod->del_consumer(prod, cons);
> -err_add_consumer:
> +
>  	return ret;
>  }
>  
> 

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

[Marc Zyngier]

Hi Eric,

On Sun, 09 May 2021 18:00:04 +0100,
Auger Eric <eric.auger@redhat.com> wrote:
> 
> Hi,
> On 5/7/21 1:02 PM, Marc Zyngier wrote:
> > On Fri, 07 May 2021 10:58:23 +0100,
> > Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> >>
> >> Hi Marc,
> >>
> >> Thanks for your quick reply.
> >>
> >> On 2021/5/7 17:03, Marc Zyngier wrote:
> >>> On Fri, 07 May 2021 06:57:04 +0100,
> >>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> >>>>
> >>>> [This letter comes from Nianyao Tang]
> >>>>
> >>>> Hi,
> >>>>
> >>>> Using GICv4/4.1 and msi capability, guest vf driver requires 3
> >>>> vectors and enable msi, will lead to guest stuck.
> >>>
> >>> Stuck how?
> >>
> >> Guest serial does not response anymore and guest network shutdown.
> >>
> >>>
> >>>> Qemu gets number of interrupts from Multiple Message Capable field
> >>>> set by guest. This field is aligned to a power of 2(if a function
> >>>> requires 3 vectors, it initializes it to 2).
> >>>
> >>> So I guess this is a MultiMSI device with 4 vectors, right?
> >>>
> >>
> >> Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi.
> >>
> >>>> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
> >>>> entries is recorded in host.  Vfio initializes msi interrupts using
> >>>> the number of interrupts 4 provide by qemu.  When it comes to the
> >>>> 4th msi without ite in vits, in irq_bypass_register_producer,
> >>>> producer and consumer will __connect fail, due to find_ite fail, and
> >>>> do not resume guest.
> >>>
> >>> Let me rephrase this to check that I understand it:
> >>> - The device has 4 vectors
> >>> - The guest only create mappings for 3 of them
> >>> - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
> >>> - KVM doesn't have a mapping for the 4th vector and returns an error
> >>> - VFIO disable this 4th vector
> >>>
> >>> Is that correct? If yes, I don't understand why that impacts the guest
> >>> at all. From what I can see, vfio_msi_set_vector_signal() just prints
> >>> a message on the console and carries on.
> >>>
> >>
> >> function calls:
> >> --> vfio_msi_set_vector_signal
> >>    --> irq_bypass_register_producer
> >>       -->__connect
> >>
> >> in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding
> >> and fails to get the 4th mapping. When add_producer fail, it does
> >> not call cons->start, calls kvm_arch_irq_bypass_start and then
> >> kvm_arm_resume_guest.
> > 
> > [+Eric, who wrote the irq_bypass infrastructure.]
> > 
> > Ah, so the guest is actually paused, not in a livelock situation
> > (which is how I interpreted "stuck").
> > 
> > I think we should handle this case gracefully, as there should be no
> > expectation that the guest will be using this interrupt. Given that
> > VFIO seems to be pretty unfazed when a producer fails, I'm temped to
> > do the same thing and restart the guest.
> > 
> > Also, __disconnect doesn't care about errors, so why should __connect
> > have this odd behaviour?
> 
> _disconnect() does not care as we should always succeed tearing off
> things. del_* ops are void functions. On the opposite we can fail
> setting up the bypass.
>
> Effectively
> a979a6aa009f ("irqbypass: do not start cons/prod when failed connect")
> needs to be reverted.
> 
> I agree the kerneldoc comments in linux/irqbypass.h may be improved to
> better explain the role of stop/start cbs and warn about their potential
> global impact.

Yup. It also begs the question of why we have producer callbacks, as
nobody seems to use them.

> wrt the case above, "in __connect, add_producer finally calls
> kvm_vgic_v4_set_forwarding and fails to get the 4th mapping", shouldn't
> we succeed in that case?

From a KVM perspective, we can't return a success because there is no
guest LPI that matches the input signal.

And such failure seems to be expected by the VFIO code, which just
prints a message on the console and set the producer token to NULL. So
returning an error from the KVM code is useful, at least to an extent.

Thanks,

	M.

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

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

[Auger Eric]

Hi Marc,

On 5/10/21 9:49 AM, Marc Zyngier wrote:
> Hi Eric,
> 
> On Sun, 09 May 2021 18:00:04 +0100,
> Auger Eric <eric.auger@redhat.com> wrote:
>>
>> Hi,
>> On 5/7/21 1:02 PM, Marc Zyngier wrote:
>>> On Fri, 07 May 2021 10:58:23 +0100,
>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>>>
>>>> Hi Marc,
>>>>
>>>> Thanks for your quick reply.
>>>>
>>>> On 2021/5/7 17:03, Marc Zyngier wrote:
>>>>> On Fri, 07 May 2021 06:57:04 +0100,
>>>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>>>>>
>>>>>> [This letter comes from Nianyao Tang]
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Using GICv4/4.1 and msi capability, guest vf driver requires 3
>>>>>> vectors and enable msi, will lead to guest stuck.
>>>>>
>>>>> Stuck how?
>>>>
>>>> Guest serial does not response anymore and guest network shutdown.
>>>>
>>>>>
>>>>>> Qemu gets number of interrupts from Multiple Message Capable field
>>>>>> set by guest. This field is aligned to a power of 2(if a function
>>>>>> requires 3 vectors, it initializes it to 2).
>>>>>
>>>>> So I guess this is a MultiMSI device with 4 vectors, right?
>>>>>
>>>>
>>>> Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi.
>>>>
>>>>>> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
>>>>>> entries is recorded in host.  Vfio initializes msi interrupts using
>>>>>> the number of interrupts 4 provide by qemu.  When it comes to the
>>>>>> 4th msi without ite in vits, in irq_bypass_register_producer,
>>>>>> producer and consumer will __connect fail, due to find_ite fail, and
>>>>>> do not resume guest.
>>>>>
>>>>> Let me rephrase this to check that I understand it:
>>>>> - The device has 4 vectors
>>>>> - The guest only create mappings for 3 of them
>>>>> - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
>>>>> - KVM doesn't have a mapping for the 4th vector and returns an error
>>>>> - VFIO disable this 4th vector
>>>>>
>>>>> Is that correct? If yes, I don't understand why that impacts the guest
>>>>> at all. From what I can see, vfio_msi_set_vector_signal() just prints
>>>>> a message on the console and carries on.
>>>>>
>>>>
>>>> function calls:
>>>> --> vfio_msi_set_vector_signal
>>>>    --> irq_bypass_register_producer
>>>>       -->__connect
>>>>
>>>> in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding
>>>> and fails to get the 4th mapping. When add_producer fail, it does
>>>> not call cons->start, calls kvm_arch_irq_bypass_start and then
>>>> kvm_arm_resume_guest.
>>>
>>> [+Eric, who wrote the irq_bypass infrastructure.]
>>>
>>> Ah, so the guest is actually paused, not in a livelock situation
>>> (which is how I interpreted "stuck").
>>>
>>> I think we should handle this case gracefully, as there should be no
>>> expectation that the guest will be using this interrupt. Given that
>>> VFIO seems to be pretty unfazed when a producer fails, I'm temped to
>>> do the same thing and restart the guest.
>>>
>>> Also, __disconnect doesn't care about errors, so why should __connect
>>> have this odd behaviour?
>>
>> _disconnect() does not care as we should always succeed tearing off
>> things. del_* ops are void functions. On the opposite we can fail
>> setting up the bypass.
>>
>> Effectively
>> a979a6aa009f ("irqbypass: do not start cons/prod when failed connect")
>> needs to be reverted.
>>
>> I agree the kerneldoc comments in linux/irqbypass.h may be improved to
>> better explain the role of stop/start cbs and warn about their potential
>> global impact.
> 
> Yup. It also begs the question of why we have producer callbacks, as
> nobody seems to use them.

At the time this was designed, I was working on VFIO platform IRQ
forwarding using direct EOI and they were used (and useful)

+	irq->producer.stop = vfio_platform_irq_bypass_stop;
+	irq->producer.start = vfio_platform_irq_bypass_start;

[PATCH v4 02/13] VFIO: platform: registration of a dummy IRQ bypass producer
[PATCH v4 07/13] VFIO: platform: add irq bypass producer management
https://lists.cs.columbia.edu/pipermail/kvmarm/2015-November/017323.html

basically the IRQ was disabled and re-enabled. This series has never
been upstreamed but that's where it originates from.



> 
>> wrt the case above, "in __connect, add_producer finally calls
>> kvm_vgic_v4_set_forwarding and fails to get the 4th mapping", shouldn't
>> we succeed in that case?
> 
> From a KVM perspective, we can't return a success because there is no
> guest LPI that matches the input signal.
right, sorry I had in mind the set_forwarding was partially successful
for 3 of 4 LPIs but it is a unitary operation.
> 
> And such failure seems to be expected by the VFIO code, which just
> prints a message on the console and set the producer token to NULL. So
> returning an error from the KVM code is useful, at least to an extent.

OK. So with the revert, the use case resume working, right?

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

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

[Marc Zyngier]

On Mon, 10 May 2021 09:29:47 +0100,
Auger Eric <eric.auger@redhat.com> wrote:
> 
> Hi Marc,
> 
> On 5/10/21 9:49 AM, Marc Zyngier wrote:
> > Hi Eric,
> > 
> > On Sun, 09 May 2021 18:00:04 +0100,
> > Auger Eric <eric.auger@redhat.com> wrote:
> >>
> >> Hi,
> >> On 5/7/21 1:02 PM, Marc Zyngier wrote:
> >>> On Fri, 07 May 2021 10:58:23 +0100,
> >>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> >>>>
> >>>> Hi Marc,
> >>>>
> >>>> Thanks for your quick reply.
> >>>>
> >>>> On 2021/5/7 17:03, Marc Zyngier wrote:
> >>>>> On Fri, 07 May 2021 06:57:04 +0100,
> >>>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> >>>>>>
> >>>>>> [This letter comes from Nianyao Tang]
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Using GICv4/4.1 and msi capability, guest vf driver requires 3
> >>>>>> vectors and enable msi, will lead to guest stuck.
> >>>>>
> >>>>> Stuck how?
> >>>>
> >>>> Guest serial does not response anymore and guest network shutdown.
> >>>>
> >>>>>
> >>>>>> Qemu gets number of interrupts from Multiple Message Capable field
> >>>>>> set by guest. This field is aligned to a power of 2(if a function
> >>>>>> requires 3 vectors, it initializes it to 2).
> >>>>>
> >>>>> So I guess this is a MultiMSI device with 4 vectors, right?
> >>>>>
> >>>>
> >>>> Yes, it can support maximum of 32 msi interrupts, and vf driver only use 3 msi.
> >>>>
> >>>>>> However, guest driver just sends 3 mapi-cmd to vits and 3 ite
> >>>>>> entries is recorded in host.  Vfio initializes msi interrupts using
> >>>>>> the number of interrupts 4 provide by qemu.  When it comes to the
> >>>>>> 4th msi without ite in vits, in irq_bypass_register_producer,
> >>>>>> producer and consumer will __connect fail, due to find_ite fail, and
> >>>>>> do not resume guest.
> >>>>>
> >>>>> Let me rephrase this to check that I understand it:
> >>>>> - The device has 4 vectors
> >>>>> - The guest only create mappings for 3 of them
> >>>>> - VFIO calls kvm_vgic_v4_set_forwarding() for each vector
> >>>>> - KVM doesn't have a mapping for the 4th vector and returns an error
> >>>>> - VFIO disable this 4th vector
> >>>>>
> >>>>> Is that correct? If yes, I don't understand why that impacts the guest
> >>>>> at all. From what I can see, vfio_msi_set_vector_signal() just prints
> >>>>> a message on the console and carries on.
> >>>>>
> >>>>
> >>>> function calls:
> >>>> --> vfio_msi_set_vector_signal
> >>>>    --> irq_bypass_register_producer
> >>>>       -->__connect
> >>>>
> >>>> in __connect, add_producer finally calls kvm_vgic_v4_set_forwarding
> >>>> and fails to get the 4th mapping. When add_producer fail, it does
> >>>> not call cons->start, calls kvm_arch_irq_bypass_start and then
> >>>> kvm_arm_resume_guest.
> >>>
> >>> [+Eric, who wrote the irq_bypass infrastructure.]
> >>>
> >>> Ah, so the guest is actually paused, not in a livelock situation
> >>> (which is how I interpreted "stuck").
> >>>
> >>> I think we should handle this case gracefully, as there should be no
> >>> expectation that the guest will be using this interrupt. Given that
> >>> VFIO seems to be pretty unfazed when a producer fails, I'm temped to
> >>> do the same thing and restart the guest.
> >>>
> >>> Also, __disconnect doesn't care about errors, so why should __connect
> >>> have this odd behaviour?
> >>
> >> _disconnect() does not care as we should always succeed tearing off
> >> things. del_* ops are void functions. On the opposite we can fail
> >> setting up the bypass.
> >>
> >> Effectively
> >> a979a6aa009f ("irqbypass: do not start cons/prod when failed connect")
> >> needs to be reverted.
> >>
> >> I agree the kerneldoc comments in linux/irqbypass.h may be improved to
> >> better explain the role of stop/start cbs and warn about their potential
> >> global impact.
> > 
> > Yup. It also begs the question of why we have producer callbacks, as
> > nobody seems to use them.
> 
> At the time this was designed, I was working on VFIO platform IRQ
> forwarding using direct EOI and they were used (and useful)
> 
> +	irq->producer.stop = vfio_platform_irq_bypass_stop;
> +	irq->producer.start = vfio_platform_irq_bypass_start;
> 
> [PATCH v4 02/13] VFIO: platform: registration of a dummy IRQ bypass producer
> [PATCH v4 07/13] VFIO: platform: add irq bypass producer management
> https://lists.cs.columbia.edu/pipermail/kvmarm/2015-November/017323.html
> 
> basically the IRQ was disabled and re-enabled. This series has never
> been upstreamed but that's where it originates from.

Ah, I remember those. Something tells me we are eventually going to
need something like that with pKVM (unfortunately, PCI isn't that
popular on Android phones...).

> >> wrt the case above, "in __connect, add_producer finally calls
> >> kvm_vgic_v4_set_forwarding and fails to get the 4th mapping", shouldn't
> >> we succeed in that case?
> > 
> > From a KVM perspective, we can't return a success because there is no
> > guest LPI that matches the input signal.
> right, sorry I had in mind the set_forwarding was partially successful
> for 3 of 4 LPIs but it is a unitary operation.
> > 
> > And such failure seems to be expected by the VFIO code, which just
> > prints a message on the console and set the producer token to NULL. So
> > returning an error from the KVM code is useful, at least to an extent.
> 
> OK. So with the revert, the use case resume working, right?

Yes, Shaokun confirmed he was back in business.

Thanks,

	M.

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