Hi, Currently mediated device management, much like SR-IOV VF management, is largely left as an exercise for the user. This is an attempt to provide something and see where it goes. I doubt we'll solve everyone's needs on the first pass, but maybe we'll solve enough and provide helpers for the rest. Without further ado, I'll point to what I have so far: https://github.com/awilliam/mdevctl This is inspired by driverctl, which is also a bash utility. mdevctl uses udev and systemd to record and recreate mdev devices for persistence and provides a command line utility for querying, listing, starting, stopping, adding, and removing mdev devices. Currently, for better or worse, it considers anything created to be persistent. I can imagine a global configuration option that might disable this and perhaps an autostart flag per mdev device, such that mdevctl might simply "know" about some mdevs but not attempt to create them automatically. Clearly command line usage help, man pages, and packaging are lacking as well, release early, release often, plus this is a discussion starter to see if perhaps this is sufficient to meet some needs. Originally I thought about making a utility to manage both mdev and SR-IOV VFs all in one, but it seemed more natural to start here (besides, I couldn't think of a good name for the combined utility). If this seems useful, maybe I'll start on a vfctl for SR-IOV and we'll see whether they have enough synergy to become one. It would be really useful if s390 folks could help me understand whether it's possible to glean all the information necessary to recreate a ccw or ap mdev device from sysfs. I expect the file where we currently only store the mdev_type to evolve into something that includes more information to facilitate more complicated devices. For now I make no claims to maintaining compatibility of recorded mdev devices, it will absolutely change, but I didn't want to get bogged down in making sure I don't accidentally source a root kit hidden in an mdev config file.

I'm also curious how or if libvirt or openstack might use this. If nothing else, it makes libvirt hook scripts easier to write, especially if we add an option not to autostart mdevs, or if users don't mind persistent mdevs, maybe there's nothing more to do. BTW, feel free to clean up by bash, I'm a brute force and ignorance shell coder ;)

Thanks, Alex

On Thu, 23 May 2019 17:20:01 -0600 Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > Hi, >

> > It would be really useful if s390 folks could help me understand > whether it's possible to glean all the information necessary to > recreate a ccw or ap mdev device from sysfs. I expect the file where > we currently only store the mdev_type to evolve into something that > includes more information to facilitate more complicated devices. For > now I make no claims to maintaining compatibility of recorded mdev > devices, it will absolutely change, but I didn't want to get bogged > down in making sure I don't accidentally source a root kit hidden in an > mdev config file. I played a bit with it on my LPAR, and it is at least not obviously broken with vfio-ccw :) I don't have any ap devices to play with, though.

> > I'm also curious how or if libvirt or openstack might use this. If > nothing else, it makes libvirt hook scripts easier to write, especially > if we add an option not to autostart mdevs, or if users don't mind > persistent mdevs, maybe there's nothing more to do. >

On Fri, 7 Jun 2019 18:06:30 +0200 Halil Pasic <pasic(a)linux.ibm.com&gt; wrote: > On Fri, 24 May 2019 12:11:06 +0200 > Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > > > On Thu, 23 May 2019 17:20:01 -0600 > > Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > > > Hi, > > > > > [..] > > > > > > > It would be really useful if s390 folks could help me understand > > > whether it's possible to glean all the information necessary to > > > recreate a ccw or ap mdev device from sysfs. I expect the file where > > > we currently only store the mdev_type to evolve into something that > > > includes more information to facilitate more complicated devices. For > > > now I make no claims to maintaining compatibility of recorded mdev > > > devices, it will absolutely change, but I didn't want to get bogged > > > down in making sure I don't accidentally source a root kit hidden in an > > > mdev config file. > > > > I played a bit with it on my LPAR, and it is at least not obviously > > broken with vfio-ccw :) I don't have any ap devices to play with, > > though. > > > > Sorry for being late... > > I guess for vfio-ccw one needs to make sure that the ccw device is bound > to the vfio-ccw driver first, and only after that can one use > create-mdev to create the mdev on top of the subchannel. > > So to make this work persistently (survive a reboot) one would need to > take care of the subchannel getting bound to the right vfio_ccw driver > before mdevctl is called. Right? > > BTW how does this concurrence situation between the drivers io_subchannel > and vfio_ccw work? Especially if both are build in? If you have two drivers that match to the same device type, you'll always have the issue that the driver that is first matched with the device will bind to it and you have to do the unbind/rebind dance to get it bound to the correct device driver. (I guess that this was the basic motivation behind the ap bus default driver infrastructure, right?) I think that in our case the io_subchannel driver will be called first (alphabetical order and the fact that vfio-ccw will often be a module). I'm not sure if it is within the scope of mdevctl to ensure that the device is bound to the correct driver, or if it rather should work with devices already bound to the correct driver only. Maybe a separate udev-rules generator?

There's also the question where that automatic configuration should stop. Should cio_ignore handling be part of it as well? [That's a non-generic interface, of course. Tooling within s390-tools, maybe?] > > > > > > I'm also curious how or if libvirt or openstack might use this. > > > If nothing else, it makes libvirt hook scripts easier to write, > > > especially if we add an option not to autostart mdevs, or if > > > users don't mind persistent mdevs, maybe there's nothing more to > > > do. > > +1 > > @Alex: I'm curious what is the big management picture for non-auto > looks like. > > Regards, > Halil > > [..] >

On Tue, 11 Jun 2019 14:28:22 -0600 Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > On Tue, 11 Jun 2019 21:45:08 +0200 > Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > > > On Fri, 7 Jun 2019 18:06:30 +0200 > > Halil Pasic <pasic(a)linux.ibm.com&gt; wrote: > > > I guess for vfio-ccw one needs to make sure that the ccw device is bound > > > to the vfio-ccw driver first, and only after that can one use > > > create-mdev to create the mdev on top of the subchannel. > > > > > > So to make this work persistently (survive a reboot) one would need to > > > take care of the subchannel getting bound to the right vfio_ccw driver > > > before mdevctl is called. Right? > > > > > > BTW how does this concurrence situation between the drivers io_subchannel > > > and vfio_ccw work? Especially if both are build in? > > > > If you have two drivers that match to the same device type, you'll > > always have the issue that the driver that is first matched with the > > device will bind to it and you have to do the unbind/rebind dance to > > get it bound to the correct device driver. (I guess that this was the > > basic motivation behind the ap bus default driver infrastructure, > > right?) I think that in our case the io_subchannel driver will be > > called first (alphabetical order and the fact that vfio-ccw will often > > be a module). I'm not sure if it is within the scope of mdevctl to > > ensure that the device is bound to the correct driver, or if it rather > > should work with devices already bound to the correct driver only. > > Maybe a separate udev-rules generator? > > Getting a device bound to a specific driver is exactly the domain of > driverctl. Implement the sysfs interfaces driverctl uses and see if it > works. Driverctl defaults to PCI and knows some extra things about > PCI, but appears to be written to be generally bus agnostic. Thanks, > > Alex

Ok, looked at driverctl. Extending this one for non-PCI seems like a reasonable path. However, we would also need to extend any non-PCI device type we want to support with a driver_override attribute like you did for PCI in 782a985d7af26db39e86070d28f987cad2 -- so this is only for newer kernels. Adding that attribute for subchannels looks feasible at a glance, but I have not tried to actually do it :) Halil, do you think that would make sense?

[This might also help with the lcs vs. ctc confusion on a certain 3088 cu model if this is added for ccw devices as well; but I'm not sure if these are still out in the wild at all. Probably not worth the effort for that.]

On Fri, 7 Jun 2019 18:06:30 +0200 Halil Pasic <pasic(a)linux.ibm.com&gt; wrote: > On Fri, 24 May 2019 12:11:06 +0200 > Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > > > On Thu, 23 May 2019 17:20:01 -0600 > > Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > > > Hi, > > > > > [..] > > > > > > > It would be really useful if s390 folks could help me understand > > > whether it's possible to glean all the information necessary to > > > recreate a ccw or ap mdev device from sysfs. I expect the file where > > > we currently only store the mdev_type to evolve into something that > > > includes more information to facilitate more complicated devices. For > > > now I make no claims to maintaining compatibility of recorded mdev > > > devices, it will absolutely change, but I didn't want to get bogged > > > down in making sure I don't accidentally source a root kit hidden in an > > > mdev config file. > > > > I played a bit with it on my LPAR, and it is at least not obviously > > broken with vfio-ccw :) I don't have any ap devices to play with, > > though. > > > > Sorry for being late... > > I guess for vfio-ccw one needs to make sure that the ccw device is bound > to the vfio-ccw driver first, and only after that can one use > create-mdev to create the mdev on top of the subchannel. > > So to make this work persistently (survive a reboot) one would need to > take care of the subchannel getting bound to the right vfio_ccw driver > before mdevctl is called. Right? > > BTW how does this concurrence situation between the drivers io_subchannel > and vfio_ccw work? Especially if both are build in? If you have two drivers that match to the same device type, you'll always have the issue that the driver that is first matched with the device will bind to it and you have to do the unbind/rebind dance to get it bound to the correct device driver. (I guess that this was the basic motivation behind the ap bus default driver infrastructure, right?)

I think that in our case the io_subchannel driver will be called first (alphabetical order and the fact that vfio-ccw will often be a module).

I'm not sure if it is within the scope of mdevctl to ensure that the device is bound to the correct driver, or if it rather should work with devices already bound to the correct driver only. Maybe a separate udev-rules generator?

There's also the question where that automatic configuration should stop. Should cio_ignore handling be part of it as well? [That's a non-generic interface, of course. Tooling within s390-tools, maybe?]

