| Message ID | 1312831898-18702-1-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 08/08/2011 02:31 PM, Anthony Liguori wrote: > We've always listened on port 501 for vgabios panic messages. In the entire > time I've worked on QEMU, I've never actually seen a vgabios panic message :-) > > If we change the semantics of this port a little bit, it makes it possible to > use it for more interesting use-cases. I chose this approach instead of adding > a new I/O port because it avoids having a guest visible change. > > This change allows single-byte access to port 501 and also uses the value > written to construct an exit code. > > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> Applied. Regards, Anthony Liguori > --- > hw/pc.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index 1c9d89a..4b07b35 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -549,8 +549,7 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) > /* LGPL'ed VGA BIOS messages */ > case 0x501: > case 0x502: > - fprintf(stderr, "VGA BIOS panic, line %d\n", val); > - exit(1); > + exit((val<< 1) | 1); > case 0x500: > case 0x503: > #ifdef DEBUG_BIOS > @@ -591,6 +590,7 @@ static void *bochs_bios_init(void) > register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL); > register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL); > > + register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL); > register_ioport_write(0x501, 1, 2, bochs_bios_write, NULL); > register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL); > register_ioport_write(0x500, 1, 1, bochs_bios_write, NULL);
On 08/08/2011 10:31 PM, Anthony Liguori wrote: > We've always listened on port 501 for vgabios panic messages. In the entire > time I've worked on QEMU, I've never actually seen a vgabios panic message :-) > > If we change the semantics of this port a little bit, it makes it possible to > use it for more interesting use-cases. I chose this approach instead of adding > a new I/O port because it avoids having a guest visible change. > > This change allows single-byte access to port 501 and also uses the value > written to construct an exit code. A little late to review but... > > diff --git a/hw/pc.c b/hw/pc.c > index 1c9d89a..4b07b35 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -549,8 +549,7 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) > /* LGPL'ed VGA BIOS messages */ > case 0x501: > case 0x502: > - fprintf(stderr, "VGA BIOS panic, line %d\n", val); > - exit(1); > + exit((val<< 1) | 1); This code (before the patch) circumvents -no-shutdown. Shifting val left is surprising. What's wrong with even exit codes?
On 08/24/2011 04:47 AM, Avi Kivity wrote: > On 08/08/2011 10:31 PM, Anthony Liguori wrote: >> We've always listened on port 501 for vgabios panic messages. In the >> entire >> time I've worked on QEMU, I've never actually seen a vgabios panic >> message :-) >> >> If we change the semantics of this port a little bit, it makes it >> possible to >> use it for more interesting use-cases. I chose this approach instead >> of adding >> a new I/O port because it avoids having a guest visible change. >> >> This change allows single-byte access to port 501 and also uses the value >> written to construct an exit code. > > A little late to review but... > >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 1c9d89a..4b07b35 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -549,8 +549,7 @@ static void bochs_bios_write(void *opaque, >> uint32_t addr, uint32_t val) >> /* LGPL'ed VGA BIOS messages */ >> case 0x501: >> case 0x502: >> - fprintf(stderr, "VGA BIOS panic, line %d\n", val); >> - exit(1); >> + exit((val<< 1) | 1); > > This code (before the patch) circumvents -no-shutdown. Indeed. I believe that's a feature though? > > Shifting val left is surprising. What's wrong with even exit codes? Hrm, the '| 1' is wrong. My intention was to make the bottom bit of the exit code mean "if set to 0, this is an exit coming from a guest'. I'll spin a patch. Regards, Anthony Liguori >
On 08/24/2011 03:38 PM, Anthony Liguori wrote: >> >>> >>> diff --git a/hw/pc.c b/hw/pc.c >>> index 1c9d89a..4b07b35 100644 >>> --- a/hw/pc.c >>> +++ b/hw/pc.c >>> @@ -549,8 +549,7 @@ static void bochs_bios_write(void *opaque, >>> uint32_t addr, uint32_t val) >>> /* LGPL'ed VGA BIOS messages */ >>> case 0x501: >>> case 0x502: >>> - fprintf(stderr, "VGA BIOS panic, line %d\n", val); >>> - exit(1); >>> + exit((val<< 1) | 1); >> >> This code (before the patch) circumvents -no-shutdown. > > > Indeed. I believe that's a feature though? Depends on what the user of -no-shutdown expects. > >> >> Shifting val left is surprising. What's wrong with even exit codes? > > Hrm, the '| 1' is wrong. My intention was to make the bottom bit of > the exit code mean "if set to 0, this is an exit coming from a guest'. Too subtle, IMO. I understand that we want to avoid a full qmp parser for one-off unit tests, but using bit fields in the exit code? Perhaps a python script that launches qemu with qmp and -no-shutdown, listens for the guest shutdown event, and prints out the result? That can be easily reused in test scripts.
On 08/24/2011 07:55 AM, Avi Kivity wrote: > On 08/24/2011 03:38 PM, Anthony Liguori wrote: >>> >>>> >>>> diff --git a/hw/pc.c b/hw/pc.c >>>> index 1c9d89a..4b07b35 100644 >>>> --- a/hw/pc.c >>>> +++ b/hw/pc.c >>>> @@ -549,8 +549,7 @@ static void bochs_bios_write(void *opaque, >>>> uint32_t addr, uint32_t val) >>>> /* LGPL'ed VGA BIOS messages */ >>>> case 0x501: >>>> case 0x502: >>>> - fprintf(stderr, "VGA BIOS panic, line %d\n", val); >>>> - exit(1); >>>> + exit((val<< 1) | 1); >>> >>> This code (before the patch) circumvents -no-shutdown. >> >> >> Indeed. I believe that's a feature though? > > Depends on what the user of -no-shutdown expects. > >> >>> >>> Shifting val left is surprising. What's wrong with even exit codes? >> >> Hrm, the '| 1' is wrong. My intention was to make the bottom bit of >> the exit code mean "if set to 0, this is an exit coming from a guest'. > > Too subtle, IMO. I understand that we want to avoid a full qmp parser > for one-off unit tests, but using bit fields in the exit code? > > Perhaps a python script that launches qemu with qmp and -no-shutdown, > listens for the guest shutdown event, and prints out the result? That > can be easily reused in test scripts. How can the test pass data via shutdown? You would need a scratch register of some form I think... Regards, Anthony Liguori >
On 08/24/2011 04:02 PM, Anthony Liguori wrote: >> Too subtle, IMO. I understand that we want to avoid a full qmp parser >> for one-off unit tests, but using bit fields in the exit code? >> >> Perhaps a python script that launches qemu with qmp and -no-shutdown, >> listens for the guest shutdown event, and prints out the result? That >> can be easily reused in test scripts. > > > How can the test pass data via shutdown? You would need a scratch > register of some form I think... I meant using the port in the patch. Just replace exit() with the ordinary shutdown path - notifications, and not exiting if -no-shutdown is requested.
diff --git a/hw/pc.c b/hw/pc.c index 1c9d89a..4b07b35 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -549,8 +549,7 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) /* LGPL'ed VGA BIOS messages */ case 0x501: case 0x502: - fprintf(stderr, "VGA BIOS panic, line %d\n", val); - exit(1); + exit((val << 1) | 1); case 0x500: case 0x503: #ifdef DEBUG_BIOS @@ -591,6 +590,7 @@ static void *bochs_bios_init(void) register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL); register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL); + register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL); register_ioport_write(0x501, 1, 2, bochs_bios_write, NULL); register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL); register_ioport_write(0x500, 1, 1, bochs_bios_write, NULL);
We've always listened on port 501 for vgabios panic messages. In the entire time I've worked on QEMU, I've never actually seen a vgabios panic message :-) If we change the semantics of this port a little bit, it makes it possible to use it for more interesting use-cases. I chose this approach instead of adding a new I/O port because it avoids having a guest visible change. This change allows single-byte access to port 501 and also uses the value written to construct an exit code. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/pc.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)