> On Dec 18, 2025, at 10:26 PM, Alexandre Courbot <[email protected]> wrote: > > On Fri Dec 19, 2025 at 5:52 AM JST, Joel Fernandes wrote: >> Hi Alex, >> >>>>> + } >>>>> +} >>>>> diff --git a/drivers/gpu/nova-core/gsp/fw.rs >>>>> b/drivers/gpu/nova-core/gsp/fw.rs >>>>> index 09549f7db52d..228464fd47a0 100644 >>>>> --- a/drivers/gpu/nova-core/gsp/fw.rs >>>>> +++ b/drivers/gpu/nova-core/gsp/fw.rs >>>>> @@ -209,6 +209,7 @@ pub(crate) enum MsgFunction { >>>>> GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU, >>>>> GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL, >>>>> GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO, >>>>> + UnloadingGuestDriver = >>>>> bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER, >>>>> >>>>> // Event codes >>>>> GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE, >>>>> @@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> { >>>>> } >>>>> bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => >>>>> Ok(MsgFunction::GspRmControl), >>>>> bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => >>>>> Ok(MsgFunction::GetStaticInfo), >>>>> + bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => { >>>>> + Ok(MsgFunction::UnloadingGuestDriver) >>>>> + } >>>>> bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => >>>>> Ok(MsgFunction::GspInitDone), >>>>> bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => { >>>>> Ok(MsgFunction::GspRunCpuSequencer) >>>>> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs >>>>> b/drivers/gpu/nova-core/gsp/fw/commands.rs >>>>> index 85465521de32..c7df4783ad21 100644 >>>>> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs >>>>> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs >>>>> @@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {} >>>>> // SAFETY: This struct only contains integer types for which all bit >>>>> patterns >>>>> // are valid. >>>>> unsafe impl FromBytes for GspStaticConfigInfo {} >>>>> + >>>>> +/// Payload of the `UnloadingGuestDriver` command and message. >>>>> +#[repr(transparent)] >>>>> +#[derive(Clone, Copy, Debug, Zeroable)] >>>>> +pub(crate) struct UnloadingGuestDriver { >>>>> + inner: bindings::rpc_unloading_guest_driver_v1F_07, >>>>> +} >>>>> + >>>>> +impl UnloadingGuestDriver { >>>>> + pub(crate) fn new(suspend: bool) -> Self { >>>>> + Self { >>>>> + inner: bindings::rpc_unloading_guest_driver_v1F_07 { >>>> >>>> We should go through intermediate firmware representation than direct >>>> bindings access? >>> >>> Only if the size of the bindings justifies it - here having an opaque >>> wrapper just just well, and spares us some code down the line as we >>> would have to initialize the bindings anyway. >> >> I am not sure about that, it sounds like a layering violation. It would be >> good not to keep the rules fuzzy about this, because then we could do it >> either way in all cases. >> >> Another reason is that we cannot anticipate in advance which specific helper >> functions we will need to add in the future. Down the line, we may need to >> add some helper functions to the struct as well. Also having V1F07 in the >> name sounds very magic number-ish. It would be good to abstract that out >> with a better-named struct anyway. > > The rules are not fuzzy. The only thing modules outside of `fw` are > seeing is a struct named `UnloadingGuestDriver`, and the name with > `V1F07` is not leaked. > > Even if we had a different structure, we would still need to write the > rpc_unloading_guest_driver_v1F_07 at some point, so we would need to > refer to it in `fw` anyway. The current design is not any more lax than > that, it just removes one step.
Ah, I missed that. This is all in FW, so I think that clarifies the rule for me now. Thank you.
