plat/drivers/virtio: Fix virtio read write

[zhxj9823]

Add support for variable length of virtio read and write.

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

Description of changes

The vm_get and vm_set in virtio_mmio can only handle reading/writing 1/2/4/8 bytes. However, there are several cases, such as in virtio_net and virtio_9p, that the length of bytes to read is not supported by this function. This PR adds support to this case by using _virtio_cread_bytes and _virtio_cwrite_bytes functions in the default case of vm_set and vm_get functions correspondingly. This approach turns an n-byte read/write operation into n 1-byte read/write operations.

[unikraft-bot]

✅ Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA	commit	checkpatch
fa4a7ea	plat/drivers/virtio: Fix virtio read write	✅

[dinhngtu]

All looks good to me.

Reviewed-by: Tu Dinh Ngoc dinhngoc.tu@irit.fr

[Krechals]

Thanks for fixing this, @zhxj9823. I'm fine with the changes. I would only add a comment as to why we are handling virtio-net (mac address) and virtio-9p (tag) in this manner, especially given that the VirtIO 1.0/1.1 specs state that the driver shouldn't do a byte by byte handling:

4.2.2.2 Driver Requirements: MMIO Device Register Layout

The driver MUST only use 32 bit wide and aligned reads and 
writes to access the control registers described in table [4.1].

For the device-specific configuration space, the driver MUST use 
8 bit wide accesses for 8 bit wide fields, 
16 bit wide and aligned accesses for 16 bit wide fields 
and 32 bit wide and aligned accesses for 32 and 64 bit wide fields.

Since we cannot avoid braking the atomicity (the mac address has 6 bytes), I would say this change is fine.
Linux does it the same: https://elixir.bootlin.com/linux/v6.4.2/source/drivers/net/virtio_net.c#L2230

The only issue with removing the assertions is that when we do so, we might silence a future bug. IMO, the risk is quite low, as hopefully these code paths won't be require many changes in the future. Without additional refactoring, which I don't believe is necessary at this time, I don't see a clear method to handle this appropriately.

[zhxj9823]

Thanks for fixing this, @zhxj9823. I'm fine with the changes. I would only add a comment as to why we are handling virtio-net (mac address) and virtio-9p (tag) in this manner, especially given that the VirtIO 1.0/1.1 specs state that the driver shouldn't do a byte by byte handling:

4.2.2.2 Driver Requirements: MMIO Device Register Layout

The driver MUST only use 32 bit wide and aligned reads and 
writes to access the control registers described in table [4.1].

For the device-specific configuration space, the driver MUST use 
8 bit wide accesses for 8 bit wide fields, 
16 bit wide and aligned accesses for 16 bit wide fields 
and 32 bit wide and aligned accesses for 32 and 64 bit wide fields.

Since we cannot avoid braking the atomicity (the mac address has 6 bytes), I would say this change is fine. Linux does it the same: https://elixir.bootlin.com/linux/v6.4.2/source/drivers/net/virtio_net.c#L2230

The only issue with removing the assertions is that when we do so, we might silence a future bug. IMO, the risk is quite low, as hopefully these code paths won't be require many changes in the future. Without additional refactoring, which I don't believe is necessary at this time, I don't see a clear method to handle this appropriately.

Thanks for the comment.

An alternative way of handling this issue is to explicitly use _virtio_cread_bytes and _virtio_cwrite_bytes on every unaligned read and write operation, and Linux does it in a way similar to this. This includes the cases in virtio-net and virtio-9p, and possibly other cases. Since I am not familiar with virtio, I am unable to do such things, so adding a case in vm_get and vm_set is quicker and simpler.

Besides, I think we can add a warning message in this case to give a hint if anything wrong arises.

[michpappas]

@zhxj9823 please add a comment on the virtio-net and virtio-9p side as requested by @Krechals so that we can get this merged. On the virtio-net there is a comment already that needs to be adapted: https://github.com/unikraft/unikraft/blob/staging/plat/drivers/virtio/virtio_net.c#L932

You can also add a warning if you wish on the virtio-mmio side.

[michpappas]

@Krechals, @eduardvintila please provide your feedback / sign-off so that we can merge this in 0.14, thanks! 🚀

[eduardvintila]

Looks good, thanks.

Reviewed-by: Eduard Vintilă eduard.vintila47@gmail.com

[Krechals]

LGTM. Thank you, @zhxj9823!

Reviewed-by: Andrei Topala topala.andrei@gmail.com

[michpappas]

Approved-by: Michalis Pappas <michalis.unikraft.io>