Add methods to Transport to read and write config space, rather than returning a pointer. #163
Conversation
|
Nice, I was looking for something like this! Having Transport offer the config space access is a good idea IMHO. I had tried stuffing the platform specific IO operations into the Hal and had volread / volwrite require the Hal as input. The Transport then of course also depends on the Hal ( |
returning a pointer. The config space is not necessarily memory mapped, some other transports require the config space to be accessed via other IO operations such as hypercalls.
This also makes some device methods return a Result.
qwandor
requested review from
chyyuu,
equation314,
wangrunji0408 and
DeathWish5
| let capacity = unsafe { | ||
| volread!(config, capacity_low) as u64 | (volread!(config, capacity_high) as u64) << 32 | ||
| }; | ||
| let capacity = transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_low))? |
There was a problem hiding this comment.
In the new code, the you have to be careful about keeping the template param and the field types in sync at every call point. It looks easy to mess up. I wonder if we could have a macro like read_config_space!(transport, BlkConfig, capacity_low) that handles the offset and makes sure the types line up properly.
There was a problem hiding this comment.
I had the same thought, but can't find any way to do so as there doesn't seem to be any macro to get the type of a struct field from its name.
There was a problem hiding this comment.
Here is a trick
macro_rules! offset_and_size {
($t:ty, $f:ident) => {{
let v: $t = Default::default();
(std::mem::offset_of!($t, $f), std::mem::size_of_val(&v.$f))
}};
}
Seems to optimize out https://godbolt.org/z/hzWGjqxY1
There was a problem hiding this comment.
I don't think getting the size helps, the issue is to get the type. But I think I have found a way around that, I'll send another PR in a bit.
| }; | ||
| let jacks = transport.read_config_space(offset_of!(VirtIOSoundConfig, jacks))?; | ||
| let streams = transport.read_config_space(offset_of!(VirtIOSoundConfig, streams))?; | ||
| let chmaps = transport.read_config_space(offset_of!(VirtIOSoundConfig, chmaps))?; |
There was a problem hiding this comment.
inferring the types seems dangerous here since the field declaration is far away
There was a problem hiding this comment.
The important thing here is that the type of e.g. VirtIOSound::jacks must match the type of VirtIOSoundConfig::jacks. I'm not sure that adding an explicit type parameter here really helps with that.
| }; | ||
| let capacity = transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_low))? | ||
| as u64 | ||
| | (transport.read_config_space::<u32>(offset_of!(BlkConfig, capacity_high))? as u64) |
There was a problem hiding this comment.
not related to this PR, but technically you should use the config generation field when doing multiple part reads like this: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-230001
(FWIW, the crosvm VMM currently always has config_generation == 0)
There was a problem hiding this comment.
Good point, I missed that. Done in #169.
| align_of::<T>()); | ||
| assert!(offset % align_of::<T>() == 0); | ||
|
|
||
| // SAFETY: The caller of `MmioTransport::new` guaranteed that the header pointer was valid, |
There was a problem hiding this comment.
the device tree should contain an upper bound for the config space (from the reg field i think) and you could check that offset doesn't go beyond it
There was a problem hiding this comment.
Good idea, will do.
| align_of::<T>()); | ||
| assert!(offset % align_of::<T>() == 0); | ||
|
|
||
| // SAFETY: The caller of `MmioTransport::new` guaranteed that the header pointer was valid, |
| Ok(config_space_ptr.cast()) | ||
| Ok((config_space.as_ptr() as *mut T) | ||
| .byte_add(offset) | ||
| .read_volatile()) |
There was a problem hiding this comment.
maybe too pedantic, but this allows any T and that doesn't satify src must point to a properly initialized value of type T. could add T: zerocopy::FromBytes if that dep is available
There was a problem hiding this comment.
Good point. I already had T: FromBytes as a constraint on the Transport trait, but I'll add it here on the implementation too.
| // TODO: Use NonNull::as_non_null_ptr once it is stable. | ||
| let config_space_ptr = NonNull::new(config_space.as_ptr() as *mut u32).unwrap(); | ||
| Ok(config_space_ptr.cast()) | ||
| Ok((config_space.as_ptr() as *mut T) |
There was a problem hiding this comment.
technically only need *const T here i think
There was a problem hiding this comment.
I'll change this to pointer::cast instead.
The config space is not necessarily memory mapped, some other transports require the config space to be accessed via other IO operations such as hypercalls.