* [PATCH RFC 0/1] mdevctl: further config for vfio-ap @ 2019-06-06 14:44 Cornelia Huck 2019-06-06 14:44 ` [PATCH RFC 1/1] allow to specify additional config data Cornelia Huck 2019-06-06 16:45 ` [PATCH RFC 0/1] mdevctl: further config for vfio-ap Matthew Rosato 0 siblings, 2 replies; 17+ messages in thread From: Cornelia Huck @ 2019-06-06 14:44 UTC (permalink / raw) To: Alex Williamson Cc: kvm, libvir-list, Matthew Rosato, Tony Krowiak, Halil Pasic, Cornelia Huck This patch adds a very rough implementation of additional config data for mdev devices. The idea is to make it possible to specify some type-specific key=value pairs in the config file for an mdev device. If a device is started automatically, the device is stopped and restarted after applying the config. The code has still some problems, like not doing a lot of error handling and being ugly in general; but most importantly, I can't really test it, as I don't have the needed hardware. Feedback welcome; would be good to know if the direction is sensible in general. Also available at https://github.com/cohuck/mdevctl conf-data Cornelia Huck (1): allow to specify additional config data mdevctl.libexec | 25 ++++++++++++++++++++++ mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 1 deletion(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RFC 1/1] allow to specify additional config data 2019-06-06 14:44 [PATCH RFC 0/1] mdevctl: further config for vfio-ap Cornelia Huck @ 2019-06-06 14:44 ` Cornelia Huck 2019-06-06 15:32 ` Alex Williamson 2019-06-13 15:56 ` Halil Pasic 2019-06-06 16:45 ` [PATCH RFC 0/1] mdevctl: further config for vfio-ap Matthew Rosato 1 sibling, 2 replies; 17+ messages in thread From: Cornelia Huck @ 2019-06-06 14:44 UTC (permalink / raw) To: Alex Williamson Cc: kvm, libvir-list, Matthew Rosato, Tony Krowiak, Halil Pasic, Cornelia Huck 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? + ;; + *) + ;; + 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) -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-06 14:44 ` [PATCH RFC 1/1] allow to specify additional config data Cornelia Huck @ 2019-06-06 15:32 ` Alex Williamson 2019-06-06 16:15 ` Alex Williamson 2019-06-13 15:56 ` Halil Pasic 1 sibling, 1 reply; 17+ messages in thread From: Alex Williamson @ 2019-06-06 15:32 UTC (permalink / raw) To: Cornelia Huck; +Cc: kvm, libvir-list, Matthew Rosato, Tony Krowiak, Halil Pasic 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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-06 15:32 ` Alex Williamson @ 2019-06-06 16:15 ` Alex Williamson 2019-06-07 18:26 ` Tony Krowiak 2019-06-18 15:11 ` Cornelia Huck 0 siblings, 2 replies; 17+ messages in thread From: Alex Williamson @ 2019-06-06 16:15 UTC (permalink / raw) To: Cornelia Huck; +Cc: kvm, libvir-list, Matthew Rosato, Tony Krowiak, Halil Pasic 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) > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-06 16:15 ` Alex Williamson @ 2019-06-07 18:26 ` Tony Krowiak 2019-06-07 20:03 ` Alex Williamson 2019-06-18 15:11 ` Cornelia Huck 1 sibling, 1 reply; 17+ messages in thread From: Tony Krowiak @ 2019-06-07 18:26 UTC (permalink / raw) To: Alex Williamson, Cornelia Huck Cc: kvm, libvir-list, Matthew Rosato, Halil Pasic 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) >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-07 18:26 ` Tony Krowiak @ 2019-06-07 20:03 ` Alex Williamson 2019-06-11 14:19 ` Tony Krowiak 0 siblings, 1 reply; 17+ messages in thread From: Alex Williamson @ 2019-06-07 20:03 UTC (permalink / raw) To: Tony Krowiak; +Cc: Cornelia Huck, kvm, libvir-list, Matthew Rosato, Halil Pasic 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) > >> > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-07 20:03 ` Alex Williamson @ 2019-06-11 14:19 ` Tony Krowiak 2019-06-13 14:18 ` Cornelia Huck 0 siblings, 1 reply; 17+ messages in thread From: Tony Krowiak @ 2019-06-11 14:19 UTC (permalink / raw) To: Alex Williamson Cc: Cornelia Huck, kvm, libvir-list, Matthew Rosato, Halil Pasic 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) >>>> >>> >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-11 14:19 ` Tony Krowiak @ 2019-06-13 14:18 ` Cornelia Huck 2019-06-14 13:24 ` Tony Krowiak 0 siblings, 1 reply; 17+ messages in thread From: Cornelia Huck @ 2019-06-13 14:18 UTC (permalink / raw) To: Tony Krowiak Cc: Alex Williamson, kvm, libvir-list, Matthew Rosato, Halil Pasic 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-13 14:18 ` Cornelia Huck @ 2019-06-14 13:24 ` Tony Krowiak 0 siblings, 0 replies; 17+ messages in thread From: Tony Krowiak @ 2019-06-14 13:24 UTC (permalink / raw) To: Cornelia Huck Cc: Alex Williamson, kvm, libvir-list, Matthew Rosato, Halil Pasic 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. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-06 16:15 ` Alex Williamson 2019-06-07 18:26 ` Tony Krowiak @ 2019-06-18 15:11 ` Cornelia Huck 1 sibling, 0 replies; 17+ messages in thread From: Cornelia Huck @ 2019-06-18 15:11 UTC (permalink / raw) To: Alex Williamson Cc: kvm, libvir-list, Matthew Rosato, Tony Krowiak, Halil Pasic 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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 1/1] allow to specify additional config data 2019-06-06 14:44 ` [PATCH RFC 1/1] allow to specify additional config data Cornelia Huck 2019-06-06 15:32 ` Alex Williamson @ 2019-06-13 15:56 ` Halil Pasic 1 sibling, 0 replies; 17+ messages in thread From: Halil Pasic @ 2019-06-13 15:56 UTC (permalink / raw) To: Cornelia Huck Cc: Alex Williamson, kvm, libvir-list, Matthew Rosato, Tony Krowiak 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 [..] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap 2019-06-06 14:44 [PATCH RFC 0/1] mdevctl: further config for vfio-ap Cornelia Huck 2019-06-06 14:44 ` [PATCH RFC 1/1] allow to specify additional config data Cornelia Huck @ 2019-06-06 16:45 ` Matthew Rosato 2019-06-07 14:56 ` Cornelia Huck 1 sibling, 1 reply; 17+ messages in thread From: Matthew Rosato @ 2019-06-06 16:45 UTC (permalink / raw) To: Cornelia Huck, Alex Williamson Cc: kvm, libvir-list, Tony Krowiak, Halil Pasic On 6/6/19 10:44 AM, Cornelia Huck wrote: > This patch adds a very rough implementation of additional config data > for mdev devices. The idea is to make it possible to specify some > type-specific key=value pairs in the config file for an mdev device. > If a device is started automatically, the device is stopped and restarted > after applying the config. > > The code has still some problems, like not doing a lot of error handling > and being ugly in general; but most importantly, I can't really test it, > as I don't have the needed hardware. Feedback welcome; would be good to > know if the direction is sensible in general. Hi Connie, This is very similar to what I was looking to do in zdev (config via key=value pairs), so I like your general approach. I pulled your code and took it for a spin on an LPAR with access to crypto cards: # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough # mdevctl set-additional-config <uuid> ap_adapters=0x4,0x5 # mdevctl set-additional-config <uuid> ap_domains=0x36 # mdevctl set-additional-config <uuid> ap_control_domains=0x37 Assuming all valid inputs, this successfully creates the appropriate mdev and what looks to be a valid mdevctl.d entry. A subsequent reboot successfully brings the same vfio_ap-passthrough device up again. Matt > > Also available at > > https://github.com/cohuck/mdevctl conf-data > > Cornelia Huck (1): > allow to specify additional config data > > mdevctl.libexec | 25 ++++++++++++++++++++++ > mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 80 insertions(+), 1 deletion(-) > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap 2019-06-06 16:45 ` [PATCH RFC 0/1] mdevctl: further config for vfio-ap Matthew Rosato @ 2019-06-07 14:56 ` Cornelia Huck 2019-06-07 18:30 ` Tony Krowiak 0 siblings, 1 reply; 17+ messages in thread From: Cornelia Huck @ 2019-06-07 14:56 UTC (permalink / raw) To: Matthew Rosato Cc: Alex Williamson, kvm, libvir-list, Tony Krowiak, Halil Pasic On Thu, 6 Jun 2019 12:45:29 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 6/6/19 10:44 AM, Cornelia Huck wrote: > > This patch adds a very rough implementation of additional config data > > for mdev devices. The idea is to make it possible to specify some > > type-specific key=value pairs in the config file for an mdev device. > > If a device is started automatically, the device is stopped and restarted > > after applying the config. > > > > The code has still some problems, like not doing a lot of error handling > > and being ugly in general; but most importantly, I can't really test it, > > as I don't have the needed hardware. Feedback welcome; would be good to > > know if the direction is sensible in general. > > Hi Connie, > > This is very similar to what I was looking to do in zdev (config via > key=value pairs), so I like your general approach. > > I pulled your code and took it for a spin on an LPAR with access to > crypto cards: > > # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough > # mdevctl set-additional-config <uuid> ap_adapters=0x4,0x5 > # mdevctl set-additional-config <uuid> ap_domains=0x36 > # mdevctl set-additional-config <uuid> ap_control_domains=0x37 > > Assuming all valid inputs, this successfully creates the appropriate > mdev and what looks to be a valid mdevctl.d entry. A subsequent reboot > successfully brings the same vfio_ap-passthrough device up again. Cool, thanks for checking! > > Matt > > > > > Also available at > > > > https://github.com/cohuck/mdevctl conf-data > > > > Cornelia Huck (1): > > allow to specify additional config data > > > > mdevctl.libexec | 25 ++++++++++++++++++++++ > > mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 80 insertions(+), 1 deletion(-) > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap 2019-06-07 14:56 ` Cornelia Huck @ 2019-06-07 18:30 ` Tony Krowiak 2019-06-13 13:54 ` Cornelia Huck 0 siblings, 1 reply; 17+ messages in thread From: Tony Krowiak @ 2019-06-07 18:30 UTC (permalink / raw) To: Cornelia Huck, Matthew Rosato Cc: Alex Williamson, kvm, libvir-list, Halil Pasic On 6/7/19 10:56 AM, Cornelia Huck wrote: > On Thu, 6 Jun 2019 12:45:29 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> On 6/6/19 10:44 AM, Cornelia Huck wrote: >>> This patch adds a very rough implementation of additional config data >>> for mdev devices. The idea is to make it possible to specify some >>> type-specific key=value pairs in the config file for an mdev device. >>> If a device is started automatically, the device is stopped and restarted >>> after applying the config. >>> >>> The code has still some problems, like not doing a lot of error handling >>> and being ugly in general; but most importantly, I can't really test it, >>> as I don't have the needed hardware. Feedback welcome; would be good to >>> know if the direction is sensible in general. >> >> Hi Connie, >> >> This is very similar to what I was looking to do in zdev (config via >> key=value pairs), so I like your general approach. >> >> I pulled your code and took it for a spin on an LPAR with access to >> crypto cards: >> >> # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough >> # mdevctl set-additional-config <uuid> ap_adapters=0x4,0x5 >> # mdevctl set-additional-config <uuid> ap_domains=0x36 >> # mdevctl set-additional-config <uuid> ap_control_domains=0x37 >> >> Assuming all valid inputs, this successfully creates the appropriate >> mdev and what looks to be a valid mdevctl.d entry. A subsequent reboot >> successfully brings the same vfio_ap-passthrough device up again. > > Cool, thanks for checking! I also confirmed this. I realize this is a very early proof of concept, if you will, but error handling could be an interesting endeavor in the case of vfio_ap given the layers of configuration involved; for example: * The necessity for the vfio_ap module to be installed * The necessity that the /sys/bus/ap/apmask and /sys/bus/ap/aqmask must be appropriately configured > >> >> Matt >> >>> >>> Also available at >>> >>> https://github.com/cohuck/mdevctl conf-data >>> >>> Cornelia Huck (1): >>> allow to specify additional config data >>> >>> mdevctl.libexec | 25 ++++++++++++++++++++++ >>> mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 80 insertions(+), 1 deletion(-) >>> >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap 2019-06-07 18:30 ` Tony Krowiak @ 2019-06-13 13:54 ` Cornelia Huck 2019-06-13 14:25 ` Matthew Rosato 2019-06-14 13:36 ` Tony Krowiak 0 siblings, 2 replies; 17+ messages in thread From: Cornelia Huck @ 2019-06-13 13:54 UTC (permalink / raw) To: Tony Krowiak Cc: Matthew Rosato, Alex Williamson, kvm, libvir-list, Halil Pasic On Fri, 7 Jun 2019 14:30:48 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 6/7/19 10:56 AM, Cornelia Huck wrote: > > On Thu, 6 Jun 2019 12:45:29 -0400 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > >> On 6/6/19 10:44 AM, Cornelia Huck wrote: > >>> This patch adds a very rough implementation of additional config data > >>> for mdev devices. The idea is to make it possible to specify some > >>> type-specific key=value pairs in the config file for an mdev device. > >>> If a device is started automatically, the device is stopped and restarted > >>> after applying the config. > >>> > >>> The code has still some problems, like not doing a lot of error handling > >>> and being ugly in general; but most importantly, I can't really test it, > >>> as I don't have the needed hardware. Feedback welcome; would be good to > >>> know if the direction is sensible in general. > >> > >> Hi Connie, > >> > >> This is very similar to what I was looking to do in zdev (config via > >> key=value pairs), so I like your general approach. > >> > >> I pulled your code and took it for a spin on an LPAR with access to > >> crypto cards: > >> > >> # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough > >> # mdevctl set-additional-config <uuid> ap_adapters=0x4,0x5 > >> # mdevctl set-additional-config <uuid> ap_domains=0x36 > >> # mdevctl set-additional-config <uuid> ap_control_domains=0x37 > >> > >> Assuming all valid inputs, this successfully creates the appropriate > >> mdev and what looks to be a valid mdevctl.d entry. A subsequent reboot > >> successfully brings the same vfio_ap-passthrough device up again. > > > > Cool, thanks for checking! > > I also confirmed this. I realize this is a very early proof of concept, > if you will, but error handling could be an interesting endeavor in > the case of vfio_ap given the layers of configuration involved; I agree; that's why I mostly ignored it for now :) > for > example: > > * The necessity for the vfio_ap module to be installed > * The necessity that the /sys/bus/ap/apmask and /sys/bus/ap/aqmask must > be appropriately configured What do you think about outsourcing that configuration to some s390-specific tool (probably something in s390-tools)? While we can (and should) rely on driverctl for vfio-ccw, this does not look like something that can be easily served by some generic tooling. > > > > >> > >> Matt > >> > >>> > >>> Also available at > >>> > >>> https://github.com/cohuck/mdevctl conf-data > >>> > >>> Cornelia Huck (1): > >>> allow to specify additional config data > >>> > >>> mdevctl.libexec | 25 ++++++++++++++++++++++ > >>> mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- > >>> 2 files changed, 80 insertions(+), 1 deletion(-) > >>> > >> > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap 2019-06-13 13:54 ` Cornelia Huck @ 2019-06-13 14:25 ` Matthew Rosato 2019-06-14 13:36 ` Tony Krowiak 1 sibling, 0 replies; 17+ messages in thread From: Matthew Rosato @ 2019-06-13 14:25 UTC (permalink / raw) To: Cornelia Huck, Tony Krowiak Cc: Alex Williamson, kvm, libvir-list, Halil Pasic On 6/13/19 9:54 AM, Cornelia Huck wrote: > On Fri, 7 Jun 2019 14:30:48 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 6/7/19 10:56 AM, Cornelia Huck wrote: >>> On Thu, 6 Jun 2019 12:45:29 -0400 >>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >>> >>>> On 6/6/19 10:44 AM, Cornelia Huck wrote: >>>>> This patch adds a very rough implementation of additional config data >>>>> for mdev devices. The idea is to make it possible to specify some >>>>> type-specific key=value pairs in the config file for an mdev device. >>>>> If a device is started automatically, the device is stopped and restarted >>>>> after applying the config. >>>>> >>>>> The code has still some problems, like not doing a lot of error handling >>>>> and being ugly in general; but most importantly, I can't really test it, >>>>> as I don't have the needed hardware. Feedback welcome; would be good to >>>>> know if the direction is sensible in general. >>>> >>>> Hi Connie, >>>> >>>> This is very similar to what I was looking to do in zdev (config via >>>> key=value pairs), so I like your general approach. >>>> >>>> I pulled your code and took it for a spin on an LPAR with access to >>>> crypto cards: >>>> >>>> # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough >>>> # mdevctl set-additional-config <uuid> ap_adapters=0x4,0x5 >>>> # mdevctl set-additional-config <uuid> ap_domains=0x36 >>>> # mdevctl set-additional-config <uuid> ap_control_domains=0x37 >>>> >>>> Assuming all valid inputs, this successfully creates the appropriate >>>> mdev and what looks to be a valid mdevctl.d entry. A subsequent reboot >>>> successfully brings the same vfio_ap-passthrough device up again. >>> >>> Cool, thanks for checking! >> >> I also confirmed this. I realize this is a very early proof of concept, >> if you will, but error handling could be an interesting endeavor in >> the case of vfio_ap given the layers of configuration involved; > > I agree; that's why I mostly ignored it for now :) > >> for >> example: >> >> * The necessity for the vfio_ap module to be installed >> * The necessity that the /sys/bus/ap/apmask and /sys/bus/ap/aqmask must >> be appropriately configured > > What do you think about outsourcing that configuration to some > s390-specific tool (probably something in s390-tools)? While we can > (and should) rely on driverctl for vfio-ccw, this does not look like > something that can be easily served by some generic tooling. > +1 mdevctl should stick to manipulation of mdevs. I think any config layer above that isn't directly modifying mdev parameters, like in this case the kernel ap/aq masks, should be separate (and for vfio_ap I agree s390-tools is the right place for this -- it's on my todo list) >> >>> >>>> >>>> Matt >>>> >>>>> >>>>> Also available at >>>>> >>>>> https://github.com/cohuck/mdevctl conf-data >>>>> >>>>> Cornelia Huck (1): >>>>> allow to specify additional config data >>>>> >>>>> mdevctl.libexec | 25 ++++++++++++++++++++++ >>>>> mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- >>>>> 2 files changed, 80 insertions(+), 1 deletion(-) >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap 2019-06-13 13:54 ` Cornelia Huck 2019-06-13 14:25 ` Matthew Rosato @ 2019-06-14 13:36 ` Tony Krowiak 1 sibling, 0 replies; 17+ messages in thread From: Tony Krowiak @ 2019-06-14 13:36 UTC (permalink / raw) To: Cornelia Huck Cc: Matthew Rosato, Alex Williamson, kvm, libvir-list, Halil Pasic On 6/13/19 9:54 AM, Cornelia Huck wrote: > On Fri, 7 Jun 2019 14:30:48 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 6/7/19 10:56 AM, Cornelia Huck wrote: >>> On Thu, 6 Jun 2019 12:45:29 -0400 >>> Matthew Rosato <mjrosato@linux.ibm.com> wrote: >>> >>>> On 6/6/19 10:44 AM, Cornelia Huck wrote: >>>>> This patch adds a very rough implementation of additional config data >>>>> for mdev devices. The idea is to make it possible to specify some >>>>> type-specific key=value pairs in the config file for an mdev device. >>>>> If a device is started automatically, the device is stopped and restarted >>>>> after applying the config. >>>>> >>>>> The code has still some problems, like not doing a lot of error handling >>>>> and being ugly in general; but most importantly, I can't really test it, >>>>> as I don't have the needed hardware. Feedback welcome; would be good to >>>>> know if the direction is sensible in general. >>>> >>>> Hi Connie, >>>> >>>> This is very similar to what I was looking to do in zdev (config via >>>> key=value pairs), so I like your general approach. >>>> >>>> I pulled your code and took it for a spin on an LPAR with access to >>>> crypto cards: >>>> >>>> # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough >>>> # mdevctl set-additional-config <uuid> ap_adapters=0x4,0x5 >>>> # mdevctl set-additional-config <uuid> ap_domains=0x36 >>>> # mdevctl set-additional-config <uuid> ap_control_domains=0x37 >>>> >>>> Assuming all valid inputs, this successfully creates the appropriate >>>> mdev and what looks to be a valid mdevctl.d entry. A subsequent reboot >>>> successfully brings the same vfio_ap-passthrough device up again. >>> >>> Cool, thanks for checking! >> >> I also confirmed this. I realize this is a very early proof of concept, >> if you will, but error handling could be an interesting endeavor in >> the case of vfio_ap given the layers of configuration involved; > > I agree; that's why I mostly ignored it for now :) > >> for >> example: >> >> * The necessity for the vfio_ap module to be installed >> * The necessity that the /sys/bus/ap/apmask and /sys/bus/ap/aqmask must >> be appropriately configured > > What do you think about outsourcing that configuration to some > s390-specific tool (probably something in s390-tools)? While we can > (and should) rely on driverctl for vfio-ccw, this does not look like > something that can be easily served by some generic tooling. I wasn't suggesting configuration of the AP bus masks etc. should be part of this tool, I was merely pointing out that errors encountered when creating and configuring an mdev can be related to items higher in the stack, thus making it difficult to isolate the real problem. There are plans in place for an s390 tool that I assume will sit on top of mdevctl should it move forward. > >> >>> >>>> >>>> Matt >>>> >>>>> >>>>> Also available at >>>>> >>>>> https://github.com/cohuck/mdevctl conf-data >>>>> >>>>> Cornelia Huck (1): >>>>> allow to specify additional config data >>>>> >>>>> mdevctl.libexec | 25 ++++++++++++++++++++++ >>>>> mdevctl.sbin | 56 ++++++++++++++++++++++++++++++++++++++++++++++++- >>>>> 2 files changed, 80 insertions(+), 1 deletion(-) >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-06-18 15:12 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-06 14:44 [PATCH RFC 0/1] mdevctl: further config for vfio-ap Cornelia Huck 2019-06-06 14:44 ` [PATCH RFC 1/1] allow to specify additional config data Cornelia Huck 2019-06-06 15:32 ` Alex Williamson 2019-06-06 16:15 ` Alex Williamson 2019-06-07 18:26 ` Tony Krowiak 2019-06-07 20:03 ` Alex Williamson 2019-06-11 14:19 ` Tony Krowiak 2019-06-13 14:18 ` Cornelia Huck 2019-06-14 13:24 ` Tony Krowiak 2019-06-18 15:11 ` Cornelia Huck 2019-06-13 15:56 ` Halil Pasic 2019-06-06 16:45 ` [PATCH RFC 0/1] mdevctl: further config for vfio-ap Matthew Rosato 2019-06-07 14:56 ` Cornelia Huck 2019-06-07 18:30 ` Tony Krowiak 2019-06-13 13:54 ` Cornelia Huck 2019-06-13 14:25 ` Matthew Rosato 2019-06-14 13:36 ` Tony Krowiak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).