plat/drivers/virtio: Fix virtio_9p tag read

[zhxj9823]

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): app-sqlite

Description of changes

Swap the last two arguments of virtio_9p_feature_negotiate's first call to virtio_config_get. As per the function definition, the last argument needs to actually be the length of the type.

[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
95a56c8	plat/drivers/virtio: Fix virtio_9p tag read	✅

[mogasergiu]

I have tested this with nginx and redis.
I am not sure this is correct. How do you know tag_len is ignored?
vpci_legacy_pci_config_get seems to be using it, or am I missing something?
Although nothing seems to break, I think the commit message is misleading.

[StefanJum]

All good, thanks.
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

[StefanJum]

I have tested this with nginx and redis. I am not sure this is correct. How do you know tag_len is ignored? vpci_legacy_pci_config_get seems to be using it, or am I missing something? Although nothing seems to break, I think the commit message is misleading.

Agree with that actually, nothing should break since in the end the vpci_legacy_pci_config_get uses len * type_len, which stays the same (I tested it with everything and it works fine), but the commit message is misleading.
Maybe just say that the virtio_mmio vm_get function ignores the last argument, instead of The underlying function of virtio_config_get.

[mogasergiu]

Maybe just say that the virtio_mmio vm_get function ignores the last argument, instead of The underlying function of virtio_config_get.

virtio_mmio vm_get is not related to 9pfs, right? 9pfs is PCI based. Better to not mention about vm_get.
What about just:

Swap the last two arguments of `virtio_9p_feature_negotiate`'s first call to `virtio_config_get`. As per the function definition, the last argument needs to actually be the length of the type.

[StefanJum]

virtio_mmio vm_get is not related to 9pfs, right? It's PCI based. Better to not mention about vm_get. What about just:

Swap the order of `virtio_9p_feature_negotiate` first call to `virtio_config_get`. As per the function definition, the last argument needs to actually be the length of the type.

Yeah much better 😄

[mogasergiu]

virtio_mmio vm_get is not related to 9pfs, right? It's PCI based. Better to not mention about vm_get. What about just:

Swap the last two arguments of `virtio_9p_feature_negotiate`'s first call to `virtio_config_get`. As per the function definition, the last argument needs to actually be the length of the type.

Yeah much better 😄

@zhxj9823 If you agree, please change the commit message accordingly and I will approve.

[razvand]

@mogasergiu , @StefanJum , is it OK now, after @zhxj9823 's update?

I re-request a review from both of you.

[mogasergiu]

Thank you for the bug fix!

Reviewed-by: Sergiu Moga sergiu@unikraft.io

[StefanJum]

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

[razvand]

Approved-by: Razvan Deaconescu razvand@unikraft.io