> On 24 May 2019, at 01:20, Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > Hi, > > Currently mediated device management, much like SR-IOV VF management, > is largely left as an exercise for the user. This is an attempt to > provide something and see where it goes. I doubt we'll solve > everyone's needs on the first pass, but maybe we'll solve enough and > provide helpers for the rest. Without further ado, I'll point to what > I have so far: > > https://github.com/awilliam/mdevctl While it’s still early, what about : mdevctl create-mdev <parent-device> <mdev-type> [<mdev-uuid>] where if the mdev-uuid is missing, you just run uuidgen within the script? I sent a small PR in case you think it makes sense.

> On 13 Jun 2019, at 18:35, Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > On Thu, 13 Jun 2019 18:17:53 +0200 > Christophe de Dinechin <cdupontd(a)redhat.com&gt; wrote: > >>> On 24 May 2019, at 01:20, Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: >>> >>> Hi, >>> >>> Currently mediated device management, much like SR-IOV VF management, >>> is largely left as an exercise for the user. This is an attempt to >>> provide something and see where it goes. I doubt we'll solve >>> everyone's needs on the first pass, but maybe we'll solve enough and >>> provide helpers for the rest. Without further ado, I'll point to what >>> I have so far: >>> >>> https://github.com/awilliam/mdevctl >> >> While it’s still early, what about : >> >> mdevctl create-mdev <parent-device> <mdev-type> [<mdev-uuid>] >> >> where if the mdev-uuid is missing, you just run uuidgen within the script? >> >> I sent a small PR in case you think it makes sense. > > It sounds racy. If the user doesn't provide the UUID then they need to > guess that an mdev device with the same parent and type is theirs. That is true irrespective of the usage, isn’t it? In other words, when you invoke `mdevctl create-mdev`, you assert “I own that specific parent/type”. At least, that’s how I read the way the script behaves today. Whether you invoke uuidgen inside or outside the script does not change that assertion (at least with today’s code).

> How do you resolve two instances of this happening in parallel and both > coming to the same conclusion which is their device. If a user wants > this sort of headache they can call mdevctl with `uuidgen` but I don't > think we should encourage it further. I agree there is a race, but if anything, having a usage where you don’t pass the UUID on the command line is a step in the right direction. It leaves the door open for the create-mdev script to do smarter things, like deferring the allocation of the mdevs to an entity that has slightly more knowledge of the global system state than uuidgen.

In other words, in my mind, `mdevctl create-mdev parent type` does not imply “this will use uuidgen” but rather, if anything, implies “this will do the right thing to prevent the race in the future, even if that’s more complex than just calling uuidgen”.

However, I believe that this means we should reorder the args further. I would suggest something like: mdevctl create-mdev <mdev-type> [<parent-device> [<mdev-uuid>]] where

1 arg means you let mdevctl choose the parent device for you (future) (e.g. I want a VGPU of this type, I don’t really care where it comes from) 2 args mean you want that specific type/parent combination 3 args mean you assert you own that device That also implies that mdevctl create-mdev should output what it allocated so that some higher-level software can tell “OK, that’s the instance I got”.

> BTW, I've moved the project to https://github.com/mdevctl/mdevctl, the > latest commit in the tree above makes that change, I've also updated > the description on my repo to point to the new location. Thanks, Done. > > Alex > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list

> On 14 Jun 2019, at 16:23, Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > On Fri, 14 Jun 2019 11:54:42 +0200 > Christophe de Dinechin <cdupontd(a)redhat.com&gt; wrote: > >> That is true irrespective of the usage, isn’t it? In other words, when you >> invoke `mdevctl create-mdev`, you assert “I own that specific parent/type”. >> At least, that’s how I read the way the script behaves today. Whether you >> invoke uuidgen inside or outside the script does not change that assertion >> (at least with today’s code). > > What gives you this impression? That your code does nothing to avoid any race today? Maybe I was confused with the existing `uuidgen` example in you README, but it looks to me like the usage model involves much more than just create-mdev, and that any race that might exist is not in create-mdev itself (or in uuidgen for that matter).

> Where is the parent/type ownership implied? I did not imply it, but I read some concern about ownership on your part in "they need to guess that an mdev device with the same parent and type is *theirs*.” (emphasis mine) I personally see no change on the “need to guess” implied by the fact that you run uuidgen inside the script, so that’s why I tried to guess what you meant.

> The intended semantics are > "try to create this type of device under this parent”. Agreed. Which is why I don’t see why trying to create with some new UUID introduces any race (as long as the script prints out that UUID, which I admit my patch entirely failed to to)

>>> How do you resolve two instances of this happening in parallel and both >>> coming to the same conclusion which is their device. If a user wants >>> this sort of headache they can call mdevctl with `uuidgen` but I don't >>> think we should encourage it further. >> >> I agree there is a race, but if anything, having a usage where you don’t >> pass the UUID on the command line is a step in the right direction. >> It leaves the door open for the create-mdev script to do smarter things, >> like deferring the allocation of the mdevs to an entity that has slightly >> more knowledge of the global system state than uuidgen. > > A user might (likely) require a specific uuid to match their VM > configuration. I can only think of very niche use cases where a user > doesn't care what uuid they get. They do care. But I typically copy-paste my UUIDs, and then 1. copy-pasting at the end is always faster than between the command and other arguments (3-args case). 2. copy-pasting the output of the previous command is faster than having one extra step where I need to copy the same thing twice (2-args case). So to me, if the script is intended to be used by humans, my proposal makes it slightly more comfortable to use. Nothing more.

>> In other words, in my mind, `mdevctl create-mdev parent type` does not >> imply “this will use uuidgen” but rather, if anything, implies “this will do the >> right thing to prevent the race in the future, even if that’s more complex >> than just calling uuidgen”. > > What race are you trying to prevent, uuid collision? Of course not ;-) I only added the part of the discussion below trying to figure out what race you were seeing that was present only with my proposed changes. I (apparently incorrectly) supposed that you had some kind of mdev management within the script in mind. Obviously I misinterpreted. That will teach me to guess when I don’t understand instead of just ask…

>> However, I believe that this means we should reorder the args further. >> I would suggest something like: >> >> mdevctl create-mdev <mdev-type> [<parent-device> [<mdev-uuid>]] >> >> where > > Absolutely not, now you've required mdevctl to implement policy in mdev > placement. No, I’m not requiring it. I’m leaving the door open if one day, say, we decide to have libvirt tell us about the placement. That usage needs not go in right away, I marked it as “(future)”. Basically, all I’m saying is that since it’s early, we can reorder the arguments so that the one you are most likely to change when you reuse the command are the one that are last on the command-line, so that it makes editing or copy-pasting easier. There isn’t more to it, and that’s why I still do not see any new race introduced by that change.

> mdevctl follows the unix standard, do one thing and do it > well. If someone wants to layer placement policy on top of mdevctl, > great, but let's not impose that within mdevctl. I’m not imposing anything (I believe). I was only trying to guess where you saw things going that would imply there was a race with my proposal that was not there without :-) > >> 1 arg means you let mdevctl choose the parent device for you (future) >> (e.g. I want a VGPU of this type, I don’t really care where it comes from) >> 2 args mean you want that specific type/parent combination >> 3 args mean you assert you own that device >> >> That also implies that mdevctl create-mdev should output what it allocated >> so that some higher-level software can tell “OK, that’s the instance I got”. > > I don't think we're aligned on what mdevctl is attempting to provide. > Maybe you're describing a layer you'd like to see above mdevctl? > Thanks, No, again, I’m just trying to understand where you see a race. Maybe instead of guessing, I should just ask: where is the race in the two-args variant (assuming it prints the UUID it used) that does not exist with the three-args variant?

