Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility

On Thu, 27 Jun 2019 11:00:31 -0400 Matthew Rosato <mjrosato(a)linux.ibm.com&gt; wrote: > On 6/27/19 8:26 AM, Cornelia Huck wrote: >> On Wed, 26 Jun 2019 19:53:50 -0600 >> Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: >> >>> On Wed, 26 Jun 2019 08:37:20 -0600 >>> Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: >>> >>>> On Wed, 26 Jun 2019 11:58:06 +0200 >>>> Cornelia Huck <cohuck(a)redhat.com&gt; wrote: >>>> >>>>> On Tue, 25 Jun 2019 16:52:51 -0600 >>>>> Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> Based on the discussions we've had, I've rewritten the bulk of >>>>>> mdevctl. I think it largely does everything we want now, modulo >>>>>> devices that will need some sort of 1:N values per key for >>>>>> configuration in the config file versus the 1:1 key:value setup we >>>>>> currently have (so don't consider the format final just yet). >>>>> >>>>> We might want to factor out that config format handling while we're >>>>> trying to finalize it. >>>>> >>>>> cc:ing Matt for his awareness. I'm currently not quite sure how to >>>>> handle those vfio-ap "write several values to an attribute one at a >>>>> time" requirements. Maybe 1:N key:value is the way to go; maybe we >>>>> need/want JSON or something like that. >>>> >>>> Maybe we should just do JSON for future flexibility. I assume there >>>> are lots of helpers that should make it easy even from a bash script. >>>> I'll look at that next. >>> >>> Done. Throw away any old mdev config files, we use JSON now. >> >> The code changes look quite straightforward, thanks. >> >>> The per >>> mdev config now looks like this: >>> >>> { >>> "mdev_type": "i915-GVTg_V4_8", >>> "start": "auto" >>> } >>> >>> My expectation, and what I've already pre-enabled support in set_key >>> and get_key functions, is that we'd use arrays for values, so we might >>> have: >>> >>> "new_key": ["value1", "value2"] >>> >>> set_key will automatically convert a comma separated list of values >>> into such an array, so I'm thinking this would be specified by the user >>> as: >>> >>> # mdevctl modify -u UUID --key=new_key --value=value1,value2 >> >> Looks sensible. >> >> For vfio-ap, we'd probably end up with something like the following: >> >> { >> "mdev_type": "vfio_ap-passthrough", >> "start": "auto", >> "assign_adapter": ["5", "6"], >> "assign_domain": ["4", "0xab"] >> } >> >> (following the Guest1 example in the kernel documentation) >> >> <As an aside, what should happen if e.g "assign_adapter" is set to >> ["6", "7"]? Remove 5, add 7? Remove all values, then set the new ones? > > IMO remove 5, add 7 would make the most sense. I'm not sure that doing > an unassign of all adapters (effectively removing all APQNs) followed by > an assign of the new ones would work nicely with Tony's vfio-ap dynamic > configuration patches. Are we conflating operating on the config file versus operating on the device? I was thinking that setting a new key value replaces the existing key, because anything else adds unnecessary complication to the code and command line. So in the above example, if the user specified: mdevctl modify -u UUID --key=assign_adapter --value=6,7 The new value is simply ["6", "7"]. This would take effect the next time the device is started. We haven't yet considered how to change running devices, but I think the semantics we have since the respin of mdevctl separate saved config vs running devices in order to generalize the support of transient devices.