[Qemu-devel] [PATCH] monitor: Add port write command

[Qemu-devel] [PATCH] monitor: Add port write command

[Jan Kiszka]

Useful for testing hardware emulations or manipulating its state to
stress guest drivers.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 monitor.c       |   19 +++++++++++++++++++
 qemu-monitor.hx |    5 +++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 342af32..2ddca95 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1185,6 +1185,25 @@ static void do_ioport_read(Monitor *mon, int count, int format, int size,
                    suffix, addr, size * 2, val);
 }
 
+static void do_ioport_write(Monitor *mon, int count, int format, int size,
+                            int addr, int val)
+{
+    addr &= IOPORTS_MASK;
+
+    switch (size) {
+    default:
+    case 1:
+        cpu_outb(NULL, addr, val);
+        break;
+    case 2:
+        cpu_outw(NULL, addr, val);
+        break;
+    case 4:
+        cpu_outl(NULL, addr, val);
+        break;
+    }
+}
+
 /* boot_set handler */
 static QEMUBootSetHandler *qemu_boot_set_handler = NULL;
 static void *boot_opaque;
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 62edbcd..70e2475 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -306,6 +306,11 @@ STEXI
 Read I/O port.
 ETEXI
 
+    { "o", "/ii", do_ioport_write,
+      "/fmt addr value", "I/O port write" },
+STEXI
+Write to I/O port.
+ETEXI
 
     { "sendkey", "si?", do_sendkey,
       "keys [hold_ms]", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1', default hold time=100 ms)" },

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Anthony Liguori]

Jan Kiszka wrote:
> Useful for testing hardware emulations or manipulating its state to
> stress guest drivers.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>   

Patch looks good.

Makes me think of an idea I had a while ago.  It's a rather radical 
change but I think we could target it for 0.12.  Today, the vast 
majority of our save/restore code looks like:

save:
// version checks
qemu_put_type(f, &field);
...

load:
// version checks
qemu_get_type(f, &field);
...
// optional depending on version
qemu_get_type(f, &field);

device_reset(s);

We could convert this to something this to be largely data drive.  For 
instance,

SaveVMFields fields[] = {
 { offsetof(DeviceState, field), "field", TYPE, VERSION },
 {},
};

SaveVMDescription desc = {
  .fields = fields,
  .version = CURRENT_VERSION,
  .reset = device_reset,
};

What would be really cool about this change is that we could introduce a 
new set of commands to manipulate device state.  We could save/restore 
individual device state and that would allow us to dump device state via 
the monitor and to manipulate individual fields of the device state.  I 
think this could be pretty useful for debugging.

I'm curious if anyone else is interested in this sort of change.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] monitor: Add port write command

[Anthony Liguori]

Juan Quintela wrote:
> We were thinking about that functionality some time ago.  We were
> looking at an apic bug during migration, and it was not trivial to dump
> apic state before and after migration.
>
> I took a look at it, and basically decided that I would have to
> replicate the savevm method.  Your apprearch mean that doing the command
> is trivial, and we have "meaningful" labels for it.
>
> Will take a look at using that in some driver.
>   

Great.  I think an incremental conversion is the right approach.  If we 
tie this into qdev correctly, then we could also do some nice things 
with migration.  We could dump out a file that describes any given 
migration protocol version.  We could then use this to automatically 
check whether we broke the migration format between versions but just 
doing a diff.

I think there's also an opportunity to adjust the migration protocol.  
One change I had to make to the savevm format was removing all of the 
size parameters.  This is because the nature of QEMUFile makes it such 
that you don't know what the size was until you actually wrote data.  
The old savevm format used to rely on seeking to update the size 
parameter.  If we know the size of the section (based on the 
description), we can reintroduce this to the protocol.

What's more, we can probably get rid of QEMUFile all together and 
introduce a more useful abstraction that doesn't have such complex 
buffering.

This refactoring is definitely high on my 0.12 wish list :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Blue Swirl]

