diff mbox

[RFC,04/11] kvm tools: console: unconditionally output to any console

Message ID 1367423416-24640-5-git-send-email-will.deacon@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Will Deacon May 1, 2013, 3:50 p.m. UTC
From: Marc Zyngier <Marc.Zyngier@arm.com>

Kvmtool suppresses any output to a console that has not been elected
as *the* console.

While this makes sense on the input side (we want the input to be sent
to one console driver only), it seems to be the wrong thing to do on
the output side, as it effectively prevents the guest from switching
from one console to another (think earlyprintk using 8250 to virtio
console).

After all, the guest *does* poke this device and outputs something
there.

Just remove the kvm->cfg.active_console test from the output paths.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 tools/kvm/hw/serial.c            | 3 +--
 tools/kvm/powerpc/spapr_hvcons.c | 5 +----
 tools/kvm/powerpc/spapr_rtas.c   | 3 +--
 tools/kvm/virtio/console.c       | 5 +----
 4 files changed, 4 insertions(+), 12 deletions(-)

Comments

Pekka Enberg May 3, 2013, 9:19 a.m. UTC | #1
On Wed, May 1, 2013 at 6:50 PM, Will Deacon <will.deacon@arm.com> wrote:
> From: Marc Zyngier <Marc.Zyngier@arm.com>
>
> Kvmtool suppresses any output to a console that has not been elected
> as *the* console.
>
> While this makes sense on the input side (we want the input to be sent
> to one console driver only), it seems to be the wrong thing to do on
> the output side, as it effectively prevents the guest from switching
> from one console to another (think earlyprintk using 8250 to virtio
> console).
>
> After all, the guest *does* poke this device and outputs something
> there.
>
> Just remove the kvm->cfg.active_console test from the output paths.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Seems reasonable. Asias, Sasha?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin May 3, 2013, 4:02 p.m. UTC | #2
On 05/03/2013 05:19 AM, Pekka Enberg wrote:
> On Wed, May 1, 2013 at 6:50 PM, Will Deacon <will.deacon@arm.com> wrote:
>> From: Marc Zyngier <Marc.Zyngier@arm.com>
>>
>> Kvmtool suppresses any output to a console that has not been elected
>> as *the* console.
>>
>> While this makes sense on the input side (we want the input to be sent
>> to one console driver only), it seems to be the wrong thing to do on
>> the output side, as it effectively prevents the guest from switching
>> from one console to another (think earlyprintk using 8250 to virtio
>> console).
>>
>> After all, the guest *does* poke this device and outputs something
>> there.
>>
>> Just remove the kvm->cfg.active_console test from the output paths.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Seems reasonable. Asias, Sasha?
> 

I remember at trying it some time ago but dropped it for a reason I don't
remember at the moment.

Can I have the weekend to play with it to try and figure out why?


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Will Deacon May 3, 2013, 4:09 p.m. UTC | #3
On Fri, May 03, 2013 at 05:02:14PM +0100, Sasha Levin wrote:
> On 05/03/2013 05:19 AM, Pekka Enberg wrote:
> > On Wed, May 1, 2013 at 6:50 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> From: Marc Zyngier <Marc.Zyngier@arm.com>
> >>
> >> Kvmtool suppresses any output to a console that has not been elected
> >> as *the* console.
> >>
> >> While this makes sense on the input side (we want the input to be sent
> >> to one console driver only), it seems to be the wrong thing to do on
> >> the output side, as it effectively prevents the guest from switching
> >> from one console to another (think earlyprintk using 8250 to virtio
> >> console).
> >>
> >> After all, the guest *does* poke this device and outputs something
> >> there.
> >>
> >> Just remove the kvm->cfg.active_console test from the output paths.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> Signed-off-by: Will Deacon <will.deacon@arm.com>
> > 
> > Seems reasonable. Asias, Sasha?
> > 
> 
> I remember at trying it some time ago but dropped it for a reason I don't
> remember at the moment.
> 
> Can I have the weekend to play with it to try and figure out why?

There's no rush from my point of view (hence the RFC) so take as long as you
need!

