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

[Alex Williamson <alex.williamson@redhat.com>]

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

> > +                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)  
>