Re: [PATCH v3] virtio_console: Introduce an ID allocator for virtual console numbers

2022-11-22 Thread Thomas Huth 

On 22/11/2022 14.46, Cédric Le Goater wrote:

When a virtio console port is initialized, it is registered as an hvc
console using a virtual console number. If a KVM guest is started with
multiple virtio console devices, the same vtermno (or virtual console
number) can be used to allocate different hvc consoles, which leads to
various communication problems later on.

This is also reported in debugfs :

   # grep vtermno /sys/kernel/debug/virtio-ports/*
   /sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1
   /sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1
   /sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2
   /sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3

Replace the next_vtermno global with an ID allocator and start the
allocation at 1 as it is today. Also recycle IDs when a console port
is removed.

Signed-off-by: Cédric Le Goater 
---

  Changes in v3:
  - made use of ida_alloc_min()
  - free ID in case of error
  
  Changes in v2:

  - introduced an ID allocator

  drivers/char/virtio_console.c | 26 +++---
  1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9fa3c76a267f..6a821118d553 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -48,22 +49,11 @@ struct ports_driver_data {
/* List of all the devices we're handling */
struct list_head portdevs;
  
-	/*

-* This is used to keep track of the number of hvc consoles
-* spawned by this driver.  This number is given as the first
-* argument to hvc_alloc().  To correctly map an initial
-* console spawned via hvc_instantiate to the console being
-* hooked up via hvc_alloc, we need to pass the same vtermno.
-*
-* We also just assume the first console being initialised was
-* the first one that got used as the initial console.
-*/
-   unsigned int next_vtermno;
-
/* All the console devices handled by this driver */
struct list_head consoles;
  };
-static struct ports_driver_data pdrvdata = { .next_vtermno = 1};
+
+static struct ports_driver_data pdrvdata;
  
  static DEFINE_SPINLOCK(pdrvdata_lock);

  static DECLARE_COMPLETION(early_console_added);
@@ -89,6 +79,8 @@ struct console {
u32 vtermno;
  };
  
+static DEFINE_IDA(vtermno_ida);

+
  struct port_buffer {
char *buf;
  
@@ -1244,18 +1236,21 @@ static int init_port_console(struct port *port)

 * pointers.  The final argument is the output buffer size: we
 * can do any size, so we put PAGE_SIZE here.
 */
-   port->cons.vtermno = pdrvdata.next_vtermno;
+   ret = ida_alloc_min(_ida, 1, GFP_KERNEL);
+   if (ret < 0)
+   return ret;
  
+	port->cons.vtermno = ret;

port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, _ops, PAGE_SIZE);
if (IS_ERR(port->cons.hvc)) {
ret = PTR_ERR(port->cons.hvc);
dev_err(port->dev,
"error %d allocating hvc for port\n", ret);
port->cons.hvc = NULL;
+   ida_free(_ida, port->cons.vtermno);
return ret;
}
spin_lock_irq(_lock);
-   pdrvdata.next_vtermno++;
list_add_tail(>cons.list, );
spin_unlock_irq(_lock);
port->guest_connected = true;
@@ -1532,6 +1527,7 @@ static void unplug_port(struct port *port)
list_del(>cons.list);
spin_unlock_irq(_lock);
hvc_remove(port->cons.hvc);
+   ida_free(_ida, port->cons.vtermno);
    }
  
  	remove_port_data(port);


Reviewed-by: Thomas Huth 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio_console: Introduce an ID allocator for virtual console numbers

2022-11-22 Thread Thomas Huth 

On 14/11/2022 18.38, Cédric Le Goater wrote:

When a virtio console port is initialized, it is registered as an hvc
console using a virtual console number. If a KVM guest is started with
multiple virtio console devices, the same vtermno (or virtual console
number) can be used to allocate different hvc consoles, which leads to
various communication problems later on.

This is also reported in debugfs :

   # grep vtermno /sys/kernel/debug/virtio-ports/*
   /sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1
   /sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1
   /sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2
   /sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3

Replace the next_vtermno global with an ID allocator and start the
allocation at 1 as it is today. Also recycle IDs when a console port
is removed.


Sounds like a good idea!


@@ -1244,8 +1236,11 @@ static int init_port_console(struct port *port)
 * pointers.  The final argument is the output buffer size: we
 * can do any size, so we put PAGE_SIZE here.
 */
-   port->cons.vtermno = pdrvdata.next_vtermno;
+   ret = ida_alloc_range(_ida, 1, ~0, GFP_KERNEL);


Just cosmetics: I think you could use ida_alloc_min() instead.


+   if (ret < 0)
+   return ret;
  
+	port->cons.vtermno = ret;

port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, _ops, PAGE_SIZE);
if (IS_ERR(port->cons.hvc)) {
ret = PTR_ERR(port->cons.hvc);


What if this if (IS_ERR()) error happens? The code seems to return early in 
this case - shouldn't the ID be freed again in this case?


 Thomas

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 03/10] virtio/s390: enable packed ring

2019-05-05 Thread Thomas Huth 
On 03/05/2019 11.44, Cornelia Huck wrote:
> On Fri, 26 Apr 2019 20:32:38 +0200
> Halil Pasic  wrote:
> 
>> Nothing precludes to accepting  VIRTIO_F_RING_PACKED any more.
> 
> "precludes us from accepting"
> 
>>
>> Signed-off-by: Halil Pasic 
>> ---
>>  drivers/s390/virtio/virtio_ccw.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/virtio/virtio_ccw.c 
>> b/drivers/s390/virtio/virtio_ccw.c
>> index 42832a164546..6d989c360f38 100644
>> --- a/drivers/s390/virtio/virtio_ccw.c
>> +++ b/drivers/s390/virtio/virtio_ccw.c
>> @@ -773,10 +773,8 @@ static u64 virtio_ccw_get_features(struct virtio_device 
>> *vdev)
>>  static void ccw_transport_features(struct virtio_device *vdev)
>>  {
>>  /*
>> - * There shouldn't be anything that precludes supporting packed.
>> - * TODO: Remove the limitation after having another look into this.
>> + * Currently nothing to do here.
>>   */
>> -__virtio_clear_bit(vdev, VIRTIO_F_RING_PACKED);
>>  }
>>  
>>  static int virtio_ccw_finalize_features(struct virtio_device *vdev)
> 
> Not sure whether we should merge this into the previous patch instead.

In case you respin, I'd vote for squashing this into the previous patch
instead, especially since you've just added the comment in that patch.

Also, what about removing that function now completely? It's a static
function and only called once in this file, so IMHO it does not make
much sense to keep an empty function around. Or do you plan to add new
code here in the near future?

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio/s390: fixup for implement PM operations for virtio_ccw

2017-12-18 Thread Thomas Huth 
On 18.12.2017 09:37, Christian Borntraeger wrote:
> We need to disable the pm callbacks if CONFIG_PM is not set.
> 
> Signed-off-by: Christian Borntraeger 
> ---
> Cornelia, you might want to squash this into the original commit.
> 
>  drivers/s390/virtio/virtio_ccw.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/s390/virtio/virtio_ccw.c 
> b/drivers/s390/virtio/virtio_ccw.c
> index 330b3fa3430a..985184ebda45 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -1315,6 +1315,7 @@ static struct ccw_device_id virtio_ids[] = {
>   {},
>  };
>  
> +#ifdef CONFIG_PM_SLEEP
>  static int virtio_ccw_freeze(struct ccw_device *cdev)
>  {
>   struct virtio_ccw_device *vcdev = dev_get_drvdata(>dev);
> @@ -1333,6 +1334,7 @@ static int virtio_ccw_restore(struct ccw_device *cdev)
>  
>   return virtio_device_restore(>vdev);
>  }
> +#endif
>  
>  static struct ccw_driver virtio_ccw_driver = {
>   .driver = {
> @@ -1346,9 +1348,11 @@ static struct ccw_driver virtio_ccw_driver = {
>   .set_online = virtio_ccw_online,
>   .notify = virtio_ccw_cio_notify,
>   .int_class = IRQIO_VIR,
> +#ifdef CONFIG_PM_SLEEP
>   .freeze = virtio_ccw_freeze,
>   .thaw = virtio_ccw_restore,
>   .restore = virtio_ccw_restore,
> +#endif
>  };

Some other drivers rather seem to test CONFIG_HIBERNATE_CALLBACKS ...
would that be a more appropriate config flag here?

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] drivers/s390/virtio: Remove the old KVM virtio transport

2017-10-26 Thread Thomas Huth 
There is no recent user space application available anymore which still
supports this old virtio transport. Additionally, commit 3b2fbb3f06ef
("virtio/s390: deprecate old transport") introduced a deprecation message
in the driver, and apparently nobody complained so far that it is still
required. So let's simply remove it.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 arch/s390/Kconfig|  13 -
 drivers/s390/virtio/Makefile |   3 -
 drivers/s390/virtio/kvm_virtio.c | 515 ---
 3 files changed, 531 deletions(-)
 delete mode 100644 drivers/s390/virtio/kvm_virtio.c

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 48af970..a0a40dd 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -929,17 +929,4 @@ config S390_GUEST
  Select this option if you want to run the kernel as a guest under
  the KVM hypervisor.
 
-config S390_GUEST_OLD_TRANSPORT
-   def_bool y
-   prompt "Guest support for old s390 virtio transport (DEPRECATED)"
-   depends on S390_GUEST
-   help
- Enable this option to add support for the old s390-virtio
- transport (i.e. virtio devices NOT based on virtio-ccw). This
- type of virtio devices is only available on the experimental
- kuli userspace or with old (< 2.6) qemu. If you are running
- with a modern version of qemu (which supports virtio-ccw since
- 1.4 and uses it by default since version 2.4), you probably won't
- need this.
-
 endmenu
diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
index df40692..d9942e0 100644
--- a/drivers/s390/virtio/Makefile
+++ b/drivers/s390/virtio/Makefile
@@ -7,7 +7,4 @@
 # as published by the Free Software Foundation.
 
 s390-virtio-objs := virtio_ccw.o
-ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
-s390-virtio-objs += kvm_virtio.o
-endif
 obj-$(CONFIG_S390_GUEST) += $(s390-virtio-objs)
diff --git a/drivers/s390/virtio/kvm_virtio.c b/drivers/s390/virtio/kvm_virtio.c
deleted file mode 100644
index a99d09a..000
--- a/drivers/s390/virtio/kvm_virtio.c
+++ /dev/null
@@ -1,515 +0,0 @@
-/*
- * virtio for kvm on s390
- *
- * Copyright IBM Corp. 2008
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License (version 2 only)
- * as published by the Free Software Foundation.
- *
- *Author(s): Christian Borntraeger <borntrae...@de.ibm.com>
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#define VIRTIO_SUBCODE_64 0x0D00
-
-/*
- * The pointer to our (page) of device descriptions.
- */
-static void *kvm_devices;
-static struct work_struct hotplug_work;
-
-struct kvm_device {
-   struct virtio_device vdev;
-   struct kvm_device_desc *desc;
-};
-
-#define to_kvmdev(vd) container_of(vd, struct kvm_device, vdev)
-
-/*
- * memory layout:
- * - kvm_device_descriptor
- *struct kvm_device_desc
- * - configuration
- *struct kvm_vqconfig
- * - feature bits
- * - config space
- */
-static struct kvm_vqconfig *kvm_vq_config(const struct kvm_device_desc *desc)
-{
-   return (struct kvm_vqconfig *)(desc + 1);
-}
-
-static u8 *kvm_vq_features(const struct kvm_device_desc *desc)
-{
-   return (u8 *)(kvm_vq_config(desc) + desc->num_vq);
-}
-
-static u8 *kvm_vq_configspace(const struct kvm_device_desc *desc)
-{
-   return kvm_vq_features(desc) + desc->feature_len * 2;
-}
-
-/*
- * The total size of the config page used by this device (incl. desc)
- */
-static unsigned desc_size(const struct kvm_device_desc *desc)
-{
-   return sizeof(*desc)
-   + desc->num_vq * sizeof(struct kvm_vqconfig)
-   + desc->feature_len * 2
-   + desc->config_len;
-}
-
-/* This gets the device's feature bits. */
-static u64 kvm_get_features(struct virtio_device *vdev)
-{
-   unsigned int i;
-   u32 features = 0;
-   struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
-   u8 *in_features = kvm_vq_features(desc);
-
-   for (i = 0; i < min(desc->feature_len * 8, 32); i++)
-   if (in_features[i / 8] & (1 << (i % 8)))
-   features |= (1 << i);
-   return features;
-}
-
-static int kvm_finalize_features(struct virtio_device *vdev)
-{
-   unsigned int i, bits;
-   struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
-   /* Second half of bitmap is features we accept. */
-   u8 *out_features = kvm_vq_features(desc) + desc->feature_len;
-
-   /* Give virtio_ring a chance to accept features. */
-   vring_transport_features(vdev);
-
-   /* Make sure we don't have any features > 32 bits! */
-   BUG_ON((u32)vdev->features != vdev->features);
-
-   memset(out

Re: [PATCH] drivers/s390/virtio: Remove the old KVM virtio transport

2017-10-26 Thread Thomas Huth 
On 27.09.2017 14:55, Heiko Carstens wrote:
> On Wed, Sep 27, 2017 at 01:42:01PM +0200, Thomas Huth wrote:
>> diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
>> index df40692..d9942e0 100644
>> --- a/drivers/s390/virtio/Makefile
>> +++ b/drivers/s390/virtio/Makefile
>> @@ -7,7 +7,4 @@
>>  # as published by the Free Software Foundation.
>>
>>  s390-virtio-objs := virtio_ccw.o
>> -ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
>> -s390-virtio-objs += kvm_virtio.o
>> -endif
>>  obj-$(CONFIG_S390_GUEST) += $(s390-virtio-objs)
> 
> Would you mind to simplify the Makefile to just the single line
> 
> obj-$(CONFIG_S390_GUEST) += virtio_ccw.o
> 
> while you are touching it anyway?

Sure ... I'll send a v2 ... or do you rather want to fix it when picking
up the patch?

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] KVM: s390: Disable CONFIG_S390_GUEST_OLD_TRANSPORT by default

2017-10-26 Thread Thomas Huth 
On 26.09.2017 12:47, Heiko Carstens wrote:
> On Tue, Sep 26, 2017 at 12:41:41PM +0200, Christian Borntraeger wrote:
>>
>>
>> On 09/26/2017 12:40 PM, Heiko Carstens wrote:
>>> On Mon, Sep 25, 2017 at 08:37:36PM +0200, Christian Borntraeger wrote:
>>>>
>>>> On 09/25/2017 07:54 PM, Halil Pasic wrote:
>>>>>
>>>>>
>>>>> On 09/25/2017 04:45 PM, Thomas Huth wrote:
>>>>>> There is no recent user space application available anymore which still
>>>>>> supports this old virtio transport, so let's disable this by default.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>>>>
>>>>> I don't have any objections, but there may be something I'm not aware of.
>>>>> Let's see what Connie says. From my side it's ack.
>>>>>
>>>>> Via whom is this supposed to go in? Looking at the MAINTAINERS, I would
>>>>> say Martin or Heiko but I don't see them among the recipients.
>>>>
>>>> FWIW as the original author of that transport
>>>> Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
>>>>
>>>> I can pick this up for Martins/Heikos tree if you want.
>>>
>>> When will this code be removed?
>>>
>>> When the config option was initially added Conny said it should survive
>>> "probably two kernel releases or so, depending on feedback".
>>> It was merged for v4.8. Now we are five kernel releases later...
>>
>> Lets wait for one release and then get rid of it?
> 
> So it's going to be removed with the next merge window.
> Where is the patch? ;)

Hmm, so far the code was always enabled by default, so in the unlikely
case that somebody is still using it, they might not have noticed that
this is marked as deprecated. So maybe it's better to keep the code for
two more released, but disable it by default, so that people have time
to realize that it is going away? ... just my 0.02 € ... but if you
prefer, I can also write a patch that removes it immediately instead.

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] KVM: s390: Disable CONFIG_S390_GUEST_OLD_TRANSPORT by default

2017-10-26 Thread Thomas Huth 
There is no recent user space application available anymore which still
supports this old virtio transport, so let's disable this by default.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 arch/s390/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 48af970..923bf04 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -930,7 +930,7 @@ config S390_GUEST
  the KVM hypervisor.
 
 config S390_GUEST_OLD_TRANSPORT
-   def_bool y
+   def_bool n
prompt "Guest support for old s390 virtio transport (DEPRECATED)"
depends on S390_GUEST
help
-- 
1.8.3.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] KVM: s390: Disable CONFIG_S390_GUEST_OLD_TRANSPORT by default

2017-10-26 Thread Thomas Huth 
On 25.09.2017 20:37, Christian Borntraeger wrote:
> 
> On 09/25/2017 07:54 PM, Halil Pasic wrote:
>>
>>
>> On 09/25/2017 04:45 PM, Thomas Huth wrote:
>>> There is no recent user space application available anymore which still
>>> supports this old virtio transport, so let's disable this by default.
>>>
>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>
>> I don't have any objections, but there may be something I'm not aware of.
>> Let's see what Connie says. From my side it's ack.
>>
>> Via whom is this supposed to go in? Looking at the MAINTAINERS, I would
>> say Martin or Heiko but I don't see them among the recipients.
> 
> FWIW as the original author of that transport
> Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
> 
> I can pick this up for Martins/Heikos tree if you want.

That would be great, yes, thanks!

 Thomas


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] Update my email address

2017-10-26 Thread Thomas Huth 
On 04.07.2017 11:30, Cornelia Huck wrote:
> Signed-off-by: Cornelia Huck <coh...@redhat.com>
> ---
>  MAINTAINERS | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 767e9d202adf..84155d593bba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7285,7 +7285,7 @@ F:  arch/powerpc/kvm/
>  
>  KERNEL VIRTUAL MACHINE for s390 (KVM/s390)
>  M:   Christian Borntraeger <borntrae...@de.ibm.com>
> -M:   Cornelia Huck <cornelia.h...@de.ibm.com>
> +M:   Cornelia Huck <coh...@redhat.com>
>  L:   linux-s...@vger.kernel.org
>  W:   http://www.ibm.com/developerworks/linux/linux390/
>  T:   git git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git
> @@ -11060,7 +11060,7 @@ S:Supported
>  F:   drivers/iommu/s390-iommu.c
>  
>  S390 VFIO-CCW DRIVER
> -M:   Cornelia Huck <cornelia.h...@de.ibm.com>
> +M:   Cornelia Huck <coh...@redhat.com>
>  M:   Dong Jia Shi <bjsdj...@linux.vnet.ibm.com>
>  L:   linux-s...@vger.kernel.org
>  L:   k...@vger.kernel.org
> @@ -13569,7 +13569,7 @@ F:include/uapi/linux/virtio_*.h
>  F:   drivers/crypto/virtio/
>  
>  VIRTIO DRIVERS FOR S390
> -M:   Cornelia Huck <cornelia.h...@de.ibm.com>
> +M:   Cornelia Huck <coh...@redhat.com>
>  M:   Halil Pasic <pa...@linux.vnet.ibm.com>
>  L:   linux-s...@vger.kernel.org
>  L:   virtualization@lists.linux-foundation.org
> 

Reviewed-by: Thomas Huth <th...@redhat.com>




signature.asc
Description: OpenPGP digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio_ring: fix complaint by sparse

2016-11-22 Thread Thomas Huth 
On 22.11.2016 16:04, Michael S. Tsirkin wrote:
> On Tue, Nov 22, 2016 at 01:51:50PM +0800, Gonglei wrote:
>>  # make C=2 CF="-D__CHECK_ENDIAN__" ./drivers/virtio/
>>
>> drivers/virtio/virtio_ring.c:423:19: warning: incorrect type in assignment 
>> (different base types)
>> drivers/virtio/virtio_ring.c:423:19:expected unsigned int [unsigned] 
>> [assigned] i
>> drivers/virtio/virtio_ring.c:423:19:got restricted __virtio16 [usertype] 
>> next
>> drivers/virtio/virtio_ring.c:423:19: warning: incorrect type in assignment 
>> (different base types)
>> drivers/virtio/virtio_ring.c:423:19:expected unsigned int [unsigned] 
>> [assigned] i
>> drivers/virtio/virtio_ring.c:423:19:got restricted __virtio16 [usertype] 
>> next
>> drivers/virtio/virtio_ring.c:423:19: warning: incorrect type in assignment 
>> (different base types)
>> drivers/virtio/virtio_ring.c:423:19:expected unsigned int [unsigned] 
>> [assigned] i
>> drivers/virtio/virtio_ring.c:423:19:got restricted __virtio16 [usertype] 
>> next
>> drivers/virtio/virtio_ring.c:604:39: warning: incorrect type in initializer 
>> (different base types)
>> drivers/virtio/virtio_ring.c:604:39:expected unsigned short [unsigned] 
>> [usertype] nextflag
>> drivers/virtio/virtio_ring.c:604:39:got restricted __virtio16
>> drivers/virtio/virtio_ring.c:612:33: warning: restricted __virtio16 degrades 
>> to integer
>>
>> Signed-off-by: Gonglei 
>> ---
>>  drivers/virtio/virtio_ring.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 489bfc6..d2863c3 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -420,7 +420,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>>  if (i == err_idx)
>>  break;
>>  vring_unmap_one(vq, [i]);
>> -i = vq->vring.desc[i].next;
>> +i = virtio16_to_cpu(_vq->vdev, vq->vring.desc[i].next);
>>  }
>>  
>>  vq->vq.num_free += total_sg;
[...]
> Wow are you saying endian-ness is all wrong for the next field?
> How do things ever work then?

The above code is only in the error cleanup path (after the
"unmap_release" label, introduced by commit 780bc7903), so it likely has
never been exercised in the field.
I think Gonlei's patch is right, there should be a virtio16_to_cpu() here.

 Thomas


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] KVM: Add Kconfig option to signal cross-endian guests

2015-07-09 Thread Thomas Huth 
On Thu, 9 Jul 2015 16:07:47 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Jul 09, 2015 at 02:57:33PM +0200, Paolo Bonzini wrote:
  
  
  On 09/07/2015 11:48, Laurent Vivier wrote:
   
   
   On 09/07/2015 09:49, Thomas Huth wrote:
   The option for supporting cross-endianness legacy guests in
   the vhost and tun code should only be available on systems
   that support cross-endian guests.
   
   I'm sure I misunderstand something, but what happens if we use QEMU with
   TCG instead of KVM, i.e. a big endian powerpc kernel guest on x86_64
   little endian host ?
  
  TCG does not yet support irqfd/ioeventfd, so it cannot be used with vhost.
  
  Paolo
 
 vhost does not require irqfd anymore.  I think ioeventfd actually works
 fine though I didn't try, it would be easy to support.

That's an interesting issue, thanks for pointing this out, Laurent! So
do we now rather want to leave everything as it currently is, in case
somebody wants to use vhost-net with a cross-endian TCG guest one day?

Or do we assume that either
a) TCG is so slow anyway that nobody wants to accelerate it with vhost
or
b) TCG vhost likely won't happen that soon so we hope that everybody
will already be using virtio 1.0 at that point in time (with a fixed
endianness) 
?
... then I think we should go on and include this patch.

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PULL] virtio/vhost: cross endian support

2015-07-07 Thread Thomas Huth 
On Thu, 2 Jul 2015 11:32:52 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Jul 02, 2015 at 11:12:56AM +0200, Greg Kurz wrote:
  On Thu, 2 Jul 2015 08:01:28 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
...
   Yea, well - support for legacy BE guests on the new LE hosts is
   exactly the motivation for this.
   
   I dislike it too, but there are two redeeming properties that
   made me merge this:
   
   1.  It's a trivial amount of code: since we wrap host/guest accesses
   anyway, almost all of it is well hidden from drivers.
   
   2.  Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY -
   and when it's clear, there's zero overhead (as some point it was
   tested by compiling with and without the patches, got the same
   stripped binary).
   
   Maybe we could create a Kconfig symbol to enforce point (2): prevent
   people from enabling it e.g. on x86. I will look into this - but it can
   be done by a patch on top, so I think this can be merged as is.
   
  
  This cross-endian *oddity* is targeting PowerPC book3s_64 processors... I
  am not aware of any other users. Maybe create a symbol that would
  be only selected by PPC_BOOK3S_64 ?
 
 I think some ARM systems are trying to support cross-endian
 configurations as well.
 
 Besides that, yes, this is more or less what I had in mind.

Would something simple like this already do the job:

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -35,6 +35,7 @@ config VHOST
 
 config VHOST_CROSS_ENDIAN_LEGACY
bool Cross-endian support for vhost
+   depends on KVM_BOOK3S_64 || KVM_ARM_HOST
default n
---help---
  This option allows vhost to support guests with a different byte

?

If that looks acceptable, I can submit a proper patch if you like.

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: I can read/write in virtio BLK Device, but I can't run a hello world program in it

2015-07-07 Thread Thomas Huth 
On Tue, 7 Jul 2015 10:52:01 +0900
Ganis Zulfa Santoso ganis.zu...@gmail.com wrote:

 Hi Linux Virtualization Mailing List Members,
 
 I am trying to develop a driver for virtio blk device in guest.

Why don't you simply use the driver that is already available in the
kernel?

 I can safely mount the virtual blk device with: mount /dev/vda /mnt/
 in /mnt/ I can read  write the device. But when I run a simple hello world 
 program in /mnt/root/, this error happens:

Have you already verified that you can successfully read the _right_
data from your block device? I.e. something like

mount /dev/xxx /mnt
cp helloworld_static /mnt/root/helloworld_static
umount /mnt  # just to make sure that it is not cached
mount /dev/xxx /mnt
md5sum helloworld_static
md5sum /mnt/root/helloworld_static

... and then compare the two md5sums whether they are the same?

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()

2015-07-06 Thread Thomas Huth 
On Sun, 5 Jul 2015 14:59:54 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
  On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
   The virtio_ring.h header is used in userspace programs (ie. QEMU),
   too. Here we can not assume that sizeof(pointer) is the same as
   sizeof(long), e.g. when compiling for Windows, so the typecast in
   vring_init() should be done with (uintptr_t) instead of (unsigned long).
   
   Signed-off-by: Thomas Huth th...@redhat.com
  
  This seems to break some userspace too:
  
INSTALL usr/include/linux/ (413 files)
CHECK   usr/include/linux/ (413 files)
HOSTCC  Documentation/accounting/getdelays
HOSTCC  Documentation/connector/ucon
HOSTCC  Documentation/mic/mpssd/mpssd.o
  In file included from Documentation/mic/mpssd/mpssd.c:36:0:
  ./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
  ./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
  (first use in this function)