Will
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Asias He May 5, 2013, 10:16 a.m. UTC | #4
On Fri, May 3, 2013 at 5:19 PM, Pekka Enberg <penberg@kernel.org> wrote:
> On Wed, May 1, 2013 at 6:50 PM, Will Deacon <will.deacon@arm.com> wrote:
>> From: Marc Zyngier <Marc.Zyngier@arm.com>
>>
>> Kvmtool suppresses any output to a console that has not been elected
>> as *the* console.
>>
>> While this makes sense on the input side (we want the input to be sent
>> to one console driver only), it seems to be the wrong thing to do on
>> the output side, as it effectively prevents the guest from switching
>> from one console to another (think earlyprintk using 8250 to virtio
>> console).
>>
>> After all, the guest *does* poke this device and outputs something
>> there.
>>
>> Just remove the kvm->cfg.active_console test from the output paths.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>
> Seems reasonable. Asias, Sasha?

This patch itself looks good to me.

But we have more issues for the console devices and termials with
regard to multiple console support:

1) All the console outputs (spapr_hvcons.c, spapr_rtas.c
virtio/console.c) are redirected to term 0.
2) With multiple console support, the cfg.active_console logic is not
very useful at all.
3) Four serial devices ttyS0-3 are initialized unconditionally and
mapped to term 0-3.
4) Using --tty option, we can map a term to /dev/pts/N on host. I
think we can merge --tty option to --console option.

I have something like this in my mind:

--console type=serial,backend=stdio
--console type=virtio,backend=pts
--console type=hv,backend=pts

e.g to add two serial consoles ttyS0 and ttyS1 and one virtio console
hvc0, ttyS0 is mapped the stdio and ttyS1 and hvc0 are mapped to pts,
we use this:

--console type=serial,backend=stdio  --console type=serial,backend=pts
 --console type=virtio,backend=pts


> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
Asias
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin May 6, 2013, 9:04 p.m. UTC | #5
On 05/03/2013 12:09 PM, Will Deacon wrote:
> On Fri, May 03, 2013 at 05:02:14PM +0100, Sasha Levin wrote:
>> On 05/03/2013 05:19 AM, Pekka Enberg wrote:
>>> On Wed, May 1, 2013 at 6:50 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>> From: Marc Zyngier <Marc.Zyngier@arm.com>
>>>>
>>>> Kvmtool suppresses any output to a console that has not been elected
>>>> as *the* console.
>>>>
>>>> While this makes sense on the input side (we want the input to be sent
>>>> to one console driver only), it seems to be the wrong thing to do on
>>>> the output side, as it effectively prevents the guest from switching
>>>> from one console to another (think earlyprintk using 8250 to virtio
>>>> console).
>>>>
>>>> After all, the guest *does* poke this device and outputs something
>>>> there.
>>>>
>>>> Just remove the kvm->cfg.active_console test from the output paths.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>>>
>>> Seems reasonable. Asias, Sasha?
>>>
>>
>> I remember at trying it some time ago but dropped it for a reason I don't
>> remember at the moment.
>>
>> Can I have the weekend to play with it to try and figure out why?
> 
> There's no rush from my point of view (hence the RFC) so take as long as you
> need!

Looks good to me!


Thanks,
Sasha

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anup Patel May 7, 2013, 2:48 a.m. UTC | #6
On Tue, May 7, 2013 at 2:34 AM, Sasha Levin <sasha.levin@oracle.com> wrote:
> On 05/03/2013 12:09 PM, Will Deacon wrote:
>> On Fri, May 03, 2013 at 05:02:14PM +0100, Sasha Levin wrote:
>>> On 05/03/2013 05:19 AM, Pekka Enberg wrote:
>>>> On Wed, May 1, 2013 at 6:50 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>>> From: Marc Zyngier <Marc.Zyngier@arm.com>
>>>>>
>>>>> Kvmtool suppresses any output to a console that has not been elected
>>>>> as *the* console.
>>>>>
>>>>> While this makes sense on the input side (we want the input to be sent
>>>>> to one console driver only), it seems to be the wrong thing to do on
>>>>> the output side, as it effectively prevents the guest from switching
>>>>> from one console to another (think earlyprintk using 8250 to virtio
>>>>> console).
>>>>>
>>>>> After all, the guest *does* poke this device and outputs something
>>>>> there.
>>>>>
>>>>> Just remove the kvm->cfg.active_console test from the output paths.
>>>>>
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>>>>
>>>> Seems reasonable. Asias, Sasha?
>>>>
>>>
>>> I remember at trying it some time ago but dropped it for a reason I don't
>>> remember at the moment.
>>>
>>> Can I have the weekend to play with it to try and figure out why?
>>
>> There's no rush from my point of view (hence the RFC) so take as long as you
>> need!
>
> Looks good to me!
>
>
> Thanks,
> Sasha
>

I am fine with having 8250 emulated by KVMTOOL, but I am more inclined towards
having a full para-virtualized (PV) machine emulated by KVMTOOL.

Best Regards,
Anup
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/tools/kvm/hw/serial.c b/tools/kvm/hw/serial.c
index 53b684a..18bf569 100644
--- a/tools/kvm/hw/serial.c
+++ b/tools/kvm/hw/serial.c
@@ -100,8 +100,7 @@  static void serial8250_flush_tx(struct kvm *kvm, struct serial8250_device *dev)
 	dev->lsr |= UART_LSR_TEMT | UART_LSR_THRE;
 
 	if (dev->txcnt) {
-		if (kvm->cfg.active_console == CONSOLE_8250)
-			term_putc(dev->txbuf, dev->txcnt, dev->id);
+		term_putc(dev->txbuf, dev->txcnt, dev->id);
 		dev->txcnt = 0;
 	}
 }
diff --git a/tools/kvm/powerpc/spapr_hvcons.c b/tools/kvm/powerpc/spapr_hvcons.c
index 0bdf75b..605367b 100644
--- a/tools/kvm/powerpc/spapr_hvcons.c
+++ b/tools/kvm/powerpc/spapr_hvcons.c
@@ -50,10 +50,7 @@  static unsigned long h_put_term_char(struct kvm_cpu *vcpu, unsigned long opcode,
 	do {
 		int ret;
 
-		if (vcpu->kvm->cfg.active_console == CONSOLE_HV)
-			ret = term_putc_iov(&iov, 1, 0);
-		else
-			ret = 0;
+		ret = term_putc_iov(&iov, 1, 0);
 		if (ret < 0) {
 			die("term_putc_iov error %d!\n", errno);
 		}
diff --git a/tools/kvm/powerpc/spapr_rtas.c b/tools/kvm/powerpc/spapr_rtas.c
index c81d82b..f3c8fa3 100644
--- a/tools/kvm/powerpc/spapr_rtas.c
+++ b/tools/kvm/powerpc/spapr_rtas.c
@@ -53,8 +53,7 @@  static void rtas_put_term_char(struct kvm_cpu *vcpu,
 {
 	char c = rtas_ld(vcpu->kvm, args, 0);
 
-	if (vcpu->kvm->cfg.active_console == CONSOLE_HV)
-		term_putc(&c, 1, 0);
+	term_putc(&c, 1, 0);
 
 	rtas_st(vcpu->kvm, rets, 0, 0);
 }
diff --git a/tools/kvm/virtio/console.c b/tools/kvm/virtio/console.c
index b18d3a9..1f88a4b 100644
--- a/tools/kvm/virtio/console.c
+++ b/tools/kvm/virtio/console.c
@@ -102,10 +102,7 @@  static void virtio_console_handle_callback(struct kvm *kvm, void *param)
 
 	while (virt_queue__available(vq)) {
 		head = virt_queue__get_iov(vq, iov, &out, &in, kvm);
-		if (kvm->cfg.active_console == CONSOLE_VIRTIO)
-			len = term_putc_iov(iov, out, 0);
-		else
-			len = 0;
+		len = term_putc_iov(iov, out, 0);
 		virt_queue__set_used_elem(vq, head, len);
 	}