Patchwork [v16] Add pvpanic device driver

login
register
mail settings
Submitter Hu Tao
Date March 29, 2013, 8:18 a.m.
Message ID <1364545124-9781-1-git-send-email-hutao@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/232312/
State New
Headers show

Comments

Hu Tao - March 29, 2013, 8:18 a.m.
pvpanic device is used to notify host(qemu) when guest panic happens.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 src/acpi.c        |  3 +++
 src/ssdt-misc.dsl | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
Paolo Bonzini - March 29, 2013, 9:28 a.m.
Il 29/03/2013 09:18, Hu Tao ha scritto:
> pvpanic device is used to notify host(qemu) when guest panic happens.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  src/acpi.c        |  3 +++
>  src/ssdt-misc.dsl | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/src/acpi.c b/src/acpi.c
> index bc4d8ea..fe504f0 100644
> --- a/src/acpi.c
> +++ b/src/acpi.c
> @@ -534,6 +534,9 @@ build_ssdt(void)
>          ssdt_ptr[acpi_pci64_valid[0]] = 0;
>      }
>  
> +    int pvpanic_port = romfile_loadint("etc/pvpanic-port", 0x0);
> +    *(u16 *)(ssdt_ptr + *ssdt_isa_pest) = pvpanic_port;
> +
>      ssdt_ptr += sizeof(ssdp_misc_aml);
>  
>      // build Scope(_SB_) header
> diff --git a/src/ssdt-misc.dsl b/src/ssdt-misc.dsl
> index 679422b..acc850e 100644
> --- a/src/ssdt-misc.dsl
> +++ b/src/ssdt-misc.dsl
> @@ -55,4 +55,50 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
>              Zero   /* reserved */
>          })
>      }
> +
> +    External(\_SB.PCI0, DeviceObj)
> +    External(\_SB.PCI0.ISA, DeviceObj)
> +
> +    Scope(\_SB.PCI0.ISA) {
> +        Device(PEVT) {
> +            Name(_HID, "QEMU0001")
> +            /* PEST will be patched to be Zero if no such device */
> +            ACPI_EXTRACT_NAME_WORD_CONST ssdt_isa_pest
> +            Name(PEST, 0xFFFF)
> +            OperationRegion(PEOR, SystemIO, PEST, 0x01)
> +            Field(PEOR, ByteAcc, NoLock, Preserve) {
> +                PEPT,   8,
> +            }
> +
> +            Method(_STA, 0, NotSerialized) {
> +                Store(PEST, Local0)
> +                If (LEqual(Local0, Zero)) {
> +                    Return (0x00)
> +                } Else {
> +                    Return (0x0F)
> +                }
> +            }
> +
> +            Method(RDPT, 0, NotSerialized) {
> +                Store(PEPT, Local0)
> +                Return (Local0)
> +            }
> +
> +            Method(WRPT, 1, NotSerialized) {
> +                Store(Arg0, PEPT)
> +            }
> +
> +            Name(_CRS, ResourceTemplate() {
> +                IO(Decode16, 0x00, 0x00, 0x01, 0x01, IO)
> +            })
> +
> +            CreateWordField(_CRS, IO._MIN, IOMN)
> +            CreateWordField(_CRS, IO._MAX, IOMX)
> +
> +            Method(_INI, 0, NotSerialized) {
> +                Store(PEST, IOMN)
> +                Store(PEST, IOMX)
> +            }
> +        }
> +    }
>  }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Kevin O'Connor - March 29, 2013, 1:33 p.m.
On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> pvpanic device is used to notify host(qemu) when guest panic happens.

Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
QEMU.  I think this should wait until after the move.

-Kevin
Paolo Bonzini - March 29, 2013, 1:49 p.m.
Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
>> pvpanic device is used to notify host(qemu) when guest panic happens.
> 
> Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> QEMU.  I think this should wait until after the move.

The device should be in QEMU 1.5, and the SSDT probably will still be in
SeaBIOS by then (and might even be the last to move, since it's quite
complex and dynamic).  I don't think it is fair to block this patch on
those grounds...

