Re: [PATCH RFC 1/1] allow to specify additional config data

From: Tony Krowiak <akrowiak@linux.ibm.com>
To: Alex Williamson <alex.williamson@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>
Cc: kvm@vger.kernel.org, libvir-list@redhat.com,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [PATCH RFC 1/1] allow to specify additional config data
Date: Fri, 7 Jun 2019 14:26:13 -0400	[thread overview]
Message-ID: <ed75a4de-da0b-f6cf-6164-44cebc82c3a5@linux.ibm.com> (raw)
In-Reply-To: <20190606101552.6fc62bef@x1.home>

On 6/6/19 12:15 PM, Alex Williamson wrote:
> On Thu, 6 Jun 2019 09:32:24 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
>> On Thu,  6 Jun 2019 16:44:17 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> Add a rough implementation for vfio-ap.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>   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 <config entry> <matrix attribute>
>>> +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.

> 
>>> +                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 <mdev UUID>: 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 <mdev UUID> {fmt...}: supply additional configuration" >&2
>>> +    echo "show-additional-config-format <mdev UUiD>:  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=<comma-separated list of adapters>"
>>> +                echo "ap_domains=<comma-separated list of domains>"
>>> +                echo "ap_control_domains=<comma-separated list of 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)
>>
> 