On Fri, 14 Jun 2019 17:06:15 +0200 Christophe de Dinechin <cdupontd(a)redhat.com&gt; wrote: > > On 14 Jun 2019, at 16:23, Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > > On Fri, 14 Jun 2019 11:54:42 +0200 > > Christophe de Dinechin <cdupontd(a)redhat.com&gt; wrote: > > > >> That is true irrespective of the usage, isn’t it? In other words, when you > >> invoke `mdevctl create-mdev`, you assert “I own that specific parent/type”. > >> At least, that’s how I read the way the script behaves today. Whether you > >> invoke uuidgen inside or outside the script does not change that assertion > >> (at least with today’s code). > > > > What gives you this impression? > > That your code does nothing to avoid any race today? > > Maybe I was confused with the existing `uuidgen` example in you README, > but it looks to me like the usage model involves much more than just > create-mdev, and that any race that might exist is not in create-mdev itself > (or in uuidgen for that matter). I believe I mentioned this was an early release, error handling is lacking. Still, I think the races are minimal, they largely involve uuid collisions. Separate users can create mdevs under the same parent concurrently, they would need to have a uuid collision to conflict. Otherwise there's the resource issue on the parent, but that's left of the kernel to manage. If an mdev fails to be created at the kernel, mdevctl should unwind, but we're not going to pretend that we have a lock on the parent's sysfs mdev interfaces. > > Where is the parent/type ownership implied? > > I did not imply it, but I read some concern about ownership > on your part in "they need to guess that an mdev device > with the same parent and type is *theirs*.” (emphasis mine) > > I personally see no change on the “need to guess” implied > by the fact that you run uuidgen inside the script, so > that’s why I tried to guess what you meant. As I noted in the reply to the pull request, putting `uuidgen` inline was probably a bad example. However, the difference is that the user has imposed the race on themselves if they invoke mdevctl like this, they've provided a uuid but they didn't record what it is. This is the user's problem. Pushing uuid selection into mdevctl makes it mdevctl's problem because the interface is fundamentally broken. > > The intended semantics are > > "try to create this type of device under this parent”. > > Agreed. Which is why I don’t see why trying to create > with some new UUID introduces any race (as long as > the script prints out that UUID, which I admit my patch > entirely failed to to) And that's the piece that makes it fundamentally broken. Beyond that, it seems unnecessary. I don't see this as the primary invocation of mdevctl and the functionality it adds is trivially accomplished in a wrapper, so what's the value? > >>> How do you resolve two instances of this happening in parallel and both > >>> coming to the same conclusion which is their device. If a user wants > >>> this sort of headache they can call mdevctl with `uuidgen` but I don't > >>> think we should encourage it further. > >> > >> I agree there is a race, but if anything, having a usage where you don’t > >> pass the UUID on the command line is a step in the right direction. > >> It leaves the door open for the create-mdev script to do smarter things, > >> like deferring the allocation of the mdevs to an entity that has slightly > >> more knowledge of the global system state than uuidgen. > > > > A user might (likely) require a specific uuid to match their VM > > configuration. I can only think of very niche use cases where a user > > doesn't care what uuid they get. > > They do care. But I typically copy-paste my UUIDs, and then > > 1. copy-pasting at the end is always faster than between > the command and other arguments (3-args case). > > 2. copy-pasting the output of the previous command is faster > than having one extra step where I need to copy the same thing twice > (2-args case). > > So to me, if the script is intended to be used by humans, my > proposal makes it slightly more comfortable to use. Nothing more. This is your preference, but I wouldn't call it universal. Specifying the uuid last seems backwards to me, we're creating an object so let's first name that object. We then specify where that object should be created and what type it has. This seems very logical to me, besides, it's also the exact same order we use when listing mdevs :P Clearly there's personal preference here, so let's not arbitrarily pick a different preference. If copy/paste order is more important to you then submit a patch to give mdevctl real argument processing so you can specify --uuid, --parent, --type in whatever order you want. > >> In other words, in my mind, `mdevctl create-mdev parent type` does not > >> imply “this will use uuidgen” but rather, if anything, implies “this will do the > >> right thing to prevent the race in the future, even if that’s more complex > >> than just calling uuidgen”. > > > > What race are you trying to prevent, uuid collision? > > Of course not ;-) > > I only added the part of the discussion below trying to figure out what > race you were seeing that was present only with my proposed changes. > > I (apparently incorrectly) supposed that you had some kind of mdev > management within the script in mind. Obviously I misinterpreted. > That will teach me to guess when I don’t understand instead of just > ask… The management mdevctl provides is persistence, interrogation, and interaction with mdevs and parent devices. Like every other tool, I'm going to defer policy related decisions to someone else. Picking a uuid is policy. Picking a parent device is policy. > >> However, I believe that this means we should reorder the args further. > >> I would suggest something like: > >> > >> mdevctl create-mdev <mdev-type> [<parent-device> [<mdev-uuid>]] > >> > >> where > > > > Absolutely not, now you've required mdevctl to implement policy in mdev > > placement. > > No, I’m not requiring it. I’m leaving the door open if one day, say, we decide > to have libvirt tell us about the placement. That usage needs not go in right away, > I marked it as “(future)”. > > Basically, all I’m saying is that since it’s early, we can reorder the > arguments so that the one you are most likely to change when you reuse > the command are the one that are last on the command-line, so that it > makes editing or copy-pasting easier. There isn’t more to it, and that’s > why I still do not see any new race introduced by that change. This is just compounding a shortcut made for this initial version, clearly the solution is to allow arbitrary option ordering, not to pick a different ordering for a perceived copy/paste efficiency > > mdevctl follows the unix standard, do one thing and do it > > well. If someone wants to layer placement policy on top of mdevctl, > > great, but let's not impose that within mdevctl. > > I’m not imposing anything (I believe). I was only trying to guess > where you saw things going that would imply there was a race with > my proposal that was not there without :-) > > > > >> 1 arg means you let mdevctl choose the parent device for you (future) > >> (e.g. I want a VGPU of this type, I don’t really care where it comes from) > >> 2 args mean you want that specific type/parent combination > >> 3 args mean you assert you own that device > >> > >> That also implies that mdevctl create-mdev should output what it allocated > >> so that some higher-level software can tell “OK, that’s the instance I got”. > > > > I don't think we're aligned on what mdevctl is attempting to provide. > > Maybe you're describing a layer you'd like to see above mdevctl? > > Thanks, > > No, again, I’m just trying to understand where you see a race. > > Maybe instead of guessing, I should just ask: where is the race in > the two-args variant (assuming it prints the UUID it used) that does not > exist with the three-args variant? Currently users deterministically know the uuid of the mdev they create, they specify it. They're not racing other users that potentially created the same type on the same parent to guess which mdev might be theirs, potentially picking the wrong one, potentially picking the same as their counterpart. Returning the uuid solves this race, but as you noted, implies a much broader uuid allocation scheme and policy which I'm not interested in providing in mdevctl until you can convince me otherwise. Thanks,

Alex

On Thu, May 23, 2019 at 05:20:01PM -0600, Alex Williamson wrote: > Hi, > > Currently mediated device management, much like SR-IOV VF management, > is largely left as an exercise for the user. This is an attempt to > provide something and see where it goes. I doubt we'll solve > everyone's needs on the first pass, but maybe we'll solve enough and > provide helpers for the rest. Without further ado, I'll point to what > I have so far: > > https://github.com/awilliam/mdevctl > > This is inspired by driverctl, which is also a bash utility. mdevctl > uses udev and systemd to record and recreate mdev devices for > persistence and provides a command line utility for querying, listing, > starting, stopping, adding, and removing mdev devices. Currently, for > better or worse, it considers anything created to be persistent. I can > imagine a global configuration option that might disable this and > perhaps an autostart flag per mdev device, such that mdevctl might > simply "know" about some mdevs but not attempt to create them > automatically. Clearly command line usage help, man pages, and > packaging are lacking as well, release early, release often, plus this > is a discussion starter to see if perhaps this is sufficient to meet > some needs. I think from libvirt's POV, we would *not* want devices to be made unconditionally persistent. We usually wish to expose a choice to applications whether to have resources be transient or persistent. So from that POV, a global config option to turn off persistence is not workable either. We would want control per-device, with autostart control per device too.

I would simply get rid of the udev rule that magically persists stuff. Any person/tool using sysfs right now expects devices to be transient. If they want to have persistence they can stop using sysfs & use higher level tools directly.

