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/[email protected]
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~
