On Mon, Feb 10, 2025 at 11:58 AM Sahil Siddiq <icegambi...@gmail.com> wrote: > > 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 <icegambi...@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.
Interesting, is a similar crash than this? (NULL ptr deference on virtnet_set_features)? https://issues.redhat.com/browse/RHEL-391 > 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. > Good! > 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). Good catch, this should be le16_to_cpu actually. But code wise is the same, so we have no visible error. Do you want to send a patch to fix it? > 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)? > This part is correct in the code, as it is used by the host, not written to the guest or read from the guest. So no conversion is needed. Thanks!
