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.