> Originally I thought about making a utility to manage both mdev and > SR-IOV VFs all in one, but it seemed more natural to start here > (besides, I couldn't think of a good name for the combined utility). > If this seems useful, maybe I'll start on a vfctl for SR-IOV and we'll > see whether they have enough synergy to become one. [snip] > I'm also curious how or if libvirt or openstack might use this. If > nothing else, it makes libvirt hook scripts easier to write, especially > if we add an option not to autostart mdevs, or if users don't mind > persistent mdevs, maybe there's nothing more to do. We currently have an API for creating host devices in libvirt which we use for NPIV devices only, which is where we'd like to put mdev creation support. This API is for creating transient devices though, so we don't want anything created this way to magically become persistent. For persistence we'd create a new API in libvirt allowing you to define & undefine the persistent config for a devices, and another set of APIs to create/delete from the persistent config. As a general rule, libvirt would prefer to use an API rather than spawning external commands, but can live with either. There's also the question of systemd integration - so far everything in libvirt still works on non-systemd based distros, but this new tool looks like it requires systemd. Personally I'm not too bothered by this but others might be more concerned.

On Mon, Jun 17, 2019 at 08:54:38AM -0600, Alex Williamson wrote: > On Mon, 17 Jun 2019 15:00:00 +0100 > Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > > On Thu, May 23, 2019 at 05:20:01PM -0600, Alex Williamson wrote: > > > Hi, > > > > > > Currently mediated device management, much like SR-IOV VF management, > > > is largely left as an exercise for the user. This is an attempt to > > > provide something and see where it goes. I doubt we'll solve > > > everyone's needs on the first pass, but maybe we'll solve enough and > > > provide helpers for the rest. Without further ado, I'll point to what > > > I have so far: > > > > > > https://github.com/awilliam/mdevctl > > > > > > This is inspired by driverctl, which is also a bash utility. mdevctl > > > uses udev and systemd to record and recreate mdev devices for > > > persistence and provides a command line utility for querying, listing, > > > starting, stopping, adding, and removing mdev devices. Currently, for > > > better or worse, it considers anything created to be persistent. I can > > > imagine a global configuration option that might disable this and > > > perhaps an autostart flag per mdev device, such that mdevctl might > > > simply "know" about some mdevs but not attempt to create them > > > automatically. Clearly command line usage help, man pages, and > > > packaging are lacking as well, release early, release often, plus this > > > is a discussion starter to see if perhaps this is sufficient to meet > > > some needs. > > > > I think from libvirt's POV, we would *not* want devices to be made > > unconditionally persistent. We usually wish to expose a choice to > > applications whether to have resources be transient or persistent. > > > > So from that POV, a global config option to turn off persistence > > is not workable either. We would want control per-device, with > > autostart control per device too. > > The code has progressed somewhat in the past 3+ weeks, we still persist > all devices, but the start-up mode can be selected per device or with a > global default mode. Devices configured with 'auto' start-up > automatically while 'manual' devices are simply known and available to > be started. I imagine we could add a 'transient' mode where we purge > the information about the device when it is removed or the next time > the parent device is added. Having a pesistent config written out & then purged later is still problematic. If the host crashes, nothing will purge the config file, so it will become a persistent device. Also when listing devices we want to be able to report whether it is persistent or transient. The obvious way todo that is to simply look if a config file exists or not.

> > I would simply get rid of the udev rule that magically persists > > stuff. Any person/tool using sysfs right now expects devices to > > be transient. If they want to have persistence they can stop using > > sysfs & use higher level tools directly. > > I think it's an interesting feature, but it's easy enough to control > via a global option in sysconfig with the default off if it's seen as > overstepping. A global option is really not desirable, as it means that the behaviour of the system that libvirt sees can silently change at any time. IMHO this udev hook is intermixing the two layers in the stack - keep the low level sysfs layer completely separate from the higher level mgmt concepts provided by this mdevctrl.

> > > Originally I thought about making a utility to manage both mdev and > > > SR-IOV VFs all in one, but it seemed more natural to start here > > > (besides, I couldn't think of a good name for the combined utility). > > > If this seems useful, maybe I'll start on a vfctl for SR-IOV and we'll > > > see whether they have enough synergy to become one. > > > > [snip] > > > > > I'm also curious how or if libvirt or openstack might use this. If > > > nothing else, it makes libvirt hook scripts easier to write, especially > > > if we add an option not to autostart mdevs, or if users don't mind > > > persistent mdevs, maybe there's nothing more to do. > > > > We currently have an API for creating host devices in libvirt which > > we use for NPIV devices only, which is where we'd like to put mdev > > creation support. This API is for creating transient devices > > though, so we don't want anything created this way to magically > > become persistent. > > > > For persistence we'd create a new API in libvirt allowing you to > > define & undefine the persistent config for a devices, and another > > set of APIs to create/delete from the persistent config. > > > > As a general rule, libvirt would prefer to use an API rather than > > spawning external commands, but can live with either. Thinking some more, the key tasks that libvirt needs to deal with are 1. Define a persistent config (Must not create any actual device) 2. Undefine a persistent config (Must not delete any actual device) 3. Create a device 4. Delete a device 5. Get list of all persistent configs 6. Enable autostart of a device 7. Disable autostart of a device For 1, 2, 5, 6 & 7 libvirt doesn't really need a command line tool. As long as there is a specification for the config file syntax and location we can directly read/write/stat files. This would be much more efficient and reliable for libvirt than spawning commands & parsing the output or exit status.

For 3 & 4 we'd desire the command to accept a config file, either as a file on disk for persistent devices, or inline on stdin for transient devices. > > There's also the question of systemd integration - so far everything > > in libvirt still works on non-systemd based distros, but this new > > tool looks like it requires systemd. Personally I'm not too bothered > > by this but others might be more concerned. > > Yes, Pavel brought up this issue offline as well and it needs more > consideration. The systemd support still needs work as well, I've > discovered it gets very confused when the mdev device is removed > outside of mdevctl, but I haven't yet been able to concoct a BindsTo= > line that can handle the hyphens in the uuid device name. I'd say > mdevctl is not intentionally systemd specific, it's simply a byproduct > of the systems it was developed on. Also, if libvirt were to focus > only on transient devices, then startup via systemctl doesn't make > sense, which probably means stopping via systemctl would also be unused > by libvirt. So I think this means we just need to make systemd an > optional feature of mdevctl (or drop it) and if libvirt doesn't use it, > that's fine. Thanks, I actually think the systemd integration is good, we just need to make sure things are still doable without it. This could be as simple as having an initscript that just iteates over the configs creating each one at startup.

On Mon, 17 Jun 2019 16:10:30 +0100 Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > On Mon, Jun 17, 2019 at 08:54:38AM -0600, Alex Williamson wrote: > > On Mon, 17 Jun 2019 15:00:00 +0100 > > Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > > > > On Thu, May 23, 2019 at 05:20:01PM -0600, Alex Williamson wrote: > > > > Hi, > > > > > > > > Currently mediated device management, much like SR-IOV VF management, > > > > is largely left as an exercise for the user. This is an attempt to > > > > provide something and see where it goes. I doubt we'll solve > > > > everyone's needs on the first pass, but maybe we'll solve enough and > > > > provide helpers for the rest. Without further ado, I'll point to what > > > > I have so far: > > > > > > > > https://github.com/awilliam/mdevctl > > > > > > > > This is inspired by driverctl, which is also a bash utility. mdevctl > > > > uses udev and systemd to record and recreate mdev devices for > > > > persistence and provides a command line utility for querying, listing, > > > > starting, stopping, adding, and removing mdev devices. Currently, for > > > > better or worse, it considers anything created to be persistent. I can > > > > imagine a global configuration option that might disable this and > > > > perhaps an autostart flag per mdev device, such that mdevctl might > > > > simply "know" about some mdevs but not attempt to create them > > > > automatically. Clearly command line usage help, man pages, and > > > > packaging are lacking as well, release early, release often, plus this > > > > is a discussion starter to see if perhaps this is sufficient to meet > > > > some needs. > > > > > > I think from libvirt's POV, we would *not* want devices to be made > > > unconditionally persistent. We usually wish to expose a choice to > > > applications whether to have resources be transient or persistent. > > > > > > So from that POV, a global config option to turn off persistence > > > is not workable either. We would want control per-device, with > > > autostart control per device too. > > > > The code has progressed somewhat in the past 3+ weeks, we still persist > > all devices, but the start-up mode can be selected per device or with a > > global default mode. Devices configured with 'auto' start-up > > automatically while 'manual' devices are simply known and available to > > be started. I imagine we could add a 'transient' mode where we purge > > the information about the device when it is removed or the next time > > the parent device is added. > > Having a pesistent config written out & then purged later is still > problematic. If the host crashes, nothing will purge the config file, > so it will become a persistent device. Also when listing devices we > want to be able to report whether it is persistent or transient. The > obvious way todo that is to simply look if a config file exists or > not. I was thinking that the config file would identify the device as transient, therefore if the system crashed we'd have the opportunity to purge those entries on the next boot as we're processing the entries for that parent device. Clearly it has yet to be implemented, but I expect there are some advantages to tracking devices via a transient config entry or else we're constantly re-discovering foreign mdevs.

On Tue, Jun 18, 2019 at 1:01 PM Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > On Mon, 17 Jun 2019 11:05:17 -0600 > Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > On Mon, 17 Jun 2019 16:10:30 +0100 > > Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > > > > On Mon, Jun 17, 2019 at 08:54:38AM -0600, Alex Williamson wrote: > > > > On Mon, 17 Jun 2019 15:00:00 +0100 > > > > Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > > > > > > > > On Thu, May 23, 2019 at 05:20:01PM -0600, Alex Williamson wrote: > > > > > > > Hi, > > > > > > > > > > > > Currently mediated device management, much like SR-IOV VF > management, > > > > > > is largely left as an exercise for the user. This is an attempt > to > > > > > > provide something and see where it goes. I doubt we'll solve > > > > > > everyone's needs on the first pass, but maybe we'll solve enough > and > > > > > > provide helpers for the rest. Without further ado, I'll point > to what > > > > > > I have so far: > > > > > > > > > > > > https://github.com/awilliam/mdevctl > > > > > > > > > > > > This is inspired by driverctl, which is also a bash utility. > mdevctl > > > > > > uses udev and systemd to record and recreate mdev devices for > > > > > > persistence and provides a command line utility for querying, > listing, > > > > > > starting, stopping, adding, and removing mdev devices. > Currently, for > > > > > > better or worse, it considers anything created to be > persistent. I can > > > > > > imagine a global configuration option that might disable this and > > > > > > perhaps an autostart flag per mdev device, such that mdevctl > might > > > > > > simply "know" about some mdevs but not attempt to create them > > > > > > automatically. Clearly command line usage help, man pages, and > > > > > > packaging are lacking as well, release early, release often, > plus this > > > > > > is a discussion starter to see if perhaps this is sufficient to > meet > > > > > > some needs. > > > > > > > > > > I think from libvirt's POV, we would *not* want devices to be made > > > > > unconditionally persistent. We usually wish to expose a choice to > > > > > applications whether to have resources be transient or persistent. > > > > > > > > > > So from that POV, a global config option to turn off persistence > > > > > is not workable either. We would want control per-device, with > > > > > autostart control per device too. > > > > > > > > The code has progressed somewhat in the past 3+ weeks, we still > persist > > > > all devices, but the start-up mode can be selected per device or > with a > > > > global default mode. Devices configured with 'auto' start-up > > > > automatically while 'manual' devices are simply known and available > to > > > > be started. I imagine we could add a 'transient' mode where we purge > > > > the information about the device when it is removed or the next time > > > > the parent device is added. > > > > > > Having a pesistent config written out & then purged later is still > > > problematic. If the host crashes, nothing will purge the config file, > > > so it will become a persistent device. Also when listing devices we > > > want to be able to report whether it is persistent or transient. The > > > obvious way todo that is to simply look if a config file exists or > > > not. > > > > I was thinking that the config file would identify the device as > > transient, therefore if the system crashed we'd have the opportunity to > > purge those entries on the next boot as we're processing the entries > > for that parent device. Clearly it has yet to be implemented, but I > > expect there are some advantages to tracking devices via a transient > > config entry or else we're constantly re-discovering foreign mdevs. > > I think we need to reach consensus about the actual scope of the > mdevctl tool. > > Thanks Cornelia, my thoughts: - Is it supposed to be responsible for managing *all* mdev devices in > the system, or is it more supposed to be a convenience helper for > users/software wanting to manage mdevs? > The latter. If an operator (or some software) wants to create mdevs by not using mdevctl (and rather directly calling the sysfs), I think it's OK. That said, mdevs created by mdevctl would be supported by systemctl, while the others not but I think it's okay.

- Do we want mdevctl to manage config files for individual mdevs, or > are they supposed to be in a common format that can also be managed > by e.g. libvirt? > Unless I misunderstand, I think mdevctl just helps to create mdevs for being used by guests created either by libvirt or QEMU or even others. How a guest would allocate a mdev (ie. saying "I'll use this specific mdev UUID") is IMHO not something for mdevctl.

- Should mdevctl be a stand-alone tool, provide library functions, or > both? Related: should it keep any internal state that is not written > to disk? (I think that also plays into the transient vs. persistent > question.)

FWIW, I'd love using mdevctl for OpenStack (Nova) just at least for creating persisted mdevs (ie. mdevs that would be recreated after rebooting using systemctl). That's the real use case I need. Whether libvirt would internally support mdevctl would be nice but that's not really something Nova needs, so I leave others providing their own thoughts. My personal opinion is that mdevctl should be able to tolerate mdevs > being configured by other means, but probably should not try to impose > its own configuration if it detects that (unless explicitly asked to do > so). Not sure how feasible that goal is. > > That's what I misunderstand : in order to have a guest using a vGPU, you need to do two things : 1/ create the mdev 2/ allocate this created dev to a specific guest config Of course, we could imagine a way to have both steps to be done directly by libvirt, but from my opinion, mdevctl is really helping 1/ and not 2/.

> A well-defined config file format is probably a win, even if it only > ends up being used by mdevctl itself.

On Tue, Jun 18, 2019 at 04:12:10PM -0600, Alex Williamson wrote: > On Tue, 18 Jun 2019 14:48:11 +0200 > Sylvain Bauza <sbauza(a)redhat.com&gt; wrote: > > > On Tue, Jun 18, 2019 at 1:01 PM Cornelia Huck <cohuck(a)redhat.com&gt; wrote:

> > > I think we need to reach consensus about the actual scope of the > > > mdevctl tool. > > > > > > > > Thanks Cornelia, my thoughts: > > > > - Is it supposed to be responsible for managing *all* mdev devices in > > > the system, or is it more supposed to be a convenience helper for > > > users/software wanting to manage mdevs? > > > > > > > The latter. If an operator (or some software) wants to create mdevs by not > > using mdevctl (and rather directly calling the sysfs), I think it's OK. > > That said, mdevs created by mdevctl would be supported by systemctl, while > > the others not but I think it's okay. > > I agree (sort of), and I'm hearing that we should drop any sort of > automatic persistence of mdevs created outside of mdevctl. The problem > comes when we try to draw the line between unmanaged and manged > devices. For instance, if we have a command to list mdevs it would > feel incomplete if it didn't list all mdevs both those managed by > mdevctl and those created elsewhere. For managed devices, I expect > we'll also have commands that allow the mode of the device to be > switched between transient, saved, and persistent. Should a user then

> be allowed to promote an unmanaged device to one of these modes via the > same command? Should they be allowed to stop an unmanaged device > through driverctl? Through systemctl? These all seem like reasonable > things to do, so what then is the difference between transient and > unmanaged mdev and is mdevctl therefore managing all mdevs, not just > those it has created? To my mind there shouldn't really need to be a difference between transient mdevs created by mdevctrl and mdevs created by an user directly using sysfs. Both are mdevs on the running system with no config file that you have to enumerate by looking at sysfs. This ties back to my belief that we shouldn't need to have any config on disk for a transient mdev, just discover them all dynamically when required.

> > - Do we want mdevctl to manage config files for individual mdevs, or > > > are they supposed to be in a common format that can also be managed > > > by e.g. libvirt? > > > > > > > Unless I misunderstand, I think mdevctl just helps to create mdevs for > > being used by guests created either by libvirt or QEMU or even others. > > How a guest would allocate a mdev (ie. saying "I'll use this specific mdev > > UUID") is IMHO not something for mdevctl. > > Right, mdevctl isn't concerned with how a specific mdev is used, but I > think what Connie is after is more the proposal from Daniel where > libvirt can essentially manage mdevctl config files itself and then > only invoke mdevctl for the dirty work of creating and deleting > devices. In fact, assuming systemd, libvirt could avoid direct > interaction with mdevctl entirely, instead using systemctl device units > to start and stop the mdevs. Maybe where that proposal takes a turn is > when we again consider non-systemd hosts, where maybe mdevctl needs to > write out an init script per mdev and libvirt injecting itself into > manipulation of the config files would either need to perform the same > or fall back to mdevctl. Unfortunately there seems to be an ultimatum > to either condone external config file manipulation or expand the scope > of the project into becoming a library. Is mdevctl really tackling a problem that is complex enough that we will gain significantly by keeping the config files private and forcing use of a CLI or Library to access them ? The amount of information that we need to store per mdev looks pretty small, with minimal compound structure. To me it feels like we can easily define a standard config format without suffering any serious long term pain, as the chances we'd need to radically change it look minimal.

> > - Should mdevctl be a stand-alone tool, provide library functions, or > > > both? Related: should it keep any internal state that is not written > > > to disk? (I think that also plays into the transient vs. persistent > > > question.) > > I don't think we want an mdevctld, if that's what you mean by internal > state not written to disk. I think we ideally want all state in the > mdev config files or discerned through sysfs. How we handle > non-systemd hosts may throw a wrench in that though since currently the > systemd integration relies on a template to support arbitrary mdevs and > I'm not sure how to replicate that in other init services. If we need > to dynamically manage per mdev init files in addition to config files, > we're not so self contained. The most important part of the init script integration is just the bulk creation of mdevs on startup. I think this could be handled on non-systemd hosts via a fairly dumb init script that does this something approximating this: for dev in `mdevctl list` do mdevctrl get-autostart $dev test $? = 0 && mdevctrl start $dev done ie, iterate over all configs. If the config is marked to autostart, then start it.

> > FWIW, I'd love using mdevctl for OpenStack (Nova) just at least for > > creating persisted mdevs (ie. mdevs that would be recreated after rebooting > > using systemctl). That's the real use case I need. > > Whether libvirt would internally support mdevctl would be nice but that's > > not really something Nova needs, so I leave others providing their own > > thoughts. > > > > > > My personal opinion is that mdevctl should be able to tolerate mdevs > > > being configured by other means, but probably should not try to impose > > > its own configuration if it detects that (unless explicitly asked to do > > > so). Not sure how feasible that goal is. > > > > > > That's what I misunderstand : in order to have a guest using a vGPU, you > > need to do two things : > > 1/ create the mdev > > 2/ allocate this created dev to a specific guest config > > > > Of course, we could imagine a way to have both steps to be done directly by > > libvirt, but from my opinion, mdevctl is really helping 1/ and not 2/.

> > Yep, we also don't want to presume libvirt is the only consumer here. > mdevctl should also support other VM management tools, users who write > their own management scripts, and even non-VM related use cases. > > > > A well-defined config file format is probably a win, even if it only > > > ends up being used by mdevctl itself. > > Yes, regardless of whether others touch them, conversion scripts on > upgrade should be avoided. Do we need something beyond a key=value > file? So far we're only storing the mdev type and startup mode, but > vfio-ap clearly needs more, apparently key=value1,value2,... type > representation. Still, I think I'd prefer simple over jumping to xml > or json or yaml. Thanks, For libvirt our preference would be something we can easily support without having to write new parsers. I'm not going to suggest XML, but JSON is probably our highest preference. If not then a simple flat file with one line of key="value" per setting is something we already parse for /etc/libvirt/libvirtd.conf file. For slightly more structure the .ini style file is also good. That's basically just flat key=value pairs, but with [section] headers so you can represent some level of structured data.

On Wed, 19 Jun 2019 11:46:59 +0200 Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > On Wed, 19 Jun 2019 08:28:02 +0100 > Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > > On Tue, Jun 18, 2019 at 04:12:10PM -0600, Alex Williamson wrote: > > > On Tue, 18 Jun 2019 14:48:11 +0200 > > > Sylvain Bauza <sbauza(a)redhat.com&gt; wrote: > > > > > > > On Tue, Jun 18, 2019 at 1:01 PM Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > > > > > > I think we need to reach consensus about the actual scope of the > > > > > mdevctl tool. > > > > > > > > > > > > > > Thanks Cornelia, my thoughts: > > > > > > > > - Is it supposed to be responsible for managing *all* mdev devices in > > > > > the system, or is it more supposed to be a convenience helper for > > > > > users/software wanting to manage mdevs? > > > > > > > > > > > > > The latter. If an operator (or some software) wants to create mdevs by not > > > > using mdevctl (and rather directly calling the sysfs), I think it's OK. > > > > That said, mdevs created by mdevctl would be supported by systemctl, while > > > > the others not but I think it's okay. > > > > > > I agree (sort of), and I'm hearing that we should drop any sort of > > > automatic persistence of mdevs created outside of mdevctl. The problem > > > comes when we try to draw the line between unmanaged and manged > > > devices. For instance, if we have a command to list mdevs it would > > > feel incomplete if it didn't list all mdevs both those managed by > > > mdevctl and those created elsewhere. For managed devices, I expect > > > we'll also have commands that allow the mode of the device to be > > > switched between transient, saved, and persistent. Should a user then > > Hm, what's the difference between 'saved' and 'persistent'? That > 'saved' devices are not necessarily present? It seems like we're coming up with the following classes: 1) transient a) mdevctl created b) foreign 2) defined a) automatic start-up b) manual start-up I was using persistent for 2b), but that's probably not a good name because devices can still be stopped, so they're not really persistently available even in this class.

> > To my mind there shouldn't really need to be a difference between > > transient mdevs created by mdevctrl and mdevs created by an user > > directly using sysfs. Both are mdevs on the running system with > > no config file that you have to enumerate by looking at sysfs. > > This ties back to my belief that we shouldn't need to have any > > config on disk for a transient mdev, just discover them all > > dynamically when required. > > So mdevctl can potentially interact with any mdev device on the system, > it just has to be instructed by a user or software to do so? I think we > can work with that. Some TBDs around systemd/init support for transient devices and how transient devices can be promoted to defined. For instance if a vfio-ap device requires matrix programming after instantiation, can we glean that programming from sysfs or is there metadata irrecoverably lost if no config file is created for a transient device? This would also imply that a 1b) foreign device could not be promoted to 2x) defined device.

