From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D7A8AC2BCA1 for ; Fri, 7 Jun 2019 20:03:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A9622206C3 for ; Fri, 7 Jun 2019 20:03:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730625AbfFGUDu (ORCPT ); Fri, 7 Jun 2019 16:03:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59134 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729342AbfFGUDu (ORCPT ); Fri, 7 Jun 2019 16:03:50 -0400 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B5C4388318; Fri, 7 Jun 2019 20:03:49 +0000 (UTC) Received: from x1.home (ovpn-116-22.phx2.redhat.com [10.3.116.22]) by smtp.corp.redhat.com (Postfix) with ESMTP id 010C91001943; Fri, 7 Jun 2019 20:03:45 +0000 (UTC) Date: Fri, 7 Jun 2019 14:03:44 -0600 From: Alex Williamson To: Tony Krowiak Cc: Cornelia Huck , kvm@vger.kernel.org, libvir-list@redhat.com, Matthew Rosato , Halil Pasic Subject: Re: [PATCH RFC 1/1] allow to specify additional config data Message-ID: <20190607140344.0399b766@x1.home> In-Reply-To: References: <20190606144417.1824-1-cohuck@redhat.com> <20190606144417.1824-2-cohuck@redhat.com> <20190606093224.3ecb92c7@x1.home> <20190606101552.6fc62bef@x1.home> Organization: Red Hat MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 07 Jun 2019 20:03:49 +0000 (UTC) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, 7 Jun 2019 14:26:13 -0400 Tony Krowiak wrote: > On 6/6/19 12:15 PM, Alex Williamson wrote: > > On Thu, 6 Jun 2019 09:32:24 -0600 > > Alex Williamson wrote: > > > >> On Thu, 6 Jun 2019 16:44:17 +0200 > >> Cornelia Huck wrote: > >> > >>> Add a rough implementation for vfio-ap. > >>> > >>> Signed-off-by: Cornelia Huck > >>> --- > >>> mdevctl.libexec | 25 ++++++++++++++++++++++ > >>> mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- > >>> 2 files changed, 80 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/mdevctl.libexec b/mdevctl.libexec > >>> index 804166b5086d..cc0546142924 100755 > >>> --- a/mdevctl.libexec > >>> +++ b/mdevctl.libexec > >>> @@ -54,6 +54,19 @@ wait_for_supported_types () { > >>> fi > >>> } > >>> > >>> +# configure vfio-ap devices > >>> +configure_ap_devices() { > >>> + list="`echo "${config[$1]}" | sed 's/,/ /'`" > >>> + [ -z "$list" ] && return > >>> + for a in $list; do > >>> + echo "$a" > "$supported_types/${config[mdev_type]}/devices/$uuid/$2" > >>> + if [ $? -ne 0 ]; then > >>> + echo "Error writing '$a' to '$uuid/$2'" >&2 > >>> + exit 1 > >>> + fi > >>> + done > >>> +} > >>> + > >>> case ${1} in > >>> start-mdev|stop-mdev) > >>> if [ $# -ne 2 ]; then > >>> @@ -148,6 +161,18 @@ case ${cmd} in > >>> echo "Error creating mdev type ${config[mdev_type]} on $parent" >&2 > >>> exit 1 > >>> fi > >>> + > >>> + # some types may specify additional config data > >>> + case ${config[mdev_type]} in > >>> + vfio_ap-passthrough) > >> > >> I think this could have some application beyond ap too, I know NVIDIA > >> GRID vGPUs do have some controls under the vendor hierarchy of the > >> device, ex. setting the frame rate limiter. The implementation here is > >> a bit rigid, we know a specific protocol for a specific mdev type, but > >> for supporting arbitrary vendor options we'd really just want to try to > >> apply whatever options are provided. If we didn't care about ordering, > >> we could just look for keys for every file in the device's immediate > >> sysfs hierarchy and apply any value we find, independent of the > >> mdev_type, ex. intel_vgpu/foo=bar Thanks, > > > > For example: > > > > for key in find -P $mdev_base/$uuid/ \( -path > > "$mdev_base/$uuid/power/*" -o -path $mdev_base/$uuid/uevent -o -path $mdev_base/$uuid/remove \) -prune -o -type f -print | sed -e "s|$mdev_base/$uuid/||g"); do > > [ -z ${config[$key]} ] && continue > > ... parse value(s) and iteratively apply to key > > done > > > > The find is a little ugly to exclude stuff, maybe we just let people do > > screwy stuff like specify remove=1 in their config. Also need to think > > about whether we're imposing a delimiter to apply multiple values to a > > key that conflicts with the attribute usage. Thanks, > > > > Alex > > I like the idea of looking for files in the device's immediate sysfs > hierarchy, but maybe the find could exclude attributes that are > not vendor defined. How would we know what attributes are vendor defined? The above `find` strips out the power, uevent, and remove attributes, which for GVT-g leaves only the vendor defined attributes[1], but I don't know how to instead do a positive match of the vendor attributes without unmaintainable lookup tables. This starts to get into the question of how much do we want to (or need to) protect the user from themselves. If we let the user specify a key=value of remove=1 and the device immediately disappears, is that a bug or a feature? Thanks, Alex [1] GVT-g doesn't actually have an writable attributes, so we'd also minimally want to add a test to skip read-only attributes. > >>> + configure_ap_devices ap_adapters assign_adapter > >>> + configure_ap_devices ap_domains assign_domain > >>> + configure_ap_devices ap_control_domains assign_control_domain > >>> + # TODO: is assigning idempotent? Should we unwind on error? > >>> + ;; > >>> + *) > >>> + ;; > >>> + esac > >>> ;; > >>> > >>> add-mdev) > >>> diff --git a/mdevctl.sbin b/mdevctl.sbin > >>> index 276cf6ddc817..eb5ee0091879 100755 > >>> --- a/mdevctl.sbin > >>> +++ b/mdevctl.sbin > >>> @@ -33,6 +33,8 @@ usage() { > >>> echo "set-start : change mdev start policy, if no option specified," >&2 > >>> echo " system default policy is used" >&2 > >>> echo " options: [--auto] [--manual]" >&2 > >>> + echo "set-additional-config {fmt...}: supply additional configuration" >&2 > >>> + echo "show-additional-config-format : prints the format expected by the device" >&2 > >>> echo "list-all: list all possible mdev types supported in the system" >&2 > >>> echo "list-available: list all mdev types currently available" >&2 > >>> echo "list-mdevs: list currently configured mdevs" >&2 > >>> @@ -48,7 +50,7 @@ while (($# > 0)); do > >>> --manual) > >>> config[start]=manual > >>> ;; > >>> - start-mdev|stop-mdev|remove-mdev|set-start) > >>> + start-mdev|stop-mdev|remove-mdev|set-start|show-additional-config-format) > >>> [ $# -ne 2 ] && usage > >>> cmd=$1 > >>> uuid=$2 > >>> @@ -67,6 +69,14 @@ while (($# > 0)); do > >>> cmd=$1 > >>> break > >>> ;; > >>> + set-additional-config) > >>> + [ $# -le 2 ] && usage > >>> + cmd=$1 > >>> + uuid=$2 > >>> + shift 2 > >>> + addtl_config="$*" > >>> + break > >>> + ;; > >>> *) > >>> usage > >>> ;; > >>> @@ -114,6 +124,50 @@ case ${cmd} in > >>> fi > >>> ;; > >>> > >>> + set-additional-config) > >>> + file=$(find $persist_base -name $uuid -type f) > >>> + if [ -w "$file" ]; then > >>> + read_config "$file" > >>> + if [ ${config[start]} == "auto" ]; then > >>> + systemctl stop mdev@$uuid.service > >>> + fi > >>> + # FIXME: validate input! > >>> + for i in $addtl_config; do > >>> + key="`echo "$i" | cut -d '=' -f 1`" > >>> + value="`echo "$i" | cut -d '=' -f 2-`" > >>> + if grep -q ^$key $file; then > >>> + if [ -z "$value" ]; then > >>> + sed -i "s/^$key=.*//g" $file > >>> + else > >>> + sed -i "s/^$key=.*/$key=$value/g" $file > >>> + fi > >>> + else > >>> + echo "$i" >> "$file" > >>> + fi > >>> + done > >>> + if [ ${config[start]} == "auto" ]; then > >>> + systemctl start mdev@$uuid.service > >>> + fi > >>> + else > >>> + exit 1 > >>> + fi > >>> + ;; > >>> + > >>> + show-additional-config-format) > >>> + file=$(find $persist_base -name $uuid -type f) > >>> + read_config "$file" > >>> + case ${config[mdev_type]} in > >>> + vfio_ap-passthrough) > >>> + echo "ap_adapters=" > >>> + echo "ap_domains=" > >>> + echo "ap_control_domains=" > >>> + ;; > >>> + *) > >>> + echo "no additional configuration defined" > >>> + ;; > >>> + esac > >>> + ;; > >>> + > >>> list-mdevs) > >>> for mdev in $(find $mdev_base/ -maxdepth 1 -mindepth 1 -type l); do > >>> uuid=$(basename $mdev) > >> > > >