Re: [libvirt PATCH v8 0/3] Ignore EPERM on implicit clearing of VF VLAN ID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Michal,

No problem, thanks for coming back to re-review it, I also acknowledge
that it was late in the year and during the holiday season so things
got slower.

> and if you agree, I'd squash them in respective commits and merge.

I looked at the fixup commits - I agree with the changes and with
squashing them so it's a +1 from me, thanks a lot for doing this!

Best Regards,
Dmitrii Shcherbakov
LP/MM/oftc: dmitriis


On Wed, Feb 2, 2022 at 9:04 PM Michal Prívozník <mprivozn@xxxxxxxxxx> wrote:
>
> On 2/1/22 09:28, Dmitrii Shcherbakov wrote:
> > SmartNIC DPUs may not expose some privileged eswitch operations
> > to the hypervisor hosts. For example, this happens with Bluefield
> > devices running in the ECPF (default) mode [1] for security reasons. While
> > VF MAC address programming is possible via an RTM_SETLINK operation,
> > trying to set a VLAN ID in the same operation may fail with EPERM.
> >
> > Specifically for the mlx5 driver this behavior was altered in the Linux
> > kernel upstream [2] to avoid getting EPERM when trying to program VLAN 0.
> > This may also get backported to older downstream kernels (e.g. [3]).
> > However, Libvirt could potentially handle this case gracefully without
> > needing a specific kernel version or depending on a specific driver fix.
> >
> > In the kernel a relevant call chain may look like
> >
> > do_setlink -> do_setvfinfo -> dev->netdev_ops->set_vf_vlan
> >
> > which calls a driver-specific function like [4] eventually.
> >
> > The equivalent ip link commands below provide an illustration:
> >
> > 1. This will work without an error:
> >
> > sudo ip link set enp130s0f0 vf 2 mac de:ad:be:ef:ca:fe
> >
> > 2. Setting (or clearing) a VLAN will fail with EPERM:
> >
> > sudo ip link set enp130s0f0 vf 2 vlan 0
> > RTNETLINK answers: Operation not permitted
> >
> > 3. This is what Libvirt attempts to do today when trying to clear a
> >    VF VLAN at the same time as programming a VF MAC:
> >
> > sudo ip link set enp130s0f0 vf 2 vlan 0 mac de:ad:be:ef:ca:fe
> > RTNETLINK answers: Operation not permitted
> >
> > If setting an explicit VLAN ID results in an EPERM, clearing a VLAN
> > (setting a VLAN ID to 0) can be handled gracefully by ignoring the
> > EPERM error with the rationale being that if we cannot set this state
> > in the first place, we cannot clear it either.
> >
> > Thus, virNetDevSetVfConfig is split into two distinct functions. If
> > clearing a VLAN ID fails with EPERM when clearing is implicit, the
> > error is simply ignored. For explicit clearing EPERM is still a
> > fatal error.
> >
> > Both new functions rely virNetDevSendVfSetLinkRequest that implements
> > common functionality related to formatting a request, sending it and
> > handling error conditions and returns 0 or an error since in both cases
> > the payload is either NLMSG_DONE (no error) or NLMSG_ERROR where an
> > error message is needed by the caller to handle known cases
> > appropriately. This function allows the conditional code to be unit tested.
> >
> > An alternative to this could be providing a higher level control plane
> > mechanism that would provide metadata about a device being remotely
> > managed in which case Libvirt would avoid trying to set or clear a
> > VLAN ID. This would be more complicated since other software (like Nova
> > in the OpenStack case) would have to annotate every guest device with an
> > attribute indicating whether a device is remotely managed or not based
> > on operator provided configuration so that Libvirt can act on this and
> > avoid VLAN programming.
> >
> > https://gitlab.com/dmitriis/libvirt/-/pipelines/460293963
> >
> > v8 change:
> >
> > * Rebased on top of the latest changes to Libvirt;
> > * Added relevant upstream Linux and downstream kernel references to
> >   this cover letter.
> >
> > [1] https://docs.mellanox.com/display/BlueFieldSWv35111601/Modes+of+Operation#ModesofOperation-SmartNICmode
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7846665d3504812acaebf920d1141851379a7f37
> > [3] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1957753
> > [4] https://github.com/torvalds/linux/blob/v5.15/drivers/net/ethernet/mellanox/mlx5/core/esw/legacy.c#L427-L434
> >
> > Dmitrii Shcherbakov (3):
> >   Set VF MAC and VLAN ID in two different operations
> >   Allow VF vlanid to be passed as a pointer
> >   Ignore EPERM on implicit clearing of VF VLAN ID
> >
> >  NEWS.rst                    |  14 ++
> >  src/hypervisor/virhostdev.c |   4 +-
> >  src/libvirt_private.syms    |   7 +
> >  src/util/virnetdev.c        | 256 +++++++++++++++++++++++++-----------
> >  src/util/virnetdevpriv.h    |  44 +++++++
> >  tests/virnetdevtest.c       | 249 ++++++++++++++++++++++++++++++++++-
> >  6 files changed, 496 insertions(+), 78 deletions(-)
> >  create mode 100644 src/util/virnetdevpriv.h
> >
>
> Sorry for allowing this go to v8 without proper review. To make it up to
> you, here's my suggestion: problems I've raised in my review are easy to
> solve. I've uploaded the 'fixup' commits onto my gilab here:
>
>   https://gitlab.com/MichalPrivoznik/libvirt/-/commits/vf_vlan_id/
>
> and if you agree, I'd squash them in respective commits and merge.
>
> Laine, any thoughts?
>
> Michal
>





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux