From: Zhenyu Wang <zhenyuw@linux.intel.com>
Subject: Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated
	resources
Date: Fri, 27 Jul 2018 10:01:56 +0800
Message-ID: <20180727020156.GM1267@zhen-hp.sh.intel.com>
References: <20180620074039.10539-1-zhenyuw@linux.intel.com>
	<20180720021928.15343-1-zhenyuw@linux.intel.com>
	<20180724114440.76564e75@t450s.home>
Reply-To: Zhenyu Wang <zhenyuw@linux.intel.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3019418335588507981=="
Cc: kevin.tian@intel.com, kvm@vger.kernel.org, libvirt-list@redhat.com,
	kwankhede@nvidia.com, intel-gvt-dev@lists.freedesktop.org,
	Zhi Wang <zhi.a.wang@intel.com>
To: Alex Williamson <alex.williamson@redhat.com>
Return-path: <libvir-list-bounces@redhat.com>
In-Reply-To: <20180724114440.76564e75@t450s.home>
List-Unsubscribe: <https://www.redhat.com/mailman/options/libvir-list>,
	<mailto:libvir-list-request@redhat.com?subject=unsubscribe>
List-Archive: <https://www.redhat.com/archives/libvir-list>
List-Post: <mailto:libvir-list@redhat.com>
List-Help: <mailto:libvir-list-request@redhat.com?subject=help>
List-Subscribe: <https://www.redhat.com/mailman/listinfo/libvir-list>,
	<mailto:libvir-list-request@redhat.com?subject=subscribe>
Sender: libvir-list-bounces@redhat.com
Errors-To: libvir-list-bounces@redhat.com
List-Id: kvm.vger.kernel.org


--===============3019418335588507981==
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="fUETZ9jfdlrBCYso"
Content-Disposition: inline


--fUETZ9jfdlrBCYso
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2018.07.24 11:44:40 -0600, Alex Williamson wrote:
> On Fri, 20 Jul 2018 10:19:24 +0800
> Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
>=20
> > Current mdev device create interface depends on fixed mdev type, which =
get uuid
> > from user to create instance of mdev device. If user wants to use custo=
mized
> > number of resource for mdev device, then only can create new mdev type =
for that
> > which may not be flexible. This requirement comes not only from to be a=
ble to
> > allocate flexible resources for KVMGT, but also from Intel scalable IO
> > virtualization which would use vfio/mdev to be able to allocate arbitra=
ry
> > resources on mdev instance. More info on [1] [2] [3].
> >=20
> > To allow to create user defined resources for mdev, it trys to extend m=
dev
> > create interface by adding new "instances=3Dxxx" parameter following uu=
id, for
> > target mdev type if aggregation is supported, it can create new mdev de=
vice
> > which contains resources combined by number of instances, e.g
> >=20
> >     echo "<uuid>,instances=3D10" > create
> >=20
> > VM manager e.g libvirt can check mdev type with "aggregation" attribute=
 which
> > can support this setting. If no "aggregation" attribute found for mdev =
type,
> > previous behavior is still kept for one instance allocation. And new sy=
sfs
> > attribute "instances" is created for each mdev device to show allocated=
 number.
> >=20
> > This trys to create new KVMGT type with minimal vGPU resources which ca=
n be
> > combined with "instances=3Dx" setting to allocate for user wanted resou=
rces.
>=20
> "instances" makes me think this is arg helps to create multiple mdev
> instances rather than consuming multiple instances for a single mdev.
> You're already exposing the "aggregation" attribute, so doesn't
> "aggregate" perhaps make more sense as the create option?  We're asking
> the driver to aggregate $NUM instances into a single mdev.  The mdev
> attribute should then perhaps also be "aggregated_instances".

yeah that seems better.

>=20
> The next user question for the interface might be what aspect of the
> device gets multiplied by this aggregation?  In i915 I see you're
> multiplying the memory sizes by the instance, but clearly the
> resolution doesn't change.  I assume this is sort of like mdev types
> themselves, ie. some degree of what a type means is buried in the
> implementation and some degree of what some number of those types
> aggregated together means is impossible to describe generically.
>

yeah, the purpose was to increase memory resource only, but due to current
free formatted 'description', can't have a meaningful expression for that,
and not sure if libvirt likes to understand vendor specific behavior e.g
for intel vGPU?

> We're also going to need to add aggregation to the checklist for device
> compatibility for migration, for example 1) is it the same mdev_type,
> 1a) are the aggregation counts the same (new), 2) is the host driver
> compatible (TBD).
>

Right, will check with Zhi on that.

> The count handling in create_store(), particularly MDEV_CREATE_OPT_LEN
> looks a little suspicious.  I think we should just be validating that
> the string before the ',' or the entire string if there is no comma is
> UUID length.  Pass only that to uuid_le_to_bin().  We can then strncmp
> as you have for "instances=3D" (or "aggregate=3D") but then let's be sure
> to end the string we pass to kstrtouint(), ie. assume there could be
> further args.

Original purpose was to limit the length of string to accept, but can take
this way without issue I think.

>=20
> Documentation/ABI/testing/sysfs-bus-vfio-mdev also needs to be updated
> with this series.

Ok.

Thanks

>=20
> I'm curious what libvirt folks and Kirti think of this, it looks like
> it has a nice degree of backwards compatibility, both in the sysfs
> interface and the vendor driver interface.  Thanks,
>=20
> Alex

--=20
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

--fUETZ9jfdlrBCYso
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iF0EARECAB0WIQTXuabgHDW6LPt9CICxBBozTXgYJwUCW1p9FAAKCRCxBBozTXgY
JxSeAJwIxAEIaUp3qOzCFLEnQT9XeNk5yQCfVmDrz7cPyCp3Ceu14KdHjt592Cw=
=DJgp
-----END PGP SIGNATURE-----

--fUETZ9jfdlrBCYso--


--===============3019418335588507981==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline


--===============3019418335588507981==--