Linux Virtualization - Jun 2018 - [libvirt] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net

On 2018?06?05? 09:41, Samudrala, Sridhar wrote:
> Ping on this patch now that the kernel patches are accepted into 
> davem's net-next tree.
> https://patchwork.ozlabs.org/cover/920005/
>
>
> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>> This feature bit can be used by hypervisor to indicate virtio_net 
>> device to
>> act as a standby for another device with the same MAC address.
>>
>> I tested this with a small change to the patch to mark the STANDBY 
>> feature 'true'
>> by default as i am using libvirt to start the VMs.
>> Is there a way to pass the newly added feature bit 'standby' to
qemu
>> via libvirt
>> XML file?
>>
Maybe you can try qemu command line passthrough:

https://libvirt.org/drvqemu.html#qemucommand

The patch looks good to me. Let's see if Michael have any comment.

Thanks
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala at intel.com>
>> ---
>> ? hw/net/virtio-net.c???????????????????????? | 2 ++
>> ? include/standard-headers/linux/virtio_net.h | 3 +++
>> ? 2 files changed, 5 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 90502fca7c..38b3140670 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>> ?????????????????????? true),
>> ????? DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, 
>> SPEED_UNKNOWN),
>> ????? DEFINE_PROP_STRING("duplex", VirtIONet,
net_conf.duplex_str),
>> +??? DEFINE_PROP_BIT64("standby", VirtIONet, host_features, 
>> VIRTIO_NET_F_STANDBY,
>> +????????????????????? false),
>> ????? DEFINE_PROP_END_OF_LIST(),
>> ? };
>> ? diff --git a/include/standard-headers/linux/virtio_net.h 
>> b/include/standard-headers/linux/virtio_net.h
>> index e9f255ea3f..01ec09684c 100644
>> --- a/include/standard-headers/linux/virtio_net.h
>> +++ b/include/standard-headers/linux/virtio_net.h
>> @@ -57,6 +57,9 @@
>> ?????????????????????? * Steering */
>> ? #define VIRTIO_NET_F_CTRL_MAC_ADDR 23??? /* Set MAC address */
>> ? +#define VIRTIO_NET_F_STANDBY????? 62??? /* Act as standby for 
>> another device
>> +???????????????????????????????????????? * with the same MAC.
>> +???????????????????????????????????????? */
>> ? #define VIRTIO_NET_F_SPEED_DUPLEX 63??? /* Device set linkspeed and 
>> duplex */
>> ? ? #ifndef VIRTIO_NET_NO_LEGACY
>

On 6/4/2018 7:06 PM, Jason Wang wrote:
>
>
> On 2018?06?05? 09:41, Samudrala, Sridhar wrote:
>> Ping on this patch now that the kernel patches are accepted into 
>> davem's net-next tree.
>> https://patchwork.ozlabs.org/cover/920005/
>>
>>
>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>>> This feature bit can be used by hypervisor to indicate virtio_net 
>>> device to
>>> act as a standby for another device with the same MAC address.
>>>
>>> I tested this with a small change to the patch to mark the STANDBY 
>>> feature 'true'
>>> by default as i am using libvirt to start the VMs.
>>> Is there a way to pass the newly added feature bit
'standby' to qemu
>>> via libvirt
>>> XML file?
>>>
>
> Maybe you can try qemu command line passthrough:
>
> https://libvirt.org/drvqemu.html#qemucommand
It looks like this can be used to pass command line arguments to qemu.
Is it possible to specify a virtio specific attribute via this method?

For ex: to say mrg_rxbuf is off we can add the following line to virtio
section of the domain xml file.
   <host mrg_rxbuf='off'/>

I think libvirt needs to be extended to to support the new 'standby'
attribute
via this mechanism.
Adding Liane Stump and libvirt to the CC list.

Michael,
Can we start with getting this patch into Qemu and an update to libvirt to
support the 'standby' feature so that this feature can be enabled via
some scripts/orchestration layer for now.

We could improve this solution by enhancing Qemu to do automatic management of
the
addition/deletion of the primary device based on feature negotiation as a later
patch.