On 7/14/09, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Jan Kiszka wrote:
>
> > Useful for testing hardware emulations or manipulating its state to
> > stress guest drivers.
> >
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >
> >
>
>  Patch looks good.
>
>  Makes me think of an idea I had a while ago.  It's a rather radical change
> but I think we could target it for 0.12.  Today, the vast majority of our
> save/restore code looks like:
>
>  save:
>  // version checks
>  qemu_put_type(f, &field);
>  ...
>
>  load:
>  // version checks
>  qemu_get_type(f, &field);
>  ...
>  // optional depending on version
>  qemu_get_type(f, &field);
>
>  device_reset(s);
>
>  We could convert this to something this to be largely data drive.  For
> instance,
>
>  SaveVMFields fields[] = {
>  { offsetof(DeviceState, field), "field", TYPE, VERSION },
>  {},
>  };
>
>  SaveVMDescription desc = {
>   .fields = fields,
>   .version = CURRENT_VERSION,
>   .reset = device_reset,
>  };
>
>  What would be really cool about this change is that we could introduce a
> new set of commands to manipulate device state.  We could save/restore
> individual device state and that would allow us to dump device state via the
> monitor and to manipulate individual fields of the device state.  I think
> this could be pretty useful for debugging.
>
>  I'm curious if anyone else is interested in this sort of change.

Excellent idea!

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Paul Brook]

> What would be really cool about this change is that we could introduce a
> new set of commands to manipulate device state.  We could save/restore
> individual device state and that would allow us to dump device state via
> the monitor and to manipulate individual fields of the device state.  I
> think this could be pretty useful for debugging.

I'd be reluctant to expose the savevm state to the user.

For debugging qemu I don't see it providing any real benefit over firing up 
GDB and poking directly at the device directly.

For debugging the guest there is some argument for exposing the device state 
(preferably via the gdb stub). However I think the savevm data tends to expose 
too many internal implementation details, rather than the interesting guest 
state. For most devices I'd expect the IO regions to be a better fit, 
especially if you want to prevent qemu crashing when the user does invalid 
writes.

Paul

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Anthony Liguori]

Paul Brook wrote:
>> What would be really cool about this change is that we could introduce a
>> new set of commands to manipulate device state.  We could save/restore
>> individual device state and that would allow us to dump device state via
>> the monitor and to manipulate individual fields of the device state.  I
>> think this could be pretty useful for debugging.
>>     
>
> I'd be reluctant to expose the savevm state to the user.
>
> For debugging qemu I don't see it providing any real benefit over firing up 
> GDB and poking directly at the device directly.
>
> For debugging the guest there is some argument for exposing the device state 
> (preferably via the gdb stub). However I think the savevm data tends to expose 
> too many internal implementation details,

That sounds like an issue with the devices.

The savevm state should not contain internal implementation details as 
that makes backwards compatibility hard to maintain.  I think for a 
large class of devices, the savevm state almost perfectly matches the 
device register state and having a nice interface to that seems like a 
useful thing.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] monitor: Add port write command

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

Anthony Liguori wrote:
> Paul Brook wrote:
>>> What would be really cool about this change is that we could introduce a
>>> new set of commands to manipulate device state.  We could save/restore
>>> individual device state and that would allow us to dump device state via
>>> the monitor and to manipulate individual fields of the device state.  I
>>> think this could be pretty useful for debugging.
>>>     
>>
>> I'd be reluctant to expose the savevm state to the user.
>>
>> For debugging qemu I don't see it providing any real benefit over
>> firing up GDB and poking directly at the device directly.
>>
>> For debugging the guest there is some argument for exposing the device
>> state (preferably via the gdb stub). However I think the savevm data
>> tends to expose too many internal implementation details,
> 
> That sounds like an issue with the devices.
> 
> The savevm state should not contain internal implementation details as
> that makes backwards compatibility hard to maintain.  I think for a
> large class of devices, the savevm state almost perfectly matches the
> device register state and having a nice interface to that seems like a
> useful thing.
> 

