Alexandre Courbot
2025-Sep-26 02:20 UTC
[PATCH v2 05/10] gpu: nova-core: gsp: Add GSP command queue handling
On Thu Sep 25, 2025 at 3:32 PM JST, Alistair Popple wrote: <snip>>> > + #[expect(unused)] >> > + pub(crate) fn receive_msg_from_gsp<M: GspMessageFromGsp, R>( >> > + &mut self, >> > + timeout: Delta, >> > + init: impl FnOnce(&M, SBuffer<core::array::IntoIter<&[u8], 2>>) -> Result<R>, >> > + ) -> Result<R> { >> > + let (driver_area, msg_header, slice_1) = wait_on(timeout, || { >> > + let driver_area = self.gsp_mem.driver_read_area(); >> > + // TODO: find an alternative to as_flattened() >> > + #[allow(clippy::incompatible_msrv)] >> > + let (msg_header_slice, slice_1) = driver_area >> > + .0 >> > + .as_flattened() >> > + .split_at(size_of::<GspMsgElement>()); >> > + >> > + // Can't fail because msg_slice will always be >> > + // size_of::<GspMsgElement>() bytes long by the above split. >> > + let msg_header = GspMsgElement::from_bytes(msg_header_slice).unwrap(); >> >> Any reason we're not just using unwrap_unchecked() here then? > > Because whilst my assertions about the code are currently correct if it ever > changes I figured it would be better to explicitly panic than end up with > undefined behaviour. Is there some other advantage to using unwrap_unchecked()? > I can't imagine there'd be much of a performance difference.Here I think we should just use the `?` operator. The function already returns a `Result` so it would fit. I'd be willing to consider unwrapping is this can prevent an obviously-unfallible method from having to return a `Result` - but here this is not the case, and handling the error doesn't cost us more than the `unwrap`, so let's do that. <snip>>> > +impl GspRpcHeader { >> > + pub(crate) fn new(cmd_size: u32, function: u32) -> Self { >> > + Self { >> > + // TODO: magic number >> > + header_version: 0x03000000, >> > + signature: bindings::NV_VGPU_MSG_SIGNATURE_VALID, >> > + function, >> > + // TODO: overflow check? >> > + length: size_of::<Self>() as u32 + cmd_size, >> >> (just curious, do you mean overflow as in arith overflow or overflow as in >> going past the boundaries of the header?) > > Actually this snuck in from some of Alex's suggested code improvements (I had > intended to credit him in the commit message! Will fix that) so maybe he can > answer what he had in mind? I assumed arith overflow but maybe he meant ring > buffer overflow or something.I was thinking about arithmetic overflow, but maybe that was just overthinking. :) We're probably not going to send a 4 GB payload anytime soon...
Alistair Popple
2025-Sep-29 01:06 UTC
[PATCH v2 05/10] gpu: nova-core: gsp: Add GSP command queue handling
On 2025-09-26 at 12:20 +1000, Alexandre Courbot <acourbot at nvidia.com> wrote...> On Thu Sep 25, 2025 at 3:32 PM JST, Alistair Popple wrote: > <snip> > >> > + #[expect(unused)] > >> > + pub(crate) fn receive_msg_from_gsp<M: GspMessageFromGsp, R>( > >> > + &mut self, > >> > + timeout: Delta, > >> > + init: impl FnOnce(&M, SBuffer<core::array::IntoIter<&[u8], 2>>) -> Result<R>, > >> > + ) -> Result<R> { > >> > + let (driver_area, msg_header, slice_1) = wait_on(timeout, || { > >> > + let driver_area = self.gsp_mem.driver_read_area(); > >> > + // TODO: find an alternative to as_flattened() > >> > + #[allow(clippy::incompatible_msrv)] > >> > + let (msg_header_slice, slice_1) = driver_area > >> > + .0 > >> > + .as_flattened() > >> > + .split_at(size_of::<GspMsgElement>()); > >> > + > >> > + // Can't fail because msg_slice will always be > >> > + // size_of::<GspMsgElement>() bytes long by the above split. > >> > + let msg_header = GspMsgElement::from_bytes(msg_header_slice).unwrap(); > >> > >> Any reason we're not just using unwrap_unchecked() here then? > > > > Because whilst my assertions about the code are currently correct if it ever > > changes I figured it would be better to explicitly panic than end up with > > undefined behaviour. Is there some other advantage to using unwrap_unchecked()? > > I can't imagine there'd be much of a performance difference. > > Here I think we should just use the `?` operator. The function already > returns a `Result` so it would fit.Actually note quite true - this is in a closure that must return `Option<_>` so returning `Result` doesn't fit. However it still fits because I just noticed `::from_bytes()` returns an `Option` so `?` will still work.> I'd be willing to consider unwrapping is this can prevent an > obviously-unfallible method from having to return a `Result` - but here > this is not the case, and handling the error doesn't cost us more > than the `unwrap`, so let's do that.Agreed. I assumed from_bytes() returned `Result<_>` which would not have worked rather than `Option<_>` which will though.> <snip> > >> > +impl GspRpcHeader { > >> > + pub(crate) fn new(cmd_size: u32, function: u32) -> Self { > >> > + Self { > >> > + // TODO: magic number > >> > + header_version: 0x03000000, > >> > + signature: bindings::NV_VGPU_MSG_SIGNATURE_VALID, > >> > + function, > >> > + // TODO: overflow check? > >> > + length: size_of::<Self>() as u32 + cmd_size, > >> > >> (just curious, do you mean overflow as in arith overflow or overflow as in > >> going past the boundaries of the header?) > > > > Actually this snuck in from some of Alex's suggested code improvements (I had > > intended to credit him in the commit message! Will fix that) so maybe he can > > answer what he had in mind? I assumed arith overflow but maybe he meant ring > > buffer overflow or something. > > I was thinking about arithmetic overflow, but maybe that was just > overthinking. :) We're probably not going to send a 4 GB payload anytime > soon...Lets hope not :) I guess we might want `checked_add()` to panic if we've gone insane though so have done that.