vr-used = (void *)(((uintptr_t)vr-avail-ring[num] +
  sizeof(__virtio16)
  ^
  ./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
  identifier is reported only once for each function it appears in
  scripts/Makefile.host:108: recipe for target

D'oh, I only built the kernel for powerpc, that's why I did not notice
this breakage in the MIC code. Sorry!

  E.g. fuse.h has this code:
  #ifdef __KERNEL__
  #include linux/types.h
  #else
  #include stdint.h
  #endif
  
  Maybe we need something similar.
 
 The following seems to help.  Does it help the windows build?
...
 diff --git a/include/uapi/linux/virtio_ring.h 
 b/include/uapi/linux/virtio_ring.h
 index 8682551..c072959 100644
 --- a/include/uapi/linux/virtio_ring.h
 +++ b/include/uapi/linux/virtio_ring.h
 @@ -31,6 +31,9 @@
   * SUCH DAMAGE.
   *
   * Copyright Rusty Russell IBM Corporation 2007. */
 +#ifndef __KERNEL__
 +#include stdint.h
 +#endif
  #include linux/types.h
  #include linux/virtio_types.h

Since the update-linux-headers.sh script from QEMU replaces the
#include linux/types.h in virtio_ring.h to include a file that again
includes stdint.h already, your above patch should not do any
difference for compiling QEMU for Windows, I think. It's really just
about replacing the typecast there.

But you're right of course for other user space applications which
include this header by other means. So I'm fine if you change my patch
with your hunk above (or add it as an additional patch). Or shall I
rather resend my patch with your above hunk included?

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()

2015-07-06 Thread Thomas Huth 
On Mon, 6 Jul 2015 12:50:22 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Jul 06, 2015 at 11:24:42AM +0200, Thomas Huth wrote:
  On Sun, 5 Jul 2015 14:59:54 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
 The virtio_ring.h header is used in userspace programs (ie. QEMU),
 too. Here we can not assume that sizeof(pointer) is the same as
 sizeof(long), e.g. when compiling for Windows, so the typecast in
 vring_init() should be done with (uintptr_t) instead of (unsigned 
 long).
 
 Signed-off-by: Thomas Huth th...@redhat.com

This seems to break some userspace too:

  INSTALL usr/include/linux/ (413 files)
  CHECK   usr/include/linux/ (413 files)
  HOSTCC  Documentation/accounting/getdelays
  HOSTCC  Documentation/connector/ucon
  HOSTCC  Documentation/mic/mpssd/mpssd.o
In file included from Documentation/mic/mpssd/mpssd.c:36:0:
./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
(first use in this function)
  vr-used = (void *)(((uintptr_t)vr-avail-ring[num] +
sizeof(__virtio16)
^
./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
identifier is reported only once for each function it appears in
scripts/Makefile.host:108: recipe for target
  
  D'oh, I only built the kernel for powerpc, that's why I did not notice
  this breakage in the MIC code. Sorry!
  
E.g. fuse.h has this code:
#ifdef __KERNEL__
#include linux/types.h
#else
#include stdint.h
#endif

Maybe we need something similar.
   
   The following seems to help.  Does it help the windows build?
  ...
   diff --git a/include/uapi/linux/virtio_ring.h 
   b/include/uapi/linux/virtio_ring.h
   index 8682551..c072959 100644
   --- a/include/uapi/linux/virtio_ring.h
   +++ b/include/uapi/linux/virtio_ring.h
   @@ -31,6 +31,9 @@
 * SUCH DAMAGE.
 *
 * Copyright Rusty Russell IBM Corporation 2007. */
   +#ifndef __KERNEL__
   +#include stdint.h
   +#endif
#include linux/types.h
#include linux/virtio_types.h
  
  Since the update-linux-headers.sh script from QEMU replaces the
  #include linux/types.h in virtio_ring.h to include a file that again
  includes stdint.h already, your above patch should not do any
  difference for compiling QEMU for Windows, I think. It's really just
  about replacing the typecast there.
  
  But you're right of course for other user space applications which
  include this header by other means. So I'm fine if you change my patch
  with your hunk above (or add it as an additional patch). Or shall I
  rather resend my patch with your above hunk included?
  
   Thomas
 
 You don't have to, but could you confirm that stdint has uintptr_t on
 mingw?
 

$ grep typedef.*uintptr /usr/x86_64-w64-mingw32/sys-root/mingw/include/*.h
...
/usr/x86_64-w64-mingw32/sys-root/mingw/include/crtdefs.h:__MINGW_EXTENSION 
typedef unsigned __int64 uintptr_t;
...

And the stdint.h from my MinGW installation includes crtdefs.h.
So yes, confirmed that stdint.h provides uintptr_t there, too.

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH RESEND] virtio: Fix typecast of pointer in vring_init()

2015-07-02 Thread Thomas Huth 
The virtio_ring.h header is used in userspace programs (ie. QEMU),
too. Here we can not assume that sizeof(pointer) is the same as
sizeof(long), e.g. when compiling for Windows, so the typecast in
vring_init() should be done with (uintptr_t) instead of (unsigned long).

Signed-off-by: Thomas Huth th...@redhat.com
---
 include/uapi/linux/virtio_ring.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 915980a..8682551 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -143,7 +143,7 @@ static inline void vring_init(struct vring *vr, unsigned 
int num, void *p,
vr-num = num;
vr-desc = p;
vr-avail = p + num*sizeof(struct vring_desc);
-   vr-used = (void *)(((unsigned long)vr-avail-ring[num] + 
sizeof(__virtio16)
+   vr-used = (void *)(((uintptr_t)vr-avail-ring[num] + 
sizeof(__virtio16)
+ align-1)  ~(align - 1));
 }
 
-- 
1.8.3.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio: Fix typecast of pointer in vring_init()

2015-05-06 Thread Thomas Huth 
The virtio_ring.h header is used in userspace programs (ie. QEMU),
too. Here we can not assume that sizeof(pointer) is the same as
sizeof(long), e.g. when compiling for Windows, so the typecast
in vring_init() should be done with (uintptr_t) instead of
(unsigned long). This fixes a compiler warning when compiling
QEMU with the mingw32 cross-compiler.

Signed-off-by: Thomas Huth th...@redhat.com
---
 include/uapi/linux/virtio_ring.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 915980a..8682551 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -143,7 +143,7 @@ static inline void vring_init(struct vring *vr, unsigned 
int num, void *p,
vr-num = num;
vr-desc = p;
vr-avail = p + num*sizeof(struct vring_desc);
-   vr-used = (void *)(((unsigned long)vr-avail-ring[num] + 
sizeof(__virtio16)
+   vr-used = (void *)(((uintptr_t)vr-avail-ring[num] + 
sizeof(__virtio16)
+ align-1)  ~(align - 1));
 }
 
-- 
1.8.3.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 1/8] virtio: introduce virtio_is_little_endian() helper

2015-04-24 Thread Thomas Huth 
Am Thu, 23 Apr 2015 17:26:20 +0200
schrieb Greg Kurz gk...@linux.vnet.ibm.com:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  include/linux/virtio_config.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
 index ca3ed78..bd1a582 100644
 --- a/include/linux/virtio_config.h
 +++ b/include/linux/virtio_config.h
 @@ -205,35 +205,40 @@ int virtqueue_set_affinity(struct virtqueue *vq, int 
 cpu)
   return 0;
  }
  
 +static inline bool virtio_is_little_endian(struct virtio_device *vdev)
 +{
 + return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
 +}

So this function returns false when _not_ using version 1, but running
on a little endian host + guest? Sounds confusing. Maybe you could name
it virtio_is_v1() or so instead?

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 4/8] vringh: introduce vringh_is_little_endian() helper

2015-04-24 Thread Thomas Huth 
On Thu, 23 Apr 2015 17:26:52 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  include/linux/vringh.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/include/linux/vringh.h b/include/linux/vringh.h
 index a3fa537..3ed62ef 100644
 --- a/include/linux/vringh.h
 +++ b/include/linux/vringh.h
 @@ -226,33 +226,38 @@ static inline void vringh_notify(struct vringh *vrh)
   vrh-notify(vrh);
  }
  
 +static inline bool vringh_is_little_endian(const struct vringh *vrh)
 +{
 + return vrh-little_endian;
 +}
 +
  static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)
  {
 - return __virtio16_to_cpu(vrh-little_endian, val);
 + return __virtio16_to_cpu(vringh_is_little_endian(vrh), val);
  }
  
  static inline __virtio16 cpu_to_vringh16(const struct vringh *vrh, u16 val)
  {
 - return __cpu_to_virtio16(vrh-little_endian, val);
 + return __cpu_to_virtio16(vringh_is_little_endian(vrh), val);
  }
  
  static inline u32 vringh32_to_cpu(const struct vringh *vrh, __virtio32 val)
  {
 - return __virtio32_to_cpu(vrh-little_endian, val);
 + return __virtio32_to_cpu(vringh_is_little_endian(vrh), val);
  }
  
  static inline __virtio32 cpu_to_vringh32(const struct vringh *vrh, u32 val)
  {
 - return __cpu_to_virtio32(vrh-little_endian, val);
 + return __cpu_to_virtio32(vringh_is_little_endian(vrh), val);
  }
  
  static inline u64 vringh64_to_cpu(const struct vringh *vrh, __virtio64 val)
  {
 - return __virtio64_to_cpu(vrh-little_endian, val);
 + return __virtio64_to_cpu(vringh_is_little_endian(vrh), val);
  }
  
  static inline __virtio64 cpu_to_vringh64(const struct vringh *vrh, u64 val)
  {
 - return __cpu_to_virtio64(vrh-little_endian, val);
 + return __cpu_to_virtio64(vringh_is_little_endian(vrh), val);
  }
  #endif /* _LINUX_VRINGH_H */

Reviewed-by: Thomas Huth th...@redhat.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 5/8] vhost: introduce vhost_is_little_endian() helper

2015-04-24 Thread Thomas Huth 
On Thu, 23 Apr 2015 17:27:05 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  drivers/vhost/vhost.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
 index 8c1c792..6a49960 100644
 --- a/drivers/vhost/vhost.h
 +++ b/drivers/vhost/vhost.h
 @@ -173,34 +173,39 @@ static inline bool vhost_has_feature(struct 
 vhost_virtqueue *vq, int bit)
   return vq-acked_features  (1ULL  bit);
  }
  
 +static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
 +{
 + return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
 +}
 +
  /* Memory accessors */
  static inline u16 vhost16_to_cpu(struct vhost_virtqueue *vq, __virtio16 val)
  {
 - return __virtio16_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio16_to_cpu(vhost_is_little_endian(vq), val);
  }
  
  static inline __virtio16 cpu_to_vhost16(struct vhost_virtqueue *vq, u16 val)
  {
 - return __cpu_to_virtio16(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio16(vhost_is_little_endian(vq), val);
  }
  
  static inline u32 vhost32_to_cpu(struct vhost_virtqueue *vq, __virtio32 val)
  {
 - return __virtio32_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio32_to_cpu(vhost_is_little_endian(vq), val);
  }
  
  static inline __virtio32 cpu_to_vhost32(struct vhost_virtqueue *vq, u32 val)
  {
 - return __cpu_to_virtio32(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio32(vhost_is_little_endian(vq), val);
  }
  
  static inline u64 vhost64_to_cpu(struct vhost_virtqueue *vq, __virtio64 val)
  {
 - return __virtio64_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio64_to_cpu(vhost_is_little_endian(vq), val);
  }
  
  static inline __virtio64 cpu_to_vhost64(struct vhost_virtqueue *vq, u64 val)
  {
 - return __cpu_to_virtio64(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio64(vhost_is_little_endian(vq), val);
  }
  #endif

Reviewed-by: Thomas Huth th...@redhat.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 6/8] virtio: add explicit big-endian support to memory accessors

2015-04-24 Thread Thomas Huth 
On Thu, 23 Apr 2015 17:29:06 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 The current memory accessors logic is:
 - little endian if little_endian
 - native endian (i.e. no byteswap) if !little_endian
 
 If we want to fully support cross-endian vhost, we also need to be
 able to convert to big endian.
 
 Instead of changing the little_endian argument to some 3-value enum, this
 patch changes the logic to:
 - little endian if little_endian
 - big endian if !little_endian
 
 The native endian case is handled by all users with a trivial helper. This
 patch doesn't change any functionality, nor it does add overhead.
 
 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
 
 Changes since v4:
 - style fixes (I have chosen if ... else in most places to stay below
   80 columns, with the notable exception of the vhost helper which gets
   shorten in a later patch)
 
  drivers/net/macvtap.c|5 -
  drivers/net/tun.c|5 -
  drivers/vhost/vhost.h|2 +-
  include/linux/virtio_byteorder.h |   24 ++--
  include/linux/virtio_config.h|5 -
  include/linux/vringh.h   |2 +-
  6 files changed, 28 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
 index a2f2958..6cf6b3e 100644
 --- a/drivers/net/macvtap.c
 +++ b/drivers/net/macvtap.c
 @@ -51,7 +51,10 @@ struct macvtap_queue {
  
  static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
  {
 - return q-flags  MACVTAP_VNET_LE;
 + if (q-flags  MACVTAP_VNET_LE)
 + return true;
 + else
 + return virtio_legacy_is_little_endian();

simply:

return (q-flags  MACVTAP_VNET_LE) ||
   virtio_legacy_is_little_endian();

?

  }
  
  static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
 diff --git a/drivers/net/tun.c b/drivers/net/tun.c
 index 3c3d6c0..5b044d4 100644
 --- a/drivers/net/tun.c
 +++ b/drivers/net/tun.c
 @@ -208,7 +208,10 @@ struct tun_struct {
  
  static inline bool tun_is_little_endian(struct tun_struct *tun)
  {
 - return tun-flags  TUN_VNET_LE;
 + if (tun-flags  TUN_VNET_LE)
 + return true;
 + else
 + return virtio_legacy_is_little_endian();

dito?

  }
  
  static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
 diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
 index 6a49960..954c657 100644
 --- a/drivers/vhost/vhost.h
 +++ b/drivers/vhost/vhost.h
 @@ -175,7 +175,7 @@ static inline bool vhost_has_feature(struct 
 vhost_virtqueue *vq, int bit)
  
  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
  {
 - return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
 + return vhost_has_feature(vq, VIRTIO_F_VERSION_1) ? true : 
 virtio_legacy_is_little_endian();
  }

That line is way longer than 80 characters ... may I suggest to switch
at least here to:

return vhost_has_feature(vq, VIRTIO_F_VERSION_1) ||
   virtio_legacy_is_little_endian();

?

Apart from the cosmetics, the patch looks good to me.

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 1/8] virtio: introduce virtio_is_little_endian() helper

2015-04-24 Thread Thomas Huth 
Am Thu, 23 Apr 2015 19:22:15 +0200
schrieb Thomas Huth th...@redhat.com:

 Am Thu, 23 Apr 2015 17:26:20 +0200
 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
 
  Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
  ---
   include/linux/virtio_config.h |   17 +++--
   1 file changed, 11 insertions(+), 6 deletions(-)
  
  diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
  index ca3ed78..bd1a582 100644
  --- a/include/linux/virtio_config.h
  +++ b/include/linux/virtio_config.h
  @@ -205,35 +205,40 @@ int virtqueue_set_affinity(struct virtqueue *vq, int 
  cpu)
  return 0;
   }
   
  +static inline bool virtio_is_little_endian(struct virtio_device *vdev)
  +{
  +   return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
  +}
 
 So this function returns false when _not_ using version 1, but running
 on a little endian host + guest? Sounds confusing. Maybe you could name
 it virtio_is_v1() or so instead?

Ah, never mind, I should have looked at patch 6 first, then it makes
sense. (maybe you could put a note to the later patch in this patch
description?)

 Thomas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 1/8] virtio: introduce virtio_is_little_endian() helper

2015-04-24 Thread Thomas Huth 
On Thu, 23 Apr 2015 17:26:20 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  include/linux/virtio_config.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
 index ca3ed78..bd1a582 100644
 --- a/include/linux/virtio_config.h
 +++ b/include/linux/virtio_config.h
 @@ -205,35 +205,40 @@ int virtqueue_set_affinity(struct virtqueue *vq, int 
 cpu)
   return 0;
  }
  
 +static inline bool virtio_is_little_endian(struct virtio_device *vdev)
 +{
 + return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
 +}
 +
  /* Memory accessors */
  static inline u16 virtio16_to_cpu(struct virtio_device *vdev, __virtio16 val)
  {
 - return __virtio16_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio16_to_cpu(virtio_is_little_endian(vdev), val);
  }
  
  static inline __virtio16 cpu_to_virtio16(struct virtio_device *vdev, u16 val)
  {
 - return __cpu_to_virtio16(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio16(virtio_is_little_endian(vdev), val);
  }
  
  static inline u32 virtio32_to_cpu(struct virtio_device *vdev, __virtio32 val)
  {
 - return __virtio32_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio32_to_cpu(virtio_is_little_endian(vdev), val);
  }
  
  static inline __virtio32 cpu_to_virtio32(struct virtio_device *vdev, u32 val)
  {
 - return __cpu_to_virtio32(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio32(virtio_is_little_endian(vdev), val);
  }
  
  static inline u64 virtio64_to_cpu(struct virtio_device *vdev, __virtio64 val)
  {
 - return __virtio64_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio64_to_cpu(virtio_is_little_endian(vdev), val);
  }
  
  static inline __virtio64 cpu_to_virtio64(struct virtio_device *vdev, u64 val)
  {
 - return __cpu_to_virtio64(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
  }

Reviewed-by: Thomas Huth th...@redhat.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 2/8] tun: add tun_is_little_endian() helper

2015-04-24 Thread Thomas Huth 
On Thu, 23 Apr 2015 17:26:30 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  drivers/net/tun.c |9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/tun.c b/drivers/net/tun.c
 index 857dca4..3c3d6c0 100644
 --- a/drivers/net/tun.c
 +++ b/drivers/net/tun.c
 @@ -206,14 +206,19 @@ struct tun_struct {
   u32 flow_count;
  };
  
 +static inline bool tun_is_little_endian(struct tun_struct *tun)
 +{
 + return tun-flags  TUN_VNET_LE;
 +}
 +
  static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
  {
 - return __virtio16_to_cpu(tun-flags  TUN_VNET_LE, val);
 + return __virtio16_to_cpu(tun_is_little_endian(tun), val);
  }
  
  static inline __virtio16 cpu_to_tun16(struct tun_struct *tun, u16 val)
  {
 - return __cpu_to_virtio16(tun-flags  TUN_VNET_LE, val);
 + return __cpu_to_virtio16(tun_is_little_endian(tun), val);
  }

Reviewed-by: Thomas Huth th...@redhat.com

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 3/8] macvtap: introduce macvtap_is_little_endian() helper

2015-04-24 Thread Thomas Huth 
On Thu, 23 Apr 2015 17:26:41 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  drivers/net/macvtap.c |9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
 index 27ecc5c..a2f2958 100644
 --- a/drivers/net/macvtap.c
 +++ b/drivers/net/macvtap.c
 @@ -49,14 +49,19 @@ struct macvtap_queue {
  
  #define MACVTAP_VNET_LE 0x8000
  
 +static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
 +{
 + return q-flags  MACVTAP_VNET_LE;
 +}
 +
  static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
  {
 - return __virtio16_to_cpu(q-flags  MACVTAP_VNET_LE, val);
 + return __virtio16_to_cpu(macvtap_is_little_endian(q), val);
  }
  
  static inline __virtio16 cpu_to_macvtap16(struct macvtap_queue *q, u16 val)
  {
 - return __cpu_to_virtio16(q-flags  MACVTAP_VNET_LE, val);
 + return __cpu_to_virtio16(macvtap_is_little_endian(q), val);
  }

Reviewed-by: Thomas Huth th...@redhat.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING

2015-02-25 Thread Thomas Huth 
On Wed, 25 Feb 2015 16:11:27 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 On Wed, 25 Feb 2015 15:36:02 +0100
 Michael S. Tsirkin m...@redhat.com wrote:
 
  virtio balloon has this code:
  wait_event_interruptible(vb-config_change,
   (diff = towards_target(vb)) != 0
   || vb-need_stats_update
   || kthread_should_stop()
   || freezing(current));
  
  Which is a problem because towards_target() call might block after
  wait_event_interruptible sets task state to TAST_INTERRUPTIBLE, causing
  the task_struct::state collision typical of nesting of sleeping
  primitives
  
  See also http://lwn.net/Articles/628628/ or Thomas's
  bug report
  http://article.gmane.org/gmane.linux.kernel.virtualization/24846
  for a fuller explanation.
  
  To fix, rewrite using wait_woken.
  
  Cc: sta...@vger.kernel.org
  Reported-by: Thomas Huth th...@linux.vnet.ibm.com
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  ---
  
  changes from v1:
  remove wait_event_interruptible
  noticed by Cornelia Huck cornelia.h...@de.ibm.com
  
   drivers/virtio/virtio_balloon.c | 19 ++-
   1 file changed, 14 insertions(+), 5 deletions(-)
  
 
 I was able to reproduce Thomas' original problem and can confirm that
 it is gone with this patch.
 
 Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

Right, I just applied the patch on my system, too, and the problem is
indeed gone! Thanks for the quick fix!

Tested-by: Thomas Huth th...@linux.vnet.ibm.com

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

2015-02-25 Thread Thomas Huth 
On Thu, 26 Feb 2015 11:50:42 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 Thomas Huth th...@linux.vnet.ibm.com writes:
   Hi all,
 
  with the recent kernel 3.19, I get a kernel warning when I start my
  KVM guest on s390 with virtio balloon enabled:
 
 The deeper problem is that virtio_ccw_get_config just silently fails on
 OOM.
 
 Neither get_config nor set_config are expected to fail.

AFAIK this is currently not a problem. According to
http://lwn.net/Articles/627419/ these kmalloc calls never
fail because they allocate less than a page.

 Thomas

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

virtio balloon: do not call blocking ops when !TASK_RUNNING

2015-02-25 Thread Thomas Huth 

 Hi all,

with the recent kernel 3.19, I get a kernel warning when I start my
KVM guest on s390 with virtio balloon enabled:

[0.839687] do not call blocking ops when !TASK_RUNNING; state=1 set at
   [00174a1e] prepare_to_wait_event+0x7e/0x108
[0.839694] [ cut here ]
[0.839697] WARNING: at kernel/sched/core.c:7326
[0.839698] Modules linked in:
[0.839702] CPU: 0 PID: 46 Comm: vballoon Not tainted 3.19.0 #233
[0.839705] task: 021d ti: 021d8000 task.ti: 
021d8000
[0.839707] Krnl PSW : 0704c0018000 0015bf8e 
(__might_sleep+0x8e/0x98)
[0.839713]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 
EA:3
Krnl GPRS: 000d 021d 0071 0001
[0.839718]00675ace 01998c50  

[0.839720]00982134 0058f824 00a008a8 

[0.839722]04d9 007ea992 0015bf8a 
021dbc28
[0.839731] Krnl Code: 0015bf7e: c0200033e838larl
%r2,7d8fee
   0015bf84: c0e50028cd62   brasl   %r14,675a48
  #0015bf8a: a7f40001   brc 15,15bf8c
  0015bf8e: 9201a000   mvi 0(%r10),1
   0015bf92: a7f4ffe2   brc 15,15bf56
   0015bf96: 0707   bcr 0,%r7
   0015bf98: ebdff0800024   stmg%r13,%r15,128(%r15)
   0015bf9e: a7f13fe0   tmll%r15,16352
[0.839749] Call Trace:
[0.839751] ([0015bf8a] __might_sleep+0x8a/0x98)
[0.839756]  [0028a562] __kmalloc+0x272/0x350
[0.839759]  [0058f824] virtio_ccw_get_config+0x3c/0x100
[0.839762]  [0049fcb0] balloon+0x1b8/0x330
[0.839765]  [001529c8] kthread+0x120/0x138
[0.839767]  [00683c22] kernel_thread_starter+0x6/0xc
[0.839770]  [00683c1c] kernel_thread_starter+0x0/0xc
[0.839772] no locks held by vballoon/46.
[0.839773] Last Breaking-Event-Address:
[0.839776]  [0015bf8a] __might_sleep+0x8a/0x98
[0.839778] ---[ end trace d27fcdfa27273d7c ]---

The problem seems to be this code in balloon() in
drivers/virtio/virtio_balloon.c:

wait_event_interruptible(vb-config_change,
 (diff = towards_target(vb)) != 0
 || vb-need_stats_update
 || kthread_should_stop()
 || freezing(current));

wait_event_interruptible() sets the state of the current task to
TASK_INTERRUPTIBLE, then checks the condition. The condition contains
towards_target() which reads the virtio config space via virtio_cread().
On s390, this then triggers virtio_ccw_get_config() - and this function
calls some other functions again that might sleep (e.g. kzalloc or
wait_event in ccw_io_helper) ... and this causes the new kernel warning
message with kernel 3.19.

I think it would be quite difficult or at least ugly to rewrite
virtio_ccw_get_config() so that it does not call sleepable functions
anymore. So would it be feasible to rewrite the balloon() function that
it does not call the towards_target() in its wait_event condition
anymore? I am unfortunately not that familiar with the balloon code
semantics, so any help is very appreciated here!

 Thanks,
  Thomas

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Qemu-devel] [PATCH RFC v6 05/20] virtio: support more feature bits

2015-01-29 Thread Thomas Huth 

 Hi,

On Thu, 29 Jan 2015 11:11:32 +1100
David Gibson da...@gibson.dropbear.id.au wrote:

 On Wed, Jan 28, 2015 at 04:59:45PM +0100, Cornelia Huck wrote:
  On Thu, 22 Jan 2015 12:43:43 +1100
  David Gibson da...@gibson.dropbear.id.au wrote:
  
   On Thu, Dec 11, 2014 at 02:25:07PM +0100, Cornelia Huck wrote:
With virtio-1, we support more than 32 feature bits. Let's extend both
host and guest features to 64, which should suffice for a while.

vhost and migration have been ignored for now.
   
   [snip]
   
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f6c0379..08141c7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -55,6 +55,12 @@
 /* A guest should never accept this.  It implies negotiation is 
broken. */
 #define VIRTIO_F_BAD_FEATURE   30
 
+/* v1.0 compliant. */
+#define VIRTIO_F_VERSION_1  32
   
   This is already in the kernel header, isn't it?
 
  
  Yes. But nearly all files include this header but not the kernel
  header.
 
 Can't you change that?  Or this file include the kernel header?
 
AFAIK non-KVM code should never try to include one of the Linux headers
to avoid breaking on non-Linux platforms (for example linux/types.h is
not available on OS X, see http://patchwork.ozlabs.org/patch/424655/ ).
So it's a little bit ugly to define these things twice, but it seems
the only way to stay portable.

 Thomas


signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Qemu-devel] [PATCH RFC v6 10/20] s390x/virtio-ccw: add virtio set-revision call

2015-01-21 Thread Thomas Huth 
On Wed, 21 Jan 2015 12:23:18 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 On Tue, 20 Jan 2015 11:08:24 +
 Stefan Hajnoczi stefa...@gmail.com wrote:
 
  On Thu, Dec 11, 2014 at 02:25:12PM +0100, Cornelia Huck wrote:
   @@ -608,6 +631,25 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
}
}
break;
   +case CCW_CMD_SET_VIRTIO_REV:
   +len = sizeof(revinfo);
   +if (ccw.count  len || (check_len  ccw.count  len)) {
   +ret = -EINVAL;
   +break;
   +}
   +if (!ccw.cda) {
   +ret = -EFAULT;
   +break;
   +}
   +cpu_physical_memory_read(ccw.cda, revinfo, len);
   +if (dev-revision = 0 ||
   +revinfo.revision  virtio_ccw_rev_max(dev)) {
  
  In the next patch virtio_ccw_handle_set_vq() uses big-endian memory
  access functions to load a struct from guest memory.
  
  Here you just copy the struct in without byteswaps.
  
  Are the byteswaps missing here?  (I guess this normally runs big-endian
  guests on big-endian hosts so it's not noticable.)
 
 Indeed, these are supposed to be big-endian. I'll double check the
 other payloads.

Right. Cornelia, can you take care of this or shall I rework the patch?

NB: Actually, there are a couple of XXX config space endianness
comments in that virtio_ccw_cb() function, so there are likely a bunch
of problems when this code should be run on little endian hosts one day.
So far, this code only runs with big-endian guests on big-endian hosts
since the virtio-ccw machine is currently KVM-only as far as I know,
that's likely why nobody complained about this yet.

 Thomas

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v6 05/20] virtio: support more feature bits

2014-12-12 Thread Thomas Huth 
On Thu, 11 Dec 2014 14:25:07 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 With virtio-1, we support more than 32 feature bits. Let's extend both
 host and guest features to 64, which should suffice for a while.
 
 vhost and migration have been ignored for now.
 
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 ---
  hw/9pfs/virtio-9p-device.c  |2 +-
  hw/block/virtio-blk.c   |2 +-
  hw/char/virtio-serial-bus.c |2 +-
  hw/core/qdev-properties.c   |   58 
 +++
  hw/net/virtio-net.c |   22 +++
  hw/s390x/s390-virtio-bus.c  |3 +-
  hw/s390x/s390-virtio-bus.h  |2 +-
  hw/s390x/virtio-ccw.c   |   39 +++---
  hw/s390x/virtio-ccw.h   |5 +---
  hw/scsi/vhost-scsi.c|3 +-
  hw/scsi/virtio-scsi.c   |4 +--
  hw/virtio/virtio-balloon.c  |2 +-
  hw/virtio/virtio-bus.c  |6 ++--
  hw/virtio/virtio-mmio.c |4 +--
  hw/virtio/virtio-pci.c  |3 +-
  hw/virtio/virtio-pci.h  |2 +-
  hw/virtio/virtio-rng.c  |2 +-
  hw/virtio/virtio.c  |   13 +
  include/hw/qdev-properties.h|   12 
  include/hw/virtio/virtio-bus.h  |8 +++---
  include/hw/virtio/virtio-net.h  |   46 +++
  include/hw/virtio/virtio-scsi.h |6 ++--
  include/hw/virtio/virtio.h  |   38 ++---
  23 files changed, 184 insertions(+), 100 deletions(-)
...
 diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
 index 9f3c58a..d6d1b98 100644
 --- a/hw/net/virtio-net.c
 +++ b/hw/net/virtio-net.c
...
 @@ -514,7 +514,7 @@ static inline uint64_t 
 virtio_net_supported_guest_offloads(VirtIONet *n)
  return virtio_net_guest_offloads_by_features(vdev-guest_features);
  }
 
 -static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 +static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
  {
  VirtIONet *n = VIRTIO_NET(vdev);
  int i;
 @@ -1036,7 +1036,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, 
 const uint8_t *buf, size_t
  return -1;
  error_report(virtio-net unexpected empty queue: 
  i %zd mergeable %d offset %zd, size %zd, 
 -guest hdr len %zd, host hdr len %zd guest features 
 0x%x,
 +guest hdr len %zd, host hdr len %zd guest features 
 0x%lx,

I think you need a different format string like PRIx64 here so that the
code also works right on a 32-bit system (where long is only 32-bit).

  i, n-mergeable_rx_bufs, offset, size,
  n-guest_hdr_len, n-host_hdr_len, vdev-guest_features);
  exit(1);
...
 diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
 index 3fee4aa..fbd909d 100644
 --- a/hw/s390x/virtio-ccw.c
 +++ b/hw/s390x/virtio-ccw.c
 @@ -371,8 +371,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
  } else {
  features.index = ldub_phys(address_space_memory,
 ccw.cda + sizeof(features.features));
 -if (features.index  ARRAY_SIZE(dev-host_features)) {
 -features.features = dev-host_features[features.index];
 +if (features.index == 0) {
 +features.features = (uint32_t)dev-host_features;
 +} else if (features.index == 1) {
 +features.features = (uint32_t)(dev-host_features  32);
  } else {
  /* Return zeroes if the guest supports more feature bits. */
  features.features = 0;
 @@ -399,8 +401,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
  features.index = ldub_phys(address_space_memory,
 ccw.cda + sizeof(features.features));
  features.features = ldl_le_phys(address_space_memory, ccw.cda);
 -if (features.index  ARRAY_SIZE(dev-host_features)) {
 -virtio_set_features(vdev, features.features);
 +if (features.index == 0) {
 +virtio_set_features(vdev,
 +(vdev-guest_features  
 0x) |
 +features.features);
 +} else if (features.index == 1) {
 +virtio_set_features(vdev,
 +(vdev-guest_features  
 0x) |
 +((uint64_t)features.features  32));

The long constants 0x and 0x should
probably get an ULL suffix.

...
 diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
 index 5814433..7f74ae5 100644
 --- a/hw/virtio/virtio.c
 +++ b/hw/virtio/virtio.c
 @@ -593,6 +593,7 @@ void virtio_reset(void *opaque)
  }
 
  vdev-guest_features = 0;
 +

Unnecessary white space change?

Re: [PATCH RFC v6 12/20] virtio: disallow late feature changes for virtio-1

2014-12-12 Thread Thomas Huth 
On Thu, 11 Dec 2014 14:25:14 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 For virtio-1 devices, the driver must not attempt to set feature bits
 after it set FEATURES_OK in the device status. Simply reject it in
 that case.
 
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 ---
  hw/virtio/virtio.c |   16 ++--
  include/hw/virtio/virtio.h |2 ++
  2 files changed, 16 insertions(+), 2 deletions(-)
 
 diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
 index 57190ba..a3dd67b 100644
 --- a/hw/virtio/virtio.c
 +++ b/hw/virtio/virtio.c
 @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
  vmstate_save_state(f, vmstate_virtio, vdev);
  }
 
 -int virtio_set_features(VirtIODevice *vdev, uint64_t val)
 +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val)

Maybe avoid the double underscores here? But unfortunately, I also fail
to come up with a better suggestion for a name here ...

  {
  BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
  VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus);
 @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
  return bad ? -1 : 0;
  }
 
 +int virtio_set_features(VirtIODevice *vdev, uint64_t val)
 +{
 +   /*
 + * The driver must not attempt to set features after feature negotiation
 + * has finished.
 + */
 +if (vdev-status  VIRTIO_CONFIG_S_FEATURES_OK) {
 +return -EINVAL;
 +}

Hmm, according to your patch description, the FEATURES_OK check only
applies to virtio-1.0 devices ... so shouldn't there be a check for
virtio-1 here? Or did I miss something?

 +return __virtio_set_features(vdev, val);
 +}

 Thomas

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v6 12/20] virtio: disallow late feature changes for virtio-1

2014-12-12 Thread Thomas Huth 
On Fri, 12 Dec 2014 12:18:25 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 On Fri, 12 Dec 2014 11:55:38 +0100
 Thomas Huth th...@linux.vnet.ibm.com wrote:
 
  On Thu, 11 Dec 2014 14:25:14 +0100
  Cornelia Huck cornelia.h...@de.ibm.com wrote:
  
   For virtio-1 devices, the driver must not attempt to set feature bits
   after it set FEATURES_OK in the device status. Simply reject it in
   that case.
   
   Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
   ---
hw/virtio/virtio.c |   16 ++--
include/hw/virtio/virtio.h |2 ++
2 files changed, 16 insertions(+), 2 deletions(-)
   
   diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
   index 57190ba..a3dd67b 100644
   --- a/hw/virtio/virtio.c
   +++ b/hw/virtio/virtio.c
   @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
vmstate_save_state(f, vmstate_virtio, vdev);
}
   
   -int virtio_set_features(VirtIODevice *vdev, uint64_t val)
   +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val)
  
  Maybe avoid the double underscores here? But unfortunately, I also fail
  to come up with a better suggestion for a name here ...
 
 virtio_set_features_nocheck()?