>
> The patch looks good to me. Let's see if Michael have any comment.
>
> Thanks
>
>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala at
intel.com>
>>> ---
>>> ? hw/net/virtio-net.c???????????????????????? | 2 ++
>>> ? include/standard-headers/linux/virtio_net.h | 3 +++
>>> ? 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 90502fca7c..38b3140670 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>>> ?????????????????????? true),
>>> ????? DEFINE_PROP_INT32("speed", VirtIONet,
net_conf.speed,
>>> SPEED_UNKNOWN),
>>> ????? DEFINE_PROP_STRING("duplex", VirtIONet,
net_conf.duplex_str),
>>> +??? DEFINE_PROP_BIT64("standby", VirtIONet,
host_features,
>>> VIRTIO_NET_F_STANDBY,
>>> +????????????????????? false),
>>> ????? DEFINE_PROP_END_OF_LIST(),
>>> ? };
>>> ? diff --git a/include/standard-headers/linux/virtio_net.h 
>>> b/include/standard-headers/linux/virtio_net.h
>>> index e9f255ea3f..01ec09684c 100644
>>> --- a/include/standard-headers/linux/virtio_net.h
>>> +++ b/include/standard-headers/linux/virtio_net.h
>>> @@ -57,6 +57,9 @@
>>> ?????????????????????? * Steering */
>>> ? #define VIRTIO_NET_F_CTRL_MAC_ADDR 23??? /* Set MAC address */
>>> ? +#define VIRTIO_NET_F_STANDBY????? 62??? /* Act as standby for 
>>> another device
>>> +???????????????????????????????????????? * with the same MAC.
>>> +???????????????????????????????????????? */
>>> ? #define VIRTIO_NET_F_SPEED_DUPLEX 63??? /* Device set linkspeed 
>>> and duplex */
>>> ? ? #ifndef VIRTIO_NET_NO_LEGACY
>>
>

On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar
wrote:
> On 6/4/2018 7:06 PM, Jason Wang wrote:
> > 
> > 
> > On 2018?06?05? 09:41, Samudrala, Sridhar wrote:
> > > Ping on this patch now that the kernel patches are accepted into
> > > davem's net-next tree.
> > > https://patchwork.ozlabs.org/cover/920005/
> > > 
> > > 
> > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
> > > > This feature bit can be used by hypervisor to indicate
> > > > virtio_net device to
> > > > act as a standby for another device with the same MAC
address.
> > > > 
> > > > I tested this with a small change to the patch to mark the
> > > > STANDBY feature 'true'
> > > > by default as i am using libvirt to start the VMs.
> > > > Is there a way to pass the newly added feature bit
'standby' to
> > > > qemu via libvirt
> > > > XML file?
> > > > 
> > 
> > Maybe you can try qemu command line passthrough:
> > 
> > https://libvirt.org/drvqemu.html#qemucommand
> 
> It looks like this can be used to pass command line arguments to qemu.
> Is it possible to specify a virtio specific attribute via this method?
> 
> For ex: to say mrg_rxbuf is off we can add the following line to virtio
> section of the domain xml file.
>   <host mrg_rxbuf='off'/>
> 
> I think libvirt needs to be extended to to support the new
'standby' attribute
> via this mechanism.
> Adding Liane Stump and libvirt to the CC list.
> 
> Michael,
> Can we start with getting this patch into Qemu and an update to libvirt to
> support the 'standby' feature so that this feature can be enabled
via
> some scripts/orchestration layer for now.
Unfortunately we don't give the hypothetical orchestration layer
enough info about driver being bound, so it does not know
when is it safe to add a primary.  And a similar issue around reset.

We could add events for that which would be a small patch,
but the issue then is that guest can trigger a storm of
these events.
> We could improve this solution by enhancing Qemu to do automatic management
of the
> addition/deletion of the primary device based on feature negotiation as a
later patch.
I'm not sure what the point of the back and forth would be though.
Typically after people invest in a specific config and get it working,
it's very hard to move them to another solution.
> 
> > 
> > The patch looks good to me. Let's see if Michael have any comment.
> > 
> > Thanks
> > 
> > > > Signed-off-by: Sridhar Samudrala <sridhar.samudrala at
intel.com>
> > > > ---
> > > > ? hw/net/virtio-net.c???????????????????????? | 2 ++
> > > > ? include/standard-headers/linux/virtio_net.h | 3 +++
> > > > ? 2 files changed, 5 insertions(+)
> > > > 
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 90502fca7c..38b3140670 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -2198,6 +2198,8 @@ static Property
virtio_net_properties[] = {
> > > > ?????????????????????? true),
> > > > ????? DEFINE_PROP_INT32("speed", VirtIONet,
net_conf.speed,
> > > > SPEED_UNKNOWN),
> > > > ????? DEFINE_PROP_STRING("duplex", VirtIONet,
net_conf.duplex_str),
> > > > +??? DEFINE_PROP_BIT64("standby", VirtIONet,
host_features,
> > > > VIRTIO_NET_F_STANDBY,
> > > > +????????????????????? false),
> > > > ????? DEFINE_PROP_END_OF_LIST(),
> > > > ? };
> > > > ? diff --git a/include/standard-headers/linux/virtio_net.h
> > > > b/include/standard-headers/linux/virtio_net.h
> > > > index e9f255ea3f..01ec09684c 100644
> > > > --- a/include/standard-headers/linux/virtio_net.h
> > > > +++ b/include/standard-headers/linux/virtio_net.h
> > > > @@ -57,6 +57,9 @@
> > > > ?????????????????????? * Steering */
> > > > ? #define VIRTIO_NET_F_CTRL_MAC_ADDR 23??? /* Set MAC
address */
> > > > ? +#define VIRTIO_NET_F_STANDBY????? 62??? /* Act as standby
for
> > > > another device
> > > > +???????????????????????????????????????? * with the same
MAC.
> > > > +???????????????????????????????????????? */
> > > > ? #define VIRTIO_NET_F_SPEED_DUPLEX 63??? /* Device set
> > > > linkspeed and duplex */
> > > > ? ? #ifndef VIRTIO_NET_NO_LEGACY
> > > 
> >

On 6/6/2018 11:52 AM, J?n Tomko wrote:
> On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote:
>> On 6/4/2018 7:06 PM, Jason Wang wrote:
>>>
>>>
>>> On 2018?06?05? 09:41, Samudrala, Sridhar wrote:
>>>> Ping on this patch now that the kernel patches are accepted
into
>>>> davem's net-next tree.
>>>> https://patchwork.ozlabs.org/cover/920005/
>>>>
>>>>
>>>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>>>>> This feature bit can be used by hypervisor to indicate
virtio_net
>>>>> device to
>>>>> act as a standby for another device with the same MAC
address.
>>>>>
>>>>> I tested this with a small change to the patch to mark the
STANDBY
>>>>> feature 'true'
>>>>> by default as i am using libvirt to start the VMs.
>>>>> Is there a way to pass the newly added feature bit
'standby' to qemu
>>>>> via libvirt
>>>>> XML file?
>>>>>
>>>
>>> Maybe you can try qemu command line passthrough:
>>>
>>> https://libvirt.org/drvqemu.html#qemucommand
>>
>> It looks like this can be used to pass command line arguments to qemu.
>> Is it possible to specify a virtio specific attribute via this method?
>>
>
> Yes, for testing purposes you should be able to do this via using
QEMU's
> -set command line argument:
> http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html
>
> i.e.:
>
> <domain type='kvm' 
> xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
> ?...
> ?<qemu:commandline>
> ?? <qemu:arg value='-set'/>
> ?? <qemu:arg value='device.net0.standby=on'/>
> ?</qemu:commandline>
> </domain>
Thanks. Will try this.
>
>> For ex: to say mrg_rxbuf is off we can add the following line to virtio
>> section of the domain xml file.
>> ? <host mrg_rxbuf='off'/>
>>
>> I think libvirt needs to be extended to to support the new
'standby'
>> attribute
>> via this mechanism.
>> Adding Liane Stump and libvirt to the CC list.
>
> *Laine
>
>>
>> Michael,
>> Can we start with getting this patch into Qemu and an update to 
>> libvirt to
>> support the 'standby' feature so that this feature can be
enabled via
>> some scripts/orchestration layer for now.
>>
>> We could improve this solution by enhancing Qemu to do automatic 
>> management of the
>> addition/deletion of the primary device based on feature negotiation 
>> as a later patch.
>>
>
> If that means the libvirt attribute would no longer be needed, I don't
> see the reason to add it to libvirt in the first place.
I think we still need this attribute supported via libvirt so that a user/admin
can enable this feature via domain XML specification.