I agree that most (but maybe not all) of the saved device states consist
of register or register-like states that could nicely be presented to
the user or even manipulated. To avoid illicit manipulations, we could
add stricter test to loadvm handlers or define valid ranges of the
fields. Or we could even mark certain fields as read-only or even
internal (ie. hidden).

In any case, such an interface would be a great improvement over having
to debug qemu itself! If you are unfamiliar with the qemu design, this
is surely no straightforward method for obtaining information about the
virtual device states...

Will see if I or some colleague can contribute to this effort.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Gleb Natapov]

On Tue, Jul 14, 2009 at 08:30:25PM +0100, Paul Brook wrote:
> > What would be really cool about this change is that we could introduce a
> > new set of commands to manipulate device state.  We could save/restore
> > individual device state and that would allow us to dump device state via
> > the monitor and to manipulate individual fields of the device state.  I
> > think this could be pretty useful for debugging.
> 
> I'd be reluctant to expose the savevm state to the user.
> 
> For debugging qemu I don't see it providing any real benefit over firing up 
> GDB and poking directly at the device directly.
> 
Not all environments where you need to debug things have gdb, qemu
sources or even non striped qemu binary.

> For debugging the guest there is some argument for exposing the device state 
> (preferably via the gdb stub). However I think the savevm data tends to expose 
> too many internal implementation details, rather than the interesting guest 
> state. For most devices I'd expect the IO regions to be a better fit, 
> especially if you want to prevent qemu crashing when the user does invalid 
> writes.
> 
> Paul
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Paul Brook]

> > I'd be reluctant to expose the savevm state to the user.
> >
> > For debugging qemu I don't see it providing any real benefit over firing
> > up GDB and poking directly at the device directly.
>
> Not all environments where you need to debug things have gdb, qemu
> sources or even non striped qemu binary.

If you don't have qemu sources than I really don't care. By definition you're 
not going to be able to do anything useful even if you do figure out what the 
problem is.  Note that there's no requirement that you run gdb on the target 
itself. Remote debug (e.g. via gdbserver on linux) is a well established 
technique.

Likewise for debugging stripped production binaries, my answer is "don't do 
that". There are very rare cases where a bug goes away on a debug build, but 
in those cases any instrumentation you add is also liable to make the bug go 
away.

Paul

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Jan Kiszka]

Paul Brook wrote:
>>> I'd be reluctant to expose the savevm state to the user.
>>>
>>> For debugging qemu I don't see it providing any real benefit over firing
>>> up GDB and poking directly at the device directly.
>> Not all environments where you need to debug things have gdb, qemu
>> sources or even non striped qemu binary.
> 
> If you don't have qemu sources than I really don't care. By definition you're 
> not going to be able to do anything useful even if you do figure out what the 
> problem is.  Note that there's no requirement that you run gdb on the target 
> itself. Remote debug (e.g. via gdbserver on linux) is a well established 
> technique.
> 
> Likewise for debugging stripped production binaries, my answer is "don't do 
> that". There are very rare cases where a bug goes away on a debug build, but 
> in those cases any instrumentation you add is also liable to make the bug go 
> away.

Again, having to debug qemu itself when all you want is some additional
information about the emulated hardware state while debugging a guest
seems like overkill to me. Imagine you are attached to qemu's gdbstub
and stopped the guest that has some issue using, say, the apic. And now
imagine you could simply issue e.g.

(gdb) monitor devstate apic.1
apicbase=...
tpr=...
icr={..., ... }
lvt={..., ... }
...

That would provide you a nice picture of the virtual hardware state
without the need to fire up and operate a second gdb just to obtain this
data. And manipulating some lvt register would only be a trivial
additional monitor command away.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Gleb Natapov]

On Wed, Jul 15, 2009 at 12:40:49PM +0200, Jan Kiszka wrote:
> Paul Brook wrote:
> >>> I'd be reluctant to expose the savevm state to the user.
> >>>
> >>> For debugging qemu I don't see it providing any real benefit over firing
> >>> up GDB and poking directly at the device directly.
> >> Not all environments where you need to debug things have gdb, qemu
> >> sources or even non striped qemu binary.
> > 
> > If you don't have qemu sources than I really don't care. By definition you're 
> > not going to be able to do anything useful even if you do figure out what the 
> > problem is.  Note that there's no requirement that you run gdb on the target 
> > itself. Remote debug (e.g. via gdbserver on linux) is a well established 
> > technique.
> > 
> > Likewise for debugging stripped production binaries, my answer is "don't do 
> > that". There are very rare cases where a bug goes away on a debug build, but 
> > in those cases any instrumentation you add is also liable to make the bug go 
> > away.
> 
> Again, having to debug qemu itself when all you want is some additional
> information about the emulated hardware state while debugging a guest
> seems like overkill to me. Imagine you are attached to qemu's gdbstub
> and stopped the guest that has some issue using, say, the apic. And now
> imagine you could simply issue e.g.
> 
> (gdb) monitor devstate apic.1
> apicbase=...
> tpr=...
> icr={..., ... }
> lvt={..., ... }
> ...
> 
> That would provide you a nice picture of the virtual hardware state
> without the need to fire up and operate a second gdb just to obtain this
> data. And manipulating some lvt register would only be a trivial
> additional monitor command away.
> 
apic/ioapic is important enough to warrant "info apic" "info ioapic"
commands.

--
			Gleb.

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Gleb Natapov]

On Wed, Jul 15, 2009 at 11:14:20AM +0100, Paul Brook wrote:
> > > I'd be reluctant to expose the savevm state to the user.
> > >
> > > For debugging qemu I don't see it providing any real benefit over firing
> > > up GDB and poking directly at the device directly.
> >
> > Not all environments where you need to debug things have gdb, qemu
> > sources or even non striped qemu binary.
> 
> If you don't have qemu sources than I really don't care. By definition you're 
I noticed that you don't care, but I do.

> not going to be able to do anything useful even if you do figure out what the 
> problem is.  Note that there's no requirement that you run gdb on the target 
I will be able to fix the bug if I'll figuring out what the problem is,
so I don't see how this is not useful.

> itself. Remote debug (e.g. via gdbserver on linux) is a well established 
> technique.
> 
Client will not allow me near his infrastructure, he is nice enough to
dump device state for me if it's simple.

> Likewise for debugging stripped production binaries, my answer is "don't do 
> that". There are very rare cases where a bug goes away on a debug build, but 
> in those cases any instrumentation you add is also liable to make the bug go 
> away.
> 
Have you sees a real production environment and tried to resolve real
customer bugs? Just asking.

--
			Gleb.

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Anthony Liguori]

Paul Brook wrote:
> If you don't have qemu sources than I really don't care. By definition you're 
> not going to be able to do anything useful even if you do figure out what the 
> problem is.  Note that there's no requirement that you run gdb on the target 
> itself. Remote debug (e.g. via gdbserver on linux) is a well established 
> technique.
>
> Likewise for debugging stripped production binaries, my answer is "don't do 
> that". There are very rare cases where a bug goes away on a debug build, but 
> in those cases any instrumentation you add is also liable to make the bug go 
> away.
>   

If we had an info device, it would fall into the same category as info 
cpu or info registers.  It duplicates the functionality of the gdbstub 
but it provides a convenience interface for those who don't have gdb 
connected.

Since there isn't really an obvious way to expose the device state via 
gdb without using Rcmd and a new monitor command, the whole debate seems 
academic :-)

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] monitor: Add port write command

[Paul Brook]

> Since there isn't really an obvious way to expose the device state via
> gdb without using Rcmd and a new monitor command, the whole debate seems
> academic :-)

The gdb bits for exposing arbitrary peripheral state do exist, though they 
aren't yet in mainline FSF gdb. Hopefully they'll be merged before too long.

Paul