Paolo
Kevin O'Connor - March 30, 2013, 1:20 p.m.
On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > 
> > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > QEMU.  I think this should wait until after the move.
> 
> The device should be in QEMU 1.5, and the SSDT probably will still be in
> SeaBIOS by then (and might even be the last to move, since it's quite
> complex and dynamic).  I don't think it is fair to block this patch on
> those grounds...

What is the user visible impact of not having a panic device?

My main concern is that the patch creates a new fw_cfg channel between
qemu and seabios thats sole purpose is to alter the OS visible ACPI
tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
(in sum) quite complex.

-Kevin
Paolo Bonzini - March 30, 2013, 3:36 p.m.
Il 30/03/2013 14:20, Kevin O'Connor ha scritto:
> On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
>> Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
>>> On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
>>>> pvpanic device is used to notify host(qemu) when guest panic happens.
>>>
>>> Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
>>> QEMU.  I think this should wait until after the move.
>>
>> The device should be in QEMU 1.5, and the SSDT probably will still be in
>> SeaBIOS by then (and might even be the last to move, since it's quite
>> complex and dynamic).  I don't think it is fair to block this patch on
>> those grounds...
> 
> What is the user visible impact of not having a panic device?

Management cannot restart or dump the guest after a panic, and cannot
log that a panic happened (as opposed to a random watchdog event).

> My main concern is that the patch creates a new fw_cfg channel between
> qemu and seabios thats sole purpose is to alter the OS visible ACPI
> tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> (in sum) quite complex.

Yes, this is true.

Paolo
Gleb Natapov - March 31, 2013, 2:34 p.m.
On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> > Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> > >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > > 
> > > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > > QEMU.  I think this should wait until after the move.
> > 
> > The device should be in QEMU 1.5, and the SSDT probably will still be in
> > SeaBIOS by then (and might even be the last to move, since it's quite
> > complex and dynamic).  I don't think it is fair to block this patch on
> > those grounds...
> 
> What is the user visible impact of not having a panic device?
> 
> My main concern is that the patch creates a new fw_cfg channel between
> qemu and seabios thats sole purpose is to alter the OS visible ACPI
> tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> (in sum) quite complex.
> 
The patch uses existing channel between qemu and seabios, one
romfile_loadint() is all it takes. We already have number of interfaces
to change OS visible ACPI tables, that's why we want to move ACPI table
creation to QEMU in the first place. It is unfortunate to start blocking
features now before we have an alternative. When ACPI table creation
will move into QEMU the code in this patch will be dropped along with
all the other code that serves similar purpose.

--
			Gleb.
Kevin O'Connor - April 2, 2013, 12:22 a.m.
On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> > > Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > > > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> > > >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > > > 
> > > > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > > > QEMU.  I think this should wait until after the move.
> > > 
> > > The device should be in QEMU 1.5, and the SSDT probably will still be in
> > > SeaBIOS by then (and might even be the last to move, since it's quite
> > > complex and dynamic).  I don't think it is fair to block this patch on
> > > those grounds...
> > 
> > What is the user visible impact of not having a panic device?
> > 
> > My main concern is that the patch creates a new fw_cfg channel between
> > qemu and seabios thats sole purpose is to alter the OS visible ACPI
> > tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> > (in sum) quite complex.
> > 
> The patch uses existing channel between qemu and seabios, one
> romfile_loadint() is all it takes. We already have number of interfaces
> to change OS visible ACPI tables, that's why we want to move ACPI table
> creation to QEMU in the first place. It is unfortunate to start blocking
> features now before we have an alternative. When ACPI table creation
> will move into QEMU the code in this patch will be dropped along with
> all the other code that serves similar purpose.

Hi Gleb,

If there is a general consensus that this feature is important then
we'll go forward with adding it as is.  To be clear though, my
preference would be to go forward with moving ACPI tables into QEMU,
and then add this stuff on top of that.  If no one beats me to it,
I'll send some initial patches myself.

If we do merge this feature as is, it will also require a SeaBIOS
release (followed by a SeaBIOS pull into QEMU).  There weren't any
short-term plans for a new SeaBIOS release - if this feature demands
that then we need to start planning it now.

