Re: A set of standard virtual devices?

[Cornelia Huck]

On Tue, 3 Apr 2007 11:41:49 +0200,
Arnd Bergmann [EMAIL PROTECTED] wrote:

 On Tuesday 03 April 2007, H. Peter Anvin wrote:
  However, one probably wants to think about what the heck one actually 
  means with virtualization in the absence of a lot of this stuff.  PCI 
  is probably the closest thing we have to a lowest common denominator for 
  device detection.
 
 I think that's true outside of s390, but a standardized virtual device
 interface should be able to work there as well. Interestingly, the
 s390 channel I/O also uses two 16 bit numbers to identify a device
 (type and model), just like PCI or USB, so in that light, we might
 be able to use the same number space for something entirely different
 depending on the virtual bus.

Even if we used those ids for cu_type and dev_type, it would still be
ugly IMO. It would be much cleaner to just define a very simple, easy
to implement virtual bus without dragging implementation details for
other types of devices around.

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

Re: A set of standard virtual devices?

[Cornelia Huck]

On Tue, 3 Apr 2007 11:26:52 +0200,
Andi Kleen [EMAIL PROTECTED] wrote:

  
  On s390, it would be more than strangeness. There's no implementation
  of PCI at all, someone would have to cook it up - and it wouldn't have
  any use beyond those special devices. Since there isn't any bus type
  that is available on *all* architectures, a generic virtual bus with
  very simple probing seems much saner...
 
 You just have to change all the distribution installers then. 
 Ok I suppose on s390 that's not that big issue because there are not
 that many for s390. But for x86 there exist quite a lot. I suppose
 it's easier to change it in the kernel.

Huh? I don't follow you here. Why should this be easier for s390 vs.
x86? (And since there seems to be a trend to use HAL as a device
discovery tool recently: A new bus type is easy enough to add there.)

And I really think we should have a clean design in the kernel instead
of trying to wedge virtual devices into a known system. Exposing
virtual devices (which may be handled totally differently) as PCI
devices just seems hackish to me.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 5/6] kvm-s390: use register_virtio_root_device()

[Cornelia Huck]

On Thu, 11 Dec 2008 10:05:49 +0100,
Christian Borntraeger [EMAIL PROTECTED] wrote:

 Am Mittwoch, 10. Dezember 2008 schrieb Mark McLoughlin:
  This is basically a no-op change, since it does exactly the
  same thing as s390_root_dev_register() when the caller isn't
  a module.
 
 Ok, I gave it a short test and it seems to work.
 Some comments: I agree with your comment in patch0, that a generic 
 device_register_root() function might be useful. 

Indeed, if this is a simple replacement, we want a generic function.
I'll take a look at the patches.

 
  --- a/drivers/s390/kvm/kvm_virtio.c
  +++ b/drivers/s390/kvm/kvm_virtio.c
 [...]
  -   kvm_root = s390_root_dev_register(kvm_s390);
  +   kvm_root = register_virtio_root_device(kvm_s390);
 [...]
  -   s390_root_dev_unregister(kvm_root);
  +   unregister_virtio_root_device(kvm_root);
 
 You can now remove the include asm/s390_rdev.h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/6] virtio: add register_virtio_root_device()

[Cornelia Huck]

On Wed, 10 Dec 2008 17:45:35 +,
Mark McLoughlin [EMAIL PROTECTED] wrote:

 Add a function to allocate a root device object to group the
 devices from a given virtio implementation.
 
 Also add a 'module' sysfs symlink to allow so that userspace
 can generically determine which virtio implementation a
 device is associated with. This will be used by Fedora
 mkinitrd to generically determine e.g. that virtio_pci is
 needed to mount a given root filesystem.

Nothing about this is really virtio-specific (just as
s390_root_dev_register() is not really s390-specific), and a 'module'
symlink doesn't really hurt in a generic implementation, even if it is
unneeded. I'm voting to put this in some generic, always built-in code
(or have the users select it) so we could also use it from s390.

 
 Signed-off-by: Mark McLoughlin [EMAIL PROTECTED]
 ---
  drivers/virtio/virtio.c |   71 
 +++
  include/linux/virtio.h  |   10 ++
  2 files changed, 81 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
 index 018c070..61e6597 100644
 --- a/drivers/virtio/virtio.c
 +++ b/drivers/virtio/virtio.c
 @@ -1,6 +1,7 @@
  #include linux/virtio.h
  #include linux/spinlock.h
  #include linux/virtio_config.h
 +#include linux/err.h
 
  /* Unique numbering for virtio devices. */
  static unsigned int dev_index;
 @@ -200,6 +201,76 @@ void unregister_virtio_device(struct virtio_device *dev)
  }
  EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
 +/* A root device for virtio devices from a given backend.  This makes them
 + * appear as /sys/devices/{name}/0,1,2 not /sys/devices/0,1,2. It also allows
 + * us to have a /sys/devices/{name}/module symlink to the backend module. */
 +struct virtio_root_device
 +{
 + struct device dev;
 + struct module *owner;
 +};
 +
 +static struct virtio_root_device *to_virtio_root(struct device *dev)
 +{
 +return container_of(dev, struct virtio_root_device, dev);
 +}
 +
 +static void release_virtio_root_device(struct device *dev)
 +{
 + struct virtio_root_device *root = to_virtio_root(dev);
 + if (root-owner)
 + sysfs_remove_link(root-dev.kobj, module);
 + kfree(root);
 +}

Can this code be a module? If yes, move the release callback to a
build-in as there are races with release-functions in modules.

 +
 +struct device *__register_virtio_root_device(const char *name,
 +  struct module *owner)
 +{
 + struct virtio_root_device *root;
 + int err = -ENOMEM;
 +
 + root = kzalloc(sizeof(struct virtio_root_device), GFP_KERNEL);
 + if (!root)
 + goto out;
 +
 + err = dev_set_name(root-dev, name);
 + if (err)
 + goto free_root;
 +
 + err = device_register(root-dev);
 + if (err)
 + goto free_root;
 +
 + root-dev.parent  = NULL;
 + root-dev.release = release_virtio_root_device;

You must set -release before calling device_register(), and setting
the parent is unneeded.

 +
 + if (owner) {
 + struct module_kobject *mk = owner-mkobj;
 +
 + err = sysfs_create_link(root-dev.kobj, mk-kobj, module);
 +if (err) {
 + device_unregister(root-dev);
 + return ERR_PTR(err);
 + }
 +
 + root-owner = owner;
 + }
 +
 + return root-dev;
 +
 +free_root:
 + kfree(root);

You need to call device_put() if you called device_register().

 +out:
 + return ERR_PTR(err);
 +}
 +EXPORT_SYMBOL_GPL(__register_virtio_root_device);
 +
 +void unregister_virtio_root_device(struct device *root)
 +{
 + device_unregister(root);
 +}
 +EXPORT_SYMBOL_GPL(unregister_virtio_root_device);
 +
  static int virtio_init(void)
  {
   if (bus_register(virtio_bus) != 0)
 diff --git a/include/linux/virtio.h b/include/linux/virtio.h
 index 06005fa..66e6c67 100644
 --- a/include/linux/virtio.h
 +++ b/include/linux/virtio.h
 @@ -93,6 +93,16 @@ struct virtio_device
  int register_virtio_device(struct virtio_device *dev);
  void unregister_virtio_device(struct virtio_device *dev);
 
 +/* A root device is a dummy device used to group virtio devices from each
 + * implementation. */
 +struct device *__register_virtio_root_device(const char *name,
 +  struct module *owner);
 +static inline struct device *register_virtio_root_device(const char *name)
 +{
 + return __register_virtio_root_device(name, THIS_MODULE);
 +}
 +void unregister_virtio_root_device(struct device *root);
 +
  /**
   * virtio_driver - operations for a virtio I/O driver
   * @driver: underlying device driver (populate name and owner).
 -- 
 1.5.4.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] driver core: add root_device_register()

[Cornelia Huck]

On Thu, 11 Dec 2008 16:16:53 +,
Mark McLoughlin [EMAIL PROTECTED] wrote:

 Add support for allocating root device objects which group
 device objects under /sys/devices directories.
 
 Also add a sysfs 'module' symlink which points to the owner
 of the root device object. This will be used in virtio to
 allow userspace to determine which virtio bus implementation
 a given device is associated with.

I was just hacking up a similar patch :)

 
 Signed-off-by: Mark McLoughlin [EMAIL PROTECTED]
 ---
  drivers/base/core.c|   88 
 
  include/linux/device.h |   11 ++
  2 files changed, 99 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/base/core.c b/drivers/base/core.c
 index 8c2cc26..db160a2 100644
 --- a/drivers/base/core.c
 +++ b/drivers/base/core.c
 @@ -1196,6 +1196,94 @@ EXPORT_SYMBOL_GPL(put_device);
  EXPORT_SYMBOL_GPL(device_create_file);
  EXPORT_SYMBOL_GPL(device_remove_file);
 
 +struct root_device
 +{
 + struct device dev;
 + struct module *owner;
 +};
 +
 +static void root_device_release(struct device *dev)
 +{
 + struct root_device *root = container_of(dev, struct root_device, dev);
 +
 + if (root-owner)
 + sysfs_remove_link(root-dev.kobj, module);

I'd rather remove the link before you unregister.

 +
 + kfree(root);
 +}
 +
 +/**
 + * __root_device_register - allocate and register a root device
 + * @name: root device name
 + * @owner: owner module of the root device, usually THIS_MODULE
 + *
 + * This function allocates a root device and registers it
 + * using device_register(). In order to free the returned
 + * device, use root_device_unregister().
 + *
 + * Root devices are dummy devices which allow other devices
 + * to be grouped under /sys/devices. Use this function to
 + * allocate a root device and then use it as the parent of
 + * any device which should appear under /sys/devices/{name}
 + *
 + * The /sys/devices/{name} directory will also contain a
 + * 'module' symlink which points to the @owner directory
 + * in sysfs.

* Note: You probably want to use root_device_register().

 + */
 +struct device *__root_device_register(const char *name, struct module *owner)
 +{
 + struct root_device *root;
 + int err = -ENOMEM;
 +
 + root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
 + if (!root)
 + return ERR_PTR(err);
 +
 + err = dev_set_name(root-dev, name);
 + if (err) {
 + kfree(root);
 + return ERR_PTR(err);
 + }
 +
 + root-dev.release = root_device_release;
 +
 + err = device_register(root-dev);
 + if (err) {
 + put_device(root-dev);
 + return ERR_PTR(err);
 + }
 +
 + if (owner) {
 + struct module_kobject *mk = owner-mkobj;
 +
 + err = sysfs_create_link(root-dev.kobj, mk-kobj, module);
 + if (err) {;

Stray ';'

 + device_unregister(root-dev);
 + return ERR_PTR(err);
 + }
 + root-owner = owner;
 + }
 +
 + return root-dev;
 +}
 +EXPORT_SYMBOL_GPL(__root_device_register);
 +
 +/**
 + * root_device_unregister - unregister and free a root device
 + * @root: device going away.
 + *
 + * We simply release @root using device_unregister(). If @root
 + * has a reference count of one, the device will be freed
 + * after it has been unregistered. Otherwise, the structure
 + * will stick around until the final reference is dropped
 + * using put_device().

I don't think you'll need to explain device handling here. How about
this:

root_device_unregister - unregister a root device
@root: device going away

This function unregisters and cleans up a device that was created by
root_device_register().

 + */
 +void root_device_unregister(struct device *root)
 +{

Clean up the symlink here.

 + device_unregister(root);
 +}
 +EXPORT_SYMBOL_GPL(root_device_unregister);
 +
 
  static void device_create_release(struct device *dev)
  {
 diff --git a/include/linux/device.h b/include/linux/device.h
 index 1a3686d..9e02980 100644
 --- a/include/linux/device.h
 +++ b/include/linux/device.h
 @@ -483,6 +483,17 @@ extern int device_rename(struct device *dev, char 
 *new_name);
  extern int device_move(struct device *dev, struct device *new_parent);
 
  /*
 + * Root device objects for grouping under /sys/devices
 + */
 +extern struct device *__root_device_register(const char *name,
 +  struct module *owner);
 +static inline struct device *root_device_register(const char *name)
 +{
 + return __root_device_register(name, THIS_MODULE);
 +}
 +extern void root_device_unregister(struct device *root);
 +
 +/*
   * Manual binding of a device to driver. See drivers/base/bus.c
   * for information on use.
   */
 -- 
 1.5.4.3
 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org

Re: [PATCH 1/4] driver core: add root_device_register()

[Cornelia Huck]

On Fri, 12 Dec 2008 08:56:49 +,
Mark McLoughlin mar...@redhat.com wrote:

 Yeah, I just figured it was a little overkill given the structure
 definition is three lines away. Here it is, though.

As you use it twice, I think it makes the code more readable.

 
 Cheers,
 Mark.
 
 From: Mark McLoughlin mar...@redhat.com
 Subject: [PATCH] driver core: add root_device_register()
 
 Add support for allocating root device objects which group
 device objects under /sys/devices directories.
 
 Also add a sysfs 'module' symlink which points to the owner
 of the root device object. This symlink will be used in virtio
 to allow userspace to determine which virtio bus implementation
 a given device is associated with.
 
 [Includes suggestions from Cornelia Huck]
 
 Signed-off-by: Mark McLoughlin mar...@redhat.com
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com

 ---
  drivers/base/core.c|   89 
 
  include/linux/device.h |   11 ++
  2 files changed, 100 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/base/core.c b/drivers/base/core.c
 index 8c2cc26..05320af 100644
 --- a/drivers/base/core.c
 +++ b/drivers/base/core.c
 @@ -1196,6 +1196,95 @@ EXPORT_SYMBOL_GPL(put_device);
  EXPORT_SYMBOL_GPL(device_create_file);
  EXPORT_SYMBOL_GPL(device_remove_file);
 
 +struct root_device
 +{
 + struct device dev;
 + struct module *owner;
 +};
 +
 +#define to_root_device(dev) container_of(dev, struct root_device, dev)
 +
 +static void root_device_release(struct device *dev)
 +{
 + kfree(to_root_device(dev));
 +}
 +
 +/**
 + * __root_device_register - allocate and register a root device
 + * @name: root device name
 + * @owner: owner module of the root device, usually THIS_MODULE
 + *
 + * This function allocates a root device and registers it
 + * using device_register(). In order to free the returned
 + * device, use root_device_unregister().
 + *
 + * Root devices are dummy devices which allow other devices
 + * to be grouped under /sys/devices. Use this function to
 + * allocate a root device and then use it as the parent of
 + * any device which should appear under /sys/devices/{name}
 + *
 + * The /sys/devices/{name} directory will also contain a
 + * 'module' symlink which points to the @owner directory
 + * in sysfs.
 + *
 + * Note: You probably want to use root_device_register().
 + */
 +struct device *__root_device_register(const char *name, struct module *owner)
 +{
 + struct root_device *root;
 + int err = -ENOMEM;
 +
 + root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
 + if (!root)
 + return ERR_PTR(err);
 +
 + err = dev_set_name(root-dev, name);
 + if (err) {
 + kfree(root);
 + return ERR_PTR(err);
 + }
 +
 + root-dev.release = root_device_release;
 +
 + err = device_register(root-dev);
 + if (err) {
 + put_device(root-dev);
 + return ERR_PTR(err);
 + }
 +
 + if (owner) {
 + struct module_kobject *mk = owner-mkobj;
 +
 + err = sysfs_create_link(root-dev.kobj, mk-kobj, module);
 + if (err) {
 + device_unregister(root-dev);
 + return ERR_PTR(err);
 + }
 + root-owner = owner;
 + }
 +
 + return root-dev;
 +}
 +EXPORT_SYMBOL_GPL(__root_device_register);
 +
 +/**
 + * root_device_unregister - unregister and free a root device
 + * @root: device going away.
 + *
 + * This function unregisters and cleans up a device that was created by
 + * root_device_register().
 + */
 +void root_device_unregister(struct device *dev)
 +{
 + struct root_device *root = to_root_device(dev);
 +
 + if (root-owner)
 + sysfs_remove_link(root-dev.kobj, module);
 +
 + device_unregister(dev);
 +}
 +EXPORT_SYMBOL_GPL(root_device_unregister);
 +
 
  static void device_create_release(struct device *dev)
  {
 diff --git a/include/linux/device.h b/include/linux/device.h
 index 1a3686d..9e02980 100644
 --- a/include/linux/device.h
 +++ b/include/linux/device.h
 @@ -483,6 +483,17 @@ extern int device_rename(struct device *dev, char 
 *new_name);
  extern int device_move(struct device *dev, struct device *new_parent);
 
  /*
 + * Root device objects for grouping under /sys/devices
 + */
 +extern struct device *__root_device_register(const char *name,
 +  struct module *owner);
 +static inline struct device *root_device_register(const char *name)
 +{
 + return __root_device_register(name, THIS_MODULE);
 +}
 +extern void root_device_unregister(struct device *root);
 +
 +/*
   * Manual binding of a device to driver. See drivers/base/bus.c
   * for information on use.
   */
 -- 
 1.6.0.3
 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/4] s390: remove s390_root_dev_*()

[Cornelia Huck]

On Thu, 11 Dec 2008 18:00:25 +0100,
Cornelia Huck cornelia.h...@de.ibm.com wrote:
 On Thu, 11 Dec 2008 16:16:56 +,
 Mark McLoughlin mar...@redhat.com wrote:
 
  Replace s390_root_dev_register() with root_device_register() etc.
 
 Nice, one more special case generalized :) I'll give it a run.

You missed one occurrence in iucv (see below); with that patch applied,
everything seems to work as expected, both for root devices created by
built-ins and by modules.

---
 net/iucv/iucv.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.orig/net/iucv/iucv.c
+++ linux-2.6/net/iucv/iucv.c
@@ -1682,7 +1682,7 @@ static void __exit iucv_exit(void)
kfree(iucv_irq_data[cpu]);
iucv_irq_data[cpu] = NULL;
}
-   s390_root_dev_unregister(iucv_root);
+   root_device_unregister(iucv_root);
bus_unregister(iucv_bus);
unregister_external_interrupt(0x4000, iucv_external_interrupt);
 }
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/4] s390: remove s390_root_dev_*()

[Cornelia Huck]

On Fri, 12 Dec 2008 09:35:46 +,
Mark McLoughlin mar...@redhat.com wrote:

 From: Mark McLoughlin mar...@redhat.com
 Subject: [PATCH] s390: remove s390_root_dev_*()
 
 Replace s390_root_dev_register() with root_device_register() etc.
 
 [Includes fix from Cornelia Huck]
 
 Signed-off-by: Mark McLoughlin mar...@redhat.com

Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: Proposal for virtio standardization.

[Cornelia Huck]

On Thu, 27 Sep 2012 09:59:33 +0930
Rusty Russell ru...@rustcorp.com.au wrote:

 Hi all,
 
   I've had several requests for a more formal approach to the
 virtio draft spec, and (after some soul-searching) I'd like to try that.
 
   The proposal is to use OASIS as the standards body, as it's
 fairly light-weight as these things go.  For me this means paperwork and
 setting up a Working Group and getting the right people involved as
 Voting members starting with the current contributors; for most of you
 it just means a new mailing list, though I'll be cross-posting any
 drafts and major changes here anyway.
 
   I believe that a documented standard (aka virtio 1.0) will
 increase visibility and adoption in areas outside our normal linux/kvm
 universe.  There's been some of that already, but this is the clearest
 path to accelerate it.  Not the easiest path, but I believe that a solid
 I/O standard is a Good Thing for everyone.
 
   Yet I also want to decouple new and experimental development
 from the standards effort; running code comes first.  New feature bits
 and new device numbers should be reservable without requiring a full
 spec change.
 
 So the essence of my proposal is:
 1) I start a Working Group within OASIS where we can aim for virtio spec
1.0.
 
 2) The current spec is textually reordered so the core is clearly
bus-independent, with PCI, mmio, etc appendices.
 
 3) Various clarifications, formalizations and cleanups to the spec text,
and possibly elimination of old deprecated features.
 
 4) The only significant change to the spec is that we use PCI
capabilities, so we can have infinite feature bits.
(see
 http://lists.linuxfoundation.org/pipermail/virtualization/2011-December/019198.html)

Infinite only applies to virtio-pci, no?

 
 5) Changes to the ring layout and other such things are deferred to a
future virtio version; whether this is done within OASIS or
externally depends on how well this works for the 1.0 release.
 
 Thoughts?
 Rusty.
 

Sounds like a good idea. I'll be happy to review the spec with an eye
to virtio-ccw.

Cornelia

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

Re: [PATCH 0/3] virtio-net: inline header support

[Cornelia Huck]

On Fri, 12 Oct 2012 09:07:46 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 Michael S. Tsirkin m...@redhat.com writes:
  On Thu, Oct 11, 2012 at 10:33:31AM +1030, Rusty Russell wrote:
  OK.  Well, Anthony wants qemu to be robust in this regard, so I am
  tempted to rework all the qemu drivers to handle arbitrary layouts.
  They could use a good audit anyway.
 
  I agree here. Still trying to understand whether we can agree to use
  a feature bit for this, or not.
 
 I'd *like* to imply it by the new PCI layout, but if it doesn't work
 we'll add a new feature bit.
 
 I'm resisting a feature bit, since it constrains future implementations
 which could otherwise assume it.
 
  This would become a glaring exception, but I'm tempted to fix it to 32
  bytes at the same time as we get the new pci layout (ie. for the virtio
  1.0 spec).
 
  But this isn't a virtio-pci only issue, is it?
  qemu has s390 bus with same limmitation.
  How can we tie it to pci layout?
 
 They can use a transport feature if they need to, of course.  But
 perhaps the timing with ccw will coincide with the fix, in which they
 don't need to, but it might be a bit late.
 
 Cornelia?

My virtio-ccw host code is still going through a bit of rework, so it
might well go in after the fix.

There's also the existing (non-spec'ed) s390-virtio transport. While it
will likely be deprecated sometime in the future, it should probably
get a feature bit for consistency's sake.

 
 Cheers,
 Rusty.
 

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

[PATCH] virtio: Don't access index after unregister.

[Cornelia Huck]

Virtio wants to release used indices after the corresponding
virtio device has been unregistered. However, virtio does not
hold an extra reference, giving up its last reference with
device_unregister(), making accessing dev-index afterwards
invalid.

I actually saw problems when testing my (not-yet-merged)
virtio-ccw code:

- device_add virtio-net,id=xxx
- creates device virtion with n0

- device_del xxx
- deletes virtion, but calls ida_simple_remove with an
   index of 0

- device_add virtio-net,id=xxx
- tries to add virtio0, which is still in use...

So let's save the index we want to release before calling
device_unregister().

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/virtio/virtio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 1e8659c..809b0de 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
 
 void unregister_virtio_device(struct virtio_device *dev)
 {
+   int index = dev-index; /* save for after device release */
+
device_unregister(dev-dev);
-   ida_simple_remove(virtio_index_ida, dev-index);
+   ida_simple_remove(virtio_index_ida, index);
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
-- 
1.7.12.4

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

Re: [PATCH v3 0/5] kvm: Make ioeventfd usable on s390.

[Cornelia Huck]

On Tue, 26 Feb 2013 13:13:39 +0100
Christian Borntraeger borntrae...@de.ibm.com wrote:

 On 26/02/13 12:18, Michael S. Tsirkin wrote:
  virtio_ccw: pass a cookie value to kvm hypercall
  
  Lookups by channel/vq pair on host during virtio notifications might be
  expensive.  Interpret hypercall return value as a cookie which host can
  use to do device lookups for the next notification more efficiently.
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  
  ---
  
  diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
  index 2029b6c..1054f3a 100644
  --- a/drivers/s390/kvm/virtio_ccw.c
  +++ b/drivers/s390/kvm/virtio_ccw.c
  @@ -77,6 +77,7 @@ struct virtio_ccw_vq_info {
  void *queue;
  struct vq_info_block *info_block;
  struct list_head node;
  +   long cookie;
   };
  
   #define KVM_VIRTIO_CCW_RING_ALIGN 4096
  @@ -145,15 +146,18 @@ static int ccw_io_helper(struct virtio_ccw_device 
  *vcdev,
   }
  
   static inline long do_kvm_notify(struct subchannel_id schid,
  -unsigned long queue_index)
  +unsigned long queue_index,
  +long cookie)
   {
  register unsigned long __nr asm(1) = KVM_S390_VIRTIO_CCW_NOTIFY;
  register struct subchannel_id __schid asm(2) = schid;
  register unsigned long __index asm(3) = queue_index;
  register long __rc asm(2);
  +   register long __cookie asm(4) = cookie;
  
  asm volatile (diag 2,4,0x500\n
  - : =d (__rc) : d (__nr), d (__schid), d (__index)
  + : =d (__rc) : d (__nr), d (__schid), d (__index),
  + d(__cookie)
: memory, cc);
  return __rc;
   }
  @@ -166,7 +170,7 @@ static void virtio_ccw_kvm_notify(struct virtqueue *vq)
  
  vcdev = to_vc_device(info-vq-vdev);
  ccw_device_get_schid(vcdev-cdev, schid);
  -   do_kvm_notify(schid, virtqueue_get_queue_index(vq));
  +   info-cookie = do_kvm_notify(schid, virtqueue_get_queue_index(vq), 
  info-cookie);
   }
  
   static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev,
 
 
 Hmmm, forget my last mail. This actually could be even forward and backward 
 compatible.
 In the virtio spec we will not define the cookie format (just 64bit int). 
 That will allow
 qemu or future kernels to use that for other things (as long as a validity 
 check is 
 possible) if we dont have a kvm bus.
 
 Now:
 
 old guest, old host: 
 works.
 
 old guest, new host: 
 the cookie from the guest contains junk, the host needs to detect that the 
 cookie is 
 junk and ignores it. It will return the new cookie anyway. 
 
 new guest, old host:
 The guest will get a junk cookie and pass it back to the host. But the host 
 will ignore
 it anyway.
 
 new guest, new host:
 works.
 
 So...
 Acked-by: Christian Borntraeger borntrae...@de.ibm.com

Yes, that sounds sane; I'll give it a try later.

However, I'd rather not want to rush this; I'd prefer to get the
initial version in first.

I'll do a v4 later.

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

Re: [PATCH v3 0/5] kvm: Make ioeventfd usable on s390.

[Cornelia Huck]

On Tue, 26 Feb 2013 15:56:43 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Tue, Feb 26, 2013 at 02:29:07PM +0100, Cornelia Huck wrote:
  On Tue, 26 Feb 2013 13:13:39 +0100
  Christian Borntraeger borntrae...@de.ibm.com wrote:
  
   On 26/02/13 12:18, Michael S. Tsirkin wrote:
