Re: [PATCH 2/4 v2] xen kconfig: relax INPUT_XEN_KBDDEV_FRONTEND deps
- Original Message - On Wed, Jan 11, 2012 at 05:36:39PM +0100, Andrew Jones wrote: PV-on-HVM guests may want to use the xen keyboard/mouse frontend, but they don't use the xen frame buffer frontend. For this case it doesn't make much sense for INPUT_XEN_KBDDEV_FRONTEND to depend on XEN_FBDEV_FRONTEND. The opposite direction always makes more sense, i.e. What is the disadvantege of keeping it as is? If you don't want FB, but you do want KBD, then you're stuck with FB anyway, even though it isn't necessary. Perhaps I'm the only one who ever considering building a config without FB? if you're using xenfb, then you'll want xenkbd. Switch the dependencies. You are missing the CC to the proper maintainer. I added them the last time you reminded me. See the following message-ids for the thread where this patch was discussed and then led to the V2 here. 20120109075911.ga4...@core.coreip.homeip.net 725950ad-d5cf-4ddb-9870-a5c8d75cf...@zmail13.collab.prod.int.phx2.redhat.com 1326131501-9610-1-git-send-email-drjo...@redhat.com I've re-added them to this thread, since the multiple posting/reposting must have lost them again. Also, did you test this with PV and PVonHVM guests? Testing-wise you don't really need to do much more then 'make oldconfig'. FB doesn't work on pv-on-hvm, the mod wouldn't even init if you tried as there's a if (!xen_pv_domain()) return -ENODEV; I have tested a pv-on-hvm guest after this patch with FB off though, which worked. For PV guests using FB, they'll work the same, as you wouldn't want to change your config. Drew Signed-off-by: Andrew Jones drjo...@redhat.com --- drivers/input/misc/Kconfig |2 +- drivers/video/Kconfig |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig index 22d875f..36c15bf 100644 --- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -533,7 +533,7 @@ config INPUT_CMA3000_I2C config INPUT_XEN_KBDDEV_FRONTEND tristate Xen virtual keyboard and mouse support - depends on XEN_FBDEV_FRONTEND + depends on XEN default y select XEN_XENBUS_FRONTEND help diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index d83e967..3e38c2f 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -2263,7 +2263,7 @@ config FB_VIRTUAL config XEN_FBDEV_FRONTEND tristate Xen virtual frame buffer support - depends on FB XEN + depends on FB XEN INPUT_XEN_KBDDEV_FRONTEND select FB_SYS_FILLRECT select FB_SYS_COPYAREA select FB_SYS_IMAGEBLIT -- 1.7.7.5 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH 1/4] xen kconfig: keep XEN_XENBUS_FRONTEND builtin
On Thu, Jan 12, 2012 at 05:49:57AM -0500, Andrew Jones wrote: - Original Message - On Wed, Jan 11, 2012 at 05:36:38PM +0100, Andrew Jones wrote: When XEN_XENBUS_FRONTEND gets selected as a module it can lead to unbootable configs. If we need it, then we should just build it in. Hm, don't the frontends by themsevles load this module? So if you do 'modprobe xen-pcifront' it would load this automatically? The problem is on boot, getting it there before we need xen-blkfront. I think it might solvable if you make sure your tooling pushes the xenbus module into your initrd. However we've had problems with So it seems we also need patches for dracut and initramfs here? Or is it possible to make the modules (fb) try to load xenbus frontend automatically? Preferrably one would do this: modprobe xen_fbfront which would then automatically load xenbus_module, xen_kbdfront Better yet if udev/kudzu figured out this automtically and loaded the modules. Is there a mechanism to automatically have udev do all of this loading? Let CC the Debian maintainer on this converstation as he might have some ideas in this. platform_pci being a module in the past, and if I'm not mistaken Right, but that driver is not tied in with the frontends. It just pokes at the ioport to disable QEMU HVM drivers. that was one of the motivators for building it in (5fbdc10395cd). I see this as a similar story. Drew Signed-off-by: Andrew Jones drjo...@redhat.com --- drivers/xen/Kconfig |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig index 8795480..1d24061 100644 --- a/drivers/xen/Kconfig +++ b/drivers/xen/Kconfig @@ -118,7 +118,7 @@ config XEN_SYS_HYPERVISOR but will have no xen contents. config XEN_XENBUS_FRONTEND - tristate + bool config XEN_GNTDEV tristate userspace grant access device driver -- 1.7.7.5 ___ Xen-devel mailing list xen-de...@lists.xensource.com http://lists.xensource.com/xen-devel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] [PATCH] xen: remove CONFIG_XEN_DOM0 compile option
On 01/10/2012 05:44 AM, Stefano Stabellini wrote: On Mon, 9 Jan 2012, Andrew Jones wrote: I guess if we did the s/XEN_DOM0/LOCAL_APIC IO_APIC ACPI/ in arch/x86/pci/xen.c it would be pretty easy to review for equivalence. Then keep CONFIG_PRIVILIGED, but drop XEN_DOM0 from everywhere else and compile in the 3 files. I don't think it makes much sense to do it though. XEN_DOM0 keeps things tidier now and might be useful later. we can keep things clean with the following: #ifdef CONFIG_LOCAL_APIC CONFIG_IO_APIC CONFIG_ACPI CONFIG_PCI_XEN #define XEN_DOM0 #endif in include/xen/xen.h. So in the source files we can still '#ifdef XEN_DOM0', but at the same time we can get rid of the build symbol: everybody wins. No, really, I think this is silly. This kind of dependency information should be encoded in the Kconfig system, and not reimplemented in an ad-hoc way. If there were a clean way to do what Andrew wants then I'd support it, but this thread has descended into a spiral of madness, which makes me think its a lost cause. If the root complaint is that customers think that anything set in .config is a supported feature, then the solutions are to support all the features in .config, re-educate the customers that they're wrong, or maintain a local patch to do this stuff. J ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-net: add module alias
On 11.01.2012 08:54, Stephen Hemminger wrote: By adding the a module alias, programs (or users) won't have to explicitly call modprobe. Vhost-net will always be available if built into the kernel. It does require assigning a permanent minor number for depmod to work. Choose one next to TUN since this driver is related to it. Why do you think a statically-allocated device number will do any good at all? Static /dev is gone almost completely, at least on the systems where whole virt stuff makes any sense, so you don't have pre-created vhost-net device anymore, and hence this allocation makes no sense. Just IMHO anyway. Thanks, /mjt ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-net: add module alias
On 11.01.2012 20:58, Stephen Hemminger wrote: On Wed, 11 Jan 2012 11:07:47 +0400 Michael Tokarev m...@tls.msk.ru wrote: On 11.01.2012 08:54, Stephen Hemminger wrote: By adding the a module alias, programs (or users) won't have to explicitly call modprobe. Vhost-net will always be available if built into the kernel. It does require assigning a permanent minor number for depmod to work. Choose one next to TUN since this driver is related to it. Why do you think a statically-allocated device number will do any good at all? Static /dev is gone almost completely, at least on the systems where whole virt stuff makes any sense, so you don't have pre-created vhost-net device anymore, and hence this allocation makes no sense. Just IMHO anyway. [] See also: https://lkml.org/lkml/2010/5/21/134 Aha. So udev pre-creates statically-allocated devnodes nowadays: Udev will pick up the depmod created file on startup and create all the static device nodes which the kernel modules specify, so that these modules get automatically loaded when the device node is accessed... This was the part I missed. Now it all looks logically. Thanks, /mjt ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-net: add module alias (v2)
On Wed, Jan 11, 2012 at 09:16:53AM -0800, Stephen Hemminger wrote:
By adding the correct module alias, programs won't have to explicitly
call modprobe. Vhost-net will always be available if built into the kernel.
It does require assigning a permanent minor number for depmod to work.
Choose one next to TUN since this driver is related to it.
Also, use C99 style initialization.
Signed-off-by: Stephen Hemminger shemmin...@vyatta.com
---
v2 - document minor number and make sure to not overlap
Documentation/devices.txt |2 ++
drivers/vhost/net.c|8 +---
include/linux/miscdevice.h |1 +
3 files changed, 8 insertions(+), 3 deletions(-)
--- a/drivers/vhost/net.c 2012-01-10 10:56:58.883179194 -0800
+++ b/drivers/vhost/net.c 2012-01-10 19:48:23.650225892 -0800
@@ -856,9 +856,9 @@ static const struct file_operations vhos
};
static struct miscdevice vhost_net_misc = {
- MISC_DYNAMIC_MINOR,
- vhost-net,
- vhost_net_fops,
+ .minor = VHOST_NET_MINOR,
+ .name = vhost-net,
+ .fops = vhost_net_fops,
};
static int vhost_net_init(void)
@@ -879,3 +879,5 @@ MODULE_VERSION(0.0.1);
MODULE_LICENSE(GPL v2);
MODULE_AUTHOR(Michael S. Tsirkin);
MODULE_DESCRIPTION(Host kernel accelerator for virtio net);
+MODULE_ALIAS_MISCDEV(VHOST_NET_MINOR);
+MODULE_ALIAS(devname:vhost-net);
--- a/include/linux/miscdevice.h 2012-01-10 10:56:59.779189436 -0800
+++ b/include/linux/miscdevice.h 2012-01-11 09:13:20.803694316 -0800
@@ -42,6 +42,7 @@
#define AUTOFS_MINOR 235
#define MAPPER_CTRL_MINOR236
#define LOOP_CTRL_MINOR 237
+#define VHOST_NET_MINOR 238
#define MISC_DYNAMIC_MINOR 255
struct device;
--- a/Documentation/devices.txt 2012-01-10 10:56:53.399116518 -0800
+++ b/Documentation/devices.txt 2012-01-11 09:12:49.251197653 -0800
@@ -447,6 +447,8 @@ Your cooperation is appreciated.
234 = /dev/btrfs-controlBtrfs control device
235 = /dev/autofs Autofs control device
236 = /dev/mapper/control Device-Mapper control device
+ 237 = /dev/vhost-netHost kernel accelerator for virtio net
+
240-254 Reserved for local use
255 Reserved for MISC_DYNAMIC_MINOR
You added vhost-net minor as 238 in the code and 237 in the
documentation. That's because loop-control is missing from the docs.
Regards.
Cascardo.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PULL] virtio and lguest
On Wed, Jan 11, 2012 at 9:22 PM, Rusty Russell ru...@rustcorp.com.au wrote: Amit Shah (12): virtio: pci: switch to new PM API Hmm. Afaik, this is broken, or at least not complete. Sure, it switches to the new PM API, but it still does the PCI ops itself. It should not need to - the PCI layer will do the power state and standard PCI device state saving. And setting the PCI_D3hot state when shared interrupts can still happen at suspend time is just a bad idea. So I think you're doing extra work and introducing bugs by doing so - the default PCI bus operations should already do all you do, just do it better. And then you can use the SIMPLE_DEV_PM_OPS() to build the dev_pm_ops structure and get all the normal cases right automatically. I don't know if there is any particularly good example of this, but you can see some of the network drivers for examples of this. Notice how they don't need to worry about PCI power states etc at all, they just need to worry about the actual chip suspend/resume (and for a network driver, you'd do the netif_device_detach/netif_device_attach etc) Linus ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 7/11] virtio_pci: new, capability-aware driver.
On Thu, Jan 12, 2012 at 12:12:17PM +1030, Rusty Russell wrote:
On Thu, 12 Jan 2012 00:02:33 +0200, Michael S. Tsirkin m...@redhat.com
wrote:
Look, we have a race currently. Let us not tie a bug fix to a huge
rewrite with unclear performance benefits, please.
In theory, yes. In practice, we bandaid it.
I think in the short term we change -get to get the entire sequence
twice, and check it's the same. Theoretically, still racy, but it does
cut the window. And we haven't seen the bug yet, either.
I thought about this some more. Since we always get
an interrupt on config changes, it seems that a rather
robust method would be to just synchronize against that.
Something like the below (warning - completely untested).
Still need to think about memory barriers, overflow etc.
What do you think?
Signed-off-by: Michael S. Tsirkin m...@redhat.com
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 03d1984..b5df385 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -57,6 +57,7 @@ struct virtio_pci_device
unsigned msix_used_vectors;
/* Whether we have vector per vq */
bool per_vq_vectors;
+ atomic_t config_changes;
};
/* Constants for MSI-X */
@@ -125,6 +126,19 @@ static void vp_finalize_features(struct virtio_device
*vdev)
iowrite32(vdev-features[0], vp_dev-ioaddr+VIRTIO_PCI_GUEST_FEATURES);
}
+/* wait for pending irq handlers */
+static void vp_synchronize_vectors(struct virtio_device *vdev)
+{
+ struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+ int i;
+
+ if (vp_dev-intx_enabled)
+ synchronize_irq(vp_dev-pci_dev-irq);
+
+ for (i = 0; i vp_dev-msix_vectors; ++i)
+ synchronize_irq(vp_dev-msix_entries[i].vector);
+}
+
/* virtio config-get() implementation */
static void vp_get(struct virtio_device *vdev, unsigned offset,
void *buf, unsigned len)
@@ -134,9 +148,20 @@ static void vp_get(struct virtio_device *vdev, unsigned
offset,
VIRTIO_PCI_CONFIG(vp_dev) + offset;
u8 *ptr = buf;
int i;
-
- for (i = 0; i len; i++)
- ptr[i] = ioread8(ioaddr + i);
+ int uninitialized_var(c);
+ c = atomic_read(vp_dev-config_changes);
+ /* Make sure read is done before we get the first config byte */
+ rmb();
+ do {
+ for (i = 0; i len; i++)
+ ptr[i] = ioread8(ioaddr + i);
+ /* Synchronize with config interrupt */
+ vp_synchronize_vectors(vdev);
+ /*
+* For multi-byte fields, we might get a config change interrupt
+* between byte reads. If this happens, retry the read.
+*/
+ } while (c != atomic_read(vp_dev-config_changes))
}
/* the config-set() implementation. it's symmetric to the config-get()
@@ -169,19 +194,6 @@ static void vp_set_status(struct virtio_device *vdev, u8
status)
iowrite8(status, vp_dev-ioaddr + VIRTIO_PCI_STATUS);
}
-/* wait for pending irq handlers */
-static void vp_synchronize_vectors(struct virtio_device *vdev)
-{
- struct virtio_pci_device *vp_dev = to_vp_device(vdev);
- int i;
-
- if (vp_dev-intx_enabled)
- synchronize_irq(vp_dev-pci_dev-irq);
-
- for (i = 0; i vp_dev-msix_vectors; ++i)
- synchronize_irq(vp_dev-msix_entries[i].vector);
-}
-
static void vp_reset(struct virtio_device *vdev)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -213,6 +225,8 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
drv = container_of(vp_dev-vdev.dev.driver,
struct virtio_driver, driver);
+ atomic_inc(vp_dev-config_changes);
+
if (drv drv-config_changed)
drv-config_changed(vp_dev-vdev);
return IRQ_HANDLED;
@@ -646,6 +660,7 @@ static int __devinit virtio_pci_probe(struct pci_dev
*pci_dev,
vp_dev-vdev.config = virtio_pci_config_ops;
vp_dev-pci_dev = pci_dev;
INIT_LIST_HEAD(vp_dev-virtqueues);
+ atomic_set(vp_dev-config_changes, 0);
spin_lock_init(vp_dev-lock);
/* Disable MSI/MSIX to bring device to a known good state. */
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 7/11] virtio_pci: new, capability-aware driver.
On Thu, 12 Jan 2012 08:00:10 +0200, Michael S. Tsirkin m...@redhat.com
wrote:
On Thu, Jan 12, 2012 at 12:31:09PM +1030, Rusty Russell wrote:
If we use a 32-bit counter, we also get this though, right?
If counter has changed, it's a config interrupt...
But we need an exit to read the counter. We can put the counter
in memory but this looks suspiciously like a simplified VQ -
so why not use a VQ then?
Because now a driver first gets the data from config space. But from
then on, they have to get it from the vq, and ignore the config space.
That's a bit weird.
If we do require config VQ anyway, why not use it to notify
guest of config changes? Guest could pre-post an in buffer
and host uses that.
We could, but it's an additional burden on each device. vqs are cheap,
but not free. And the config area is so damn convenient...
Not if you start playing with counters, checking it twice,
reinvent all kind of barriers ...
None of that appears inside the driver, though. And let's be honest,
it's not *that* bad (very approx code):
static u32 vp_get_gen(struct virtio_pci_device *vp_dev)
{
u32 gen;
do {
gen = ioread32(vp_dev-ioaddr + VIRTIO_PCI_CONFIG_GEN);
} while (unlikely((gen 1) == 1));
virtio_rmb();
return gen;
}
static bool vp_check_gen(struct virtio_pci_device *vp_dev, u32 gen)
{
virtio_rmb();
return ioread32(vp_dev-ioaddr + VIRTIO_PCI_CONFIG_GEN) == gen;
}
static void vp_get32(struct virtio_device *vdev, unsigned offset, u32 *buf)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
u32 gen;
do {
gen = vp_get_gen(vdev);
*buf = ioread32(vp_dev-ioaddr + VIRTIO_PCI_CONFIG(vp_dev) +
offset);
} while (unlikely(!vp_check_gen(vp_dev, gen)));
}
...
It was suggested by others, but I think TCP Acks are the classic one.
12 + 14 + 20 + 40 = 86 bytes with virtio_net_hdr_mrg_rxbuf at the front.
That's only the simplest IPv4, right?
Anyway, this spans multiple descriptors so this complicates allocation
significantly.
Yes, I think any general-but-useful inline will need to span multiple
descriptors. That's part of the fun!
Let's get totally crazy and implement our ring in stripes, like:
00 04 08 12 01 05 09 13 02 06 10 14 03 07 11 15
That way consecutive (32-byte) descriptors don't share a cacheline!
(Not serious... quiet.)
Yes, I'm thinking #define VIRTIO_F_VIRTIO2 (-1). For PCI, this gets
mapped into a are we using the new config layout?. For others, it
gets mapped into a transport-specific feature.
(I'm sure you get it, but for the others) This is because I want to be
draw a clear line between all the legacy stuff at the same time, not
have to support part of it later because someone might not flip the
feature bit.
So my point is, config stuff and ring are completely separate,
they are different layers.
Absolutely, and we should analyze them separately as well as together.
*But* for maintenance is far easier if we only have to test new
config+new ring and old config+old ring. They do interact, because
remember, the allocation of the ring changes with new config, too...
Cheers,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PULL] virtio and lguest
On Thu, 12 Jan 2012 16:29:14 -0800, Linus Torvalds torva...@linux-foundation.org wrote: On Wed, Jan 11, 2012 at 9:22 PM, Rusty Russell ru...@rustcorp.com.au wrote: Amit Shah (12): virtio: pci: switch to new PM API Hmm. Afaik, this is broken, or at least not complete. Sure, it switches to the new PM API, but it still does the PCI ops itself. It should not need to - the PCI layer will do the power state and standard PCI device state saving. And setting the PCI_D3hot state when shared interrupts can still happen at suspend time is just a bad idea. So I think you're doing extra work and introducing bugs by doing so - the default PCI bus operations should already do all you do, just do it better. And then you can use the SIMPLE_DEV_PM_OPS() to build the dev_pm_ops structure and get all the normal cases right automatically. I don't know if there is any particularly good example of this, but you can see some of the network drivers for examples of this. Notice how they don't need to worry about PCI power states etc at all, they just need to worry about the actual chip suspend/resume (and for a network driver, you'd do the netif_device_detach/netif_device_attach etc) Ok, I'll confess complete ignorance, and wait for Amit to respond. I must admit that PM for virtual devices is not a personal priority... Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-net: add module alias (v2.1)
From: Stephen Hemminger shemmin...@vyatta.com Date: Wed, 11 Jan 2012 21:30:38 -0800 Subject: vhost-net: add module alias (v2.1) By adding some module aliases, programs (or users) won't have to explicitly call modprobe. Vhost-net will always be available if built into the kernel. It does require assigning a permanent minor number for depmod to work. Also: - use C99 style initialization. - add missing entry in documentation for loop-control Signed-off-by: Stephen Hemminger shemmin...@vyatta.com ACKs, NACKs? What is happening here? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost-net: add module alias (v2.1)
On Fri, Jan 13, 2012 at 05:07, David Miller da...@davemloft.net wrote: From: Stephen Hemminger shemmin...@vyatta.com Date: Wed, 11 Jan 2012 21:30:38 -0800 Subject: vhost-net: add module alias (v2.1) By adding some module aliases, programs (or users) won't have to explicitly call modprobe. Vhost-net will always be available if built into the kernel. It does require assigning a permanent minor number for depmod to work. Also: - use C99 style initialization. - add missing entry in documentation for loop-control Signed-off-by: Stephen Hemminger shemmin...@vyatta.com ACKs, NACKs? What is happening here? In general, static minors are acceptable and very useful to make on-demand loading of kernel modules working. They should be used only for single-instance devices though, which usually means: One single static device name associated with a module. That looks all fine here, and for what it's worth: Acked-By: Kay Sievers kay.siev...@vrfy.org Kay ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
