* Re: [Qemu-devel] [RFC 0/4] virtio-mmio transport
From: Pawel Moll @ 2011-12-12 11:16 UTC (permalink / raw)
To: Paul Brook
Cc: Peter Maydell, qemu-devel@nongnu.org, virtualization,
Anthony Liguori, Paolo Bonzini
In-Reply-To: <201112120252.35295.paul@codesourcery.com>
On Mon, 2011-12-12 at 02:52 +0000, Paul Brook wrote:
> I've taken a look at the virtion-mmio spec, and it looks fairly
> reasonable.
>
> The only thing I'd change is the GuestPageSize/QueuePFN mess. Seems like just
> using straight 64-bit addresses would be a better solution. Maybe split into
> a high/low pair to keep all registers as 32-bit registers.
This can be done fairly simple by:
1. Bumping Version register content.
2. Adding two registers: QueueAddrLow (0x44) and QueueAddrHigh (0x48).
3. Describing the QueuePFN as obsolete.
Than the driver would just use Addr or PFN depending on the device's
version.
I can do that, but not this year (on holiday from Friday 16th, without
any access to Internet whatsoever :-) One think to be decided is in what
order the halfs should be filled? Low first, then high? High then low?
Does it matter at all? :-)
Cheers!
Paweł
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC 7/11] virtio_pci: new, capability-aware driver.
From: Michael S. Tsirkin @ 2011-12-12 11:49 UTC (permalink / raw)
To: Rusty Russell; +Cc: Sasha Levin, virtualization
In-Reply-To: <87boreohhs.fsf@rustcorp.com.au>
On Mon, Dec 12, 2011 at 09:15:03AM +1030, Rusty Russell wrote:
> On Sun, 11 Dec 2011 11:42:56 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Dec 08, 2011 at 09:09:33PM +1030, Rusty Russell wrote:
> > > +/* There is no iowrite64. We use two 32-bit ops. */
> > > +static void iowrite64(u64 val, const __le64 *addr)
> > > +{
> > > + iowrite32((u32)val, (__le32 *)addr);
> > > + iowrite32(val >> 32, (__le32 *)addr + 1);
> > > +}
> > > +
> >
> > Let's put addr_lo/addr_hi in the structure then,
> > to make the fact this field is not atomic explicit?
>
> Good point, assuming I haven't missed something.
>
> Are 64-bit accesses actually unknown in PCI-land? Or is this a limited
> availability thing?
>
> Thanks,
> Rusty.
I think PCI can optionally support atomic 64 bit accesses, but not all
architectures can generate them.
--
MST
^ permalink raw reply
* Re: [PATCH RFC] virtio_net: fix refill related races
From: Michael S. Tsirkin @ 2011-12-12 11:54 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, netdev, linux-kernel, virtualization
In-Reply-To: <878vmioh10.fsf@rustcorp.com.au>
On Mon, Dec 12, 2011 at 09:25:07AM +1030, Rusty Russell wrote:
> On Sun, 11 Dec 2011 16:44:29 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Dec 08, 2011 at 03:07:29PM +1030, Rusty Russell wrote:
> > > On Wed, 7 Dec 2011 17:21:22 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > Fix theoretical races related to refill work:
> > > > 1. After napi is disabled by ndo_stop, refill work
> > > > can run and re-enable it.
> > > > 2. Refill can reschedule itself, if this happens
> > > > it can run after cancel_delayed_work_sync,
> > > > and will access device after it is destroyed.
> > > >
> > > > As a solution, add flags to track napi state and
> > > > to disable refill, and toggle them on start, stop
> > > > and remove; check these flags on refill.
> > >
> > > Why isn't a "dont-readd" flag sufficient?
> > >
> > > Cheers,
> > > Rusty.
> >
> > I started with that, but here's the problem I wanted to
> > address:
> >
> > - we run out of descriptors and schedule refill work
> > - ndo_close runs
> > - refill work runs
> > - ndo_open runs
>
> (s/ndo_close/ndo_stop/)
>
> You don't think we should do any refills on a closed device? If so, we
> simply move the refill-stop code into ndo_stop (aka virtnet_close), and
> the refill-start code into ndo_open (aka. virtnet_open). Right?
We can do that, yes. We'll have to also cancel work if outstanding.
It seems a bigger change though. I'm especially concerned
about putting refill-start in virtnet_open, suddenly
it's always running where it used to only run after
we have consumed some buffers. I'm just concerned about
doing big changes that late in release cycle but maybe
that's OK.
> Orthogonally, the refill-stop code is still buggy, as you noted.
Sorry I don't understand how it's still buggy.
> And
> for self-rearming timers the pattern I've always used is a flag.
>
> Or am I being obtuse again? :)
> Rusty.
^ permalink raw reply
* Re: [Qemu-devel] [RFC 0/4] virtio-mmio transport
From: Paul Brook @ 2011-12-12 14:45 UTC (permalink / raw)
To: Pawel Moll
Cc: Peter Maydell, qemu-devel@nongnu.org, virtualization,
Anthony Liguori, Paolo Bonzini
In-Reply-To: <1323688579.2391.11.camel@hornet.cambridge.arm.com>
> I can do that, but not this year (on holiday from Friday 16th, without
> any access to Internet whatsoever :-) One think to be decided is in what
> order the halfs should be filled? Low first, then high? High then low?
> Does it matter at all? :-)
My inital though was that you shouldn't be changing this value when the ring
is enabled. Unfortunately you disable the ring by setting the address to zero
so that argument doesn't work :-/
I suggest that the device to buffer writes to the high part, and construct the
actual 64-bit value when the low part is written. That allows 32-bit guests
can ignore the high part entirely. Requiring the guest always write high then
low also works, though I don't see any benefit to either guest or host.
Acting on writes as soon as they occur is not a viable option as it causes the
device to be enabled before the full address has bee written. You could say
the address takes effect after both halves have been written, writes must come
in pairs, but may occur in either order. However there is a risk that host
and guest will somehow get out of sync on which values pair together, so IMO
this is a bad idea.
If you can't stomach the above then I guess changing the condition for ring
enablement to QueueNum != 0 and rearanging the configuration sequence
appropriately would also do the trick. Having this be different between PCI
and mmio is probably not worth the confusion though.
Paul
^ permalink raw reply
* Re: [Qemu-devel] [RFC 0/4] virtio-mmio transport
From: Pawel Moll @ 2011-12-12 14:51 UTC (permalink / raw)
To: Paul Brook
Cc: Peter Maydell, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, Anthony Liguori,
Paolo Bonzini
In-Reply-To: <201112121445.27873.paul@codesourcery.com>
On Mon, 2011-12-12 at 14:45 +0000, Paul Brook wrote:
> I suggest that the device to buffer writes to the high part, and construct the
> actual 64-bit value when the low part is written. That allows 32-bit guests
> can ignore the high part entirely.
This sounds good to me. If we define the reset value of both registers
as 0 (which is consistent with "0 = stop" condition) a 32-bit guest will
be able to behave as you are suggesting.
Cool, I will keep this in mind.
Thanks!
Paweł
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 0000/0004] Drivers: hv: Fixes
From: K. Y. Srinivasan @ 2011-12-12 17:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering; +Cc: K. Y. Srinivasan
Some fixes to the vmbus driver:
1. Fix a memory leak in a failure path.
2. Make vmbus driver unloadable.
3. Get rid of an unnecessary check in hv_init()
in preparation for supporting kexec().
4. Make it possible to build the vmbus driver as part of the
kernel. This patch was submitted earlier, but seems to have been
lost.
Regards,
K. Y
^ permalink raw reply
* [PATCH 1/4] Drivers: hv: Fix a memory leak
From: K. Y. Srinivasan @ 2011-12-12 17:29 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1323710931-29616-1-git-send-email-kys@microsoft.com>
There was a memory leak in a failure path in vmbus_process_offer().
Fix it.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/channel_mgmt.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 12b85ff..b91af50 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -287,6 +287,7 @@ static void vmbus_process_offer(struct work_struct *work)
spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
list_del(&newchannel->listentry);
spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
+ kfree(newchannel->device_obj);
free_channel(newchannel);
} else {
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/4] Drivers: hv: Make the vmbus driver unloadable
From: K. Y. Srinivasan @ 2011-12-12 17:29 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1323710959-29655-1-git-send-email-kys@microsoft.com>
It turns out that the vmbus driver can be made unloadable. Make it
unloadable.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/channel_mgmt.c | 11 +++++++++++
drivers/hv/hv.c | 3 +++
drivers/hv/hyperv_vmbus.h | 1 +
drivers/hv/vmbus_drv.c | 11 +++++++++++
4 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index b91af50..36484db 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -223,6 +223,17 @@ static void vmbus_process_rescind_offer(struct work_struct *work)
vmbus_device_unregister(channel->device_obj);
}
+void vmbus_free_channels(void)
+{
+ struct vmbus_channel *channel;
+
+ list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
+ vmbus_device_unregister(channel->device_obj);
+ kfree(channel->device_obj);
+ free_channel(channel);
+ }
+}
+
/*
* vmbus_process_offer - Process the offer by creating a channel/device
* associated with this offer
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 0fb100e..f8a77d0 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -237,6 +237,9 @@ void hv_cleanup(void)
{
union hv_x64_msr_hypercall_contents hypercall_msr;
+ /* Reset our OS id */
+ wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
+
kfree(hv_context.signal_event_buffer);
hv_context.signal_event_buffer = NULL;
hv_context.signal_event_param = NULL;
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 0aee112..6d7d286 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -611,6 +611,7 @@ void vmbus_device_unregister(struct hv_device *device_obj);
struct vmbus_channel *relid2channel(u32 relid);
+void vmbus_free_channels(void);
/* Connection interface */
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0c048dd..aafee6b 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -779,8 +779,19 @@ cleanup:
return ret;
}
+static void __exit vmbus_exit(void)
+{
+
+ free_irq(irq, hv_acpi_dev);
+ vmbus_free_channels();
+ bus_unregister(&hv_bus);
+ hv_cleanup();
+ acpi_bus_unregister_driver(&vmbus_acpi_driver);
+}
+
MODULE_LICENSE("GPL");
MODULE_VERSION(HV_DRV_VERSION);
module_init(hv_acpi_init);
+module_exit(vmbus_exit);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 3/4] Drivers: hv: Get rid of an unnecessary check in hv.c
From: K. Y. Srinivasan @ 2011-12-12 17:29 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1323710959-29655-1-git-send-email-kys@microsoft.com>
In preparation for eventually supporting kexec in Linux VMs on Hyper-V,
get rid of an unnecessary check in hv_init().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/hv.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index f8a77d0..12aa97f 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -164,11 +164,6 @@ int hv_init(void)
max_leaf = query_hypervisor_info();
- rdmsrl(HV_X64_MSR_GUEST_OS_ID, hv_context.guestid);
-
- if (hv_context.guestid != 0)
- goto cleanup;
-
/* Write our OS info */
wrmsrl(HV_X64_MSR_GUEST_OS_ID, HV_LINUX_GUEST_ID);
hv_context.guestid = HV_LINUX_GUEST_ID;
--
1.7.4.1
^ permalink raw reply related
* [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of the kernel
From: K. Y. Srinivasan @ 2011-12-12 17:29 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1323710959-29655-1-git-send-email-kys@microsoft.com>
Support building the vmbus driver as part of the kernel.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/vmbus_drv.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index aafee6b..1d3d809 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -793,5 +793,5 @@ static void __exit vmbus_exit(void)
MODULE_LICENSE("GPL");
MODULE_VERSION(HV_DRV_VERSION);
-module_init(hv_acpi_init);
+subsys_initcall(hv_acpi_init);
module_exit(vmbus_exit);
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] virtio-mmio: Devices parameter parsing
From: Pawel Moll @ 2011-12-12 17:53 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <877h2ht58w.fsf@rustcorp.com.au>
Hello,
Sorry for the delay - last weeks were a bit hectic for me (and nothing
will change till Friday when I'm off for holiday! :-)
On Thu, 2011-12-01 at 02:06 +0000, Rusty Russell wrote:
> > Yes, this was my initial idea as well. The only problem I faced is the
> > fact that there is no "between levels"... It's easy to add parameters
> > parsing _at_ any particular level, but hard to do this _after_ level A
> > and _before_ level B. The initcalls section simply contains all the
> > calls, ordered by the level - the only "separated" level is the pre-SMP
> > early one. And order within one level is determined by the link order,
> > so I can't guarantee parsing the parameters as the first call of a level
> > (nor as the last call of the previous level).
>
> Yeah, that's why I suggested changing the linker script.
Sounded a bit scary, but turned out to be much less intrusive that I
expected... I'll post the proposal in a second.
> > /* This is the fundamental function for registering boot/module
> > parameters. */
> > -#define __module_param_call(prefix, name, ops, arg, isbool, perm) \
> > +#define __module_param_call(prefix, name, ops, arg, isbool, late, perm) \
> > /* Default value instead of permissions? */ \
> > static int __param_perm_check_##name __attribute__((unused)) = \
> > BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2)) \
>
> Might as well change isbool to "flags", since we have to fix callers
> anyway.
Sure thing, done.
> > diff --git a/init/main.c b/init/main.c
> > index 217ed23..ce89a53 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -407,7 +407,7 @@ static int __init do_early_param(char *param, char *val)
> >
> > void __init parse_early_options(char *cmdline)
> > {
> > - parse_args("early options", cmdline, NULL, 0, do_early_param);
> > + parse_args("early options", cmdline, NULL, 0, 0, 0, do_early_param);
>
> It'd be nice to replace the early param stuff too, but that's probably a
> separate patch. As is getting rid of the old __setup() calls everywhere
> ;)
I promise to look into it next year ;-)
> But so far, it looks good!
Cool, have a look at the patches following this mail then. I hope they
make some sense, however it's unlikely I'll get them ready for 3.3
(unless they already are? ;-) because of my holidays. Maybe at least
they could be linux-next-ed to see what did I break?
Cheers!
Paweł
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 1/2] params: <level>_initcall-like kernel parameters
From: Pawel Moll @ 2011-12-12 17:57 UTC (permalink / raw)
To: linux-kernel, virtualization; +Cc: Pawel Moll
In-Reply-To: <1323712401.2391.93.camel@hornet.cambridge.arm.com>
This patch adds a set of macros that can be used to declare
kernel parameters to be parsed _before_ initcalls at a chosen
level are executed. Such parameters are marked using existing
"flags" field of the "kernel_param" structure.
Linker macro collating init calls had to be modified in order
to add additional symbols between levels that are later used
by the init code to split the calls into blocks.
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
include/asm-generic/vmlinux.lds.h | 35 ++++++++------------
include/linux/moduleparam.h | 59 ++++++++++++++++++++++++++++----
init/main.c | 66 +++++++++++++++++++++++++++++++++---
kernel/module.c | 3 +-
kernel/params.c | 9 ++++-
5 files changed, 135 insertions(+), 37 deletions(-)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b5e2e4c..c4ad0b1 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -615,30 +615,23 @@
*(.init.setup) \
VMLINUX_SYMBOL(__setup_end) = .;
-#define INITCALLS \
- *(.initcallearly.init) \
- VMLINUX_SYMBOL(__early_initcall_end) = .; \
- *(.initcall0.init) \
- *(.initcall0s.init) \
- *(.initcall1.init) \
- *(.initcall1s.init) \
- *(.initcall2.init) \
- *(.initcall2s.init) \
- *(.initcall3.init) \
- *(.initcall3s.init) \
- *(.initcall4.init) \
- *(.initcall4s.init) \
- *(.initcall5.init) \
- *(.initcall5s.init) \
- *(.initcallrootfs.init) \
- *(.initcall6.init) \
- *(.initcall6s.init) \
- *(.initcall7.init) \
- *(.initcall7s.init)
+#define INIT_CALLS_LEVEL(level) \
+ VMLINUX_SYMBOL(__initcall##level##_start) = .; \
+ *(.initcall##level##.init) \
+ *(.initcall##level##s.init) \
#define INIT_CALLS \
VMLINUX_SYMBOL(__initcall_start) = .; \
- INITCALLS \
+ *(.initcallearly.init) \
+ INIT_CALLS_LEVEL(0) \
+ INIT_CALLS_LEVEL(1) \
+ INIT_CALLS_LEVEL(2) \
+ INIT_CALLS_LEVEL(3) \
+ INIT_CALLS_LEVEL(4) \
+ INIT_CALLS_LEVEL(5) \
+ INIT_CALLS_LEVEL(rootfs) \
+ INIT_CALLS_LEVEL(6) \
+ INIT_CALLS_LEVEL(7) \
VMLINUX_SYMBOL(__initcall_end) = .;
#define CON_INITCALL \
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 7939f63..7f4c9b5 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -48,7 +48,14 @@ struct kernel_param_ops {
};
/* Flag bits for kernel_param.flags */
-#define KPARAM_ISBOOL 2
+#define KPARAM_ISBOOL (1 << 1)
+#define KPARAM_ISLEVEL (1 << 2)
+#define KPARAM_LEVEL_SHIFT 3
+#define KPARAM_LEVEL_MASK (7 << KPARAM_LEVEL_SHIFT)
+
+#define __kparam_isbool(arg) (__same_type((arg), bool) ? KPARAM_ISBOOL : 0)
+#define __kparam_isboolp(arg) (__same_type((arg), bool *) ? KPARAM_ISBOOL : 0)
+#define __kparam_level(level) (KPARAM_ISLEVEL | (level) << KPARAM_LEVEL_SHIFT)
struct kernel_param {
const char *name;
@@ -132,7 +139,42 @@ struct kparam_array
*/
#define module_param_cb(name, ops, arg, perm) \
__module_param_call(MODULE_PARAM_PREFIX, \
- name, ops, arg, __same_type((arg), bool *), perm)
+ name, ops, arg, __kparam_isboolp(arg), perm)
+
+/**
+ * <level>_param_cb - general callback for a module/cmdline parameter
+ * to be evaluated before certain initcall level
+ * @name: a valid C identifier which is the parameter name.
+ * @ops: the set & get operations for this parameter.
+ * @perm: visibility in sysfs.
+ *
+ * The ops can have NULL set or get functions.
+ */
+#define __level_param_cb(level, name, ops, arg, perm) \
+ __module_param_call(MODULE_PARAM_PREFIX, \
+ name, ops, arg, \
+ __kparam_isboolp(arg) | __kparam_level(level), perm)
+
+#define core_param_cb(name, ops, arg, perm) \
+ __level_param_cb(1, name, ops, arg, perm)
+
+#define postcore_param_cb(name, ops, arg, perm) \
+ __level_param_cb(2, name, ops, arg, perm)
+
+#define arch_param_cb(name, ops, arg, perm) \
+ __level_param_cb(3, name, ops, arg, perm)
+
+#define subsys_param_cb(name, ops, arg, perm) \
+ __level_param_cb(4, name, ops, arg, perm)
+
+#define fs_param_cb(name, ops, arg, perm) \
+ __level_param_cb(5, name, ops, arg, perm)
+
+#define device_param_cb(name, ops, arg, perm) \
+ __level_param_cb(6, name, ops, arg, perm)
+
+#define late_param_cb(name, ops, arg, perm) \
+ __level_param_cb(7, name, ops, arg, perm)
/* On alpha, ia64 and ppc64 relocations to global data cannot go into
read-only sections (which is part of respective UNIX ABI on these
@@ -146,7 +188,7 @@ struct kparam_array
/* This is the fundamental function for registering boot/module
parameters. */
-#define __module_param_call(prefix, name, ops, arg, isbool, perm) \
+#define __module_param_call(prefix, name, ops, arg, flags, perm) \
/* Default value instead of permissions? */ \
static int __param_perm_check_##name __attribute__((unused)) = \
BUILD_BUG_ON_ZERO((perm) < 0 || (perm) > 0777 || ((perm) & 2)) \
@@ -155,8 +197,7 @@ struct kparam_array
static struct kernel_param __moduleparam_const __param_##name \
__used \
__attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
- = { __param_str_##name, ops, perm, isbool ? KPARAM_ISBOOL : 0, \
- { arg } }
+ = { __param_str_##name, ops, perm, flags, { arg } }
/* Obsolete - use module_param_cb() */
#define module_param_call(name, set, get, arg, perm) \
@@ -164,7 +205,7 @@ struct kparam_array
{ (void *)set, (void *)get }; \
__module_param_call(MODULE_PARAM_PREFIX, \
name, &__param_ops_##name, arg, \
- __same_type(arg, bool *), \
+ __kparam_isboolp(arg), \
(perm) + sizeof(__check_old_set_param(set))*0)
/* We don't get oldget: it's often a new-style param_get_uint, etc. */
@@ -246,7 +287,7 @@ static inline void __kernel_param_unlock(void)
#define core_param(name, var, type, perm) \
param_check_##type(name, &(var)); \
__module_param_call("", name, ¶m_ops_##type, \
- &var, __same_type(var, bool), perm)
+ &var, __kparam_isbool(var), perm)
#endif /* !MODULE */
/**
@@ -292,6 +333,8 @@ extern int parse_args(const char *name,
char *args,
const struct kernel_param *params,
unsigned num,
+ u16 flags_mask,
+ u16 flags,
int (*unknown)(char *param, char *val));
/* Called by module remove. */
@@ -402,7 +445,7 @@ extern int param_get_invbool(char *buffer, const struct kernel_param *kp);
__module_param_call(MODULE_PARAM_PREFIX, name, \
¶m_array_ops, \
.arr = &__param_arr_##name, \
- __same_type(array[0], bool), perm); \
+ __kparam_isbool(array[0]), perm); \
__MODULE_PARM_TYPE(name, "array of " #type)
extern struct kernel_param_ops param_array_ops;
diff --git a/init/main.c b/init/main.c
index 217ed23..a7838c8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -407,7 +407,7 @@ static int __init do_early_param(char *param, char *val)
void __init parse_early_options(char *cmdline)
{
- parse_args("early options", cmdline, NULL, 0, do_early_param);
+ parse_args("early options", cmdline, NULL, 0, 0, 0, do_early_param);
}
/* Arch code calls this early on, or if not, just before other parsing. */
@@ -511,7 +511,7 @@ asmlinkage void __init start_kernel(void)
parse_early_param();
parse_args("Booting kernel", static_command_line, __start___param,
__stop___param - __start___param,
- &unknown_bootoption);
+ KPARAM_ISLEVEL, 0, &unknown_bootoption);
jump_label_init();
@@ -708,16 +708,70 @@ int __init_or_module do_one_initcall(initcall_t fn)
}
-extern initcall_t __initcall_start[], __initcall_end[], __early_initcall_end[];
+extern initcall_t __initcall_start[];
+extern initcall_t __initcall0_start[];
+extern initcall_t __initcall1_start[];
+extern initcall_t __initcall2_start[];
+extern initcall_t __initcall3_start[];
+extern initcall_t __initcall4_start[];
+extern initcall_t __initcall5_start[];
+extern initcall_t __initcall6_start[];
+extern initcall_t __initcall7_start[];
+extern initcall_t __initcall_end[];
+
+static initcall_t *initcall_levels[] __initdata = {
+ __initcall0_start,
+ __initcall1_start,
+ __initcall2_start,
+ __initcall3_start,
+ __initcall4_start,
+ __initcall5_start,
+ __initcall6_start,
+ __initcall7_start,
+ __initcall_end,
+};
+
+static char *initcall_level_names[] __initdata = {
+ "core parameters",
+ "postcore parameters",
+ "arch parameters",
+ "subsys parameters",
+ "fs parameters",
+ "device parameters",
+ "late parameters",
+};
+
+static int __init ignore_unknown_bootoption(char *param, char *val)
+{
+ return 0;
+}
-static void __init do_initcalls(void)
+static void __init do_initcall_level(int level)
{
+ extern const struct kernel_param __start___param[], __stop___param[];
initcall_t *fn;
- for (fn = __early_initcall_end; fn < __initcall_end; fn++)
+ strcpy(static_command_line, saved_command_line);
+ parse_args(initcall_level_names[level],
+ static_command_line, __start___param,
+ __stop___param - __start___param,
+ KPARAM_ISLEVEL | KPARAM_LEVEL_MASK,
+ __kparam_level(level),
+ ignore_unknown_bootoption);
+
+ for (fn = initcall_levels[level]; fn < initcall_levels[level + 1];
+ fn++)
do_one_initcall(*fn);
}
+static void __init do_initcalls(void)
+{
+ int level;
+
+ for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
+ do_initcall_level(level);
+}
+
/*
* Ok, the machine is now initialized. None of the devices
* have been touched yet, but the CPU subsystem is up and
@@ -741,7 +795,7 @@ static void __init do_pre_smp_initcalls(void)
{
initcall_t *fn;
- for (fn = __initcall_start; fn < __early_initcall_end; fn++)
+ for (fn = __initcall_start; fn < __initcall0_start; fn++)
do_one_initcall(*fn);
}
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..66d3e75 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2893,7 +2893,8 @@ static struct module *load_module(void __user *umod,
mutex_unlock(&module_mutex);
/* Module is ready to execute: parsing args may do that. */
- err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
+ err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
+ 0, 0, NULL);
if (err < 0)
goto unlink;
diff --git a/kernel/params.c b/kernel/params.c
index 65aae11..ddde6a7 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -94,6 +94,8 @@ static int parse_one(char *param,
char *val,
const struct kernel_param *params,
unsigned num_params,
+ int flags_mask,
+ int flags,
int (*handle_unknown)(char *param, char *val))
{
unsigned int i;
@@ -102,6 +104,8 @@ static int parse_one(char *param,
/* Find parameter */
for (i = 0; i < num_params; i++) {
if (parameq(param, params[i].name)) {
+ if ((params[i].flags & flags_mask) != flags)
+ return 0;
/* No one handled NULL, so do it here. */
if (!val && params[i].ops->set != param_set_bool)
return -EINVAL;
@@ -180,6 +184,8 @@ int parse_args(const char *name,
char *args,
const struct kernel_param *params,
unsigned num,
+ u16 flags_mask,
+ u16 flags,
int (*unknown)(char *param, char *val))
{
char *param, *val;
@@ -195,7 +201,8 @@ int parse_args(const char *name,
args = next_arg(args, ¶m, &val);
irq_was_disabled = irqs_disabled();
- ret = parse_one(param, val, params, num, unknown);
+ ret = parse_one(param, val, params, num,
+ flags_mask, flags, unknown);
if (irq_was_disabled && !irqs_disabled()) {
printk(KERN_WARNING "parse_args(): option '%s' enabled "
"irq's!\n", param);
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/2] virtio-mmio: Devices parameter parsing
From: Pawel Moll @ 2011-12-12 17:57 UTC (permalink / raw)
To: linux-kernel, virtualization; +Cc: Pawel Moll
In-Reply-To: <1323712627-17353-1-git-send-email-pawel.moll@arm.com>
This patch adds an option to instantiate guest virtio-mmio devices
basing on a kernel command line (or module) parameter, for example:
virtio_mmio.devices=0x100@0x100b0000:48
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
Documentation/kernel-parameters.txt | 17 ++++
drivers/virtio/Kconfig | 11 +++
drivers/virtio/virtio_mmio.c | 163 +++++++++++++++++++++++++++++++++++
3 files changed, 191 insertions(+), 0 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a0c5c5f..c155d13 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -110,6 +110,7 @@ parameter is applicable:
USB USB support is enabled.
USBHID USB Human Interface Device support is enabled.
V4L Video For Linux support is enabled.
+ VMMIO Driver for memory mapped virtio devices is enabled.
VGA The VGA console has been enabled.
VT Virtual terminal support is enabled.
WDT Watchdog support is enabled.
@@ -2720,6 +2721,22 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
video= [FB] Frame buffer configuration
See Documentation/fb/modedb.txt.
+ virtio_mmio.device=
+ [VMMIO] Memory mapped virtio (platform) device.
+
+ <size>@<baseaddr>:<irq>[:<id>]
+ where:
+ <size> := size (can use standard suffixes
+ like K, M and G)
+ <baseaddr> := physical base address
+ <irq> := interrupt number (as passed to
+ request_irq())
+ <id> := (optional) platform device id
+ example:
+ virtio_mmio.device=1K@0x100b0000:48:7
+
+ Can be used multiple times for multiple devices.
+
vga= [BOOT,X86-32] Select a particular video mode
See Documentation/x86/boot.txt and
Documentation/svga.txt.
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 1a61939..f38b17a 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -46,4 +46,15 @@ config VIRTIO_BALLOON
If unsure, say N.
+config VIRTIO_MMIO_CMDLINE_DEVICES
+ bool "Memory mapped virtio devices parameter parsing"
+ depends on VIRTIO_MMIO
+ ---help---
+ Allow virtio-mmio devices instantiation via the kernel command line
+ or module parameters. Be aware that using incorrect parameters (base
+ address in particular) can crash your system - you have been warned.
+ See Documentation/kernel-parameters.txt for details.
+
+ If unsure, say 'N'.
+
endmenu
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 7317dc2..eab0007 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -6,6 +6,50 @@
* This module allows virtio devices to be used over a virtual, memory mapped
* platform device.
*
+ * The guest device(s) may be instantiated in one of three equivalent ways:
+ *
+ * 1. Static platform device in board's code, eg.:
+ *
+ * static struct platform_device v2m_virtio_device = {
+ * .name = "virtio-mmio",
+ * .id = -1,
+ * .num_resources = 2,
+ * .resource = (struct resource []) {
+ * {
+ * .start = 0x1001e000,
+ * .end = 0x1001e0ff,
+ * .flags = IORESOURCE_MEM,
+ * }, {
+ * .start = 42 + 32,
+ * .end = 42 + 32,
+ * .flags = IORESOURCE_IRQ,
+ * },
+ * }
+ * };
+ *
+ * 2. Device Tree node, eg.:
+ *
+ * virtio_block@1e000 {
+ * compatible = "virtio,mmio";
+ * reg = <0x1e000 0x100>;
+ * interrupts = <42>;
+ * }
+ *
+ * 3. Kernel module (or command line) parameter. Can be used more than once -
+ * one device will be created for each one. Syntax:
+ *
+ * [virtio_mmio.]device=<size>@<baseaddr>:<irq>[:<id>]
+ * where:
+ * <size> := size (can use standard suffixes like K, M or G)
+ * <baseaddr> := physical base address
+ * <irq> := interrupt number (as passed to request_irq())
+ * <id> := (optional) platform device id
+ * eg.:
+ * virtio_mmio.device=0x100@0x100b0000:48 \
+ * virtio_mmio.device=1K@0x1001e000:74
+ *
+ *
+ *
* Registers layout (all 32-bit wide):
*
* offset d. name description
@@ -42,6 +86,8 @@
* See the COPYING file in the top-level directory.
*/
+#define pr_fmt(fmt) "virtio-mmio: " fmt
+
#include <linux/highmem.h>
#include <linux/interrupt.h>
#include <linux/io.h>
@@ -443,6 +489,122 @@ static int __devexit virtio_mmio_remove(struct platform_device *pdev)
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+
+static struct device vm_cmdline_parent = {
+ .init_name = "virtio-mmio-cmdline",
+};
+
+static int vm_cmdline_parent_registered;
+static int vm_cmdline_id;
+
+static int vm_cmdline_set(const char *device,
+ const struct kernel_param *kp)
+{
+ int err;
+ struct resource resources[2] = {};
+ char *str;
+ long long int base;
+ int processed, consumed = 0;
+ struct platform_device *pdev;
+
+ resources[0].flags = IORESOURCE_MEM;
+ resources[1].flags = IORESOURCE_IRQ;
+
+ resources[0].end = memparse(device, &str) - 1;
+
+ processed = sscanf(str, "@%lli:%u%n:%d%n",
+ &base, &resources[1].start, &consumed,
+ &vm_cmdline_id, &consumed);
+
+ if (processed < 2 || processed > 3 || str[consumed])
+ return -EINVAL;
+
+ resources[0].start = base;
+ resources[0].end += base;
+ resources[1].end = resources[1].start;
+
+ if (!vm_cmdline_parent_registered) {
+ err = device_register(&vm_cmdline_parent);
+ if (err) {
+ pr_err("Failed to register parent device!\n");
+ return err;
+ }
+ vm_cmdline_parent_registered = 1;
+ }
+
+ pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
+ vm_cmdline_id++,
+ (unsigned long long)resources[0].start,
+ (unsigned long long)resources[0].end,
+ (int)resources[1].start);
+
+ pdev = platform_device_register_resndata(&vm_cmdline_parent,
+ "virtio-mmio", vm_cmdline_id++,
+ resources, ARRAY_SIZE(resources), NULL, 0);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
+
+ return 0;
+}
+
+static int vm_cmdline_get_device(struct device *dev, void *data)
+{
+ char *buffer = data;
+ unsigned int len = strlen(buffer);
+ struct platform_device *pdev = to_platform_device(dev);
+
+ snprintf(buffer + len, PAGE_SIZE - len, "0x%llx@0x%llx:%llu:%d\n",
+ pdev->resource[0].end - pdev->resource[0].start + 1ULL,
+ (unsigned long long)pdev->resource[0].start,
+ (unsigned long long)pdev->resource[1].start,
+ pdev->id);
+ return 0;
+}
+
+static int vm_cmdline_get(char *buffer, const struct kernel_param *kp)
+{
+ buffer[0] = '\0';
+ device_for_each_child(&vm_cmdline_parent, buffer,
+ vm_cmdline_get_device);
+ return strlen(buffer) + 1;
+}
+
+static struct kernel_param_ops vm_cmdline_param_ops = {
+ .set = vm_cmdline_set,
+ .get = vm_cmdline_get,
+};
+
+device_param_cb(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
+
+static int vm_unregister_cmdline_device(struct device *dev,
+ void *data)
+{
+ platform_device_unregister(to_platform_device(dev));
+
+ return 0;
+}
+
+static void vm_unregister_cmdline_devices(void)
+{
+ if (vm_cmdline_parent_registered) {
+ device_for_each_child(&vm_cmdline_parent, NULL,
+ vm_unregister_cmdline_device);
+ device_unregister(&vm_cmdline_parent);
+ vm_cmdline_parent_registered = 0;
+ }
+}
+
+#else
+
+static void vm_unregister_cmdline_devices(void)
+{
+}
+
+#endif
+
/* Platform driver */
static struct of_device_id virtio_mmio_match[] = {
@@ -469,6 +631,7 @@ static int __init virtio_mmio_init(void)
static void __exit virtio_mmio_exit(void)
{
platform_driver_unregister(&virtio_mmio_driver);
+ vm_unregister_cmdline_devices();
}
module_init(virtio_mmio_init);
--
1.7.5.4
^ permalink raw reply related
* Re: [RFC 7/11] virtio_pci: new, capability-aware driver.
From: Don Dutile @ 2011-12-12 18:10 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Sasha Levin, virtualization
In-Reply-To: <20111212114912.GA7946@redhat.com>
On 12/12/2011 06:49 AM, Michael S. Tsirkin wrote:
> On Mon, Dec 12, 2011 at 09:15:03AM +1030, Rusty Russell wrote:
>> On Sun, 11 Dec 2011 11:42:56 +0200, "Michael S. Tsirkin"<mst@redhat.com> wrote:
>>> On Thu, Dec 08, 2011 at 09:09:33PM +1030, Rusty Russell wrote:
>>>> +/* There is no iowrite64. We use two 32-bit ops. */
>>>> +static void iowrite64(u64 val, const __le64 *addr)
>>>> +{
>>>> + iowrite32((u32)val, (__le32 *)addr);
>>>> + iowrite32(val>> 32, (__le32 *)addr + 1);
>>>> +}
>>>> +
>>>
>>> Let's put addr_lo/addr_hi in the structure then,
>>> to make the fact this field is not atomic explicit?
>>
>> Good point, assuming I haven't missed something.
>>
>> Are 64-bit accesses actually unknown in PCI-land? Or is this a limited
>> availability thing?
>>
>> Thanks,
>> Rusty.
>
> I think PCI can optionally support atomic 64 bit accesses, but not all
> architectures can generate them.
>
yes. PCI(e) support atomic 64-bit ops; it's dependent on CPU & chipset interface
to PCI that determines ability to generate a 64-bit length xaction.
^ permalink raw reply
* Re: [RFC 7/11] virtio_pci: new, capability-aware driver.
From: Michael S. Tsirkin @ 2011-12-12 18:25 UTC (permalink / raw)
To: Rusty Russell; +Cc: Sasha Levin, virtualization
In-Reply-To: <87boreohhs.fsf@rustcorp.com.au>
On Mon, Dec 12, 2011 at 09:15:03AM +1030, Rusty Russell wrote:
> On Sun, 11 Dec 2011 11:42:56 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Dec 08, 2011 at 09:09:33PM +1030, Rusty Russell wrote:
> > > +/* There is no iowrite64. We use two 32-bit ops. */
> > > +static void iowrite64(u64 val, const __le64 *addr)
> > > +{
> > > + iowrite32((u32)val, (__le32 *)addr);
> > > + iowrite32(val >> 32, (__le32 *)addr + 1);
> > > +}
> > > +
> >
> > Let's put addr_lo/addr_hi in the structure then,
> > to make the fact this field is not atomic explicit?
>
> Good point, assuming I haven't missed something.
>
> Are 64-bit accesses actually unknown in PCI-land? Or is this a limited
> availability thing?
>
> Thanks,
> Rusty.
By the way, a generic question on virtio-pci: we now have:
/* virtio config->get() implementation */
static void vp_get(struct virtio_device *vdev, unsigned offset,
void *buf, unsigned len)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
void __iomem *ioaddr = vp_dev->ioaddr +
VIRTIO_PCI_CONFIG(vp_dev) + offset;
u8 *ptr = buf;
int i;
for (i = 0; i < len; i++)
ptr[i] = ioread8(ioaddr + i);
}
This means that if configuration is read while
it is changed, we might get an inconsistent state,
with parts of a 64 bit field coming from old
and parts from new value.
Isn't this a problem?
--
MST
^ permalink raw reply
* Re: [PATCH v3 2/3] hvc_init(): Enforce one-time initialization.
From: Miche Baker-Harvey @ 2011-12-12 19:11 UTC (permalink / raw)
To: Amit Shah
Cc: Stephen Rothwell, xen-devel, Konrad Rzeszutek Wilk,
Benjamin Herrenschmidt, linux-kernel, virtualization,
Anton Blanchard, Mike Waychison, ppc-dev, Greg Kroah-Hartman,
Eric Northrup
In-Reply-To: <20111208120804.GF23531@amit-x200.redhat.com>
So on a CONSOLE_PORT_ADD message, we would take the
(existing)ports_device::ports_lock, and for other control messages we
would justtake the (new) port::port_lock? You are concerned that just
takingthe ports_lock for all control messages could be too
restrictive? Iwouldn't have expected these messages to be frequent
occurrences, butI'll defer to your experience here.
The CONSOLE_CONSOLE_PORT message calls hvc_alloc, which also
needsserialization. That's in another one of these three patches; are
youthinking we could leave that patch be, or that we would we use
theport_lock for CONSOLE_CONSOLE_PORT? Using the port_lock
wouldprovide the HVC serialization "for free" but it would be cleaner
if weput HVC related synchronization in hvc_console.c.
On Thu, Dec 8, 2011 at 4:08 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Tue) 06 Dec 2011 [09:05:38], Miche Baker-Harvey wrote:
>> Amit,
>>
>> Ah, indeed. I am not using MSI-X, so virtio_pci::vp_try_to_find_vqs()
>> calls vp_request_intx() and sets up an interrupt callback. From
>> there, when an interrupt occurs, the stack looks something like this:
>>
>> virtio_pci::vp_interrupt()
>> virtio_pci::vp_vring_interrupt()
>> virtio_ring::vring_interrupt()
>> vq->vq.callback() <-- in this case, that's virtio_console::control_intr()
>> workqueue::schedule_work()
>> workqueue::queue_work()
>> queue_work_on(get_cpu()) <-- queues the work on the current CPU.
>>
>> I'm not doing anything to keep multiple control message from being
>> sent concurrently to the guest, and we will take those interrupts on
>> any CPU. I've confirmed that the two instances of
>> handle_control_message() are occurring on different CPUs.
>
> So let's have a new helper, port_lock() that takes the port-specific
> spinlock. There has to be a new helper, since the port lock should
> depend on the portdev lock being taken too. For the port addition
> case, just the portdev lock should be taken. For any other
> operations, the port lock should be taken.
>
> My assumption was that we would be able to serialise the work items,
> but that will be too restrictive. Taking port locks sounds like a
> better idea.
>
> We'd definitely need the port lock in the control work handler. We
> might need it in a few more places (like module removal), but we'll
> worry about that later.
>
> Does this sound fine?
>
> Amit
^ permalink raw reply
* Re: [PATCH v3 2/3] hvc_init(): Enforce one-time initialization.
From: Amit Shah @ 2011-12-12 19:25 UTC (permalink / raw)
To: Miche Baker-Harvey
Cc: Stephen Rothwell, xen-devel, Konrad Rzeszutek Wilk,
Benjamin Herrenschmidt, linux-kernel, virtualization,
Anton Blanchard, Mike Waychison, ppc-dev, Greg Kroah-Hartman,
Eric Northrup
In-Reply-To: <CAB8RdapDDV0kYW_uASpBoJ3hbx6zScKnG+LWWcjYtqMnqXN4Xw@mail.gmail.com>
On (Mon) 12 Dec 2011 [11:11:55], Miche Baker-Harvey wrote:
> So on a CONSOLE_PORT_ADD message, we would take the
> (existing)ports_device::ports_lock, and for other control messages we
> would justtake the (new) port::port_lock? You are concerned that just
> takingthe ports_lock for all control messages could be too
> restrictive? Iwouldn't have expected these messages to be frequent
> occurrences, butI'll defer to your experience here.
No, I mean we'll have to take the new port_lock() everywhere we
currently take the port lock, plus in a few more places. I only
suggest using port_lock() helper since we'll need a dependency on the
portdev lock as well.
> The CONSOLE_CONSOLE_PORT message calls hvc_alloc, which also
> needsserialization. That's in another one of these three patches; are
> youthinking we could leave that patch be, or that we would we use
> theport_lock for CONSOLE_CONSOLE_PORT? Using the port_lock
> wouldprovide the HVC serialization "for free" but it would be cleaner
> if weput HVC related synchronization in hvc_console.c.
Yes, definitely, since other users of hvc_console may get bitten in
similar ways. However, I'm not too familiar with the hvc code, the
people at linux-ppc can be of help.
> On Thu, Dec 8, 2011 at 4:08 AM, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Tue) 06 Dec 2011 [09:05:38], Miche Baker-Harvey wrote:
> >> Amit,
> >>
> >> Ah, indeed. I am not using MSI-X, so virtio_pci::vp_try_to_find_vqs()
> >> calls vp_request_intx() and sets up an interrupt callback. From
> >> there, when an interrupt occurs, the stack looks something like this:
> >>
> >> virtio_pci::vp_interrupt()
> >> virtio_pci::vp_vring_interrupt()
> >> virtio_ring::vring_interrupt()
> >> vq->vq.callback() <-- in this case, that's virtio_console::control_intr()
> >> workqueue::schedule_work()
> >> workqueue::queue_work()
> >> queue_work_on(get_cpu()) <-- queues the work on the current CPU.
> >>
> >> I'm not doing anything to keep multiple control message from being
> >> sent concurrently to the guest, and we will take those interrupts on
> >> any CPU. I've confirmed that the two instances of
> >> handle_control_message() are occurring on different CPUs.
> >
> > So let's have a new helper, port_lock() that takes the port-specific
> > spinlock. There has to be a new helper, since the port lock should
> > depend on the portdev lock being taken too. For the port addition
> > case, just the portdev lock should be taken. For any other
> > operations, the port lock should be taken.
> >
> > My assumption was that we would be able to serialise the work items,
> > but that will be too restrictive. Taking port locks sounds like a
> > better idea.
> >
> > We'd definitely need the port lock in the control work handler. We
> > might need it in a few more places (like module removal), but we'll
> > worry about that later.
> >
> > Does this sound fine?
> >
> > Amit
Amit
^ permalink raw reply
* Re: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of the kernel
From: Greg KH @ 2011-12-12 22:27 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering,
Haiyang Zhang
In-Reply-To: <1323710959-29655-4-git-send-email-kys@microsoft.com>
On Mon, Dec 12, 2011 at 09:29:19AM -0800, K. Y. Srinivasan wrote:
> Support building the vmbus driver as part of the kernel.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
This was already applied to my tree, I don't know why you missed it...
greg k-h
^ permalink raw reply
* Re: [PATCH 0000/0004] Drivers: hv: Fixes
From: Greg KH @ 2011-12-12 22:28 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, linux-kernel, devel, virtualization, ohering
In-Reply-To: <1323710931-29616-1-git-send-email-kys@microsoft.com>
On Mon, Dec 12, 2011 at 09:28:51AM -0800, K. Y. Srinivasan wrote:
> Some fixes to the vmbus driver:
>
> 1. Fix a memory leak in a failure path.
>
> 2. Make vmbus driver unloadable.
>
> 3. Get rid of an unnecessary check in hv_init()
> in preparation for supporting kexec().
>
> 4. Make it possible to build the vmbus driver as part of the
> kernel. This patch was submitted earlier, but seems to have been
> lost.
No, it was not lost, it was applied to my driver-core-next tree a while
ago. Because of this, I had to hand-edit patch 2 above and skip patch 4
here.
greg k-h
^ permalink raw reply
* RE: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of the kernel
From: KY Srinivasan @ 2011-12-12 23:44 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Haiyang Zhang
In-Reply-To: <20111212222712.GA13510@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Monday, December 12, 2011 5:27 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Haiyang Zhang
> Subject: Re: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of
> the kernel
>
> On Mon, Dec 12, 2011 at 09:29:19AM -0800, K. Y. Srinivasan wrote:
> > Support building the vmbus driver as part of the kernel.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> This was already applied to my tree, I don't know why you missed it...
Thanks Greg. I pulled down your tree yesterday (staging.git) and checked out
the staging-next branch and did not find the fix needed to build this driver
as part of the kernel. I will check again.
Thanks,
K. Y
^ permalink raw reply
* Re: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of the kernel
From: Greg KH @ 2011-12-12 23:49 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Haiyang Zhang
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AD7FDDF@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Mon, Dec 12, 2011 at 11:44:30PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Monday, December 12, 2011 5:27 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > Haiyang Zhang
> > Subject: Re: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of
> > the kernel
> >
> > On Mon, Dec 12, 2011 at 09:29:19AM -0800, K. Y. Srinivasan wrote:
> > > Support building the vmbus driver as part of the kernel.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > This was already applied to my tree, I don't know why you missed it...
>
> Thanks Greg. I pulled down your tree yesterday (staging.git) and checked out
> the staging-next branch and did not find the fix needed to build this driver
> as part of the kernel. I will check again.
That is because I'm not applying these patches to the staging.git tree,
as the commit messages you get show...
^ permalink raw reply
* RE: [PATCH 0000/0004] Drivers: hv: Fixes
From: KY Srinivasan @ 2011-12-12 23:50 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com
In-Reply-To: <20111212222801.GA16054@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Monday, December 12, 2011 5:28 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com
> Subject: Re: [PATCH 0000/0004] Drivers: hv: Fixes
>
> On Mon, Dec 12, 2011 at 09:28:51AM -0800, K. Y. Srinivasan wrote:
> > Some fixes to the vmbus driver:
> >
> > 1. Fix a memory leak in a failure path.
> >
> > 2. Make vmbus driver unloadable.
> >
> > 3. Get rid of an unnecessary check in hv_init()
> > in preparation for supporting kexec().
> >
> > 4. Make it possible to build the vmbus driver as part of the
> > kernel. This patch was submitted earlier, but seems to have been
> > lost.
>
> No, it was not lost, it was applied to my driver-core-next tree a while
> ago. Because of this, I had to hand-edit patch 2 above and skip patch 4
> here.
Thanks Greg. I will check to see why I got some stale bits yesterday.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of the kernel
From: KY Srinivasan @ 2011-12-12 23:54 UTC (permalink / raw)
To: Greg KH
Cc: Greg KH, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Haiyang Zhang
In-Reply-To: <20111212234918.GA20625@suse.de>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Monday, December 12, 2011 6:49 PM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; ohering@suse.com; Haiyang Zhang
> Subject: Re: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as part of
> the kernel
>
> On Mon, Dec 12, 2011 at 11:44:30PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Monday, December 12, 2011 5:27 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > Haiyang Zhang
> > > Subject: Re: [PATCH 4/4] Drivers: hv: Support building the vmbus driver as
> part of
> > > the kernel
> > >
> > > On Mon, Dec 12, 2011 at 09:29:19AM -0800, K. Y. Srinivasan wrote:
> > > > Support building the vmbus driver as part of the kernel.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > >
> > > This was already applied to my tree, I don't know why you missed it...
> >
> > Thanks Greg. I pulled down your tree yesterday (staging.git) and checked out
> > the staging-next branch and did not find the fix needed to build this driver
> > as part of the kernel. I will check again.
>
> That is because I'm not applying these patches to the staging.git tree,
> as the commit messages you get show...
Oops! Thanks Greg.
Regards,
K. Y
^ permalink raw reply
* Re: [RFC] virtio: use mandatory barriers for remote processor vdevs
From: Amos Kong @ 2011-12-12 23:56 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, Benjamin Herrenschmidt, linux-kernel,
virtualization, linux-arm-kernel
In-Reply-To: <87wra2tlue.fsf@rustcorp.com.au>
On 12/12/2011 01:12 PM, Rusty Russell wrote:
> On Mon, 12 Dec 2011 11:06:53 +0800, Amos Kong <akong@redhat.com> wrote:
>> On 12/12/11 06:27, Benjamin Herrenschmidt wrote:
>>> On Sun, 2011-12-11 at 14:25 +0200, Michael S. Tsirkin wrote:
>>>
>>>> Forwarding some results by Amos, who run multiple netperf streams in
>>>> parallel, from an external box to the guest. TCP_STREAM results were
>>>> noisy. This could be due to buffering done by TCP, where packet size
>>>> varies even as message size is constant.
>>>>
>>>> TCP_RR results were consistent. In this benchmark, after switching
>>>> to mandatory barriers, CPU utilization increased by up to 35% while
>>>> throughput went down by up to 14%. the normalized throughput/cpu
>>>> regressed consistently, between 7 and 35%
>>>>
>>>> The "fix" applied was simply this:
>>>
>>> What machine& processor was this ?
>>
>> pined guest memory to numa node 1
>
> Please try this patch. How much does the branch cost us?
Ok, I will provide the result later.
Thanks,
Amos
> (Compiles, untested).
>
> Thanks,
> Rusty.
>
> From: Rusty Russell <rusty@rustcorp.com.au>
> Subject: virtio: harsher barriers for virtio-mmio.
>
> We were cheating with our barriers; using the smp ones rather than the
> real device ones. That was fine, until virtio-mmio came along, which
> could be talking to a real device (a non-SMP CPU).
>
> Unfortunately, just putting back the real barriers (reverting
> d57ed95d) causes a performance regression on virtio-pci. In
> particular, Amos reports netbench's TCP_RR over virtio_net CPU
> utilization increased up to 35% while throughput went down by up to
> 14%.
>
> By comparison, this branch costs us???
>
> Reference: https://lkml.org/lkml/2011/12/11/22
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
> drivers/lguest/lguest_device.c | 10 ++++++----
> drivers/s390/kvm/kvm_virtio.c | 2 +-
> drivers/virtio/virtio_mmio.c | 7 ++++---
> drivers/virtio/virtio_pci.c | 4 ++--
> drivers/virtio/virtio_ring.c | 34 +++++++++++++++++++++-------------
> include/linux/virtio_ring.h | 1 +
> tools/virtio/linux/virtio.h | 1 +
> tools/virtio/virtio_test.c | 3 ++-
> 8 files changed, 38 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
> --- a/drivers/lguest/lguest_device.c
> +++ b/drivers/lguest/lguest_device.c
> @@ -291,11 +291,13 @@ static struct virtqueue *lg_find_vq(stru
> }
>
> /*
> - * OK, tell virtio_ring.c to set up a virtqueue now we know its size
> - * and we've got a pointer to its pages.
> + * OK, tell virtio_ring.c to set up a virtqueue now we know its size
> + * and we've got a pointer to its pages. Note that we set weak_barriers
> + * to 'true': the host just a(nother) SMP CPU, so we only need inter-cpu
> + * barriers.
> */
> - vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN,
> - vdev, lvq->pages, lg_notify, callback, name);
> + vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN, vdev,
> + true, lvq->pages, lg_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> goto unmap;
> diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
> --- a/drivers/s390/kvm/kvm_virtio.c
> +++ b/drivers/s390/kvm/kvm_virtio.c
> @@ -198,7 +198,7 @@ static struct virtqueue *kvm_find_vq(str
> goto out;
>
> vq = vring_new_virtqueue(config->num, KVM_S390_VIRTIO_RING_ALIGN,
> - vdev, (void *) config->address,
> + vdev, true, (void *) config->address,
> kvm_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -309,9 +309,10 @@ static struct virtqueue *vm_setup_vq(str
> writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
> vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
>
> - /* Create the vring */
> - vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN,
> - vdev, info->queue, vm_notify, callback, name);
> + /* Create the vring: no weak barriers, the other side is could
> + * be an independent "device". */
> + vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
> + false, info->queue, vm_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> goto error_new_virtqueue;
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -414,8 +414,8 @@ static struct virtqueue *setup_vq(struct
> vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
>
> /* create the vring */
> - vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN,
> - vdev, info->queue, vp_notify, callback, name);
> + vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
> + true, info->queue, vp_notify, callback, name);
> if (!vq) {
> err = -ENOMEM;
> goto out_activate_queue;
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -28,17 +28,20 @@
> #ifdef CONFIG_SMP
> /* Where possible, use SMP barriers which are more lightweight than mandatory
> * barriers, because mandatory barriers control MMIO effects on accesses
> - * through relaxed memory I/O windows (which virtio does not use). */
> -#define virtio_mb() smp_mb()
> -#define virtio_rmb() smp_rmb()
> -#define virtio_wmb() smp_wmb()
> + * through relaxed memory I/O windows (which virtio-pci does not use). */
> +#define virtio_mb(vq) \
> + do { if ((vq)->weak_barriers) smp_mb(); else mb(); } while(0)
> +#define virtio_rmb(vq) \
> + do { if ((vq)->weak_barriers) smp_rmb(); else rmb(); } while(0)
> +#define virtio_wmb(vq) \
> + do { if ((vq)->weak_barriers) smp_rmb(); else rmb(); } while(0)
> #else
> /* We must force memory ordering even if guest is UP since host could be
> * running on another CPU, but SMP barriers are defined to barrier() in that
> * configuration. So fall back to mandatory barriers instead. */
> -#define virtio_mb() mb()
> -#define virtio_rmb() rmb()
> -#define virtio_wmb() wmb()
> +#define virtio_mb(vq) mb()
> +#define virtio_rmb(vq) rmb()
> +#define virtio_wmb(vq) wmb()
> #endif
>
> #ifdef DEBUG
> @@ -77,6 +80,9 @@ struct vring_virtqueue
> /* Actual memory layout for this queue */
> struct vring vring;
>
> + /* Can we use weak barriers? */
> + bool weak_barriers;
> +
> /* Other side has made a mess, don't try any more. */
> bool broken;
>
> @@ -245,14 +251,14 @@ void virtqueue_kick(struct virtqueue *_v
> START_USE(vq);
> /* Descriptors and available array need to be set before we expose the
> * new available array entries. */
> - virtio_wmb();
> + virtio_wmb(vq);
>
> old = vq->vring.avail->idx;
> new = vq->vring.avail->idx = old + vq->num_added;
> vq->num_added = 0;
>
> /* Need to update avail index before checking if we should notify */
> - virtio_mb();
> + virtio_mb(vq);
>
> if (vq->event ?
> vring_need_event(vring_avail_event(&vq->vring), new, old) :
> @@ -314,7 +320,7 @@ void *virtqueue_get_buf(struct virtqueue
> }
>
> /* Only get used array entries after they have been exposed by host. */
> - virtio_rmb();
> + virtio_rmb(vq);
>
> i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
> *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
> @@ -337,7 +343,7 @@ void *virtqueue_get_buf(struct virtqueue
> * the read in the next get_buf call. */
> if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
> vring_used_event(&vq->vring) = vq->last_used_idx;
> - virtio_mb();
> + virtio_mb(vq);
> }
>
> END_USE(vq);
> @@ -366,7 +372,7 @@ bool virtqueue_enable_cb(struct virtqueu
> * entry. Always do both to keep code simple. */
> vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
> vring_used_event(&vq->vring) = vq->last_used_idx;
> - virtio_mb();
> + virtio_mb(vq);
> if (unlikely(more_used(vq))) {
> END_USE(vq);
> return false;
> @@ -393,7 +399,7 @@ bool virtqueue_enable_cb_delayed(struct
> /* TODO: tune this threshold */
> bufs = (u16)(vq->vring.avail->idx - vq->last_used_idx) * 3 / 4;
> vring_used_event(&vq->vring) = vq->last_used_idx + bufs;
> - virtio_mb();
> + virtio_mb(vq);
> if (unlikely((u16)(vq->vring.used->idx - vq->last_used_idx) > bufs)) {
> END_USE(vq);
> return false;
> @@ -453,6 +459,7 @@ EXPORT_SYMBOL_GPL(vring_interrupt);
> struct virtqueue *vring_new_virtqueue(unsigned int num,
> unsigned int vring_align,
> struct virtio_device *vdev,
> + bool weak_barriers,
> void *pages,
> void (*notify)(struct virtqueue *),
> void (*callback)(struct virtqueue *),
> @@ -476,6 +483,7 @@ struct virtqueue *vring_new_virtqueue(un
> vq->vq.vdev = vdev;
> vq->vq.name = name;
> vq->notify = notify;
> + vq->weak_barriers = weak_barriers;
> vq->broken = false;
> vq->last_used_idx = 0;
> vq->num_added = 0;
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -168,6 +168,7 @@ struct virtqueue;
> struct virtqueue *vring_new_virtqueue(unsigned int num,
> unsigned int vring_align,
> struct virtio_device *vdev,
> + bool weak_barriers,
> void *pages,
> void (*notify)(struct virtqueue *vq),
> void (*callback)(struct virtqueue *vq),
> diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
> --- a/tools/virtio/linux/virtio.h
> +++ b/tools/virtio/linux/virtio.h
> @@ -214,6 +214,7 @@ void *virtqueue_detach_unused_buf(struct
> struct virtqueue *vring_new_virtqueue(unsigned int num,
> unsigned int vring_align,
> struct virtio_device *vdev,
> + bool weak_barriers,
> void *pages,
> void (*notify)(struct virtqueue *vq),
> void (*callback)(struct virtqueue *vq),
> diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
> --- a/tools/virtio/virtio_test.c
> +++ b/tools/virtio/virtio_test.c
> @@ -92,7 +92,8 @@ static void vq_info_add(struct vdev_info
> assert(r >= 0);
> memset(info->ring, 0, vring_size(num, 4096));
> vring_init(&info->vring, num, info->ring, 4096);
> - info->vq = vring_new_virtqueue(info->vring.num, 4096, &dev->vdev, info->ring,
> + info->vq = vring_new_virtqueue(info->vring.num, 4096, &dev->vdev,
> + true, info->ring,
> vq_notify, vq_callback, "test");
> assert(info->vq);
> info->vq->priv = info;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 7/11] virtio_pci: new, capability-aware driver.
From: Rusty Russell @ 2011-12-13 2:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Sasha Levin, virtualization
In-Reply-To: <20111212182533.GB25916@redhat.com>
On Mon, 12 Dec 2011 20:25:34 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> By the way, a generic question on virtio-pci: we now have:
>
> /* virtio config->get() implementation */
> static void vp_get(struct virtio_device *vdev, unsigned offset,
> void *buf, unsigned len)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> void __iomem *ioaddr = vp_dev->ioaddr +
> VIRTIO_PCI_CONFIG(vp_dev) + offset;
> u8 *ptr = buf;
> int i;
>
> for (i = 0; i < len; i++)
> ptr[i] = ioread8(ioaddr + i);
> }
>
> This means that if configuration is read while
> it is changed, we might get an inconsistent state,
> with parts of a 64 bit field coming from old
> and parts from new value.
>
> Isn't this a problem?
I don't think so; it's the caller's problem if they need to do locking.
Is there a caller which needs this?
Or am I missing something?
Rusty.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox