[RFC,1/1] allow to specify additional config data
diff mbox series

Message ID 20190606144417.1824-2-cohuck@redhat.com
State New
Headers show
Series
  • mdevctl: further config for vfio-ap
Related show

Commit Message

Cornelia Huck June 6, 2019, 2:44 p.m. UTC
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(-)

Comments

Alex Williamson June 6, 2019, 3:32 p.m. UTC | #1
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,

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)
Alex Williamson June 6, 2019, 4:15 p.m. UTC | #2
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)  
>
Tony Krowiak June 7, 2019, 6:26 p.m. UTC | #3
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)
>>
>
Alex Williamson June 7, 2019, 8:03 p.m. UTC | #4
On Fri, 7 Jun 2019 14:26:13 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> 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.

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 <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)  
> >>  
> >   
>
Tony Krowiak June 11, 2019, 2:19 p.m. UTC | #5
On 6/7/19 4:03 PM, Alex Williamson wrote:
> On Fri, 7 Jun 2019 14:26:13 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> 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.
> 
> 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

By vendor defined, I meant attributes that are not defined by the mdev
framework, such as the 'remove' attribute. As far as whether allowing
specification of remove-1, I'd have to play with that and see what all
of the ramifications are.

Tony K

> 
> [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.

Probably a good idea.

> 
>>>>> +                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)
>>>>   
>>>    
>>
>
Cornelia Huck June 13, 2019, 2:18 p.m. UTC | #6
On Tue, 11 Jun 2019 10:19:29 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 6/7/19 4:03 PM, Alex Williamson wrote:
> > On Fri, 7 Jun 2019 14:26:13 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >   
> >> 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  

One thing that this does is limiting us to things that can be expressed
with "if you encounter key=value, take value (possibly decomposed) and
write it to <device>/key". A problem with this generic approach is that
the code cannot decide itself whether value should be decomposed (and
if yes, with which delimiter), or not. We also cannot cover any
configuration that does not fit this pattern; so I think we need both
generic (for flexibility, and easy extensibility), and explicitly
defined options to cover more complex cases.

[As an aside, how should we deal with duplicate key= entries? Not
allowed, last one wins, or all are written to the sysfs attribute?]

> >>
> >> 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  
> 
> By vendor defined, I meant attributes that are not defined by the mdev
> framework, such as the 'remove' attribute.

And those defined by the base driver core like uevent, I guess.

> As far as whether allowing
> specification of remove-1, I'd have to play with that and see what all
> of the ramifications are.

It does feel a bit odd to me (why would you configure it if you
immediately want to remove it again?)

> 
> Tony K
> 
> > 
> > [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.  
> 
> Probably a good idea.

Agreed.
Halil Pasic June 13, 2019, 3:56 p.m. UTC | #7
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)
> +                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?

It is largely idempotent AFAIR. The pathological case is queues go away
between the two assigns, but that results in the worst case just
in an error code -- the previous assignment is still effective. Why are
you asking? When doing this next time we will start with a freshly
created mdev I guess.

Regarding unwind. Keeping a half configured mdev (errors happened) looks
like a bad idea to me. Currently we don't fail the start operation if
we can't configure a device. So I guess the in case of vfio_ap the
guest would just start with whatever we managed to get.

What about concurrent updates to the config?

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

This is a disruptive action for 'auto' at the moment. I'm not sure about
that, but if we want to have this disruptive, then we need to document
it as such.

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

If the mdev is not started stop has no effect. If there
is an mdev started, and in use by a VM destroying the
mdev is a disruptive operation.

I'm a bit concerned about this semantic. We have a case
where the change takes effect immediately in a disruptive
or not disruptive fashion, and then we have a case where
only the persistent configuration is changed. And then,
when the configuration are applied, it may only get partially
applied.

Tony is working on hotplug/unplug on vfio_ap_mdevs. I do
not see if that is also supposed to fit in here. Probably
not.

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

How about concurrency? I guess we could end up loosing distinct
updates without detecting it.

> +            done

Basically we append or change but don't remove. So it is a
cumulative thing I suppose.


I'm not sure 'set-additional-config' is a good idea. For vfio_ap
I would hope for a tool that is more intelligent, and can help
with avoiding and managing conflicts.

Regards,
Halil

[..]
Tony Krowiak June 14, 2019, 1:24 p.m. UTC | #8
On 6/13/19 10:18 AM, Cornelia Huck wrote:
> On Tue, 11 Jun 2019 10:19:29 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 6/7/19 4:03 PM, Alex Williamson wrote:
>>> On Fri, 7 Jun 2019 14:26:13 -0400
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>    
>>>> 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
> 
> One thing that this does is limiting us to things that can be expressed
> with "if you encounter key=value, take value (possibly decomposed) and
> write it to <device>/key". A problem with this generic approach is that
> the code cannot decide itself whether value should be decomposed (and
> if yes, with which delimiter), or not. We also cannot cover any
> configuration that does not fit this pattern; so I think we need both
> generic (for flexibility, and easy extensibility), and explicitly
> defined options to cover more complex cases.
> 
> [As an aside, how should we deal with duplicate key= entries? Not
> allowed, last one wins, or all are written to the sysfs attribute?]
> 
>>>>
>>>> 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
>>
>> By vendor defined, I meant attributes that are not defined by the mdev
>> framework, such as the 'remove' attribute.
> 
> And those defined by the base driver core like uevent, I guess.

Yes

> 
>> As far as whether allowing
>> specification of remove-1, I'd have to play with that and see what all
>> of the ramifications are.
> 
> It does feel a bit odd to me (why would you configure it if you
> immediately want to remove it again?)

This was in response to Alex's comment. My personal preference is to
exclude attributes that are not vendor created, at least to the
extent it is possible to determine such.

> 
>>
>> Tony K
>>
>>>
>>> [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.
>>
>> Probably a good idea.
> 
> Agreed.
>
Cornelia Huck June 18, 2019, 3:11 p.m. UTC | #9
On Thu, 6 Jun 2019 10:15:52 -0600
Alex Williamson <alex.williamson@redhat.com> 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

Hm, so I tried to write something like that, but there's an obvious
drawback for the vfio-ap use case: we want to specify a list of values
to be written to an attribute. We would have to model that as a list of
key=value pairs; but that would make it harder to remove a specific
value. (I currently overwrite a given key=value pair with a new value
or delete it.) We could specify something like ^key=value to cancel out
key=value.

Does it make sense to write *all* values specified for a specific key
to that attribute in sequence, or may this have surprising
consequences? Can we live with those possible surprises?

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

Patch
diff mbox series

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