Re: [PATCH 02/14] drm/i915/gvt: simplify vgpu configuration management

From: Jason Gunthorpe <jgg@nvidia.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Kirti Wankhede <kwankhede@nvidia.com>,
	Tony Krowiak <akrowiak@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Jason Herne <jjherne@linux.ibm.com>,
	Eric Farman <farman@linux.ibm.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	Zhenyu Wang <zhenyuw@linux.intel.com>,
	Zhi Wang <zhi.a.wang@intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	intel-gvt-dev@lists.freedesktop.org
Subject: Re: [PATCH 02/14] drm/i915/gvt: simplify vgpu configuration management
Date: Mon, 4 Jul 2022 11:15:36 -0300	[thread overview]
Message-ID: <20220704141536.GB1423020@nvidia.com> (raw)
In-Reply-To: <20220704125144.157288-3-hch@lst.de>

On Mon, Jul 04, 2022 at 02:51:32PM +0200, Christoph Hellwig wrote:

> +/*
> + * vGPU type name is defined as GVTg_Vx_y which contains the physical GPU
> + * generation type (e.g V4 as BDW server, V5 as SKL server).
> + *
> + * Depening on the physical SKU resource, we might see vGPU types like
> + * GVTg_V4_8, GVTg_V4_4, GVTg_V4_2, etc. We can create different types of
> + * vGPU on same physical GPU depending on available resource. Each vGPU
> + * type will have a different number of avail_instance to indicate how
> + * many vGPU instance can be created for this type.
> + */
>  #define VGPU_MAX_WEIGHT 16
>  #define VGPU_WEIGHT(vgpu_num)	\
>  	(VGPU_MAX_WEIGHT / (vgpu_num))
>  
> -static const struct {
> -	unsigned int low_mm;
> -	unsigned int high_mm;
> -	unsigned int fence;
> -
> -	/* A vGPU with a weight of 8 will get twice as much GPU as a vGPU
> -	 * with a weight of 4 on a contended host, different vGPU type has
> -	 * different weight set. Legal weights range from 1 to 16.
> -	 */
> -	unsigned int weight;
> -	enum intel_vgpu_edid edid;
> -	const char *name;
> -} vgpu_types[] = {
> -/* Fixed vGPU type table */
> +static const struct intel_vgpu_config intel_vgpu_configs[] = {
>  	{ MB_TO_BYTES(64), MB_TO_BYTES(384), 4, VGPU_WEIGHT(8), GVT_EDID_1024_768, "8" },
>  	{ MB_TO_BYTES(128), MB_TO_BYTES(512), 4, VGPU_WEIGHT(4), GVT_EDID_1920_1200, "4" },
>  	{ MB_TO_BYTES(256), MB_TO_BYTES(1024), 4, VGPU_WEIGHT(2), GVT_EDID_1920_1200, "2" },
> @@ -106,63 +103,34 @@ static const struct {
>   */

[..]

>  	for (i = 0; i < num_types; ++i) {
> -		if (low_avail / vgpu_types[i].low_mm == 0)
> -			break;
> -
> -		gvt->types[i].low_gm_size = vgpu_types[i].low_mm;
> -		gvt->types[i].high_gm_size = vgpu_types[i].high_mm;
> -		gvt->types[i].fence = vgpu_types[i].fence;
> +		const struct intel_vgpu_config *conf = &intel_vgpu_configs[i];
>  
> -		if (vgpu_types[i].weight < 1 ||
> -					vgpu_types[i].weight > VGPU_MAX_WEIGHT)
> +		if (low_avail / conf->low_mm == 0)
> +			break;
> +		if (conf->weight < 1 || conf->weight > VGPU_MAX_WEIGHT)
>  			goto out_free_types;

This is now clearly impossible right? Maybe a BUILD_BUG_ON is all that
is needed:

  #define VGPU_WEIGHT(vgpu_num)	\
         (VGPU_MAX_WEIGHT + BUILD_BUG_ON_ZERO((vgpu_num) > VGPU_MAX_WEIGHT) / (vgpu_num))

> +		sprintf(gvt->types[i].name, "GVTg_V%u_%s",
> +			GRAPHICS_VER(gvt->gt->i915) == 8 ? 4 : 5, conf->name);
> +		gvt->types->conf = conf;
> +		gvt->types[i].avail_instance = min(low_avail / conf->low_mm,
> +						   high_avail / conf->high_mm);

snprintf and check for failure?

Regardless, makes sense to me:

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason