Re: [libvirt] [PATCH 3/4] virsh-domain: update attach-interface to support type=hostdev

On 10/21/2015 08:22 AM, Pavel Hrdina wrote: > Adding this feature will allow users to easily attach a hostdev network > interface using PCI passthrough. > > The interface can be attached using --type=hostdev and PCI address or > network device name as --source. This command also allows you to tell, > whether the interface should be managed and to choose a assignment > driver. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=997561 I think the PCI address version of this command is fairly straightforward, so it can and should go in. But I've been thinking more about the "netdev name" version since our exchange in the BZ, and even had a very long treatise prepared defending my position from there, but then I decided to look at it all again from the beginning and came to the conclusion that we are *both* wrong :-) The main aim here is convenience, and while I still have the position that if we're going to make it more convenient, we should make it more convenient at the XML level so that all consumers of libvirt can take advantage without needing a ton of extra code, I also have realized that specifying the netdev name is not very convenient anyway. Why? * Keep in mind that <interface type='hostdev'> will only work with SRIOV VFs (because you can't set the MAC address of a normal netdev from the host and have that MAC address persist through the guest driver's device init). * So any device that you assign in this way is a VF. * A user will know which PF ("Physical Function", but from their point of view it's "the physical port used for the connection") they want a VF from; they don't care which VF (they're all functionally equivalent), and anyway the standard utilities don't even tell you the netdev names of the VFs associated with a particular PF. So it's not that simple to learn the netdev name of the VF you want to use: * virsh nodedev-list --tree doesn't group the VFs under their PF (because its hierarchy is according to the PCI bus layout, which has all the PFs and VFs at the same level) * virsh nodedev-dumpxml for the PF device shows the VFs' PCI addresses, NOT their netdev names. <capability type='virt_functions'> <address domain='0x0000' bus='0x02' slot='0x10' function='0x0'/> <address domain='0x0000' bus='0x02' slot='0x10' function='0x2'/> <address domain='0x0000' bus='0x02' slot='0x10' function='0x4'/> <address domain='0x0000' bus='0x02' slot='0x10' function='0x6'/> <address domain='0x0000' bus='0x02' slot='0x11' function='0x0'/> <address domain='0x0000' bus='0x02' slot='0x11' function='0x2'/> <address domain='0x0000' bus='0x02' slot='0x11' function='0x4'/> (Note the leading "0x" on these values :-P) * "ip link show" lists the VFs directly under their PF, but shows the VF#, not the netdev name 11: p4p2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000 link/ether 90:e2:ba:02:22:01 brd ff:ff:ff:ff:ff:ff vf 0 MAC 00:00:00:00:00:00, spoof checking on, link-state auto vf 1 MAC 00:00:00:00:00:00, spoof checking on, link-state auto vf 2 MAC 00:00:00:00:00:00, spoof checking on, link-state auto vf 3 MAC 00:00:00:00:00:00, spoof checking on, link-state auto vf 4 MAC 00:00:00:00:00:00, spoof checking on, link-state auto vf 5 MAC 00:00:00:00:00:00, spoof checking on, link-state auto vf 6 MAC 00:00:00:00:00:00, spoof checking on, link-state auto So if there's not a simple way to get a list of the netdev names of the VFs for a PF, then what convenience is gained by allowing use of netdev name? What exactly was I thinking when I wrote comment 1 in that BZ??? What *could* be useful would be the ability to specify the PF name and VF# of the device you want to assign, e.g.: "<source pf='p4p2' vf='3'/>", since that's info you can easily get from "ip link show". Anymore, though, I don't even know how useful *that* is, since you can already get the PCI addresses of the VFs from the output of virsh nodedev-dumpxml (in almost exactly the format you need - just add "type='pci'". (I'm now even wondering if I misunderstood what the original reporter was asking for, but unfortunately it's not possible to ask him, because his account has been closed :-( Thinking back to what he said - there is a *different* place where it's common to know the netdev name and want to translate it into a PCI address - when you are assigning a *non-SRIOV* ethernet device using plain <hostdev> (not <interface type='hostdev'>, which only works for SRIOV VFs). In this case you probably know the netdev name but not the PCI address. So for this case a translation from netdev name to PCI address would make sense, and here I would agree that the translation should happen in virsh rather than the generic <hostdev> XML understanding what a netdev name is (contrary to <interface type='hostdev'>, where it is well established that the device you're assigning is a network device, and there are already "netdev-y" config attributes, up until now <hostdev> has not contained any config items specific to a particular type of PCI device, and I don't think it should have any added)). TL;DR of my opinion: 1) this patch minus the netdev-->PCI address translation is good

2) we don't need the netdev-->PCI translation for <interface type='hostdev'>; it's just extra code for (in my newly revised opinion) no gain.

3) a netdev-->PCI translation in virsh that creates a plain <hostdev> might be useful (hmm, maybe keep (2) as you have it, with an additional check for

4) adding <source pf='ethX' vf='n'/> support to <interface type='hostdev'> might be useful.

How much sense does any of that make? > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > tools/virsh-domain.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 86 insertions(+), 4 deletions(-) > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index e8503ec..b124441 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -56,6 +56,7 @@ > #include "virtime.h" > #include "virtypedparam.h" > #include "virxml.h" > +#include "virsh-nodedev.h" > > /* Gnulib doesn't guarantee SA_SIGINFO support. */ > #ifndef SA_SIGINFO > @@ -866,6 +867,14 @@ static const vshCmdOptDef opts_attach_interface[] = { > .type = VSH_OT_BOOL, > .help = N_("print XML document rather than attach the interface") > }, > + {.name = "managed", > + .type = VSH_OT_BOOL, > + .help = N_("set the interface to be managed by libvirt") Maybe a more descriptive help - "have libvirt automatically manage detach/attach of device from host driver" (or something like that; I know that's too long).

> + }, > + {.name = "driver", > + .type = VSH_OT_STRING, > + .help = N_("set driver for hostdev interface, default is 'kvm'") Actually the default behavior depends on what is available on the host - if only one of legacy-kvm and vfio is available, then that is the one used, but if both are available, then vfio is used (so vfio is closer to being "default" than kvm). The kvm method of assigning devices is deprecated, and it has been completely disabled in some distros (definitely RHEL7 and CentOS7, not sure about Fedora). These days manually specifying which driver to use is mostly pointless; I vaguely recall that it was initially added because when VFIO was first added some were nervous about not having a simple way to fallback to old behavior if something went wrong while using VFIO; those days are long gone though, and I can't think of a situation where anyone would want/need to specify which driver to use (i.e. I'm not sure we even need to expose this option in virsh; it may confuse more than help).