On Thu, Jan 16, 2025 at 1:30 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote: > > On 2025/01/16 10:06, Jason Wang wrote: > > On Wed, Jan 15, 2025 at 1:07 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote: > >> > >> On 2025/01/13 12:04, Jason Wang wrote: > >>> On Fri, Jan 10, 2025 at 7:12 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote: > >>>> > >>>> On 2025/01/10 19:23, Michael S. Tsirkin wrote: > >>>>> On Fri, Jan 10, 2025 at 11:27:13AM +0800, Jason Wang wrote: > >>>>>> On Thu, Jan 9, 2025 at 2:59 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote: > >>>>>>> > >>>>>>> The specification says the device MUST set num_buffers to 1 if > >>>>>>> VIRTIO_NET_F_MRG_RXBUF has not been negotiated. > >>>>>> > >>>>>> Have we agreed on how to fix the spec or not? > >>>>>> > >>>>>> As I replied in the spec patch, if we just remove this "MUST", it > >>>>>> looks like we are all fine? > >>>>>> > >>>>>> Thanks > >>>>> > >>>>> We should replace MUST with SHOULD but it is not all fine, > >>>>> ignoring SHOULD is a quality of implementation issue. > >>>>> > >>> > >>> So is this something that the driver should notice? > >>> > >>>> > >>>> Should we really replace it? It would mean that a driver conformant with > >>>> the current specification may not be compatible with a device conformant > >>>> with the future specification. > >>> > >>> I don't get this. We are talking about devices and we want to relax so > >>> it should compatibile. > >> > >> > >> The problem is: > >> 1) On the device side, the num_buffers can be left uninitialized due to bugs > >> 2) On the driver side, the specification allows assuming the num_buffers > >> is set to one. > >> > >> Relaxing the device requirement will replace "due to bugs" with > >> "according to the specification" in 1). It still contradicts with 2) so > >> does not fix compatibility. > > > > Just to clarify I meant we can simply remove the following: > > > > """ > > The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF > > was not negotiated. Note: This means that num_buffers will always be 1 > > if VIRTIO_NET_F_MRG_RXBUF is not negotiated. > > """ > > > > And > > > > """ > > If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set > > num_buffers to 1. > > """ > > > > This seems easier as it reflects the fact where some devices don't set > > it. And it eases the transitional device as it doesn't need to have > > any special care. > > That can potentially break existing drivers that are compliant with the > current and assumes the num_buffers is set to 1. Those drivers are already 'broken'. Aren't they? Thanks > > Regards, > Akihiko Odaki > > > > > Then we don't need any driver normative so I don't see any conflict. > > > > Michael suggests we use "SHOULD", but if this is something that the > > driver needs to be aware of I don't know how "SHOULD" can help a lot > > or not. > > > >> > >> Instead, we should make the driver requirement stricter to change 2). > >> That is what "[PATCH v3] virtio-net: Ignore num_buffers when unused" does: > >> https://lore.kernel.org/r/20250110-reserved-v3-1-2ade0a5d2090@xxxxxxxxxx > >> > >>> > >>>> > >>>> We are going to fix all implementations known to buggy (QEMU and Linux) > >>>> anyway so I think it's just fine to leave that part of specification as is. > >>> > >>> I don't think we can fix it all. > >> > >> It essentially only requires storing 16 bits. There are details we need > >> to work out, but it should be possible to fix. > > > > I meant it's not realistic to fix all the hypervisors. Note that > > modern devices have been implemented for about a decade so we may have > > too many versions of various hypervisors. (E.g DPDK seems to stick > > with the same behaviour of the current kernel). > > >> > >> Regards, > >> Akihiko Odaki > >> > > > > Thanks > > >
