Re: [PATCH v2] virtio-mmio: Update the device to OASIS spec version
On Tue, 2015-04-28 at 22:06 +0100, Christopher Covington wrote:
Hi,
On 01/20/2015 01:12 PM, Pawel Moll wrote:
@@ -356,13 +346,6 @@ static struct virtqueue *vm_setup_vq(struct
virtio_device *vdev, unsigned index,
info-num /= 2;
}
- /* Activate the queue */
- writel(info-num, vm_dev-base + VIRTIO_MMIO_QUEUE_NUM);
- writel(VIRTIO_MMIO_VRING_ALIGN,
- vm_dev-base + VIRTIO_MMIO_QUEUE_ALIGN);
- writel(virt_to_phys(info-queue) PAGE_SHIFT,
- vm_dev-base + VIRTIO_MMIO_QUEUE_PFN);
-
/* Create the vring */
vq = vring_new_virtqueue(index, info-num, VIRTIO_MMIO_VRING_ALIGN,
vdev,
true, info-queue, vm_notify, callback, name);
@@ -371,6 +354,33 @@ static struct virtqueue *vm_setup_vq(struct
virtio_device *vdev, unsigned index,
goto error_new_virtqueue;
}
+ /* Activate the queue */
+ writel(info-num, vm_dev-base + VIRTIO_MMIO_QUEUE_NUM);
+ if (vm_dev-version == 1) {
+ writel(PAGE_SIZE, vm_dev-base + VIRTIO_MMIO_QUEUE_ALIGN);
+ writel(virt_to_phys(info-queue) PAGE_SHIFT,
+ vm_dev-base + VIRTIO_MMIO_QUEUE_PFN);
+ } else {
+ u64 addr;
+
+ addr = virt_to_phys(info-queue);
+ writel((u32)addr, vm_dev-base + VIRTIO_MMIO_QUEUE_DESC_LOW);
+ writel((u32)(addr 32),
+ vm_dev-base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
+
+ addr = virt_to_phys(virtqueue_get_avail(vq));
+ writel((u32)addr, vm_dev-base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
+ writel((u32)(addr 32),
+ vm_dev-base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
+
+ addr = virt_to_phys(virtqueue_get_used(vq));
+ writel((u32)addr, vm_dev-base + VIRTIO_MMIO_QUEUE_USED_LOW);
+ writel((u32)(addr 32),
+ vm_dev-base + VIRTIO_MMIO_QUEUE_USED_HIGH);
+
+ writel(1, vm_dev-base + VIRTIO_MMIO_QUEUE_READY);
+ }
+
vq-priv = info;
info-vq = vq;
This patch moved the call to vring_new_virtqueue() in the legacy code flow
before the VIRTIO_MMIO_QUEUE_NUM, VIRTIO_MMIO_QUEUE_ALIGN, and
VIRTIO_MMIO_QUEUE_PFN writes.
Just to make sure: we're talking the legacy case only here, correct?
Was this intentional?
Yes, it simply made the code cleaner. I remember stopping for a moment
doing this change and thinking what bad can it make. Haven't figured out
anything, but it seems I was wrong ;-)
Could the old behavior be reinstated?
I see no big problem with this, but only for the if (vm_dev-version ==
1) case.
We have an implementation that relies on knowing ahead of time what address
range will be used, and is blind to memory accesses that occur before
VIRTIO_MMIO_QUEUE_PFN is written to (or VIRTIO_MMIO_QUEUE_READY when we
upgrade). Is such an implementation supported by the specification? We can't
find any explicit mention that the driver is forbidden from writing to the
memory region before VIRTIO_MMIO_QUEUE_READY is set to 1 (or
VIRTIO_MMIO_QUEUE_PFN is set for legacy devices).
Hm. At the first glance I wouldn't expect the spec to impose such ban.
After all the driver is responsible for providing the ring memory, spec
doesn't care (or does it?) how is it coming into existence - it's the
guest's memory after all. Am I missing something obvious?
Pawel
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] virtio_mmio: support non-legacy balloon devices
On Mon, 2015-03-30 at 18:37 +0100, Michael S. Tsirkin wrote: virtio_device_is_legacy_only is always false now, drop the test from virtio mmio. Signed-off-by: Michael S. Tsirkin m...@redhat.com Slightly ironic ack ;-) after all the battle you fought for this: Acked-by: Pawel Moll pawel.m...@arm.com Thanks! Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_mmio: generation support
On Thu, 2015-03-12 at 02:07 +, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: virtio_mmio currently lacks generation support which makes multi-byte field access racy. Fix by getting the value at offset 0xfc for version 2 devices. Nothing we can do for version 1, so return generation id 0. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- Pawel, you mentioned you have a working virtio 1.0 hypervisor, can you pls confirm this works correctly? Again, belated Acked-by: Pawel Moll pawel.m...@arm.com although not yet tested-by... Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 19/35 linux-next] virtio_mmio: constify of_device_id array
On Mon, 2015-03-16 at 19:20 +, Fabian Frederick wrote:
of_device_id is always used as const.
(See driver.of_match_table and open firmware functions)
Signed-off-by: Fabian Frederick f...@skynet.be
---
drivers/virtio/virtio_mmio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 9c877d2..0ce8cda 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -671,7 +671,7 @@ static void vm_unregister_cmdline_devices(void)
/* Platform driver */
-static struct of_device_id virtio_mmio_match[] = {
+static const struct of_device_id virtio_mmio_match[] = {
{ .compatible = virtio,mmio, },
{},
};
Acked-by: Pawel Moll pawel.m...@arm.com
Thanks!
Pawel
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_mmio: fix endian-ness for mmio
On Fri, 2015-03-13 at 01:22 +, Rusty Russell wrote: I'm sure Pawel is on a beach somewhere sipping cocktails, Ehm... There's still a some more prohibition for me (granted, by choice, just to share the pain ;-), so it wasn't exactly so I'll apply this immediately (with your updated commit message) to my pending-rebases and give him the weekend to Ack. Not that it matters any more, but a belated Acked-by: Pawel Moll pawel.m...@arm.com Thanks, Michael, for fixing this! (although, I must admit, I haven't tested it yet - all in right time :-) Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio fixes pull for 4.0?
On Mon, 2015-03-09 at 07:13 +, Rusty Russell wrote: virtio_mmio: generation support virtio_mmio: fix endian-ness for mmio these two are waiting for ack by Pawel These two fix bugs in virtio 1.0 code for mmio. Host code for that was AFAIK not posted, so I can't test properly. Pawel? I'm waiting on Acks for these two. Right, sorry about being silent for a while - I forked and was on paternity leave... Will go through the thread and respond today. Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC v5 net-next 4/6] virtio-net: add basic interrupt coalescing support
On Fri, 2015-02-13 at 02:52 +, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: On Tue, Feb 10, 2015 at 12:02:37PM +1030, Rusty Russell wrote: Jason Wang jasow...@redhat.com writes: This patch enables the interrupt coalescing setting through ethtool. The problem is that there's nothing network specific about interrupt coalescing. I can see other devices wanting exactly the same thing, which means we'd deprecate this in the next virtio standard. I think the right answer is to extend like we did with vring_used_event(), eg: 1) Add a new feature VIRTIO_F_RING_COALESCE. 2) Add another a 32-bit field after vring_used_event(), eg: #define vring_used_delay(vr) (*(u32 *)((vr)-avail-ring[(vr)-num + 2])) This loses the ability to coalesce by number of frames, but we can still do number of sg entries, as we do now with used_event, and we could change virtqueue_enable_cb_delayed() to take a precise number if we wanted. But do we expect delay to be update dynamically? If not, why not stick it in config space? Hmm, we could update it dynamically (and will, in the case of ethtool). But it won't be common, so we could append a field to virtio_pci_common_cfg for PCI. I think MMIO and CCW would be easy to extend too, but CC'd to check. As far as I understand the virtio_pci_common_cfg principle (just had a look, for the first time ;-), it's now an equivalent of the MMIO control registers block. I see no major problem with adding another one. Or were you thinking about introducing some standard for the real config space? (fine with me as well - the transport will have nothing to do :-) Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-mmio: Update the device to OASIS spec version
This patch add a support for second version of the virtio-mmio device,
which follows OASIS Virtual I/O Device (VIRTIO) Version 1.0
specification.
Main changes:
1. The control register symbolic names use the new device/driver
nomenclature rather than the old guest/host one.
2. The driver detect the device version (version 1 is the pre-OASIS
spec, version 2 is compatible with fist revision of the OASIS spec)
and drives the device accordingly.
3. New version uses direct addressing (64 bit address split into two
low/high register) instead of the guest page size based one,
and addresses each part of the queue (descriptors, available, used)
separately.
4. The device activity is now explicitly triggered by writing to the
queue ready register.
5. Whole 64 bit features are properly handled now (both ways).
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
Based on
Tested with our custom models (*not* qemu).
Changed from RFC:
* removed the version sysfs attribute at its usablity was questioned
* added #ifndefs allowing to disable legacy register names in the header
* started using handy virtqueue_get_used/avail() functions instead of
doing all the calculations manually
* ID 0 special handled by returning -ENODEV instead of 0 - still prints
no error (unless you've got DEBUG for drivers/base), but doesn't
actually bind the device
drivers/virtio/virtio_mmio.c | 116 ---
include/linux/virtio_mmio.h | 44 +---
2 files changed, 103 insertions(+), 57 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 00d115b..9126e14 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -1,7 +1,7 @@
/*
* Virtio memory mapped device driver
*
- * Copyright 2011, ARM Ltd.
+ * Copyright 2011-2014, ARM Ltd.
*
* This module allows virtio devices to be used over a virtual, memory mapped
* platform device.
@@ -50,36 +50,6 @@
*
*
*
- * Registers layout (all 32-bit wide):
- *
- * offset d. name description
- * -- -- -
- *
- * 0x000 R MagicValue Magic value virt
- * 0x004 R Version Device version (current max. 1)
- * 0x008 R DeviceID Virtio device ID
- * 0x00c R VendorID Virtio vendor ID
- *
- * 0x010 R HostFeatures Features supported by the host
- * 0x014 W HostFeaturesSel Set of host features to access via HostFeatures
- *
- * 0x020 W GuestFeaturesFeatures activated by the guest
- * 0x024 W GuestFeaturesSel Set of activated features to set via
GuestFeatures
- * 0x028 W GuestPageSizeSize of guest's memory page in bytes
- *
- * 0x030 W QueueSel Queue selector
- * 0x034 R QueueNumMax Maximum size of the currently selected queue
- * 0x038 W QueueNum Queue size for the currently selected queue
- * 0x03c W QueueAlign Used Ring alignment for the current queue
- * 0x040 RW QueuePFN PFN for the currently selected queue
- *
- * 0x050 W QueueNotify Queue notifier
- * 0x060 R InterruptStatus Interrupt status register
- * 0x064 W InterruptACK Interrupt acknowledge register
- * 0x070 RW Status Device status register
- *
- * 0x100+ RW Device-specific configuration space
- *
* Based on Virtio PCI driver by Anthony Liguori, copyright IBM Corp. 2007
*
* This work is licensed under the terms of the GNU GPL, version 2 or later.
@@ -145,11 +115,16 @@ struct virtio_mmio_vq_info {
static u64 vm_get_features(struct virtio_device *vdev)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ u64 features;
- /* TODO: Features 32 bits */
- writel(0, vm_dev-base + VIRTIO_MMIO_HOST_FEATURES_SEL);
+ writel(1, vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features = readl(vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES);
+ features = 32;
- return readl(vm_dev-base + VIRTIO_MMIO_HOST_FEATURES);
+ writel(0, vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features |= readl(vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES);
+
+ return features;
}
static int vm_finalize_features(struct virtio_device *vdev)
@@ -159,11 +134,13 @@ static int vm_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);
+ writel(1, vm_dev-base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+ writel((u32)(vdev-features 32),
+ vm_dev-base + VIRTIO_MMIO_DRIVER_FEATURES);
- writel(0, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
- writel(vdev-features, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES);
+ writel(0, vm_dev-base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+ writel((u32)vdev-features
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Mon, 2015-01-19 at 18:36 +, Michael S. Tsirkin wrote: Well, not quite: as of now I've got legacy block device driver happily working on top of compliant (so v2 in MMIO speech) MMIO device - the transport if completely transparent here. Spec says explicitly it's an illegal configuration. What part of the spec exactly? The closest I can think of are 2.2.3, 6.2 and 6.3. Both legacy and spec-complaint MMIO transports will satisfy all requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is negotiated or not. The parts that say VIRTIO_F_VERSION_1 MUST be set. I'll look up the chapter and verse later if you like. That would be: 6.1 Driver Requirements: Reserved Feature Bits A driver MUST accept VIRTIO_F_VERSION_1 if it is offered. A driver MAY fail to operate further if VIRTIO_F_VERSION_1 is not offered. 6.2 Device Requirements: Reserved Feature Bits A device MUST offer VIRTIO_F_VERSION_1. A device MAY fail to operate further if VIRTIO_F_VERSION_1 is not accepted. Both are perfectly clear and reasonable for me. And the MMIO transport in both versions (pre-spec and the spec one) will meet those requirements, as it was able to pass more than 32 features bits from the very beginning. Can you please also add a comment? E.g. /* * MMIO uses ID 0 to mean device not present. Avoid probing * the device further: it's sure to fail anyway. */ There is a comment: On Fri, 2014-12-19 at 18:38 +, Pawel Moll wrote: + /* +* ID 0 means a dummy (placeholder) device, skip quietly +* (as in: no error) with no further actions +*/ But I'm can use your wording if you find it better. Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio-mmio: Update the device to OASIS spec version
This patch add a support for second version of the virtio-mmio device,
which follows OASIS Virtual I/O Device (VIRTIO) Version 1.0
specification.
Main changes:
1. The control register symbolic names use the new device/driver
nomenclature rather than the old guest/host one.
2. The driver detect the device version (version 1 is the pre-OASIS
spec, version 2 is compatible with fist revision of the OASIS spec)
and drives the device accordingly.
3. New version uses direct addressing (64 bit address split into two
low/high register) instead of the guest page size based one,
and addresses each part of the queue (descriptors, available, used)
separately.
4. The device activity is now explicitly triggered by writing to the
queue ready register.
5. Whole 64 bit features are properly handled now (both ways).
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/virtio_mmio.c | 131 ++-
include/linux/virtio_mmio.h | 44 ---
2 files changed, 118 insertions(+), 57 deletions(-)
Tested with our custom models (*not* qemu).
Changes from v1:
* added legacy/modern checks in probe and finalize_features.
Changes from RFC:
* removed the version sysfs attribute at its usablity was questioned
* added #ifndefs allowing to disable legacy register names in the header
* started using handy virtqueue_get_used/avail() functions instead of
doing all the calculations manually
* ID 0 special handled by returning -ENODEV instead of 0 - still prints
no error (unless you've got DEBUG for drivers/base), but doesn't
actually bind the device
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 00d115b..cad5698 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -1,7 +1,7 @@
/*
* Virtio memory mapped device driver
*
- * Copyright 2011, ARM Ltd.
+ * Copyright 2011-2014, ARM Ltd.
*
* This module allows virtio devices to be used over a virtual, memory mapped
* platform device.
@@ -50,36 +50,6 @@
*
*
*
- * Registers layout (all 32-bit wide):
- *
- * offset d. name description
- * -- -- -
- *
- * 0x000 R MagicValue Magic value virt
- * 0x004 R Version Device version (current max. 1)
- * 0x008 R DeviceID Virtio device ID
- * 0x00c R VendorID Virtio vendor ID
- *
- * 0x010 R HostFeatures Features supported by the host
- * 0x014 W HostFeaturesSel Set of host features to access via HostFeatures
- *
- * 0x020 W GuestFeaturesFeatures activated by the guest
- * 0x024 W GuestFeaturesSel Set of activated features to set via
GuestFeatures
- * 0x028 W GuestPageSizeSize of guest's memory page in bytes
- *
- * 0x030 W QueueSel Queue selector
- * 0x034 R QueueNumMax Maximum size of the currently selected queue
- * 0x038 W QueueNum Queue size for the currently selected queue
- * 0x03c W QueueAlign Used Ring alignment for the current queue
- * 0x040 RW QueuePFN PFN for the currently selected queue
- *
- * 0x050 W QueueNotify Queue notifier
- * 0x060 R InterruptStatus Interrupt status register
- * 0x064 W InterruptACK Interrupt acknowledge register
- * 0x070 RW Status Device status register
- *
- * 0x100+ RW Device-specific configuration space
- *
* Based on Virtio PCI driver by Anthony Liguori, copyright IBM Corp. 2007
*
* This work is licensed under the terms of the GNU GPL, version 2 or later.
@@ -145,11 +115,16 @@ struct virtio_mmio_vq_info {
static u64 vm_get_features(struct virtio_device *vdev)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ u64 features;
+
+ writel(1, vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features = readl(vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES);
+ features = 32;
- /* TODO: Features 32 bits */
- writel(0, vm_dev-base + VIRTIO_MMIO_HOST_FEATURES_SEL);
+ writel(0, vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features |= readl(vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES);
- return readl(vm_dev-base + VIRTIO_MMIO_HOST_FEATURES);
+ return features;
}
static int vm_finalize_features(struct virtio_device *vdev)
@@ -159,11 +134,20 @@ static int vm_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);
+ /* Make sure there is are no mixed devices */
+ if (vm_dev-version == 2
+ !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
+ dev_err(vdev-dev, New virtio-mmio devices (version 2) must
provide VIRTIO_F_VERSION_1 feature!\n);
+ return -EINVAL;
+ }
+
+ writel(1, vm_dev-base
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Tue, 2015-01-20 at 17:44 +, Michael S. Tsirkin wrote: Good. But your current code will also try to support a mix: device that uses the new transport underneath, but old layout and semantics on top. It will not try to support the mix, but rather will not stop it from working. This has no value: the spec does *not* define such devices and it also does not match any hosts, so this just adds to support matrix. And if linux supports it, someone *will* ship such a device and we'll be stuck with it forever. So please, detect out of spec devices and fail gracefully. pci and s390 do this already. Ok, let it be. Will send v2 in a second. Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Thu, 2015-01-15 at 19:12 +, Michael S. Tsirkin wrote:
+static struct device_attribute vm_dev_attr_version =
+ __ATTR(version, S_IRUGO,
vm_dev_attr_version_show, NULL);
+
static int virtio_mmio_probe(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev;
We already expose feature bits - this one really necessary?
Necessary? Of course not, just a debugging feature, really, to see
what
version of control registers are available. Useful - I strongly
believe
so.
Yes but the point is the same info is already available
in core: just look at feature bit 31.
If you think it's important enough to expose in a decoded
way, let's add this in core?
How do you mean in core? It's a mmio-specific value. Content of the
VIRTIO_MMIO_VERSION control register.
Yes but if driver loaded, then revision is always in sync
with the feature bit.
Well, not quite: as of now I've got legacy block device driver happily
working on top of compliant (so v2 in MMIO speech) MMIO device - the
transport if completely transparent here.
Spec says explicitly it's an illegal configuration.
What part of the spec exactly? The closest I can think of are 2.2.3, 6.2
and 6.3. Both legacy and spec-complaint MMIO transports will satisfy all
requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
negotiated or not.
Make no mistake - I don't know why would anyone wanted to do this kind
of mishmash (other than for testing purposes), but from the MMIO
transport point of view, it's not a problem.
Does the check in finalize_features() solves the problem? If I
understand correctly it should, so there's nothing more to be done,
either in the MMIO spec or in the driver.
If driver failed to attach, attribute is not there
so can't be used for debugging.
Interesting point on its own, haven't thought of that. This is an issue
with platform devices, no standard set of attributes, always available.
Will have a look at this.
Absolutely. So what happens if you drop these code lines?
There's no driver registered for this ID, so it's just ignored.
Seems like what spec is asking for, no?
Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
Yes - there will be a device, but no driver will drive it.
A device with an *illegal* ID.
So? The device is just a bunch of allocated memory. There's no driver
and no one touches is, which is what the spec requires.
So what exactly is wrong with the if (id == 0) ignore case handling? I
bet a compare-to-zero and a branch in two places takes less memory than
the allocated struct virtio_device. And certainly takes less cycles (not
that this matters at all :-). And it's pretty well documented in the
spec.
@@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct
platform_device *pdev)
{
struct virtio_mmio_device *vm_dev =
platform_get_drvdata(pdev);
- unregister_virtio_device(vm_dev-vdev);
+ if (vm_dev)
+ unregister_virtio_device(vm_dev-vdev);
Will remove ever be called if probe fails?
No.
Then this if isn't necessary: vm_dev is always set.
Not (in the current code) if ID is 0.
So just return -ENODEV, then probe will be considered
failed and remove won't be called.
4.2.2.2 Driver Requirements: MMIO Device Register Layout
The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any
error.
Returning -ENODEV *reports* an error.
Reports to whom? Do you refer to http://xkcd.com/838/ ?
ENODEV is not reported.
No, you're right - -ENODEV doesn't print out probe of xxx failed with
error yyy message, I was wrong. I'm pretty sure I tried it before (I
was hoping to find an error which would block probing in a silent way)
and I saw something, but it was probably the pr_debug() level message.
It's just a function return value,
it tells kernel not to call remove.
Users don't know: module probe succeeds, core will not print any errors, user
is not
queried.
What happens with your patch? Driver is attached to
device (check where does driver attribute points to!),
but doesn't do anything.
Looks like if you just keep going, you'll achieve
the same result.
Yes, returning -ENODEV when ID == 0 seems perfectly fine for me.
Or - better - just register the device, it's harmless
as no driver will try to attach to it, and there
won't be any need to special-case it.
Really, you may want to refresh your memory. We've been there. This *is*
a special case. Intentionally.
Hmm I found https://issues.oasis-open.org/browse/VIRTIO-7
but the resolution there is merely asking that no driver
matches ID 0. Seems OK.
The MMIO changes were all made as
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Thu, 2015-01-15 at 16:51 +, Michael S. Tsirkin wrote:
+ uint64_t addr = virt_to_phys(info-queue);
Kernel normally uses u64 for this type.
Sure, well spotted.
+
+ writel(addr 0x,
+ vm_dev-base + VIRTIO_MMIO_QUEUE_DESC_LOW);
+ writel((addr 32) 0x,
+ vm_dev-base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
+
+ addr += info-num * sizeof(struct vring_desc);
+ writel(addr 0x,
+ vm_dev-base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
+ writel((addr 32) 0x,
+ vm_dev-base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
0x isn't really needed, is it?
I admit I'm never sure what are the narrowing side effects. You are
probably right that u64 32 will be always 32 bit.
+
+ addr += sizeof(struct vring_avail) + info-num *
sizeof(__u16);
+ addr += VIRTIO_MMIO_VRING_ALIGN - 1;
+ addr = ~(VIRTIO_MMIO_VRING_ALIGN - 1);
Host no longer knows the alignment, so why is it needed?
[skipped the spec reference, it's a separate discussion]
I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
it's a legacy thing.
But I still need to pass something to vring_new_virtqueue() below, don't
I? And it will allocate the queue based on some alignment value. I can't
see anything that would create the layout for me, neither in mainline
nor in next. Have I missed something? (wouldn't be surprised if I have)
+ writel(addr 0x,
+ vm_dev-base + VIRTIO_MMIO_QUEUE_USED_LOW);
+ writel((addr 32) 0x,
+ vm_dev-base + VIRTIO_MMIO_QUEUE_USED_HIGH);
+
+ writel(1, vm_dev-base + VIRTIO_MMIO_QUEUE_READY);
+ }
/* Create the vring */
vq = vring_new_virtqueue(index, info-num, VIRTIO_MMIO_VRING_ALIGN,
vdev,
[...]
+static struct device_attribute vm_dev_attr_version =
+ __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
+
static int virtio_mmio_probe(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev;
We already expose feature bits - this one really necessary?
Necessary? Of course not, just a debugging feature, really, to see what
version of control registers are available. Useful - I strongly believe
so.
@@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device
*pdev)
/* Check device version */
vm_dev-version = readl(vm_dev-base + VIRTIO_MMIO_VERSION);
- if (vm_dev-version != 1) {
+ if (vm_dev-version 1 || vm_dev-version 2) {
dev_err(pdev-dev, Version %ld not supported!\n,
vm_dev-version);
return -ENXIO;
}
vm_dev-vdev.id.device = readl(vm_dev-base + VIRTIO_MMIO_DEVICE_ID);
+ if (vm_dev-vdev.id.device == 0) {
+ /*
+ * ID 0 means a dummy (placeholder) device, skip quietly
+ * (as in: no error) with no further actions
+ */
+ return 0;
Necessary?
We don't have drivers for this id anyway.
I'm not sure if you are joking or not, after the battle we fought over
it.
The short answer is: yes. Necessary.
4.2.2 MMIO Device Register Layout
[...]
Virtio Subsystem Device ID
See 5 Device Types for possible values. Value zero (0x0) is used to de-
fine a system memory map with placeholder devices at static, well known
addresses, assigning functions to them depending on user’s needs.
[...]
4.2.2.2 Driver Requirements: MMIO Device Register Layout
The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report
any error.
+ }
Need to also
1. validate that feature bit VIRTIO_1 is set
2. validate that ID is not for a legacy device
otherwise device specific drivers might get invoked
on future devices (e.g. when we update balloon for 1.0)
and they not do the right thing.
I'm not following you, but I admit I haven't though this problem
thoroughly. If you can volunteer an example of things going on, it would
be useful. Either way, I'll think about it again.
@@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device
*pdev)
{
struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
- unregister_virtio_device(vm_dev-vdev);
+ if (vm_dev)
+ unregister_virtio_device(vm_dev-vdev);
Will remove ever be called if probe fails?
No.
-/* Guest's memory page size in bytes - Write Only */
+/* Guest's memory page size in bytes - Write Only
+ * LEGACY DEVICES ONLY! */
This is not the preferred style for multi-line comments :)
Fact. Will fix.
Also - maybe add a flag to selectively disable legacy
or modern macros?
Might be clearer than comments that, after
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Thu, 2015-01-15 at 17:51 +, Michael S. Tsirkin wrote:
I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
it's a legacy thing.
But I still need to pass something to vring_new_virtqueue() below, don't
I? And it will allocate the queue based on some alignment value. I can't
see anything that would create the layout for me, neither in mainline
nor in next. Have I missed something? (wouldn't be surprised if I have)
No, but it's no longer a virtio thing - just an optimization
choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
#define VIRTIO_MMIO_VRING_ALIGN PAGE_SIZE
And maybe add a TODO item - we can optimize by allocating chunks
separately.
I'll wait and see how do deal with
vq = vring_new_virtqueue(index, info-num,
VIRTIO_PCI_VRING_ALIGN, vp_dev-vdev,
+static struct device_attribute vm_dev_attr_version =
+ __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
+
static int virtio_mmio_probe(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev;
We already expose feature bits - this one really necessary?
Necessary? Of course not, just a debugging feature, really, to see what
version of control registers are available. Useful - I strongly believe
so.
Yes but the point is the same info is already available
in core: just look at feature bit 31.
If you think it's important enough to expose in a decoded
way, let's add this in core?
How do you mean in core? It's a mmio-specific value. Content of the
VIRTIO_MMIO_VERSION control register.
@@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct
platform_device *pdev)
/* Check device version */
vm_dev-version = readl(vm_dev-base + VIRTIO_MMIO_VERSION);
- if (vm_dev-version != 1) {
+ if (vm_dev-version 1 || vm_dev-version 2) {
dev_err(pdev-dev, Version %ld not supported!\n,
vm_dev-version);
return -ENXIO;
}
vm_dev-vdev.id.device = readl(vm_dev-base +
VIRTIO_MMIO_DEVICE_ID);
+ if (vm_dev-vdev.id.device == 0) {
+ /*
+ * ID 0 means a dummy (placeholder) device, skip quietly
+ * (as in: no error) with no further actions
+ */
+ return 0;
Necessary?
We don't have drivers for this id anyway.
I'm not sure if you are joking or not, after the battle we fought over
it.
Sorry, I don't remember anymore. Just asking.
The short answer is: yes. Necessary.
4.2.2 MMIO Device Register Layout
[...]
Virtio Subsystem Device ID
See 5 Device Types for possible values. Value zero (0x0) is used to de-
fine a system memory map with placeholder devices at static, well known
addresses, assigning functions to them depending on user’s needs.
[...]
4.2.2.2 Driver Requirements: MMIO Device Register Layout
The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report
any error.
Absolutely. So what happens if you drop these code lines?
There's no driver registered for this ID, so it's just ignored.
Seems like what spec is asking for, no?
Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
+ }
Need to also
1. validate that feature bit VIRTIO_1 is set
2. validate that ID is not for a legacy device
otherwise device specific drivers might get invoked
on future devices (e.g. when we update balloon for 1.0)
and they not do the right thing.
I'm not following you, but I admit I haven't though this problem
thoroughly. If you can volunteer an example of things going on, it would
be useful. Either way, I'll think about it again.
1. you need to check ID 0, and assume rev 0. If device also
says it needs rev 1, fail. E.g. see my patch for virtio_pci_modern:
if (virtio_device_is_legacy_only(vp_dev-vdev.id))
return -ENODEV;
you can find the code in my tree, see below.
2. it's easy - just get features on probe and validate VIRTIO_1
bit is set.
s390 does it differently since same device supports version 1 and 0.
No example yet - I forgot to code this up for virtio pci. I'll copy you
on patch.
Ok, will have a look.
@@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device
*pdev)
{
struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
- unregister_virtio_device(vm_dev-vdev);
+ if (vm_dev)
+ unregister_virtio_device(vm_dev-vdev);
Will remove ever be called if probe fails?
No.
Then this if isn't necessary: vm_dev is always set.
Not (in the current
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Thu, 2015-01-15 at 17:12 +, Michael S. Tsirkin wrote: On Thu, Jan 15, 2015 at 06:51:01PM +0200, Michael S. Tsirkin wrote: Host no longer knows the alignment, so why is it needed? In fact, I notice that 4.3.2.3 Virtqueue Layout seems completely wrong: it corresponds to legacy devices, and it does not say what align is. I created https://issues.oasis-open.org/browse/VIRTIO-129 to track this. Can you write up a patch please? 4.3 Virtio Over Channel I/O (hint: not MMIO) Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Thu, 2015-01-15 at 18:39 +, Michael S. Tsirkin wrote: On Fri, Dec 19, 2014 at 06:38:04PM +, Pawel Moll wrote: Tested with our custom models (*not* qemu). Could you look into updating qemu as well please? No, sorry. I'm officially not allowed to touch qemu, due to some legal issues. I'll talk to Peter Maydell. Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio-mmio: Update the device to OASIS spec version
On Thu, 2015-01-15 at 18:29 +, Michael S. Tsirkin wrote:
On Thu, Jan 15, 2015 at 06:11:17PM +, Pawel Moll wrote:
On Thu, 2015-01-15 at 17:51 +, Michael S. Tsirkin wrote:
I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
it's a legacy thing.
But I still need to pass something to vring_new_virtqueue() below, don't
I? And it will allocate the queue based on some alignment value. I can't
see anything that would create the layout for me, neither in mainline
nor in next. Have I missed something? (wouldn't be surprised if I have)
No, but it's no longer a virtio thing - just an optimization
choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
#define VIRTIO_MMIO_VRING_ALIGN PAGE_SIZE
And maybe add a TODO item - we can optimize by allocating chunks
separately.
I'll wait and see how do deal with
vq = vring_new_virtqueue(index, info-num,
VIRTIO_PCI_VRING_ALIGN, vp_dev-vdev,
Check out the code in my tree.
It really does, fundamentally:
/* TODO: don't allocate contigiously */
vq = vring_new_virtqueue(index, info-num,
SMP_CACHE_BYTES, vp_dev-vdev,
Will have a look.
+static struct device_attribute vm_dev_attr_version =
+ __ATTR(version, S_IRUGO, vm_dev_attr_version_show,
NULL);
+
static int virtio_mmio_probe(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev;
We already expose feature bits - this one really necessary?
Necessary? Of course not, just a debugging feature, really, to see what
version of control registers are available. Useful - I strongly believe
so.
Yes but the point is the same info is already available
in core: just look at feature bit 31.
If you think it's important enough to expose in a decoded
way, let's add this in core?
How do you mean in core? It's a mmio-specific value. Content of the
VIRTIO_MMIO_VERSION control register.
Yes but if driver loaded, then revision is always in sync
with the feature bit.
Well, not quite: as of now I've got legacy block device driver happily
working on top of compliant (so v2 in MMIO speech) MMIO device - the
transport if completely transparent here.
If driver failed to attach, attribute is not there
so can't be used for debugging.
Interesting point on its own, haven't thought of that. This is an issue
with platform devices, no standard set of attributes, always available.
Will have a look at this.
Absolutely. So what happens if you drop these code lines?
There's no driver registered for this ID, so it's just ignored.
Seems like what spec is asking for, no?
Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
Yes - there will be a device, but no driver will drive it.
A device with an *illegal* ID.
@@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct
platform_device *pdev)
{
struct virtio_mmio_device *vm_dev =
platform_get_drvdata(pdev);
- unregister_virtio_device(vm_dev-vdev);
+ if (vm_dev)
+ unregister_virtio_device(vm_dev-vdev);
Will remove ever be called if probe fails?
No.
Then this if isn't necessary: vm_dev is always set.
Not (in the current code) if ID is 0.
So just return -ENODEV, then probe will be considered
failed and remove won't be called.
4.2.2.2 Driver Requirements: MMIO Device Register Layout
The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any
error.
Returning -ENODEV *reports* an error.
Or - better - just register the device, it's harmless
as no driver will try to attach to it, and there
won't be any need to special-case it.
Really, you may want to refresh your memory. We've been there. This *is*
a special case. Intentionally.
Not necessarily - the point is for userspace to be able to
avoid getting useless legacy macros by means of a simple #define.
Again, take a look at my tree:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next
I have no idea what are you talking about here. Why would userspace ever
access the memory mapped control registers?
Userspace being qemu which implements these.
Right, qemu is a valid userspace case.
That's all what this header
describes. Actually, if I was typing the driver today, I'd define the
offsets in virtio_mmio.c file, not in a separate header. If fact, I may
move them there...
And then you will have to copy and paste them in qemu.
Which we do ATM for silly
[RFC] virtio-mmio: Update the device to OASIS spec version
This patch add a support for second version of the virtio-mmio device,
which follows OASIS Virtual I/O Device (VIRTIO) Version 1.0
specification.
Main changes:
1. The control register symbolic names use the new device/driver
nomenclature rather than the old guest/host one.
2. The driver detect the device version (version 1 is the pre-OASIS
spec, version 2 is compatible with fist revision of the OASIS spec)
and drives the device accordingly.
3. New version uses direct addressing (64 bit address split into two
low/high register) instead of the guest page size based one,
and addresses each part of the queue (descriptors, available, used)
separately.
4. The device activity is now explicitly triggered by writing to the
queue ready register.
5. The platform device got a sysfs attribute with the version number.
6. Whole 64 bit features are properly handled now (both ways).
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
I had the code typed for months now, but finally (just before
disappearing for the end-of-year break) got time to test it (and
fix the bugs), so I though I'd share it at least as RFC.
It's based on Linus tree still in merge window, but as far as I can
see all virtio changes have been already pulled, so I don't expect
any changes in rc1.
Tested with our custom models (*not* qemu).
Regards and till next year!
Pawel
drivers/virtio/virtio_mmio.c | 132 +++
include/linux/virtio_mmio.h | 46 +++
2 files changed, 120 insertions(+), 58 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 00d115b..d60675a 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -1,7 +1,7 @@
/*
* Virtio memory mapped device driver
*
- * Copyright 2011, ARM Ltd.
+ * Copyright 2011-2014, ARM Ltd.
*
* This module allows virtio devices to be used over a virtual, memory mapped
* platform device.
@@ -50,36 +50,6 @@
*
*
*
- * Registers layout (all 32-bit wide):
- *
- * offset d. name description
- * -- -- -
- *
- * 0x000 R MagicValue Magic value virt
- * 0x004 R Version Device version (current max. 1)
- * 0x008 R DeviceID Virtio device ID
- * 0x00c R VendorID Virtio vendor ID
- *
- * 0x010 R HostFeatures Features supported by the host
- * 0x014 W HostFeaturesSel Set of host features to access via HostFeatures
- *
- * 0x020 W GuestFeaturesFeatures activated by the guest
- * 0x024 W GuestFeaturesSel Set of activated features to set via
GuestFeatures
- * 0x028 W GuestPageSizeSize of guest's memory page in bytes
- *
- * 0x030 W QueueSel Queue selector
- * 0x034 R QueueNumMax Maximum size of the currently selected queue
- * 0x038 W QueueNum Queue size for the currently selected queue
- * 0x03c W QueueAlign Used Ring alignment for the current queue
- * 0x040 RW QueuePFN PFN for the currently selected queue
- *
- * 0x050 W QueueNotify Queue notifier
- * 0x060 R InterruptStatus Interrupt status register
- * 0x064 W InterruptACK Interrupt acknowledge register
- * 0x070 RW Status Device status register
- *
- * 0x100+ RW Device-specific configuration space
- *
* Based on Virtio PCI driver by Anthony Liguori, copyright IBM Corp. 2007
*
* This work is licensed under the terms of the GNU GPL, version 2 or later.
@@ -145,11 +115,16 @@ struct virtio_mmio_vq_info {
static u64 vm_get_features(struct virtio_device *vdev)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ u64 features;
+
+ writel(1, vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features = readl(vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES);
+ features = 32;
- /* TODO: Features 32 bits */
- writel(0, vm_dev-base + VIRTIO_MMIO_HOST_FEATURES_SEL);
+ writel(0, vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+ features |= readl(vm_dev-base + VIRTIO_MMIO_DEVICE_FEATURES);
- return readl(vm_dev-base + VIRTIO_MMIO_HOST_FEATURES);
+ return features;
}
static int vm_finalize_features(struct virtio_device *vdev)
@@ -159,11 +134,13 @@ static int vm_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);
+ writel(1, vm_dev-base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+ writel((vdev-features 32) 0x,
+ vm_dev-base + VIRTIO_MMIO_DRIVER_FEATURES);
- writel(0, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
- writel(vdev-features, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES);
+ writel(0, vm_dev-base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+ writel(vdev-features 0x
Re: [RFC PATCH] virtio-mmio: support for multiple irqs
On Thu, 2014-11-13 at 09:39 +, Shannon Zhao wrote: When we use only virtio-mmio or vhost-net without irqfd, the device uses qemu_set_irq(within qemu) to inject interrupt and at the same time qemu update VIRTIO_MMIO_INTERRUPT_STATUS to tell guest driver whom this interrupt to. All these things are done by qemu. Though qemu_set_irq will call kvm_set_irq to do the real interrupt inject, but the trigger is qemu and it can update the VIRTIO_MMIO_INTERRUPT_STATUS register before injecting the interrupt. But if we use vhost-net with irqfd, the device uses ioeventfd mechanism to inject interrupt. When an interrupt happened, it doesn't transfer to qemu, while the irqfd finally call kvm_set_irq to inject the interrupt directly. All these things are done in kvm and it can't update the VIRTIO_MMIO_INTERRUPT_STATUS register. So if the guest driver still uses the old interrupt handler, it has to read the VIRTIO_MMIO_INTERRUPT_STATUS register to get the interrupt reason and run different handlers for different reasons. But the register has nothing and none of handlers will be called. I make myself clear? :-) I see... (well, at least I believe I see ;-) Of course this means that with irqfd, the device is simply non-compliant with the spec. And that extending the status register doesn't help you at all, so we can drop this idea. Paradoxically, it's a good news (of a sort :-) because I don't think we should be doing such a fundamental change in the spec at this date. I'll discuss it with others in the TC and I'm open to be convinced otherwise, but I believe the spec should stay as it is. Having said that, I see no problem with experimenting with the device model so the next version of the standard is extended. Two suggestions I have would be: 1. Drop the virtio-pci like case with two interrupts (one for config changes, one for all queues), as it doesn't bring anything new. Just make it all interrupts individual. 2. Change the way the interrupts are acknowledge - instead of a bitmask, have a register which takes a queue number to ack the queue's interrupt and ~0 to ack the config interrupt. 3. Change the version of the device to (intially) 0x8003 - I've just made an executive decision :-) that non-standard compliant devices should have the MSB of the version number set (I'll propose to reserve this range of versions in future version of the spec). We'll consider it a prototype of the version 3. Then make the driver behave in the new way when (and only when) such device version is observed. Also, I remembered I haven't addressed one of your previous comments: On Wed, 2014-11-12 at 08:32 +, Shannon Zhao wrote: One point I'd like to make is that the device was intentionally designed with simplicity in mind first, performance later (something about embedded etc :-). Changing this assumption is of course possible, but Ah, I think ARM is not only about embedded things. Maybe it could has a wider application such as micro server. Just my personal opinion. By all means, I couldn't agree more. But there's one thing you have to keep in mind - it doesn't matter whether the real hardware has got PCI or not, but what is emulated by qemu/KVM. Therefore, if only you can get the PCI host controller working in the qemu virt machine (which should be much simpler now, as we have generic support for PCI controllers/bridges in the kernel now), you'll be able to forget the issue of virtio-mmio and multiple interrupts and still enjoy your performance gains :-) Does it all make sense? Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH] virtio-mmio: support for multiple irqs
On Wed, 2014-11-12 at 08:32 +, Shannon Zhao wrote: On 2014/11/11 23:11, Pawel Moll wrote: On Tue, 2014-11-04 at 09:35 +, Shannon Zhao wrote: As the current virtio-mmio only support single irq, so some advanced features such as vhost-net with irqfd are not supported. And the net performance is not the best without vhost-net and irqfd supporting. Could you, please, help understanding me where does the main issue is? Is it about: 1. The fact that the existing implementation blindly kicks all queues, instead only of the updated ones? or: 2. Literally having a dedicated interrupt line (remember, we're talking real interrupts here, not message signalled ones) per queue, so they can be handled by different processors at the same time? The main issue is that current virtio-mmio only support one interrupt which is shared by config and queues. Yes. Additional question: is your device changing the configuration space often? Other devices (for example block devices) change configuration very rarely (eg. change of the media size, usually meaning hot-un-plugging), so unless you tell me that the config space is frequently changes, I'll consider the config event irrelevant from the performance point of view. Therefore the virtio-mmio driver should read the VIRTIO_MMIO_INTERRUPT_STATUS to get the interrupt reason and check whom this interrupt is to. Correct. If we use vhost-net which uses irqfd to inject interrupt, the vhost-net doesn't update VIRTIO_MMIO_INTERRUPT_STATUS, then the guest driver can't read the interrupt reason and doesn't call a handler to process. Ok, so this is the bit I don't understand. Explain, please how does this whole thing work. And keep in mind that I've just looked up irqfd for the first time in my life, so know nothing about its implementation. The same applies to vhost-net. I'm simply coming from a completely different background, so bear with me on this. Now, the virtio-mmio device *must* behave in a certain way, as specified in both the old and the new version of the spec. If a Used Ring of *any* of the queues has been updated the device *must* set bit 0 of the interrupt status register, and - in effect - generate the interrupt. But you are telling me that in some circumstances the interrupt is *not* generated when a queue requires handling. Sounds like a bug to me, as it is not following the requirements of the device specification. It is quite likely that I simply don't see some facts which are obvious for you, so please - help me understand. So we can assign a dedicated interrupt line per queue for virtio-mmio and it can work with irqfd. If my reasoning above is correct, I'd say that you are just working around a functional bug, not improving performance. And this is a wrong way of solving the problem. Theoretically the number of interrupts has no limit, but as the limit of ARM interrupt line, the number should be less than ARM interrupt lines. Let me just emphasize the fact that virtio-mmio is not related to ARM in any way, it's just a memory mapped device with a generic interrupt output. Any limitations of a particular hardware platform (I guess you're referring to the maximum number of peripheral interrupts a GIC can take) are irrelevant - if we go with multiple interrupts, it must cater for as many interrupt lines as one wishes... :-) In the real situation, I think, the number is generally less than 17 (8 pairs of vring interrupts and one config interrupt). ... but of course we could optimize for the real scenarios. And I'm glad to hear that the number you quoted is less then 30 :-) we use GSI for multiple irq. I'm not sure what GSI stands for, but looking at the code I assume it's just a normal peripheral interrupt. In this patch we use vm_try_to_find_vqs to check whether multiple irqs are supported like virtio-pci. Yeah, I can see that you have followed virtio-pci quite literally. I'm particularly not convinced to the one interrupt for config, one for all queues option. Doesn't make any sense to me here. About one interrupt for all queues, it's not a typical case. But just offer one more choice for users. Users should configure the number of interrupts according to their situation. Is this the right direction? is there other ways to make virtio-mmio support multiple irq? Hope for feedback. One point I'd like to make is that the device was intentionally designed with simplicity in mind first, performance later (something about embedded etc :-). Changing this assumption is of course possible, but Ah, I think ARM is not only about embedded things. Maybe it could has a wider application such as micro server. Just my personal opinion. - I must say - makes me slightly uncomfortable... The extensions we're discussing here seem doable, but I've noticed your other patches doing with a shared memory region and I didn't like them at all, sorry
Re: [RFC PATCH] virtio-mmio: support for multiple irqs
On Tue, 2014-11-04 at 09:35 +, Shannon Zhao wrote: As the current virtio-mmio only support single irq, so some advanced features such as vhost-net with irqfd are not supported. And the net performance is not the best without vhost-net and irqfd supporting. Could you, please, help understanding me where does the main issue is? Is it about: 1. The fact that the existing implementation blindly kicks all queues, instead only of the updated ones? or: 2. Literally having a dedicated interrupt line (remember, we're talking real interrupts here, not message signalled ones) per queue, so they can be handled by different processors at the same time? Now, if it's only about 1, the simplest solution would be to extend the VIRTIO_MMIO_INTERRUPT_STATUS register to signal up to 30 queues readiness in bits 2-31, still keeping bit 0 as a combined VIRTIO_MMIO_INT_VRING. In case when VIRTIO_MMIO_INT_VRING is set and none of the individual bits is (a device which doesn't support this feature or one that has more than 30 queues and of of those is ready) we would fall back to the original kick all queues approach. This could be a useful (and pretty simple) extension. In the worst case scenario it could be a post-1.0 standard addition, as it would provide backward compatibility. However, if it's about 2, we're talking larger changes here. From the device perspective, we can define it as having per-queue (plus one for config) interrupt output *and* a combined output, being simple logical or of all the others. Then, the Device Tree bindings would be used to express the implementation choices (I'd keep the kernel parameter approach supporting the single interrupt case only). This is a very popular and well understood approach for memory mapped peripherals (for example, see the . It allows the system integrator to make a decision when it's coming to latency vs number interrupt lines trade off. The main issue is that we can't really impose a limit on a number of queues, therefore on a number of interrupts. This would require adding a new interrupt acknowledge register, which would take a number of the queue (or a symbolic value for the config one) instead of a bit mask. And I must say that I'm not enjoying the idea of such substantial change to the specification that late in the process... (in other words: you'll have to put extra effort into convincing me :-) This patch support virtio-mmio to request multiple irqs like virtio-pci. With this patch and qemu assigning multiple irqs for virtio-mmio device, it's ok to use vhost-net with irqfd on arm/arm64. Could you please tell me how many queues (interrupts) are we talking about in this case? 5? A dozen? Hundreds? Disclaimer: I have no personal experience with virtio and network (due to the fact how our Fast Models are implemented, I mostly us block devices and 9p protocol over virtio and I get enough performance from them :-). As arm doesn't support msi-x now, To be precise: ARM does support MSI-X :-) (google for GICv2m) The correct statement would be: normal memory mapped devices have no interface for message signalled interrupts (like MSI-X) we use GSI for multiple irq. I'm not sure what GSI stands for, but looking at the code I assume it's just a normal peripheral interrupt. In this patch we use vm_try_to_find_vqs to check whether multiple irqs are supported like virtio-pci. Yeah, I can see that you have followed virtio-pci quite literally. I'm particularly not convinced to the one interrupt for config, one for all queues option. Doesn't make any sense to me here. Is this the right direction? is there other ways to make virtio-mmio support multiple irq? Hope for feedback. One point I'd like to make is that the device was intentionally designed with simplicity in mind first, performance later (something about embedded etc :-). Changing this assumption is of course possible, but - I must say - makes me slightly uncomfortable... The extensions we're discussing here seem doable, but I've noticed your other patches doing with a shared memory region and I didn't like them at all, sorry. I see the subject has been already touched in the discussions, but let me bring PCI to the surface again. We're getting more server-class SOCs in the market, which obviously bring PCI with them to both arm and arm64 world, something unheard of in the mobile past. I believe the PCI patches for the arm64 have been already merged in the kernel. Therefore: I'm not your boss so, obviously, I can't tell you what to do, but could you consider redirecting your efforts into getting the ARM PCI up and running in qemu so you can simply use the existing infrastructure? This would save us a lot of work and pain in doing late functional changes to the standard and will be probably more future-proof from your perspective (PCI will happen, sooner or later - you can make it sooner ;-) Regards Pawel ___ Virtualization mailing list
Re: VirtIO-MMIO on MMU-less System
On Tue, 2014-10-28 at 16:42 +, Christopher Covington wrote: Just out of curiosity, could VirtIO-MMIO peripherals work on an MMU-less system, such as a hypothetical M-flavor QEMU TCG virt machine? The interface is using physical addresses (or physical page numbers), so MMU or not - it doesn't care. Now, whether the existing implementation of the drivers (or host-side implementation of the virtoo device) would work, that's another question. I see nothing in virtio-mmio driver that clearly wouldn't work, but you never know... Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: what should a virtio-mmio transport without a backend look like?
On Fri, 2013-06-21 at 17:41 +0100, Peter Maydell wrote: On 21 June 2013 17:02, Christopher Covington c...@codeaurora.org wrote: Would using CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES enumeration instead of device tree be any easier? My general view is that the kernel command line is the user's to manipulate, and that QEMU shouldn't touch it at all (just pass it through). (Conversely, QEMU shouldn't require the user to specify odd kernel command line arguments in order to make things work.) I couldn't agree more. Command line is for the user and user only. And the mention config option is optional ;-) (and, interestingly enough it wasn't there in the original version of the driver). As it happens, if you use the command line to specify a virtio device it doesn't make the same complaint about bad magic number as if you specify it via dtb, but that should probably be fixed in the kernel :-) I don't really see how this would be possible - the complaining code is just a normal platform device probe function. And whether specified in the command line or in the tree, it's the same - platform - device. Are you sure you haven't misspelled the parameter name or something in the definition syntax? ;-) Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: what should a virtio-mmio transport without a backend look like?
On Thu, 2013-06-20 at 11:29 +0100, Peter Maydell wrote: I'm (finally) trying to add virtio-mmio support properly to QEMU now Fred has put all the refactoring foundations in place. 1. One question I've run into is: what should a virtio-mmio transport with no backend look like to the guest OS? The spec as written seems to assume that there's always some backend present. (The idea is that QEMU might just always instantiate say 8 mmio transports, and then whether they actually have a blk/net/whatever backend depends on user options). It looks as if the current linux driver insists (if it sees a device tree node) that the MagicValue register at least is correct (otherwise it complains). So one possibility would be MagicValue/Version/VendorID must read as usual, DeviceID should read as some special nothing here value (0?), everything else can RAZ/WI. We could just say all RAZ/WI, since this merely causes Linux currently to print a warning about the bad magic number, and then subsequently make Linux less alarmist about the zero. We could also define that the transport should look as if there's some sort of 'null' backend plugged in. This would be more effort on the qemu side though, I think. There are two aspects of the problem: 1. Current implementation of the virtio core won't do anything to the device if the device/vendor IDs don't match with any of the drivers. 2. All that current virtio-mmio implementation will do is: * read magic * read device vendor id * write page size So, a device behaving as you mentioned - magic ok, all register read as zero, all writes ignored, will do exactly what you want. Now, as to mandating this: 1. We could mandate device ID 0 (zero) as NOOP. This is in line with current ID numbers allocation, just needs formalizing at the top level of the spec. 2. I have no problem with adding the magic/RAZ/WI behaviour to the current appendix X, however I must say that the write page size will disappear in the next version of the spec (no more page numbers, just normal 64-bit addresses), so defining device ID as above will cover your use case, I believe (as in: correct magic == correct device, NOOP device == no further actions). 2. What was the rationale behind prohibiting interrupt line sharing between two virtio-mmio transports? (device operation section of appendix X). Lack of spare IRQs turns out to be the major limit on how many transports you can have. Hm. Simplicity was the intention, really, no hidden agenda. I've never actually tried to have two platform devices sharing an interrupt, so I'm not sure how will the generic infrastructure behave in such situation. (Is there a mailing list I should be asking this question on?) virtualization@lists.linux-foundation.org (now on copy) Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: BUG: virtio_mmio multi-queue competely broken -- virtio *registers* considered harmful
Hi Tom, On Thu, 2013-05-02 at 04:40 +0100, Tom Lyon wrote: Virtiio_mmio attempts to mimic the layout of some control registers from virtio_pci. These registers, in particular VIRTIO_MMIO_QUEUE_SEL and VIRTIO_PCI_QUEUE_SEL, are active in nature, and not just passive like a normal memory location. Thus, the host side must react immediately upon write of these registers to map some other registers (queue address, size, etc) to queue-specific locations. This is just not possible for mmio, and, I would argue, not desirable for PCI either. Could you, please, elaborate more about the environment you are talking about here? The intention of the MMIO device is to behave like a normal memory mapped peripheral, say serial port. In the world of architecture without separate I/O address space (like ARM, MIPS, SH-4 to name only those I know anything about), such peripherals are usually mapped into the virtual address space with special attributes, eg. guaranteeing transactions order. That's why the host can react immediately and to my knowledge multi-queue virtio devices work just fine. I'd love see comments from someone with x86 expertise in such areas. Maybe we are missing some memory barriers here? So the host implementation would have a chance to react to the QUEUE_SEL access before servicing other transactions? Regards Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/7] virtio_config: make transports implement accessors.
On Fri, 2013-04-05 at 04:37 +0100, Rusty Russell wrote:
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 1ba0d68..4c2c6be 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -167,26 +167,18 @@ static void vm_finalize_features(struct virtio_device
*vdev)
}
}
-static void vm_get(struct virtio_device *vdev, unsigned offset,
- void *buf, unsigned len)
+static u8 vm_get8(struct virtio_device *vdev, unsigned offset)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
- u8 *ptr = buf;
- int i;
- for (i = 0; i len; i++)
- ptr[i] = readb(vm_dev-base + VIRTIO_MMIO_CONFIG + offset +
i);
+ return readb(vm_dev-base + VIRTIO_MMIO_CONFIG + offset);
}
-static void vm_set(struct virtio_device *vdev, unsigned offset,
- const void *buf, unsigned len)
+static void vm_set8(struct virtio_device *vdev, unsigned offset, u8 v)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
- const u8 *ptr = buf;
- int i;
- for (i = 0; i len; i++)
- writeb(ptr[i], vm_dev-base + VIRTIO_MMIO_CONFIG + offset +
i);
+ writeb(v, vm_dev-base + VIRTIO_MMIO_CONFIG + offset);
}
static u8 vm_get_status(struct virtio_device *vdev)
@@ -424,8 +416,9 @@ static const char *vm_bus_name(struct virtio_device *vdev)
}
static const struct virtio_config_ops virtio_mmio_config_ops = {
- .get= vm_get,
- .set= vm_set,
+ .get8 = vm_get8,
+ .set8 = vm_set8,
+ VIRTIO_CONFIG_OPS_NOCONV,
.get_status = vm_get_status,
.set_status = vm_set_status,
.reset = vm_reset,
Acked-by: Pawel Moll pawel.m...@arm.com
Thanks!
Pawel
___
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.
On Thu, 2013-03-21 at 08:29 +, Rusty Russell wrote: diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c index d933150..84ef5fc 100644 --- a/drivers/virtio/virtio_mmio.c +++ b/drivers/virtio/virtio_mmio.c [...] static void vm_finalize_features(struct virtio_device *vdev) @@ -160,7 +162,9 @@ static void vm_finalize_features(struct virtio_device *vdev) vring_transport_features(vdev); writel(0, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES_SEL); - writel(vdev-features, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES); + writel((u32)vdev-features, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES); + writel(1, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES_SEL); + writel(vdev-features 32, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES); Maybe (u32)(vdev-features 32), just to keep the lines consistent? I'm just being fussy... ;-) } Otherwise perfectly fine with me (together with 04/22 virtio: use u32, not bitmap for struct virtio_device's features) Acked-by: Pawel Moll pawel.m...@arm.com Pawel ___ 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.
On Thu, 2013-03-21 at 08:29 +, Rusty Russell wrote:
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 1ba0d68..ad7f38f 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -178,6 +178,19 @@ static void vm_get(struct virtio_device *vdev, unsigned
offset,
ptr[i] = readb(vm_dev-base + VIRTIO_MMIO_CONFIG + offset +
i);
}
+#define VM_GETx(bits) \
+static u##bits vm_get##bits(struct virtio_device *vdev, unsigned int offset)
\
+{ \
+ u##bits v; \
+ vm_get(vdev, offset, v, sizeof(v));\
+ return v; \
+}
+
+VM_GETx(8)
+VM_GETx(16)
+VM_GETx(32)
+VM_GETx(64)
+
static void vm_set(struct virtio_device *vdev, unsigned offset,
const void *buf, unsigned len)
{
@@ -189,6 +202,18 @@ static void vm_set(struct virtio_device *vdev, unsigned
offset,
writeb(ptr[i], vm_dev-base + VIRTIO_MMIO_CONFIG + offset +
i);
}
+#define VM_SETx(bits) \
+static void vm_set##bits(struct virtio_device *vdev, unsigned int offset, \
+u##bits v) \
+{ \
+ vm_set(vdev, offset, v, sizeof(v));\
+}
+
+VM_SETx(8)
+VM_SETx(16)
+VM_SETx(32)
+VM_SETx(64)
+
static u8 vm_get_status(struct virtio_device *vdev)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
@@ -424,8 +449,14 @@ static const char *vm_bus_name(struct virtio_device
*vdev)
}
static const struct virtio_config_ops virtio_mmio_config_ops = {
- .get= vm_get,
- .set= vm_set,
+ .get8 = vm_get8,
+ .set8 = vm_set8,
+ .get16 = vm_get16,
+ .set16 = vm_set16,
+ .get32 = vm_get32,
+ .set32 = vm_set32,
+ .get64 = vm_get64,
+ .set64 = vm_set64,
.get_status = vm_get_status,
.set_status = vm_set_status,
.reset = vm_reset,
The idea is by all means fine with me. I wouldn't write it like this,
but I see you're already toying with other alternatives. And I'll have
to make it LE only anyway, so I guess the implementation details don't
really matter right now.
Paweł
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE
On Tue, 2013-03-05 at 00:11 +, Rusty Russell wrote: Pawel Moll pawel.m...@arm.com writes: On Fri, 2013-03-01 at 11:21 +, Marc Zyngier wrote: Having said that, Rusty was contemplating enforcing LE config space in the new PCI layout... I wouldn't complain about that, and would like to see a similar thing on MMIO. Wherever PCI goes, MMIO follows :-) Yes, but if you switch from 'guest-endian' to 'little-endian' how will you tell? For PCI, we'd detect it by using the new layout. The version register/value. At some point of time there will be a new(ish) MMIO layout anyway to deal with 64-bit addresses, replacing the ring page number with two 32-bit hi/lo physical address registers. This was discussed not long after the driver got merged... I'd rather you specify MMIO as little endian, and we fix the kernel config accessors to be endian aware (ie. 8, 16, 32, 64-bit accessors). Since noone BE is using MMIO right now, it's safe... That's absolutely fine with me, however I don't see anything I could do in the virtio_mmio driver and spec - the virtio_config_ops specifies get/set as void * operations and I simply do byte-by-byte copy. Have I missed some config/endianess/PCI related discussion? Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE
On Fri, 2013-03-01 at 10:41 +, Marc Zyngier wrote: On 14/02/13 10:54, Pawel Moll wrote: To solve the never-ending confusions between hosts and guests of different endianess, define all virtio-mmio registers as LE. This change should be safe at this stage, because no known working mixed-endian system exists so there is virtually no risk of breaking compatibility. Signed-off-by: Pawel Moll pawel.m...@arm.com Shouldn't we exclude the config space? PCI defines it as guest-endian, and the above tends to indicate that it should be LE with MMIO. The spec says: Device-specific configuration space starts at an offset 0x100 and is accessed with byte alignment. Its meaning and size depends on the device and the driver. I would hope that accessed with byte alignment is enough of a clue in this subject, but if you think otherwise I can add a sentence stating this explicitly. Having said that, Rusty was contemplating enforcing LE config space in the new PCI layout... Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE
On Fri, 2013-03-01 at 11:21 +, Marc Zyngier wrote: Having said that, Rusty was contemplating enforcing LE config space in the new PCI layout... I wouldn't complain about that, and would like to see a similar thing on MMIO. Wherever PCI goes, MMIO follows :-) Well, it was unclear enough for me to get confused... ;-) It would make sense to have a wording similar to the one in the PCI section. How about this? 8- From a80f01f15397395a8fc49ef424a2d47c8be0937a Mon Sep 17 00:00:00 2001 From: Pawel Moll pawel.m...@arm.com Date: Fri, 1 Mar 2013 12:35:06 + Subject: [PATCH] virtio-spec: Clarify virtio-mmio configuration space organisation To clarify potential confusion regarding the configuration space organisation (endianness in particular) the spec should clearly state that it follows the PCI device behaviour. Signed-off-by: Pawel Moll pawel.m...@arm.com --- virtio-spec.lyx | 22 +- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/virtio-spec.lyx b/virtio-spec.lyx index a00b675..2fba0b0 100644 --- a/virtio-spec.lyx +++ b/virtio-spec.lyx @@ -696,6 +696,17 @@ rproc serial \begin_layout Section Device Configuration +\change_inserted -875410574 1362141102 + +\begin_inset CommandInset label +LatexCommand label +name sec:Device-Configuration + +\end_inset + + +\change_unchanged + \end_layout \begin_layout Standard @@ -9865,7 +9876,7 @@ a \change_deleted -875410574 1360838214 The endianness of the registers follows the native endianness of the Guest. -\change_inserted -875410574 1360838930 +\change_inserted -875410574 1362141146 All register values are organized as Little Endian, similarly to the PCI variant, see also \begin_inset CommandInset ref @@ -9875,6 +9886,15 @@ reference sub:Virtqueue-Endianness \end_inset . + The device-specific configuration space organisation follows the PCI device + specification, see +\begin_inset CommandInset ref +LatexCommand ref +reference sec:Device-Configuration + +\end_inset + +. \change_deleted -875410574 1360838262 -- 1.7.10.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-spec: Define virtio-mmio registers as LE
To solve the never-ending confusions between hosts and guests
of different endianess, define all virtio-mmio registers as LE.
This change should be safe at this stage, because no known
working mixed-endian system exists so there is virtually no
risk of breaking compatibility.
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
virtio-spec.lyx | 30 +-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 1ba9992..a00b675 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -56,6 +56,7 @@
\html_math_output 0
\html_css_as_file 0
\html_be_strict false
+\author -875410574 Pawel Moll
\author -608949062 Rusty Russell,,,
\author -385801441 Cornelia Huck cornelia.h...@de.ibm.com
\author 1112500848 Rusty Russell ru...@rustcorp.com.au
@@ -1850,6 +1851,17 @@ struct vring {
\begin_layout Subsection
A Note on Virtqueue Endianness
+\change_inserted -875410574 1360838374
+
+\begin_inset CommandInset label
+LatexCommand label
+name sub:Virtqueue-Endianness
+
+\end_inset
+
+
+\change_unchanged
+
\end_layout
\begin_layout Standard
@@ -9850,8 +9862,24 @@ a
\end_layout
\begin_layout Standard
+
+\change_deleted -875410574 1360838214
The endianness of the registers follows the native endianness of the Guest.
- Writing to registers described as
+\change_inserted -875410574 1360838930
+All register values are organized as Little Endian, similarly to the PCI
+ variant, see also
+\begin_inset CommandInset ref
+LatexCommand ref
+reference sub:Virtqueue-Endianness
+
+\end_inset
+
+.
+
+\change_deleted -875410574 1360838262
+
+\change_unchanged
+Writing to registers described as
\begin_inset Quotes eld
\end_inset
--
1.7.10.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests
On Wed, 2013-02-13 at 14:25 +, Marc Zyngier wrote: Using readl() to read the magic value and then memcmp() to check it fails on BE, as bytes will be the other way around (by virtue of the registers to follow the endianess of the guest). Hm. Interesting. I missed the fact that readl() as a PCI operation will always assume LE values... Fix it by encoding the magic as an integer instead of a string. So I'm not completely sure this is the right fix, It seems right, however... - Using __raw_readl() instead. Is that a generic enough API? ... this implies that either the spec is wrong (as it should say: the device registers are always LE, in the PCI spirit) or all readl()s co. should be replaced with __raw equivalents. Having said that, does the change make everything else work with a BE guest? (I assume we're talking about the guest being BE, right? ;-) If so it means that the host is not following the current spec and it treats all the registers as LE. - Reading the MAGIC register byte by byte. Is that allowed? The spec only says it is 32bit wide. And the spirit of the spec was: _exactly 32bit wide_. It's just simpler to implement one access width on the host side. Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests
On Wed, 2013-02-13 at 15:28 +, Marc Zyngier wrote: Fix it by encoding the magic as an integer instead of a string. So I'm not completely sure this is the right fix, It seems right, however... - Using __raw_readl() instead. Is that a generic enough API? ... this implies that either the spec is wrong (as it should say: the device registers are always LE, in the PCI spirit) or all readl()s co. should be replaced with __raw equivalents. Well, the spec clearly says that the registers reflect the endianess of the guest, and it makes sense: when performing the MMIO access, KVM needs to convert between host and guest endianess. The virtio-mmio spec says so because it seemed like a good idea at the time ;-) after reading the PCI device spec. But - as I said - I missed the fact that the readl()-like accessors will always do le32_to_cpu(). Apparently ioread32() does the same (there's a separate ioread32be()). So I'm not sure that the spec is correct in this aspect any more. Maybe it should specify the registers as LE always, similarly to PCI? This problem is already covered by 2.3.1 A Note on Virtqueue Endianness in the spec... Having said that, does the change make everything else work with a BE guest? (I assume we're talking about the guest being BE, right? ;-) If so it means that the host is not following the current spec and it treats all the registers as LE. Yes, I only care about a BE guest. And no, not much is actually working (kvmtool is not happy about the guest addresses it finds in the virtio-ring). Need to dive into it and understand what needs to be fixed... Do the other registers like queuenum make sense? Could it be that the page number of the ring you're getting has wrong endianness? Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH][trivial] virtio-mmio: fix wrong comment about register offset
On Sat, 2013-01-26 at 08:54 +, Ryota Ozaki wrote: The register offset of InterruptACK is 0x064, not 0x060. Fix it. Signed-off-by: Ryota Ozaki ozaki.ry...@gmail.com CC: Pawel Moll pawel.m...@arm.com CC: Rusty Russell ru...@rustcorp.com.au CC: Jiri Kosina triv...@kernel.org --- drivers/virtio/virtio_mmio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c index 31f966f..833e6db 100644 --- a/drivers/virtio/virtio_mmio.c +++ b/drivers/virtio/virtio_mmio.c @@ -75,7 +75,7 @@ * * 0x050 W QueueNotify Queue notifier * 0x060 R InterruptStatus Interrupt status register - * 0x060 W InterruptACK Interrupt acknowledge register + * 0x064 W InterruptACK Interrupt acknowledge register * 0x070 RW Status Device status register * * 0x100+ RW Device-specific configuration space Acked-by: Pawel Moll pawel.m...@arm.com Thanks! Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3] virtio-mmio: Fix irq parsing in command line parameter
When the resource_size_t is 64-bit long, the sscanf() on
the virtio device command line paramter string may return
wrong value because its format was defined as %u. Fixed
by using an intermediate local value of a known length.
Also added cleaned up the resource creation and added extra
comments to make the parameters parsing easier to follow.
Reported-by: Lee Jones lee.jo...@linaro.org
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/virtio_mmio.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..9df4578 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,33 @@ static int vm_cmdline_set(const char *device,
int err;
struct resource resources[2] = {};
char *str;
- long long int base;
+ long long int base, size;
+ unsigned int irq;
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;
+ /* Consume size part of the command line parameter */
+ size = memparse(device, str);
+ /* Get @base:irq[:id] chunks */
processed = sscanf(str, @%lli:%u%n:%d%n,
- base, resources[1].start, consumed,
+ base, irq, consumed,
vm_cmdline_id, consumed);
- if (processed 2 || processed 3 || str[consumed])
+ /*
+* sscanf() must processes at least 2 chunks; also there
+* must be no extra characters after the last chunk, so
+* str[consumed] must be '\0'
+*/
+ if (processed 2 || str[consumed])
return -EINVAL;
+ resources[0].flags = IORESOURCE_MEM;
resources[0].start = base;
- resources[0].end += base;
- resources[1].end = resources[1].start;
+ resources[0].end = base + size - 1;
+
+ resources[1].flags = IORESOURCE_IRQ;
+ resources[1].start = resources[1].end = irq;
if (!vm_cmdline_parent_registered) {
err = device_register(vm_cmdline_parent);
--
1.7.10.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio-mmio: Fix irq parsing in the command line
On 64-bit machines resource_size_t is a 64-bit value, while
sscanf() format for this argument was defined as %u. Fixed
by using an intermediate local value of a known length.
Also added cleaned up the resource creation and adde extra
comments to make the parameters parsing easier to follow.
Reported-by: Lee Jones lee.jo...@linaro.org
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/virtio_mmio.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
Comparing to v1 I've simply polished the size assignment
(moved -1 from one place to another) and removed two of the
new comments, as they weren't really useful any more.
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..e807c2a 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,33 @@ static int vm_cmdline_set(const char *device,
int err;
struct resource resources[2] = {};
char *str;
- long long int base;
+ long long int base, size;
+ unsigned int irq;
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;
+ /* Consume size part of the command line parameter */
+ size = memparse(device, str);
+ /* Get @base:irq[:id] chunks */
processed = sscanf(str, @%lli:%u%n:%d%n,
- base, resources[1].start, consumed,
+ base, irq, consumed,
vm_cmdline_id, consumed);
+ /*
+* sscanf() processes 3 chunks if id is given, 2 if not;
+* also there must be no extra characters after the last
+* chunk, so str[consumed] should be '\0'
+*/
if (processed 2 || processed 3 || str[consumed])
return -EINVAL;
+ resources[0].flags = IORESOURCE_MEM;
resources[0].start = base;
- resources[0].end += base;
- resources[1].end = resources[1].start;
+ resources[0].end = base + size - 1;
+
+ resources[1].flags = IORESOURCE_IRQ;
+ resources[1].start = resources[1].end = irq;
if (!vm_cmdline_parent_registered) {
err = device_register(vm_cmdline_parent);
--
1.7.10.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/9] virtio_mmio: Cast resources[1].start to ‘unsigned int *’ to rid compiler warning
On Mon, 2012-11-05 at 08:21 +, Lee Jones wrote: processed = sscanf(str, @%lli:%u%n:%d%n, - base, resources[1].start, consumed, + base, (unsigned int *)resources[1].start, consumed, vm_cmdline_id, consumed); This suppresses a valid warning, leaving our code no less wrong than before. But now it's silently wrong! Do you want to try again? For sure I'll try again. How does `s/%u/%p/` suit? sscanf doesn't do %p... The only reasonable way of fixing this is a intermediate local variable - will post a patch in a second. Thanks for spotting this! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-mmio: Fix irq parsing in command line parameter
On 64-bit machines resource_size_t is a 64-bit value, while
sscanf() format for this argument was defined as %u. Fixed
by using an intermediate local value of a known length.
Also added cleaned up the resource creation and adde extra
comments to make the parameters parsing easier to follow.
Reported-by: Lee Jones lee.jo...@linaro.org
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/virtio_mmio.c | 26 ++
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..0d08843 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,35 @@ static int vm_cmdline_set(const char *device,
int err;
struct resource resources[2] = {};
char *str;
- long long int base;
+ long long int base, size;
+ unsigned int irq;
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;
+ /* Get size part of the command line parameter */
+ size = memparse(device, str) - 1;
+ /* Get @base:irq[:id] chunks */
processed = sscanf(str, @%lli:%u%n:%d%n,
- base, resources[1].start, consumed,
+ base, irq, consumed,
vm_cmdline_id, consumed);
+ /*
+* sscanf() processes 3 chunks if id is given, 2 if not;
+* also there must be no extra characters after the last
+* chunk, so str[consumed] should be '\0'
+*/
if (processed 2 || processed 3 || str[consumed])
return -EINVAL;
+ /* Memory resource */
+ resources[0].flags = IORESOURCE_MEM;
resources[0].start = base;
- resources[0].end += base;
- resources[1].end = resources[1].start;
+ resources[0].end = base + size;
+
+ /* Interrupt resource */
+ resources[1].flags = IORESOURCE_IRQ;
+ resources[1].start = resources[1].end = irq;
if (!vm_cmdline_parent_registered) {
err = device_register(vm_cmdline_parent);
--
1.7.10.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter
On Mon, 2012-11-05 at 13:44 +, Lee Jones wrote: On Mon, 05 Nov 2012, Pawel Moll wrote: On 64-bit machines resource_size_t is a 64-bit value, while sscanf() format for this argument was defined as %u. Fixed by using an intermediate local value of a known length. Also added cleaned up the resource creation and adde extra comments to make the parameters parsing easier to follow. Reported-by: Lee Jones lee.jo...@linaro.org Signed-off-by: Pawel Moll pawel.m...@arm.com --- drivers/virtio/virtio_mmio.c | 26 ++ 1 file changed, 18 insertions(+), 8 deletions(-) Resorted to poaching now have we Pawel? ;) Sure thing - the poached eggs are my firm favourite :-P ;-) But seriously speaking - it's an old patch (all the comments and resources creation, the IRQ stuff is a new thing :-), but never had enough motivation to push it out... Cheers! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/2] virtio-mmio updates for 3.7
On Wed, 2012-09-26 at 01:10 +0100, Rusty Russell wrote: I was thinking about it, but as the problem manifests itself mainly on architectures with large page size, the impact is limited. But if you think it's worth it, I will repost with Cc: stable. I agree with you. But if it's not CC' stable, it's not for 3.7 either, since at this late stage the rules are basically synonymous. Ok, let's postpone it till 3.8 for now, than. If we find out that this is a real problem I'll re-post it with Cc: stable around 3.7-rc2, 3. Cheers! Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 0/2] virtio-mmio updates for 3.7
On Tue, 2012-09-25 at 01:11 +0100, Rusty Russell wrote: Would you be so kind and consider getting those two small fixes into 3.7 merge? All credits go to Brian, all shame on me :-) Should these also be cc'd to sta...@kernel.org? I was thinking about it, but as the problem manifests itself mainly on architectures with large page size, the impact is limited. But if you think it's worth it, I will repost with Cc: stable. Cheers! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/2] virtio-mmio updates for 3.7
Hi Rusty, Would you be so kind and consider getting those two small fixes into 3.7 merge? All credits go to Brian, all shame on me :-) Cheers! Pawel Brian Foley (2): virtio_mmio: fix off by one error allocating queue virtio_mmio: Don't attempt to create empty virtqueues drivers/virtio/virtio_mmio.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) -- 1.7.9.5 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] virtio_mmio: Don't attempt to create empty virtqueues
From: Brian Foley brian.fo...@arm.com
If a virtio device reports a QueueNumMax of 0, vring_new_virtqueue()
doesn't check this, and thanks to an unsigned (i num - 1) loop
guard, scribbles over memory when initialising the free list.
Avoid by not trying to create zero-descriptor queues, as there's no
way to do any I/O with one.
Signed-off-by: Brian Foley brian.fo...@arm.com
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/virtio_mmio.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 58e2d78..6979c1b 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -332,6 +332,16 @@ static struct virtqueue *vm_setup_vq(struct virtio_device
*vdev, unsigned index,
* and two rings (which makes it alignment_size * 2)
*/
info-num = readl(vm_dev-base + VIRTIO_MMIO_QUEUE_NUM_MAX);
+
+ /* If the device reports a 0 entry queue, we won't be able to
+* use it to perform I/O, and vring_new_virtqueue() can't create
+* empty queues anyway, so don't bother to set up the device.
+*/
+ if (info-num == 0) {
+ err = -ENOENT;
+ goto error_alloc_pages;
+ }
+
while (1) {
size = PAGE_ALIGN(vring_size(info-num,
VIRTIO_MMIO_VRING_ALIGN));
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/2] virtio_mmio: fix off by one error allocating queue
From: Brian Foley brian.fo...@arm.com
vm_setup_vq fails to allow VirtQueues needing only 2 pages of
storage, as it should. Found with a kernel using 64kB pages, but
can be provoked if a virtio device reports QueueNumMax where the
descriptor table and available ring fit in one page, and the used
ring on the second (= 227 descriptors with 4kB pages and = 3640
with 64kB pages.)
Signed-off-by: Brian Foley brian.fo...@arm.com
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/virtio_mmio.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 453db0c..58e2d78 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -335,8 +335,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device
*vdev, unsigned index,
while (1) {
size = PAGE_ALIGN(vring_size(info-num,
VIRTIO_MMIO_VRING_ALIGN));
- /* Already smallest possible allocation? */
- if (size = VIRTIO_MMIO_VRING_ALIGN * 2) {
+ /* Did the last iter shrink the queue below minimum size? */
+ if (size VIRTIO_MMIO_VRING_ALIGN * 2) {
err = -ENOMEM;
goto error_alloc_pages;
}
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_mmio: fix off by one error allocating queue
From: Brian Foley brian.fo...@arm.com
vm_setup_vq fails to allow VirtQueues needing only 2 pages of
storage, as it should. Found with a kernel using 64kB pages, but
can be provoked if a virtio device reports QueueNumMax where the
descriptor table and available ring fit in one page, and the used
ring on the second (= 227 descriptors with 4kB pages and = 3640
with 64kB pages.)
Signed-off-by: Brian Foley brian.fo...@arm.com
---
drivers/virtio/virtio_mmio.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 453db0c..58e2d78 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -335,8 +335,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device
*vdev, unsigned index,
while (1) {
size = PAGE_ALIGN(vring_size(info-num,
VIRTIO_MMIO_VRING_ALIGN));
- /* Already smallest possible allocation? */
- if (size = VIRTIO_MMIO_VRING_ALIGN * 2) {
+ /* Did the last iter shrink the queue below minimum size? */
+ if (size VIRTIO_MMIO_VRING_ALIGN * 2) {
err = -ENOMEM;
goto error_alloc_pages;
}
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-mmio: Devices parameter parsing
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@0x100b:48
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
Hi Rusty,
As the param changes are in now, could you queue this
long-time-forgotten patch for 3.5?
Cheers!
Pawel
PS. Congratulations once again and see you in HK.
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 c1601e5..8b35051 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.
@@ -2847,6 +2848,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@0x100b: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 01d6dc2..453db0c 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@0x100b:48
Re: [PATCH 2/2] virtio-mmio: Devices parameter parsing
On Mon, Dec 12, 2011 at 6:57 PM, Pawel Moll pawel.m...@arm.com wrote: 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@0x100b:48 Signed-off-by: Pawel Moll pawel.m...@arm.com On Mon, 2012-04-09 at 17:32 +0100, Sasha Levin wrote: Ping? Was this patch forgotten? Damn, it has been forgetten indeed. It took a while to get the required level-param patch got merged and then it slipped through cracks... I'll re-test it against v3.4-rc and make sure it gets queued for 3.5 (3.4 is unlikely, especially now when Rusty is honeymooning ;-) Thanks for the reminder and sorry about the delay! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] params: level_initcall-like kernel parameters
Morning, On Thu, 2011-12-15 at 03:51 +, Rusty Russell wrote: On Mon, 12 Dec 2011 17:57:06 +, Pawel Moll pawel.m...@arm.com wrote: 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. This patch wasn't quite what I was thinking, but I've realized that we can't put the params in the .init section, your approach is probably the best one. The only way I could think of to put the parameters passing code in between levels was adding new linker sections, and that sounded like an overkill... Note that I've just created a series which gets rid of that silly ISBOOL thing, so you can use the whole field for level. Then I set the level to -1 for the normal calls; I want to use -2 for the early calls, but that's not done yet... I'll rework and rebase your patch like that now. Cool, it's all yours, especially now that I'm the last day at work this year so won't be able to contribute much in the following weeks... Could I just ask you to remember about the virtio_mmio parameters patch if you get somewhere with this? I'll be most grateful! Cheers! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Qemu-devel] [RFC 0/4] virtio-mmio transport
On Mon, 2011-12-12 at 02:52 +, 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
Re: [Qemu-devel] [RFC 0/4] virtio-mmio transport
On Mon, 2011-12-12 at 14:45 +, 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
[PATCH 1/2] params: level_initcall-like kernel parameters
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.m...@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
[PATCH 2/2] virtio-mmio: Devices parameter parsing
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@0x100b:48
Signed-off-by: Pawel Moll pawel.m...@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@0x100b: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@0x100b:48 \
+ * virtio_mmio.device=1K@0x1001e000:74
+ *
+ *
+ *
* Registers layout (all 32-bit wide):
*
* offset d. name description
@@ -42,6 +86,8 @@
* See
Re: [Xen-devel] [ANNOUNCE] Xen port to Cortex-A15 / ARMv7 with virt extensions
On Wed, 2011-11-30 at 14:32 +, Arnd Bergmann wrote:
I don't care much either way, but I think it would be good to
use similar solutions across all hypervisors. The two options
that I've seen discussed for KVM were to use either a virtual PCI
bus with individual virtio-pci devices as on the PC, or to
use the new virtio-mmio driver and individually put virtio devices
into the device tree.
Let me just add that the virtio-mmio devices can already be instantiated
from DT (see Documentation/devicetree/bindings/virtio/mmio.txt).
For A9-based VE I'd suggest placing them around 0x1001e000, eg.:
virtio_block@1001e000 {
compatible = virtio,mmio;
reg = 0x1001e000 0x100;
interrupts = 41;
}
Cheers!
Paweł
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Mon, 2011-11-28 at 00:31 +, Rusty Russell wrote:
Off the top of my head, this makes me think of the way initcalls are
ordered. We could put a parameter parsing initcall at the start of each
initcall level in include/asm-generic/vmlinux.lds.h's INITCALLS macro.
Then we steal four bits from struct kernel_param's flags to indicate the
level of the initcall (-1 == existing ones, otherwise N == before level
N initcalls).
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).
I've came up with a simple prototype (below, only relevant fragments),
doing exactly what I want at the late level (maybe that's all we
actually need? ;-) Of course adding all other levels is possible,
probably some clever generalization will be useful. And of course the
level would be simply a number in the flags, but what to do with _sync
levels (like arch_initcall_sync, which is 3s not just 3).
If it makes any sense I'll carry on, any feedback will be useful.
Cheers!
Paweł
---
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 7317dc2..22e6196 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -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,
+};
+
+module_param_cb_late(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,
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Tue, 2011-11-22 at 00:53 +, Rusty Russell wrote:
Any ideas?
Perhaps not today. So, we will need a linked list of devices and
resources. Yeah, that means allocating, which means YA
slab_is_available()/alloc_bootmem() hack.
Hm. It doesn't sound like a good deal, really... Loads of tricks to get
data I need quite late in the boot process...
Think of yourself as a pioneer...
I had a vague idea of late parameters last night, which would be
parsed once whole machinery is up and running. Will look around and try
to propose something.
On Tue, 2011-11-22 at 00:44 +, Rusty Russell wrote:
Or would it be simpler to enhance sscanf() with some weird format option
for suffixing? I haven't looked for similar cases, but I'd suspect a
big win in general.
This would be neater than anything else we've got:
if (sscanf(device, %llu@%llu[KMG]:%u, ...) != 3
sscanf(device, %llu@%llu[KMG]:%u:%u, ...) != 4)
return -EINVAL;
sscanf was a good hint! Thanks, why haven't I thought of it myself? ;-)
That's what I came up with:
static int vm_cmdline_set(const char *device,
const struct kernel_param *kp)
{
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;
The only bit missing from sscanf() would be some sort of %m format,
which behaved like memparse and also processed unsigned number with 0
base (hard to believe but the only universal - as in octal, dec and
hex - format is %i, which is signed). But still, looks quite neat
already :-)
I'll try to have a look at the late parameters idea tomorrow. Any
early warnings?
Cheers!
Pawel
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Mon, 2011-11-21 at 03:32 +, Rusty Russell wrote: No it's not; I didn't bother when I converted things across, and it shows. 546970bc (Rusty Russell 2010-08-11 23:04:20 -0600 133) #define module_param_cb Now I understand... :-) But we expect virtio hackers to be smarter than average :) Hope I'll stand up to the challenge ;-) there would be a clash. So I wanted to add a first_id parameter, but ... Well, tell them to get the cmdline order right, or allow an explicit device id in the commandline. Yeah, I've came up with the same idea last night: size[KMG]@base:irq[:id] id, if specified, sets the id for the current device and a base for the next one. Since I hope we're going to be coding together more often, I've written this how I would have done it (based loosely on your version) to compare. Thanks, your efforts are truly appreciated! Main changes other than the obvious: 1) Documentation in kernel-parameters.txt I was considering this previously but for some reason I thought kernel-parameters.txt wasn't a place for module parameters at all. Now I had another look and see I was wrong. 2) Doesn't leak mem on error paths. Em, I don't think my code was leaking memory in any error case? (no offence meant or taken ;-) 3) Handles error from platform_device_register_resndata(). As my code was ignoring wrong descriptions (all the continues) I ignored this result as well (the implementation complains on its own anyway), but I get your point. 4) Uses shorter names for static functions/variables. Ah, I get the hint :-) I'm trying to keep the naming convention in static symbols as well, as it makes cscope/ctags/grep usage easier... I'll just use the abbreviated vm_ prefixes then. See what you think... Funnily enough when I proposed some string parser few years ago (totally different story) I was flamed for using strchr() instead of strsep() ;-) But ok, I don't mind getting back to basics. +static int set_cmdline_device(const char *device, const struct kernel_param *kp) [...] + delim = strchr(device, '@'); [...] + *delim = '\0'; Ah. I forgot that strchr() takes const char * but returns non-const char *... Cheating, that's what it is ;-), but will work. Probably what we really want is something like kstrtoull_delim(device, 0, val, @\0) I'll have a look and may try to propose something of that sort, but that's another story. Maybe I should just use simple_strtol() for the time being? I'll post v3 tonight or tomorrow. Cheers! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Mon, 2011-11-21 at 14:44 +, Pawel Moll wrote:
I'll post v3 tonight or tomorrow.
Ok, it won't be v3 then...
I tried again, using your suggestions - see below (just the crucial bit,
however I have the kernel-parameters.txt bits ready as well). Parsing
works like charm, however when it gets to device_register() and
platform_device_register_resndata() I hit the same problem as
previously. Both are using slab allocator...
device_register()-device_add()-device_private_init()-kzalloc()
and
platform_device_register_resndata()-platform_device_register_full()-
platform_device_alloc()-kzalloc().
Essentially it seems that the parameter callbacks are just executed
waaay to early to do more than just set few integers here and there...
Any ideas?
Cheers!
Paweł
8---
@@ -443,6 +489,145 @@ 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 *str,
+ const struct kernel_param *kp)
+{
+ int err;
+ struct resource resources[2];
+ unsigned long long val;
+ char *delim;
+ struct platform_device *pdev;
+
+ /* Size */
+ resources[0].flags = IORESOURCE_MEM;
+ resources[0].end = memparse(str, delim) - 1;
+ if (*delim != '@')
+ return -EINVAL;
+ str = delim + 1;
+
+ /* Base address */
+ delim = strchr(str, ':');
+ if (!delim)
+ return -EINVAL;
+ /* kstrtoull is strict, so we have to temporarily truncate */
+ *delim = '\0';
+ err = kstrtoull(str, 0, val);
+ *delim = ':';
+ if (err)
+ return err;
+ resources[0].start = val;
+ resources[0].end += val;
+ str = delim + 1;
+
+ /* Interrupt */
+ delim = strchr(str, ':');
+ if (delim) /* Optional device id delimiter */
+ *delim = '\0';
+ err = kstrtoull(str, 0, val);
+ if (delim)
+ *delim = ':';
+ resources[1].flags = IORESOURCE_IRQ;
+ resources[1].start = resources[1].end = val;
+ str = delim + 1;
+
+ /* Optional device id */
+ if (delim) {
+ err = kstrtoull(str, 0, val);
+ if (err)
+ return err;
+ vm_cmdline_id = val;
+ }
+
+ 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,
+};
+
+module_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
[PATCH v2] virtio-mmio: Devices parameter parsing
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@0x100b:48,1K@0x1001e000:74
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/Kconfig | 31 +++
drivers/virtio/virtio_mmio.c | 181 +-
2 files changed, 211 insertions(+), 1 deletions(-)
Hi Rusty,
This version adds the first_id parameter I mentioned yesterday,
but still using single charp parameter instead of the _cb version.
And unless you are really (_really_) against it, I'd rather see
this variant.
Cheers!
Paweł
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 816ed08..00f2c82 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -46,4 +46,35 @@ 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 parameter. Be aware that using incorrect parameters (base
+address in particular) can crash your system - you have been warned.
+
+The format for the parameter is as follows:
+
+ [virtio_mmio.]devices=device[delimdevice]
+
+where:
+ device := size@baseaddr:irq
+ delim:= ',' or ';'
+ size := size (can use standard suffixes like K or M)
+ baseaddr := physical base address
+ irq := interrupt number (as passed to request_irq())
+
+Example kernel command line parameter:
+
+ virtio_mmio.devices=0x100@0x100b:48,1K@0x1001e000:74
+
+This will register platform devices virtio_mmio.id, where id
+values are consecutive integer numbers starting from 0 by default.
+The first id value can be changed with first_id parameter:
+
+ [virtio_mmio.]first_id=id
+
+If unsure, say 'N'.
+
endmenu
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index acc5e43..05b39c1 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -6,6 +6,55 @@
* 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:
+ *
+ * [virtio_mmio.]devices=device[delimdevice]
+ *where:
+ * device := size@baseaddr:irq
+ * delim:= ',' or ';'
+ * size := size (can use standard suffixes like K or M)
+ * baseaddr := physical base address
+ * irq := interrupt number (as passed to request_irq())
+ *eg.:
+ * virtio_mmio.devices=0x100@0x100b:48,1K@0x1001e000:74
+ *
+ *This will register platform devices virtio_mmio.id, where id
+ *values are consecutive integer numbers starting from 0 by default.
+ *The first id value can be changed with first_id parameter:
+ *
+ * [virtio_mmio.]first_id=id
+ *
+ *
+ *
* Registers layout (all 32-bit wide):
*
* offset d. name description
@@ -42,6 +91,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 +494,130 @@ static int __devexit virtio_mmio_remove(struct
platform_device *pdev)
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+
+static char *virtio_mmio_cmdline_devices;
+module_param_named(devices, virtio_mmio_cmdline_devices, charp, 0);
+
+static int virtio_mmio_cmdline_id;
+module_param_named(first_id, virtio_mmio_cmdline_id, int, 0);
+
+static struct device virtio_mmio_cmdline_parent = {
+ .init_name = virtio-mmio-cmdline,
+};
+
+static int
Re: [RFC] kvm tools: Add support for virtio-mmio
On Tue, 2011-11-15 at 17:56 +, Sasha Levin wrote: Hmm... If thats the plan, it should probably be a virtio thing (not virtio-mmio specific). Either way, it could also use some clarification in the spec. Well, the spec (p. 2.1) says: The Subsystem Vendor ID should reflect the PCI Vendor ID of the environment (it's currently only used for informational purposes by the guest).. The fact is that all the current virtio drivers simply ignore this field. So unless this changes I simply have no idea how to describe that register. Put anything there, no one cares? Write zero now, may change in future? Any ideas welcomed. Cheers! Paweł PS. Thanks for defending my honour in the delayed-explosive-device thread ;-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Wed, 2011-11-16 at 00:42 +, Rusty Russell wrote:
On Tue, 15 Nov 2011 13:53:05 +, Pawel Moll pawel.m...@arm.com wrote:
+static char *virtio_mmio_cmdline_devices;
+module_param_named(devices, virtio_mmio_cmdline_devices, charp, 0);
This is the wrong way to do this.
Don't put things in a charp and process it later. It's lazy.
Definitely not lazy - string parsing is very absorbing, really! ;-)
You
should write parsers for it and call it straight from module_param.
And if you do it that way, multiple devices are simply multiple
arguments.
module_param_cb(device, param_ops_virtio_mmio, NULL, 0400);
Hm. Honestly, first time I hear someone suggesting using the param_cb
variant... It doesn't seem to be too popular ;-)
$ git grep ^module_param\(.\*charp.\*\)\; | wc -l
159
$ git grep ^module_param_cb\(.\*\)\; | wc -l
7
But anyway, I tried to do use your suggestion (see below), even if I'm
not convinced it's winning anything. But, in order to use the strsep()
and kstrtoull() I need a non-const version of the string. And as the
slab is not available at the time, I can't simply do kstrdup(), I'd have
to abuse the const char *val params_ops.set's argument...
Interestingly charp operations have the same problem:
int param_set_charp(const char *val, const struct kernel_param *kp)
...
/* This is a hack. We can't kmalloc in early boot, and we
* don't need to; this mangled commandline is preserved. */
if (slab_is_available()) {
Also, regarding the fact that one parameter define more than one
entity - this is how mtd partitions are defined (all similarities
intended ;-), see drivers/mtd/cmdlinepart.c. And I quite like this
syntax...
There's one more thing I realize I missed. The platform devices are
registered starting from id 0 (so as virtio-mmio.0). Now, if you
happened to have a statically defined virtio-mmio with the same id,
there would be a clash. So I wanted to add a first_id parameter, but
with the _cb parameter I can't guarantee ordering (I mean, to have the
first_id available _before_ first device is being instantiated). So
I'd have to cache the devices and then create them in one go. Sounds
like the charp parameter for me :-)
So, unless you have really strong feelings about it, I'd stick to the
single-string version, I'll just add the first_id parameter tomorrow.
Cheers!
Paweł
8---
/* Devices list parameter */
#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
static struct device virtio_mmio_cmdline_parent = {
.init_name = virtio-mmio-cmdline,
};
static int virtio_mmio_cmdline_parent_registered;
static int virtio_mmio_cmdline_id;
static int virtio_mmio_register_cmdline_device(const char *device,
const struct kernel_param *kp)
{
int err;
struct resource resources[] = {
{
.flags = IORESOURCE_IRQ,
}, {
.flags = IORESOURCE_MEM,
}
};
char *__token, *token, *size, *base;
unsigned long long val;
token = __token = kstrdup(device, GFP_KERNEL);
printk(token=%s\n, token);
/* Split memory and IRQ resources */
pr_info(base=%s, token=%s\n, base, token);
if (base == token || !token || !*token)
goto error;
/* Get IRQ */
if (kstrtoull(token, 0, val) != 0)
goto error;
resources[0].start = val;
resources[0].end = val;
/* Split base address and size */
size = strsep(base, @);
if (size == base || !base || !*base)
pr_err(No base in '%s'!\n, device);
/* Get base address */
if (kstrtoull(base, 0, val) != 0)
pr_err(Wrong base in '%s'!\n, device);
resources[1].start = val;
resources[1].end = val;
/* Get size */
resources[1].end += memparse(size, token) - 1;
if (size == token || *token)
goto error;
kfree(__token);
pr_info(Registering device virtio-mmio.%d at 0x%x-0x%x, IRQ %u.\n,
virtio_mmio_cmdline_id, resources[1].start,
resources[1].end, resources[0].start);
if (!virtio_mmio_cmdline_parent_registered) {
err = device_register(virtio_mmio_cmdline_parent);
if (err) {
pr_err(Failed to register parent device!\n);
return err;
}
virtio_mmio_cmdline_parent_registered = 1;
}
return platform_device_register_resndata(virtio_mmio_cmdline_parent,
virtio-mmio, virtio_mmio_cmdline_id++,
resources, ARRAY_SIZE(resources), NULL, 0) != NULL;
error:
kfree(__token);
return -EINVAL;
}
static struct kernel_param_ops virtio_mmio_cmdline_param_ops = {
.set
Re: [PATCH] virtio-mmio: Correct the name of the guest features selector
On Tue, 2011-11-15 at 14:17 +, Sasha Levin wrote:
Guest features selector spelling mistake.
diff --git a/include/linux/virtio_mmio.h b/include/linux/virtio_mmio.h
index 27c7ede..5c7b6f0 100644
--- a/include/linux/virtio_mmio.h
+++ b/include/linux/virtio_mmio.h
@@ -63,7 +63,7 @@
#define VIRTIO_MMIO_GUEST_FEATURES 0x020
/* Activated features set selector - Write Only */
-#define VIRTIO_MMIO_GUEST_FEATURES_SET 0x024
+#define VIRTIO_MMIO_GUEST_FEATURES_SEL 0x024
/* Guest's memory page size in bytes - Write Only */
#define VIRTIO_MMIO_GUEST_PAGE_SIZE 0x028
Damn. Sorry about it. But if you change the header you'll need to change
the driver:
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 1f25bb9..10bb8b9 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -161,7 +161,7 @@ static void vm_finalize_features(struct virtio_device *vdev)
vring_transport_features(vdev);
for (i = 0; i ARRAY_SIZE(vdev-features); i++) {
- writel(i, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES_SET);
+ writel(i, vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
writel(vdev-features[i],
vm_dev-base + VIRTIO_MMIO_GUEST_FEATURES);
}
Cheers!
Paweł
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] virtio: Add platform bus driver for memory mapped virtio device
On Mon, 2011-10-24 at 03:33 +0100, Rusty Russell wrote:
No, that's it I think. Please send a diff for the documentation, since
I'm updating the LyX master and I've already applied your previous
version.
Here it goes (below). Also do you think you would be able to merge the
driver (corresponding v4 patch follows) in the 3.2 merge window that
seems to have just opened? ;-)
Cheers!
Pawel
--- virtio-mmio.orig2011-10-24 11:17:08.263907000 +0100
+++ virtio-mmio.tex 2011-10-24 13:58:29.752757000 +0100
@@ -59,9 +59,18 @@
\item 0x050 | W | QueueNotify \\
Queue notifier.\\
Writing a queue index to this register notifies the Host that there are new
buffers to process in the queue.
-\item 0x060 | W | InterruptACK \\
+\item 0x060 | R | InterruptStatus \\
+Interrupt status. \\
+Reading from this register returns a bit mask of interrupts asserted by the
device. An interrupt is asserted if the corresponding bit is set, ie. equals
one (1). \\
+\begin{itemize}
+\item Bit 0 | Used Ring update \\
+This interrupt is asserted when the Host has updated the Used Ring in at least
one of the active virtual queues.
+\item Bit 1 | Configuration change \\
+This interrupt is asserted when configuration of the device has changed.
+\end{itemize}
+\item 0x064 | W | InterruptACK \\
Interrupt acknowledge. \\
-Writing to this register notifies the Host that the Guest finished receiving
used buffers from the device and therefore serviced an asserted interrupt.
Values written to this register are currently not used, but for future
extensions it must be set to one (0x1).
+Writing to this register notifies the Host that the Guest finished handling
interrupts. Every bit of the value clears corresponding bit of the
InterruptStatus register. \\
\item 0x070 | RW | Status \\
Device status. \\
Reading from this register returns the current device status flags. \\
@@ -100,8 +109,7 @@
The memory mapped virtio device behaves in the same way as described in p. 2.4
``Device Operation'', with the following exceptions:
\begin{enumerate}
\item The device is notified about new buffers available in a queue by writing
the queue index to register QueueNum instead of the virtio header in PCI I/O
space (p. 2.4.1.4 ``Notifying The Device'').
-\item As the memory mapped virtio device is using single, dedicated interrupt
signal, its handling is much simpler than in the PCI (MSI-X) case (p. 2.4.2
``Receiving Used Buffer From The Device''). Therefore all the Guest interrupt
handler should do after receiving used buffers is acknowledging the interrupt
by writing a value to the InterruptACK register. Currently this value does not
carry any meaning, but for future extensions it must be set to one (0x1).
-\item The dynamic configuration changes, as described in p. 2.4.3 ``Dealing
With Configuration Changes'' are not permitted.
+\item The memory mapped virtio device is using single, dedicated interrupt
signal. After receiving an interrupt, the driver must read the InterruptStatus
register to check what caused the interrupt (see the register description).
After the interrupt is handled, the driver must acknowledge it by writing a bit
mask corresponding to the serviced interrupt to the InterruptACK register.
\end{enumerate}
\end{document}
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v4] virtio: Add platform bus driver for memory mapped virtio device
This patch, based on virtio PCI driver, adds support for memory
mapped (platform) virtio device. This should allow environments
like qemu to use virtio-based block network devices even on
platforms without PCI support.
One can define and register a platform device which resources
will describe memory mapped control registers and mailbox
interrupt. Such device can be also instantiated using the Device
Tree node with compatible property equal virtio,mmio.
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Anthony Liguori aligu...@us.ibm.com
Cc: Michael S.Tsirkin m...@redhat.com
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
Changes since v3:
* Dynamic config changes are handled now
* Interrupt acknowledge register moved to 0x064
* Added interrupt status register at 0x060
* Added interrupt flags (VIRTIO_MMIO_INT_VRING VIRTIO_MMIO_INT_CONFIG)
Changes since v2:
* Fixed bug a bug in vm_find_vqs() error handling code (interrupt was
freed twice)
Changes since v1:
* Added new QueueNumMax register at 0x034, shifting QueueNum,
QueueAlign and QueuePFN by 4 bytes
* Queue page allocation strategy changed
Documentation/devicetree/bindings/virtio/mmio.txt | 17 +
drivers/virtio/Kconfig| 11 +
drivers/virtio/Makefile |1 +
drivers/virtio/virtio_mmio.c | 479 +
include/linux/virtio_mmio.h | 111 +
5 files changed, 619 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/virtio/mmio.txt
create mode 100644 drivers/virtio/virtio_mmio.c
create mode 100644 include/linux/virtio_mmio.h
diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt
b/Documentation/devicetree/bindings/virtio/mmio.txt
new file mode 100644
index 000..5069c1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/virtio/mmio.txt
@@ -0,0 +1,17 @@
+* virtio memory mapped device
+
+See http://ozlabs.org/~rusty/virtio-spec/ for more details.
+
+Required properties:
+
+- compatible: virtio,mmio compatibility string
+- reg: control registers base address and size including configuration
space
+- interrupts: interrupt generated by the device
+
+Example:
+
+ virtio_block@3000 {
+ compatible = virtio,mmio;
+ reg = 0x3000 0x100;
+ interrupts = 41;
+ }
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 57e493b..816ed08 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -35,4 +35,15 @@ config VIRTIO_BALLOON
If unsure, say M.
+ config VIRTIO_MMIO
+ tristate Platform bus driver for memory mapped virtio devices
(EXPERIMENTAL)
+ depends on EXPERIMENTAL
+ select VIRTIO
+ select VIRTIO_RING
+ ---help---
+This drivers provides support for memory mapped virtio
+platform device driver.
+
+If unsure, say N.
+
endmenu
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 6738c44..5a4c63c 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -1,4 +1,5 @@
obj-$(CONFIG_VIRTIO) += virtio.o
obj-$(CONFIG_VIRTIO_RING) += virtio_ring.o
+obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
new file mode 100644
index 000..acc5e43
--- /dev/null
+++ b/drivers/virtio/virtio_mmio.c
@@ -0,0 +1,479 @@
+/*
+ * Virtio memory mapped device driver
+ *
+ * Copyright 2011, ARM Ltd.
+ *
+ * This module allows virtio devices to be used over a virtual, memory mapped
+ * platform device.
+ *
+ * Registers layout (all 32-bit wide):
+ *
+ * offset d. name description
+ * -- -- -
+ *
+ * 0x000 R MagicValue Magic value virt
+ * 0x004 R Version Device version (current max. 1)
+ * 0x008 R DeviceID Virtio device ID
+ * 0x00c R VendorID Virtio vendor ID
+ *
+ * 0x010 R HostFeatures Features supported by the host
+ * 0x014 W HostFeaturesSel Set of host features to access via HostFeatures
+ *
+ * 0x020 W GuestFeaturesFeatures activated by the guest
+ * 0x024 W GuestFeaturesSel Set of activated features to set via
GuestFeatures
+ * 0x028 W GuestPageSizeSize of guest's memory page in bytes
+ *
+ * 0x030 W QueueSel Queue selector
+ * 0x034 R QueueNumMax Maximum size of the currently selected queue
+ * 0x038 W QueueNum Queue size for the currently selected queue
+ * 0x03c W QueueAlign Used Ring alignment for the current queue
+ * 0x040 RW QueuePFN PFN for the currently selected queue
+ *
+ * 0x050 W QueueNotify Queue notifier
+ * 0x060 R InterruptStatus Interrupt status register
+ * 0x060 W InterruptACK Interrupt acknowledge register
+ * 0x070 RW Status Device status register
+ *
+ * 0x100+ RW
Additional virtio-mmio spec changes
Hi Rusty, Peter (on Cc) got the qemu implementation up and running (thanks!) and provided some useful feedback regarding the spec. To make the long story short, this is what came out of the discussion (feel free to correct me at any point, Peter ;-): diff --git a/virtio-spec.lyx b/virtio-spec.lyx index 6426f8f..79a6498 100644 --- a/virtio-spec.lyx +++ b/virtio-spec.lyx @@ -6681,7 +6681,9 @@ s on QueueNum, QueueAlign and QueuePFN apply to. Writing to this register notifies the Host what size of the queue the Guest will use. - This applies to the queue selected by writing to QueueSel. + This applies to the queue selected by writing to QueueSel and is allowed + only when QueuePFN is set to zero (0x0), so when the queue is not actively + used. \end_layout @@ -6790,8 +6792,8 @@ This interrupt is asserted when configuration of the device has changed. Writing to this register notifies the Host that the Guest finished handling interrupts. - Every bit of the value clears corresponding bit of the InterruptStatus - register. + Set bits in the value clear the corresponding bits of the + InterruptStatus register. \end_layout @@ -6975,7 +6977,9 @@ reference sub:Notifying-The-Device \end_layout \begin_layout Enumerate -The memory mapped virtio device is using single, dedicated interrupt signal. +The memory mapped virtio device is using single, dedicated interrupt signal, + which is raised when at least one of the interrupts described in the + InterruptStatus register description is asserted. After receiving an interrupt, the driver must read the InterruptStatus register to check what caused the interrupt (see the register description). After the interrupt is handled, the driver must acknowledge it by writing Of course those changes are not critical so can easily wait till the next release. Peter also mentioned that he didn't like the Num in QueueNum register name and would rather see it called QueueSize. I have no strong opinions either way and can provide patches to both the spec and the driver (and header) to change it. Any thoughts? Cheers! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC] virtio-spec: flexible configuration layout
On Wed, 2011-11-09 at 10:20 +, Sasha Levin wrote: I'm also wondering it it's ok to move virtio configuration out of virtio space and into PCI space for archs that don't have PCI (such as ARM). Just a note - ARM-based chips can by all means have PCI (grep -r PCI arch/arm/ ;-). The fact is that most of the SOCs available on the market don't have it, but this is slowly changing. The main architectural difference is that ARM doesn't provide separate I/O space so the PCI I/O space is usually remapped somewhere into normal address space (grep -r #define __io_address arch/arm/) Would it mean they get stuck with legacy configuration (and no new features)? Or is there an alternative for them? The change only affects the layout of virtio PCI. Arches that don't have PCI don't use virtio PCI, presumably? BTW, the spec only covers x86 ATM, this needs to be fixed. From what I see there is a WIP by Pawel Moll pawel.m...@arm.com to add virtio platform drivers which get virtio working on ARM for example, and by Peter Maydell peter.mayd...@linaro.org to modify the spec to support MMIO access (besides PCI). Yep, it's actually already in 3.2-rc1 (drivers/virtio/virtio_mmio.c) and in the spec (see Appendix X). And actually the control registers layout I used was originally based on the PCI legacy headers (slightly simplified), but evolved a bit since. My understanding is that the changes Michael is proposing affect the PCI device interface only so they shouldn't affect my interface. By the way, I vaguely remember Peter mentioning that he got the PCI device experimentally running some time ago on one of the PCI-enabled ARM platform models (realview or versatile)... Cheers! Pawel ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
Also, an unrelated questions: With PIO, requests were ordered, which means that if we wrote to the queue selector and then read from a queue register we would read the correct queue info. Is the same thing assured to us with MMIO? For real PCI, reads do not bypass writes in PCI. However this is only true if both are MMIO or both PIO reads. I don't think the ordering of MMIO versus PIO is guaranteed. On KVM, the kernel doesn't do anything to guarantee ordering. So you get the natural ordering of the CPU. If we write to a queue selector and immediately read from queue info would we be reading the right info, or is there the slight chance that it would get reordered and we would be reading queue info first and writing to the selector later? The thing to realize is that write to queue selector with KVM is in the end performed by host. And reading queue info means that host will be reading the queue selector. So this is a write followed by read from the same address. AFAIK no CPUs can reorder such accesses, so you get the right info. (As far as I understand all the complexity ;-) Memory mapped using ioremap()-like functions should preserve access order. In case of ARM architecture such memory region is defined (at the page tables level) as a device memory (contrary to normal memory) and the processor will not try to be clever about it. I know next-to-nothing about x86, but I suppose similar idea applies. Cheers! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