virtio_ccw: pass a cookie value to kvm hypercall

Lookups by channel/vq pair on host during virtio notifications might be
expensive.  Interpret hypercall return value as a cookie which host can
use to do device lookups for the next notification more efficiently.

Signed-off-by: Michael S. Tsirkin m...@redhat.com

---

diff --git a/drivers/s390/kvm/virtio_ccw.c 
b/drivers/s390/kvm/virtio_ccw.c
index 2029b6c..1054f3a 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -77,6 +77,7 @@ struct virtio_ccw_vq_info {
void *queue;
struct vq_info_block *info_block;
struct list_head node;
+   long cookie;
 };

 #define KVM_VIRTIO_CCW_RING_ALIGN 4096
@@ -145,15 +146,18 @@ static int ccw_io_helper(struct virtio_ccw_device 
*vcdev,
 }

 static inline long do_kvm_notify(struct subchannel_id schid,
-unsigned long queue_index)
+unsigned long queue_index,
+long cookie)
 {
register unsigned long __nr asm(1) = 
KVM_S390_VIRTIO_CCW_NOTIFY;
register struct subchannel_id __schid asm(2) = schid;
register unsigned long __index asm(3) = queue_index;
register long __rc asm(2);
+   register long __cookie asm(4) = cookie;

asm volatile (diag 2,4,0x500\n
- : =d (__rc) : d (__nr), d (__schid), d 
(__index)
+ : =d (__rc) : d (__nr), d (__schid), d 
(__index),
+ d(__cookie)
  : memory, cc);
return __rc;
 }
@@ -166,7 +170,7 @@ static void virtio_ccw_kvm_notify(struct virtqueue 
*vq)

vcdev = to_vc_device(info-vq-vdev);
ccw_device_get_schid(vcdev-cdev, schid);
-   do_kvm_notify(schid, virtqueue_get_queue_index(vq));
+   info-cookie = do_kvm_notify(schid, 
virtqueue_get_queue_index(vq), info-cookie);
 }

 static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev,
   
   
   Hmmm, forget my last mail. This actually could be even forward and 
   backward compatible.
   In the virtio spec we will not define the cookie format (just 64bit int). 
   That will allow
   qemu or future kernels to use that for other things (as long as a 
   validity check is 
   possible) if we dont have a kvm bus.
   
   Now:
   
   old guest, old host: 
   works.
   
   old guest, new host: 
   the cookie from the guest contains junk, the host needs to detect that 
   the cookie is 
   junk and ignores it. It will return the new cookie anyway. 
   
   new guest, old host:
   The guest will get a junk cookie and pass it back to the host. But the 
   host will ignore
   it anyway.
   
   new guest, new host:
   works.
   
   So...
   Acked-by: Christian Borntraeger borntrae...@de.ibm.com
  
  Yes, that sounds sane; I'll give it a try later.
  
  However, I'd rather not want to rush this; I'd prefer to get the
  initial version in first.
 
 Well planning to obsolete an interface from the start sounds wrong
 somehow. We could always drop ccw in 3.9 if we feel we need more
 time, but to me, this looks like a minor enough change to do even after
 the merge window closed.

This was aimed at the exploitation; I'm fine with this patch for 3.9
after we let it mature for a day or two.

 
 Want me to write you a spec patch too?

That would be great.

 
  I'll do a v4 later.
 
 Right, just return 0 and it'll work.
 

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

Re: [PATCH 03/22] virtio_config: make transports implement accessors.

[Cornelia Huck]

On Thu, 21 Mar 2013 18:59:24 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 All transports just pass through at the moment.
 
 Cc: Ohad Ben-Cohen o...@wizery.com
 Cc: Brian Swetland swetl...@google.com
 Cc: Cornelia Huck cornelia.h...@de.ibm.com
 Cc: Pawel Moll pawel.m...@arm.com
 Cc: Christian Borntraeger borntrae...@de.ibm.com
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au
 ---
  drivers/lguest/lguest_device.c |   79 
 ++--
  drivers/net/caif/caif_virtio.c |2 +-
  drivers/s390/kvm/kvm_virtio.c  |   78 +--
  drivers/s390/kvm/virtio_ccw.c  |   39 +++-
  drivers/virtio/virtio_mmio.c   |   35 +-
  drivers/virtio/virtio_pci.c|   39 +---
  include/linux/virtio_config.h  |   70 +--
  7 files changed, 283 insertions(+), 59 deletions(-)
 

 diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
 index 6711e65..dcf35b1 100644
 --- a/drivers/s390/kvm/kvm_virtio.c
 +++ b/drivers/s390/kvm/kvm_virtio.c
 @@ -112,26 +112,82 @@ static void kvm_finalize_features(struct virtio_device 
 *vdev)
  }
 
  /*
 - * Reading and writing elements in config space
 + * Reading and writing elements in config space.  Host and guest are always
 + * big-endian, so no conversion necessary.
   */
 -static void kvm_get(struct virtio_device *vdev, unsigned int offset,
 -void *buf, unsigned len)
 +static u8 kvm_get8(struct virtio_device *vdev, unsigned int offset)
  {
 - struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
   ^^
 
This looks weird?

 
 - BUG_ON(offset + len  desc-config_len);
 - memcpy(buf, kvm_vq_configspace(desc) + offset, len);
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u8)  desc-config_len);
 + return *(u8 *)(kvm_vq_configspace(desc) + offset);
  }
 
 -static void kvm_set(struct virtio_device *vdev, unsigned int offset,
 -const void *buf, unsigned len)
 +static void kvm_set8(struct virtio_device *vdev, unsigned int offset, u8 val)
  {
 - struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u8 *)(kvm_vq_configspace(desc) + offset) = val;
 +}
 +
 +static u16 kvm_get16(struct virtio_device *vdev, unsigned int offset)
 +{
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u16)  desc-config_len);
 + return *(u16 *)(kvm_vq_configspace(desc) + offset);
 +}
 +
 +static void kvm_set16(struct virtio_device *vdev, unsigned int offset, u16 
 val)
 +{
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u16 *)(kvm_vq_configspace(desc) + offset) = val;
 +}
 +
 +static u32 kvm_get32(struct virtio_device *vdev, unsigned int offset)
 +{
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
 
 - BUG_ON(offset + len  desc-config_len);
 - memcpy(kvm_vq_configspace(desc) + offset, buf, len);
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u32)  desc-config_len);
 + return *(u32 *)(kvm_vq_configspace(desc) + offset);
  }
 
 +static void kvm_set32(struct virtio_device *vdev, unsigned int offset, u32 
 val)
 +{
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u32 *)(kvm_vq_configspace(desc) + offset) = val;
 +}
 +
 +static u64 kvm_get64(struct virtio_device *vdev, unsigned int offset)
 +{
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u64)  desc-config_len);
 + return *(u64 *)(kvm_vq_configspace(desc) + offset);
 +}
 +
 +static void kvm_set64(struct virtio_device *vdev, unsigned int offset, u64 
 val)
 +{
 + struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u64 *)(kvm_vq_configspace(desc) + offset) = val;
 +}
 +
 +

The new functions don't seem to be hooked up anywhere?

  /*
   * The operations to get and set the status word just access
   * the status field of the device descriptor. set_status will also
 diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
 index 2029b6c

Re: [PATCH 05/22] virtio: add support for 64 bit features.

[Cornelia Huck]

On Thu, 21 Mar 2013 18:59:26 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 Change the u32 to a u64, and make sure to use 1ULL everywhere!
 
 Cc: Ohad Ben-Cohen o...@wizery.com
 Cc: Brian Swetland swetl...@google.com
 Cc: Cornelia Huck cornelia.h...@de.ibm.com
 Cc: Pawel Moll pawel.m...@arm.com
 Cc: Christian Borntraeger borntrae...@de.ibm.com
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au
 ---
  drivers/char/virtio_console.c  |2 +-
  drivers/lguest/lguest_device.c |   10 +-
  drivers/remoteproc/remoteproc_virtio.c |6 +-
  drivers/s390/kvm/kvm_virtio.c  |   10 +-
  drivers/virtio/virtio.c|   12 ++--
  drivers/virtio/virtio_mmio.c   |   14 +-
  drivers/virtio/virtio_pci.c|5 ++---
  drivers/virtio/virtio_ring.c   |2 +-
  include/linux/virtio.h |2 +-
  include/linux/virtio_config.h  |8 
  tools/virtio/linux/virtio.h|2 +-
  tools/virtio/linux/virtio_config.h |2 +-
  12 files changed, 41 insertions(+), 34 deletions(-)
 

And a not-even-compiled change for virtio_ccw as well:

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index eb0616b..2ca6dc5 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -454,8 +454,17 @@ static void virtio_ccw_finalize_features(struct 
virtio_device *vdev)
vring_transport_features(vdev);
 
features-index = 0;
-   features-features = cpu_to_le32(vdev-features);
-   /* Write the feature bits to the host. */
+   features-features = cpu_to_le32((u32)vdev-features);
+   /* Write the first half of the feature bits to the host. */
+   ccw-cmd_code = CCW_CMD_WRITE_FEAT;
+   ccw-flags = 0;
+   ccw-count = sizeof(*features);
+   ccw-cda = (__u32)(unsigned long)features;
+   ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
+
+   features-index = 1;
+   features-features = cpu_to_le32(vdev-features  32);
+   /* Write the second half of the feature bits to the host. */
ccw-cmd_code = CCW_CMD_WRITE_FEAT;
ccw-flags = 0;
ccw-count = sizeof(*features);

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

Re: [PATCH 03/22] virtio_config: make transports implement accessors.

[Cornelia Huck]

On Fri, 22 Mar 2013 11:01:20 +1030
Rusty Russell ru...@rustcorp.com.au wrote:


 Nice understatement.  I guess you know where I cut  pasted from...
 
 Here is the updated version.

Looks sane to me.

 
 Thanks,
 Rusty.
 
 diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
 index 6711e65..9b8c527 100644
 --- a/drivers/s390/kvm/kvm_virtio.c
 +++ b/drivers/s390/kvm/kvm_virtio.c
 @@ -112,24 +112,79 @@ static void kvm_finalize_features(struct virtio_device 
 *vdev)
  }
 
  /*
 - * Reading and writing elements in config space
 + * Reading and writing elements in config space.  Host and guest are always
 + * big-endian, so no conversion necessary.
   */
 -static void kvm_get(struct virtio_device *vdev, unsigned int offset,
 -void *buf, unsigned len)
 +static u8 kvm_get8(struct virtio_device *vdev, unsigned int offset)
  {
   struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 
 - BUG_ON(offset + len  desc-config_len);
 - memcpy(buf, kvm_vq_configspace(desc) + offset, len);
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u8)  desc-config_len);
 + return *(u8 *)(kvm_vq_configspace(desc) + offset);
  }
 
 -static void kvm_set(struct virtio_device *vdev, unsigned int offset,
 -const void *buf, unsigned len)
 +static void kvm_set8(struct virtio_device *vdev, unsigned int offset, u8 val)
  {
   struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 
 - BUG_ON(offset + len  desc-config_len);
 - memcpy(kvm_vq_configspace(desc) + offset, buf, len);
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u8 *)(kvm_vq_configspace(desc) + offset) = val;
 +}
 +
 +static u16 kvm_get16(struct virtio_device *vdev, unsigned int offset)
 +{
 + struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u16)  desc-config_len);
 + return *(u16 *)(kvm_vq_configspace(desc) + offset);
 +}
 +
 +static void kvm_set16(struct virtio_device *vdev, unsigned int offset, u16 
 val)
 +{
 + struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u16 *)(kvm_vq_configspace(desc) + offset) = val;
 +}
 +
 +static u32 kvm_get32(struct virtio_device *vdev, unsigned int offset)
 +{
 + struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u32)  desc-config_len);
 + return *(u32 *)(kvm_vq_configspace(desc) + offset);
 +}
 +
 +static void kvm_set32(struct virtio_device *vdev, unsigned int offset, u32 
 val)
 +{
 + struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u32 *)(kvm_vq_configspace(desc) + offset) = val;
 +}
 +
 +static u64 kvm_get64(struct virtio_device *vdev, unsigned int offset)
 +{
 + struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(u64)  desc-config_len);
 + return *(u64 *)(kvm_vq_configspace(desc) + offset);
 +}
 +
 +static void kvm_set64(struct virtio_device *vdev, unsigned int offset, u64 
 val)
 +{
 + struct kvm_device_desc *desc = to_kvmdev(vdev)-desc;
 +
 + /* Check they didn't ask for more than the length of the config! */
 + BUG_ON(offset + sizeof(val)  desc-config_len);
 + *(u64 *)(kvm_vq_configspace(desc) + offset) = val;
  }
 
  /*
 @@ -278,8 +333,14 @@ static const char *kvm_bus_name(struct virtio_device 
 *vdev)
  static const struct virtio_config_ops kvm_vq_configspace_ops = {
   .get_features = kvm_get_features,
   .finalize_features = kvm_finalize_features,
 - .get = kvm_get,
 - .set = kvm_set,
 + .get8 = kvm_get8,
 + .set8 = kvm_set8,
 + .get16 = kvm_get16,
 + .set16 = kvm_set16,
 + .get32 = kvm_get32,
 + .set32 = kvm_set32,
 + .get64 = kvm_get64,
 + .set64 = kvm_set64,
   .get_status = kvm_get_status,
   .set_status = kvm_set_status,
   .reset = kvm_reset,
 

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

Re: [PATCH 05/22] virtio: add support for 64 bit features.

[Cornelia Huck]

On Fri, 22 Mar 2013 11:20:05 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 Cornelia Huck cornelia.h...@de.ibm.com writes:
  On Thu, 21 Mar 2013 18:59:26 +1030
  Rusty Russell ru...@rustcorp.com.au wrote:
 
  Change the u32 to a u64, and make sure to use 1ULL everywhere!
 
  And a not-even-compiled change for virtio_ccw as well:
 
 Thanks, applied that too.
 
 BTW, this will all be in my virtio-pci-new-layout branch on
 git.kernel.org once I've processed all this feedback.

I'll give it a run then once your branch is ready.

 
 Thanks,
 Rusty.
 

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

Re: [PATCH] KVM: Fix kvm_irqfd_init initialization

[Cornelia Huck]

On Tue,  7 May 2013 22:54:16 +0800
Asias He as...@redhat.com wrote:

 In commit a0f155e96 'KVM: Initialize irqfd from kvm_init()', when
 kvm_init() is called the second time (e.g kvm-amd.ko and kvm-intel.ko),
 kvm_arch_init() will fail with -EEXIST, then kvm_irqfd_exit() will be
 called on the error handling path. This way, the kvm_irqfd system will
 not be ready.

Since we can't have the initialization as kvm module init function
without forcing everyone to split modules, this is probably the best
way to handle this.

Acked-by: Cornelia Huck cornelia.h...@de.ibm.com

 
 This patch fix the following:
 
 BUG: unable to handle kernel NULL pointer dereference at   (null)
 IP: [81c0721e] _raw_spin_lock+0xe/0x30
 PGD 0
 Oops: 0002 [#1] SMP
 Modules linked in: vhost_net
 CPU 6
 Pid: 4257, comm: qemu-system-x86 Not tainted 3.9.0-rc3+ #757 Dell Inc. 
 OptiPlex 790/0V5HMK
 RIP: 0010:[81c0721e]  [81c0721e] _raw_spin_lock+0xe/0x30
 RSP: 0018:880221721cc8  EFLAGS: 00010046
 RAX: 0100 RBX: 88022dcc003f RCX: 880221734950
 RDX: 8802208f6ca8 RSI: 7fff RDI: 
 RBP: 880221721cc8 R08: 0002 R09: 0002
 R10: 7f7fd01087e0 R11: 0246 R12: 8802208f6ca8
 R13: 0080 R14: 880223e2a900 R15: 
 FS:  7f7fd38488e0() GS:88022dcc() knlGS:
 CS:  0010 DS:  ES:  CR0: 80050033
 CR2:  CR3: 00022309f000 CR4: 000427e0
 DR0:  DR1:  DR2: 
 DR3:  DR6: 0ff0 DR7: 0400
 Process qemu-system-x86 (pid: 4257, threadinfo 88022172, task 
 880222bd5640)
 Stack:
  880221721d08 810ac5c5 88022431dc00 0086
  0080 880223e2a900 8802208f6ca8 
  880221721d48 810ac8fe  880221734000
 Call Trace:
  [810ac5c5] __queue_work+0x45/0x2d0
  [810ac8fe] queue_work_on+0x8e/0xa0
  [810ac949] queue_work+0x19/0x20
  [81009b6b] irqfd_deactivate+0x4b/0x60
  [8100a69d] kvm_irqfd+0x39d/0x580
  [81007a27] kvm_vm_ioctl+0x207/0x5b0
  [810c9545] ? update_curr+0xf5/0x180
  [811b66e8] do_vfs_ioctl+0x98/0x550
  [810c1f5e] ? finish_task_switch+0x4e/0xe0
  [81c054aa] ? __schedule+0x2ea/0x710
  [811b6bf7] sys_ioctl+0x57/0x90
  [8140ae9e] ? trace_hardirqs_on_thunk+0x3a/0x3c
  [81c0f602] system_call_fastpath+0x16/0x1b
 Code: c1 ea 08 38 c2 74 0f 66 0f 1f 44 00 00 f3 90 0f b6 03 38 c2 75 f7 48 83 
 c4 08 5b c9 c3 55 48 89 e5 66 66 66 66 90 b8 00 01 00 00 f0 66 0f c1 07 89 
 c2 66 c1 ea 08 38 c2 74 0c 0f 1f 00 f3 90 0f
 RIP  [81c0721e] _raw_spin_lock+0xe/0x30
 RSP 880221721cc8
 CR2: 
 ---[ end trace 13fb1e4b6e5ab21f ]---
 
 Signed-off-by: Asias He as...@redhat.com
 ---
  virt/kvm/kvm_main.c | 11 ++-
  1 file changed, 6 insertions(+), 5 deletions(-)
 
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 8fd325a..3c8a992 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -3078,13 +3078,14 @@ int kvm_init(void *opaque, unsigned vcpu_size, 
 unsigned vcpu_align,
   int r;
   int cpu;
 
 - r = kvm_irqfd_init();
 - if (r)
 - goto out_irqfd;
   r = kvm_arch_init(opaque);
   if (r)
   goto out_fail;
 
 + r = kvm_irqfd_init();
 + if (r)
 + goto out_irqfd;
 +
   if (!zalloc_cpumask_var(cpus_hardware_enabled, GFP_KERNEL)) {
   r = -ENOMEM;
   goto out_free_0;
 @@ -3159,10 +3160,10 @@ out_free_1:
  out_free_0a:
   free_cpumask_var(cpus_hardware_enabled);
  out_free_0:
 - kvm_arch_exit();
 -out_fail:
   kvm_irqfd_exit();
  out_irqfd:
 + kvm_arch_exit();
 +out_fail:
   return r;
  }
  EXPORT_SYMBOL_GPL(kvm_init);

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

Re: [PATCH v2] KVM: Fix kvm_irqfd_init initialization

[Cornelia Huck]

On Wed,  8 May 2013 10:57:29 +0800
Asias He as...@redhat.com wrote:

 In commit a0f155e96 'KVM: Initialize irqfd from kvm_init()', when
 kvm_init() is called the second time (e.g kvm-amd.ko and kvm-intel.ko),
 kvm_arch_init() will fail with -EEXIST, then kvm_irqfd_exit() will be
 called on the error handling path. This way, the kvm_irqfd system will
 not be ready.
 
 This patch fix the following:
 
 BUG: unable to handle kernel NULL pointer dereference at   (null)
 IP: [81c0721e] _raw_spin_lock+0xe/0x30
 PGD 0
 Oops: 0002 [#1] SMP
 Modules linked in: vhost_net
 CPU 6
 Pid: 4257, comm: qemu-system-x86 Not tainted 3.9.0-rc3+ #757 Dell Inc. 
 OptiPlex 790/0V5HMK
 RIP: 0010:[81c0721e]  [81c0721e] _raw_spin_lock+0xe/0x30
 RSP: 0018:880221721cc8  EFLAGS: 00010046
 RAX: 0100 RBX: 88022dcc003f RCX: 880221734950
 RDX: 8802208f6ca8 RSI: 7fff RDI: 
 RBP: 880221721cc8 R08: 0002 R09: 0002
 R10: 7f7fd01087e0 R11: 0246 R12: 8802208f6ca8
 R13: 0080 R14: 880223e2a900 R15: 
 FS:  7f7fd38488e0() GS:88022dcc() knlGS:
 CS:  0010 DS:  ES:  CR0: 80050033
 CR2:  CR3: 00022309f000 CR4: 000427e0
 DR0:  DR1:  DR2: 
 DR3:  DR6: 0ff0 DR7: 0400
 Process qemu-system-x86 (pid: 4257, threadinfo 88022172, task 
 880222bd5640)
 Stack:
  880221721d08 810ac5c5 88022431dc00 0086
  0080 880223e2a900 8802208f6ca8 
  880221721d48 810ac8fe  880221734000
 Call Trace:
  [810ac5c5] __queue_work+0x45/0x2d0
  [810ac8fe] queue_work_on+0x8e/0xa0
  [810ac949] queue_work+0x19/0x20
  [81009b6b] irqfd_deactivate+0x4b/0x60
  [8100a69d] kvm_irqfd+0x39d/0x580
  [81007a27] kvm_vm_ioctl+0x207/0x5b0
  [810c9545] ? update_curr+0xf5/0x180
  [811b66e8] do_vfs_ioctl+0x98/0x550
  [810c1f5e] ? finish_task_switch+0x4e/0xe0
  [81c054aa] ? __schedule+0x2ea/0x710
  [811b6bf7] sys_ioctl+0x57/0x90
  [8140ae9e] ? trace_hardirqs_on_thunk+0x3a/0x3c
  [81c0f602] system_call_fastpath+0x16/0x1b
 Code: c1 ea 08 38 c2 74 0f 66 0f 1f 44 00 00 f3 90 0f b6 03 38 c2 75 f7 48 83 
 c4 08 5b c9 c3 55 48 89 e5 66 66 66 66 90 b8 00 01 00 00 f0 66 0f c1 07 89 
 c2 66 c1 ea 08 38 c2 74 0c 0f 1f 00 f3 90 0f
 RIP  [81c0721e] _raw_spin_lock+0xe/0x30
 RSP 880221721cc8
 CR2: 
 ---[ end trace 13fb1e4b6e5ab21f ]---
 
 Signed-off-by: Asias He as...@redhat.com

Acked-by: Cornelia Huck cornelia.h...@de.ibm.com

 ---
  virt/kvm/kvm_main.c | 18 +-
  1 file changed, 13 insertions(+), 5 deletions(-)
 
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 8fd325a..85b93d2 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -3078,13 +3078,21 @@ int kvm_init(void *opaque, unsigned vcpu_size, 
 unsigned vcpu_align,
   int r;
   int cpu;
 
 - r = kvm_irqfd_init();
 - if (r)
 - goto out_irqfd;
   r = kvm_arch_init(opaque);
   if (r)
   goto out_fail;
 
 + /*
 +  * kvm_arch_init makes sure there's at most one caller
 +  * for architectures that support multiple implementations,
 +  * like intel and amd on x86.
 +  * kvm_arch_init must be called before kvm_irqfd_init to avoid creating
 +  * conflicts in case kvm is already setup for another implementation.
 +  */
 + r = kvm_irqfd_init();
 + if (r)
 + goto out_irqfd;
 +
   if (!zalloc_cpumask_var(cpus_hardware_enabled, GFP_KERNEL)) {
   r = -ENOMEM;
   goto out_free_0;
 @@ -3159,10 +3167,10 @@ out_free_1:
  out_free_0a:
   free_cpumask_var(cpus_hardware_enabled);
  out_free_0:
 - kvm_arch_exit();
 -out_fail:
   kvm_irqfd_exit();
  out_irqfd:
 + kvm_arch_exit();
 +out_fail:
   return r;
  }
  EXPORT_SYMBOL_GPL(kvm_init);

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

[RFC PATCH v2] virtio-ccw: Document adapter interrupts.

[Cornelia Huck]

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 virtio-spec.lyx |  147 +--
 1 file changed, 144 insertions(+), 3 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index c920155..4729766 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -10721,11 +10721,18 @@ status open
 
 \begin_layout LyX-Code
 
-\change_inserted -385801441 1343732726
+\change_inserted -385801441 1369814105
 
 #define CCW_CMD_READ_VQ_CONF 0x32
 \end_layout
 
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369814140
+
+#define CCW_CMD_SET_IND_ADAPTER 0x63
+\end_layout
+
 \end_inset
 
 
@@ -11065,11 +11072,136 @@ To communicate the location of the indicator bits 
for host-guest notification,
 
 \begin_layout Standard
 
-\change_inserted -385801441 1347015749
+\change_inserted -385801441 1369814376
 For the indicator bits used in the configuration change host-guest 
notification
 , the CCW_CMD_SET_CONF_IND command is used analogously.
 \end_layout
 
+\begin_layout Subsubsection*
+
+\change_inserted -385801441 1369814399
+Setting Up Indicators For Adapter Interrupts
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted -385801441 1369815013
+If the guest wishes to use adapter interrupts for host-guest notification,
+ it may use the CCW_CMD_SET_IND_ADAPTER command instead of CCW_CMD_SET_IND.
+ Note that usage of those two mechanisms is mutually exclusive.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted -385801441 1369815065
+CCW_CMD_SET_IND_ADAPTER uses the following communication block:
+\end_layout
+
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369815367
+\begin_inset listings
+inline false
+status open
+
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369815367
+
+struct thinint_area {
+\end_layout
+
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369815367
+
+unsigned long summary_indicator;
+\end_layout
+
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369815367
+
+unsigned long indicator;
+\end_layout
+
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369815367
+
+u16 shift;
+\end_layout
+
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369815367
+
+u8 isc;
+\end_layout
+
+\begin_layout LyX-Code
+
+\change_inserted -385801441 1369815367
+
+} __packed;
+\change_unchanged
+
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted -385801441 1370345028
+
+\family typewriter
+summary_indicator
+\family default
+ contains the guest address of a byte value to be used as a summary indicator
+ which is set to != 0 every time the host wants to signal the guest for
+ any of the indictors and unset by the guest to signify that it received
+ the notification.
+ 
+\family typewriter
+isc
+\family default
+ is the interruption subclass to be used for the adapter interrupt.
+ Note that an isc/summary indicator pair must match for any subsequent requests
+ to set up adapter interrupts .
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted -385801441 1369816401
+
+\family typewriter
+indicator
+\family default
+ contains the guest address of the 64 bit indicators to be used; 
+\family typewriter
+shift
+\family default
+ contains the offset of the queue indicators for the device in this value.
+ All queue indicators for a device must fit into the same 64 bit value.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted -385801441 1369814707
+Hosts not supporting adapter interrupts for virtio-ccw may fail this command
+ with a command reject.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted -385801441 1369814766
+Configuration change host-guest notification is always setup using 
CCW_CMD_SET_
+CONF_IND.
+\end_layout
+
 \begin_layout Subsection*
 
 \change_inserted -385801441 1343732726
