Re: [PATCH 1/6] drm/amdgpu: Updated naming of SRIOV critical region offsets/sizes with _V1 suffix

["Christian König" <christian.koenig@amd.com>]

That starts to look better, just a few notes:

Please use "git send-email" to send out the patches. I can review them, but they don't seem to be send as consecutive patch set which makes it a bit tricky to track it in patchwork.

On 09.10.25 05:29, Ellen Pan wrote:
>  - This change updated existing SRIOV critical offsets with suffix _V1.
>  - Therefore to prepare for the later patches to intro SRIOV critical region v2.
> 
> Signed-off-by: Ellen Pan <yunru.pan@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com> for this patch and patch #2 in the series.

When a patch only touches amdgpu_virt.c and/or amdgv_sriovmsg.h it is fine with me that you review that inside your team.

So feel free to add my rb together with the one of teammate and push the patches to amd-staging-drm-next.

Regards,
Christian.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c    | 20 ++++----
>  drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h | 57 ++++++++++++++-------
>  2 files changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 3328ab63376b..e95adf0407a0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -686,7 +686,7 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
>  		/* got through this logic in early init stage to get necessary flags, e.g. rlcg_acc related*/
>  		adev->virt.fw_reserve.p_pf2vf =
>  			(struct amd_sriov_msg_pf2vf_info_header *)
> -			(adev->bios + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +			(adev->bios + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB_V1 << 10));
>  
>  		amdgpu_virt_read_pf2vf_data(adev);
>  	}
> @@ -703,21 +703,21 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
>  		if (adev->mman.fw_vram_usage_va) {
>  			adev->virt.fw_reserve.p_pf2vf =
>  				(struct amd_sriov_msg_pf2vf_info_header *)
> -				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB_V1 << 10));
>  			adev->virt.fw_reserve.p_vf2pf =
>  				(struct amd_sriov_msg_vf2pf_info_header *)
> -				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB_V1 << 10));
>  			adev->virt.fw_reserve.ras_telemetry =
> -				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_RAS_TELEMETRY_OFFSET_KB << 10));
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_RAS_TELEMETRY_OFFSET_KB_V1 << 10));
>  		} else if (adev->mman.drv_vram_usage_va) {
>  			adev->virt.fw_reserve.p_pf2vf =
>  				(struct amd_sriov_msg_pf2vf_info_header *)
> -				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB_V1 << 10));
>  			adev->virt.fw_reserve.p_vf2pf =
>  				(struct amd_sriov_msg_vf2pf_info_header *)
> -				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB_V1 << 10));
>  			adev->virt.fw_reserve.ras_telemetry =
> -				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_RAS_TELEMETRY_OFFSET_KB << 10));
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_RAS_TELEMETRY_OFFSET_KB_V1 << 10));
>  		}
>  
>  		amdgpu_virt_read_pf2vf_data(adev);
> @@ -1304,7 +1304,7 @@ static int amdgpu_virt_cache_host_error_counts(struct amdgpu_device *adev,
>  	checksum = host_telemetry->header.checksum;
>  	used_size = host_telemetry->header.used_size;
>  
> -	if (used_size > (AMD_SRIOV_RAS_TELEMETRY_SIZE_KB << 10))
> +	if (used_size > (AMD_SRIOV_MSG_RAS_TELEMETRY_SIZE_KB_V1 << 10))
>  		return 0;
>  
>  	tmp = kmemdup(&host_telemetry->body.error_count, used_size, GFP_KERNEL);
> @@ -1383,7 +1383,7 @@ amdgpu_virt_write_cpers_to_ring(struct amdgpu_device *adev,
>  	checksum = host_telemetry->header.checksum;
>  	used_size = host_telemetry->header.used_size;
>  
> -	if (used_size > (AMD_SRIOV_RAS_TELEMETRY_SIZE_KB << 10))
> +	if (used_size > (AMD_SRIOV_MSG_RAS_TELEMETRY_SIZE_KB_V1 << 10))
>  		return -EINVAL;
>  
>  	cper_dump = kmemdup(&host_telemetry->body.cper_dump, used_size, GFP_KERNEL);
> @@ -1515,7 +1515,7 @@ static int amdgpu_virt_cache_chk_criti_hit(struct amdgpu_device *adev,
>  	checksum = host_telemetry->header.checksum;
>  	used_size = host_telemetry->header.used_size;
>  
> -	if (used_size > (AMD_SRIOV_RAS_TELEMETRY_SIZE_KB << 10))
> +	if (used_size > (AMD_SRIOV_MSG_RAS_TELEMETRY_SIZE_KB_V1 << 10))
>  		return 0;
>  
>  	tmp = kmemdup(&host_telemetry->body.chk_criti, used_size, GFP_KERNEL);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h b/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
> index 3a79ed7d8031..3b35154e2df6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgv_sriovmsg.h
> @@ -23,26 +23,47 @@
>  #ifndef AMDGV_SRIOV_MSG__H_
>  #define AMDGV_SRIOV_MSG__H_
>  
> -/* unit in kilobytes */
> -#define AMD_SRIOV_MSG_VBIOS_OFFSET	     0
> -#define AMD_SRIOV_MSG_VBIOS_SIZE_KB	     64
> -#define AMD_SRIOV_MSG_DATAEXCHANGE_OFFSET_KB AMD_SRIOV_MSG_VBIOS_SIZE_KB
> -#define AMD_SRIOV_MSG_DATAEXCHANGE_SIZE_KB   4
> -#define AMD_SRIOV_MSG_TMR_OFFSET_KB	     2048
> -#define AMD_SRIOV_MSG_BAD_PAGE_SIZE_KB	     2
> -#define AMD_SRIOV_RAS_TELEMETRY_SIZE_KB	     64
>  /*
> - * layout
> + * layout v1
>   * 0           64KB        65KB        66KB           68KB                   132KB
>   * |   VBIOS   |   PF2VF   |   VF2PF   |   Bad Page   | RAS Telemetry Region | ...
>   * |   64KB    |   1KB     |   1KB     |   2KB        | 64KB                 | ...
>   */
>  
> -#define AMD_SRIOV_MSG_SIZE_KB                   1
> -#define AMD_SRIOV_MSG_PF2VF_OFFSET_KB           AMD_SRIOV_MSG_DATAEXCHANGE_OFFSET_KB
> -#define AMD_SRIOV_MSG_VF2PF_OFFSET_KB           (AMD_SRIOV_MSG_PF2VF_OFFSET_KB + AMD_SRIOV_MSG_SIZE_KB)
> -#define AMD_SRIOV_MSG_BAD_PAGE_OFFSET_KB        (AMD_SRIOV_MSG_VF2PF_OFFSET_KB + AMD_SRIOV_MSG_SIZE_KB)
> -#define AMD_SRIOV_MSG_RAS_TELEMETRY_OFFSET_KB   (AMD_SRIOV_MSG_BAD_PAGE_OFFSET_KB + AMD_SRIOV_MSG_BAD_PAGE_SIZE_KB)
> +/*
> + * layout v2 (offsets are dynamically allocated and the offsets below are examples)
> + * 0           1KB         64KB        65KB        66KB           68KB                   132KB
> + * |  INITD_H  |   VBIOS   |   PF2VF   |   VF2PF   |   Bad Page   | RAS Telemetry Region | ...
> + * |   1KB     |   64KB    |   1KB     |   1KB     |   2KB        | 64KB                 | ...
> + *
> + * Note: PF2VF + VF2PF + Bad Page = DataExchange region (allocated contiguously)
> + */
> +
> +/* v1 layout sizes */
> +#define AMD_SRIOV_MSG_VBIOS_SIZE_KB_V1			64
> +#define AMD_SRIOV_MSG_PF2VF_SIZE_KB_V1			1
> +#define AMD_SRIOV_MSG_VF2PF_SIZE_KB_V1			1
> +#define AMD_SRIOV_MSG_BAD_PAGE_SIZE_KB_V1		2
> +#define AMD_SRIOV_MSG_RAS_TELEMETRY_SIZE_KB_V1		64
> +#define AMD_SRIOV_MSG_DATAEXCHANGE_SIZE_KB_V1		\
> +	(AMD_SRIOV_MSG_PF2VF_SIZE_KB_V1 + AMD_SRIOV_MSG_VF2PF_SIZE_KB_V1 + \
> +	 AMD_SRIOV_MSG_BAD_PAGE_SIZE_KB_V1)
> +
> +/* v1 offsets */
> +#define AMD_SRIOV_MSG_VBIOS_OFFSET_V1			0
> +#define AMD_SRIOV_MSG_DATAEXCHANGE_OFFSET_KB_V1		AMD_SRIOV_MSG_VBIOS_SIZE_KB_V1
> +#define AMD_SRIOV_MSG_TMR_OFFSET_KB			2048
> +#define AMD_SRIOV_MSG_SIZE_KB_V1			1
> +#define AMD_SRIOV_MSG_PF2VF_OFFSET_KB_V1		AMD_SRIOV_MSG_DATAEXCHANGE_OFFSET_KB_V1
> +#define AMD_SRIOV_MSG_VF2PF_OFFSET_KB_V1		\
> +	(AMD_SRIOV_MSG_PF2VF_OFFSET_KB_V1 + AMD_SRIOV_MSG_SIZE_KB_V1)
> +#define AMD_SRIOV_MSG_BAD_PAGE_OFFSET_KB_V1		\
> +	(AMD_SRIOV_MSG_VF2PF_OFFSET_KB_V1 + AMD_SRIOV_MSG_SIZE_KB_V1)
> +#define AMD_SRIOV_MSG_RAS_TELEMETRY_OFFSET_KB_V1	\
> +	(AMD_SRIOV_MSG_BAD_PAGE_OFFSET_KB_V1 + AMD_SRIOV_MSG_BAD_PAGE_SIZE_KB_V1)
> +#define AMD_SRIOV_MSG_INIT_DATA_TOT_SIZE_KB_V1		\
> +	(AMD_SRIOV_MSG_VBIOS_SIZE_KB_V1 + AMD_SRIOV_MSG_DATAEXCHANGE_SIZE_KB_V1 + \
> +	 AMD_SRIOV_MSG_RAS_TELEMETRY_SIZE_KB_V1)
>  
>  /*
>   * PF2VF history log:
> @@ -436,12 +457,12 @@ unsigned int amd_sriov_msg_checksum(void *obj, unsigned long obj_size, unsigned
>  #define _stringification(s) #s
>  
>  _Static_assert(
> -	sizeof(struct amd_sriov_msg_vf2pf_info) == AMD_SRIOV_MSG_SIZE_KB << 10,
> -	"amd_sriov_msg_vf2pf_info must be " stringification(AMD_SRIOV_MSG_SIZE_KB) " KB");
> +	sizeof(struct amd_sriov_msg_vf2pf_info) == AMD_SRIOV_MSG_SIZE_KB_V1 << 10,
> +	"amd_sriov_msg_vf2pf_info must be " stringification(AMD_SRIOV_MSG_SIZE_KB_V1) " KB");
>  
>  _Static_assert(
> -	sizeof(struct amd_sriov_msg_pf2vf_info) == AMD_SRIOV_MSG_SIZE_KB << 10,
> -	"amd_sriov_msg_pf2vf_info must be " stringification(AMD_SRIOV_MSG_SIZE_KB) " KB");
> +	sizeof(struct amd_sriov_msg_pf2vf_info) == AMD_SRIOV_MSG_SIZE_KB_V1 << 10,
> +	"amd_sriov_msg_pf2vf_info must be " stringification(AMD_SRIOV_MSG_SIZE_KB_V1) " KB");
>  
>  _Static_assert(AMD_SRIOV_MSG_RESERVE_UCODE % 4 == 0,
>  	       "AMD_SRIOV_MSG_RESERVE_UCODE must be multiple of 4");