Re: [RFC v4 0/5] Add packed virtqueue to shadow virtqueue

[Sahil Siddiq <icegambit91@gmail.com>]

Hi,

On 2/6/25 8:47 PM, Sahil Siddiq wrote:
> On 2/6/25 12:42 PM, Eugenio Perez Martin wrote:
>> On Thu, Feb 6, 2025 at 6:26 AM Sahil Siddiq <icegambit91@gmail.com> wrote:
>>> On 2/4/25 11:45 PM, Eugenio Perez Martin wrote:
>>>> PS: Please note that you can check packed_vq SVQ implementation
>>>> already without CVQ, as these features are totally orthogonal :).
>>>>
>>>
>>> Right. Now that I can ping with the ctrl features turned off, I think
>>> this should take precedence. There's another issue specific to the
>>> packed virtqueue case. It causes the kernel to crash. I have been
>>> investigating this and the situation here looks very similar to what's
>>> explained in Jason Wang's mail [2]. My plan of action is to apply his
>>> changes in L2's kernel and check if that resolves the problem.
>>>
>>> The details of the crash can be found in this mail [3].
>>>
>>
>> If you're testing this series without changes, I think that is caused
>> by not implementing the packed version of vhost_svq_get_buf.
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2024-12/msg01902.html
>>
> 
> Oh, apologies, I think I had misunderstood your response in the linked mail.
> Until now, I thought they were unrelated. In that case, I'll implement the
> packed version of vhost_svq_get_buf. Hopefully that fixes it :).
> 

I noticed one thing while testing some of the changes that I have made.
I haven't finished making the relevant changes to all the functions which
will have to handle split and packed vq differently. L2's kernel crashes
when I launch L0-QEMU with ctrl_vq=on,ctrl_rx=on. However, when I start
L0-QEMU with ctrl_vq=off,ctrl_rx=off,ctrl_vlan=off,ctrl_mac_addr=off, L2's
kernel boots successfully. Tracing L2-QEMU also confirms that the packed
feature is enabled. With all the ctrl features disabled, I think pinging
will also be possible once I finish implementing the packed versions of
the other functions.

There's another thing that I am confused about regarding the current
implementation (in the master branch).

In hw/virtio/vhost-shadow-virtqueue.c:vhost_svq_vring_write_descs() [1],
svq->free_head saves the descriptor in the specified format using
"le16_to_cpu" (line 171). On the other hand, the value of i is stored
in the native endianness using "cpu_to_le16" (line 168). If "i" is to be
stored in the native endianness (little endian in this case), then
should svq->free_head first be converted to little endian before being
assigned to "i" at the start of the function (line 142)?

Thanks,
Sahil

[1] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-shadow-virtqueue.c