On 11/19/2014 05:39 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 19, 2014 at 05:33:26PM +0800, Jason Wang wrote:
>> On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote:
>>> On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
>>>> This patch validates feature dependencies during probe and fail the probing
>>>> if a dependency is missed. This fixes the issues of hitting BUG()
>>>> when qemu fails to advertise features correctly. One example is booting
>>>> guest with ctrl_vq=off through qemu.
>>>>
>>>> Cc: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
>>>> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
>>>> Cc: Cornelia Huck <cornelia.huck@xxxxxxxxxx>
>>>> Cc: Wanlong Gao <gaowanlong@xxxxxxxxxxxxxx>
>>>> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
>>>> ---
>>>> drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 93 insertions(+)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index ec2a8b4..4a0ad46 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>>>> };
>>>> #endif
>>>>
>>>> +static int virtnet_validate_features(struct virtio_device *dev,
>>>> + unsigned int *table,
>>>> + int table_size,
>>>> + unsigned int feature)
>>>> +{
>>>> + int i;
>>>> +
>>>> + if (!virtio_has_feature(dev, feature)) {
>>>> + for (i = 0; i < table_size; i++) {
>>>> + unsigned int f = table[i];
>>>> +
>>>> + if (virtio_has_feature(dev, f)) {
>>>> + dev_err(&dev->dev,
>>>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
>>> This line's way too long.
>> Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be
>> split)
[...]
>>>> +
>>>> static int virtnet_probe(struct virtio_device *vdev)
>>>> {
>>>> int i, err;
>>>> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>> struct virtnet_info *vi;
>>>> u16 max_queue_pairs;
>>>>
>>>> + err = virtnet_check_features(vdev);
>>>> + if (err)
>>>> + return -EINVAL;
>>>> +
>>>> /* Find if host supports multiqueue virtio_net device */
>>>> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>>>> struct virtio_net_config,
>>> The API seems too complex, and you still had to open-code ECN logic.
>>> Just open-code most of it.
>> Yes, the ECN could be done through the same way as ctrl_vq did.
>>
>> How about move those checking into virtio core by just letting device
>> export its dependency table?
> So far we only have this for net, let's not add
> one-off APIs.
>
>>> You can use a helper macro to output a
>>> friendly message without code duplication.
>>> For example like the below (completely untested)?
>>>
>>>
>>> I would also like to split things: dependencies on
>>> VIRTIO_NET_F_CTRL_VQ might go into this kernel,
>>> since they help fix BUG.
>>>
>>> Others should wait since they don't fix any crashes, and there's a small
>>> chance of a regression for some hypervisor in the field.
>> Probably ok but not sure, since the rest features are all related to
>> csum and offloading, we are in fact depends on network core to fix them.
> Well it does fix them so ... there's no bug, is there?
>
No.
>>> -->
>>>
>>> virtio-net: friendlier handling of misconfigured hypervisors
>>>
>>> We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
>>> is not set but one of features depending on it is.
>>> That's not a friendly way to report errors to
>>> hypervisors.
>>> Let's check, and fail probe instead.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>>>
>>> ---
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 26e1330..7a7d1a3 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>>> };
>>> #endif
>>>
>>> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
>>> + const char *fname)
>>> +{
>>> + if (!virtio_has_feature(vdev, fbit))
>>> + return false;
>>> +
>>> + dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
>>> + fbit, fname);
>>> +
>>> + return true;
>>> +}
>>> +
>>> +#define VIRTNET_FAIL_ON(vdev, fbit) \
>>> + __virtnet_fail_on_feature(vdev, fbit, #fbit)
>>> +
>>> static int virtnet_probe(struct virtio_device *vdev)
>>> {
>>> int i, err;
>>> @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>> struct virtnet_info *vi;
>>> u16 max_queue_pairs;
>>>
>>> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
>>> + (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
>>> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
>>> + return -EINVAL;
>>> +
>>> /* Find if host supports multiqueue virtio_net device */
>>> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>>> struct virtio_net_config,
>>>
>> Patch looks good, but consider we may check more dependencies in the
>> future, may need a helper instead. Or just use this and switch to
>> dependency table in 3.19.
> Pls note this is just pseudo-code - I didn't even compile it.
> If you want something like this merged, go ahead and
> post it, probably addressing Cornelia's request to
> print the dependency too. Maybe:
Ok, will post v3.
>
>>> (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ, "ctrl_vq") ||
>>> VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR, "ctrl_vq")))
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