@@ -11084,7 +11216,7 @@ Host-Guest Notification
 
 \begin_layout Standard
 
-\change_inserted -385801441 1347015762
+\change_inserted -385801441 1369814838
 For notifying the guest of virtqueue buffers, the host sets the corresponding
  bit in the guest-provided indicators.
  If an interrupt is not already pending for the subchannel, the host generates
@@ -11093,6 +11225,15 @@ For notifying the guest of virtqueue buffers, the host 
sets the corresponding
 
 \begin_layout Standard
 
+\change_inserted -385801441 1369815397
+Alternatively, if the guest enabled adapter interrupts for a device, 
notificatio
+n happens via setting the bit in the guest-provided indicators, setting
+ the summary indicator and generating an adapter interrupt for the registered
+ interruption subclass.
+\end_layout
+
+\begin_layout Standard
+
 \change_inserted -385801441 1347015847
 If the host wants to notify the guest about configuration changes, it sets
  bit 0 in the configuration indicators and generates an unsolicited I/O
-- 
1.7.9.5

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https

[RFC PATCH v2] Adapter interrupts for virtio-ccw.

[Cornelia Huck]

Hi,

here's again my proposal for adapter (thin) interrupt support for virtio-ccw
devices, originally posted at
http://marc.info/?l=linux-virtualizationm=137060143904927w=2

The document is unchanged, only rebased against current master.

Cornelia Huck (1):
  virtio-ccw: Document adapter interrupts.

 virtio-spec.lyx |  147 +--
 1 file changed, 144 insertions(+), 3 deletions(-)

-- 
1.7.9.5

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

[RFC PATCH v2] qemu: Adapter interrupts for virtio-ccw.

[Cornelia Huck]

Hi,

here's the current implementation of virtio-ccw adapter interrupts in qemu.

Code is unchanged, only rebased against current master.

Cornelia Huck (1):
  s390/virtio-ccw: Adapter interrupt support.

 hw/s390x/css.c|   10 
 hw/s390x/css.h|2 ++
 hw/s390x/virtio-ccw.c |   64 -
 hw/s390x/virtio-ccw.h |4 
 target-s390x/ioinst.h |2 ++
 target-s390x/kvm.c|8 +--
 trace-events  |1 +
 7 files changed, 88 insertions(+), 3 deletions(-)

-- 
1.7.9.5

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

[RFC PATCH v2] s390/virtio-ccw: Adapter interrupt support.

[Cornelia Huck]

Handle the new CCW_CMD_SET_IND_ADAPTER command enabling adapter interrupts
on guest request. When active, host-guest notifications will be handled
via global_indicator - queue indicators instead of queue indicators +
subchannel I/O interrupt. Indicators for virtqueues may be present at an
offset.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/css.c|   10 
 hw/s390x/css.h|2 ++
 hw/s390x/virtio-ccw.c |   64 -
 hw/s390x/virtio-ccw.h |4 
 target-s390x/ioinst.h |2 ++
 target-s390x/kvm.c|8 +--
 trace-events  |1 +
 7 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 93b0b97..849879f 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -115,6 +115,15 @@ void css_conditional_io_interrupt(SubchDev *sch)
 }
 }
 
+void css_adapter_interrupt(uint8_t isc)
+{
+S390CPU *cpu = s390_cpu_addr2state(0);
+uint32_t io_int_word = (isc  27) | IO_INT_WORD_AI;
+
+trace_css_adapter_interrupt(isc);
+s390_io_interrupt(cpu, 0, 0, 0, io_int_word);
+}
+
 static void sch_handle_clear_func(SubchDev *sch)
 {
 PMCW *p = sch-curr_status.pmcw;
@@ -1256,6 +1265,7 @@ void css_reset_sch(SubchDev *sch)
 sch-channel_prog = 0x0;
 sch-last_cmd_valid = false;
 sch-orb = NULL;
+sch-thinint_active = false;
 }
 
 void css_reset(void)
diff --git a/hw/s390x/css.h b/hw/s390x/css.h
index b536ab5..e9b4405 100644
--- a/hw/s390x/css.h
+++ b/hw/s390x/css.h
@@ -77,6 +77,7 @@ struct SubchDev {
 CCW1 last_cmd;
 bool last_cmd_valid;
 ORB *orb;
+bool thinint_active;
 /* transport-provided data: */
 int (*ccw_cb) (SubchDev *, CCW1);
 SenseId id;
@@ -97,4 +98,5 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, 
uint16_t rsid);
 void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
int hotplugged, int add);
 void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
+void css_adapter_interrupt(uint8_t isc);
 #endif
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e744957..846e288 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -184,6 +184,13 @@ typedef struct VirtioFeatDesc {
 uint8_t index;
 } QEMU_PACKED VirtioFeatDesc;
 
+typedef struct VirtioThinintInfo {
+hwaddr summary_indicator;
+hwaddr device_indicator;
+uint16_t ind_shift;
+uint8_t isc;
+} QEMU_PACKED VirtioThinintInfo;
+
 /* Specify where the virtqueues for the subchannel are in guest memory. */
 static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
   uint16_t index, uint16_t num)
@@ -232,6 +239,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
 bool check_len;
 int len;
 hwaddr hw_len;
+VirtioThinintInfo *thinint;
 
 if (!dev) {
 return -EINVAL;
@@ -418,6 +426,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
 ret = -EINVAL;
 break;
 }
+if (sch-thinint_active) {
+/* Trigger a command reject. */
+ret = -ENOSYS;
+break;
+}
 if (!ccw.cda) {
 ret = -EFAULT;
 } else {
@@ -469,6 +482,42 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
 ret = 0;
 }
 break;
+case CCW_CMD_SET_IND_ADAPTER:
+if (check_len) {
+if (ccw.count != sizeof(*thinint)) {
+ret = -EINVAL;
+break;
+}
+} else if (ccw.count  sizeof(*thinint)) {
+/* Can't execute command. */
+ret = -EINVAL;
+break;
+}
+len = sizeof(*thinint);
+hw_len = len;
+if (!ccw.cda) {
+ret = -EFAULT;
+} else if (dev-indicators  !sch-thinint_active) {
+/* Trigger a command reject. */
+ret = -ENOSYS;
+} else {
+thinint = cpu_physical_memory_map(ccw.cda, hw_len, 0);
+if (!thinint) {
+ret = -EFAULT;
+} else {
+len = hw_len;
+dev-summary_indicator = thinint-summary_indicator;
+dev-indicators = thinint-device_indicator;
+dev-thinint_isc = thinint-isc;
+dev-ind_shift = thinint-ind_shift;
+cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);
+sch-thinint_active = ((dev-indicators != 0) 
+   (dev-summary_indicator != 0));
+sch-curr_status.scsw.count = ccw.count - len;
+ret = 0;
+}
+}
+break;
 default:
 ret = -ENOSYS;
 break;
@@ -501,6 +550,7 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, 
VirtIODevice *vdev)
 sch-channel_prog = 0x0;
 sch-last_cmd_valid = false;
 sch-orb = NULL;
+sch-thinint_active = false

[RFC PATCH v2 0/2] KVM: s390: virtio-ccw adapter interrupts.

[Cornelia Huck]

Hi,

next version of the guest exploitation of virtio-ccw adapter interrupts.

Changes from the last version:

- adapt to latest kvm-next
- changed housekeeping for indicator locations: we now use cacheline-sized
  and aligned areas
- minor tweaks

Cornelia Huck (2):
  KVM: s390: virtio-ccw: Handle command rejects.
  KVM: s390: virtio-ccw adapter interrupt support.

 arch/s390/include/asm/irq.h   |   1 +
 arch/s390/kernel/irq.c|   1 +
 drivers/s390/kvm/virtio_ccw.c | 314 --
 3 files changed, 305 insertions(+), 11 deletions(-)

-- 
1.8.2.2

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

[RFC PATCH v2 1/2] KVM: s390: virtio-ccw: Handle command rejects.

[Cornelia Huck]

A command reject for a ccw may happen if we run on a host not supporting
a certain feature. We want to be able to handle this as special case of
command failure, so let's split this off from the generic -EIO error code.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/s390/kvm/virtio_ccw.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 779dc51..d6c7aba 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -639,8 +639,15 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
 (SCSW_STCTL_ALERT_STATUS | SCSW_STCTL_STATUS_PEND))) {
/* OK */
}
-   if (irb_is_error(irb))
-   vcdev-err = -EIO; /* XXX - use real error */
+   if (irb_is_error(irb)) {
+   /* Command reject? */
+   if ((scsw_dstat(irb-scsw)  DEV_STAT_UNIT_CHECK) 
+   (irb-ecw[0]  SNS0_CMD_REJECT))
+   vcdev-err = -EOPNOTSUPP;
+   else
+   /* Map everything else to -EIO. */
+   vcdev-err = -EIO;
+   }
if (vcdev-curr_io  activity) {
switch (activity) {
case VIRTIO_CCW_DOING_READ_FEAT:
-- 
1.8.2.2

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

[RFC PATCH v2 2/2] KVM: s390: virtio-ccw adapter interrupt support.

[Cornelia Huck]

Implement the new CCW_CMD_SET_IND_ADAPTER command and try to enable
adapter interrupts for every device on the first startup. If the host
does not support adapter interrupts, fall back to normal I/O interrupts.

virtio-ccw adapter interrupts use the same isc as normal I/O subchannels
and share a summary indicator for all devices sharing the same indicator
area.

Indicator bits for the individual virtqueues may be contained in the same
indicator area for different devices.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 arch/s390/include/asm/irq.h   |   1 +
 arch/s390/kernel/irq.c|   1 +
 drivers/s390/kvm/virtio_ccw.c | 303 --
 3 files changed, 296 insertions(+), 9 deletions(-)

diff --git a/arch/s390/include/asm/irq.h b/arch/s390/include/asm/irq.h
index 87c17bf..ba75d32 100644
--- a/arch/s390/include/asm/irq.h
+++ b/arch/s390/include/asm/irq.h
@@ -42,6 +42,7 @@ enum interruption_class {
IRQIO_PCI,
IRQIO_MSI,
IRQIO_VIR,
+   IRQIO_VAI,
NMI_NMI,
CPU_RST,
NR_ARCH_IRQS
diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index 54b0995..2fd938b 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -82,6 +82,7 @@ static const struct irq_class irqclass_sub_desc[NR_ARCH_IRQS] 
= {
[IRQIO_PCI]  = {.name = PCI, .desc = [I/O] PCI Interrupt },
[IRQIO_MSI]  = {.name = MSI, .desc = [I/O] MSI Interrupt },
[IRQIO_VIR]  = {.name = VIR, .desc = [I/O] Virtual I/O Devices},
+   [IRQIO_VAI]  = {.name = VAI, .desc = [I/O] Virtual I/O Devices AI},
[NMI_NMI]= {.name = NMI, .desc = [NMI] Machine Check},
[CPU_RST]= {.name = RST, .desc = [CPU] CPU Restart},
 };
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index d6c7aba..edf6bd3 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -32,6 +32,8 @@
 #include asm/cio.h
 #include asm/ccwdev.h
 #include asm/virtio-ccw.h
+#include asm/airq.h
+#include asm/isc.h
 
 /*
  * virtio related functions
@@ -58,6 +60,8 @@ struct virtio_ccw_device {
unsigned long indicators;
unsigned long indicators2;
struct vq_config_block *config_block;
+   bool is_thinint;
+   void *airq_info;
 };
 
 struct vq_info_block {
@@ -72,15 +76,46 @@ struct virtio_feature_desc {
__u8 index;
 } __packed;
 
+struct virtio_thinint_area {
+   unsigned long summary_indicator;
+   unsigned long indicator;
+   u16 shift;
+   u8 isc;
+} __packed;
+
 struct virtio_ccw_vq_info {
struct virtqueue *vq;
int num;
void *queue;
struct vq_info_block *info_block;
+   int bit_nr;
struct list_head node;
long cookie;
 };
 
+#define VIRTIO_AIRQ_ISC IO_SCH_ISC /* inherit from subchannel */
+
+#define VIRTIO_DEV_CHUNK (L1_CACHE_BYTES/sizeof(unsigned long))
+#define MAX_AIRQ_AREAS 20
+
+static int virtio_ccw_use_airq = 1;
+
+struct airq_map {
+   void *map[BITS_PER_LONG];
+};
+struct airq_info {
+   unsigned long used[VIRTIO_DEV_CHUNK];
+   unsigned long indicators[VIRTIO_DEV_CHUNK];
+   struct airq_map vqs[VIRTIO_DEV_CHUNK];
+   rwlock_t lock;
+   u8 summary_indicator;
+   int max_used;
+   struct airq_struct airq;
+};
+static struct airq_info *airq_areas[MAX_AIRQ_AREAS];
+
+static struct kmem_cache *airq_cache;
+
 #define CCW_CMD_SET_VQ 0x13
 #define CCW_CMD_VDEV_RESET 0x33
 #define CCW_CMD_SET_IND 0x43
@@ -91,6 +126,7 @@ struct virtio_ccw_vq_info {
 #define CCW_CMD_WRITE_CONF 0x21
 #define CCW_CMD_WRITE_STATUS 0x31
 #define CCW_CMD_READ_VQ_CONF 0x32
+#define CCW_CMD_SET_IND_ADAPTER 0x63
 
 #define VIRTIO_CCW_DOING_SET_VQ 0x0001
 #define VIRTIO_CCW_DOING_RESET 0x0004
@@ -102,6 +138,7 @@ struct virtio_ccw_vq_info {
 #define VIRTIO_CCW_DOING_SET_IND 0x0100
 #define VIRTIO_CCW_DOING_READ_VQ_CONF 0x0200
 #define VIRTIO_CCW_DOING_SET_CONF_IND 0x0400
+#define VIRTIO_CCW_DOING_SET_IND_ADAPTER 0x0800
 #define VIRTIO_CCW_INTPARM_MASK 0x
 
 static struct virtio_ccw_device *to_vc_device(struct virtio_device *vdev)
@@ -109,6 +146,136 @@ static struct virtio_ccw_device *to_vc_device(struct 
virtio_device *vdev)
return container_of(vdev, struct virtio_ccw_device, vdev);
 }
 
+static void drop_airq_indicator(struct virtqueue *vq, struct airq_info *info)
+{
+   int i, j;
+   unsigned long flags;
+
+   write_lock_irqsave(info-lock, flags);
+   for (i = 0; i  VIRTIO_DEV_CHUNK; i++) {
+   for_each_set_bit(j, info-used[i],
+sizeof(info-used[i]) * BITS_PER_BYTE)
+   if (info-vqs[i].map[j] == vq) {
+   info-vqs[i].map[j] = NULL;
+   clear_bit(j, info-used[i]);
+   if ((info-max_used == i)  !info-used[i])
+   info-max_used

Re: [RFC PATCH v2] s390/virtio-ccw: Adapter interrupt support.

[Cornelia Huck]

On Tue, 09 Jul 2013 15:27:14 +0200
Christian Borntraeger borntrae...@de.ibm.com wrote:

 On 09/07/13 13:34, Cornelia Huck wrote:
  Handle the new CCW_CMD_SET_IND_ADAPTER command enabling adapter interrupts
  on guest request. When active, host-guest notifications will be handled
  via global_indicator - queue indicators instead of queue indicators +
  subchannel I/O interrupt. Indicators for virtqueues may be present at an
  offset.
  
 
 You might want to add why we want adapter interrupts:
 - no test subchannel - less qemu mutex contention
 - no test subchannel - we can implement something like irqfd without moving
 most of ccw device mgmt into the kernel
 - interrupt coalescing
 - the guest common I/O layer already supports adapter interrupts
 for all newer hardware

How about the following:

With traditional I/O interrupts, status needs to be collected from the
subchannel via TEST SUBCHANNEL as well. With adapter interrupts, we
- avoid the extra exit due to TEST SUBCHANNEL
- can deliver multiple queue interrupts via the same I/O interrupt
- make it possible to implement irqfds without having to track
  subchannel status inside kvm

 
 
 the interesting part of this patch is the guest-host interface. As far as I
 can see, we are able to register 
 - an isc per device
 - an arbitrary summary indicator byte per device
 - an arbitrary bit position in guest memory where the queue indicator bits of 
 this
 device start
 
 This allows for packing the indicators for all virtqueues  of all devices or
 spreading them in memory. The layout and amount of coalescing of bits is then 
 an optimization that can be changed all the time without the need to change 
 the interface.
 
  Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 Acked-by: Christian Borntraeger borntrae...@de.ibm.com

Thx!

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

Re: [PATCH 0/3] virtio: Clean up scatterlists and use the DMA API

[Cornelia Huck]

On Tue, 26 Aug 2014 14:16:59 -0700
Andy Lutomirski l...@amacapital.net wrote:

 This fixes virtio on Xen guests as well as on any other platform on
 which physical addresses don't match bus addresses.
 
 This can be tested with:
 
 virtme-run --xen xen --kimg arch/x86/boot/bzImage --console
 
 using virtme from here:
 
 https://git.kernel.org/cgit/utils/kernel/virtme/virtme.git
 
 Without these patches, the guest hangs forever.  With these patches,
 everything works.
 
 There are two outstanding issues.  virtio_net warns if DMA debugging
 is on because it does DMA from the stack.  (The warning is correct.)
 This also is likely to do something unpleasant to s390.

s/is likely to do something unpleasant to/breaks/

Sorry, this just won't work for s390, as Christian already explained.

 (Maintainers are cc'd -- I don't know what to do about it.)
 
 Andy Lutomirski (3):
   virtio_ring: Remove sg_next indirection
   virtio_ring: Use DMA APIs
   virtio_pci: Use the DMA API for virtqueues
 
  drivers/virtio/Kconfig   |   1 +
  drivers/virtio/virtio_pci.c  |  25 ++--
  drivers/virtio/virtio_ring.c | 150 
 ++-
  3 files changed, 125 insertions(+), 51 deletions(-)
 

Aren't you also changing some basic assumptions with the conversion to
DMA apis? virtqueues currently aren't accessed via dma (or it wouldn't
work on s390); if you want (say) virtio-pci to use dma, shouldn't that
be something that is negotiated between guest and host?

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

Re: [PATCH 02/16] virtio: unify config_changed handling

[Cornelia Huck]

On Sun, 5 Oct 2014 19:06:52 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Replace duplicated code in all transports with a single wrapper in
 virtio.c.
 
 The only functional change is in virtio_mmio.c: if a buggy device sends
 us an interrupt before driver is set, we previously returned IRQ_NONE,
 now we return IRQ_HANDLED.
 
 As this must not happen in practice, this does not look like a big deal.
 
 See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934
   virtio-pci: do not oops on config change if driver not loaded.
 for the original motivation behind the driver check.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio.h |  2 ++
  drivers/misc/mic/card/mic_virtio.c |  6 +-
  drivers/s390/kvm/kvm_virtio.c  |  9 +
  drivers/s390/kvm/virtio_ccw.c  |  6 +-
  drivers/virtio/virtio.c| 10 ++
  drivers/virtio/virtio_mmio.c   |  7 ++-
  drivers/virtio/virtio_pci.c|  6 +-
  7 files changed, 18 insertions(+), 28 deletions(-)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH 03/16] virtio: refactor to use drv_to_virtio

[Cornelia Huck]

On Sun, 5 Oct 2014 19:06:59 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Use drv_to_virtio instead of open-coding it.
 Fix whitespace damage introduced by
 virtio: unify config_changed handling

Would it make sense to merge this into the previous patch?

 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/virtio/virtio.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

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

Re: [PATCH 04/16] virtio-pci: move freeze/restore to virtio core

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:02 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 This is in preparation to extending config changed event handling
 in core.
 Wrapping these in an API also seems to make for a cleaner code.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio.h  |  6 +
  drivers/virtio/virtio.c | 53 +++
  drivers/virtio/virtio_pci.c | 55 
 ++---
  3 files changed, 61 insertions(+), 53 deletions(-)
 
 diff --git a/include/linux/virtio.h b/include/linux/virtio.h
 index 3c19bd3..8df7ba8 100644
 --- a/include/linux/virtio.h
 +++ b/include/linux/virtio.h
 @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
  /**
   * virtio_device - representation of a device using virtio
   * @index: unique position on the virtio bus
 + * @failed: saved value for CONFIG_S_FAILED bit (for restore)
   * @dev: underlying device.
   * @id: the device type identification (used to match it with a driver).
   * @config: the configuration ops for this device.

The only minor gripe I have is with the naming of this field: To a
cursory reader, it might indicate the current 'failed' status of the
device. What about 'saved_failed'?

Otherwise, moving this into common virtio code makes sense.

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH 05/16] virtio: defer config changed notifications

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:05 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Defer config changed notifications that arrive during
 probe/scan/freeze/restore.
 
 This will allow drivers to set DRIVER_OK earlier, without worrying about
 racing with config change interrupts.
 
 This change will also benefit old hypervisors (before 2009)
 that send interrupts without checking DRIVER_OK: previously,
 the callback could race with driver-specific initialization.
 
 This will also help simplify drivers.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio.h  |  6 ++
  drivers/virtio/virtio.c | 55 
 +
  2 files changed, 52 insertions(+), 9 deletions(-)
 
 diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
 index 657f817..578e02d 100644
 --- a/drivers/virtio/virtio.c
 +++ b/drivers/virtio/virtio.c
 @@ -117,6 +117,40 @@ void virtio_check_driver_offered_feature(const struct 
 virtio_device *vdev,
  }
  EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
 
 +static void __virtio_config_changed(struct virtio_device *dev)
 +{
 + struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
 +
 + if (!dev-config_enabled)
 + dev-config_changed = true;
 + else if (drv  drv-config_changed)
 + drv-config_changed(dev);
 +}
 +
 +void virtio_config_changed(struct virtio_device *dev)
 +{
 + spin_lock_irq(dev-config_lock);
 + __virtio_config_changed(dev);
 + spin_unlock_irq(dev-config_lock);

Hm, isn't this function called from the interrupt handler?

 +}
 +EXPORT_SYMBOL_GPL(virtio_config_changed);
 +
 +static void virtio_config_disable(struct virtio_device *dev)
 +{
 + spin_lock_irq(dev-config_lock);
 + dev-config_enabled = false;
 + spin_unlock_irq(dev-config_lock);
 +}
 +
 +static void virtio_config_enable(struct virtio_device *dev)
 +{
 + spin_lock_irq(dev-config_lock);
 + dev-config_enabled = true;
 + __virtio_config_changed(dev);
 + dev-config_changed = false;
 + spin_unlock_irq(dev-config_lock);
 +}
 +

Maybe call these virtio_config_change_{dis,en}able()?

  static int virtio_dev_probe(struct device *_d)
  {
   int err, i;

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

Re: [PATCH 06/16] virtio_blk: drop config_enable

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:07 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Now that virtio core ensures config changes don't
 arrive during probing, drop config_enable flag
 in virtio blk.
 On removal, flush is now sufficient to guarantee that
 no change work is queued.
 
 This help simplify the driver, and will allow
 setting DRIVER_OK earlier without losing config
 change notifications.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/block/virtio_blk.c | 19 ++-
  1 file changed, 2 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
 index 0a58140..c8cf6a1 100644
 --- a/drivers/block/virtio_blk.c
 +++ b/drivers/block/virtio_blk.c

 @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev)
   int refc;
 
   /* Prevent config work handler from accessing the device. */

/* Common code ensures no further work will be queued. */

instead?

 - mutex_lock(vblk-config_lock);
 - vblk-config_enable = false;
 - mutex_unlock(vblk-config_lock);
 + flush_work(vblk-config_work);
 
   del_gendisk(vblk-disk);
   blk_cleanup_queue(vblk-disk-queue);

 @@ -806,10 +796,6 @@ static int virtblk_freeze(struct virtio_device *vdev)
   vdev-config-reset(vdev);
 
   /* Prevent config work handler from accessing the device. */

dito on the comment

 - mutex_lock(vblk-config_lock);
 - vblk-config_enable = false;
 - mutex_unlock(vblk-config_lock);
 -
   flush_work(vblk-config_work);
 
   blk_mq_stop_hw_queues(vblk-disk-queue);

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

Re: [PATCH 07/16] virtio-blk: drop config_mutex

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:10 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 config_mutex served two purposes: prevent multiple concurrent config
 change handlers, and synchronize access to config_enable flag.
 
 Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
 workqueue: make all workqueues non-reentrant
 all workqueues are non-reentrant, and config_enable
 is now gone.
 
 Get rid of the unnecessary lock.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/block/virtio_blk.c | 8 
  1 file changed, 8 deletions(-)
 

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH 09/16] virtio-net: drop config_mutex

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:16 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 config_mutex served two purposes: prevent multiple concurrent config
 change handlers, and synchronize access to config_enable flag.
 
 Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
 workqueue: make all workqueues non-reentrant
 all workqueues are non-reentrant, and config_enable
 is now gone.
 
 Get rid of the unnecessary lock.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/net/virtio_net.c | 7 +--
  1 file changed, 1 insertion(+), 6 deletions(-)
 
 diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
 index fa17afa..d80fef4 100644
 --- a/drivers/net/virtio_net.c
 +++ b/drivers/net/virtio_net.c

 @@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct 
 work_struct *work)
   netif_tx_stop_all_queues(vi-dev);
   }
  done:
 - mutex_unlock(vi-config_lock);
 + return;
  }
 
  static void virtnet_config_changed(struct virtio_device *vdev)

I'd probably return directly in the remaining 'goto done;' cases, but still

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH 11/16] virtio_net: minor cleanup

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:21 +0300
Michael S. Tsirkin m...@redhat.com wrote:

   goto done;
 done:
   return;
 is ugly, it was put there to make diff review easier.
 replace by open-coded return.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/net/virtio_net.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)
 

If you don't want to merge it into the mutex removal patch, maybe move
it one up in the series?

Acked-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH 06/16] virtio_blk: drop config_enable

[Cornelia Huck]

On Mon, 6 Oct 2014 15:09:53 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote:
  On Sun, 5 Oct 2014 19:07:07 +0300
  Michael S. Tsirkin m...@redhat.com wrote:
  
   Now that virtio core ensures config changes don't
   arrive during probing, drop config_enable flag
   in virtio blk.
   On removal, flush is now sufficient to guarantee that
   no change work is queued.
   
   This help simplify the driver, and will allow
   setting DRIVER_OK earlier without losing config
   change notifications.
   
   Signed-off-by: Michael S. Tsirkin m...@redhat.com
   ---
drivers/block/virtio_blk.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
   
   diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
   index 0a58140..c8cf6a1 100644
   --- a/drivers/block/virtio_blk.c
   +++ b/drivers/block/virtio_blk.c
  
   @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device *vdev)
 int refc;
   
 /* Prevent config work handler from accessing the device. */
  
  /* Common code ensures no further work will be queued. */
  
  instead?
 
 No, I think you missed the point:
 this comment now refers to the flush below: flush is required to
 ensure work handler is not running.
 
 Agree?

I think we both mean the same thing.

Preventing the handler from access sounds to me more like when the
handler starts running, it is prevented from accessing the
device (like with setting config_enable, as the code did before). What
I meant was common code has already ensured that our work-queueing
function will not be called, therefore flushing the workqueue is
enough.

(same for net)

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

Re: [PATCH 06/16] virtio_blk: drop config_enable

[Cornelia Huck]

On Mon, 6 Oct 2014 15:31:10 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Oct 06, 2014 at 02:20:38PM +0200, Cornelia Huck wrote:
  On Mon, 6 Oct 2014 15:09:53 +0300
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Mon, Oct 06, 2014 at 01:42:29PM +0200, Cornelia Huck wrote:
On Sun, 5 Oct 2014 19:07:07 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Now that virtio core ensures config changes don't
 arrive during probing, drop config_enable flag
 in virtio blk.
 On removal, flush is now sufficient to guarantee that
 no change work is queued.
 
 This help simplify the driver, and will allow
 setting DRIVER_OK earlier without losing config
 change notifications.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/block/virtio_blk.c | 19 ++-
  1 file changed, 2 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
 index 0a58140..c8cf6a1 100644
 --- a/drivers/block/virtio_blk.c
 +++ b/drivers/block/virtio_blk.c

 @@ -772,9 +766,7 @@ static void virtblk_remove(struct virtio_device 
 *vdev)
   int refc;
 
   /* Prevent config work handler from accessing the device. */

/* Common code ensures no further work will be queued. */

instead?
   
   No, I think you missed the point:
   this comment now refers to the flush below: flush is required to
   ensure work handler is not running.
   
   Agree?
  
  I think we both mean the same thing.
  
  Preventing the handler from access sounds to me more like when the
  handler starts running, it is prevented from accessing the
  device (like with setting config_enable, as the code did before). What
  I meant was common code has already ensured that our work-queueing
  function will not be called, therefore flushing the workqueue is
  enough.
  
  (same for net)
 
 OK so I'll rewrite this to
   /* Make sure no work handler is accessing the device. */
 ?
 I prefer not duplicating core guarantees in all devices.
 

Sounds good to me.

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

Re: [PATCH 10/16] virtio: add API to enable VQs early

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:19 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 virtio spec requires DRIVER_OK to be set before
 VQs are used, but some drivers use VQs before probe
 function returns.
 Since DRIVER_OK is set after probe, this violates the spec.

Even though transitional devices support this behaviour, we want to
make it possible for those early callers to become spec compliant.

?

 
 Add API for drivers to call before using VQs.
 
 Sets DRIVER_OK internally.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio_config.h | 17 +
  1 file changed, 17 insertions(+)
 
 diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
 index e8f8f71..6127fc8 100644
 --- a/include/linux/virtio_config.h
 +++ b/include/linux/virtio_config.h
 @@ -109,6 +109,23 @@ struct virtqueue *virtio_find_single_vq(struct 
 virtio_device *vdev,
   return vq;
  }
 
 +/**
 + * virtio_early_enable_vqs - enable vq use in probe function
 + * @vdev: the device
 + *
 + * Driver must call this to use vqs in the probe function.
 + *
 + * Note: vqs are enabled automatically after probe returns.
 + */
 +static inline
 +void virtio_early_enable_vqs(struct virtio_device *dev)
 +{
 + unsigned status = dev-config-get_status(dev);
 +
 + BUG_ON(status  VIRTIO_CONFIG_S_DRIVER_OK);
 + dev-config-set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
 +}
 +

Maybe virtio_enable_vqs_early() instead?

  static inline
  const char *virtio_bus_name(struct virtio_device *vdev)
  {

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

Re: [PATCH 16/16] virtio_net: fix use after free on allocation failure

[Cornelia Huck]

On Sun, 5 Oct 2014 19:07:38 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 In the extremely unlikely event that driver initialization fails after
 RX buffers are added, virtio net frees RX buffers while VQs are
 still active, potentially causing device to use a freed buffer.
 
 To fix, reset device first - same as we do on device removal.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/net/virtio_net.c | 2 ++
  1 file changed, 2 insertions(+)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH v2 02/15] virtio: unify config_changed handling

[Cornelia Huck]

On Mon, 6 Oct 2014 18:10:44 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Replace duplicated code in all transports with a single wrapper in
 virtio.c.
 
 The only functional change is in virtio_mmio.c: if a buggy device sends
 us an interrupt before driver is set, we previously returned IRQ_NONE,
 now we return IRQ_HANDLED.
 
 As this must not happen in practice, this does not look like a big deal.
 
 See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934
   virtio-pci: do not oops on config change if driver not loaded.
 for the original motivation behind the driver check.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio.h | 2 ++
  drivers/misc/mic/card/mic_virtio.c | 6 +-
  drivers/s390/kvm/kvm_virtio.c  | 9 +
  drivers/s390/kvm/virtio_ccw.c  | 6 +-
  drivers/virtio/virtio.c| 9 +
  drivers/virtio/virtio_mmio.c   | 7 ++-
  drivers/virtio/virtio_pci.c| 6 +-
  7 files changed, 17 insertions(+), 28 deletions(-)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH v2 03/15] virtio-pci: move freeze/restore to virtio core

[Cornelia Huck]

On Mon, 6 Oct 2014 18:10:51 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 This is in preparation to extending config changed event handling
 in core.
 Wrapping these in an API also seems to make for a cleaner code.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio.h  |  6 +
  drivers/virtio/virtio.c | 53 +++
  drivers/virtio/virtio_pci.c | 55 
 ++---
  3 files changed, 61 insertions(+), 53 deletions(-)
 
 diff --git a/include/linux/virtio.h b/include/linux/virtio.h
 index 3c19bd3..8df7ba8 100644
 --- a/include/linux/virtio.h
 +++ b/include/linux/virtio.h
 @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
  /**
   * virtio_device - representation of a device using virtio
   * @index: unique position on the virtio bus
 + * @failed: saved value for CONFIG_S_FAILED bit (for restore)

Have you considered s/failed/saved_failed/ ?

   * @dev: underlying device.
   * @id: the device type identification (used to match it with a driver).
   * @config: the configuration ops for this device.

Otherwise, my

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

still stands.

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

Re: [PATCH v2 04/15] virtio: defer config changed notifications

[Cornelia Huck]

On Mon, 6 Oct 2014 18:10:55 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Defer config changed notifications that arrive during
 probe/scan/freeze/restore.
 
 This will allow drivers to set DRIVER_OK earlier, without worrying about
 racing with config change interrupts.
 
 This change will also benefit old hypervisors (before 2009)
 that send interrupts without checking DRIVER_OK: previously,
 the callback could race with driver-specific initialization.
 
 This will also help simplify drivers.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio.h  |  6 ++
  drivers/virtio/virtio.c | 57 
 +
  2 files changed, 54 insertions(+), 9 deletions(-)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH v2 05/15] virtio_blk: drop config_enable

[Cornelia Huck]

On Mon, 6 Oct 2014 18:10:58 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Now that virtio core ensures config changes don't
 arrive during probing, drop config_enable flag
 in virtio blk.
 On removal, flush is now sufficient to guarantee that
 no change work is queued.
 
 This help simplify the driver, and will allow
 setting DRIVER_OK earlier without losing config
 change notifications.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/block/virtio_blk.c | 23 ---
  1 file changed, 4 insertions(+), 19 deletions(-)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH v2 07/15] virtio_net: drop config_enable

[Cornelia Huck]

On Mon, 6 Oct 2014 18:11:05 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Now that virtio core ensures config changes don't arrive during probing,
 drop config_enable flag in virtio net.
 On removal, flush is now sufficient to guarantee that no change work is
 queued.
 
 This help simplify the driver, and will allow setting DRIVER_OK earlier
 without losing config change notifications.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/net/virtio_net.c | 27 ---
  1 file changed, 4 insertions(+), 23 deletions(-)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH v2 10/15] virtio: add API to enable VQs early

[Cornelia Huck]

On Mon, 6 Oct 2014 18:11:16 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 virtio spec 0.9.X requires DRIVER_OK to be set before
 VQs are used, but some drivers use VQs before probe
 function returns.
 Since DRIVER_OK is set after probe, this violates the spec.
 
 Even though under virtio 1.0 transitional devices support this
 behaviour, we want to make it possible for those early callers to become
 spec compliant and eventually support non-transitional devices.
 
 Add API for drivers to call before using VQs.
 
 Sets DRIVER_OK internally.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio_config.h | 17 +
  1 file changed, 17 insertions(+)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH v2 03/15] virtio-pci: move freeze/restore to virtio core

[Cornelia Huck]

On Mon, 6 Oct 2014 18:26:55 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Oct 06, 2014 at 05:20:55PM +0200, Cornelia Huck wrote:
  On Mon, 6 Oct 2014 18:10:51 +0300
  Michael S. Tsirkin m...@redhat.com wrote:
  
   This is in preparation to extending config changed event handling
   in core.
   Wrapping these in an API also seems to make for a cleaner code.
   
   Signed-off-by: Michael S. Tsirkin m...@redhat.com
   ---
include/linux/virtio.h  |  6 +
drivers/virtio/virtio.c | 53 
   +++
drivers/virtio/virtio_pci.c | 55 
   ++---
3 files changed, 61 insertions(+), 53 deletions(-)
   
   diff --git a/include/linux/virtio.h b/include/linux/virtio.h
   index 3c19bd3..8df7ba8 100644
   --- a/include/linux/virtio.h
   +++ b/include/linux/virtio.h
   @@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
/**
 * virtio_device - representation of a device using virtio
 * @index: unique position on the virtio bus
   + * @failed: saved value for CONFIG_S_FAILED bit (for restore)
  
  Have you considered s/failed/saved_failed/ ?
 
 I kind of prefer the shorter name, your reviewed tag made me think
 you don't cosider this too important?

Indeed; if you prefer the short name, just keep it.
 
 * @dev: underlying device.
 * @id: the device type identification (used to match it with a driver).
 * @config: the configuration ops for this device.
  
  Otherwise, my
  
  Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
  
  still stands.
 

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

Re: [PATCH v2 11/15] virtio_net: enable VQs early

[Cornelia Huck]

On Mon, 6 Oct 2014 18:11:19 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 virtio spec requires drivers to set DRIVER_OK before using VQs.
 This is set automatically after probe returns, virtio net violated this
 rule by using receive VQs within probe.
 
 To fix, call virtio_enable_vqs_early before using VQs.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/net/virtio_net.c | 2 ++
  1 file changed, 2 insertions(+)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH v2 12/15] virtio_blk: enable VQs early

[Cornelia Huck]

On Mon, 6 Oct 2014 18:11:22 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 virtio spec requires drivers to set DRIVER_OK before using VQs.
 This is set automatically after probe returns, virtio block violated this
 rule by calling add_disk, which causes the VQ to be used directly within
 probe.
 
 To fix, call virtio_enable_vqs_early before using VQs.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/block/virtio_blk.c | 2 ++
  1 file changed, 2 insertions(+)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

[PATCH RFC 00/11] qemu: towards virtio-1 host support

[Cornelia Huck]

This patchset aims to get us some way to implement virtio-1 compliant
and transitional devices in qemu. Branch available at

git://github.com/cohuck/qemu virtio-1

I've mainly focused on:
- endianness handling
- extended feature bits
- virtio-ccw new/changed commands

Thanks go to Thomas for some preliminary work in this area.

I've been able to start guests both with and without the virtio-1 patches
in the linux guest patchset, with virtio-net and virtio-blk devices (with
and without dataplane). virtio-ccw only :) vhost, migration and loads of
other things have been ignored for now.

I'd like to know whether I walk into the right direction, so let's consider
this as a starting point.

Cornelia Huck (8):
  virtio: cull virtio_bus_set_vdev_features
  virtio: support more feature bits
  s390x/virtio-ccw: fix check for WRITE_FEAT
  virtio: introduce legacy virtio devices
  virtio: allow virtio-1 queue layout
  dataplane: allow virtio-1 devices
  s390x/virtio-ccw: support virtio-1 set_vq format
  s390x/virtio-ccw: enable virtio 1.0

Thomas Huth (3):
  linux-headers/virtio_config: Update with VIRTIO_F_VERSION_1
  s390x/css: Add a callback for when subchannel gets disabled
  s390x/virtio-ccw: add virtio set-revision call

 hw/9pfs/virtio-9p-device.c  |7 +-
 hw/block/dataplane/virtio-blk.c |3 +-
 hw/block/virtio-blk.c   |9 +-
 hw/char/virtio-serial-bus.c |9 +-
 hw/net/virtio-net.c |   38 ---
 hw/s390x/css.c  |   12 +++
 hw/s390x/css.h  |1 +
 hw/s390x/s390-virtio-bus.c  |9 +-
 hw/s390x/virtio-ccw.c   |  188 +++
 hw/s390x/virtio-ccw.h   |7 +-
 hw/scsi/vhost-scsi.c|7 +-
 hw/scsi/virtio-scsi-dataplane.c |2 +-
 hw/scsi/virtio-scsi.c   |8 +-
 hw/virtio/Makefile.objs |2 +-
 hw/virtio/dataplane/Makefile.objs   |2 +-
 hw/virtio/dataplane/vring.c |   95 ++
 hw/virtio/virtio-balloon.c  |8 +-
 hw/virtio/virtio-bus.c  |   23 +
 hw/virtio/virtio-mmio.c |9 +-
 hw/virtio/virtio-pci.c  |   13 +--
 hw/virtio/virtio-rng.c  |2 +-
 hw/virtio/virtio.c  |   51 +++---
 include/hw/virtio/dataplane/vring.h |   64 +++-
 include/hw/virtio/virtio-access.h   |4 +
 include/hw/virtio/virtio-bus.h  |   10 +-
 include/hw/virtio/virtio.h  |   34 +--
 linux-headers/linux/virtio_config.h |3 +
 27 files changed, 442 insertions(+), 178 deletions(-)

-- 
1.7.9.5

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

[PATCH RFC 01/11] virtio: use u32, not bitmap for struct virtio_device's features

[Cornelia Huck]

From: Rusty Russell ru...@rustcorp.com.au

It seemed like a good idea, but it's actually a pain when we get more
than 32 feature bits.  Just change it to a u32 for now.

Cc: Brian Swetland swetl...@google.com
Cc: Christian Borntraeger borntrae...@de.ibm.com
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
Acked-by: Pawel Moll pawel.m...@arm.com
Acked-by: Ohad Ben-Cohen o...@wizery.com
---
 drivers/char/virtio_console.c  |2 +-
 drivers/lguest/lguest_device.c |8 
 drivers/remoteproc/remoteproc_virtio.c |2 +-
 drivers/s390/kvm/kvm_virtio.c  |2 +-
 drivers/s390/kvm/virtio_ccw.c  |   23 +--
 drivers/virtio/virtio.c|   10 +-
 drivers/virtio/virtio_mmio.c   |8 ++--
 drivers/virtio/virtio_pci.c|3 +--
 drivers/virtio/virtio_ring.c   |2 +-
 include/linux/virtio.h |3 +--
 include/linux/virtio_config.h  |2 +-
 tools/virtio/linux/virtio.h|   22 +-
 tools/virtio/linux/virtio_config.h |2 +-
 tools/virtio/virtio_test.c |5 ++---
 tools/virtio/vringh_test.c |   16 
 15 files changed, 39 insertions(+), 71 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b585b47..c4a437e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device 
*portdev)
 */