-Kevin
Gleb Natapov - April 2, 2013, 9:07 a.m.
On Mon, Apr 01, 2013 at 08:22:57PM -0400, Kevin O'Connor wrote:
> On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> > On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > > On Fri, Mar 29, 2013 at 02:49:12PM +0100, Paolo Bonzini wrote:
> > > > Il 29/03/2013 14:33, Kevin O'Connor ha scritto:
> > > > > On Fri, Mar 29, 2013 at 04:18:44PM +0800, Hu Tao wrote:
> > > > >> pvpanic device is used to notify host(qemu) when guest panic happens.
> > > > > 
> > > > > Thanks.  However, we're planning a move of ACPI tables from SeaBIOS to
> > > > > QEMU.  I think this should wait until after the move.
> > > > 
> > > > The device should be in QEMU 1.5, and the SSDT probably will still be in
> > > > SeaBIOS by then (and might even be the last to move, since it's quite
> > > > complex and dynamic).  I don't think it is fair to block this patch on
> > > > those grounds...
> > > 
> > > What is the user visible impact of not having a panic device?
> > > 
> > > My main concern is that the patch creates a new fw_cfg channel between
> > > qemu and seabios thats sole purpose is to alter the OS visible ACPI
> > > tables.  These types of QEMU->SeaBIOS interfaces are fragile and are
> > > (in sum) quite complex.
> > > 
> > The patch uses existing channel between qemu and seabios, one
> > romfile_loadint() is all it takes. We already have number of interfaces
> > to change OS visible ACPI tables, that's why we want to move ACPI table
> > creation to QEMU in the first place. It is unfortunate to start blocking
> > features now before we have an alternative. When ACPI table creation
> > will move into QEMU the code in this patch will be dropped along with
> > all the other code that serves similar purpose.
> 
> Hi Gleb,
> 
> If there is a general consensus that this feature is important then
> we'll go forward with adding it as is.  To be clear though, my
> preference would be to go forward with moving ACPI tables into QEMU,
> and then add this stuff on top of that.  If no one beats me to it,
> I'll send some initial patches myself.
> 
If we can accomplish the move before next major QEMU release we do not
need this new fw_cfg file obviously. Paolo thinks this is not feasible,
I haven't followed this work to close to have informed opinion.

> If we do merge this feature as is, it will also require a SeaBIOS
> release (followed by a SeaBIOS pull into QEMU).  There weren't any
> short-term plans for a new SeaBIOS release - if this feature demands
> that then we need to start planning it now.
> 
It is always good to have latest BIOS with new QEMU release IMO, so lest
plan it regardless.

--
			Gleb.
Kevin O'Connor - April 10, 2013, 12:37 a.m.
On Tue, Apr 02, 2013 at 12:07:46PM +0300, Gleb Natapov wrote:
> On Mon, Apr 01, 2013 at 08:22:57PM -0400, Kevin O'Connor wrote:
> > On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> > > On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > > The patch uses existing channel between qemu and seabios, one
> > > romfile_loadint() is all it takes. We already have number of interfaces
> > > to change OS visible ACPI tables, that's why we want to move ACPI table
> > > creation to QEMU in the first place. It is unfortunate to start blocking
> > > features now before we have an alternative. When ACPI table creation
> > > will move into QEMU the code in this patch will be dropped along with
> > > all the other code that serves similar purpose.
> > 
> > If there is a general consensus that this feature is important then
> > we'll go forward with adding it as is.  To be clear though, my
> > preference would be to go forward with moving ACPI tables into QEMU,
> > and then add this stuff on top of that.  If no one beats me to it,
> > I'll send some initial patches myself.
> > 
> If we can accomplish the move before next major QEMU release we do not
> need this new fw_cfg file obviously. Paolo thinks this is not feasible,
> I haven't followed this work to close to have informed opinion.

I was hoping I'd get a chance to submit some QEMU patches for this
before the soft-freeze, but unfortunately I have not been able to.
Since I don't want to hold up features, I remove my earlier objection
and I'm okay with committing this to SeaBIOS.

Hu Tao - if the QEMU part of the pvpanic series is committed to QEMU
I'll commit the corresponding SeaBIOS parts.

Apologies for the delay.
-Kevin
Hu Tao - April 10, 2013, 5:06 a.m.
On Tue, Apr 09, 2013 at 08:37:16PM -0400, Kevin O'Connor wrote:
> On Tue, Apr 02, 2013 at 12:07:46PM +0300, Gleb Natapov wrote:
> > On Mon, Apr 01, 2013 at 08:22:57PM -0400, Kevin O'Connor wrote:
> > > On Sun, Mar 31, 2013 at 05:34:10PM +0300, Gleb Natapov wrote:
> > > > On Sat, Mar 30, 2013 at 09:20:09AM -0400, Kevin O'Connor wrote:
> > > > The patch uses existing channel between qemu and seabios, one
> > > > romfile_loadint() is all it takes. We already have number of interfaces
> > > > to change OS visible ACPI tables, that's why we want to move ACPI table
> > > > creation to QEMU in the first place. It is unfortunate to start blocking
> > > > features now before we have an alternative. When ACPI table creation
> > > > will move into QEMU the code in this patch will be dropped along with
> > > > all the other code that serves similar purpose.
> > > 
> > > If there is a general consensus that this feature is important then
> > > we'll go forward with adding it as is.  To be clear though, my
> > > preference would be to go forward with moving ACPI tables into QEMU,
> > > and then add this stuff on top of that.  If no one beats me to it,
> > > I'll send some initial patches myself.
> > > 
> > If we can accomplish the move before next major QEMU release we do not
> > need this new fw_cfg file obviously. Paolo thinks this is not feasible,
> > I haven't followed this work to close to have informed opinion.
> 
> I was hoping I'd get a chance to submit some QEMU patches for this
> before the soft-freeze, but unfortunately I have not been able to.
> Since I don't want to hold up features, I remove my earlier objection
> and I'm okay with committing this to SeaBIOS.

Glad to hear that!

> 
> Hu Tao - if the QEMU part of the pvpanic series is committed to QEMU
> I'll commit the corresponding SeaBIOS parts.

Thanks a lot!

Patch

diff --git a/src/acpi.c b/src/acpi.c
index bc4d8ea..fe504f0 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -534,6 +534,9 @@  build_ssdt(void)
         ssdt_ptr[acpi_pci64_valid[0]] = 0;
     }
 
+    int pvpanic_port = romfile_loadint("etc/pvpanic-port", 0x0);
+    *(u16 *)(ssdt_ptr + *ssdt_isa_pest) = pvpanic_port;
+
     ssdt_ptr += sizeof(ssdp_misc_aml);
 
     // build Scope(_SB_) header
diff --git a/src/ssdt-misc.dsl b/src/ssdt-misc.dsl
index 679422b..acc850e 100644
--- a/src/ssdt-misc.dsl
+++ b/src/ssdt-misc.dsl
@@ -55,4 +55,50 @@  DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
             Zero   /* reserved */
         })
     }
+
+    External(\_SB.PCI0, DeviceObj)
+    External(\_SB.PCI0.ISA, DeviceObj)
+
+    Scope(\_SB.PCI0.ISA) {
+        Device(PEVT) {
+            Name(_HID, "QEMU0001")
+            /* PEST will be patched to be Zero if no such device */
+            ACPI_EXTRACT_NAME_WORD_CONST ssdt_isa_pest
+            Name(PEST, 0xFFFF)
+            OperationRegion(PEOR, SystemIO, PEST, 0x01)
+            Field(PEOR, ByteAcc, NoLock, Preserve) {
+                PEPT,   8,
+            }
+
+            Method(_STA, 0, NotSerialized) {
+                Store(PEST, Local0)
+                If (LEqual(Local0, Zero)) {
+                    Return (0x00)
+                } Else {
+                    Return (0x0F)
+                }
+            }
+
+            Method(RDPT, 0, NotSerialized) {
+                Store(PEPT, Local0)
+                Return (Local0)
+            }
+
+            Method(WRPT, 1, NotSerialized) {
+                Store(Arg0, PEPT)
+            }
+
+            Name(_CRS, ResourceTemplate() {
+                IO(Decode16, 0x00, 0x00, 0x01, 0x01, IO)
+            })
+
+            CreateWordField(_CRS, IO._MIN, IOMN)
+            CreateWordField(_CRS, IO._MAX, IOMX)
+
+            Method(_INI, 0, NotSerialized) {
+                Store(PEST, IOMN)
+                Store(PEST, IOMX)
+            }
+        }
+    }
 }