[PATCH 1/2] kvm tools: Fix IRQ assignments

[PATCH 1/2] kvm tools: Fix IRQ assignments

[Sasha Levin]

virtio-blk needs a block of 4 IRQs (currently - staticly defined).
Giving the initial IRQ of 15 causes breakage when adding more
than one device.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/virtio-pci-dev.h |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/kvm/include/kvm/virtio-pci-dev.h b/tools/kvm/include/kvm/virtio-pci-dev.h
index 431289d..41125c2 100644
--- a/tools/kvm/include/kvm/virtio-pci-dev.h
+++ b/tools/kvm/include/kvm/virtio-pci-dev.h
@@ -31,10 +31,16 @@ enum {
 };
 
 enum {
-	VIRTIO_RNG_IRQ				= 11,
-	VIRTIO_CONSOLE_IRQ			= 13,
-	VIRTIO_NET_IRQ				= 14,
-	VIRTIO_BLK_IRQ				= 15,
+	VIRTIO_RNG_IRQ				= 9,
+	VIRTIO_CONSOLE_IRQ			= 10,
+	VIRTIO_NET_IRQ				= 11,
+	VIRTIO_BLK_IRQ				= 12,
+	/*
+	 * FIXME: Currently IRQs 12-15 are reserved for possible
+	 * virtio-blk devices. Static assignment will be avoided
+	 * once dynamic IRQ assignment is added (or switch to
+	 * MSI-X).
+	 */
 };
 
 #endif /* VIRTIO_PCI_DEV_H_ */
-- 
1.7.5.rc3

[PATCH 2/2] kvm tools: Simplify search for root device

[Sasha Levin]

Use /dev/block to find the block device used for root
instead of searching through mounts.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/kvm-run.c |   48 +++---------------------------------------------
 1 files changed, 3 insertions(+), 45 deletions(-)

diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
index d5a952f..6fee8ae 100644
--- a/tools/kvm/kvm-run.c
+++ b/tools/kvm/kvm-run.c
@@ -249,56 +249,14 @@ static const char *find_kernel(void)
 
 static int root_device(char *dev, long *part)
 {
-	FILE   *fp;
-	char   *line;
-	int    tmp;
-	size_t nr_read;
-	char   device[PATH_MAX];
-	char   mnt_pt[PATH_MAX];
-	char   resolved_path[PATH_MAX];
-	char   *p;
 	struct stat st;
 
-	fp = fopen("/proc/mounts", "r");
-	if (!fp)
+	if (stat("/", &st) < 0)
 		return -1;
 
-	line = NULL;
-	tmp  = 0;
-	while (!feof(fp)) {
-		if (getline(&line, &nr_read, fp) < 0)
-			break;
-		sscanf(line, "%s %s", device, mnt_pt);
-		if (!strncmp(device, "/dev", 4) && !strcmp(mnt_pt, "/")) {
-			tmp = 1;
-			break;
-		}
-	}
-	fclose(fp);
-	free(line);
-
-	if (!tmp)
-		return -1;
-
-	/* get the absolute path */
-	if (!realpath(device, resolved_path))
-		return -1;
-
-	/* find the partition number */
-	p = resolved_path;
-	while (*p) {
-		if (isdigit(*p)) {
-			strncpy(dev, resolved_path, p - resolved_path);
-			*part = atol(p);
-			break;
-		}
-		p++;
-	}
-
-	/* verify the device path */
-	if (stat(dev, &st) < 0)
-		return -1;
+	*part = minor(st.st_dev);
 
+	sprintf(dev, "/dev/block/%u:0", major(st.st_dev));
 	if (access(dev, R_OK) < 0)
 		return -1;
 
-- 
1.7.5.rc3

Re: [PATCH 2/2] kvm tools: Simplify search for root device

[Pekka Enberg]

On Fri, 6 May 2011, Sasha Levin wrote:
> Use /dev/block to find the block device used for root
> instead of searching through mounts.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>

Weren't there distro differences in this area? Prasad, Asias, does this 
work for you?

Re: [PATCH 2/2] kvm tools: Simplify search for root device

[Sasha Levin]

On Sat, 2011-05-07 at 11:41 +0300, Pekka Enberg wrote:
> On Fri, 6 May 2011, Sasha Levin wrote:
> > Use /dev/block to find the block device used for root
> > instead of searching through mounts.
> >
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> Weren't there distro differences in this area? Prasad, Asias, does this 
> work for you?

The distro differences were when we tried using /dev/ROOT and found it's
optional and doesn't always exist.

-- 

Sasha.

Re: [PATCH 2/2] kvm tools: Simplify search for root device

[Prasad Joshi]

On Sat, May 7, 2011 at 9:41 AM, Pekka Enberg <penberg@kernel.org> wrote:
> On Fri, 6 May 2011, Sasha Levin wrote:
>>
>> Use /dev/block to find the block device used for root
>> instead of searching through mounts.
>>
>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>
> Weren't there distro differences in this area? Prasad, Asias, does this work
> for you?

Ya it works on my machine.

Thanks and Regards,
Prasad
>

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Pekka Enberg]