Sounds ok to me.

 This function is only called within virtio.c anyway...

Right, so the double underscores should be ok here, too. (I still do
not like them very much, but that's just my personal taste in this case)

{
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus);
   @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t 
   val)
return bad ? -1 : 0;
}
   
   +int virtio_set_features(VirtIODevice *vdev, uint64_t val)
   +{
   +   /*
   + * The driver must not attempt to set features after feature 
   negotiation
   + * has finished.
   + */
   +if (vdev-status  VIRTIO_CONFIG_S_FEATURES_OK) {
   +return -EINVAL;
   +}
  
  Hmm, according to your patch description, the FEATURES_OK check only
  applies to virtio-1.0 devices ... so shouldn't there be a check for
  virtio-1 here? Or did I miss something?
 
 A device in legacy mode will never have FEATURES_OK set. But it is a
 bit non-obvious - maybe adding a check for VERSION_1 does not hurt.

Ah, ok, right, and if it is a legacy device and has FEATURES_OK set, it
is certainly a misbehavior wrt the legacy protocol. So it really should
be ok or even good to _not_ check for virtio-1.0 here. So sorry for the
confusion, I think now the patch is good as it is:

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v6 04/20] virtio: add feature checking helpers

2014-12-11 Thread Thomas Huth 
On Thu, 11 Dec 2014 14:25:06 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 Add a helper function for checking whether a bit is set in the guest
 features for a vdev as well as one that works on a feature bit set.
 
 Convert code that open-coded this: It cleans up the code and makes it
 easier to extend the guest feature bits.
 
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
...
 diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
 index ef48550..56c92fb 100644
 --- a/hw/scsi/virtio-scsi.c
 +++ b/hw/scsi/virtio-scsi.c
 @@ -144,7 +144,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
   *
   * TODO: always disable this workaround for virtio 1.0 devices.
   */
 -if ((vdev-guest_features  VIRTIO_F_ANY_LAYOUT) == 0) {
 +if (!virtio_has_feature(vdev, VIRTIO_F_ANY_LAYOUT)) {

Wait ... this does not only look like a clean-up, but also like a
bug-fix to me, since it should have been (1  VIRTIO_F_ANY_LAYOUT)
instead of VIRTIO_F_ANY_LAYOUT in the original code instead?

So in case this patch queue takes a little bit longer 'til it gets
upstream, do we might want to submit a separate patch for fixing this
issue first?

  req_size = req-elem.out_sg[0].iov_len;
  resp_size = req-elem.in_sg[0].iov_len;
  }
 @@ -748,7 +748,7 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice 
 *dev, SCSISense sense)
  VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
  VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
 -if (((vdev-guest_features  VIRTIO_SCSI_F_CHANGE)  1) 
 +if (virtio_has_feature(vdev, VIRTIO_SCSI_F_CHANGE) 
  dev-type != TYPE_ROM) {
  virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE,
 sense.asc | (sense.ascq  8));
 @@ -769,7 +769,7 @@ static void virtio_scsi_hotplug(HotplugHandler 
 *hotplug_dev, DeviceState *dev,
  blk_op_block_all(sd-conf.blk, s-blocker);
  }
 
 -if ((vdev-guest_features  VIRTIO_SCSI_F_HOTPLUG)  1) {
 +if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
  virtio_scsi_push_event(s, sd,
 VIRTIO_SCSI_T_TRANSPORT_RESET,
 VIRTIO_SCSI_EVT_RESET_RESCAN);
 @@ -783,7 +783,7 @@ static void virtio_scsi_hotunplug(HotplugHandler 
 *hotplug_dev, DeviceState *dev,
  VirtIOSCSI *s = VIRTIO_SCSI(vdev);
  SCSIDevice *sd = SCSI_DEVICE(dev);
 
 -if ((vdev-guest_features  VIRTIO_SCSI_F_HOTPLUG)  1) {
 +if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
  virtio_scsi_push_event(s, sd,
 VIRTIO_SCSI_T_TRANSPORT_RESET,
 VIRTIO_SCSI_EVT_RESET_REMOVED);
...
 diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
 index 2fede2e..f6c0379 100644
 --- a/include/hw/virtio/virtio.h
 +++ b/include/hw/virtio/virtio.h
 @@ -278,6 +278,17 @@ static inline void virtio_clear_feature(uint32_t 
 *features, unsigned int fbit)
  *features = ~(1  fbit);
  }
 
 +static inline bool __virtio_has_feature(uint32_t features, unsigned int fbit)
 +{
 +assert(fbit  32);
 +return !!(features  (1  fbit));
 +}
 +
 +static inline bool virtio_has_feature(VirtIODevice *vdev, unsigned int fbit)
 +{
 +return __virtio_has_feature(vdev-guest_features, fbit);
 +}
 +

I've got to say that I'm a little bit unhappy with the naming of the
functions - and in contrast to the Linux kernel code, I think it is
also quite uncommon in the QEMU sources to use function names with
double underscores at the beginning.

Could you maybe rename the second function to virtio_vdev_has_feature
instead? And then remove the double underscores from the first function?

 Thomas

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC v6 03/20] virtio: feature bit manipulation helpers

2014-12-11 Thread Thomas Huth 
On Thu, 11 Dec 2014 14:25:05 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 Add virtio_{add,clear}_feature helper functions for manipulating a
 feature bits variable. This has some benefits over open coding:
 - add check that the bit is in a sane range
 - make it obvious at a glance what is going on
 - have a central point to change when we want to extend feature bits
 
 Convert existing code manipulating features to use the new helpers.
 
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 ---
  hw/9pfs/virtio-9p-device.c  |2 +-
  hw/block/virtio-blk.c   |   16 
  hw/char/virtio-serial-bus.c |2 +-
  hw/net/virtio-net.c |   34 +-
  hw/s390x/virtio-ccw.c   |4 ++--
  hw/virtio/virtio-mmio.c |2 +-
  hw/virtio/virtio-pci.c  |4 ++--
  include/hw/virtio/virtio.h  |   12 
  8 files changed, 44 insertions(+), 32 deletions(-)

Patch looks fine to me.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio_net: Fixed a trivial typo (fitler -- filter)

2013-11-29 Thread Thomas Huth 
MAC filter sounds more reasonable than MAC fitler.

Signed-off-by: Thomas Huth th...@linux.vnet.ibm.com
---
 drivers/net/virtio_net.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 01f4eb5..fd96f09 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1075,7 +1075,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
  VIRTIO_NET_CTRL_MAC_TABLE_SET,
  sg, NULL))
