[libvirt] [PATCH RFC 1/1] allow to specify additional config data
Tony Krowiak
akrowiak at linux.ibm.com
Tue Jun 11 14:19:29 UTC 2019
On 6/7/19 4:03 PM, Alex Williamson wrote:
> On Fri, 7 Jun 2019 14:26:13 -0400
> Tony Krowiak <akrowiak at 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 at redhat.com> wrote:
>>>
>>>> On Thu, 6 Jun 2019 16:44:17 +0200
>>>> Cornelia Huck <cohuck at redhat.com> wrote:
>>>>
>>>>> Add a rough implementation for vfio-ap.
>>>>>
>>>>> Signed-off-by: Cornelia Huck <cohuck at 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)
>>>>
>>>
>>
>
More information about the libvir-list
mailing list