On Fri, May 6, 2011 at 10:26 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> virtio-blk needs a block of 4 IRQs (currently - staticly defined).
> Giving the initial IRQ of 15 causes breakage when adding more
> than one device.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>

Can we make the allocation dynamic instead? AFAICT, it could be a simple as

  int kvm__request_irq(struct kvm *kvm)
  {
          return kvm->next_irq++;
  }

that's done at device init time and then we'd just add "int irq" to
the individual struct devices.

                        Pekka

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Cyrill Gorcunov]

On 05/06/2011 11:29 AM, Pekka Enberg wrote:
> On Fri, May 6, 2011 at 10:26 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> virtio-blk needs a block of 4 IRQs (currently - staticly defined).
>> Giving the initial IRQ of 15 causes breakage when adding more
>> than one device.
>>
>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> Can we make the allocation dynamic instead? AFAICT, it could be a simple as
> 
>   int kvm__request_irq(struct kvm *kvm)
>   {
>           return kvm->next_irq++;
>   }
> 
> that's done at device init time and then we'd just add "int irq" to
> the individual struct devices.
> 
>                         Pekka

Yes, but start with 0x40 as initial vector for devices please.

-- 
Thanks,
  Cyrill

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Sasha Levin]

On Fri, 2011-05-06 at 10:29 +0300, Pekka Enberg wrote:
> On Fri, May 6, 2011 at 10:26 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> > virtio-blk needs a block of 4 IRQs (currently - staticly defined).
> > Giving the initial IRQ of 15 causes breakage when adding more
> > than one device.
> >
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> Can we make the allocation dynamic instead? AFAICT, it could be a simple as
> 
>   int kvm__request_irq(struct kvm *kvm)
>   {
>           return kvm->next_irq++;
>   }
> 
> that's done at device init time and then we'd just add "int irq" to
> the individual struct devices.
> 
>                         Pekka

afaik, we need to have same IRQ pins for devices which may have
different IRQ lines (All virtio-blk share same IRQ pin but different
line, I assume it'll be same with different virtio devices).

So mptable has to manage association between device type and the
corresponding IRQ pin/line assignment, So dynamic assignment will be
more of a call with device type and return IRQ pin + line - which makes
it more complex than just allocating next free IRQ line.

-- 

Sasha.

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Pekka Enberg]

On Fri, May 6, 2011 at 10:36 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> afaik, we need to have same IRQ pins for devices which may have
> different IRQ lines (All virtio-blk share same IRQ pin but different
> line, I assume it'll be same with different virtio devices).
>
> So mptable has to manage association between device type and the
> corresponding IRQ pin/line assignment, So dynamic assignment will be
> more of a call with device type and return IRQ pin + line - which makes
> it more complex than just allocating next free IRQ line.

Well then make kvm__request_irq() return a pointer to struct irq and
have a hard-coded array of IRQ pin + line pairs in irq.c, for example.
That'll reduce complexity in the virtio drivers and it'll serve as a
starting point for proper IRQ allocator.

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Sasha Levin]

On Fri, 2011-05-06 at 10:43 +0300, Pekka Enberg wrote:
> On Fri, May 6, 2011 at 10:36 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> > afaik, we need to have same IRQ pins for devices which may have
> > different IRQ lines (All virtio-blk share same IRQ pin but different
> > line, I assume it'll be same with different virtio devices).
> >
> > So mptable has to manage association between device type and the
> > corresponding IRQ pin/line assignment, So dynamic assignment will be
> > more of a call with device type and return IRQ pin + line - which makes
> > it more complex than just allocating next free IRQ line.
> 
> Well then make kvm__request_irq() return a pointer to struct irq and
> have a hard-coded array of IRQ pin + line pairs in irq.c, for example.
> That'll reduce complexity in the virtio drivers and it'll serve as a
> starting point for proper IRQ allocator.

The big problem with dynamic allocations is having mptable register IRQ
sources properly, So I went ahead and deleted IRQ source definitions
from mptable.c and noticed that everything still works.

I've spoke with Cyrill and neither him or myself were too sure if it's
ok to do that now and if so, would it still be correct once SMP gets
added.

So here is my question: Would it be ok to drop IRQ source definitions
from mptable?

-- 

Sasha.

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Pekka Enberg]

On Fri, May 6, 2011 at 11:01 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Fri, 2011-05-06 at 10:43 +0300, Pekka Enberg wrote:
>> On Fri, May 6, 2011 at 10:36 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> > afaik, we need to have same IRQ pins for devices which may have
>> > different IRQ lines (All virtio-blk share same IRQ pin but different
>> > line, I assume it'll be same with different virtio devices).
>> >
>> > So mptable has to manage association between device type and the
>> > corresponding IRQ pin/line assignment, So dynamic assignment will be
>> > more of a call with device type and return IRQ pin + line - which makes
>> > it more complex than just allocating next free IRQ line.
>>
>> Well then make kvm__request_irq() return a pointer to struct irq and
>> have a hard-coded array of IRQ pin + line pairs in irq.c, for example.
>> That'll reduce complexity in the virtio drivers and it'll serve as a
>> starting point for proper IRQ allocator.
>
> The big problem with dynamic allocations is having mptable register IRQ
> sources properly, So I went ahead and deleted IRQ source definitions
> from mptable.c and noticed that everything still works.
>
> I've spoke with Cyrill and neither him or myself were too sure if it's
> ok to do that now and if so, would it still be correct once SMP gets
> added.
>
> So here is my question: Would it be ok to drop IRQ source definitions
> from mptable?

Now why do we need to do that? Shouldn't we pass the "device tree" to
mptable.c and build the IRQ mappings based on that? Isn't that exactly
what we do right now with the hard-coded enums?

                       Pekka

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Cyrill Gorcunov]

On 05/06/2011 12:04 PM, Pekka Enberg wrote:
> On Fri, May 6, 2011 at 11:01 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> On Fri, 2011-05-06 at 10:43 +0300, Pekka Enberg wrote:
>>> On Fri, May 6, 2011 at 10:36 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>>>> afaik, we need to have same IRQ pins for devices which may have
>>>> different IRQ lines (All virtio-blk share same IRQ pin but different
>>>> line, I assume it'll be same with different virtio devices).
>>>>
>>>> So mptable has to manage association between device type and the
>>>> corresponding IRQ pin/line assignment, So dynamic assignment will be
>>>> more of a call with device type and return IRQ pin + line - which makes
>>>> it more complex than just allocating next free IRQ line.
>>>
>>> Well then make kvm__request_irq() return a pointer to struct irq and
>>> have a hard-coded array of IRQ pin + line pairs in irq.c, for example.
>>> That'll reduce complexity in the virtio drivers and it'll serve as a
>>> starting point for proper IRQ allocator.
>>
>> The big problem with dynamic allocations is having mptable register IRQ
>> sources properly, So I went ahead and deleted IRQ source definitions
>> from mptable.c and noticed that everything still works.
>>
>> I've spoke with Cyrill and neither him or myself were too sure if it's
>> ok to do that now and if so, would it still be correct once SMP gets
>> added.
>>
>> So here is my question: Would it be ok to drop IRQ source definitions
>> from mptable?
> 
> Now why do we need to do that? Shouldn't we pass the "device tree" to
> mptable.c and build the IRQ mappings based on that? Isn't that exactly
> what we do right now with the hard-coded enums?
> 
>                        Pekka

  Yes, that is what we do, and it seems to be the "right" way how sane bios
should behave, same time (i might be wrong since I didn't checked this
part of kernel sources) the kernel might scan memory for pci configs and
read pis/irq directly from there, I think this way kernel workaround broken
bioses but now sure if we should rely on this feature.

-- 
Thanks,
  Cyrill

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Pekka Enberg]

