Re: [PATCH v3 8/8] gpu: nova-core: add NVKV GSP_INIT schemas

["Eliot Courtney" <ecourtney@nvidia.com>]

On Wed Oct 7, 2026 at 7:53 PM JST, Alexandre Courbot wrote:
[...]
>> +impl RegKey {
>> +    // Define the Key IDs read/written by GSP.
>> +    const REGKEY_NAME_KEY: KeyId = 0x3070;
>> +    const REGKEY_VALUE_U32_KEY: KeyId = 0x3071;
>
> I guess the `REGKEY` prefix is unneeded here since it's the name of the
> wrapping type. If we want a common prefix, let's use `KEY`, e.g.
> `KEY_NAME`? Although we might not even need these at all - please see my
> last comment on this patch.

Removed the constants here as you suggested.

>
>> +}
>> +
>> +impl Encodable for KVVec<RegKey> {
>> +    fn encode(&self, encoder: &mut Encoder) -> Result {
>> +        for regkey in self {
>> +            regkey.encode(encoder)?;
>> +        }
>> +        Ok(())
>> +    }
>> +}
>> +
>> +nvkv_encode! {
>> +    /// SR-IOV virtual function information.
>> +    struct VfInfo {
>> +        total_vfs: Key<u32, { Self::VF_TOTAL_VFS_KEY }>,
>> +        first_vf_offset: Key<u32, { Self::VF_FIRST_VF_OFFSET_KEY }>,
>> +        flags: Key<u64, { Self::VF_FLAGS_KEY }>,
>> +        first_bar0_address: Key<u64, { Self::VF_FIRST_BAR0_ADDRESS_KEY }>,
>> +        first_bar1_address: Key<u64, { Self::VF_FIRST_BAR1_ADDRESS_KEY }>,
>> +        first_bar2_address: Key<u64, { Self::VF_FIRST_BAR2_ADDRESS_KEY }>,
>> +    }
>> +}
>> +
>> +impl VfInfo {
>> +    // Define the Key IDs read/written by GSP.
>> +    const VF_TOTAL_VFS_KEY: KeyId = 0x0080;
>> +    const VF_FIRST_VF_OFFSET_KEY: KeyId = 0x0081;
>> +    const VF_FLAGS_KEY: KeyId = 0x1003;
>> +    const VF_FIRST_BAR0_ADDRESS_KEY: KeyId = 0x1050;
>> +    const VF_FIRST_BAR1_ADDRESS_KEY: KeyId = 0x1051;
>> +    const VF_FIRST_BAR2_ADDRESS_KEY: KeyId = 0x1052;
>> +}
>
> That's a bit of boilerplate. Ideally we would have this `#[nvkv(key =
> value)]` notation (without the bidirectional feature) you mentioned in
> [1] that takes the literal value and defines a constant to access it,
> but I guess that's more rework than we want for now.
>
> [1] https://lore.kernel.org/DLIHSKKUVP7V.153Z4TTEYFL3Y@nvidia.com

Yes, I think that's a bigger rework where we have the macro generate the
schema types. We could consider that later IMO.

>
>> +
>> +nvkv_encode! {
>> +    /// Payload of the `GSP_INIT` command.
>> +    // TODO: expect() doesn't work here due to Self:: reference, fixed in 1.97.0
>> +    // https://github.com/rust-lang/rust/pull/154377
>> +    #[cfg_attr(not(CONFIG_KUNIT), allow(dead_code))]
>> +    struct GspInitRequest {
>> +        pci_device_id: Key<u32, { Self::PCI_DEVICE_ID_KEY }>,
>> +        pci_sub_device_id: Key<u32, { Self::PCI_SUBDEVICE_ID_KEY }>,
>> +        pci_revision_id: Key<u32, { Self::PCI_REVISION_ID_KEY }>,
>> +        pci_config_mirror_base: Key<u32, { Self::PCI_CONFIG_MIRROR_BASE_KEY }>,
>> +        pci_config_mirror_size: Key<u32, { Self::PCI_CONFIG_MIRROR_SIZE_KEY }>,
>> +        host_arch: Key<HostArch, { Self::HOST_ARCH_KEY }, u32>,
>> +        bus_device_func: Key<u64, { Self::NV_DOMAIN_BUS_DEVICE_FUNC_KEY }>,
>> +        regkeys: KVVec<RegKey>,
>> +        vf_info: Option<VfInfo>,
>> +    }
>> +}
>> +
>> +impl GspInitRequest {
>> +    // Define the Key IDs read/written by GSP.
>> +    const PCI_DEVICE_ID_KEY: KeyId = 0x0001;
>> +    const PCI_SUBDEVICE_ID_KEY: KeyId = 0x0002;
>> +    const PCI_REVISION_ID_KEY: KeyId = 0x0003;
>> +    const PCI_CONFIG_MIRROR_BASE_KEY: KeyId = 0x0010;
>> +    const PCI_CONFIG_MIRROR_SIZE_KEY: KeyId = 0x0011;
>> +    const HOST_ARCH_KEY: KeyId = 0x0070;
>> +    const NV_DOMAIN_BUS_DEVICE_FUNC_KEY: KeyId = 0x1020;
>> +}
>> +
>> +// Decode:
>> +
>> +// Should decode with UnknownKeyPolicy::Ignore.
>> +nvkv_decode! {
>> +    /// Schema for the `GSP_INIT` response.
>> +    // TODO: expect() doesn't work here due to Self:: reference, fixed in 1.97.0
>> +    // https://github.com/rust-lang/rust/pull/154377
>> +    #[cfg_attr(not(CONFIG_KUNIT), allow(dead_code))]
>> +    struct GspInitResponseSchema => GspInitResponse {
>> +        gpu_name:
>> +            Array<u8, { GspInitResponse::MAX_GPU_NAME_LEN }, { Self::GPU_NAME_STRING_KEY }>,
>> +        fb_regions: Accumulated<FbRegionSchema>,
>> +        bar1_pde_base: Required<u64, { Self::BAR1_PDE_BASE_KEY }>,
>> +        vmmu_segment_size: Key<u64, { Self::VMMU_SEGMENT_SIZE_KEY }>,
>
> Is this ok to have `vmmu_segment_size` not `Required`?

Let me check. OpenRM's behaviour here is to check against 0, however.

[...]
>> +
>> +#[kunit_tests(nova_core_fw_commands)]
>> +mod tests {
>
> These tests below mostly re-check the same things as the previous
> patches, only with different data. I am not sure they bring much new
> coverage, except maybe nested structs (which we could/should also cover
> in the previous patches anyway). I think we would have more value (and
> less code) if we covered the things newly tested to the tests of patches
> 4-7, and dropped the test module here altogether. Because if we follow
> the pattern, then we will repeat these tests again and again for every
> command we support, which is going to result in tons of redundant tests
> at the end of the day.
>
> Another incentive for not having more tests here: they are the only
> other user of the key constants, outside of the `nvkv_decode` and
> `nvkv_encode` macros themselves. If we can get rid of them then the
> macros become the only users of these consts, and in that case why not
> replace
>
>     pci_device_id: Key<u32, { Self::PCI_DEVICE_ID_KEY }>,
>     ...
>     const PCI_DEVICE_ID_KEY: KeyId = 0x0001;
>
> with just
>
>     pci_device_id: Key<u32, 0x0001>,
>
> as it is clear from that line alone that `0x0001` is the key for
> `pci_device_id`. Bonus, the unsightly brackets required because we
> reference `Self` can also go away.

Will drop these, I agree with your point that it's noisy to add these
everywhere we have commands. I also folded in the constants like you
suggest.

Thank you for your reviews~