-   dev_warn(dev-dev, Failed to set MAC fitler table.\n);
+   dev_warn(dev-dev, Failed to set MAC filter table.\n);
 
kfree(buf);
 }
-- 
1.7.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio-net: Set RXCSUM feature if GUEST_CSUM is available

2013-08-27 Thread Thomas Huth 
If the VIRTIO_NET_F_GUEST_CSUM virtio feature is available, the guest
does not have to calculate the checksums on all received packets. This
is pretty much the same feature as RX checksum offloading on real
network cards, so the virtio-net driver should report this by setting
the NETIF_F_RXCSUM flag. When the user now runs ethtool -k, he or she
can see whether the virtio-net interface has to calculate RX checksums
or not.

Signed-off-by: Thomas Huth th...@linux.vnet.ibm.com
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f216002..defec2b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1538,6 +1538,8 @@ static int virtnet_probe(struct virtio_device *vdev)
dev-features |= dev-hw_features  
(NETIF_F_ALL_TSO|NETIF_F_UFO);
/* (!csum  gso) case will be fixed by register_netdev() */
}
+   if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
+   dev-features |= NETIF_F_RXCSUM;
 
dev-vlan_features = dev-features;
 
-- 
1.8.1.4

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Virtio-net, ethtool and rx-checksumming

2013-08-23 Thread Thomas Huth 

 Hi all,

when running ethtool -k ethX on a virtio-net device, it always
reports rx-checksumming: off with recent kernels. With older kernels
(e.g. 2.6.32), it used to report on instead, so this difference
caused some confusion here (do newer guest kernels have to compute the
RX checksums now? That's certainly a bug / performance problem ...).

However, after inspecting the code a little bit, it seems to me that
this ethtool setting is not taken care of by the virtio_net driver at
all, so the values reported by ethtool seem to be rather arbitrary than
on purpose.

Now my question: Would it make sense to map the rx-checksumming
reporting to the VIRTIO_NET_F_GUEST_CSUM feature bit, so that the user
of the guest system can see whether the guest OS has to calculate RX
checksums or not? I guess it does not make sense to also make this
configurable during run-time, but IMHO it really would be good if at
least the reporting was available.

 Regards,
  Thomas Huth

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization