[libvirt] [Fwd: [PATCH v9] add 802.1Qbh and 802.1Qbg handling]

[Arnd Bergmann]

On Thursday 27 May 2010, Stefan Berger wrote:
> > 
> > But this still goes down the IFLA_PORT_SELF route because you pass PORT_SELF_VF
> > even for nltarget_kernel==false, where it makes no sense.
> > Maybe make the above
> > 
> >       if (vf == PORT_SELF_VF && nltarget_kernel)
> > 
> 
> So I'll pass vf = 0 and be done with it? I don't find this check for
> nltarget_kernel particularly intuitive. Why should the function create
> different messages for a kernel driver versus a user space daemon? Is
> this a technology specific thing that would prevent this type of message
> from being sent. Obviously it does work for Scott's 802.1Qbh part.

The difference is that enic (and likely any other driver implementing
Qbg or Qbh in the firmware) has access to the underlying data channel.
IFLA_PORT_SELF essentially means that we ask a logical device to associate
itself with the switch, which is very unlike the case where we ask a master
device to associate a slave to the switch.

Neither the PORT_SELF nor the VF_PORTS list are exactly what we really
want, but the VF_PORTS stuff is closer. PORT_SELF does not work to start
with, because it does not allow to query information about more than one
VF.
VF_PORTS is a bit fishy because we don't actually have VFs but an arbitrary
number of software defined ports, but I think it's close enough.
We could also invent another list for software ports that are identified
by uuid instead of VF, but that would require defining attributes in the
kernel that are only used in user space.

> > > +    if (vf != PORT_SELF_VF) {
> > > +        /* end nesting of vfports */
> > > +        vfports->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)vfports;
> > > +    }
> > 
> > Here too.
> > 
> > > +    if (nltarget_kernel) {
> > > +        if (nlComm(nlm, &recvbuf, &recvbuflen) < 0)
> > > +            return -1;
> > > +    } else {
> > > +        if (nlCommWaitSuccess(nlm, RTMGRP_LINK, &recvbuf, &recvbuflen,
> > > +                              5 * MICROSEC_PER_SEC) < 0)
> > > +            return -1;
> > > +    }
> > 
> > I don't understand this part yet. Do we need this difference?
> 
> From my experience with experiments that I have made I can only say that
> we do need it. The sending part is a little different and the receiveing
> part needs to filter out noise and only pick the response to the request
> that was previously sent.

I'm even more confused now. Why should the response be different
from the response we get from the kernel? What's different on the
sending side other than the PID?
Also, what is the RTMGRP_LINK argument used for? I thought we don't
need multicast any more because we don't target kernel and lldpad
in the same message but only one of them.

> > > +    while (--repeats >= 0) {
> > > +        rc = link_dump(nltarget_kernel, NULL, ifindex, tb, &recvbuf);
> > > +        if (rc)
> > > +            goto err_exit;
> > > +        rc = getPortProfileStatus(tb, vf, &status);
> > > +        if (rc == 0) {
> > > +            if (status == PORT_PROFILE_RESPONSE_SUCCESS ||
> > > +                status == PORT_VDP_RESPONSE_SUCCESS) {
> > > +                break;
> > > +            } else if (status == PORT_PROFILE_RESPONSE_INPROGRESS) {
> > > +                // keep trying...
> > > +            } else {
> > > +                virReportSystemError(EINVAL,
> > > +                    _("error %d during port-profile setlink on ifindex %d"),
> > > +                    status, ifindex);
> > > +                rc = 1;
> > > +                break;
> > > +            }
> > 
> > Hmm, we seem to be missing an INPROGRESS status for Qbg. Any suggestions
> 
> You mean that's not defined in the (pre-)standard?

Yes, the Qbg wire protocol has no need for this, because the
messages are only sent after the state has changed, we never
see a VDP message with an incomplete status in there, so there
is no need to specify it in Qbg, but we need something in the netlink
protocol.

> > what we should return there? Should we possibly just leave out
> > IFLA_PORT_RESPONSE in order to signal INPROGRESS, as in not clear yet?
> 
> We want to be able to signal failure in case the switch setup failed so
> that we don't have a malfunctioning network interface where the user
> then doesn't know what to debug. In case of failure we simply wouldn't
> start the VM or hotplug the interface and return an indication that
> something went wrong during the negotiation with the switch.

So leaving out IFLA_PORT_RESPONSE would work for that, right?

The cases we need to care about are:

getPortProfileStatus returns without IFLA_PORT_RESPONSE -> keep trying
getPortProfileStatus returns success from IFLA_PORT_RESPONSE -> done
getPortProfileStatus returns failure from IFLA_PORT_RESPONSE -> give up

> > > +    rc = doPortProfileOpCommon(nltarget_kernel,
> > > +                               physdev_ifname, physdev_ifindex,
> > > +                               macaddr,
> > > +                               vlanid,
> > > +                               NULL,
> > > +                               &portVsi,
> > > +                               virtPort->u.virtPort8021Qbg.instanceID,
> > > +                               NULL,
> > > +                               PORT_SELF_VF,
> > > +                               op);
> > 
> > This is where we pass PORT_SELF_VF together with nltarget_kernel=false,
> > as mentioned above.
> > 
> 
> nltarget_kernel is in fact false here. So now if I change the
> PORT_SELF_VF to '0', then I suppose it will be ok? The vf in the netlink
> message to lldpad will then show 0 rather than PORT_SELF_VF (-1). I
> guess 0 wouldn't have the meaning of the 0-st virtual function, but
> counting would start at 1?

That's something we need to define now. You already put vf=0 into the
IFLA_VF_MAC and IFLA_VF_VLAN attributes, and you leave out the IFLA_PORT_VF
attribute for Qbg, which is probably the best approximation we can get.
I think we should just mandate this for the request, and let lldpad
decide on the fake vf number, which it returns to getPortProfileStatus.

	Arnd