driver/virtio/blk: Ensure request header stays in-page boundaries AND some minor sglist cleanup #1290

mogasergiu · 2024-01-29T14:31:00Z

Prerequisite checklist

Read the contribution guidelines regarding submitting new changes to the project;
Tested your changes against relevant architectures and platforms;
Ran the checkpatch.uk on your commit series before opening this PR;
Updated relevant documentation.

Base target

Architecture(s): N/A
Platform(s): kvm
Application(s): N/A

Additional configuration

Description of changes

Move some sglist stuff

netbuf appending to sglist method to lib/uknetdev/netbuf.c
some sglist definitions/macro's for saving/restoring to library header

The important addition of the PR:

The header (first descriptor) of the virtio-blk request must always
come in one piece! By aligning to its size (16), we ensure that
it will be contained entirely in one page only, i.e. the
scatter-gather list will not process it as two segments, which
would result in two descriptors.

E.g. If the address ends something like 0x...ff8 then the header
will span 0x...ff8 -> 0x...008 crossing the next page and resulting
in the scatter-gather list splitting it into two segments and
thus into two descriptors, which QEMU seems to not like.

To help with this, declare the request header at the beginning
of the virtio-blk request structure and make its allocation
16-byte aligned, guaranteeing its length will never cross the page
boundary.

lib/uknetdev/netbuf.c

andreistan26

Looks good,

Reviewed-by: Andrei Stan andreistan2003@gmail.com

razvanvirtan

Looks good to me, thanks @mogasergiu!
Only one nitpick: I suggest having some consistency about the "scatter-gather" writing.
For instance, in this comment the two words are separated by a dash, while in this one they are not.

Reviewed-by: Razvan Virtan virtanrazvan@gmail.com

skuenzer

Hey @mogasergiu, I have only some minor comments, otherwise, this PR looks good. 👍🏻

lib/uknetdev/include/uk/netbuf.h

lib/uksglist/include/uk/sglist.h

lib/uknetdev/include/uk/netbuf.h

It may come in handy to do scatter-gather list save and restore in places other than the scatter-gather library. Therefore, allow others to see the corresponding definitions by placing them in the header. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

Rename `uk_sglist_append_netbuf` to `uk_netbuf_sglist_append` and move it to the libuknetdev, since that is its only client. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

The header (first descriptor) of the virtio-blk request must always come in one piece! By aligning to its size (16), we ensure that it will be contained entirely in one page only, i.e. the scatter-gather list will not process it as two segments, which would result in two descriptors. E.g. If the address ends something like 0x...ff8 then the header will span 0x...ff8 -> 0x...008 crossing the next page and resulting in the scatter-gather list splitting it into two segments and thus into two descriptors, which QEMU seems to not like. To help with this, declare the request header at the beginning of the virtio-blk request structure and make its allocation 16-byte aligned, guaranteeing its length will never cross the page boundary. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

skuenzer

Thanks for the updates.

Approved-by: Simon Kuenzer simon@unikraft.io

It may come in handy to do scatter-gather list save and restore in places other than the scatter-gather library. Therefore, allow others to see the corresponding definitions by placing them in the header. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #1290

Rename `uk_sglist_append_netbuf` to `uk_netbuf_sglist_append` and move it to the libuknetdev, since that is its only client. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #1290

The header (first descriptor) of the virtio-blk request must always come in one piece! By aligning to its size (16), we ensure that it will be contained entirely in one page only, i.e. the scatter-gather list will not process it as two segments, which would result in two descriptors. E.g. If the address ends something like 0x...ff8 then the header will span 0x...ff8 -> 0x...008 crossing the next page and resulting in the scatter-gather list splitting it into two segments and thus into two descriptors, which QEMU seems to not like. To help with this, declare the request header at the beginning of the virtio-blk request structure and make its allocation 16-byte aligned, guaranteeing its length will never cross the page boundary. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #1290

It may come in handy to do scatter-gather list save and restore in places other than the scatter-gather library. Therefore, allow others to see the corresponding definitions by placing them in the header. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: unikraft#1290

Rename `uk_sglist_append_netbuf` to `uk_netbuf_sglist_append` and move it to the libuknetdev, since that is its only client. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: unikraft#1290

The header (first descriptor) of the virtio-blk request must always come in one piece! By aligning to its size (16), we ensure that it will be contained entirely in one page only, i.e. the scatter-gather list will not process it as two segments, which would result in two descriptors. E.g. If the address ends something like 0x...ff8 then the header will span 0x...ff8 -> 0x...008 crossing the next page and resulting in the scatter-gather list splitting it into two segments and thus into two descriptors, which QEMU seems to not like. To help with this, declare the request header at the beginning of the virtio-blk request structure and make its allocation 16-byte aligned, guaranteeing its length will never cross the page boundary. Signed-off-by: Sergiu Moga <sergiu@unikraft.io> Reviewed-by: Razvan Virtan <virtanrazvan@gmail.com> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: unikraft#1290

mogasergiu requested review from a team as code owners January 29, 2024 14:31

github-actions bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/uknetdev lib/uksglist labels Jan 29, 2024

mogasergiu requested review from skuenzer and removed request for a team January 29, 2024 14:32

mogasergiu assigned skuenzer Jan 29, 2024

skuenzer reviewed Jan 31, 2024

View reviewed changes

lib/uknetdev/netbuf.c Show resolved Hide resolved

razvand added this to the v0.17.0 (Calypso) milestone Feb 1, 2024

razvand requested review from razvanvirtan and andreistan26 March 13, 2024 20:39

mogasergiu force-pushed the smoga/#1290/sglist_virtioblk_outhdr branch 2 times, most recently from 4fc766a to 020f7fa Compare March 15, 2024 15:16

mogasergiu requested a review from skuenzer March 15, 2024 15:18

andreistan26 approved these changes Mar 16, 2024

View reviewed changes

mogasergiu force-pushed the smoga/#1290/sglist_virtioblk_outhdr branch from 020f7fa to fb3dbe3 Compare March 18, 2024 19:41

razvanvirtan approved these changes Mar 18, 2024

View reviewed changes

skuenzer requested changes Mar 18, 2024

View reviewed changes

mogasergiu added 3 commits March 19, 2024 11:03

lib/{uksglist,uknetdev}: Move uk_sglist_append_netbuf to uknetdev

34d8ddd

Rename `uk_sglist_append_netbuf` to `uk_netbuf_sglist_append` and move it to the libuknetdev, since that is its only client. Signed-off-by: Sergiu Moga <sergiu@unikraft.io>

mogasergiu force-pushed the smoga/#1290/sglist_virtioblk_outhdr branch from fb3dbe3 to fb49749 Compare March 19, 2024 09:04

mogasergiu requested a review from skuenzer March 19, 2024 09:05

skuenzer approved these changes Mar 19, 2024

View reviewed changes

razvand changed the base branch from staging to staging-1290 March 20, 2024 06:12

razvand merged commit 610d845 into unikraft:staging-1290 Mar 20, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver/virtio/blk: Ensure request header stays in-page boundaries AND some minor sglist cleanup #1290

driver/virtio/blk: Ensure request header stays in-page boundaries AND some minor sglist cleanup #1290

mogasergiu commented Jan 29, 2024 •

edited

Loading

andreistan26 left a comment

razvanvirtan left a comment

skuenzer left a comment

skuenzer left a comment

driver/virtio/blk: Ensure request header stays in-page boundaries **AND** some minor sglist cleanup #1290

driver/virtio/blk: Ensure request header stays in-page boundaries **AND** some minor sglist cleanup #1290

Conversation

mogasergiu commented Jan 29, 2024 • edited Loading

Prerequisite checklist

Base target

Additional configuration

Description of changes

andreistan26 left a comment

Choose a reason for hiding this comment

razvanvirtan left a comment

Choose a reason for hiding this comment

skuenzer left a comment

Choose a reason for hiding this comment

skuenzer left a comment

Choose a reason for hiding this comment

driver/virtio/blk: Ensure request header stays in-page boundaries AND some minor sglist cleanup #1290

driver/virtio/blk: Ensure request header stays in-page boundaries AND some minor sglist cleanup #1290

mogasergiu commented Jan 29, 2024 •

edited

Loading