Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Donnerstag, 7. Oktober 2021 17:18:03 CEST Stefan Hajnoczi wrote: > On Thu, Oct 07, 2021 at 03:09:16PM +0200, Christian Schoenebeck wrote: > > On Mittwoch, 6. Oktober 2021 16:42:34 CEST Stefan Hajnoczi wrote: > > > On Wed, Oct 06, 2021 at 02:50:07PM +0200, Christian Schoenebeck wrote: > > > > On Mittwoch, 6. Oktober 2021 13:06:55 CEST Stefan Hajnoczi wrote: > > > > > On Tue, Oct 05, 2021 at 06:32:46PM +0200, Christian Schoenebeck wrote: > > > > > > On Dienstag, 5. Oktober 2021 17:10:40 CEST Stefan Hajnoczi wrote: > > > > > > > On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck > > > > wrote: > > > > > > > > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > > > > > > > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian > > > > > > > > > Schoenebeck > > > > > > > > wrote: > > > > > > > > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a > > > > > > > > > > runtime > > > > > > > > > > variable per virtio user. > > > > > > > > > > > > > > > > > > virtio user == virtio device model? > > > > > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > > Reasons: > > > > > > > > > > > > > > > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute > > > > > > > > > > theoretical > > > > > > > > > > > > > > > > > > > > maximum queue size possible. Which is actually the > > > > > > > > > > maximum > > > > > > > > > > queue size allowed by the virtio protocol. The > > > > > > > > > > appropriate > > > > > > > > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > > > > > > > > > > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/vi > > > > > > > > > > rtio > > > > > > > > > > -v1. > > > > > > > > > > 1-cs > > > > > > > > > > 01.h > > > > > > > > > > tml#x1-240006 > > > > > > > > > > > > > > > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with > > > > > > > > > > a > > > > > > > > > > more or less arbitrary value of 1024 in the past, > > > > > > > > > > which > > > > > > > > > > limits the maximum transfer size with virtio to 4M > > > > > > > > > > (more precise: 1024 * PAGE_SIZE, with the latter > > > > > > > > > > typically > > > > > > > > > > being 4k). > > > > > > > > > > > > > > > > > > Being equal to IOV_MAX is a likely reason. Buffers with more > > > > > > > > > iovecs > > > > > > > > > than > > > > > > > > > that cannot be passed to host system calls (sendmsg(2), > > > > > > > > > pwritev(2), > > > > > > > > > etc). > > > > > > > > > > > > > > > > Yes, that's use case dependent. Hence the solution to opt-in > > > > > > > > if it > > > > > > > > is > > > > > > > > desired and feasible. > > > > > > > > > > > > > > > > > > (2) Additionally the current value of 1024 poses a hidden > > > > > > > > > > limit, > > > > > > > > > > > > > > > > > > > > invisible to guest, which causes a system hang with > > > > > > > > > > the > > > > > > > > > > following QEMU error if guest tries to exceed it: > > > > > > > > > > > > > > > > > > > > virtio: too many write descriptors in indirect table > > > > > > > > > > > > > > > > > > I don't understand this point. 2.6.5 The Virtqueue > > > > > > > > > Descriptor > > > > > > > > > Table > > > > > > > > > > > > says: > > > > > > > > > The number of descriptors in the table is defined by the > > > > > > > > > queue > > > > > > > > > size > > > > > > > > > for > > > > > > > > > > > > > > > > > > this virtqueue: this is the maximum possible descriptor > > > > > > > > > chain > > > > > > > > > length. > > > > > > > > > > > > > > > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > > > > > > > A driver MUST NOT create a descriptor chain longer than > > > > > > > > > the > > > > > > > > > Queue > > > > > > > > > Size > > > > > > > > > of > > > > > > > > > > > > > > > > > > the device. > > > > > > > > > > > > > > > > > > Do you mean a broken/malicious guest driver that is > > > > > > > > > violating > > > > > > > > > the > > > > > > > > > spec? > > > > > > > > > That's not a hidden limit, it's defined by the spec. > > > > > > > > > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781 > > > > > > > > .htm > > > > > > > > l > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788 > > > > > > > > .htm > > > > > > > > l > > > > > > > > > > > > > > > > You can already go beyond that queue size at runtime with the > > > > > > > > indirection > > > > > > > > table. The only actual limit is the currently hard coded value > > > > > > > > of > > > > > > > > 1k > > > > > > > > pages. > > > > > > > > Hence the suggestion to turn that into a variable. > > > > > > > > > > > > > > Exceeding Queue Size is a VIRTIO spec violation. Drivers that > > > > > > > operate > > > > > > > outsided the spec do so at their own risk. They may not be > > > > > > > compatible > > > > > > > with all device implement
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Thu, Oct 07, 2021 at 03:09:16PM +0200, Christian Schoenebeck wrote: > On Mittwoch, 6. Oktober 2021 16:42:34 CEST Stefan Hajnoczi wrote: > > On Wed, Oct 06, 2021 at 02:50:07PM +0200, Christian Schoenebeck wrote: > > > On Mittwoch, 6. Oktober 2021 13:06:55 CEST Stefan Hajnoczi wrote: > > > > On Tue, Oct 05, 2021 at 06:32:46PM +0200, Christian Schoenebeck wrote: > > > > > On Dienstag, 5. Oktober 2021 17:10:40 CEST Stefan Hajnoczi wrote: > > > > > > On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck > wrote: > > > > > > > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > > > > > > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck > > > > > > wrote: > > > > > > > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > > > > > > > > variable per virtio user. > > > > > > > > > > > > > > > > virtio user == virtio device model? > > > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > Reasons: > > > > > > > > > > > > > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > > > > > > > > > > > > > > > maximum queue size possible. Which is actually the maximum > > > > > > > > > queue size allowed by the virtio protocol. The appropriate > > > > > > > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > > > > > > > > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio > > > > > > > > > -v1. > > > > > > > > > 1-cs > > > > > > > > > 01.h > > > > > > > > > tml#x1-240006 > > > > > > > > > > > > > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > > > > > > > > more or less arbitrary value of 1024 in the past, which > > > > > > > > > limits the maximum transfer size with virtio to 4M > > > > > > > > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > > > > > > > > being 4k). > > > > > > > > > > > > > > > > Being equal to IOV_MAX is a likely reason. Buffers with more > > > > > > > > iovecs > > > > > > > > than > > > > > > > > that cannot be passed to host system calls (sendmsg(2), > > > > > > > > pwritev(2), > > > > > > > > etc). > > > > > > > > > > > > > > Yes, that's use case dependent. Hence the solution to opt-in if it > > > > > > > is > > > > > > > desired and feasible. > > > > > > > > > > > > > > > > (2) Additionally the current value of 1024 poses a hidden > > > > > > > > > limit, > > > > > > > > > > > > > > > > > > invisible to guest, which causes a system hang with the > > > > > > > > > following QEMU error if guest tries to exceed it: > > > > > > > > > > > > > > > > > > virtio: too many write descriptors in indirect table > > > > > > > > > > > > > > > > I don't understand this point. 2.6.5 The Virtqueue Descriptor > > > > > > > > Table > > > > > > > > > > says: > > > > > > > > The number of descriptors in the table is defined by the queue > > > > > > > > size > > > > > > > > for > > > > > > > > > > > > > > > > this virtqueue: this is the maximum possible descriptor chain > > > > > > > > length. > > > > > > > > > > > > > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > > > > > > A driver MUST NOT create a descriptor chain longer than the > > > > > > > > Queue > > > > > > > > Size > > > > > > > > of > > > > > > > > > > > > > > > > the device. > > > > > > > > > > > > > > > > Do you mean a broken/malicious guest driver that is violating > > > > > > > > the > > > > > > > > spec? > > > > > > > > That's not a hidden limit, it's defined by the spec. > > > > > > > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.htm > > > > > > > l > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.htm > > > > > > > l > > > > > > > > > > > > > > You can already go beyond that queue size at runtime with the > > > > > > > indirection > > > > > > > table. The only actual limit is the currently hard coded value of > > > > > > > 1k > > > > > > > pages. > > > > > > > Hence the suggestion to turn that into a variable. > > > > > > > > > > > > Exceeding Queue Size is a VIRTIO spec violation. Drivers that > > > > > > operate > > > > > > outsided the spec do so at their own risk. They may not be > > > > > > compatible > > > > > > with all device implementations. > > > > > > > > > > Yes, I am ware about that. And still, this practice is already done, > > > > > which > > > > > apparently is not limited to 9pfs. > > > > > > > > > > > The limit is not hidden, it's Queue Size as defined by the spec :). > > > > > > > > > > > > If you have a driver that is exceeding the limit, then please fix > > > > > > the > > > > > > driver. > > > > > > > > > > I absolutely understand your position, but I hope you also understand > > > > > that > > > > > this violation of the specs is a theoretical issue, it is not a > > > > > real-life > > > > > problem right now, and due to lack of man power unfortunately I have > >
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Mittwoch, 6. Oktober 2021 16:42:34 CEST Stefan Hajnoczi wrote: > On Wed, Oct 06, 2021 at 02:50:07PM +0200, Christian Schoenebeck wrote: > > On Mittwoch, 6. Oktober 2021 13:06:55 CEST Stefan Hajnoczi wrote: > > > On Tue, Oct 05, 2021 at 06:32:46PM +0200, Christian Schoenebeck wrote: > > > > On Dienstag, 5. Oktober 2021 17:10:40 CEST Stefan Hajnoczi wrote: > > > > > On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck wrote: > > > > > > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > > > > > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck > > > > wrote: > > > > > > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > > > > > > > variable per virtio user. > > > > > > > > > > > > > > virtio user == virtio device model? > > > > > > > > > > > > Yes > > > > > > > > > > > > > > Reasons: > > > > > > > > > > > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > > > > > > > > > > > > > maximum queue size possible. Which is actually the maximum > > > > > > > > queue size allowed by the virtio protocol. The appropriate > > > > > > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > > > > > > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio > > > > > > > > -v1. > > > > > > > > 1-cs > > > > > > > > 01.h > > > > > > > > tml#x1-240006 > > > > > > > > > > > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > > > > > > > more or less arbitrary value of 1024 in the past, which > > > > > > > > limits the maximum transfer size with virtio to 4M > > > > > > > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > > > > > > > being 4k). > > > > > > > > > > > > > > Being equal to IOV_MAX is a likely reason. Buffers with more > > > > > > > iovecs > > > > > > > than > > > > > > > that cannot be passed to host system calls (sendmsg(2), > > > > > > > pwritev(2), > > > > > > > etc). > > > > > > > > > > > > Yes, that's use case dependent. Hence the solution to opt-in if it > > > > > > is > > > > > > desired and feasible. > > > > > > > > > > > > > > (2) Additionally the current value of 1024 poses a hidden > > > > > > > > limit, > > > > > > > > > > > > > > > > invisible to guest, which causes a system hang with the > > > > > > > > following QEMU error if guest tries to exceed it: > > > > > > > > > > > > > > > > virtio: too many write descriptors in indirect table > > > > > > > > > > > > > > I don't understand this point. 2.6.5 The Virtqueue Descriptor > > > > > > > Table > > > > > > > > says: > > > > > > > The number of descriptors in the table is defined by the queue > > > > > > > size > > > > > > > for > > > > > > > > > > > > > > this virtqueue: this is the maximum possible descriptor chain > > > > > > > length. > > > > > > > > > > > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > > > > > A driver MUST NOT create a descriptor chain longer than the > > > > > > > Queue > > > > > > > Size > > > > > > > of > > > > > > > > > > > > > > the device. > > > > > > > > > > > > > > Do you mean a broken/malicious guest driver that is violating > > > > > > > the > > > > > > > spec? > > > > > > > That's not a hidden limit, it's defined by the spec. > > > > > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.htm > > > > > > l > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.htm > > > > > > l > > > > > > > > > > > > You can already go beyond that queue size at runtime with the > > > > > > indirection > > > > > > table. The only actual limit is the currently hard coded value of > > > > > > 1k > > > > > > pages. > > > > > > Hence the suggestion to turn that into a variable. > > > > > > > > > > Exceeding Queue Size is a VIRTIO spec violation. Drivers that > > > > > operate > > > > > outsided the spec do so at their own risk. They may not be > > > > > compatible > > > > > with all device implementations. > > > > > > > > Yes, I am ware about that. And still, this practice is already done, > > > > which > > > > apparently is not limited to 9pfs. > > > > > > > > > The limit is not hidden, it's Queue Size as defined by the spec :). > > > > > > > > > > If you have a driver that is exceeding the limit, then please fix > > > > > the > > > > > driver. > > > > > > > > I absolutely understand your position, but I hope you also understand > > > > that > > > > this violation of the specs is a theoretical issue, it is not a > > > > real-life > > > > problem right now, and due to lack of man power unfortunately I have > > > > to > > > > prioritize real-life problems over theoretical ones ATM. Keep in mind > > > > that > > > > right now I am the only person working on 9pfs actively, I do this > > > > voluntarily whenever I find a free time slice, and I am not paid for > > > > it > > > > either. > > > > > > > > I don
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Wed, Oct 06, 2021 at 02:50:07PM +0200, Christian Schoenebeck wrote: > On Mittwoch, 6. Oktober 2021 13:06:55 CEST Stefan Hajnoczi wrote: > > On Tue, Oct 05, 2021 at 06:32:46PM +0200, Christian Schoenebeck wrote: > > > On Dienstag, 5. Oktober 2021 17:10:40 CEST Stefan Hajnoczi wrote: > > > > On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck wrote: > > > > > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > > > > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck > wrote: > > > > > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > > > > > > variable per virtio user. > > > > > > > > > > > > virtio user == virtio device model? > > > > > > > > > > Yes > > > > > > > > > > > > Reasons: > > > > > > > > > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > > > > > > > > > > > maximum queue size possible. Which is actually the maximum > > > > > > > queue size allowed by the virtio protocol. The appropriate > > > > > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > > > > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1. > > > > > > > 1-cs > > > > > > > 01.h > > > > > > > tml#x1-240006 > > > > > > > > > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > > > > > > more or less arbitrary value of 1024 in the past, which > > > > > > > limits the maximum transfer size with virtio to 4M > > > > > > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > > > > > > being 4k). > > > > > > > > > > > > Being equal to IOV_MAX is a likely reason. Buffers with more iovecs > > > > > > than > > > > > > that cannot be passed to host system calls (sendmsg(2), pwritev(2), > > > > > > etc). > > > > > > > > > > Yes, that's use case dependent. Hence the solution to opt-in if it is > > > > > desired and feasible. > > > > > > > > > > > > (2) Additionally the current value of 1024 poses a hidden limit, > > > > > > > > > > > > > > invisible to guest, which causes a system hang with the > > > > > > > following QEMU error if guest tries to exceed it: > > > > > > > > > > > > > > virtio: too many write descriptors in indirect table > > > > > > > > > > > > I don't understand this point. 2.6.5 The Virtqueue Descriptor Table > > > > > > says: > > > > > > The number of descriptors in the table is defined by the queue > > > > > > size > > > > > > for > > > > > > > > > > > > this virtqueue: this is the maximum possible descriptor chain > > > > > > length. > > > > > > > > > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > > > > A driver MUST NOT create a descriptor chain longer than the Queue > > > > > > Size > > > > > > of > > > > > > > > > > > > the device. > > > > > > > > > > > > Do you mean a broken/malicious guest driver that is violating the > > > > > > spec? > > > > > > That's not a hidden limit, it's defined by the spec. > > > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.html > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.html > > > > > > > > > > You can already go beyond that queue size at runtime with the > > > > > indirection > > > > > table. The only actual limit is the currently hard coded value of 1k > > > > > pages. > > > > > Hence the suggestion to turn that into a variable. > > > > > > > > Exceeding Queue Size is a VIRTIO spec violation. Drivers that operate > > > > outsided the spec do so at their own risk. They may not be compatible > > > > with all device implementations. > > > > > > Yes, I am ware about that. And still, this practice is already done, which > > > apparently is not limited to 9pfs. > > > > > > > The limit is not hidden, it's Queue Size as defined by the spec :). > > > > > > > > If you have a driver that is exceeding the limit, then please fix the > > > > driver. > > > > > > I absolutely understand your position, but I hope you also understand that > > > this violation of the specs is a theoretical issue, it is not a real-life > > > problem right now, and due to lack of man power unfortunately I have to > > > prioritize real-life problems over theoretical ones ATM. Keep in mind that > > > right now I am the only person working on 9pfs actively, I do this > > > voluntarily whenever I find a free time slice, and I am not paid for it > > > either. > > > > > > I don't see any reasonable way with reasonable effort to do what you are > > > asking for here in 9pfs, and Greg may correct me here if I am saying > > > anything wrong. If you are seeing any specific real-life issue here, then > > > please tell me which one, otherwise I have to postpone that "specs > > > violation" issue. > > > > > > There is still a long list of real problems that I need to hunt down in > > > 9pfs, afterwards I can continue with theoretical ones if you want, but > > > right now I simply c
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Mittwoch, 6. Oktober 2021 13:06:55 CEST Stefan Hajnoczi wrote: > On Tue, Oct 05, 2021 at 06:32:46PM +0200, Christian Schoenebeck wrote: > > On Dienstag, 5. Oktober 2021 17:10:40 CEST Stefan Hajnoczi wrote: > > > On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck wrote: > > > > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > > > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck wrote: > > > > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > > > > > variable per virtio user. > > > > > > > > > > virtio user == virtio device model? > > > > > > > > Yes > > > > > > > > > > Reasons: > > > > > > > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > > > > > > > > > maximum queue size possible. Which is actually the maximum > > > > > > queue size allowed by the virtio protocol. The appropriate > > > > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1. > > > > > > 1-cs > > > > > > 01.h > > > > > > tml#x1-240006 > > > > > > > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > > > > > more or less arbitrary value of 1024 in the past, which > > > > > > limits the maximum transfer size with virtio to 4M > > > > > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > > > > > being 4k). > > > > > > > > > > Being equal to IOV_MAX is a likely reason. Buffers with more iovecs > > > > > than > > > > > that cannot be passed to host system calls (sendmsg(2), pwritev(2), > > > > > etc). > > > > > > > > Yes, that's use case dependent. Hence the solution to opt-in if it is > > > > desired and feasible. > > > > > > > > > > (2) Additionally the current value of 1024 poses a hidden limit, > > > > > > > > > > > > invisible to guest, which causes a system hang with the > > > > > > following QEMU error if guest tries to exceed it: > > > > > > > > > > > > virtio: too many write descriptors in indirect table > > > > > > > > > > I don't understand this point. 2.6.5 The Virtqueue Descriptor Table > > > > says: > > > > > The number of descriptors in the table is defined by the queue > > > > > size > > > > > for > > > > > > > > > > this virtqueue: this is the maximum possible descriptor chain > > > > > length. > > > > > > > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > > > A driver MUST NOT create a descriptor chain longer than the Queue > > > > > Size > > > > > of > > > > > > > > > > the device. > > > > > > > > > > Do you mean a broken/malicious guest driver that is violating the > > > > > spec? > > > > > That's not a hidden limit, it's defined by the spec. > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.html > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.html > > > > > > > > You can already go beyond that queue size at runtime with the > > > > indirection > > > > table. The only actual limit is the currently hard coded value of 1k > > > > pages. > > > > Hence the suggestion to turn that into a variable. > > > > > > Exceeding Queue Size is a VIRTIO spec violation. Drivers that operate > > > outsided the spec do so at their own risk. They may not be compatible > > > with all device implementations. > > > > Yes, I am ware about that. And still, this practice is already done, which > > apparently is not limited to 9pfs. > > > > > The limit is not hidden, it's Queue Size as defined by the spec :). > > > > > > If you have a driver that is exceeding the limit, then please fix the > > > driver. > > > > I absolutely understand your position, but I hope you also understand that > > this violation of the specs is a theoretical issue, it is not a real-life > > problem right now, and due to lack of man power unfortunately I have to > > prioritize real-life problems over theoretical ones ATM. Keep in mind that > > right now I am the only person working on 9pfs actively, I do this > > voluntarily whenever I find a free time slice, and I am not paid for it > > either. > > > > I don't see any reasonable way with reasonable effort to do what you are > > asking for here in 9pfs, and Greg may correct me here if I am saying > > anything wrong. If you are seeing any specific real-life issue here, then > > please tell me which one, otherwise I have to postpone that "specs > > violation" issue. > > > > There is still a long list of real problems that I need to hunt down in > > 9pfs, afterwards I can continue with theoretical ones if you want, but > > right now I simply can't, sorry. > > I understand. If you don't have time to fix the Linux virtio-9p driver > then that's fine. I will look at this again, but it might be tricky. On doubt I'll postpone it. > I still wanted us to agree on the spec position because the commit > description says it's a "
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Tue, Oct 05, 2021 at 06:32:46PM +0200, Christian Schoenebeck wrote: > On Dienstag, 5. Oktober 2021 17:10:40 CEST Stefan Hajnoczi wrote: > > On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck wrote: > > > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck wrote: > > > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > > > > variable per virtio user. > > > > > > > > virtio user == virtio device model? > > > > > > Yes > > > > > > > > Reasons: > > > > > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > > > > > > > maximum queue size possible. Which is actually the maximum > > > > > queue size allowed by the virtio protocol. The appropriate > > > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs > > > > > 01.h > > > > > tml#x1-240006 > > > > > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > > > > more or less arbitrary value of 1024 in the past, which > > > > > limits the maximum transfer size with virtio to 4M > > > > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > > > > being 4k). > > > > > > > > Being equal to IOV_MAX is a likely reason. Buffers with more iovecs than > > > > that cannot be passed to host system calls (sendmsg(2), pwritev(2), > > > > etc). > > > > > > Yes, that's use case dependent. Hence the solution to opt-in if it is > > > desired and feasible. > > > > > > > > (2) Additionally the current value of 1024 poses a hidden limit, > > > > > > > > > > invisible to guest, which causes a system hang with the > > > > > following QEMU error if guest tries to exceed it: > > > > > > > > > > virtio: too many write descriptors in indirect table > > > > > > > > I don't understand this point. 2.6.5 The Virtqueue Descriptor Table > says: > > > > The number of descriptors in the table is defined by the queue size > > > > for > > > > > > > > this virtqueue: this is the maximum possible descriptor chain length. > > > > > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > > A driver MUST NOT create a descriptor chain longer than the Queue Size > > > > of > > > > > > > > the device. > > > > > > > > Do you mean a broken/malicious guest driver that is violating the spec? > > > > That's not a hidden limit, it's defined by the spec. > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.html > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.html > > > > > > You can already go beyond that queue size at runtime with the indirection > > > table. The only actual limit is the currently hard coded value of 1k > > > pages. > > > Hence the suggestion to turn that into a variable. > > > > Exceeding Queue Size is a VIRTIO spec violation. Drivers that operate > > outsided the spec do so at their own risk. They may not be compatible > > with all device implementations. > > Yes, I am ware about that. And still, this practice is already done, which > apparently is not limited to 9pfs. > > > The limit is not hidden, it's Queue Size as defined by the spec :). > > > > If you have a driver that is exceeding the limit, then please fix the > > driver. > > I absolutely understand your position, but I hope you also understand that > this violation of the specs is a theoretical issue, it is not a real-life > problem right now, and due to lack of man power unfortunately I have to > prioritize real-life problems over theoretical ones ATM. Keep in mind that > right now I am the only person working on 9pfs actively, I do this > voluntarily > whenever I find a free time slice, and I am not paid for it either. > > I don't see any reasonable way with reasonable effort to do what you are > asking for here in 9pfs, and Greg may correct me here if I am saying anything > wrong. If you are seeing any specific real-life issue here, then please tell > me which one, otherwise I have to postpone that "specs violation" issue. > > There is still a long list of real problems that I need to hunt down in 9pfs, > afterwards I can continue with theoretical ones if you want, but right now I > simply can't, sorry. I understand. If you don't have time to fix the Linux virtio-9p driver then that's fine. I still wanted us to agree on the spec position because the commit description says it's a "hidden limit", which is incorrect. It might seem pedantic, but my concern is that misconceptions can spread if we let them. That could cause people to write incorrect code later on. Please update the commit description either by dropping 2) or by replacing it with something else. For example: 2) The Linux virtio-9p guest driver does not honor the VIRTIO Queue Size value and can submit descriptor chains that exce
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Dienstag, 5. Oktober 2021 17:10:40 CEST Stefan Hajnoczi wrote: > On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck wrote: > > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck wrote: > > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > > > variable per virtio user. > > > > > > virtio user == virtio device model? > > > > Yes > > > > > > Reasons: > > > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > > > > > maximum queue size possible. Which is actually the maximum > > > > queue size allowed by the virtio protocol. The appropriate > > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs > > > > 01.h > > > > tml#x1-240006 > > > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > > > more or less arbitrary value of 1024 in the past, which > > > > limits the maximum transfer size with virtio to 4M > > > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > > > being 4k). > > > > > > Being equal to IOV_MAX is a likely reason. Buffers with more iovecs than > > > that cannot be passed to host system calls (sendmsg(2), pwritev(2), > > > etc). > > > > Yes, that's use case dependent. Hence the solution to opt-in if it is > > desired and feasible. > > > > > > (2) Additionally the current value of 1024 poses a hidden limit, > > > > > > > > invisible to guest, which causes a system hang with the > > > > following QEMU error if guest tries to exceed it: > > > > > > > > virtio: too many write descriptors in indirect table > > > > > > I don't understand this point. 2.6.5 The Virtqueue Descriptor Table says: > > > The number of descriptors in the table is defined by the queue size > > > for > > > > > > this virtqueue: this is the maximum possible descriptor chain length. > > > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > A driver MUST NOT create a descriptor chain longer than the Queue Size > > > of > > > > > > the device. > > > > > > Do you mean a broken/malicious guest driver that is violating the spec? > > > That's not a hidden limit, it's defined by the spec. > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.html > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.html > > > > You can already go beyond that queue size at runtime with the indirection > > table. The only actual limit is the currently hard coded value of 1k > > pages. > > Hence the suggestion to turn that into a variable. > > Exceeding Queue Size is a VIRTIO spec violation. Drivers that operate > outsided the spec do so at their own risk. They may not be compatible > with all device implementations. Yes, I am ware about that. And still, this practice is already done, which apparently is not limited to 9pfs. > The limit is not hidden, it's Queue Size as defined by the spec :). > > If you have a driver that is exceeding the limit, then please fix the > driver. I absolutely understand your position, but I hope you also understand that this violation of the specs is a theoretical issue, it is not a real-life problem right now, and due to lack of man power unfortunately I have to prioritize real-life problems over theoretical ones ATM. Keep in mind that right now I am the only person working on 9pfs actively, I do this voluntarily whenever I find a free time slice, and I am not paid for it either. I don't see any reasonable way with reasonable effort to do what you are asking for here in 9pfs, and Greg may correct me here if I am saying anything wrong. If you are seeing any specific real-life issue here, then please tell me which one, otherwise I have to postpone that "specs violation" issue. There is still a long list of real problems that I need to hunt down in 9pfs, afterwards I can continue with theoretical ones if you want, but right now I simply can't, sorry. > > > > (3) Unfortunately not all virtio users in QEMU would currently > > > > > > > > work correctly with the new value of 32768. > > > > > > > > So let's turn this hard coded global value into a runtime > > > > variable as a first step in this commit, configurable for each > > > > virtio user by passing a corresponding value with virtio_init() > > > > call. > > > > > > virtio_add_queue() already has an int queue_size argument, why isn't > > > that enough to deal with the maximum queue size? There's probably a good > > > reason for it, but please include it in the commit description. > > > > [...] > > > > > Can you make this value per-vq instead of per-vdev since virtqueues can > > > have different queue sizes? > > > > > > The same applies to the rest of this patch. Anything using > > > vdev->queue_max_size should probably use vq->vring.num ins
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Tue, Oct 05, 2021 at 03:15:26PM +0200, Christian Schoenebeck wrote: > On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck wrote: > > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > > variable per virtio user. > > > > virtio user == virtio device model? > > Yes > > > > Reasons: > > > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > > > maximum queue size possible. Which is actually the maximum > > > queue size allowed by the virtio protocol. The appropriate > > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.h > > > tml#x1-240006 > > > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > > more or less arbitrary value of 1024 in the past, which > > > limits the maximum transfer size with virtio to 4M > > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > > being 4k). > > > > Being equal to IOV_MAX is a likely reason. Buffers with more iovecs than > > that cannot be passed to host system calls (sendmsg(2), pwritev(2), > > etc). > > Yes, that's use case dependent. Hence the solution to opt-in if it is desired > and feasible. > > > > (2) Additionally the current value of 1024 poses a hidden limit, > > > > > > invisible to guest, which causes a system hang with the > > > following QEMU error if guest tries to exceed it: > > > > > > virtio: too many write descriptors in indirect table > > > > I don't understand this point. 2.6.5 The Virtqueue Descriptor Table says: > > > > The number of descriptors in the table is defined by the queue size for > > this virtqueue: this is the maximum possible descriptor chain length. > > > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > > > A driver MUST NOT create a descriptor chain longer than the Queue Size of > > the device. > > > > Do you mean a broken/malicious guest driver that is violating the spec? > > That's not a hidden limit, it's defined by the spec. > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.html > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.html > > You can already go beyond that queue size at runtime with the indirection > table. The only actual limit is the currently hard coded value of 1k pages. > Hence the suggestion to turn that into a variable. Exceeding Queue Size is a VIRTIO spec violation. Drivers that operate outsided the spec do so at their own risk. They may not be compatible with all device implementations. The limit is not hidden, it's Queue Size as defined by the spec :). If you have a driver that is exceeding the limit, then please fix the driver. > > > (3) Unfortunately not all virtio users in QEMU would currently > > > > > > work correctly with the new value of 32768. > > > > > > So let's turn this hard coded global value into a runtime > > > variable as a first step in this commit, configurable for each > > > virtio user by passing a corresponding value with virtio_init() > > > call. > > > > virtio_add_queue() already has an int queue_size argument, why isn't > > that enough to deal with the maximum queue size? There's probably a good > > reason for it, but please include it in the commit description. > [...] > > Can you make this value per-vq instead of per-vdev since virtqueues can > > have different queue sizes? > > > > The same applies to the rest of this patch. Anything using > > vdev->queue_max_size should probably use vq->vring.num instead. > > I would like to avoid that and keep it per device. The maximum size stored > there is the maximum size supported by virtio user (or vortio device model, > however you want to call it). So that's really a limit per device, not per > queue, as no queue of the device would ever exceed that limit. > > Plus a lot more code would need to be refactored, which I think is > unnecessary. I'm against a per-device limit because it's a concept that cannot accurately describe reality. Some devices have multiple classes of virtqueues and they are sized differently, so a per-device limit is insufficient. virtio-net has separate rx_queue_size and tx_queue_size parameters (plus a control vq hardcoded to 64 descriptors). The specification already gives us Queue Size (vring.num in QEMU). The variable exists in QEMU and just needs to be used. If per-vq limits require a lot of work, please describe why. I think replacing the variable from this patch with virtio_queue_get_num() should be fairly straightforward, but maybe I'm missing something? (If you prefer VirtQueue *vq instead of the index-based virtio_queue_get_num() API, you can introduce a virtqueue_get_num() API.) Stefan signature.asc Description: PGP signature
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Dienstag, 5. Oktober 2021 14:45:56 CEST Stefan Hajnoczi wrote: > On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck wrote: > > Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime > > variable per virtio user. > > virtio user == virtio device model? Yes > > Reasons: > > > > (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical > > > > maximum queue size possible. Which is actually the maximum > > queue size allowed by the virtio protocol. The appropriate > > value for VIRTQUEUE_MAX_SIZE would therefore be 32768: > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.h > > tml#x1-240006 > > > > Apparently VIRTQUEUE_MAX_SIZE was instead defined with a > > more or less arbitrary value of 1024 in the past, which > > limits the maximum transfer size with virtio to 4M > > (more precise: 1024 * PAGE_SIZE, with the latter typically > > being 4k). > > Being equal to IOV_MAX is a likely reason. Buffers with more iovecs than > that cannot be passed to host system calls (sendmsg(2), pwritev(2), > etc). Yes, that's use case dependent. Hence the solution to opt-in if it is desired and feasible. > > (2) Additionally the current value of 1024 poses a hidden limit, > > > > invisible to guest, which causes a system hang with the > > following QEMU error if guest tries to exceed it: > > > > virtio: too many write descriptors in indirect table > > I don't understand this point. 2.6.5 The Virtqueue Descriptor Table says: > > The number of descriptors in the table is defined by the queue size for > this virtqueue: this is the maximum possible descriptor chain length. > > and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says: > > A driver MUST NOT create a descriptor chain longer than the Queue Size of > the device. > > Do you mean a broken/malicious guest driver that is violating the spec? > That's not a hidden limit, it's defined by the spec. https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00781.html https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00788.html You can already go beyond that queue size at runtime with the indirection table. The only actual limit is the currently hard coded value of 1k pages. Hence the suggestion to turn that into a variable. > > (3) Unfortunately not all virtio users in QEMU would currently > > > > work correctly with the new value of 32768. > > > > So let's turn this hard coded global value into a runtime > > variable as a first step in this commit, configurable for each > > virtio user by passing a corresponding value with virtio_init() > > call. > > virtio_add_queue() already has an int queue_size argument, why isn't > that enough to deal with the maximum queue size? There's probably a good > reason for it, but please include it in the commit description. [...] > Can you make this value per-vq instead of per-vdev since virtqueues can > have different queue sizes? > > The same applies to the rest of this patch. Anything using > vdev->queue_max_size should probably use vq->vring.num instead. I would like to avoid that and keep it per device. The maximum size stored there is the maximum size supported by virtio user (or vortio device model, however you want to call it). So that's really a limit per device, not per queue, as no queue of the device would ever exceed that limit. Plus a lot more code would need to be refactored, which I think is unnecessary. Best regards, Christian Schoenebeck
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Mon, Oct 04, 2021 at 09:38:04PM +0200, Christian Schoenebeck wrote:
> Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime
> variable per virtio user.
virtio user == virtio device model?
>
> Reasons:
>
> (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical
> maximum queue size possible. Which is actually the maximum
> queue size allowed by the virtio protocol. The appropriate
> value for VIRTQUEUE_MAX_SIZE would therefore be 32768:
>
>
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-240006
>
> Apparently VIRTQUEUE_MAX_SIZE was instead defined with a
> more or less arbitrary value of 1024 in the past, which
> limits the maximum transfer size with virtio to 4M
> (more precise: 1024 * PAGE_SIZE, with the latter typically
> being 4k).
Being equal to IOV_MAX is a likely reason. Buffers with more iovecs than
that cannot be passed to host system calls (sendmsg(2), pwritev(2),
etc).
> (2) Additionally the current value of 1024 poses a hidden limit,
> invisible to guest, which causes a system hang with the
> following QEMU error if guest tries to exceed it:
>
> virtio: too many write descriptors in indirect table
I don't understand this point. 2.6.5 The Virtqueue Descriptor Table says:
The number of descriptors in the table is defined by the queue size for this
virtqueue: this is the maximum possible descriptor chain length.
and 2.6.5.3.1 Driver Requirements: Indirect Descriptors says:
A driver MUST NOT create a descriptor chain longer than the Queue Size of the
device.
Do you mean a broken/malicious guest driver that is violating the spec?
That's not a hidden limit, it's defined by the spec.
> (3) Unfortunately not all virtio users in QEMU would currently
> work correctly with the new value of 32768.
>
> So let's turn this hard coded global value into a runtime
> variable as a first step in this commit, configurable for each
> virtio user by passing a corresponding value with virtio_init()
> call.
virtio_add_queue() already has an int queue_size argument, why isn't
that enough to deal with the maximum queue size? There's probably a good
reason for it, but please include it in the commit description.
>
> Signed-off-by: Christian Schoenebeck
> ---
> hw/9pfs/virtio-9p-device.c | 3 ++-
> hw/block/vhost-user-blk.c | 2 +-
> hw/block/virtio-blk.c | 3 ++-
> hw/char/virtio-serial-bus.c| 2 +-
> hw/display/virtio-gpu-base.c | 2 +-
> hw/input/virtio-input.c| 2 +-
> hw/net/virtio-net.c| 15 ---
> hw/scsi/virtio-scsi.c | 2 +-
> hw/virtio/vhost-user-fs.c | 2 +-
> hw/virtio/vhost-user-i2c.c | 3 ++-
> hw/virtio/vhost-vsock-common.c | 2 +-
> hw/virtio/virtio-balloon.c | 4 ++--
> hw/virtio/virtio-crypto.c | 3 ++-
> hw/virtio/virtio-iommu.c | 2 +-
> hw/virtio/virtio-mem.c | 2 +-
> hw/virtio/virtio-pmem.c| 2 +-
> hw/virtio/virtio-rng.c | 2 +-
> hw/virtio/virtio.c | 35 +++---
> include/hw/virtio/virtio.h | 5 -
> 19 files changed, 57 insertions(+), 36 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 54ee93b71f..cd5d95dd51 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -216,7 +216,8 @@ static void virtio_9p_device_realize(DeviceState *dev,
> Error **errp)
> }
>
> v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
> -virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
> +virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size,
> +VIRTQUEUE_MAX_SIZE);
> v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
> }
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index ba13cb87e5..336f56705c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -491,7 +491,7 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
> }
>
> virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> -sizeof(struct virtio_blk_config));
> +sizeof(struct virtio_blk_config), VIRTQUEUE_MAX_SIZE);
>
> s->virtqs = g_new(VirtQueue *, s->num_queues);
> for (i = 0; i < s->num_queues; i++) {
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f139cd7cc9..9c0f46815c 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1213,7 +1213,8 @@ static void virtio_blk_device_realize(DeviceState *dev,
> Error **errp)
>
> virtio_blk_set_config_size(s, s->host_features);
>
> -virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
> +virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size,
> +VIRTQUEUE_MAX_SIZE);
>
> s->blk = conf->conf.blk;
> s->rq = NULL;
> diff --git a/hw/char/virti
Re: [PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
On Mon, 4 Oct 2021 21:38:04 +0200
Christian Schoenebeck wrote:
> Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime
> variable per virtio user.
>
> Reasons:
>
> (1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical
> maximum queue size possible. Which is actually the maximum
> queue size allowed by the virtio protocol. The appropriate
> value for VIRTQUEUE_MAX_SIZE would therefore be 32768:
>
>
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-240006
>
> Apparently VIRTQUEUE_MAX_SIZE was instead defined with a
> more or less arbitrary value of 1024 in the past, which
> limits the maximum transfer size with virtio to 4M
> (more precise: 1024 * PAGE_SIZE, with the latter typically
> being 4k).
>
> (2) Additionally the current value of 1024 poses a hidden limit,
> invisible to guest, which causes a system hang with the
> following QEMU error if guest tries to exceed it:
>
> virtio: too many write descriptors in indirect table
>
> (3) Unfortunately not all virtio users in QEMU would currently
> work correctly with the new value of 32768.
>
> So let's turn this hard coded global value into a runtime
> variable as a first step in this commit, configurable for each
> virtio user by passing a corresponding value with virtio_init()
> call.
>
> Signed-off-by: Christian Schoenebeck
> ---
Reviewed-by: Greg Kurz
> hw/9pfs/virtio-9p-device.c | 3 ++-
> hw/block/vhost-user-blk.c | 2 +-
> hw/block/virtio-blk.c | 3 ++-
> hw/char/virtio-serial-bus.c| 2 +-
> hw/display/virtio-gpu-base.c | 2 +-
> hw/input/virtio-input.c| 2 +-
> hw/net/virtio-net.c| 15 ---
> hw/scsi/virtio-scsi.c | 2 +-
> hw/virtio/vhost-user-fs.c | 2 +-
> hw/virtio/vhost-user-i2c.c | 3 ++-
> hw/virtio/vhost-vsock-common.c | 2 +-
> hw/virtio/virtio-balloon.c | 4 ++--
> hw/virtio/virtio-crypto.c | 3 ++-
> hw/virtio/virtio-iommu.c | 2 +-
> hw/virtio/virtio-mem.c | 2 +-
> hw/virtio/virtio-pmem.c| 2 +-
> hw/virtio/virtio-rng.c | 2 +-
> hw/virtio/virtio.c | 35 +++---
> include/hw/virtio/virtio.h | 5 -
> 19 files changed, 57 insertions(+), 36 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 54ee93b71f..cd5d95dd51 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -216,7 +216,8 @@ static void virtio_9p_device_realize(DeviceState *dev,
> Error **errp)
> }
>
> v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
> -virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
> +virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size,
> +VIRTQUEUE_MAX_SIZE);
> v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
> }
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index ba13cb87e5..336f56705c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -491,7 +491,7 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
> }
>
> virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> -sizeof(struct virtio_blk_config));
> +sizeof(struct virtio_blk_config), VIRTQUEUE_MAX_SIZE);
>
> s->virtqs = g_new(VirtQueue *, s->num_queues);
> for (i = 0; i < s->num_queues; i++) {
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f139cd7cc9..9c0f46815c 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1213,7 +1213,8 @@ static void virtio_blk_device_realize(DeviceState *dev,
> Error **errp)
>
> virtio_blk_set_config_size(s, s->host_features);
>
> -virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
> +virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size,
> +VIRTQUEUE_MAX_SIZE);
>
> s->blk = conf->conf.blk;
> s->rq = NULL;
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index f01ec2137c..9ad915 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1045,7 +1045,7 @@ static void virtio_serial_device_realize(DeviceState
> *dev, Error **errp)
> config_size = offsetof(struct virtio_console_config, emerg_wr);
> }
> virtio_init(vdev, "virtio-serial", VIRTIO_ID_CONSOLE,
> -config_size);
> +config_size, VIRTQUEUE_MAX_SIZE);
>
> /* Spawn a new virtio-serial bus on which the ports will ride as devices
> */
> qbus_init(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index c8da4806e0..20b06a7adf 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -171,7 +171,7 @@ virtio_gpu_ba
[PATCH v2 1/3] virtio: turn VIRTQUEUE_MAX_SIZE into a variable
Refactor VIRTQUEUE_MAX_SIZE to effectively become a runtime
variable per virtio user.
Reasons:
(1) VIRTQUEUE_MAX_SIZE should reflect the absolute theoretical
maximum queue size possible. Which is actually the maximum
queue size allowed by the virtio protocol. The appropriate
value for VIRTQUEUE_MAX_SIZE would therefore be 32768:
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-240006
Apparently VIRTQUEUE_MAX_SIZE was instead defined with a
more or less arbitrary value of 1024 in the past, which
limits the maximum transfer size with virtio to 4M
(more precise: 1024 * PAGE_SIZE, with the latter typically
being 4k).
(2) Additionally the current value of 1024 poses a hidden limit,
invisible to guest, which causes a system hang with the
following QEMU error if guest tries to exceed it:
virtio: too many write descriptors in indirect table
(3) Unfortunately not all virtio users in QEMU would currently
work correctly with the new value of 32768.
So let's turn this hard coded global value into a runtime
variable as a first step in this commit, configurable for each
virtio user by passing a corresponding value with virtio_init()
call.
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/virtio-9p-device.c | 3 ++-
hw/block/vhost-user-blk.c | 2 +-
hw/block/virtio-blk.c | 3 ++-
hw/char/virtio-serial-bus.c| 2 +-
hw/display/virtio-gpu-base.c | 2 +-
hw/input/virtio-input.c| 2 +-
hw/net/virtio-net.c| 15 ---
hw/scsi/virtio-scsi.c | 2 +-
hw/virtio/vhost-user-fs.c | 2 +-
hw/virtio/vhost-user-i2c.c | 3 ++-
hw/virtio/vhost-vsock-common.c | 2 +-
hw/virtio/virtio-balloon.c | 4 ++--
hw/virtio/virtio-crypto.c | 3 ++-
hw/virtio/virtio-iommu.c | 2 +-
hw/virtio/virtio-mem.c | 2 +-
hw/virtio/virtio-pmem.c| 2 +-
hw/virtio/virtio-rng.c | 2 +-
hw/virtio/virtio.c | 35 +++---
include/hw/virtio/virtio.h | 5 -
19 files changed, 57 insertions(+), 36 deletions(-)
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 54ee93b71f..cd5d95dd51 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -216,7 +216,8 @@ static void virtio_9p_device_realize(DeviceState *dev,
Error **errp)
}
v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
-virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
+virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size,
+VIRTQUEUE_MAX_SIZE);
v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
}
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index ba13cb87e5..336f56705c 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -491,7 +491,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev,
Error **errp)
}
virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
-sizeof(struct virtio_blk_config));
+sizeof(struct virtio_blk_config), VIRTQUEUE_MAX_SIZE);
s->virtqs = g_new(VirtQueue *, s->num_queues);
for (i = 0; i < s->num_queues; i++) {
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f139cd7cc9..9c0f46815c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1213,7 +1213,8 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
virtio_blk_set_config_size(s, s->host_features);
-virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
+virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size,
+VIRTQUEUE_MAX_SIZE);
s->blk = conf->conf.blk;
s->rq = NULL;
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index f01ec2137c..9ad915 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1045,7 +1045,7 @@ static void virtio_serial_device_realize(DeviceState
*dev, Error **errp)
config_size = offsetof(struct virtio_console_config, emerg_wr);
}
virtio_init(vdev, "virtio-serial", VIRTIO_ID_CONSOLE,
-config_size);
+config_size, VIRTQUEUE_MAX_SIZE);
/* Spawn a new virtio-serial bus on which the ports will ride as devices */
qbus_init(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index c8da4806e0..20b06a7adf 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -171,7 +171,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
g->virtio_config.num_scanouts = cpu_to_le32(g->conf.max_outputs);
virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
-sizeof(struct virtio_gpu_config));
+sizeof(struct virtio_gpu_config), VIRTQUEUE_MAX_SIZE);
if (virtio_gpu_virgl_enable