On Fri, May 6, 2011 at 11:08 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>  Yes, that is what we do, and it seems to be the "right" way how sane bios
> should behave, same time (i might be wrong since I didn't checked this
> part of kernel sources) the kernel might scan memory for pci configs and
> read pis/irq directly from there, I think this way kernel workaround broken
> bioses but now sure if we should rely on this feature.

That's not a problem, though, is it? As long as you use the "device
tree" to set up the PCI configs as well, it's irrelevant where the
kernel picks them up, no?

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Cyrill Gorcunov]

On 05/06/2011 12:10 PM, Pekka Enberg wrote:
> On Fri, May 6, 2011 at 11:08 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>  Yes, that is what we do, and it seems to be the "right" way how sane bios
>> should behave, same time (i might be wrong since I didn't checked this
>> part of kernel sources) the kernel might scan memory for pci configs and
>> read pis/irq directly from there, I think this way kernel workaround broken
>> bioses but now sure if we should rely on this feature.
> 
> That's not a problem, though, is it? As long as you use the "device
> tree" to set up the PCI configs as well, it's irrelevant where the
> kernel picks them up, no?

  Yeah, if we have own "tracking" tool to allocate irqs/pins we put consistent
data into both mptable and pci config space. Which of course means we need
some more code for allocator/tracker ;)

-- 
Thanks,
  Cyrill

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Cyrill Gorcunov]

On 05/06/2011 11:36 AM, Sasha Levin wrote:
> On Fri, 2011-05-06 at 10:29 +0300, Pekka Enberg wrote:
>> On Fri, May 6, 2011 at 10:26 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>>> virtio-blk needs a block of 4 IRQs (currently - staticly defined).
>>> Giving the initial IRQ of 15 causes breakage when adding more
>>> than one device.
>>>
>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>>
>> Can we make the allocation dynamic instead? AFAICT, it could be a simple as
>>
>>   int kvm__request_irq(struct kvm *kvm)
>>   {
>>           return kvm->next_irq++;
>>   }
>>
>> that's done at device init time and then we'd just add "int irq" to
>> the individual struct devices.
>>
>>                         Pekka
> 
> afaik, we need to have same IRQ pins for devices which may have
> different IRQ lines (All virtio-blk share same IRQ pin but different
> line, I assume it'll be same with different virtio devices).
> 
> So mptable has to manage association between device type and the
> corresponding IRQ pin/line assignment, So dynamic assignment will be
> more of a call with device type and return IRQ pin + line - which makes
> it more complex than just allocating next free IRQ line.
> 

  Nod, making it dynamic force us to implement some kind of collector for
pci devices, so that it would know pin/vector pair for every device. So
in device init time we need to call kinda mptable_request_irq_pin()
where we could "hint" allocator which pin and irq we need, and say "exclusive"
flag which would tell allocator to fail if irq/pin already borrowed.

  The "hints" for every device might left hardcoded in some enum, for other
irqs we might need pure dynamic vector allocator.

-- 
Thanks,
  Cyrill

Re: [PATCH 1/2] kvm tools: Fix IRQ assignments

[Pekka Enberg]

On Fri, May 6, 2011 at 10:47 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>  Nod, making it dynamic force us to implement some kind of collector for
> pci devices, so that it would know pin/vector pair for every device. So
> in device init time we need to call kinda mptable_request_irq_pin()
> where we could "hint" allocator which pin and irq we need, and say "exclusive"
> flag which would tell allocator to fail if irq/pin already borrowed.
>
>  The "hints" for every device might left hardcoded in some enum, for other
> irqs we might need pure dynamic vector allocator.

That's a separate problem, I think. What I'm proposing is moving the
complexity from the drivers to a centralized irq.c. We can still have
hard-coded pin/line pairs but we'd let drivers take advantage of them
in a more dynamic way.

                        Pekka