On Wed, Jun 19, 2019 at 12:27 AM Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > On Tue, 18 Jun 2019 14:48:11 +0200 > Sylvain Bauza <sbauza(a)redhat.com&gt; wrote: > > > On Tue, Jun 18, 2019 at 1:01 PM Cornelia Huck <cohuck(a)redhat.com&gt; wrote:

> > > I think we need to reach consensus about the actual scope of the > > > mdevctl tool. > > > > > > > > Thanks Cornelia, my thoughts: > > > > - Is it supposed to be responsible for managing *all* mdev devices in > > > the system, or is it more supposed to be a convenience helper for > > > users/software wanting to manage mdevs? > > > > > > > The latter. If an operator (or some software) wants to create mdevs by > not > > using mdevctl (and rather directly calling the sysfs), I think it's OK. > > That said, mdevs created by mdevctl would be supported by systemctl, > while > > the others not but I think it's okay. > > I agree (sort of), and I'm hearing that we should drop any sort of > automatic persistence of mdevs created outside of mdevctl. The problem > comes when we try to draw the line between unmanaged and manged > devices. For instance, if we have a command to list mdevs it would > feel incomplete if it didn't list all mdevs both those managed by > mdevctl and those created elsewhere. For managed devices, I expect > we'll also have commands that allow the mode of the device to be > switched between transient, saved, and persistent. Should a user then > be allowed to promote an unmanaged device to one of these modes via the > same command? Should they be allowed to stop an unmanaged device > through driverctl? Through systemctl? These all seem like reasonable > things to do, so what then is the difference between transient and > unmanaged mdev and is mdevctl therefore managing all mdevs, not just > those it has created? > > Well, IMHO, mdevs created by mdevctl could all be persisted or transient just by adding an option when calling mdevctl, like : "mdevctl create-mdev [--transient] <uuid> <pci_id> <type>" where default would be persisting the mdev.

For mdevs *not* created by mdevctl, then a usecase could be "I'd like to ask mdevctl to manage mdevs I already created" and if so, a mdevctl command like : "mdevctl manage-mdev [--transient] <mdev_uuid>"

Of course, that would mean that when you list mdevs by "mdev list-all" you wouldn't get mdevs managed by mdevctl. Thoughts ? > - Do we want mdevctl to manage config files for individual mdevs, or > > > are they supposed to be in a common format that can also be managed > > > by e.g. libvirt? > > > > > > > Unless I misunderstand, I think mdevctl just helps to create mdevs for > > being used by guests created either by libvirt or QEMU or even others. > > How a guest would allocate a mdev (ie. saying "I'll use this specific > mdev > > UUID") is IMHO not something for mdevctl. > > Right, mdevctl isn't concerned with how a specific mdev is used, but I > think what Connie is after is more the proposal from Daniel where > libvirt can essentially manage mdevctl config files itself and then > only invoke mdevctl for the dirty work of creating and deleting > devices. In fact, assuming systemd, libvirt could avoid direct > interaction with mdevctl entirely, instead using systemctl device units > to start and stop the mdevs. Maybe where that proposal takes a turn is > when we again consider non-systemd hosts, where maybe mdevctl needs to > write out an init script per mdev and libvirt injecting itself into > manipulation of the config files would either need to perform the same > or fall back to mdevctl. Unfortunately there seems to be an ultimatum > to either condone external config file manipulation or expand the scope > of the project into becoming a library. > > Well, like I said, I think it's maybe another user case : just using libvirt when you want a guest having vGPUs and then libvirt would create mdevs (so users wouln't need to know at that). That said, for the moment, I think we don't really need it so maybe a new RFE once we at least have mdevctl packaged and supported by RHEL ?

> - Should mdevctl be a stand-alone tool, provide library functions, or > > > both? Related: should it keep any internal state that is not written > > > to disk? (I think that also plays into the transient vs. persistent > > > question.) > > I don't think we want an mdevctld, if that's what you mean by internal

> state not written to disk. I think we ideally want all state in the > mdev config files or discerned through sysfs. How we handle > non-systemd hosts may throw a wrench in that though since currently the > systemd integration relies on a template to support arbitrary mdevs and > I'm not sure how to replicate that in other init services. If we need > to dynamically manage per mdev init files in addition to config files, > we're not so self contained. > > > FWIW, I'd love using mdevctl for OpenStack (Nova) just at least for > > creating persisted mdevs (ie. mdevs that would be recreated after > rebooting > > using systemctl). That's the real use case I need. > > Whether libvirt would internally support mdevctl would be nice but that's > > not really something Nova needs, so I leave others providing their own > > thoughts. > > > > > > My personal opinion is that mdevctl should be able to tolerate mdevs > > > being configured by other means, but probably should not try to impose > > > its own configuration if it detects that (unless explicitly asked to do > > > so). Not sure how feasible that goal is. > > > > > > That's what I misunderstand : in order to have a guest using a vGPU, > you > > need to do two things : > > 1/ create the mdev > > 2/ allocate this created dev to a specific guest config > > > > Of course, we could imagine a way to have both steps to be done directly > by > > libvirt, but from my opinion, mdevctl is really helping 1/ and not 2/. > > Yep, we also don't want to presume libvirt is the only consumer here. > mdevctl should also support other VM management tools, users who write > their own management scripts, and even non-VM related use cases. > > Oh yes, please don't premuse mdevctl is only needed by libvirt. FWIW, once mdevctl is supported by RHEL, I'd love to use it for OpenStack Nova at least because I want to persist the mdevs. At the moment, Nova just creates mdevs directly by sysfs and look the existing onces up by sysfs, but upstream community in Nova thinks the mission statement is not about managing mdevs so we don't really want to add in Nova some kind of DB persistence just for mdevs.

> > A well-defined config file format is probably a win, even if it only > > > ends up being used by mdevctl itself. > > Yes, regardless of whether others touch them, conversion scripts on > upgrade should be avoided. Do we need something beyond a key=value > file? So far we're only storing the mdev type and startup mode, but > vfio-ap clearly needs more, apparently key=value1,value2,... type > representation. Still, I think I'd prefer simple over jumping to xml > or json or yaml. Thanks, > > Heh, in OpenStack discussing about a file format is possibly one of the longest arguments we already have, so I leave others to say their own opinions but FWIW, as we use Python we tend to prefer YAML files. I don't care about the format tho, just take the most convenient for libvirt I'd say.

On Wed, Jun 19, 2019 at 12:27 AM Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > On Tue, 18 Jun 2019 14:48:11 +0200 > Sylvain Bauza <sbauza(a)redhat.com&gt; wrote: > > > On Tue, Jun 18, 2019 at 1:01 PM Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > > > > > On Mon, 17 Jun 2019 11:05:17 -0600 > > > Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > > > > > On Mon, 17 Jun 2019 16:10:30 +0100 > > > > Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > > > > > > > > On Mon, Jun 17, 2019 at 08:54:38AM -0600, Alex Williamson wrote: > > > > > > On Mon, 17 Jun 2019 15:00:00 +0100 > > > > > > Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > > > > > > > > > > > > On Thu, May 23, 2019 at 05:20:01PM -0600, Alex Williamson > wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > Currently mediated device management, much like SR-IOV VF > > > management, > > > > > > > > is largely left as an exercise for the user. This is an > attempt > > > to > > > > > > > > provide something and see where it goes. I doubt we'll solve > > > > > > > > everyone's needs on the first pass, but maybe we'll solve > enough > > > and > > > > > > > > provide helpers for the rest. Without further ado, I'll > point > > > to what > > > > > > > > I have so far: > > > > > > > > > > > > > > > > https://github.com/awilliam/mdevctl > > > > > > > > > > > > > > > > This is inspired by driverctl, which is also a bash > utility. > > > mdevctl > > > > > > > > uses udev and systemd to record and recreate mdev devices for > > > > > > > > persistence and provides a command line utility for > querying, > > > listing, > > > > > > > > starting, stopping, adding, and removing mdev devices. > > > Currently, for > > > > > > > > better or worse, it considers anything created to be > > > persistent. I can > > > > > > > > imagine a global configuration option that might disable > this and > > > > > > > > perhaps an autostart flag per mdev device, such that > mdevctl > > > might > > > > > > > > simply "know" about some mdevs but not attempt to create them > > > > > > > > automatically. Clearly command line usage help, man pages, > and > > > > > > > > packaging are lacking as well, release early, release > often, > > > plus this > > > > > > > > is a discussion starter to see if perhaps this is sufficient > to > > > meet > > > > > > > > some needs. > > > > > > > > > > > > > > I think from libvirt's POV, we would *not* want devices to be > made > > > > > > > unconditionally persistent. We usually wish to expose a choice > to > > > > > > > applications whether to have resources be transient or > persistent. > > > > > > > > > > > > > > So from that POV, a global config option to turn off > persistence > > > > > > > is not workable either. We would want control per-device, with > > > > > > > autostart control per device too. > > > > > > > > > > > > The code has progressed somewhat in the past 3+ weeks, we still > > > persist > > > > > > all devices, but the start-up mode can be selected per device > or > > > with a > > > > > > global default mode. Devices configured with 'auto' start-up > > > > > > automatically while 'manual' devices are simply known and > available > > > to > > > > > > be started. I imagine we could add a 'transient' mode where we > purge > > > > > > the information about the device when it is removed or the next > time > > > > > > the parent device is added. > > > > > > > > > > Having a pesistent config written out & then purged later is still > > > > > problematic. If the host crashes, nothing will purge the config > file, > > > > > so it will become a persistent device. Also when listing devices we > > > > > want to be able to report whether it is persistent or transient. > The > > > > > obvious way todo that is to simply look if a config file exists or > > > > > not. > > > > > > > > I was thinking that the config file would identify the device as > > > > transient, therefore if the system crashed we'd have the opportunity > to > > > > purge those entries on the next boot as we're processing the entries > > > > for that parent device. Clearly it has yet to be implemented, but I > > > > expect there are some advantages to tracking devices via a transient > > > > config entry or else we're constantly re-discovering foreign mdevs. > > > > > > I think we need to reach consensus about the actual scope of the > > > mdevctl tool. > > > > > > > > Thanks Cornelia, my thoughts: > > > > - Is it supposed to be responsible for managing *all* mdev devices in > > > the system, or is it more supposed to be a convenience helper for > > > users/software wanting to manage mdevs? > > > > > > > The latter. If an operator (or some software) wants to create mdevs by > not > > using mdevctl (and rather directly calling the sysfs), I think it's OK. > > That said, mdevs created by mdevctl would be supported by systemctl, > while > > the others not but I think it's okay. > > I agree (sort of), and I'm hearing that we should drop any sort of > automatic persistence of mdevs created outside of mdevctl. The problem > comes when we try to draw the line between unmanaged and manged > devices. For instance, if we have a command to list mdevs it would > feel incomplete if it didn't list all mdevs both those managed by > mdevctl and those created elsewhere. For managed devices, I expect > we'll also have commands that allow the mode of the device to be > switched between transient, saved, and persistent. Should a user then > be allowed to promote an unmanaged device to one of these modes via the > same command? Should they be allowed to stop an unmanaged device > through driverctl? Through systemctl? These all seem like reasonable > things to do, so what then is the difference between transient and > unmanaged mdev and is mdevctl therefore managing all mdevs, not just > those it has created? > > Well, IMHO, mdevs created by mdevctl could all be persisted or transient just by adding an option when calling mdevctl, like : "mdevctl create-mdev [--transient] <uuid> <pci_id> <type>" where default would be persisting the mdev. For mdevs *not* created by mdevctl, then a usecase could be "I'd like to ask mdevctl to manage mdevs I already created" and if so, a mdevctl command like : "mdevctl manage-mdev [--transient] <mdev_uuid>" Of course, that would mean that when you list mdevs by "mdev list-all" you wouldn't get mdevs managed by mdevctl. Thoughts ?

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.

> We now support "transient" devices and there's no distinction or > difference in handling of such devices whether they're created by > mdevctl or externally. All devices will also have systemd management, > though systemd is no longer required, it's used if available. The > instance name used for systemd device units has also changed to allow > us to use BindsTo= such that services are not only created, but are > also removed if the device is removed. Unfortunately it's not a simple > UUID via the systemd route any longer. That's a bit unfortunate; however, making it workable without systemd certainly is a good thing :)

> Since the original posting, the project has moved from my personal > github to here: > > https://github.com/mdevctl/mdevctl > > Please see the README there for overview of the new commands and > example of their usage. There is no attempt to maintain backwards > compatibility with previous versions, this tool is in its infancy. > Also since the original posting, RPM packaging is included, so simply > run 'make rpm' and install the resulting package. Nice. > > Highlights of this version include proper argument parsing via getopts > so that options can be provided in any order. I'm still using the > format 'mdevctl {command} [options]' but now it's consistent that all > the options come after the command, in any order. I think this is > relatively consistent with a variety of other tools. Parsing via getops is also very nice. > > Devices are no longer automatically persisted, we handle them as > transient, but we also can promote them to persistent through the > 'define' command. The define, undefine, and modify commands all > operate only on the config file, so that we can define separate from > creating. When promoting from a transient to defined device, we can > use the existing device to create the config. Both the type and the > startup of a device can be modified in the config, without affecting > the running device. > > Starting an mdev device no longer relies exclusively on a saved config, > the device can be fully specified via options to create a transient > device. Specifying only a uuid is also sufficient for a defined > device. Some consideration has also been given to uuid collisions. > The mdev interface in the kernel prevents multiple mdevs with the same > uuid running concurrently, but mdevctl allows mdevs to be defined with > the same uuid under separate parent devices. Some options therefore > allow both a uuid and parent to be specified and require this if the > uuid alone is ambiguous. Clearly starting two such devices at the same > time will fail and is left to higher level tools to manage, just like > the ability to define more devices than there are available instances on > the host system. I still have to look into the details of this. > > The stop and list commands are largely the same ideas as previous > though the semantics are completely different. Listing running devices > now notes which are defined versus transient. Perhaps it might also be > useful when listing defined devices to note which are running. Yes, I think it would be useful. > > The sbin/libexec split of mdevctl has been squashed. There are some > commands in the script that are currently only intended to be used from > udev or systemd, these are simply excluded from the help. It's > possible we may want to promote the start-parent-mdevs command out of > this class, but the rest are specifically systemd helpers. > > I'll include the current help test message below for further semantic > details, but please have a look-see, or better yet give it a try. Had a quick look, sent two pull requests with some minor tweaks. Still have to do a proper review, and actually try the thing on an s390.

> > PS - I'm looking at adding udev change events when a device registers > or unregisters with the mdev core, which should help us know when to > trigger creation of persistent, auto started devices. That support is > included here with the MDEV_STATE="registered|unregistered" environment > values. Particularly, kvmgt now supports dynamic loading an unloading, > so as long as the enable_gvt=1 option is provided to the i915 driver > mdev support can come and go independent of the parent device. The > change uevents are necessary to trigger on that, so I'd appreciate any > feedback on those as well. Until then, the persistence of mdevctl > really depends on mdev support on the parent device being _completely_ > setup prior to processing the udev rules. I'll need to think about this. Do you have some preliminary patches somewhere?

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

