[PATCH 1/2] drm/i915/gvt: Move mdev attribute groups into kvmgt module

[Jason Gunthorpe]

On Wed, Apr 28, 2021 at 01:32:43PM +0800, Zhenyu Wang wrote:
> On 2021.04.27 09:12:35 -0300, Jason Gunthorpe wrote:
> > On Tue, Apr 27, 2021 at 10:45:06AM +0800, Zhenyu Wang wrote:
> > > On 2021.04.26 10:39:24 -0300, Jason Gunthorpe wrote:
> > > > On Mon, Apr 26, 2021 at 05:41:42PM +0800, Zhenyu Wang wrote:
> > > > > @@ -1667,19 +1773,26 @@ static struct mdev_parent_ops intel_vgpu_ops = {
> > > > >  
> > > > >  static int kvmgt_host_init(struct device *dev, void *gvt, const void *ops)
> > > > >  {
> > > > > -	struct attribute_group **kvm_vgpu_type_groups;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = intel_gvt_init_vgpu_type_groups((struct intel_gvt *)gvt);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > >  
> > > > >  	intel_gvt_ops = ops;
> > > > > -	if (!intel_gvt_ops->get_gvt_attrs(&kvm_vgpu_type_groups))
> > > > > -		return -EFAULT;
> > > > > -	intel_vgpu_ops.supported_type_groups = kvm_vgpu_type_groups;
> > > > > +	intel_vgpu_ops.supported_type_groups = gvt_vgpu_type_groups;
> > > > 
> > > > This patch is an improvement, but this fictional dynamic behavior is
> > > > still wrong. The supported_type_groups directly flows from the
> > > > vgpu_types array in vgpu.c and it should not be split up like this
> > > > 
> > > > The code copies the rodata vgpu_types into dynamic memory gvt->types
> > > > then copies that dynamic memory into a dynamic gvt_vgpu_type_groups,
> > > > which makes very little sense at all.
> > > 
> > > vgpu_types is static for we want fixed vgpu mdev type, but actual supported
> > > types depend on HW resources e.g aperture mem, fence, etc, that's dynamic for
> > > gvt->types, so gvt_vgpu_type_groups is dynamic from gvt->types.
> > 
> > Well, that's worse then, intel_vgpu_ops.supported_type_groups is a
> > static global, it cannot depend on HW variable calculations.
> 
> sorry, maybe I didn't describe this properly, gvt_vgpu_type_groups is
> static global, but the actual supported types is determined from
> gvt->types and initialized before mdev device register.

No, I got it, I saw the code too.

Think about what happens if there are two GPUs in the system, you get
two gvt's and with different properties and you are trying to squeeze
that into a single static global - it doesn't work. 

Jason