if (!portdev-vdev)
return 0;
-   return portdev-vdev-features[0]  (1  VIRTIO_CONSOLE_F_MULTIPORT);
+   return portdev-vdev-features  (1  VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
 static DEFINE_SPINLOCK(dma_bufs_lock);
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index d0a1d8a..c831c47 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -137,14 +137,14 @@ static void lg_finalize_features(struct virtio_device 
*vdev)
vring_transport_features(vdev);
 
/*
-* The vdev-feature array is a Linux bitmask: this isn't the same as a
-* the simple array of bits used by lguest devices for features.  So we
-* do this slow, manual conversion which is completely general.
+* Since lguest is currently x86-only, we're little-endian.  That
+* means we could just memcpy.  But it's not time critical, and in
+* case someone copies this code, we do it the slow, obvious way.
 */
memset(out_features, 0, desc-feature_len);
bits = min_t(unsigned, desc-feature_len, sizeof(vdev-features)) * 8;
for (i = 0; i  bits; i++) {
-   if (test_bit(i, vdev-features))
+   if (vdev-features  (1  i))
out_features[i / 8] |= (1  (i % 8));
}
 
diff --git a/drivers/remoteproc/remoteproc_virtio.c 
b/drivers/remoteproc/remoteproc_virtio.c
index a34b506..dafaf38 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -231,7 +231,7 @@ static void rproc_virtio_finalize_features(struct 
virtio_device *vdev)
 * Remember the finalized features of our vdev, and provide it
 * to the remote processor once it is powered on.
 */
-   rsc-gfeatures = vdev-features[0];
+   rsc-gfeatures = vdev-features;
 }
 
 static void rproc_virtio_get(struct virtio_device *vdev, unsigned offset,
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index a134965..d747ca4 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -106,7 +106,7 @@ static void kvm_finalize_features(struct virtio_device 
*vdev)
memset(out_features, 0, desc-feature_len);
bits = min_t(unsigned, desc-feature_len, sizeof(vdev-features)) * 8;
for (i = 0; i  bits; i++) {
-   if (test_bit(i, vdev-features))
+   if (vdev-features  (1  i))
out_features[i / 8] |= (1  (i % 8));
}
 }
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index d2c0b44..c5acd19 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -701,7 +701,6 @@ static void virtio_ccw_finalize_features(struct 
virtio_device *vdev)
 {
struct virtio_ccw_device *vcdev = to_vc_device(vdev);
struct virtio_feature_desc *features;
-   int i;
struct ccw1 *ccw;
 
ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
@@ -715,19 +714,15 @@ static void virtio_ccw_finalize_features(struct 
virtio_device *vdev)
/* Give virtio_ring a chance to accept features. */
vring_transport_features(vdev);
 
-   for (i = 0; i  sizeof(*vdev-features) / sizeof(features-features);
-i++) {
-   int highbits = i % 2 ? 32 : 0;
-   features

[PATCH RFC 05/11] virtio_config: endian conversion for v1.0.

[Cornelia Huck]

From: Rusty Russell ru...@rustcorp.com.au

Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 include/linux/virtio_config.h |9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index a0e16d8..ca22e3a 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -222,12 +222,13 @@ static inline u16 virtio_cread16(struct virtio_device 
*vdev,
 {
u16 ret;
vdev-config-get(vdev, offset, ret, sizeof(ret));
-   return ret;
+   return virtio_to_cpu_u16(vdev, ret);
 }
 
 static inline void virtio_cwrite16(struct virtio_device *vdev,
   unsigned int offset, u16 val)
 {
+   val = cpu_to_virtio_u16(vdev, val);
vdev-config-set(vdev, offset, val, sizeof(val));
 }
 
@@ -236,12 +237,13 @@ static inline u32 virtio_cread32(struct virtio_device 
*vdev,
 {
u32 ret;
vdev-config-get(vdev, offset, ret, sizeof(ret));
-   return ret;
+   return virtio_to_cpu_u32(vdev, ret);
 }
 
 static inline void virtio_cwrite32(struct virtio_device *vdev,
   unsigned int offset, u32 val)
 {
+   val = cpu_to_virtio_u32(vdev, val);
vdev-config-set(vdev, offset, val, sizeof(val));
 }
 
@@ -250,12 +252,13 @@ static inline u64 virtio_cread64(struct virtio_device 
*vdev,
 {
u64 ret;
vdev-config-get(vdev, offset, ret, sizeof(ret));
-   return ret;
+   return virtio_to_cpu_u64(vdev, ret);
 }
 
 static inline void virtio_cwrite64(struct virtio_device *vdev,
   unsigned int offset, u64 val)
 {
+   val = cpu_to_virtio_u64(vdev, val);
vdev-config-set(vdev, offset, val, sizeof(val));
 }
 
-- 
1.7.9.5

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

[PATCH RFC 06/11] virtio: allow virtio-1 queue layout

[Cornelia Huck]

For virtio-1 devices, we allow a more complex queue layout that doesn't
require descriptor table and rings on a physically-contigous memory area:
add virtio_queue_set_rings() to allow transports to set this up.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/virtio/virtio.c |   16 
 include/hw/virtio/virtio.h |2 ++
 2 files changed, 18 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e6ae3a0..147d227 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -96,6 +96,13 @@ static void virtqueue_init(VirtQueue *vq)
 {
 hwaddr pa = vq-pa;
 
+if (pa == -1ULL) {
+/*
+ * This is a virtio-1 style vq that has already been setup
+ * in virtio_queue_set.
+ */
+return;
+}
 vq-vring.desc = pa;
 vq-vring.avail = pa + vq-vring.num * sizeof(VRingDesc);
 vq-vring.used = vring_align(vq-vring.avail +
@@ -719,6 +726,15 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
 return vdev-vq[n].pa;
 }
 
+void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
+hwaddr avail, hwaddr used)
+{
+vdev-vq[n].pa = -1ULL;
+vdev-vq[n].vring.desc = desc;
+vdev-vq[n].vring.avail = avail;
+vdev-vq[n].vring.used = used;
+}
+
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
 {
 /* Don't allow guest to flip queue between existent and
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 40e567c..f840320 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -227,6 +227,8 @@ void virtio_queue_set_addr(VirtIODevice *vdev, int n, 
hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
+void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
+hwaddr avail, hwaddr used);
 void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
-- 
1.7.9.5

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

[PATCH RFC 10/11] KVM: s390: virtio-ccw revision 1 SET_VQ

[Cornelia Huck]

The CCW_CMD_SET_VQ command has a different format for revision 1+
devices, allowing to specify a more complex virtqueue layout. For
now, we stay however with the old layout and simply use the new
command format for virtio-1 devices.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/s390/kvm/virtio_ccw.c |   54 -
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index cbe2ba8..f97d3fb 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -68,13 +68,22 @@ struct virtio_ccw_device {
void *airq_info;
 };
 
-struct vq_info_block {
+struct vq_info_block_legacy {
__u64 queue;
__u32 align;
__u16 index;
__u16 num;
 } __packed;
 
+struct vq_info_block {
+   __u64 desc;
+   __u32 res0;
+   __u16 index;
+   __u16 num;
+   __u64 avail;
+   __u64 used;
+} __packed;
+
 struct virtio_feature_desc {
__u32 features;
__u8 index;
@@ -100,7 +109,10 @@ struct virtio_ccw_vq_info {
struct virtqueue *vq;
int num;
void *queue;
-   struct vq_info_block *info_block;
+   union {
+   struct vq_info_block s;
+   struct vq_info_block_legacy l;
+   } *info_block;
int bit_nr;
struct list_head node;
long cookie;
@@ -411,13 +423,22 @@ static void virtio_ccw_del_vq(struct virtqueue *vq, 
struct ccw1 *ccw)
spin_unlock_irqrestore(vcdev-lock, flags);
 
/* Release from host. */
-   info-info_block-queue = 0;
-   info-info_block-align = 0;
-   info-info_block-index = index;
-   info-info_block-num = 0;
+   if (vcdev-revision == 0) {
+   info-info_block-l.queue = 0;
+   info-info_block-l.align = 0;
+   info-info_block-l.index = index;
+   info-info_block-l.num = 0;
+   ccw-count = sizeof(info-info_block-l);
+   } else {
+   info-info_block-s.desc = 0;
+   info-info_block-s.index = index;
+   info-info_block-s.num = 0;
+   info-info_block-s.avail = 0;
+   info-info_block-s.used = 0;
+   ccw-count = sizeof(info-info_block-s);
+   }
ccw-cmd_code = CCW_CMD_SET_VQ;
ccw-flags = 0;
-   ccw-count = sizeof(*info-info_block);
ccw-cda = (__u32)(unsigned long)(info-info_block);
ret = ccw_io_helper(vcdev, ccw,
VIRTIO_CCW_DOING_SET_VQ | index);
@@ -500,13 +521,22 @@ static struct virtqueue *virtio_ccw_setup_vq(struct 
virtio_device *vdev,
}
 
/* Register it with the host. */
-   info-info_block-queue = (__u64)info-queue;
-   info-info_block-align = KVM_VIRTIO_CCW_RING_ALIGN;
-   info-info_block-index = i;
-   info-info_block-num = info-num;
+   if (vcdev-revision == 0) {
+   info-info_block-l.queue = (__u64)info-queue;
+   info-info_block-l.align = KVM_VIRTIO_CCW_RING_ALIGN;
+   info-info_block-l.index = i;
+   info-info_block-l.num = info-num;
+   ccw-count = sizeof(info-info_block-l);
+   } else {
+   info-info_block-s.desc = (__u64)info-queue;
+   info-info_block-s.index = i;
+   info-info_block-s.num = info-num;
+   info-info_block-s.avail = (__u64)virtqueue_get_avail(vq);
+   info-info_block-s.used = (__u64)virtqueue_get_used(vq);
+   ccw-count = sizeof(info-info_block-s);
+   }
ccw-cmd_code = CCW_CMD_SET_VQ;
ccw-flags = 0;
-   ccw-count = sizeof(*info-info_block);
ccw-cda = (__u32)(unsigned long)(info-info_block);
err = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_SET_VQ | i);
if (err) {
-- 
1.7.9.5

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

[PATCH RFC 09/11] s390x/virtio-ccw: add virtio set-revision call

[Cornelia Huck]

From: Thomas Huth th...@linux.vnet.ibm.com

Handle the virtio-ccw revision according to what the guest sets.
When revision 1 is selected, we have a virtio-1 standard device
with byteswapping for the virtio rings.

When a channel gets disabled, we have to revert to the legacy behavior
in case the next user of the device does not negotiate the revision 1
anymore (e.g. the boot firmware uses revision 1, but the operating
system only uses the legacy mode).

Note that revisions  0 are still disabled; but we still extend the
feature bit size to be able to handle the VERSION_1 bit.

Signed-off-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/virtio-ccw.c |   54 +
 hw/s390x/virtio-ccw.h |7 ++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 69add47..0d414f6 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -20,9 +20,11 @@
 #include hw/virtio/virtio-net.h
 #include hw/sysbus.h
 #include qemu/bitops.h
+#include hw/virtio/virtio-access.h
 #include hw/virtio/virtio-bus.h
 #include hw/s390x/adapter.h
 #include hw/s390x/s390_flic.h
+#include linux/virtio_config.h
 
 #include ioinst.h
 #include css.h
@@ -260,6 +262,12 @@ typedef struct VirtioThinintInfo {
 uint8_t isc;
 } QEMU_PACKED VirtioThinintInfo;
 
+typedef struct VirtioRevInfo {
+uint16_t revision;
+uint16_t length;
+uint8_t data[0];
+} QEMU_PACKED VirtioRevInfo;
+
 /* Specify where the virtqueues for the subchannel are in guest memory. */
 static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
   uint16_t index, uint16_t num)
@@ -299,6 +307,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
 {
 int ret;
 VqInfoBlock info;
+VirtioRevInfo revinfo;
 uint8_t status;
 VirtioFeatDesc features;
 void *config;
@@ -373,6 +382,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
 if (features.index  ARRAY_SIZE(dev-host_features)) {
 features.features = dev-host_features[features.index];
+/*
+ * Don't offer version 1 to the guest if it did not
+ * negotiate at least revision 1.
+ */
+if (features.index == 1  dev-revision = 0) {
+features.features = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 } else {
 /* Return zeroes if the guest supports more feature bits. */
 features.features = 0;
@@ -400,6 +416,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
 features.features = ldl_le_phys(address_space_memory, ccw.cda);
 if (features.index  ARRAY_SIZE(vdev-guest_features)) {
+/*
+ * The guest should not set version 1 if it didn't
+ * negotiate a revision = 1.
+ */
+if (features.index == 1  dev-revision = 0) {
+features.features = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 virtio_set_features(vdev, features.index, features.features);
 } else {
 /*
@@ -600,6 +623,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) {
+ret = -ENOSYS;
+break;
+}
+ret = 0;
+dev-revision = revinfo.revision;
+break;
 default:
 ret = -ENOSYS;
 break;
@@ -607,6 +649,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
 return ret;
 }
 
+static void virtio_sch_disable_cb(SubchDev *sch)
+{
+VirtioCcwDevice *dev = sch-driver_data;
+
+dev-revision = -1;
+}
+
 static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
 {
 unsigned int cssid = 0;
@@ -733,6 +782,7 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, 
VirtIODevice *vdev)
 css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE);
 
 sch-ccw_cb = virtio_ccw_cb;
+sch-disable_cb = virtio_sch_disable_cb;
 
 /* Build senseid data. */
 memset(sch-id, 0, sizeof(SenseId));
@@ -740,6 +790,8 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, 
VirtIODevice *vdev)
 sch-id.cu_type = VIRTIO_CCW_CU_TYPE;
 sch-id.cu_model = vdev-device_id;
 
+dev-revision

[PATCH RFC 02/11] virtio: cull virtio_bus_set_vdev_features

[Cornelia Huck]

The only user of this function was virtio-ccw, and it should use
virtio_set_features() like everybody else: We need to make sure
that bad features are masked out properly, which this function did
not do.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/virtio-ccw.c  |3 +--
 hw/virtio/virtio-bus.c |   14 --
 include/hw/virtio/virtio-bus.h |3 ---
 3 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e7d3ea1..7833dd2 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -400,8 +400,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
 features.features = ldl_le_phys(address_space_memory, ccw.cda);
 if (features.index  ARRAY_SIZE(dev-host_features)) {
-virtio_bus_set_vdev_features(dev-bus, features.features);
-vdev-guest_features = features.features;
+virtio_set_features(vdev, features.features);
 } else {
 /*
  * If the guest supports more feature bits, assert that it
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index eb77019..a8ffa07 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -109,20 +109,6 @@ uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
 return k-get_features(vdev, requested_features);
 }
 
-/* Set the features of the plugged device. */
-void virtio_bus_set_vdev_features(VirtioBusState *bus,
-  uint32_t requested_features)
-{
-VirtIODevice *vdev = virtio_bus_get_device(bus);
-VirtioDeviceClass *k;
-
-assert(vdev != NULL);
-k = VIRTIO_DEVICE_GET_CLASS(vdev);
-if (k-set_features != NULL) {
-k-set_features(vdev, requested_features);
-}
-}
-
 /* Get bad features of the plugged device. */
 uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus)
 {
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 0756545..0d2e7b4 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -84,9 +84,6 @@ size_t virtio_bus_get_vdev_config_len(VirtioBusState *bus);
 /* Get the features of the plugged device. */
 uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
 uint32_t requested_features);
-/* Set the features of the plugged device. */
-void virtio_bus_set_vdev_features(VirtioBusState *bus,
-  uint32_t requested_features);
 /* Get bad features of the plugged device. */
 uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus);
 /* Get config of the plugged device. */
-- 
1.7.9.5

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

[PATCH RFC 07/11] dataplane: allow virtio-1 devices

[Cornelia Huck]

Handle endianness conversion for virtio-1 virtqueues correctly.

Note that dataplane now needs to be built per-target.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/block/dataplane/virtio-blk.c |3 +-
 hw/scsi/virtio-scsi-dataplane.c |2 +-
 hw/virtio/Makefile.objs |2 +-
 hw/virtio/dataplane/Makefile.objs   |2 +-
 hw/virtio/dataplane/vring.c |   85 +++
 include/hw/virtio/dataplane/vring.h |   64 --
 6 files changed, 113 insertions(+), 45 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5458f9d..eb45a3d 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -16,6 +16,7 @@
 #include qemu/iov.h
 #include qemu/thread.h
 #include qemu/error-report.h
+#include hw/virtio/virtio-access.h
 #include hw/virtio/dataplane/vring.h
 #include block/block.h
 #include hw/virtio/virtio-blk.h
@@ -75,7 +76,7 @@ static void complete_request_vring(VirtIOBlockReq *req, 
unsigned char status)
 VirtIOBlockDataPlane *s = req-dev-dataplane;
 stb_p(req-in-status, status);
 
-vring_push(req-dev-dataplane-vring, req-elem,
+vring_push(s-vdev, req-dev-dataplane-vring, req-elem,
req-qiov.size + sizeof(*req-in));
 
 /* Suppress notification to guest by BH and its scheduled
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index b778e05..3e2b706 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -81,7 +81,7 @@ VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
 
 void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
 {
-vring_push(req-vring-vring, req-elem,
+vring_push((VirtIODevice *)req-dev, req-vring-vring, req-elem,
req-qsgl.size + req-resp_iov.size);
 event_notifier_set(req-vring-guest_notifier);
 }
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index d21c397..19b224a 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -2,7 +2,7 @@ common-obj-y += virtio-rng.o
 common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 common-obj-y += virtio-bus.o
 common-obj-y += virtio-mmio.o
-common-obj-$(CONFIG_VIRTIO) += dataplane/
+obj-$(CONFIG_VIRTIO) += dataplane/
 
 obj-y += virtio.o virtio-balloon.o 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
diff --git a/hw/virtio/dataplane/Makefile.objs 
b/hw/virtio/dataplane/Makefile.objs
index 9a8cfc0..753a9ca 100644
--- a/hw/virtio/dataplane/Makefile.objs
+++ b/hw/virtio/dataplane/Makefile.objs
@@ -1 +1 @@
-common-obj-y += vring.o
+obj-y += vring.o
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index b84957f..4624521 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -18,6 +18,7 @@
 #include hw/hw.h
 #include exec/memory.h
 #include exec/address-spaces.h
+#include hw/virtio/virtio-access.h
 #include hw/virtio/dataplane/vring.h
 #include qemu/error-report.h
 
@@ -83,7 +84,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
 vring_init(vring-vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
 
 vring-last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
-vring-last_used_idx = vring-vr.used-idx;
+vring-last_used_idx = vring_get_used_idx(vdev, vring);
 vring-signalled_used = 0;
 vring-signalled_used_valid = false;
 
@@ -104,7 +105,7 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
 void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
 {
 if (!(vdev-guest_features[0]  (1  VIRTIO_RING_F_EVENT_IDX))) {
-vring-vr.used-flags |= VRING_USED_F_NO_NOTIFY;
+vring_set_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
 }
 }
 
@@ -117,10 +118,10 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring 
*vring)
 if (vdev-guest_features[0]  (1  VIRTIO_RING_F_EVENT_IDX)) {
 vring_avail_event(vring-vr) = vring-vr.avail-idx;
 } else {
-vring-vr.used-flags = ~VRING_USED_F_NO_NOTIFY;
+vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
 }
 smp_mb(); /* ensure update is seen before reading avail_idx */
-return !vring_more_avail(vring);
+return !vring_more_avail(vdev, vring);
 }
 
 /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
@@ -134,12 +135,13 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
 smp_mb();
 
 if ((vdev-guest_features[0]  VIRTIO_F_NOTIFY_ON_EMPTY) 
-unlikely(vring-vr.avail-idx == vring-last_avail_idx)) {
+unlikely(!vring_more_avail(vdev, vring))) {
 return true;
 }
 
 if (!(vdev-guest_features[0]  VIRTIO_RING_F_EVENT_IDX)) {
-return !(vring-vr.avail-flags  VRING_AVAIL_F_NO_INTERRUPT);
+return !(vring_get_avail_flags(vdev, vring) 
+ VRING_AVAIL_F_NO_INTERRUPT);
 }
 old = vring-signalled_used;
 v = vring-signalled_used_valid

[PATCH RFC 07/11] virtio_net: use v1.0 endian.

[Cornelia Huck]

From: Rusty Russell ru...@rustcorp.com.au

[Cornelia Huck: converted some missed fields]
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c |   31 +++
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 59caa06..cd18946 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -353,13 +353,14 @@ err:
 }
 
 static struct sk_buff *receive_mergeable(struct net_device *dev,
+struct virtnet_info *vi,
 struct receive_queue *rq,
 unsigned long ctx,
 unsigned int len)
 {
void *buf = mergeable_ctx_to_buf_address(ctx);
struct skb_vnet_hdr *hdr = buf;
-   int num_buf = hdr-mhdr.num_buffers;
+   u16 num_buf = virtio_to_cpu_u16(rq-vq-vdev, hdr-mhdr.num_buffers);
struct page *page = virt_to_head_page(buf);
int offset = buf - page_address(page);
unsigned int truesize = max(len, mergeable_ctx_to_buf_truesize(ctx));
@@ -375,7 +376,9 @@ static struct sk_buff *receive_mergeable(struct net_device 
*dev,
ctx = (unsigned long)virtqueue_get_buf(rq-vq, len);
if (unlikely(!ctx)) {
pr_debug(%s: rx error: %d buffers out of %d missing\n,
-dev-name, num_buf, hdr-mhdr.num_buffers);
+dev-name, num_buf,
+virtio_to_cpu_u16(rq-vq-vdev,
+  hdr-mhdr.num_buffers));
dev-stats.rx_length_errors++;
goto err_buf;
}
@@ -460,7 +463,7 @@ static void receive_buf(struct receive_queue *rq, void 
*buf, unsigned int len)
}
 
if (vi-mergeable_rx_bufs)
-   skb = receive_mergeable(dev, rq, (unsigned long)buf, len);
+   skb = receive_mergeable(dev, vi, rq, (unsigned long)buf, len);
else if (vi-big_packets)
skb = receive_big(dev, rq, buf, len);
else
@@ -479,8 +482,8 @@ static void receive_buf(struct receive_queue *rq, void 
*buf, unsigned int len)
if (hdr-hdr.flags  VIRTIO_NET_HDR_F_NEEDS_CSUM) {
pr_debug(Needs csum!\n);
if (!skb_partial_csum_set(skb,
- hdr-hdr.csum_start,
- hdr-hdr.csum_offset))
+ virtio_to_cpu_u16(vi-vdev, hdr-hdr.csum_start),
+ virtio_to_cpu_u16(vi-vdev, hdr-hdr.csum_offset)))
goto frame_err;
} else if (hdr-hdr.flags  VIRTIO_NET_HDR_F_DATA_VALID) {
skb-ip_summed = CHECKSUM_UNNECESSARY;
@@ -511,7 +514,8 @@ static void receive_buf(struct receive_queue *rq, void 
*buf, unsigned int len)
if (hdr-hdr.gso_type  VIRTIO_NET_HDR_GSO_ECN)
skb_shinfo(skb)-gso_type |= SKB_GSO_TCP_ECN;
 
-   skb_shinfo(skb)-gso_size = hdr-hdr.gso_size;
+   skb_shinfo(skb)-gso_size = virtio_to_cpu_u16(vi-vdev,
+ 
hdr-hdr.gso_size);
if (skb_shinfo(skb)-gso_size == 0) {
net_warn_ratelimited(%s: zero gso size.\n, dev-name);
goto frame_err;
@@ -871,16 +875,19 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff 
*skb)
 
if (skb-ip_summed == CHECKSUM_PARTIAL) {
hdr-hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
-   hdr-hdr.csum_start = skb_checksum_start_offset(skb);
-   hdr-hdr.csum_offset = skb-csum_offset;
+   hdr-hdr.csum_start = cpu_to_virtio_u16(vi-vdev,
+   skb_checksum_start_offset(skb));
+   hdr-hdr.csum_offset = cpu_to_virtio_u16(vi-vdev,
+skb-csum_offset);
} else {
hdr-hdr.flags = 0;
hdr-hdr.csum_offset = hdr-hdr.csum_start = 0;
}
 
if (skb_is_gso(skb)) {
-   hdr-hdr.hdr_len = skb_headlen(skb);
-   hdr-hdr.gso_size = skb_shinfo(skb)-gso_size;
+   hdr-hdr.hdr_len = cpu_to_virtio_u16(vi-vdev, 
skb_headlen(skb));
+   hdr-hdr.gso_size = cpu_to_virtio_u16(vi-vdev,
+ 
skb_shinfo(skb)-gso_size);
if (skb_shinfo(skb)-gso_type  SKB_GSO_TCPV4)
hdr-hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (skb_shinfo(skb)-gso_type  SKB_GSO_TCPV6)
@@ -1181,7 +1188,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
sg_init_table(sg, 2);
 
/* Store the unicast

[PATCH RFC 05/11] virtio: introduce legacy virtio devices

[Cornelia Huck]

Introduce a helper function to indicate  whether a virtio device is
operating in legacy or virtio standard mode.

It may be used to make decisions about the endianess of virtio accesses
and other virtio-1 specific changes, enabling us to support transitional
devices.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/virtio/virtio.c|6 +-
 include/hw/virtio/virtio-access.h |4 
 include/hw/virtio/virtio.h|   13 +++--
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7aaa953..e6ae3a0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void *opaque)
 VirtIODevice *vdev = opaque;
 
 assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
-return vdev-device_endian != virtio_default_endian();
+if (virtio_device_is_legacy(vdev)) {
+return vdev-device_endian != virtio_default_endian();
+}
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
 }
 
 static const VMStateDescription vmstate_virtio_device_endian = {
diff --git a/include/hw/virtio/virtio-access.h 
b/include/hw/virtio/virtio-access.h
index 46456fd..c123ee0 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -19,6 +19,10 @@
 
 static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 {
+if (!virtio_device_is_legacy(vdev)) {
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return false;
+}
 #if defined(TARGET_IS_BIENDIAN)
 return virtio_is_big_endian(vdev);
 #elif defined(TARGET_WORDS_BIGENDIAN)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b408166..40e567c 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -275,9 +275,18 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue 
*vq, bool assign,
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
 
+static inline bool virtio_device_is_legacy(VirtIODevice *vdev)
+{
+return !(vdev-guest_features[1]  (1  (VIRTIO_F_VERSION_1 - 32)));
+}
+
 static inline bool virtio_is_big_endian(VirtIODevice *vdev)
 {
-assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
-return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+if (virtio_device_is_legacy(vdev)) {
+assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return false;
 }
 #endif
-- 
1.7.9.5

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

[PATCH RFC 02/11] virtio: add support for 64 bit features.

[Cornelia Huck]

From: Rusty Russell ru...@rustcorp.com.au

Change the u32 to a u64, and make sure to use 1ULL everywhere!

Cc: Brian Swetland swetl...@google.com
Cc: Christian Borntraeger borntrae...@de.ibm.com
[Thomas Huth: fix up virtio-ccw get_features]
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
Acked-by: Pawel Moll pawel.m...@arm.com
Acked-by: Ohad Ben-Cohen o...@wizery.com
---
 drivers/char/virtio_console.c  |2 +-
 drivers/lguest/lguest_device.c |   10 +-
 drivers/remoteproc/remoteproc_virtio.c |5 -
 drivers/s390/kvm/kvm_virtio.c  |   10 +-
 drivers/s390/kvm/virtio_ccw.c  |   29 -
 drivers/virtio/virtio.c|   12 ++--
 drivers/virtio/virtio_mmio.c   |   14 +-
 drivers/virtio/virtio_pci.c|5 ++---
 drivers/virtio/virtio_ring.c   |2 +-
 include/linux/virtio.h |2 +-
 include/linux/virtio_config.h  |8 
 tools/virtio/linux/virtio.h|2 +-
 tools/virtio/linux/virtio_config.h |2 +-
 13 files changed, 64 insertions(+), 39 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c4a437e..f9c6288 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device 
*portdev)
 */
if (!portdev-vdev)
return 0;
-   return portdev-vdev-features  (1  VIRTIO_CONSOLE_F_MULTIPORT);
+   return portdev-vdev-features  (1ULL  VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
 static DEFINE_SPINLOCK(dma_bufs_lock);
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index c831c47..4d29bcd 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -94,17 +94,17 @@ static unsigned desc_size(const struct lguest_device_desc 
*desc)
 }
 
 /* This gets the device's feature bits. */
-static u32 lg_get_features(struct virtio_device *vdev)
+static u64 lg_get_features(struct virtio_device *vdev)
 {
unsigned int i;
-   u32 features = 0;
+   u64 features = 0;
struct lguest_device_desc *desc = to_lgdev(vdev)-desc;
u8 *in_features = lg_features(desc);
 
/* We do this the slow but generic way. */
-   for (i = 0; i  min(desc-feature_len * 8, 32); i++)
+   for (i = 0; i  min(desc-feature_len * 8, 64); i++)
if (in_features[i / 8]  (1  (i % 8)))
-   features |= (1  i);
+   features |= (1ULL  i);
 
return features;
 }
@@ -144,7 +144,7 @@ static void lg_finalize_features(struct virtio_device *vdev)
memset(out_features, 0, desc-feature_len);
bits = min_t(unsigned, desc-feature_len, sizeof(vdev-features)) * 8;
for (i = 0; i  bits; i++) {
-   if (vdev-features  (1  i))
+   if (vdev-features  (1ULL  i))
out_features[i / 8] |= (1  (i % 8));
}
 
diff --git a/drivers/remoteproc/remoteproc_virtio.c 
b/drivers/remoteproc/remoteproc_virtio.c
index dafaf38..627737e 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -207,7 +207,7 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
 }
 
 /* provide the vdev features as retrieved from the firmware */
-static u32 rproc_virtio_get_features(struct virtio_device *vdev)
+static u64 rproc_virtio_get_features(struct virtio_device *vdev)
 {
struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
struct fw_rsc_vdev *rsc;
@@ -227,6 +227,9 @@ static void rproc_virtio_finalize_features(struct 
virtio_device *vdev)
/* 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);
+
/*
 * Remember the finalized features of our vdev, and provide it
 * to the remote processor once it is powered on.
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index d747ca4..6d4cbea 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -80,16 +80,16 @@ static unsigned desc_size(const struct kvm_device_desc 
*desc)
 }
 
 /* This gets the device's feature bits. */
-static u32 kvm_get_features(struct virtio_device *vdev)
+static u64 kvm_get_features(struct virtio_device *vdev)
 {
unsigned int i;
-   u32 features = 0;
+   u64 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++)
+   for (i = 0; i  min(desc-feature_len * 8, 64); i++)
if (in_features[i / 8]  (1  (i % 8)))
-   features |= (1  i);
+   features

[PATCH RFC 03/11] virtio: endianess conversion helpers

[Cornelia Huck]

Provide helper functions that convert from/to LE for virtio devices that
are not operating in legacy mode. We check for the VERSION_1 feature bit
to determine that.

Based on original patches by Rusty Russell and Thomas Huth.

Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/virtio/virtio.c|4 
 include/linux/virtio.h |   40 
 include/uapi/linux/virtio_config.h |3 +++
 3 files changed, 47 insertions(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index cfd5d00..8f74cd6 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -144,6 +144,10 @@ static int virtio_dev_probe(struct device *_d)
if (device_features  (1ULL  i))
dev-features |= (1ULL  i);
 
+   /* Version 1.0 compliant devices set the VIRTIO_F_VERSION_1 bit */
+   if (device_features  (1ULL  VIRTIO_F_VERSION_1))
+   dev-features |= (1ULL  VIRTIO_F_VERSION_1);
+
dev-config-finalize_features(dev);
 
err = drv-probe(dev);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index a24b41f..68cadd4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -9,6 +9,7 @@
 #include linux/mod_devicetable.h
 #include linux/gfp.h
 #include linux/vringh.h
+#include uapi/linux/virtio_config.h
 
 /**
  * virtqueue - a queue to register buffers for sending or receiving.
@@ -102,6 +103,11 @@ static inline struct virtio_device *dev_to_virtio(struct 
device *_dev)
return container_of(_dev, struct virtio_device, dev);
 }
 
+static inline bool virtio_device_legacy(const struct virtio_device *dev)
+{
+   return !(dev-features  (1ULL  VIRTIO_F_VERSION_1));
+}
+
 int register_virtio_device(struct virtio_device *dev);
 void unregister_virtio_device(struct virtio_device *dev);
 
@@ -149,4 +155,38 @@ void unregister_virtio_driver(struct virtio_driver *drv);
 #define module_virtio_driver(__virtio_driver) \
module_driver(__virtio_driver, register_virtio_driver, \
unregister_virtio_driver)
+
+/*
+ * v1.0 specifies LE headers, legacy was native endian. Therefore, we must
+ * convert from/to LE if and only if vdev is not legacy.
+ */
+static inline u16 virtio_to_cpu_u16(const struct virtio_device *vdev, u16 v)
+{
+   return virtio_device_legacy(vdev) ? v : le16_to_cpu(v);
+}
+
+static inline u32 virtio_to_cpu_u32(const struct virtio_device *vdev, u32 v)
+{
+   return virtio_device_legacy(vdev) ? v : le32_to_cpu(v);
+}
+
+static inline u64 virtio_to_cpu_u64(const struct virtio_device *vdev, u64 v)
+{
+   return virtio_device_legacy(vdev) ? v : le64_to_cpu(v);
+}
+
+static inline u16 cpu_to_virtio_u16(const struct virtio_device *vdev, u16 v)
+{
+   return virtio_device_legacy(vdev) ? v : cpu_to_le16(v);
+}
+
+static inline u32 cpu_to_virtio_u32(const struct virtio_device *vdev, u32 v)
+{
+   return virtio_device_legacy(vdev) ? v : cpu_to_le32(v);
+}
+
+static inline u64 cpu_to_virtio_u64(const struct virtio_device *vdev, u64 v)
+{
+   return virtio_device_legacy(vdev) ? v : cpu_to_le64(v);
+}
 #endif /* _LINUX_VIRTIO_H */
diff --git a/include/uapi/linux/virtio_config.h 
b/include/uapi/linux/virtio_config.h
index 3ce768c..80e7381 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -54,4 +54,7 @@
 /* Can the device handle any descriptor layout? */
 #define VIRTIO_F_ANY_LAYOUT27
 
+/* v1.0 compliant. */
+#define VIRTIO_F_VERSION_1 32
+
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
-- 
1.7.9.5

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

[PATCH RFC 04/11] virtio_ring: implement endian reversal based on VERSION_1 feature.

[Cornelia Huck]

From: Rusty Russell ru...@rustcorp.com.au

[Cornelia Huck: we don't need the vq-vring.num - vq-ring_mask change]
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/virtio/virtio_ring.c |  195 ++
 1 file changed, 138 insertions(+), 57 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 1cfb5ba..350c39b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -145,42 +145,54 @@ static inline int vring_add_indirect(struct 
vring_virtqueue *vq,
i = 0;
for (n = 0; n  out_sgs; n++) {
for (sg = sgs[n]; sg; sg = next(sg, total_out)) {
-   desc[i].flags = VRING_DESC_F_NEXT;
-   desc[i].addr = sg_phys(sg);
-   desc[i].len = sg-length;
-   desc[i].next = i+1;
+   desc[i].flags = cpu_to_virtio_u16(vq-vq.vdev,
+ VRING_DESC_F_NEXT);
+   desc[i].addr = cpu_to_virtio_u64(vq-vq.vdev,
+sg_phys(sg));
+   desc[i].len = cpu_to_virtio_u32(vq-vq.vdev,
+   sg-length);
+   desc[i].next = cpu_to_virtio_u16(vq-vq.vdev,
+i+1);
i++;
}
}
for (; n  (out_sgs + in_sgs); n++) {
for (sg = sgs[n]; sg; sg = next(sg, total_in)) {
-   desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
-   desc[i].addr = sg_phys(sg);
-   desc[i].len = sg-length;
-   desc[i].next = i+1;
+   desc[i].flags = cpu_to_virtio_u16(vq-vq.vdev,
+ VRING_DESC_F_NEXT|
+ VRING_DESC_F_WRITE);
+   desc[i].addr = cpu_to_virtio_u64(vq-vq.vdev,
+sg_phys(sg));
+   desc[i].len = cpu_to_virtio_u32(vq-vq.vdev,
+   sg-length);
+   desc[i].next = cpu_to_virtio_u16(vq-vq.vdev, i+1);
i++;
}
}
-   BUG_ON(i != total_sg);
 
/* Last one doesn't continue. */
-   desc[i-1].flags = ~VRING_DESC_F_NEXT;
+   desc[i-1].flags = ~cpu_to_virtio_u16(vq-vq.vdev, VRING_DESC_F_NEXT);
desc[i-1].next = 0;
 
-   /* We're about to use a buffer */
-   vq-vq.num_free--;
-
/* Use a single buffer which doesn't continue */
head = vq-free_head;
-   vq-vring.desc[head].flags = VRING_DESC_F_INDIRECT;
-   vq-vring.desc[head].addr = virt_to_phys(desc);
+   vq-vring.desc[head].flags =
+   cpu_to_virtio_u16(vq-vq.vdev, VRING_DESC_F_INDIRECT);
+   vq-vring.desc[head].addr =
+   cpu_to_virtio_u64(vq-vq.vdev, virt_to_phys(desc));
/* kmemleak gives a false positive, as it's hidden by virt_to_phys */
kmemleak_ignore(desc);
-   vq-vring.desc[head].len = i * sizeof(struct vring_desc);
+   vq-vring.desc[head].len =
+   cpu_to_virtio_u32(vq-vq.vdev, i * sizeof(struct vring_desc));
 
-   /* Update free pointer */
+   BUG_ON(i != total_sg);
+
+   /* Update free pointer (we store this in native endian) */
vq-free_head = vq-vring.desc[head].next;
 
+   /* We've just used a buffer */
+   vq-vq.num_free--;
+
return head;
 }
 
@@ -199,6 +211,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
struct scatterlist *sg;
unsigned int i, n, avail, uninitialized_var(prev), total_sg;
int head;
+   u16 nexti;
 
START_USE(vq);
 
@@ -253,26 +266,46 @@ static inline int virtqueue_add(struct virtqueue *_vq,
vq-vq.num_free -= total_sg;
 
head = i = vq-free_head;
+
for (n = 0; n  out_sgs; n++) {
for (sg = sgs[n]; sg; sg = next(sg, total_out)) {
-   vq-vring.desc[i].flags = VRING_DESC_F_NEXT;
-   vq-vring.desc[i].addr = sg_phys(sg);
-   vq-vring.desc[i].len = sg-length;
+   vq-vring.desc[i].flags =
+   cpu_to_virtio_u16(vq-vq.vdev,
+ VRING_DESC_F_NEXT);
+   vq-vring.desc[i].addr =
+   cpu_to_virtio_u64(vq-vq.vdev, sg_phys(sg));
+   vq-vring.desc[i].len =
+   cpu_to_virtio_u32(vq-vq.vdev, sg-length);
+
+   /* We chained .next in native: fix endian

[PATCH RFC 10/11] s390x/virtio-ccw: support virtio-1 set_vq format

[Cornelia Huck]

Support the new CCW_CMD_SET_VQ format for virtio-1 devices.

While we're at it, refactor the code a bit and enforce big endian
fields (which had always been required, even for legacy).

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/virtio-ccw.c |  114 ++---
 1 file changed, 80 insertions(+), 34 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 0d414f6..80efe88 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -238,11 +238,20 @@ VirtualCssBus *virtual_css_bus_init(void)
 }
 
 /* Communication blocks used by several channel commands. */
-typedef struct VqInfoBlock {
+typedef struct VqInfoBlockLegacy {
 uint64_t queue;
 uint32_t align;
 uint16_t index;
 uint16_t num;
+} QEMU_PACKED VqInfoBlockLegacy;
+
+typedef struct VqInfoBlock {
+uint64_t desc;
+uint32_t res0;
+uint16_t index;
+uint16_t num;
+uint64_t avail;
+uint64_t used;
 } QEMU_PACKED VqInfoBlock;
 
 typedef struct VqConfigBlock {
@@ -269,17 +278,20 @@ typedef struct VirtioRevInfo {
 } QEMU_PACKED VirtioRevInfo;
 
 /* Specify where the virtqueues for the subchannel are in guest memory. */
-static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
-  uint16_t index, uint16_t num)
+static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
+  VqInfoBlockLegacy *linfo)
 {
 VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
+uint16_t index = info ? info-index : linfo-index;
+uint16_t num = info ? info-num : linfo-num;
+uint64_t desc = info ? info-desc : linfo-queue;
 
 if (index  VIRTIO_PCI_QUEUE_MAX) {
 return -EINVAL;
 }
 
 /* Current code in virtio.c relies on 4K alignment. */
-if (addr  (align != 4096)) {
+if (linfo  desc  (linfo-align != 4096)) {
 return -EINVAL;
 }
 
@@ -287,8 +299,12 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t 
addr, uint32_t align,
 return -EINVAL;
 }
 
-virtio_queue_set_addr(vdev, index, addr);
-if (!addr) {
+if (info) {
+virtio_queue_set_rings(vdev, index, desc, info-avail, info-used);
+} else {
+virtio_queue_set_addr(vdev, index, desc);
+}
+if (!desc) {
 virtio_queue_set_vector(vdev, index, 0);
 } else {
 /* Fail if we don't have a big enough queue. */
@@ -303,10 +319,66 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t 
addr, uint32_t align,
 return 0;
 }
 
-static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
+static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
+bool is_legacy)
 {
 int ret;
 VqInfoBlock info;
+VqInfoBlockLegacy linfo;
+size_t info_len = is_legacy ? sizeof(linfo) : sizeof(info);
+
+if (check_len) {
+if (ccw.count != info_len) {
+return -EINVAL;
+}
+} else if (ccw.count  info_len) {
+/* Can't execute command. */
+return -EINVAL;
+}
+if (!ccw.cda) {
+return -EFAULT;
+}
+if (is_legacy) {
+linfo.queue = ldq_be_phys(address_space_memory, ccw.cda);
+linfo.align = ldl_be_phys(address_space_memory,
+  ccw.cda + sizeof(linfo.queue));
+linfo.index = lduw_be_phys(address_space_memory,
+   ccw.cda + sizeof(linfo.queue)
+   + sizeof(linfo.align));
+linfo.num = lduw_be_phys(address_space_memory,
+ ccw.cda + sizeof(linfo.queue)
+ + sizeof(linfo.align)
+ + sizeof(linfo.index));
+ret = virtio_ccw_set_vqs(sch, NULL, linfo);
+} else {
+info.desc = ldq_be_phys(address_space_memory, ccw.cda);
+info.index = lduw_be_phys(address_space_memory,
+  ccw.cda + sizeof(info.desc)
+  + sizeof(info.res0));
+info.num = lduw_be_phys(address_space_memory,
+ccw.cda + sizeof(info.desc)
+  + sizeof(info.res0)
+  + sizeof(info.index));
+info.avail = ldq_be_phys(address_space_memory,
+ ccw.cda + sizeof(info.desc)
+ + sizeof(info.res0)
+ + sizeof(info.index)
+ + sizeof(info.num));
+info.used = ldq_be_phys(address_space_memory,
+ccw.cda + sizeof(info.desc)
++ sizeof(info.res0)
++ sizeof(info.index)
++ sizeof(info.num)
++ sizeof(info.avail));
+ret = virtio_ccw_set_vqs(sch

[PATCH RFC 08/11] virtio_blk: use virtio v1.0 endian

[Cornelia Huck]

Note that we care only about the fields still in use for virtio v1.0.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/block/virtio_blk.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 0a58140..08a8012 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -119,6 +119,10 @@ static int __virtblk_add_req(struct virtqueue *vq,
sg_init_one(status, vbr-status, sizeof(vbr-status));
sgs[num_out + num_in++] = status;
 
+   /* we only care about fields valid for virtio-1 */
+   vbr-out_hdr.type = cpu_to_virtio_u32(vq-vdev, vbr-out_hdr.type);
+   vbr-out_hdr.sector = cpu_to_virtio_u64(vq-vdev, vbr-out_hdr.sector);
+
return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
 }
 
-- 
1.7.9.5

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

[PATCH RFC 09/11] KVM: s390: Set virtio-ccw transport revision

[Cornelia Huck]

From: Thomas Huth th...@linux.vnet.ibm.com

With the new SET-VIRTIO-REVISION command of the virtio 1.0 standard, we
can now negotiate the virtio-ccw revision after setting a channel online.

Note that we don't negotiate version 1 yet.

[Cornelia Huck: reworked revision loop a bit]
Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com
Signed-off-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/s390/kvm/virtio_ccw.c |   63 +
 1 file changed, 63 insertions(+)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 4173b59..cbe2ba8 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -55,6 +55,7 @@ struct virtio_ccw_device {
struct ccw_device *cdev;
__u32 curr_io;
int err;
+   unsigned int revision; /* Transport revision */
wait_queue_head_t wait_q;
spinlock_t lock;
struct list_head virtqueues;
@@ -86,6 +87,15 @@ struct virtio_thinint_area {
u8 isc;
 } __packed;
 
+struct virtio_rev_info {
+   __u16 revision;
+   __u16 length;
+   __u8 data[];
+};
+
+/* the highest virtio-ccw revision we support */
+#define VIRTIO_CCW_REV_MAX 0
+
 struct virtio_ccw_vq_info {
struct virtqueue *vq;
int num;
@@ -122,6 +132,7 @@ static struct airq_info *airq_areas[MAX_AIRQ_AREAS];
 #define CCW_CMD_WRITE_STATUS 0x31
 #define CCW_CMD_READ_VQ_CONF 0x32
 #define CCW_CMD_SET_IND_ADAPTER 0x73
+#define CCW_CMD_SET_VIRTIO_REV 0x83
 
 #define VIRTIO_CCW_DOING_SET_VQ 0x0001
 #define VIRTIO_CCW_DOING_RESET 0x0004
@@ -134,6 +145,7 @@ static struct airq_info *airq_areas[MAX_AIRQ_AREAS];
 #define VIRTIO_CCW_DOING_READ_VQ_CONF 0x0200
 #define VIRTIO_CCW_DOING_SET_CONF_IND 0x0400
 #define VIRTIO_CCW_DOING_SET_IND_ADAPTER 0x0800
+#define VIRTIO_CCW_DOING_SET_VIRTIO_REV 0x1000
 #define VIRTIO_CCW_INTPARM_MASK 0x
 
 static struct virtio_ccw_device *to_vc_device(struct virtio_device *vdev)
@@ -934,6 +946,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
case VIRTIO_CCW_DOING_RESET:
case VIRTIO_CCW_DOING_READ_VQ_CONF:
case VIRTIO_CCW_DOING_SET_IND_ADAPTER:
+   case VIRTIO_CCW_DOING_SET_VIRTIO_REV:
vcdev-curr_io = ~activity;
wake_up(vcdev-wait_q);
break;
@@ -1053,6 +1066,51 @@ static int virtio_ccw_offline(struct ccw_device *cdev)
return 0;
 }
 
+static int virtio_ccw_set_transport_rev(struct virtio_ccw_device *vcdev)
+{
+   struct virtio_rev_info *rev;
+   struct ccw1 *ccw;
+   int ret;
+
+   ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
+   if (!ccw)
+   return -ENOMEM;
+   rev = kzalloc(sizeof(*rev), GFP_DMA | GFP_KERNEL);
+   if (!rev) {
+   kfree(ccw);
+   return -ENOMEM;
+   }
+
+   /* Set transport revision */
+   ccw-cmd_code = CCW_CMD_SET_VIRTIO_REV;
+   ccw-flags = 0;
+   ccw-count = sizeof(*rev);
+   ccw-cda = (__u32)(unsigned long)rev;
+
+   vcdev-revision = VIRTIO_CCW_REV_MAX;
+   do {
+   rev-revision = vcdev-revision;
+   /* none of our supported revisions carry payload */
+   rev-length = 0;
+   ret = ccw_io_helper(vcdev, ccw,
+   VIRTIO_CCW_DOING_SET_VIRTIO_REV);
+   if (ret == -EOPNOTSUPP) {
+   if (vcdev-revision == 0)
+   /*
+* The host device does not support setting
+* the revision: let's operate it in legacy
+* mode.
+*/
+   ret = 0;
+   else
+   vcdev-revision--;
+   }
+   } while (ret == -EOPNOTSUPP);
+
+   kfree(ccw);
+   kfree(rev);
+   return ret;
+}
 
 static int virtio_ccw_online(struct ccw_device *cdev)
 {
@@ -1093,6 +1151,11 @@ static int virtio_ccw_online(struct ccw_device *cdev)
spin_unlock_irqrestore(get_ccwdev_lock(cdev), flags);
vcdev-vdev.id.vendor = cdev-id.cu_type;
vcdev-vdev.id.device = cdev-id.cu_model;
+
+   ret = virtio_ccw_set_transport_rev(vcdev);
+   if (ret)
+   goto out_free;
+
ret = register_virtio_device(vcdev-vdev);
if (ret) {
dev_warn(cdev-dev, Failed to register virtio device: %d\n,
-- 
1.7.9.5

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

[PATCH RFC 11/11] s390x/virtio-ccw: enable virtio 1.0

[Cornelia Huck]

virtio-ccw should now have everything in place to operate virtio 1.0
devices, so let's enable revision 1.

Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/virtio-ccw.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 03d5955..08edd8d 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -73,7 +73,7 @@ typedef struct VirtIOCCWDeviceClass {
 #define VIRTIO_CCW_FEATURE_SIZE NR_VIRTIO_FEATURE_WORDS
 
 /* The maximum virtio revision we support. */
-#define VIRTIO_CCW_REV_MAX 0
+#define VIRTIO_CCW_REV_MAX 1
 
 /* Performance improves when virtqueue kick processing is decoupled from the
  * vcpu thread using ioeventfd for some devices. */
-- 
1.7.9.5

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

[PATCH RFC 03/11] virtio: support more feature bits

[Cornelia Huck]

With virtio-1, we support more than 32 feature bits. Let's make
vdev-guest_features depend on the number of supported feature bits,
allowing us to grow the feature bits automatically.

We also need to enhance the internal functions dealing with getting
and setting features with an additional index field, so that all feature
bits may be accessed (in chunks of 32 bits).

vhost and migration have been ignored for now.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/9pfs/virtio-9p-device.c |7 ++-
 hw/block/virtio-blk.c  |9 +++--
 hw/char/virtio-serial-bus.c|9 +++--
 hw/net/virtio-net.c|   38 ++
 hw/s390x/s390-virtio-bus.c |9 +
 hw/s390x/virtio-ccw.c  |   17 ++---
 hw/scsi/vhost-scsi.c   |7 +--
 hw/scsi/virtio-scsi.c  |8 
 hw/virtio/dataplane/vring.c|   10 +-
 hw/virtio/virtio-balloon.c |8 ++--
 hw/virtio/virtio-bus.c |9 +
 hw/virtio/virtio-mmio.c|9 +
 hw/virtio/virtio-pci.c |   13 +++--
 hw/virtio/virtio-rng.c |2 +-
 hw/virtio/virtio.c |   29 +
 include/hw/virtio/virtio-bus.h |7 ---
 include/hw/virtio/virtio.h |   19 ++-
 17 files changed, 134 insertions(+), 76 deletions(-)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 2572747..c29c8c8 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -21,8 +21,13 @@
 #include virtio-9p-coth.h
 #include hw/virtio/virtio-access.h
 
-static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
+static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
+   uint32_t features)
 {
+if (index  0) {
+return features;
+}
+
 features |= 1  VIRTIO_9P_MOUNT_TAG;
 return features;
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 45e0c8f..5abc327 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -561,10 +561,15 @@ static void virtio_blk_set_config(VirtIODevice *vdev, 
const uint8_t *config)
 aio_context_release(bdrv_get_aio_context(s-bs));
 }
 
-static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
+static uint32_t virtio_blk_get_features(VirtIODevice *vdev, unsigned int index,
+uint32_t features)
 {
 VirtIOBlock *s = VIRTIO_BLK(vdev);
 
+if (index  0) {
+return features;
+}
+
 features |= (1  VIRTIO_BLK_F_SEG_MAX);
 features |= (1  VIRTIO_BLK_F_GEOMETRY);
 features |= (1  VIRTIO_BLK_F_TOPOLOGY);
@@ -597,7 +602,7 @@ static void virtio_blk_set_status(VirtIODevice *vdev, 
uint8_t status)
 return;
 }
 
-features = vdev-guest_features;
+features = vdev-guest_features[0];
 
 /* A guest that supports VIRTIO_BLK_F_CONFIG_WCE must be able to send
  * cache flushes.  Thus, the auto writethrough behavior is never
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3931085..0d843fe 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -75,7 +75,7 @@ static VirtIOSerialPort *find_port_by_name(char *name)
 static bool use_multiport(VirtIOSerial *vser)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(vser);
-return vdev-guest_features  (1  VIRTIO_CONSOLE_F_MULTIPORT);
+return vdev-guest_features[0]  (1  VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
 static size_t write_to_port(VirtIOSerialPort *port,
@@ -467,10 +467,15 @@ static void handle_input(VirtIODevice *vdev, VirtQueue 
*vq)
 {
 }
 
-static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
+static uint32_t get_features(VirtIODevice *vdev, unsigned int index,
+ uint32_t features)
 {
 VirtIOSerial *vser;
 
+if (index  0) {
+return features;
+}
+
 vser = VIRTIO_SERIAL(vdev);
 
 if (vser-bus.max_nr_ports  1) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 2040eac..67f91c0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -86,7 +86,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const 
uint8_t *config)
 
 memcpy(netcfg, config, n-config_size);
 
-if (!(vdev-guest_features  VIRTIO_NET_F_CTRL_MAC_ADDR  1) 
+if (!(vdev-guest_features[0]  VIRTIO_NET_F_CTRL_MAC_ADDR  1) 
 memcmp(netcfg.mac, n-mac, ETH_ALEN)) {
 memcpy(n-mac, netcfg.mac, ETH_ALEN);
 qemu_format_nic_info_str(qemu_get_queue(n-nic), n-mac);
@@ -305,7 +305,7 @@ static RxFilterInfo 
*virtio_net_query_rxfilter(NetClientState *nc)
 info-multicast_table = str_list;
 info-vlan_table = get_vlan_table(n);
 
-if (!((1  VIRTIO_NET_F_CTRL_VLAN)  vdev-guest_features)) {
+if (!((1  VIRTIO_NET_F_CTRL_VLAN)  vdev

[PATCH RFC 04/11] s390x/virtio-ccw: fix check for WRITE_FEAT

[Cornelia Huck]

We need to check guest feature size, not host feature size to find
out whether we should call virtio_set_features(). This check is
possible now that vdev-guest_features is an array.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/virtio-ccw.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 8aa79a7..69add47 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -399,7 +399,7 @@ 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)) {
+if (features.index  ARRAY_SIZE(vdev-guest_features)) {
 virtio_set_features(vdev, features.index, features.features);
 } else {
 /*
-- 
1.7.9.5

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

[PATCH RFC 08/11] s390x/css: Add a callback for when subchannel gets disabled

[Cornelia Huck]

From: Thomas Huth th...@linux.vnet.ibm.com

We need a possibility to run code when a subchannel gets disabled.
This patch adds the necessary infrastructure.

Signed-off-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/css.c |   12 
 hw/s390x/css.h |1 +
 2 files changed, 13 insertions(+)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index b67c039..735ec55 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -588,6 +588,7 @@ int css_do_msch(SubchDev *sch, SCHIB *orig_schib)
 {
 SCSW *s = sch-curr_status.scsw;
 PMCW *p = sch-curr_status.pmcw;
+uint16_t oldflags;
 int ret;
 SCHIB schib;
 
@@ -610,6 +611,7 @@ int css_do_msch(SubchDev *sch, SCHIB *orig_schib)
 copy_schib_from_guest(schib, orig_schib);
 /* Only update the program-modifiable fields. */
 p-intparm = schib.pmcw.intparm;
+oldflags = p-flags;
 p-flags = ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA |
   PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME |
   PMCW_FLAGS_MASK_MP);
@@ -625,6 +627,12 @@ int css_do_msch(SubchDev *sch, SCHIB *orig_schib)
 (PMCW_CHARS_MASK_MBFC | PMCW_CHARS_MASK_CSENSE);
 sch-curr_status.mba = schib.mba;
 
+/* Has the channel been disabled? */
+if (sch-disable_cb  (oldflags  PMCW_FLAGS_MASK_ENA) != 0
+ (p-flags  PMCW_FLAGS_MASK_ENA) == 0) {
+sch-disable_cb(sch);
+}
+
 ret = 0;
 
 out:
@@ -1443,6 +1451,10 @@ void css_reset_sch(SubchDev *sch)
 {
 PMCW *p = sch-curr_status.pmcw;
 
+if ((p-flags  PMCW_FLAGS_MASK_ENA) != 0  sch-disable_cb) {
+sch-disable_cb(sch);
+}
+
 p-intparm = 0;
 p-flags = ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA |
   PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME |
diff --git a/hw/s390x/css.h b/hw/s390x/css.h
index 33104ac..7fa807b 100644
--- a/hw/s390x/css.h
+++ b/hw/s390x/css.h
@@ -81,6 +81,7 @@ struct SubchDev {
 uint8_t ccw_no_data_cnt;
 /* transport-provided data: */
 int (*ccw_cb) (SubchDev *, CCW1);
+void (*disable_cb)(SubchDev *);
 SenseId id;
 void *driver_data;
 };
-- 
1.7.9.5

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

Re: [PATCH RFC 00/11] qemu: towards virtio-1 host support

[Cornelia Huck]

On Tue, 07 Oct 2014 18:24:22 -0700
Andy Lutomirski l...@amacapital.net wrote:

 On 10/07/2014 07:39 AM, Cornelia Huck wrote:
  This patchset aims to get us some way to implement virtio-1 compliant
  and transitional devices in qemu. Branch available at
  
  git://github.com/cohuck/qemu virtio-1
  
  I've mainly focused on:
  - endianness handling
  - extended feature bits
  - virtio-ccw new/changed commands
 
 At the risk of some distraction, would it be worth thinking about a
 solution to the IOMMU bypassing mess as part of this?

I think that is a whole different issue. virtio-1 is basically done - we
just need to implement it - while the IOMMU/DMA stuff certainly needs
more discussion. Therefore, I'd like to defer to the other discussion
thread here.

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

Re: [PATCH RFC 08/11] virtio_blk: use virtio v1.0 endian

[Cornelia Huck]

On Mon, 13 Oct 2014 16:28:32 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 Cornelia Huck cornelia.h...@de.ibm.com writes:
  Note that we care only about the fields still in use for virtio v1.0.
 
  Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
  Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com
  Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 
 Hi Cornelia,
 
 These patches all look good; 

Cool, thanks.

 I'm a bit nervous about our testing
 missing some conversion, so we'll need qemu patches for PCI so we can
 test on other platforms too.

The devices I looked at (blk and net) are probably OK as ccw needs to
convert always. I agree that we want to test on other platforms as
well, but unfortunately I not familiar enough with other platforms to
feel confident enough to convert virtio-pci and test it :(

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

Re: [PATCH RFC 03/11] virtio: support more feature bits

[Cornelia Huck]

On Mon, 13 Oct 2014 16:23:58 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 Cornelia Huck cornelia.h...@de.ibm.com writes:
  With virtio-1, we support more than 32 feature bits. Let's make
  vdev-guest_features depend on the number of supported feature bits,
  allowing us to grow the feature bits automatically.
 
 It's a judgement call, but I would say that simply using uint64_t
 will be sufficient for quite a while.

I prefer this as an array for two reasons:

- It matches what ccw does anyway (we read/write features in chunks of
  32 bit), so this makes the backend handling for this transport very
  straightforward.
- While I don't see us running out of 64 feature bits for a while, we'd
  have to touch every device and transport again when we do. I'd prefer
  to do this once, as we need to change code anyway.

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

Re: [PATCH] virtio_ccw: remove unsued variable

[Cornelia Huck]

On Mon, 20 Oct 2014 12:48:18 +0200 (CEST)
Sebastian Ott seb...@linux.vnet.ibm.com wrote:

 Hi,
 
 016c98c6f virtio: unify config_changed handling
 
 introduced a warning in virtio_ccw which is fixed by the following patch.
 
 Regards,
 Sebastian
 ---
 virtio_ccw: remove unsued variable
 
 Fix this warning:
 drivers/s390/kvm/virtio_ccw.c: In function ‘virtio_ccw_int_handler’:
 drivers/s390/kvm/virtio_ccw.c:891:24: warning: unused variable ‘drv’ 
 [-Wunused-variable]
 struct virtio_driver *drv;
 
 Signed-off-by: Sebastian Ott seb...@linux.vnet.ibm.com
 ---
  drivers/s390/kvm/virtio_ccw.c | 1 -
  1 file changed, 1 deletion(-)

Acked-by: Cornelia Huck cornelia.h...@de.ibm.com

Rusty: Will you take this through the virtio tree, or should I push
this through the kvm/s390 tree?

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

Re: [PATCH v4 13/25] virtio_console: enable VQs early

[Cornelia Huck]

On Mon, 20 Oct 2014 13:07:50 +0100
Thomas Graf tg...@suug.ch wrote:

 On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:
  virtio spec requires drivers to set DRIVER_OK before using VQs.
  This is set automatically after probe returns, virtio console violated this
  rule by adding inbufs, which causes the VQ to be used directly within
  probe.
  
  To fix, call virtio_device_ready before using VQs.
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  ---
   drivers/char/virtio_console.c | 2 ++
   1 file changed, 2 insertions(+)
  
  diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
  index b585b47..6ebe8f6 100644
  --- a/drivers/char/virtio_console.c
  +++ b/drivers/char/virtio_console.c
  @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 
  id)
  spin_lock_init(port-outvq_lock);
  init_waitqueue_head(port-waitqueue);
   
  +   virtio_device_ready(portdev-vdev);
  +
  /* Fill the in_vq with buffers so the host can send us data. */
  nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
  if (!nr_added_bufs) {
 
 Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK

I think we need to set this in the probe function instead, otherwise we
fail for multiqueue (which also wants to use the control queue early).

Completely untested patch below; I can send this with proper s-o-b if
it helps.

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bfa6400..cf7a561 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,8 +1449,6 @@ static int add_port(struct ports_device *portdev, u32 id)
spin_lock_init(port-outvq_lock);
init_waitqueue_head(port-waitqueue);
 
-   virtio_device_ready(portdev-vdev);
-
/* Fill the in_vq with buffers so the host can send us data. */
nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
if (!nr_added_bufs) {
@@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
spin_lock_init(portdev-ports_lock);
INIT_LIST_HEAD(portdev-ports);
 
+   virtio_device_ready(portdev-vdev);
+
if (multiport) {
unsigned int nr_added_bufs;
 

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

[PATCH] virtio_console: move early VQ enablement

[Cornelia Huck]

Commit f5866db6 (virtio_console: enable VQs early) tried to make
sure that DRIVER_OK was set when virtio_console started using its
virtqueues. Doing this in add_port(), however, means that we try
to set DRIVER_OK again when when a port is dynamically added after
the probe function is done.

Let's move virtio_device_ready() to the probe function just before
trying to use the virtqueues instead. This is fine as nothing can
fail inbetween.

Reported-by: Thomas Graf tg...@suug.ch
Reviewed-by: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/char/virtio_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bfa6400..cf7a561 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,8 +1449,6 @@ static int add_port(struct ports_device *portdev, u32 id)
spin_lock_init(port-outvq_lock);
init_waitqueue_head(port-waitqueue);
 
-   virtio_device_ready(portdev-vdev);
-
/* Fill the in_vq with buffers so the host can send us data. */
nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
if (!nr_added_bufs) {
@@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
spin_lock_init(portdev-ports_lock);
INIT_LIST_HEAD(portdev-ports);
 
+   virtio_device_ready(portdev-vdev);
+
if (multiport) {
unsigned int nr_added_bufs;
 
-- 
1.8.5.5

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

Re: [PATCH RFC] virtio 1.0 vring endian-ness

[Cornelia Huck]

On Wed, 22 Oct 2014 01:09:40 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 This adds wrappers to switch between native endian-ness
 (virtio 0.9) and virtio endian-ness (virtio 1.0).
 Add new typedefs as well, so that we can check
 statically that we didn't miss any accesses.
 All callers simply pass in false (0.9) so no
 functional change for now.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 
 ---
 
 Sending this early so I can get feedback on this style.

Hm...

http://marc.info/?l=linux-virtualizationm=141270444612625w=2

(and other in that series. Forgot to cc: you on those patches...)

 Rusty, what's your opinion? Reasonable?
 
 diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
 index 67e06fe..32211aa 100644
 --- a/include/linux/virtio_ring.h
 +++ b/include/linux/virtio_ring.h
 @@ -62,6 +62,26 @@ static inline void virtio_wmb(bool weak_barriers)
  }
  #endif
 
 +#define DEFINE_VIRTIO_XX_TO_CPU(bits) \
 +static inline u##bits virtio##bits##_to_cpu(bool little_endian, 
 __virtio##bits val) \
 +{ \
 + if (little_endian) \
 + return le##bits##_to_cpu((__force __le##bits)val); \
 + else \
 + return (__force u##bits)val; \
 +} \
 +static inline __virtio##bits cpu_to_virtio##bits(bool little_endian, u##bits 
 val) \
 +{ \
 + if (little_endian) \
 + return (__force __virtio##bits)cpu_to_le##bits(val); \
 + else \
 + return val; \
 +}
 +
 +DEFINE_VIRTIO_XX_TO_CPU(16)
 +DEFINE_VIRTIO_XX_TO_CPU(32)
 +DEFINE_VIRTIO_XX_TO_CPU(64)
 +

I'm usually not very fond of creating functions via macros like that as
it makes it hard to grep for them.

  struct virtio_device;
  struct virtqueue;
 
 diff --git a/include/uapi/linux/virtio_ring.h 
 b/include/uapi/linux/virtio_ring.h
 index a99f9b7..744cee1 100644
 --- a/include/uapi/linux/virtio_ring.h
 +++ b/include/uapi/linux/virtio_ring.h
 @@ -33,6 +33,10 @@
   * Copyright Rusty Russell IBM Corporation 2007. */
  #include linux/types.h
 
 +typedef __u16 __bitwise __virtio16;
 +typedef __u32 __bitwise __virtio32;
 +typedef __u64 __bitwise __virtio64;
 +
  /* This marks a buffer as continuing via the next field. */
  #define VRING_DESC_F_NEXT1
  /* This marks a buffer as write-only (otherwise read-only). */
 @@ -61,32 +65,32 @@
  /* Virtio ring descriptors: 16 bytes.  These can chain together via next. 
 */
  struct vring_desc {
   /* Address (guest-physical). */
 - __u64 addr;
 + __virtio64 addr;
   /* Length. */
 - __u32 len;
 + __virtio32 len;
   /* The flags as indicated above. */
 - __u16 flags;
 + __virtio16 flags;
   /* We chain unused descriptors via this, too */
 - __u16 next;
 + __virtio16 next;
  };
 
  struct vring_avail {
 - __u16 flags;
 - __u16 idx;
 - __u16 ring[];
 + __virtio16 flags;
 + __virtio16 idx;
 + __virtio16 ring[];
  };

This looks weird. At least to me, that would scream virtio endian,
which is only true for legacy devices.

I understand that you want it to be statically checkable, but I think
it decreases readability.

 
  /* u32 is used here for ids for padding reasons. */
  struct vring_used_elem {
   /* Index of start of used descriptor chain. */
 - __u32 id;
 + __virtio32 id;
   /* Total length of the descriptor chain which was used (written to) */
 - __u32 len;
 + __virtio32 len;
  };
 
  struct vring_used {
 - __u16 flags;
 - __u16 idx;
 + __virtio16 flags;
 + __virtio16 idx;
   struct vring_used_elem ring[];
  };
 

 diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
 index 61a1fe1..a2f2f22 100644
 --- a/drivers/virtio/virtio_ring.c
 +++ b/drivers/virtio/virtio_ring.c
 @@ -98,6 +98,8 @@ struct vring_virtqueue
  };
 
  #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 +/* Will become vq-little_endian once we support virtio 1.0 */
 +#define vq_le(vq) (false)

All virtqueues inherit this property from their device, right? Do you
want to propagate this to the virtqueues if the guest negotiated
virtio-1 for the device?

 
  static struct vring_desc *alloc_indirect(unsigned int total_sg, gfp_t gfp)
  {

 @@ -235,13 +237,13 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 
   /* Put entry in available array (but don't update avail-idx until they
* do sync). */
 - avail = (vq-vring.avail-idx  (vq-vring.num-1));
 - vq-vring.avail-ring[avail] = head;
 + avail = virtio16_to_cpu(vq_le(_vq), vq-vring.avail-idx)  
 (vq-vring.num - 1);
 + vq-vring.avail-ring[avail] = cpu_to_virtio16(vq_le(_vq), head);
 
   /* Descriptors and available array need to be set before we expose the
* new available array entries. */
   virtio_wmb(vq-weak_barriers);
 - vq-vring.avail-idx++;
 + vq-vring.avail-idx = cpu_to_virtio16(vq_le(_vq), 
 virtio16_to_cpu(vq_le(_vq), vq-vring.avail-idx) + 1);

I think this line looks awful :(

I also notice that you need to call the 

Re: [PATCH RFC 04/11] virtio_ring: implement endian reversal based on VERSION_1 feature.

[Cornelia Huck]

On Wed, 22 Oct 2014 17:02:26 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Tue, Oct 07, 2014 at 04:39:45PM +0200, Cornelia Huck wrote:
  From: Rusty Russell ru...@rustcorp.com.au
  
  [Cornelia Huck: we don't need the vq-vring.num - vq-ring_mask change]
  Signed-off-by: Rusty Russell ru...@rustcorp.com.au
  Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
  ---
   drivers/virtio/virtio_ring.c |  195 
  ++
   1 file changed, 138 insertions(+), 57 deletions(-)
  
  diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
  index 1cfb5ba..350c39b 100644
  --- a/drivers/virtio/virtio_ring.c
  +++ b/drivers/virtio/virtio_ring.c
  @@ -145,42 +145,54 @@ static inline int vring_add_indirect(struct 
  vring_virtqueue *vq,
  i = 0;
  for (n = 0; n  out_sgs; n++) {
  for (sg = sgs[n]; sg; sg = next(sg, total_out)) {
  -   desc[i].flags = VRING_DESC_F_NEXT;
  -   desc[i].addr = sg_phys(sg);
  -   desc[i].len = sg-length;
  -   desc[i].next = i+1;
  +   desc[i].flags = cpu_to_virtio16(vq-vq.vdev,
  + VRING_DESC_F_NEXT);
  +   desc[i].addr = cpu_to_virtio64(vq-vq.vdev,
  +sg_phys(sg));
  +   desc[i].len = cpu_to_virtio32(vq-vq.vdev,
  +   sg-length);
  +   desc[i].next = cpu_to_virtio16(vq-vq.vdev,
  +i+1);
  i++;
  }
  }
  for (; n  (out_sgs + in_sgs); n++) {
  for (sg = sgs[n]; sg; sg = next(sg, total_in)) {
  -   desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
  -   desc[i].addr = sg_phys(sg);
  -   desc[i].len = sg-length;
  -   desc[i].next = i+1;
  +   desc[i].flags = cpu_to_virtio16(vq-vq.vdev,
  + VRING_DESC_F_NEXT|
  + VRING_DESC_F_WRITE);
  +   desc[i].addr = cpu_to_virtio64(vq-vq.vdev,
  +sg_phys(sg));
  +   desc[i].len = cpu_to_virtio32(vq-vq.vdev,
  +   sg-length);
  +   desc[i].next = cpu_to_virtio16(vq-vq.vdev, i+1);
  i++;
  }
  }
  -   BUG_ON(i != total_sg);
   
  /* Last one doesn't continue. */
  -   desc[i-1].flags = ~VRING_DESC_F_NEXT;
  +   desc[i-1].flags = ~cpu_to_virtio16(vq-vq.vdev, VRING_DESC_F_NEXT);
  desc[i-1].next = 0;
   
  -   /* We're about to use a buffer */
  -   vq-vq.num_free--;
  -
  /* Use a single buffer which doesn't continue */
  head = vq-free_head;
  -   vq-vring.desc[head].flags = VRING_DESC_F_INDIRECT;
  -   vq-vring.desc[head].addr = virt_to_phys(desc);
  +   vq-vring.desc[head].flags =
  +   cpu_to_virtio16(vq-vq.vdev, VRING_DESC_F_INDIRECT);
  +   vq-vring.desc[head].addr =
  +   cpu_to_virtio64(vq-vq.vdev, virt_to_phys(desc));
  /* kmemleak gives a false positive, as it's hidden by virt_to_phys */
  kmemleak_ignore(desc);
  -   vq-vring.desc[head].len = i * sizeof(struct vring_desc);
  +   vq-vring.desc[head].len =
  +   cpu_to_virtio32(vq-vq.vdev, i * sizeof(struct vring_desc));
   
  -   /* Update free pointer */
  +   BUG_ON(i != total_sg);
  +
 
 Why move the BUG_ON here?
 I think I'll move it back ...

IIRC this was in the original patch I applied - but you're right, the
statement can stay in the old place.

 
  +   /* Update free pointer (we store this in native endian) */
  vq-free_head = vq-vring.desc[head].next;
   
  +   /* We've just used a buffer */
  +   vq-vq.num_free--;
  +
  return head;
   }
   

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

Re: [PATCH RFC v3 01/16] virtio: memory access APIs

[Cornelia Huck]

On Wed, 22 Oct 2014 21:44:08 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 virtio 1.0 makes all memory structures LE, so
 we need APIs to conditionally do a byteswap on BE
 architectures.
 
 To make it easier to check code statically,
 add virtio specific types for multi-byte integers
 in memory.
 
 Add low level wrappers that do a byteswap conditionally, these will be
 useful e.g. for vhost.  Add high level wrappers that will (in the
 future) query device endian-ness and act accordingly.
 
 At the moment, stub them out and assume native endian-ness everywhere.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio_byteorder.h | 29 
  include/linux/virtio_config.h| 16 +
  include/uapi/linux/virtio_ring.h | 49 
 
  include/uapi/linux/Kbuild|  1 +
  4 files changed, 71 insertions(+), 24 deletions(-)
  create mode 100644 include/linux/virtio_byteorder.h
 
 diff --git a/include/linux/virtio_byteorder.h 
 b/include/linux/virtio_byteorder.h
 new file mode 100644
 index 000..7afdd8a
 --- /dev/null
 +++ b/include/linux/virtio_byteorder.h
 @@ -0,0 +1,29 @@
 +#ifndef _LINUX_VIRTIO_BYTEORDER_H
 +#define _LINUX_VIRTIO_BYTEORDER_H
 +#include linux/types.h
 +#include uapi/linux/virtio_types.h
 +
 +/* Memory accessors for handling virtio in modern little endian and in
 + * compatibility big endian format. */

s/big/native/

 +
 +#define __DEFINE_VIRTIO_XX_TO_CPU(bits) \
 +static inline u##bits __virtio##bits##_to_cpu(bool little_endian, 
 __virtio##bits val) \
 +{ \
 + if (little_endian) \
 + return le##bits##_to_cpu((__force __le##bits)val); \
 + else \
 + return (__force u##bits)val; \
 +} \
 +static inline __virtio##bits __cpu_to_virtio##bits(bool little_endian, 
 u##bits val) \
 +{ \
 + if (little_endian) \
 + return (__force __virtio##bits)cpu_to_le##bits(val); \
 + else \
 + return val; \
 +}
 +
 +__DEFINE_VIRTIO_XX_TO_CPU(16)
 +__DEFINE_VIRTIO_XX_TO_CPU(32)
 +__DEFINE_VIRTIO_XX_TO_CPU(64)

...although I'm still not too happy with macro-generated helpers.

 +
 +#endif /* _LINUX_VIRTIO_BYTEORDER */

 diff --git a/include/uapi/linux/virtio_ring.h 
 b/include/uapi/linux/virtio_ring.h
 index a99f9b7..6c00632 100644
 --- a/include/uapi/linux/virtio_ring.h
 +++ b/include/uapi/linux/virtio_ring.h

 @@ -61,32 +62,32 @@
  /* Virtio ring descriptors: 16 bytes.  These can chain together via next. 
 */
  struct vring_desc {
   /* Address (guest-physical). */
 - __u64 addr;
 + __virtio64 addr;
   /* Length. */
 - __u32 len;
 + __virtio32 len;
   /* The flags as indicated above. */
 - __u16 flags;
 + __virtio16 flags;
   /* We chain unused descriptors via this, too */
 - __u16 next;
 + __virtio16 next;
  };

I think all of these __virtio types need an explanation somewhere as to
what they mean, e.g.:

/*
 * __virtio{16,32,64} have the following meaning:
 * - __u{16,32,64} for virtio devices in legacy mode,
 *   accessed in native endian
 * - __le{16,32,64} for standard-compliant virtio devices
 */

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

Re: [PATCH RFC v3 05/16] virtio: add virtio 1.0 feature bit

[Cornelia Huck]

On Wed, 22 Oct 2014 21:44:31 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 Based on original patches by Rusty Russell, Thomas Huth
 and Cornelia Huck.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/uapi/linux/virtio_config.h | 7 +--
  drivers/virtio/virtio_ring.c   | 2 ++
  2 files changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

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

Re: [PATCH RFC v3 06/16] virtio: make endian-ness depend on virtio 1.0

[Cornelia Huck]

On Wed, 22 Oct 2014 21:44:34 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 virtio 1.0 is LE, virtio without 1.0 is native endian.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio_config.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 

Looks sane.

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

Re: [PATCH RFC v3 09/16] virtio: set FEATURES_OK

[Cornelia Huck]

On Wed, 22 Oct 2014 21:44:44 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 set FEATURES_OK as per virtio 1.0 spec
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/uapi/linux/virtio_config.h |  2 ++
  drivers/virtio/virtio.c| 29 ++---
  2 files changed, 24 insertions(+), 7 deletions(-)
 

   dev-config-finalize_features(dev);
 
 + if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
 + add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
 + status = dev-config-get_status(dev);
 + if (!(status  VIRTIO_CONFIG_S_FEATURES_OK)) {
 + printk(KERN_ERR virtio: device refuses features: %x\n,
 +status);
 + err = -ENODEV;
 + goto err;
 + }
 + }
 +

Ugh, I just realize that virtio-ccw has a problem with that mechanism :(

Up to now, the driver only propagated status to the device: For
virtio-ccw, this was easily implemented via a ccw that transmitted
status to the device. However, the read back status part now
actually requires that the driver can get status from the device, or
has a comparable way to find out that the device won't accept the
status it tried to write.

I can think of two solutions:

(1) Introduce a new ccw that actually reads the device status.
(2) Make the WRITE_STATUS ccw fail (with a unit check) if the driver
sets FEATURES_OK after it tried to set features the device won't
accept.

(1) is probably more generic, while (2) is more straightforward to
implement.

Good thing we actually try to finally implement this, I did not notice
this problem during the review :(

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

Re: [PATCH RFC v3 09/16] virtio: set FEATURES_OK

[Cornelia Huck]

On Thu, 23 Oct 2014 15:51:39 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Oct 23, 2014 at 02:28:08PM +0200, Cornelia Huck wrote:
  On Wed, 22 Oct 2014 21:44:44 +0300
  Michael S. Tsirkin m...@redhat.com wrote:
  
   set FEATURES_OK as per virtio 1.0 spec
   
   Signed-off-by: Michael S. Tsirkin m...@redhat.com
   ---
include/uapi/linux/virtio_config.h |  2 ++
drivers/virtio/virtio.c| 29 ++---
2 files changed, 24 insertions(+), 7 deletions(-)
   
  
 dev-config-finalize_features(dev);
   
   + if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
   + add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
   + status = dev-config-get_status(dev);
   + if (!(status  VIRTIO_CONFIG_S_FEATURES_OK)) {
   + printk(KERN_ERR virtio: device refuses features: %x\n,
   +status);
   + err = -ENODEV;
   + goto err;
   + }
   + }
   +
  
  Ugh, I just realize that virtio-ccw has a problem with that mechanism :(
  
  Up to now, the driver only propagated status to the device: For
  virtio-ccw, this was easily implemented via a ccw that transmitted
  status to the device. However, the read back status part now
  actually requires that the driver can get status from the device, or
  has a comparable way to find out that the device won't accept the
  status it tried to write.
 
 Ugh, it actually caches the status in the transport :(

Well, it worked as long as this was unidirectional...

 
 
  I can think of two solutions:
  
  (1) Introduce a new ccw that actually reads the device status.
  (2) Make the WRITE_STATUS ccw fail (with a unit check) if the driver
  sets FEATURES_OK after it tried to set features the device won't
  accept.
  
  (1) is probably more generic, while (2) is more straightforward to
  implement.
  
  Good thing we actually try to finally implement this,
 
  I did not notice
  this problem during the review :(
 
 Well, it's a nuisance, but the spec is out.
 It seems to me a new command would be a substantive change so we can't
 do this in errata.
 
 Option (2) would require two statements for drivers and devices,
 but since it's clearly the case for correct drivers/devices
 that command does not fail, it follows that this
 is not a substantive change so it can be fixed
 in an errata.

It only adds a new failure case, so it's not really magic, agreed.

 
 So the new command would have to be optional, 

I think a new command should be tied to a new virtio-ccw revision.

 please open
 two issues in the TC: one documenting that driver must check
 WRITE_STATUS and device can fail WRITE_STATUS, and another
 for adding READ_STATUS (which will have to wait until
 the next CS).

I think I need to contemplate that a bit more. The problem with a new
READ_STATUS is that it is, by nature, an asynchronous command (as all
ccw commands are - the Linux guest driver uses a simple wrapper that
makes it appear synchronous). This means, for example, that every
status update now involves two channel programs instead of one, and
that reading e.g. the status sysfs attribute in Linux now triggers
channel I/O, which means several exits (at least ssch, interrupt
injection, tsch; probably more depending on what the guest does)...

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

Re: [PATCH RFC 00/11] qemu: towards virtio-1 host support

[Cornelia Huck]

On Fri, 24 Oct 2014 00:42:20 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
  This patchset aims to get us some way to implement virtio-1 compliant
  and transitional devices in qemu. Branch available at
  
  git://github.com/cohuck/qemu virtio-1
  
  I've mainly focused on:
  - endianness handling
  - extended feature bits
  - virtio-ccw new/changed commands
 
 So issues identified so far:

Thanks for taking a look.

 - devices not converted yet should not advertize 1.0

Neither should an uncoverted transport. So we either can
- have transport set the bit and rely on devices -get_features
  callback to mask it out
  (virtio-ccw has to change the calling order for get_features, btw.)
- have device set the bit and the transport mask it out later. Feels a
  bit weird, as virtio-1 is a transport feature bit.

I'm tending towards the first option; smth like this (on top of my
branch):

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index c29c8c8..f6501ea 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -24,6 +24,9 @@
 static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
uint32_t features)
 {
+if (index == 1) {
+features = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 if (index  0) {
 return features;
 }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 0d843fe..07a7a6f 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -472,6 +472,9 @@ static uint32_t get_features(VirtIODevice *vdev, unsigned 
int index,
 {
 VirtIOSerial *vser;
 
+if (index == 1) {
+features = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 if (index  0) {
 return features;
 }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 67f91c0..4b75105 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -447,6 +447,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, 
unsigned int index,
 VirtIONet *n = VIRTIO_NET(vdev);
 NetClientState *nc = qemu_get_queue(n-nic);
 
+if (index == 1  get_vhost_net(nc-peer)) {
+features = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 if (index  0) {
 return features;
 }
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 80efe88..07fbf40 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -839,16 +839,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, 
VirtIODevice *vdev)
 dev-revision = -1;
 
 /* Set default feature bits that are offered by the host. */
+dev-host_features[0] = 0x1  VIRTIO_F_NOTIFY_ON_EMPTY;
+dev-host_features[0] |= 0x1  VIRTIO_F_BAD_FEATURE;
+
+dev-host_features[1] = 1  (VIRTIO_F_VERSION_1 - 32);
+
 for (i = 0; i  ARRAY_SIZE(dev-host_features); i++) {
 dev-host_features[i] =
 virtio_bus_get_vdev_features(dev-bus, i, dev-host_features[i]);
 }
 
-dev-host_features[0] |= 0x1  VIRTIO_F_NOTIFY_ON_EMPTY;
-dev-host_features[0] |= 0x1  VIRTIO_F_BAD_FEATURE;
-
-dev-host_features[1] = 1  (VIRTIO_F_VERSION_1 - 32);
-
 css_generate_sch_crws(sch-cssid, sch-ssid, sch-schid,
   parent-hotplugged, 1);
 return 0;
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 8e1afa0..8a8fdb9 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -155,6 +155,9 @@ static uint32_t vhost_scsi_get_features(VirtIODevice *vdev, 
unsigned int index,
 {
 VHostSCSI *s = VHOST_SCSI(vdev);
 
+if (index == 1) {
+features = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 if (index  0) {
 return features;
 }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 088e688..378783f 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -611,6 +611,9 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, unsigned int 
index,
  uint32_t requested_features)
 {
+if (index == 1) {
+requested_features = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 return requested_features;
 }
 
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5af17e2..bd52845 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -306,6 +306,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev,
 unsigned int index, uint32_t f)
 {
+if (index == 1) {
+f = ~(1  (VIRTIO_F_VERSION_1 - 32));
+}
 if (index  0) {
 return f;
 }
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 2814017..bf6101d 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -101,6 +101,9 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq

Re: [PATCH RFC v4 07/17] virtio_config: endian conversion for v1.0

[Cornelia Huck]

On Thu, 23 Oct 2014 19:24:30 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 We (ab)use virtio conversion functions for device-specific
 config space accesses.
 
 Reviewed-by: David Hildenbrand d...@linux.vnet.ibm.com
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio_config.h | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

Not objecting to the patch, but something seems to have gotten messed
up wrt authorship? You only adapted it, right?

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

Re: [PATCH RFC 00/11] qemu: towards virtio-1 host support

[Cornelia Huck]

On Fri, 24 Oct 2014 10:38:39 +0200
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 On Fri, 24 Oct 2014 00:42:20 +0300
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
   This patchset aims to get us some way to implement virtio-1 compliant
   and transitional devices in qemu. Branch available at
   
   git://github.com/cohuck/qemu virtio-1
   
   I've mainly focused on:
   - endianness handling
   - extended feature bits
   - virtio-ccw new/changed commands
  
  So issues identified so far:
 
 Thanks for taking a look.
 
  - devices not converted yet should not advertize 1.0
 
 Neither should an uncoverted transport. So we either can
 - have transport set the bit and rely on devices -get_features
   callback to mask it out
   (virtio-ccw has to change the calling order for get_features, btw.)
 - have device set the bit and the transport mask it out later. Feels a
   bit weird, as virtio-1 is a transport feature bit.
 
 I'm tending towards the first option; smth like this (on top of my
 branch):

(...)

OK, I played around with this patch on top and the vhost-next branch as
guest. It seems to work reasonably well so far: a virtio-blk device
used virtio-1, a virtio-balloon device legacy.

One thing I noticed, though, is that I may need to think about
virtio-ccw revision vs. virtio version again. As a device can refuse
virtio-1 after the driver negotiated revision 1, we're operating a
legacy device with (some) standard ccws. Probably not a big deal, as
(a) both driver and device already have indicated that they support
revision 1 which those ccws are tied to and (b) some legacy
devices/drivers already support standard ccws (adapter interrupt
support). I might want to clarify the standard a bit, let me think
about that over the weekend.

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

Re: [Qemu-devel] [PATCH RFC 07/11] dataplane: allow virtio-1 devices

[Cornelia Huck]

On Tue, 28 Oct 2014 16:22:54 +0100
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 On Tue,  7 Oct 2014 16:40:03 +0200
 Cornelia Huck cornelia.h...@de.ibm.com wrote:
 
  Handle endianness conversion for virtio-1 virtqueues correctly.
  
  Note that dataplane now needs to be built per-target.
  
 
 It also affects hw/virtio/virtio-pci.c:
 
 In file included from include/hw/virtio/dataplane/vring.h:23:0,
  from include/hw/virtio/virtio-scsi.h:21,
  from hw/virtio/virtio-pci.c:24:
 include/hw/virtio/virtio-access.h: In function ‘virtio_access_is_big_endian’:
 include/hw/virtio/virtio-access.h:28:15: error: attempt to use poisoned 
 TARGET_WORDS_BIGENDIAN
  #elif defined(TARGET_WORDS_BIGENDIAN)
^
 
 FWIW when I added endian ambivalent support to virtio, I remember *some 
 people*
 getting angry at the idea of turning common code into per-target... :)

Well, it probably can't be helped for something that is
endian-sensitive like virtio :( (Although we should try to keep it as
local as possible.)

 
 See comment below.
 
  Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
  ---
   hw/block/dataplane/virtio-blk.c |3 +-
   hw/scsi/virtio-scsi-dataplane.c |2 +-
   hw/virtio/Makefile.objs |2 +-
   hw/virtio/dataplane/Makefile.objs   |2 +-
   hw/virtio/dataplane/vring.c |   85 
  +++
   include/hw/virtio/dataplane/vring.h |   64 --
   6 files changed, 113 insertions(+), 45 deletions(-)
  

  diff --git a/include/hw/virtio/dataplane/vring.h 
  b/include/hw/virtio/dataplane/vring.h
  index d3e086a..fde15f3 100644
  --- a/ 
  +++ b/include/hw/virtio/dataplane/vring.h
  @@ -20,6 +20,7 @@
   #include qemu-common.h
   #include hw/virtio/virtio_ring.h
   #include hw/virtio/virtio.h
  +#include hw/virtio/virtio-access.h
  
 
 Since the following commit:
 
 commit 244e2898b7a7735b3da114c120abe206af56a167
 Author: Fam Zheng f...@redhat.com
 Date:   Wed Sep 24 15:21:41 2014 +0800
 
 virtio-scsi: Add VirtIOSCSIVring in VirtIOSCSIReq
 
 The include/hw/virtio/dataplane/vring.h header is indirectly included
 by hw/virtio/virtio-pci.c. Why don't you move all this target dependent
 helpers to another header ?

Ah, this seems to have come in after I hacked on that code - I'll take a
look at splitting off the accessors.

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

Re: [PATCH RFC 00/11] qemu: towards virtio-1 host support

[Cornelia Huck]

On Tue, 28 Oct 2014 06:43:29 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Fri, Oct 24, 2014 at 10:38:39AM +0200, Cornelia Huck wrote:
  On Fri, 24 Oct 2014 00:42:20 +0300
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
This patchset aims to get us some way to implement virtio-1 compliant
and transitional devices in qemu. Branch available at

git://github.com/cohuck/qemu virtio-1

I've mainly focused on:
- endianness handling
- extended feature bits
- virtio-ccw new/changed commands
   
   So issues identified so far:
  
  Thanks for taking a look.
  
   - devices not converted yet should not advertize 1.0
  
  Neither should an uncoverted transport. So we either can
  - have transport set the bit and rely on devices -get_features
callback to mask it out
(virtio-ccw has to change the calling order for get_features, btw.)
  - have device set the bit and the transport mask it out later. Feels a
bit weird, as virtio-1 is a transport feature bit.
 
 
 I thought more about it, I think the right thing
 would be for unconverted transports to clear
 high bits on ack and get features.

This should work out of the box with my patches (virtio-pci and
virtio-mmio return 0 for high feature bits).

 
 So bit 32 is set, but not exposed to guests.
 In fact at least for PCI, we have a 32 bit field for
 features in 0.9 so it's automatic.
 Didn't check mmio yet.

We still to make sure the bit is not set for unconverted devices,
though. But you're probably right that having the device set the bit is
less error-prone.

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

Re: [PATCH] virtio_ccw: remove unsued variable

[Cornelia Huck]

On Tue, 28 Oct 2014 19:37:58 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Tue, Oct 28, 2014 at 04:39:12PM +0100, Sebastian Ott wrote:

  virtio_ccw: remove unused variable
  
  Fix this warning:
  drivers/s390/kvm/virtio_ccw.c: In function ‘virtio_ccw_int_handler’:
  drivers/s390/kvm/virtio_ccw.c:891:24: warning: unused variable ‘drv’ 
  [-Wunused-variable]
  struct virtio_driver *drv;
  
  Signed-off-by: Sebastian Ott seb...@linux.vnet.ibm.com
  Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
 
 Acked-by: Michael S. Tsirkin m...@redhat.com
 
  ---
   drivers/s390/kvm/virtio_ccw.c | 1 -
   1 file changed, 1 deletion(-)
  
  diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
  index 6cbe6ef..bda52f1 100644
  --- a/drivers/s390/kvm/virtio_ccw.c
  +++ b/drivers/s390/kvm/virtio_ccw.c
  @@ -888,7 +888,6 @@ static void virtio_ccw_int_handler(struct ccw_device 
  *cdev,
  struct virtio_ccw_device *vcdev = dev_get_drvdata(cdev-dev);
  int i;
  struct virtqueue *vq;
  -   struct virtio_driver *drv;
   
  if (!vcdev)
  return;

*pokes Rusty*

Do you want to send this via the virtio tree? Otherwise, I'll queue
this with kvm/s390.

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

Re: [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices

[Cornelia Huck]

On Tue, 28 Oct 2014 16:40:18 +0100
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 On Tue,  7 Oct 2014 16:40:01 +0200
 Cornelia Huck cornelia.h...@de.ibm.com wrote:
 
  Introduce a helper function to indicate  whether a virtio device is
  operating in legacy or virtio standard mode.
  
  It may be used to make decisions about the endianess of virtio accesses
  and other virtio-1 specific changes, enabling us to support transitional
  devices.
  
  Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
  Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
  ---
   hw/virtio/virtio.c|6 +-
   include/hw/virtio/virtio-access.h |4 
   include/hw/virtio/virtio.h|   13 +++--
   3 files changed, 20 insertions(+), 3 deletions(-)
  
  diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
  index 7aaa953..e6ae3a0 100644
  --- a/hw/virtio/virtio.c
  +++ b/hw/virtio/virtio.c
  @@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void *opaque)
   VirtIODevice *vdev = opaque;
  
   assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
  -return vdev-device_endian != virtio_default_endian();
  +if (virtio_device_is_legacy(vdev)) {
  +return vdev-device_endian != virtio_default_endian();
  +}
  +/* Devices conforming to VIRTIO 1.0 or later are always LE. */
  +return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
   }
  
 
 Shouldn't we have some code doing the following somewhere ?
 
 if (!virtio_device_is_legacy(vdev)) {
 vdev-device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
 }
 
 also, since virtio-1 is LE only, do we expect device_endian to
 be different from VIRTIO_DEVICE_ENDIAN_LITTLE ?

device_endian should not depend on whether the device is legacy or not.
virtio_is_big_endian always returns false for virtio-1 devices, though.

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

Re: [PATCH] virtio_ccw: remove unsued variable

[Cornelia Huck]

On Fri, 31 Oct 2014 10:31:19 +1030
Rusty Russell ru...@rustcorp.com.au wrote:

 Cornelia Huck cornelia.h...@de.ibm.com writes:
  On Tue, 28 Oct 2014 19:37:58 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
 
  On Tue, Oct 28, 2014 at 04:39:12PM +0100, Sebastian Ott wrote:
 
   virtio_ccw: remove unused variable
   
   Fix this warning:
   drivers/s390/kvm/virtio_ccw.c: In function ‘virtio_ccw_int_handler’:
   drivers/s390/kvm/virtio_ccw.c:891:24: warning: unused variable ‘drv’ 
   [-Wunused-variable]
   struct virtio_driver *drv;
   
   Signed-off-by: Sebastian Ott seb...@linux.vnet.ibm.com
   Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
  
  Acked-by: Michael S. Tsirkin m...@redhat.com
  
   ---
drivers/s390/kvm/virtio_ccw.c | 1 -
1 file changed, 1 deletion(-)
   
   diff --git a/drivers/s390/kvm/virtio_ccw.c 
   b/drivers/s390/kvm/virtio_ccw.c
   index 6cbe6ef..bda52f1 100644
   --- a/drivers/s390/kvm/virtio_ccw.c
   +++ b/drivers/s390/kvm/virtio_ccw.c
   @@ -888,7 +888,6 @@ static void virtio_ccw_int_handler(struct ccw_device 
   *cdev,
struct virtio_ccw_device *vcdev = dev_get_drvdata(cdev-dev);
int i;
struct virtqueue *vq;
   -struct virtio_driver *drv;

if (!vcdev)
return;
 
  *pokes Rusty*
 
  Do you want to send this via the virtio tree? Otherwise, I'll queue
  this with kvm/s390.
 
 *Rusty wakes*.
 
 I assumed you would take it when I skimmed the patch, but to be formal:
 
 Acked-by: Rusty Russell ru...@rustcorp.com.au

Applied.

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

Re: [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices

[Cornelia Huck]

On Thu, 30 Oct 2014 23:29:50 +0100
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 On Thu, 30 Oct 2014 19:02:01 +0100
 Cornelia Huck cornelia.h...@de.ibm.com wrote:
 
  On Tue, 28 Oct 2014 16:40:18 +0100
  Greg Kurz gk...@linux.vnet.ibm.com wrote:
  
   On Tue,  7 Oct 2014 16:40:01 +0200
   Cornelia Huck cornelia.h...@de.ibm.com wrote:
   
Introduce a helper function to indicate  whether a virtio device is
operating in legacy or virtio standard mode.

It may be used to make decisions about the endianess of virtio accesses
and other virtio-1 specific changes, enabling us to support transitional
devices.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/virtio/virtio.c|6 +-
 include/hw/virtio/virtio-access.h |4 
 include/hw/virtio/virtio.h|   13 +++--
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7aaa953..e6ae3a0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void 
*opaque)
 VirtIODevice *vdev = opaque;

 assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
-return vdev-device_endian != virtio_default_endian();
+if (virtio_device_is_legacy(vdev)) {
+return vdev-device_endian != virtio_default_endian();
+}
+/* Devices conforming to VIRTIO 1.0 or later are always LE. */
+return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
 }

   
   Shouldn't we have some code doing the following somewhere ?
   
   if (!virtio_device_is_legacy(vdev)) {
   vdev-device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
   }
   
   also, since virtio-1 is LE only, do we expect device_endian to
   be different from VIRTIO_DEVICE_ENDIAN_LITTLE ?
  
  device_endian should not depend on whether the device is legacy or not.
  virtio_is_big_endian always returns false for virtio-1 devices, though.
 
 Sorry, I had missed the virtio_is_big_endian() change: it that makes
 device_endian a legacy virtio only matter. 
 So why would we care to migrate the endian subsection when we have a
 virtio-1 device ?  Shouldn't virtio_device_endian_needed() return false
 for virtio-1 ?

Indeeed, we can just leave device_endian at the default value for
virtio-1 devices.

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

Re: [PATCH 1/2] virito: introduce methods of fixing device features

[Cornelia Huck]

On Thu, 13 Nov 2014 13:52:53 +0800
Jason Wang jasow...@redhat.com wrote:

typo in subject-prefix: s/virito/virtio/

 Buggy host may advertised buggy host features (a usual case is that host
 advertise a feature whose dependencies were missed). In this case, driver
 should detect and disable the buggy features by itself.
 
 This patch introduces driver specific fix_features() method which is called
 just before features finalizing to detect and disable buggy features
 advertised by host.

So the basic problem this patch fixes is that an individual driver may
only specify a static set of features but cannot specify any
dependencies, right? Adding a sanitizer step makes sense, I guess.

 
 Virtio-net will be the first user.
 
 Cc: Rusty Russell ru...@rustcorp.com.au
 Cc: Michael S. Tsirkin m...@redhat.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
  drivers/virtio/virtio.c   |  4 
  include/linux/virtio.h|  1 +
  include/linux/virtio_config.h | 12 
  3 files changed, 17 insertions(+)
 
 diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
 index df598dd..7001d6e 100644
 --- a/drivers/virtio/virtio.c
 +++ b/drivers/virtio/virtio.c
 @@ -181,6 +181,10 @@ static int virtio_dev_probe(struct device *_d)
   if (device_features  (1  i))
   set_bit(i, dev-features);
 
 + /* Fix buggy features advertised by host */
 + if (drv-fix_features)
 + drv-fix_features(dev);

I'd probably call this sanitize_features instead.

 +
   dev-config-finalize_features(dev);
 
   err = drv-probe(dev);

 diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
 index 7f4ef66..7bd89ea 100644
 --- a/include/linux/virtio_config.h
 +++ b/include/linux/virtio_config.h
 @@ -96,6 +96,18 @@ static inline bool virtio_has_feature(const struct 
 virtio_device *vdev,
   return test_bit(fbit, vdev-features);
  }
 
 +static inline void virtio_disable_feature(struct virtio_device *vdev,
 +  unsigned int fbit)
 +{
 + BUG_ON(fbit = VIRTIO_TRANSPORT_F_START);
 + BUG_ON(vdev-config-get_status(vdev) 
 +~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER));

When we add virtio-1 support, we can add a check for FEATURES_OK here,
so we're really on the safe side.

 +
 + virtio_check_driver_offered_feature(vdev, fbit);
 +
 + clear_bit(fbit, vdev-features);
 +}
 +
  static inline
  struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
   vq_callback_t *c, const char *n)

The approach looks good to me.

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

Re: [PATCH 2/2] virtio-net: fix buggy features advertised by host

[Cornelia Huck]

On Thu, 13 Nov 2014 13:52:54 +0800
Jason Wang jasow...@redhat.com wrote:

 This patch tries to detect the possible buggy features advertised by host
 and fix them. One example is booting virtio-net with only ctrl_vq disabled,
 qemu may still advertise many features which depends on it. This will
 trigger several BUG()s in virtnet_send_command().
 
 This patch utilizes the fix_features() method, and disables all features that
 depends on ctrl_vq if it was not advertised.
 
 This fixes the crash when booting with ctrl_vq=off.

That's a qemu device property, right? Might want to mention that, as
this line sounds like it is a kernel parameter.

 
 Cc: Rusty Russell ru...@rustcorp.com.au
 Cc: Michael S. Tsirkin m...@redhat.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
 Changes from V1:
 - fix the cut-and-paste error
 ---
  drivers/net/virtio_net.c | 35 +++
  1 file changed, 35 insertions(+)
 
 diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
 index ec2a8b4..6ce125e 100644
 --- a/drivers/net/virtio_net.c
 +++ b/drivers/net/virtio_net.c
 @@ -1948,6 +1948,40 @@ static int virtnet_restore(struct virtio_device *vdev)
  }
  #endif
 
 +static void virtnet_fix_features(struct virtio_device *dev)
 +{
 + if (!virtio_has_feature(dev, VIRTIO_NET_F_CTRL_VQ)) {
 + if (virtio_has_feature(dev, VIRTIO_NET_F_CTRL_RX)) {
 + pr_warning(Disable VIRTIO_NET_F_CTRL_RX since host 
 +does not advertise VIRTIO_NET_F_CTRL_VQ);
 + virtio_disable_feature(dev, VIRTIO_NET_F_CTRL_RX);
 + }

You should probably use dev_warn() or so, so that the user can figure
out which device the message is for. And perhaps add buggy hypervisor
to the message to make clear that it's not a guest problem.

I also like the suggestion to use a dependency array.

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

Re: [PATCH 1/2] virito: introduce methods of fixing device features

[Cornelia Huck]

On Thu, 13 Nov 2014 17:11:30 +0800
Jason Wang jasow...@redhat.com wrote:

 On 11/13/2014 04:46 PM, Cornelia Huck wrote:
  On Thu, 13 Nov 2014 13:52:53 +0800
  Jason Wang jasow...@redhat.com wrote:

  +static inline void virtio_disable_feature(struct virtio_device *vdev,
  +  unsigned int fbit)
  +{
  +  BUG_ON(fbit = VIRTIO_TRANSPORT_F_START);
  +  BUG_ON(vdev-config-get_status(vdev) 
  + ~(VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER));
  When we add virtio-1 support, we can add a check for FEATURES_OK here,
  so we're really on the safe side.
 
 
 If I read the spec correctly, FEATURES_OK was set only after writing the
 features bits to device. But we want to sanitize the them before.

I meant doing a BUG when FEATURES_OK is set - sorry for not being clear.

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

Re: [PATCH V3 1/2] virtio: introduce methods of sanitizing device features

[Cornelia Huck]

On Mon, 17 Nov 2014 11:37:01 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Nov 17, 2014 at 05:17:17PM +0800, Jason Wang wrote:
  Buggy host may advertised buggy host features (a usual case is that host
  advertise a feature whose dependencies were missed). In this case, driver
  should detect and disable the buggy features by itself.
  
  This patch introduces driver specific sanitize_features() method which is
  called just before features finalizing to detect and disable buggy features
  advertised by host.
  
  Virtio-net will be the first user.
  
  Cc: Rusty Russell ru...@rustcorp.com.au
  Cc: Michael S. Tsirkin m...@redhat.com
  Cc: Cornelia Huck cornelia.h...@de.ibm.com
  Cc: Wanlong Gao gaowanl...@cn.fujitsu.com
  Signed-off-by: Jason Wang jasow...@redhat.com
 
 Hmm this conflicts with virtio 1.0 work: we drop
 features as bitmap there.

But that's an implementation detail, no? We'll still need a way for the
driver to sanitize features, and I think this interface works just fine.

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

Re: [PATCH V3 1/2] virtio: introduce methods of sanitizing device features

[Cornelia Huck]

On Mon, 17 Nov 2014 12:11:39 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Nov 17, 2014 at 10:44:30AM +0100, Cornelia Huck wrote:
  On Mon, 17 Nov 2014 11:37:01 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Mon, Nov 17, 2014 at 05:17:17PM +0800, Jason Wang wrote:
Buggy host may advertised buggy host features (a usual case is that host
advertise a feature whose dependencies were missed). In this case, 
driver
should detect and disable the buggy features by itself.

This patch introduces driver specific sanitize_features() method which 
is
called just before features finalizing to detect and disable buggy 
features
advertised by host.

Virtio-net will be the first user.

Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Cornelia Huck cornelia.h...@de.ibm.com
Cc: Wanlong Gao gaowanl...@cn.fujitsu.com
Signed-off-by: Jason Wang jasow...@redhat.com
   
   Hmm this conflicts with virtio 1.0 work: we drop
   features as bitmap there.
  
  But that's an implementation detail, no? We'll still need a way for the
  driver to sanitize features, and I think this interface works just fine.
 
 Now that you mention it, I don't think we do.
 
 The spec is quite explicit that devices must not expose invalid
 combinations of features.

Unfortunately, this does not ensure that there won't be buggy
hypervisors out there, just as there's buggy hardware floating around.

 
 Admittedly, BUG_ON isn't very friendly to hypervisors.
 
 But e.g. failing probe seems better than trying to work around
 hypervisor bugs - otherwise we'll be stuck maintaining compatibility
 with hypervisors forever.

Good point. Failing probe is still much better than hitting BUG_ONs.

We'll still need a driver callback, though, that can return an error on
bogus feature bit combinations.

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

Re: [PATCH V3 1/2] virtio: introduce methods of sanitizing device features

[Cornelia Huck]

On Mon, 17 Nov 2014 12:28:49 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Nov 17, 2014 at 11:20:48AM +0100, Cornelia Huck wrote:
  On Mon, 17 Nov 2014 12:11:39 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Mon, Nov 17, 2014 at 10:44:30AM +0100, Cornelia Huck wrote:
On Mon, 17 Nov 2014 11:37:01 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Nov 17, 2014 at 05:17:17PM +0800, Jason Wang wrote:
  Buggy host may advertised buggy host features (a usual case is that 
  host
  advertise a feature whose dependencies were missed). In this case, 
  driver
  should detect and disable the buggy features by itself.
  
  This patch introduces driver specific sanitize_features() method 
  which is
  called just before features finalizing to detect and disable buggy 
  features
  advertised by host.
  
  Virtio-net will be the first user.
  
  Cc: Rusty Russell ru...@rustcorp.com.au
  Cc: Michael S. Tsirkin m...@redhat.com
  Cc: Cornelia Huck cornelia.h...@de.ibm.com
  Cc: Wanlong Gao gaowanl...@cn.fujitsu.com
  Signed-off-by: Jason Wang jasow...@redhat.com
 
 Hmm this conflicts with virtio 1.0 work: we drop
 features as bitmap there.

But that's an implementation detail, no? We'll still need a way for the
driver to sanitize features, and I think this interface works just fine.
   
   Now that you mention it, I don't think we do.
   
   The spec is quite explicit that devices must not expose invalid
   combinations of features.
  
  Unfortunately, this does not ensure that there won't be buggy
  hypervisors out there, just as there's buggy hardware floating around.
  
   
   Admittedly, BUG_ON isn't very friendly to hypervisors.
   
   But e.g. failing probe seems better than trying to work around
   hypervisor bugs - otherwise we'll be stuck maintaining compatibility
   with hypervisors forever.
  
  Good point. Failing probe is still much better than hitting BUG_ONs.
  
  We'll still need a driver callback, though, that can return an error on
  bogus feature bit combinations.
 
 Why bother?  Just check features at start of probe, and return an error.

So we'd fail probing due to bogus features after setting FEATURES_OK in
the virtio-1 case, won't we? Feels a bit weird, but seems to be covered
by the spec.

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

Re: [PATCH V2 net] virtio-net: validate features during probe

[Cornelia Huck]

On Wed, 19 Nov 2014 15:21:29 +0800
Jason Wang jasow...@redhat.com 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 ru...@rustcorp.com.au
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Cornelia Huck cornelia.h...@de.ibm.com
 Cc: Wanlong Gao gaowanl...@cn.fujitsu.com
 Signed-off-by: Jason Wang jasow...@redhat.com
 ---
 Changes from V1:
 - Drop VIRTIO_NET_F_*_UFO from the checklist, since it was disabled
 ---
  drivers/net/virtio_net.c | 91 
 
  1 file changed, 91 insertions(+)
 
 diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
 index ec2a8b4..b16a761 100644
 --- a/drivers/net/virtio_net.c
 +++ b/drivers/net/virtio_net.c
 @@ -1673,6 +1673,93 @@ 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)) {

Do an early return, get rid of one indentation level?

 + 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,

s/hyperviser/hypervisor/ (also below)

 + f, feature);
 + return -EINVAL;
 + }
 + }
 + }
 +
 + return 0;
 +}
 +
 +static int virtnet_check_features(struct virtio_device *dev)
 +{
 + unsigned int features_for_ctrl_vq[] = {
 + VIRTIO_NET_F_CTRL_RX,
 + VIRTIO_NET_F_CTRL_VLAN,
 + VIRTIO_NET_F_GUEST_ANNOUNCE,
 + VIRTIO_NET_F_MQ,
 + VIRTIO_NET_F_CTRL_MAC_ADDR
 + };
 + unsigned int features_for_guest_csum[] = {
 + VIRTIO_NET_F_GUEST_TSO4,
 + VIRTIO_NET_F_GUEST_TSO6,
 + VIRTIO_NET_F_GUEST_ECN,
 + };
 + unsigned int features_for_host_csum[] = {
 + VIRTIO_NET_F_HOST_TSO4,
 + VIRTIO_NET_F_HOST_TSO6,
 + VIRTIO_NET_F_HOST_ECN,
 + };

I'm wondering whether it would be easier to read if you listed all
prereqs per feature instead of all features that depend on a feature?
It would still be hard to express the v4/v6 or conditions below in
tables, though.

Or call the arrays features_depending_on_foo?

 + int err;
 +
 + err = virtnet_validate_features(dev, features_for_ctrl_vq,
 + ARRAY_SIZE(features_for_ctrl_vq),
 + VIRTIO_NET_F_CTRL_VQ);
 + if (err)
 + return err;

If you already print a message that a user may use to fix their
hypervisor (or bug someone about it), would it make sense to check all
dependencies and print a full list of everything that is broken in the
advertised feature bits? I usually hate it if I fix one thing only to
hit the next bug when the program could have already told me about
everything I need to fix :)

 +
 + err = virtnet_validate_features(dev, features_for_guest_csum,
 + ARRAY_SIZE(features_for_guest_csum),
 + VIRTIO_NET_F_GUEST_CSUM);
 + if (err)
 + return err;
 +
 + err = virtnet_validate_features(dev, features_for_host_csum,
 + ARRAY_SIZE(features_for_host_csum),
 + VIRTIO_NET_F_CSUM);
 + if (err)
 + return err;
 +
 + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) 
 + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
 +  !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
 + dev_err(dev-dev,
 + buggy hyperviser: feature 0x%x was advertised but its 
 dependency 0x%x or 0x%x was not,
 + VIRTIO_NET_F_GUEST_ECN,
 + VIRTIO_NET_F_GUEST_TSO4,
 + VIRTIO_NET_F_GUEST_TSO6);
 + return -EINVAL;
 + }
 +
 + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) 
 + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
 +  !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
 + dev_err(dev-dev,
 + buggy hyperviser: feature 0x%x was advertised but its 
 dependency 0x%x or 0x%x was not,

Hypervisor bug: advertised feature foo but not bar or baz

?

 + VIRTIO_NET_F_HOST_ECN

Re: [PATCH net V5] virtio-net: validate features during probe

[Cornelia Huck]

On Thu, 20 Nov 2014 17:03:05 +0800
Jason Wang jasow...@redhat.com wrote:

 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.
 
 Cc: Rusty Russell ru...@rustcorp.com.au
 Cc: Cornelia Huck cornelia.h...@de.ibm.com
 Cc: Wanlong Gao gaowanl...@cn.fujitsu.com
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 Signed-off-by: Jason Wang jasow...@redhat.com

 +static bool virtnet_validate_features(struct virtio_device *vdev)
 +{
 + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) 
 + (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX,
 +  VIRTIO_NET_F_CTRL_VQ) ||
 +  VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN,
 +  VIRTIO_NET_F_CTRL_VQ) ||
 +  VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE,
 +  VIRTIO_NET_F_CTRL_VQ) ||
 +  VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ, VIRTIO_NET_F_CTRL_VQ) ||
 +  VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR,
 +  VIRTIO_NET_F_CTRL_VQ))) {
 + return false;
 + }
 +
 + return true;
 +}

I still think it may make sense to print all incorrectly set bits, but
let's just fix the BUG() trigger for now.

Acked-by: Cornelia Huck cornelia.h...@de.ibm.com

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

[PATCH RFC v2 02/12] virtio: cull virtio_bus_set_vdev_features

[Cornelia Huck]

The only user of this function was virtio-ccw, and it should use
virtio_set_features() like everybody else: We need to make sure
that bad features are masked out properly, which this function did
not do.

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 hw/s390x/virtio-ccw.c  |3 +--
 hw/virtio/virtio-bus.c |   14 --
 include/hw/virtio/virtio-bus.h |3 ---
 3 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index ea236c9..84f17bc 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -400,8 +400,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
 features.features = ldl_le_phys(address_space_memory, ccw.cda);
 if (features.index  ARRAY_SIZE(dev-host_features)) {
-virtio_bus_set_vdev_features(dev-bus, features.features);
-vdev-guest_features = features.features;
+virtio_set_features(vdev, features.features);
 } else {
 /*
  * If the guest supports more feature bits, assert that it
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index eb77019..a8ffa07 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -109,20 +109,6 @@ uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
 return k-get_features(vdev, requested_features);
 }
 
-/* Set the features of the plugged device. */
-void virtio_bus_set_vdev_features(VirtioBusState *bus,
-  uint32_t requested_features)
-{
-VirtIODevice *vdev = virtio_bus_get_device(bus);
-VirtioDeviceClass *k;
-
-assert(vdev != NULL);
-k = VIRTIO_DEVICE_GET_CLASS(vdev);
-if (k-set_features != NULL) {
-k-set_features(vdev, requested_features);
-}
-}
-
 /* Get bad features of the plugged device. */
 uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus)
 {
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 0756545..0d2e7b4 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -84,9 +84,6 @@ size_t virtio_bus_get_vdev_config_len(VirtioBusState *bus);
 /* Get the features of the plugged device. */
 uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
 uint32_t requested_features);
-/* Set the features of the plugged device. */
-void virtio_bus_set_vdev_features(VirtioBusState *bus,
-  uint32_t requested_features);
 /* Get bad features of the plugged device. */
 uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus);
 /* Get config of the plugged device. */
-- 
1.7.9.5

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