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

-- 
2.32.0

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

On 2/2/22 1:04 PM, Michal Prívozník wrote:
> 
> Laine, any thoughts?

I somehow thought this had already been pushed, so I was surprised when 
it showed up again :-/

I think the only issue I had before was that I thought the new unit 
tests were more testing the test setup than the actual code, but Dan 
convinced me otherwise.

So I'm fine with this change.

A newly discovered (but pre-existing, so non-consequential to these 
patches) problem associated with vlans is that we don't differentiate 
between "set vlan 0" and "don't set any vlan", which I hadn't even 
considered before, until this BZ came up:

  https://bugzilla.redhat.com/2035726

(The reporter is trying to use <tag id='-1'/> to "unset" the vlan tag 
for an interface)

On 2/3/22 15:45, Laine Stump wrote:
> On 2/2/22 1:04 PM, Michal Prívozník wrote:
>>
>> Laine, any thoughts?
> 
> I somehow thought this had already been pushed, so I was surprised when
> it showed up again :-/
> 
> I think the only issue I had before was that I thought the new unit
> tests were more testing the test setup than the actual code, but Dan
> convinced me otherwise.
> 
> So I'm fine with this change.
> 

Cool, pushed now.

> A newly discovered (but pre-existing, so non-consequential to these
> patches) problem associated with vlans is that we don't differentiate
> between "set vlan 0" and "don't set any vlan", which I hadn't even
> considered before, until this BZ came up:
> 
>  https://bugzilla.redhat.com/2035726
> 
> (The reporter is trying to use <tag id='-1'/> to "unset" the vlan tag
> for an interface)
> 

Ah, but the bug report is for openvswitch port profile, so that's doubly
unrelated :-)

Michal

Thanks again for the feedback - much appreciated!

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

On Thu, Feb 3, 2022 at 6:52 PM Michal Prívozník <mprivozn@redhat.com> wrote:
>
> On 2/3/22 15:45, Laine Stump wrote:
> > On 2/2/22 1:04 PM, Michal Prívozník wrote:
> >>
> >> Laine, any thoughts?
> >
> > I somehow thought this had already been pushed, so I was surprised when
> > it showed up again :-/
> >
> > I think the only issue I had before was that I thought the new unit
> > tests were more testing the test setup than the actual code, but Dan
> > convinced me otherwise.
> >
> > So I'm fine with this change.
> >
>
> Cool, pushed now.
>
> > A newly discovered (but pre-existing, so non-consequential to these
> > patches) problem associated with vlans is that we don't differentiate
> > between "set vlan 0" and "don't set any vlan", which I hadn't even
> > considered before, until this BZ came up:
> >
> >  https://bugzilla.redhat.com/2035726
> >
> > (The reporter is trying to use <tag id='-1'/> to "unset" the vlan tag
> > for an interface)
> >
>
> Ah, but the bug report is for openvswitch port profile, so that's doubly
> unrelated :-)
>
> Michal
>

On 2/3/22 10:52 AM, Michal Prívozník wrote:
> On 2/3/22 15:45, Laine Stump wrote:
>> On 2/2/22 1:04 PM, Michal Prívozník wrote:
>>>
>>> Laine, any thoughts?
>>
>> I somehow thought this had already been pushed, so I was surprised when
>> it showed up again :-/
>>
>> I think the only issue I had before was that I thought the new unit
>> tests were more testing the test setup than the actual code, but Dan
>> convinced me otherwise.
>>
>> So I'm fine with this change.
>>
> 
> Cool, pushed now.
> 
>> A newly discovered (but pre-existing, so non-consequential to these
>> patches) problem associated with vlans is that we don't differentiate
>> between "set vlan 0" and "don't set any vlan", which I hadn't even
>> considered before, until this BZ came up:
>>
>>   https://bugzilla.redhat.com/2035726
>>
>> (The reporter is trying to use <tag id='-1'/> to "unset" the vlan tag
>> for an interface)
>>
> 
> Ah, but the bug report is for openvswitch port profile, so that's doubly
> unrelated :-)

Yes and now. The original report was about openvswitch ports, but then 
the reporter also tried things with hostdev SRIOV devices and macvtap 
passthrough devices (with varying results). I'm sure that whatever is 
done to fix it will be deeply intertwined and complicated by the current 
topic (since the whole point of these patches is that smartnics 
want/need to just leave the entire vlan tag thing alone) :-)

Additionally, Dmitrii's change might make it possible to easily/simply 
fix the case of updating vlan tag for existing macvtap passthrough and 
hostdev SRIOV VFs (since we can now set the vlan tag without doing 
anything else). So for once there is an unintended *good* side effect!

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@redhat.com> 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
>

Squashed & auto-tested here:

https://gitlab.com/dmitriis/libvirt/-/commits/2021-ignore-eperm-for-vid0
https://gitlab.com/dmitriis/libvirt/-/pipelines/462532823


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

On Wed, Feb 2, 2022 at 11:10 PM Dmitrii Shcherbakov
<dmitrii.shcherbakov@canonical.com> wrote:
>
> 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@redhat.com> 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
> >