We should think about whether ordering is important and maybe incorporate that into key naming conventions or come up with some syntax for specifying startup blocks. Thanks, Alex

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.

> Similar for deleting the "assign_adapter" key. We have an > "unassign_adapter" attribute, but this is not something we can infer > automatically; we need to know that we're dealing with an vfio-ap > matrix device...> > >> >> We should think about whether ordering is important and maybe >> incorporate that into key naming conventions or come up with some >> syntax for specifying startup blocks. Thanks, >> >> Alex > > Hm... > > { > "foo": "1", > "bar": "42", > "baz": { > "depends": ["foo", "bar"], > "value": "plahh" > } > } > > Something like that?

> On 6/27/19 8:26 AM, Cornelia Huck wrote: > > > > { > > "foo": "1", > > "bar": "42", > > "baz": { > > "depends": ["foo", "bar"], > > "value": "plahh" > > } > > } > > > > Something like that? I'm not sure yet. I think we need to look at what's feasible (and easy) with jq. Thanks,

On Thu, 27 Jun 2019 15:15:02 -0600 Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > On Thu, 27 Jun 2019 09:38:32 -0600 > Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > On 6/27/19 8:26 AM, Cornelia Huck wrote: > > > > > > > > { > > > > "foo": "1", > > > > "bar": "42", > > > > "baz": { > > > > "depends": ["foo", "bar"], > > > > "value": "plahh" > > > > } > > > > } > > > > > > > > Something like that? > > > > I'm not sure yet. I think we need to look at what's feasible (and > > easy) with jq. Thanks, > > I think it's not too much trouble to remove and insert into arrays, so > what if we were to define the config as: > > { > "mdev_type":"vendor-type", > "start":"auto", > "attrs": [ > {"attrX":["Xvalue1","Xvalue2"]}, > {"dir/attrY": "Yvalue1"}, > {"attrX": "Xvalue3"} > ] > } > > "attr" here would define sysfs attributes under the device. The array > would be processed in order, so in the above example we'd do the > following: > > 1. echo Xvalue1 > attrX > 2. echo Xvalue2 > attrX > 3. echo Yvalue1 > dir/attrY > 4. echo Xvalue3 > attrX > > When starting the device mdevctl would simply walk the array, if the > attribute key exists write the value(s). If a write fails or the > attribute doesn't exist, remove the device and report error.

> > I think it's easiest with jq to manipulate arrays by removing and > inserting by index. Also if we end up with something like above, it's > ambiguous if we reference the "attrX" key. So perhaps we add the > following options to the modify command: > > --addattr=ATTRIBUTE --delattr --index=INDEX --value=VALUE1[,VALUE2] > > We could handle it like a stack, so if --index is not supplied, add to > the end or remove from the end. If --index is provided, delete that > index or add the attribute at that index. So if you had the above and > wanted to remove Xvalue1 but keep the ordering, you'd do: > > --delattr --index=0 > --addattr --index=0 --value=Xvalue2 > > Which should results in: > > "attrs": [ > {"attrX": "Xvalue2"}, > {"dir/attrY": "Yvalue1"}, > {"attrX": "Xvalue3"} > ]

> > If we want to modify a running device, I'm thinking we probably want a > new command and options --attr=ATTRIBUTE --value=VALUE might suffice. > > Do we need to support something like this for the 'start' command or > should we leave that for simple devices and require a sequence of: > > # mdevctl define ... > # mdevctl modify --addattr... > ... > # mdevctl start > # mdevctl undefine > > This is effectively the long way to get a transient device. Otherwise > we'd need to figure out how to have --attr --value appear multiple > times on the start command line. Thanks,

This is now implemented, and yes you can specify '--addattr remove --value 1' and mdevctl will immediately remove the device after it's created (more power to the admin). Listing defined devices also lists

any attributes defined for easy inspection. It is also possible to override the conversion of comma separated values into an array by encoding and escaping the comma. It's a little cumbersome, but possible in case a driver isn't fully on board with the one attribute, one value rule of sysfs. Does this work for vfio-ap? I also still

need to check if this allows an NVIDIA vGPU mdev to be configured such that the framerate limiter can be automatically controlled. Thanks, Alex

On Thu, 27 Jun 2019 19:57:04 -0600 Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > On Thu, 27 Jun 2019 15:15:02 -0600 > Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > On Thu, 27 Jun 2019 09:38:32 -0600 > > Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > > > > On 6/27/19 8:26 AM, Cornelia Huck wrote: > > > > > > > > > > { > > > > > "foo": "1", > > > > > "bar": "42", > > > > > "baz": { > > > > > "depends": ["foo", "bar"], > > > > > "value": "plahh" > > > > > } > > > > > } > > > > > > > > > > Something like that? > > > > > > I'm not sure yet. I think we need to look at what's feasible (and > > > easy) with jq. Thanks, > > > > I think it's not too much trouble to remove and insert into arrays, so > > what if we were to define the config as: > > > > { > > "mdev_type":"vendor-type", > > "start":"auto", > > "attrs": [ > > {"attrX":["Xvalue1","Xvalue2"]}, > > {"dir/attrY": "Yvalue1"}, > > {"attrX": "Xvalue3"} > > ] > > } > > > > "attr" here would define sysfs attributes under the device. The array > > would be processed in order, so in the above example we'd do the > > following: > > > > 1. echo Xvalue1 > attrX > > 2. echo Xvalue2 > attrX > > 3. echo Yvalue1 > dir/attrY > > 4. echo Xvalue3 > attrX > > > > When starting the device mdevctl would simply walk the array, if the > > attribute key exists write the value(s). If a write fails or the > > attribute doesn't exist, remove the device and report error. Yes, I think it makes sense to fail the startup of a device where we cannot set all attributes to the requested values. > > > > I think it's easiest with jq to manipulate arrays by removing and > > inserting by index. Also if we end up with something like above, it's > > ambiguous if we reference the "attrX" key. So perhaps we add the > > following options to the modify command: > > > > --addattr=ATTRIBUTE --delattr --index=INDEX --value=VALUE1[,VALUE2] > > > > We could handle it like a stack, so if --index is not supplied, add to > > the end or remove from the end. If --index is provided, delete that > > index or add the attribute at that index. So if you had the above and > > wanted to remove Xvalue1 but keep the ordering, you'd do: > > > > --delattr --index=0 > > --addattr --index=0 --value=Xvalue2 > > > > Which should results in: > > > > "attrs": [ > > {"attrX": "Xvalue2"}, > > {"dir/attrY": "Yvalue1"}, > > {"attrX": "Xvalue3"} > > ] Modifying by index looks reasonable; I just sent a pull request to print the index of an attribute out as well, so it is easier to specify the right attribute to modify.

> > If we want to modify a running device, I'm thinking we probably want a > > new command and options --attr=ATTRIBUTE --value=VALUE might suffice. > > > > Do we need to support something like this for the 'start' command or > > should we leave that for simple devices and require a sequence of: > > > > # mdevctl define ... > > # mdevctl modify --addattr... > > ... > > # mdevctl start > > # mdevctl undefine > > > > This is effectively the long way to get a transient device. Otherwise > > we'd need to figure out how to have --attr --value appear multiple > > times on the start command line. Thanks, What do you think of a way to specify JSON for the attributes directly on the command line? Or would it be better to just edit the config files directly?

> This is now implemented, and yes you can specify '--addattr remove > --value 1' and mdevctl will immediately remove the device after it's > created (more power to the admin). Listing defined devices also lists Fun ;) > any attributes defined for easy inspection. It is also possible to > override the conversion of comma separated values into an array by > encoding and escaping the comma. It's a little cumbersome, but > possible in case a driver isn't fully on board with the one attribute, > one value rule of sysfs. Does this work for vfio-ap? I also still I do not have ap devices to actually test this with; but defining a device and adding attributes seems to work. > need to check if this allows an NVIDIA vGPU mdev to be configured such > that the framerate limiter can be automatically controlled. Thanks, > > Alex

On Fri, 28 Jun 2019 11:05:46 -0600 Alex Williamson <alex.williamson(a)redhat.com&gt; wrote: > On Fri, 28 Jun 2019 11:06:48 +0200 > Cornelia Huck <cohuck(a)redhat.com&gt; wrote: > > What do you think of a way to specify JSON for the attributes directly > > on the command line? Or would it be better to just edit the config > > files directly? > > Supplying json on the command like seems difficult, even doing so with > with jq requires escaping quotes. It's not a very friendly > experience. Maybe something more like how virsh allows snippets of xml > to be included, we could use jq to validate a json snippet provided > as a file and add it to the attributes... of course if we need to allow > libvirt to modify the json config files directly, the user could do > that as well. Is there a use case you're thinking of? Maybe we could > augment the 'list' command to take a --uuid and --dumpjson option and > the 'define' command to accept a --jsonfile. Maybe the 'start' command > could accept the same, so a transient device could define attributes > w/o excessive command line options. Thanks, > > Alex I was mostly thinking about complex configurations where writing a JSON config would be simpler than adding a lot of command line options. Something like dumping a JSON file and allowing to refer to a JSON file as you suggested could be useful; but then, those very complex use cases are probably already covered by editing the config file directly. Not sure if it is worth the effort; maybe just leave it as it is for now.