On Thu, 23 Jul 2026 at 03:11, Arnaud POULIQUEN <[email protected]> wrote: > > Hello Mathieu, > > On 7/22/26 16:49, Mathieu Poirier wrote: > > On Tue, Jul 21, 2026 at 11:02:13AM -0500, Shah, Tanmay wrote: > >> > >> > >> On 7/21/2026 10:50 AM, Mathieu Poirier wrote: > >>> On Thu, Jul 16, 2026 at 11:12:55AM -0500, Shah, Tanmay wrote: > >>>> > >>>> > >>>> On 7/16/2026 10:48 AM, Mathieu Poirier wrote: > >>>>> On Wed, 15 Jul 2026 at 11:28, Shah, Tanmay <[email protected]> wrote: > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> Please find my response below: > >>>>>> > >>>>>> On 7/15/2026 11:24 AM, Mathieu Poirier wrote: > >>>>>>> On Fri, Jul 10, 2026 at 12:28:29PM -0700, Tanmay Shah wrote: > >>>>>>>> 512 bytes isn't always suitable for all case, let firmware > >>>>>>>> maker decide the best value from resource table. > >>>>>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit. > >>>>>>>> > >>>>>>>> Signed-off-by: Tanmay Shah <[email protected]> > >>>>>>>> --- > >>>>>>>> Changes in v5: > >>>>>>>> > >>>>>>>> - fix documentation about alignment of the buffer size > >>>>>>>> - change version field from u16 to u8 > >>>>>>>> - remove buffer alignment check > >>>>>>>> - Separate buffer alignment vs MTU of a single buffer > >>>>>>>> - Use buffer alignment only to get next buffer address at > >>>>>>>> alignment > >>>>>>>> boundary > >>>>>>>> > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>>> +#ifndef _LINUX_VIRTIO_RPMSG_H > >>>>>>>> +#define _LINUX_VIRTIO_RPMSG_H > >>>>>>>> + > >>>>>>>> +#include <linux/types.h> > >>>>>>>> +#include <linux/virtio_types.h> > >>>>>>>> + > >>>>>>>> +/* The feature bitmap for virtio rpmsg */ > >>>>>>>> +#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service > >>>>>>>> notifications */ > >>>>>>>> +#define VIRTIO_RPMSG_F_BUFSZ 1 /* RP get buffer size from > >>>>>>>> config space */ > >>>>>>>> + > >>>>>>>> +/* Version of struct virtio_rpmsg_config understood by this driver > >>>>>>>> */ > >>>>>>>> +#define RPMSG_VDEV_CONFIG_V1 1 > >>>>>>>> + > >>>>>>>> +/** > >>>>>>>> + * struct virtio_rpmsg_config - config space for rpmsg virtio device > >>>>>>>> + * > >>>>>>>> + * @version: version of this structure, currently > >>>>>>>> %RPMSG_VDEV_CONFIG_V1. > >>>>>>>> + * @size: size of this structure in bytes. > >>>>>>>> + * @rpmsg_buf_align: alignment in bytes for each buffer. Must be a > >>>>>>>> power of > >>>>>>>> + * two. If 0 then no alignment will be done. This > >>>>>>>> alignment > >>>>>>>> + * will not decide actual size of the buffer but will > >>>>>>>> be > >>>>>>>> + * used to decided the start address of the buffer. > >>>>>>>> The > >>>>>>>> + * actual size of the buffer can be different than the > >>>>>>>> + * aligned size of the buffer. > >>>>>>> > >>>>>>> Is there really a need to have a buffer size different from its > >>>>>>> alignment? It's > >>>>>>> not like the (small) delta between the buffer size and its alignment > >>>>>>> will be > >>>>>>> used for something else. I'm fine with a buffer alignment > >>>>>>> requirement but in > >>>>>>> those cases, the firmware should set the size of the buffer in > >>>>>>> accordance with > >>>>>>> its alignment requirement. Otherwise, the complexity needed to > >>>>>>> manage the > >>>>>>> discrpancy between the two yields a driver that is hard to maintain > >>>>>>> and prone to > >>>>>>> bugs. > >>>>>>> > >>>>>> > >>>>>> I had the same concern before. However, following example changed my > >>>>>> mind: > >>>>>> > >>>>>> So, a single buffer size is the MTU size of a packet for the protocol > >>>>>> supported by the firmware. Now that can be different than the aligned > >>>>>> size of the buffer. > >>>>>> > >>>>>> For example, the higher level protocol (not rpmsg) has 430 bytes as the > >>>>>> max size of a payload. However, cache line alignment is 64-bytes. Then > >>>>>> in that case, the aligned buffer size is 448 bytes. But, that doesn't > >>>>>> mean we can say protocol's MTU size is 448 bytes. If user end up > >>>>>> treating MTU size 448 bytes and use space beyond 430 bytes, then the > >>>>>> higher level apps might discard that data and communication may fail. > >>>>>> > >>>>> > >>>>> How is that scenario different from today's 512 byte buffer size? > >>>>> Most users don't use all 512 bytes and we don't run in the problem > >>>>> described above? > >>>>> > >>>> > >>>> 512 buffer size is hardcoded, so it is enforced on the protocol by the > >>>> framework. But by allowing the configuration of the buffer size we are > >>>> allowing the protocol to decide what the buffer size should be. So, when > >>>> user request the buffer via rpmsg_get_mtu() API, then that should be the > >>>> original buffer size which is expected by the protocol, which may not be > >>>> same as the aligned buffer size. > >>> > >>> Regardless of the buffer size, whether it is set to 512 byte or some > >>> arbitrary > >>> value by the remote processsor's firmware, there is a possibility of a > >>> discrepancy with what is expected by the protocol. Right now > >>> rpmsg_get_mtu() > >>> returns 512 regardless of what a protocol uses. The only thing that > >>> should be > >>> important to the protocol is not to exceed that limit. > >>> > >> > >> I think I am missing something. Are you saying that buffer size can not > >> be configured greater than 512 bytes? > > > > I am not. > > > > What I am saying is that if alignment is important to a remote processor, it > > should choose the buffer size accordingly. rpmsg_get_mtu() should return > > the > > value of the buffer size, exactly the way it is today. > > Did you get a chance to look to my previous reply [1]? >
Unfortunately I haven't had the opportunity to reach that point in my Inbox. > From my understanding, due to the Virtio specification, we cannot > guarantee that > the buffer size will not be updated by the main processor. Having an > alignment > field ensures that the main processor takes the remote processor’s > constraints > into account if it updates the RPMsg buffer size. > Where does the main processor change the buffer size? > That said, if we decide to remove the alignment field, we must clearly > document the constraints for the RPMsg Virtio config space: > > - virtio_rpmsg_config cannot be changed by the Virtio driver. > - the sizes must respect the device and driver alignment constraint. > If the size chosen by the device isn't compatible with what the driver can accommodate, then initialization simply fails. > This is not my preferred solution,because that means that the remote > processor has > to know the alignment constraints of the main processor. > It would have to anyway wouldn't it? And as I said from the beginning, this is not something that has come up in the 20 years this subsystem existed. There is no point in adding all this complexity for a something that _may_ happen. > [1]https://www.mail-archive.com/[email protected]/msg2643398.html > > Regards, > > Arnaud > > > >> > >> If the higher level protocol (not RPMsg) wants to use 4030 bytes for > >> single packet payload then that is what the MTU size should be. And so > >> the firmware will configure 4030 bytes as single buffer size in the vdev > >> config space. That is why alignment should be treated separately. > >> Because it is not equal to payload size needed by higher level protocol. > > > > In that case and assuming alignment is required, the buffer size should be > > 4096 > > and rpmsg_get_mtu() should also return 4096. How a higher protocol uses the > > buffer space is none of our concern. > > > > Currently, the buffer size is set to 512 and users don't always fill the > > entire > > buffer. I don't see why things should be different with a configurable > > buffer > > size. > > > >> > >>>> > >>>> If for internal management we want to treat buffer size = aligned buffer > >>>> size, I am okay. But rpmsg_get_mtu() must give unaligned buffer size > >>>> which is expected by the protocol. > >>> > >>> I agree with the first sentence but not the second. The only thing > >>> protocols > >>> should care about is the start address of a buffer and that its size is > >>> sufficient for what it needs. > >>> > >>>> > >>>> Thanks, > >>>> Tanmay > >>>> > >>>>>> The alignment field is used only to decide where the next buffer start > >>>>>> address is to ease cache operations. > >>>>>> > >>>>>> Sure, we need to maintain this complexity, but I think it's worth it. > >>>>>> > >>>>> > >>>>> The same as in my previous email to Arnaud applies here - is this an > >>>>> immediate requirement of something we think may be happening in the > >>>>> future? > >>>>> > >>>> > >>>> IMHO, vendors will use it if the feature is available, otherwise the > >>>> need to optimize alignment is not easily encountered. > >>>> > >>>>> > >>>>>> Thanks, > >>>>>> Tanmay > >>>>>> > >>>>>>>> + * @txbuf_size: Tx buf size from remote's view. For Linux this > >>>>>>>> is rx buf size. > >>>>>>>> + * @rxbuf_size: Rx buf size from remote's view. For Linux this > >>>>>>>> is tx buf size. > >>>>>>>> + * > >>>>>>>> + * This is the configuration structure shared by the device and the > >>>>>>>> driver, > >>>>>>>> + * read when %VIRTIO_RPMSG_F_BUFSZ is negotiated. The fields are > >>>>>>>> laid out so > >>>>>>>> + * the structure is naturally 32-bit aligned. > >>>>>>>> + */ > >>>>>>>> +struct virtio_rpmsg_config { > >>>>>>>> + u8 version; > >>>>>>>> + __virtio16 size; > >>>>>>>> + __virtio16 rpmsg_buf_align; > >>>>>>>> + /* The tx/rx individual buffer size (if VIRTIO_RPMSG_F_BUFSZ) */ > >>>>>>>> + __virtio32 txbuf_size; > >>>>>>>> + __virtio32 rxbuf_size; > >>>>>>>> +} __packed; > >>>>>>>> + > >>>>>>>> +#endif /* _LINUX_VIRTIO_RPMSG_H */ > >>>>>>>> -- > >>>>>>>> 2.34.1 > >>>>>>>> > >>>>>> > >>>> > >> > > >
