Patchwork [v14,3/4] introduce pvevent device to deal with panicked event

login
register
mail settings
Submitter Hu Tao
Date March 14, 2013, 8:15 a.m.
Message ID <5d85837fd58683b7e13ac78d6d468af6e152a512.1363243596.git.hutao@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/227475/
State New
Headers show

Comments

Hu Tao - March 14, 2013, 8:15 a.m.
pvevent device is used to send guest panic event from guest to qemu.

When guest panic happens, pvevent device driver will write a event
number to IO port 0x505(which is the IO port occupied by pvevent device,
by default). On receiving the event, pvevent device will pause guest
cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  

TODO: make the IO port configurable

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/Makefile.objs |   2 +
 hw/pvevent.c     | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 hw/pvevent.c
Paolo Bonzini - March 14, 2013, 9:14 a.m.
Il 14/03/2013 09:15, Hu Tao ha scritto:
> pvevent device is used to send guest panic event from guest to qemu.
> 
> When guest panic happens, pvevent device driver will write a event
> number to IO port 0x505(which is the IO port occupied by pvevent device,
> by default). On receiving the event, pvevent device will pause guest
> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> 
> TODO: make the IO port configurable

The port is already configurable as far as the device is concerned; when
you add the port to the PC boards you will have to wind up fw-cfg.

So these patches can go in already, with the sole change that the HID
must be in the QEMU namespace rather than MSFT.

Paolo
Gleb Natapov - March 14, 2013, 9:19 a.m.
On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 09:15, Hu Tao ha scritto:
> > pvevent device is used to send guest panic event from guest to qemu.
> > 
> > When guest panic happens, pvevent device driver will write a event
> > number to IO port 0x505(which is the IO port occupied by pvevent device,
> > by default). On receiving the event, pvevent device will pause guest
> > cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> > 
> > TODO: make the IO port configurable
> 
> The port is already configurable as far as the device is concerned; when
> you add the port to the PC boards you will have to wind up fw-cfg.
> 
Why not add fw-cfg when device is created (with -device for instance)?

> So these patches can go in already, with the sole change that the HID
> must be in the QEMU namespace rather than MSFT.
> 
We should request one for QEMU.

--
			Gleb.
Paolo Bonzini - March 14, 2013, 9:43 a.m.
Il 14/03/2013 10:19, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>> pvevent device is used to send guest panic event from guest to qemu.
>>>
>>> When guest panic happens, pvevent device driver will write a event
>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>> by default). On receiving the event, pvevent device will pause guest
>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>
>>> TODO: make the IO port configurable
>>
>> The port is already configurable as far as the device is concerned; when
>> you add the port to the PC boards you will have to wind up fw-cfg.
>
> Why not add fw-cfg when device is created (with -device for instance)?

It depends on what we decide is the supported interface for the device:

* it can be an ISA device; the interface is the I/O port and ACPI
support is provided just for convenience of the OSPM.  In this case,
"-device pvevent" should just add handlers for the port.  The ACPI
support is similar to what we do for other on-board ISA devices, for
example serial ports (the serial ports use PIIX PCI configuration
instead of fw-cfg, but that's a minor detail).  It only needs to work
for port 0x505, so the fw-cfg data can be a single yes/no value and only
the _STA method needs patching.  See piix4_pm_machine_ready in
hw/acpi_piix4.c.

* ACPI support is a first-class part of the device.  Each instance of
the device should be there in the ACPI tables.  In this case the fw-cfg
data needs to be a list of ports, and it is probably simpler to combine
all the definitions in an SSDT that is dynamically-built (similar to
what we do for PCI hotplug slots).  Or even provide a separate SSDT for
each instance of the device.


I prefer the first, the second seems to be over-engineered.

>> So these patches can go in already, with the sole change that the HID
>> must be in the QEMU namespace rather than MSFT.
>>
> We should request one for QEMU.

Yes.  BTW, the QEMU patches can go in only in the first of the two cases
above.

Paolo
Hu Tao - March 14, 2013, 9:46 a.m.
On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 09:15, Hu Tao ha scritto:
> > pvevent device is used to send guest panic event from guest to qemu.
> > 
> > When guest panic happens, pvevent device driver will write a event
> > number to IO port 0x505(which is the IO port occupied by pvevent device,
> > by default). On receiving the event, pvevent device will pause guest
> > cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> > 
> > TODO: make the IO port configurable
> 
> The port is already configurable as far as the device is concerned; when
> you add the port to the PC boards you will have to wind up fw-cfg.

Yes, by configurable, I mean to pass io port through fw-cfg to seabios
and patch it.
Alexander Graf - March 14, 2013, 11 a.m.
On 14.03.2013, at 10:43, Paolo Bonzini wrote:

> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>> 
>>>> When guest panic happens, pvevent device driver will write a event
>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>> by default). On receiving the event, pvevent device will pause guest
>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>> 
>>>> TODO: make the IO port configurable
>>> 
>>> The port is already configurable as far as the device is concerned; when
>>> you add the port to the PC boards you will have to wind up fw-cfg.
>> 
>> Why not add fw-cfg when device is created (with -device for instance)?
> 
> It depends on what we decide is the supported interface for the device:
> 
> * it can be an ISA device; the interface is the I/O port and ACPI

Is there any particular reason it's an ISA device with a PIO port, rather than a platform / sysbus device with MMIO access? With the latter, we could easily reuse the device on other platforms (ppc, arm) and even the guest driver code for platforms that do ACPI (arm?).

Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?


Alex
Paolo Bonzini - March 14, 2013, 11:03 a.m.
Il 14/03/2013 12:00, Alexander Graf ha scritto:
> 
> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
> 
>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>>>
>>>>> When guest panic happens, pvevent device driver will write a event
>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>>> by default). On receiving the event, pvevent device will pause guest
>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>>>
>>>>> TODO: make the IO port configurable
>>>>
>>>> The port is already configurable as far as the device is concerned; when
>>>> you add the port to the PC boards you will have to wind up fw-cfg.
>>>
>>> Why not add fw-cfg when device is created (with -device for instance)?
>>
>> It depends on what we decide is the supported interface for the device:
>>
>> * it can be an ISA device; the interface is the I/O port and ACPI
> 
> Is there any particular reason it's an ISA device with a PIO port,
> rather than a platform / sysbus device with MMIO access? With the
> latter, we could easily reuse the device on other platforms (ppc, arm)
> and even the guest driver code for platforms that do ACPI (arm?).

Where would you place the MMIO area on x86?  But anyway you can easily
define an MMIO variant, the guest driver code will be shared (the ACPI
in the firmware no, of course).

> Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?

I think Xen uses xenstore for this, or a hypercall I don't remember.
But not something that can be shared unfortunately.

Paolo
Alexander Graf - March 14, 2013, 11:23 a.m.
On 14.03.2013, at 12:03, Paolo Bonzini wrote:

> Il 14/03/2013 12:00, Alexander Graf ha scritto:
>> 
>> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
>> 
>>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>>>> 
>>>>>> When guest panic happens, pvevent device driver will write a event
>>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>>>> by default). On receiving the event, pvevent device will pause guest
>>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>>>> 
>>>>>> TODO: make the IO port configurable
>>>>> 
>>>>> The port is already configurable as far as the device is concerned; when
>>>>> you add the port to the PC boards you will have to wind up fw-cfg.
>>>> 
>>>> Why not add fw-cfg when device is created (with -device for instance)?
>>> 
>>> It depends on what we decide is the supported interface for the device:
>>> 
>>> * it can be an ISA device; the interface is the I/O port and ACPI
>> 
>> Is there any particular reason it's an ISA device with a PIO port,
>> rather than a platform / sysbus device with MMIO access? With the
>> latter, we could easily reuse the device on other platforms (ppc, arm)
>> and even the guest driver code for platforms that do ACPI (arm?).
> 
> Where would you place the MMIO area on x86?  

Wherever the board thinks it makes sense.

> But anyway you can easily
> define an MMIO variant, the guest driver code will be shared (the ACPI
> in the firmware no, of course).

Yes, at which point we have 2 variants where we could have had 1. I don't know if it's worth caring about it, just wanted to bring it up.

> 
>> Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?
> 
> I think Xen uses xenstore for this, or a hypercall I don't remember.
> But not something that can be shared unfortunately.

At least the guest kernel hook could be shared. In fact, how does that one work with this device? I've only seen an ACPI patch so far. Does ACPI already support panic hooks?


Alex
Gleb Natapov - March 14, 2013, 11:28 a.m.
On Thu, Mar 14, 2013 at 12:23:01PM +0100, Alexander Graf wrote:
> 
> On 14.03.2013, at 12:03, Paolo Bonzini wrote:
> 
> > Il 14/03/2013 12:00, Alexander Graf ha scritto:
> >> 
> >> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
> >> 
> >>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
> >>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> >>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
> >>>>>> pvevent device is used to send guest panic event from guest to qemu.
> >>>>>> 
> >>>>>> When guest panic happens, pvevent device driver will write a event
> >>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
> >>>>>> by default). On receiving the event, pvevent device will pause guest
> >>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> >>>>>> 
> >>>>>> TODO: make the IO port configurable
> >>>>> 
> >>>>> The port is already configurable as far as the device is concerned; when
> >>>>> you add the port to the PC boards you will have to wind up fw-cfg.
> >>>> 
> >>>> Why not add fw-cfg when device is created (with -device for instance)?
> >>> 
> >>> It depends on what we decide is the supported interface for the device:
> >>> 
> >>> * it can be an ISA device; the interface is the I/O port and ACPI
> >> 
> >> Is there any particular reason it's an ISA device with a PIO port,
> >> rather than a platform / sysbus device with MMIO access? With the
> >> latter, we could easily reuse the device on other platforms (ppc, arm)
> >> and even the guest driver code for platforms that do ACPI (arm?).
> > 
> > Where would you place the MMIO area on x86?  
> 
> Wherever the board thinks it makes sense.
> 
On x86 it makes sense to put it in IO space :)

> > But anyway you can easily
> > define an MMIO variant, the guest driver code will be shared (the ACPI
> > in the firmware no, of course).
> 
> Yes, at which point we have 2 variants where we could have had 1. I don't know if it's worth caring about it, just wanted to bring it up.
> 
We can have the same device and control via properties what address
space to use for the device.

> > 
> >> Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?
> > 
> > I think Xen uses xenstore for this, or a hypercall I don't remember.
> > But not something that can be shared unfortunately.
> 
> At least the guest kernel hook could be shared. In fact, how does that one work with this device? I've only seen an ACPI patch so far. Does ACPI already support panic hooks?
> 
> 
No need for any special hook to use the device. There was a separate
patch with guest platform driver that uses the device. IN fact it should
work on XEN too.

--
			Gleb.
Gleb Natapov - March 14, 2013, 12:34 p.m.
On Thu, Mar 14, 2013 at 10:43:43AM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 09:15, Hu Tao ha scritto:
> >>> pvevent device is used to send guest panic event from guest to qemu.
> >>>
> >>> When guest panic happens, pvevent device driver will write a event
> >>> number to IO port 0x505(which is the IO port occupied by pvevent device,
> >>> by default). On receiving the event, pvevent device will pause guest
> >>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> >>>
> >>> TODO: make the IO port configurable
> >>
> >> The port is already configurable as far as the device is concerned; when
> >> you add the port to the PC boards you will have to wind up fw-cfg.
> >
> > Why not add fw-cfg when device is created (with -device for instance)?
> 
> It depends on what we decide is the supported interface for the device:
> 
> * it can be an ISA device; the interface is the I/O port and ACPI
> support is provided just for convenience of the OSPM.  In this case,
> "-device pvevent" should just add handlers for the port.  The ACPI
> support is similar to what we do for other on-board ISA devices, for
> example serial ports (the serial ports use PIIX PCI configuration
> instead of fw-cfg, but that's a minor detail).  It only needs to work
> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> the _STA method needs patching.  See piix4_pm_machine_ready in
> hw/acpi_piix4.c.
> 
Again I think there is a big difference between well knows device and
PV devices that we add at random location. And if we make the later
configurable i.e it may or may not be present and location where it is
present can be changed then we better not make a guest to do guesses.

> * ACPI support is a first-class part of the device.  Each instance of
> the device should be there in the ACPI tables.  In this case the fw-cfg
> data needs to be a list of ports, and it is probably simpler to combine
> all the definitions in an SSDT that is dynamically-built (similar to
> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> each instance of the device.
> 
> 
> I prefer the first, the second seems to be over-engineered.
> 
Second is over-engineering indeed. The device should be singleton and
fail if second instance is created. Do we have such capability in qdev?

--
			Gleb.
Paolo Bonzini - March 14, 2013, 1:49 p.m.
Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>> * it can be an ISA device; the interface is the I/O port and ACPI
>> support is provided just for convenience of the OSPM.  In this case,
>> "-device pvevent" should just add handlers for the port.  The ACPI
>> support is similar to what we do for other on-board ISA devices, for
>> example serial ports (the serial ports use PIIX PCI configuration
>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>> the _STA method needs patching.  See piix4_pm_machine_ready in
>> hw/acpi_piix4.c.
>
> Again I think there is a big difference between well knows device and
> PV devices that we add at random location. And if we make the later
> configurable i.e it may or may not be present and location where it is
> present can be changed then we better not make a guest to do guesses.

No guesses here on part of the guest, and no probing in the firmware
two.  The same number is hard-coded in QEMU and the DSDT, which go in
pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
trick with the methods).

I think it's a nice compromise.

>> * ACPI support is a first-class part of the device.  Each instance of
>> the device should be there in the ACPI tables.  In this case the fw-cfg
>> data needs to be a list of ports, and it is probably simpler to combine
>> all the definitions in an SSDT that is dynamically-built (similar to
>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
>> each instance of the device.
>>
>> I prefer the first, the second seems to be over-engineered.
>>
> Second is over-engineering indeed. The device should be singleton and
> fail if second instance is created. Do we have such capability in qdev?

No, but why should it fail?

Paolo
Gleb Natapov - March 14, 2013, 1:56 p.m.
On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >> * it can be an ISA device; the interface is the I/O port and ACPI
> >> support is provided just for convenience of the OSPM.  In this case,
> >> "-device pvevent" should just add handlers for the port.  The ACPI
> >> support is similar to what we do for other on-board ISA devices, for
> >> example serial ports (the serial ports use PIIX PCI configuration
> >> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >> the _STA method needs patching.  See piix4_pm_machine_ready in
> >> hw/acpi_piix4.c.
> >
> > Again I think there is a big difference between well knows device and
> > PV devices that we add at random location. And if we make the later
> > configurable i.e it may or may not be present and location where it is
> > present can be changed then we better not make a guest to do guesses.
> 
> No guesses here on part of the guest, and no probing in the firmware
> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> trick with the methods).
> 
That's the problem. The number is not hard coded in QEMU only DSDT. If
you hard code it in QEMU (make it non configurable) and make device mandatory
static DSDT make sense if provided by QEMU.

> I think it's a nice compromise.
> 
> >> * ACPI support is a first-class part of the device.  Each instance of
> >> the device should be there in the ACPI tables.  In this case the fw-cfg
> >> data needs to be a list of ports, and it is probably simpler to combine
> >> all the definitions in an SSDT that is dynamically-built (similar to
> >> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> >> each instance of the device.
> >>
> >> I prefer the first, the second seems to be over-engineered.
> >>
> > Second is over-engineering indeed. The device should be singleton and
> > fail if second instance is created. Do we have such capability in qdev?
> 
> No, but why should it fail?
> 
Why should it not? Guest cannot use more than on of them, why allow to
create insane configs?

--
			Gleb.
Paolo Bonzini - March 14, 2013, 2:05 p.m.
Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>>>> * it can be an ISA device; the interface is the I/O port and ACPI
>>>> support is provided just for convenience of the OSPM.  In this case,
>>>> "-device pvevent" should just add handlers for the port.  The ACPI
>>>> support is similar to what we do for other on-board ISA devices, for
>>>> example serial ports (the serial ports use PIIX PCI configuration
>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
>>>> hw/acpi_piix4.c.
>>>
>>> Again I think there is a big difference between well knows device and
>>> PV devices that we add at random location. And if we make the later
>>> configurable i.e it may or may not be present and location where it is
>>> present can be changed then we better not make a guest to do guesses.
>>
>> No guesses here on part of the guest, and no probing in the firmware
>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
>> trick with the methods).
>
> That's the problem. The number is not hard coded in QEMU only DSDT.

It is hard-coded where the board creates it, or at least as the default
value of the qdev property.

> If you hard code it in QEMU (make it non configurable) and make device mandatory
> static DSDT make sense if provided by QEMU.

You cannot make it mandatory due to versioned machine types, but my plan
would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
makes sense to have a static DSDT.  Sorry if it was unclear.

>> I think it's a nice compromise.
>>
>>>> * ACPI support is a first-class part of the device.  Each instance of
>>>> the device should be there in the ACPI tables.  In this case the fw-cfg
>>>> data needs to be a list of ports, and it is probably simpler to combine
>>>> all the definitions in an SSDT that is dynamically-built (similar to
>>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
>>>> each instance of the device.
>>>>
>>>> I prefer the first, the second seems to be over-engineered.
>>>>
>>> Second is over-engineering indeed. The device should be singleton and
>>> fail if second instance is created. Do we have such capability in qdev?
>>
>> No, but why should it fail?
>>
> Why should it not? Guest cannot use more than on of them, why allow to
> create insane configs?

Who cares?  Insane ISA device configs anyway are not discoverable by
guests, you need to teach the guest about the device manually.

Paolo
Gleb Natapov - March 14, 2013, 2:23 p.m.
On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >>>> * it can be an ISA device; the interface is the I/O port and ACPI
> >>>> support is provided just for convenience of the OSPM.  In this case,
> >>>> "-device pvevent" should just add handlers for the port.  The ACPI
> >>>> support is similar to what we do for other on-board ISA devices, for
> >>>> example serial ports (the serial ports use PIIX PCI configuration
> >>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >>>> the _STA method needs patching.  See piix4_pm_machine_ready in
> >>>> hw/acpi_piix4.c.
> >>>
> >>> Again I think there is a big difference between well knows device and
> >>> PV devices that we add at random location. And if we make the later
> >>> configurable i.e it may or may not be present and location where it is
> >>> present can be changed then we better not make a guest to do guesses.
> >>
> >> No guesses here on part of the guest, and no probing in the firmware
> >> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> >> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> >> trick with the methods).
> >
> > That's the problem. The number is not hard coded in QEMU only DSDT.
> 
> It is hard-coded where the board creates it, or at least as the default
> value of the qdev property.
> 
Default value that can be changes is not hard coded. Why do you allow
change in one place, but not the other?

> > If you hard code it in QEMU (make it non configurable) and make device mandatory
> > static DSDT make sense if provided by QEMU.
> 
> You cannot make it mandatory due to versioned machine types, but my plan
> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
> makes sense to have a static DSDT.  Sorry if it was unclear.
And then you will have to have different DSDT for pre pc-1.5. Dynamic
patching solves exactly that problem.

> 
> >> I think it's a nice compromise.
> >>
> >>>> * ACPI support is a first-class part of the device.  Each instance of
> >>>> the device should be there in the ACPI tables.  In this case the fw-cfg
> >>>> data needs to be a list of ports, and it is probably simpler to combine
> >>>> all the definitions in an SSDT that is dynamically-built (similar to
> >>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> >>>> each instance of the device.
> >>>>
> >>>> I prefer the first, the second seems to be over-engineered.
> >>>>
> >>> Second is over-engineering indeed. The device should be singleton and
> >>> fail if second instance is created. Do we have such capability in qdev?
> >>
> >> No, but why should it fail?
> >>
> > Why should it not? Guest cannot use more than on of them, why allow to
> > create insane configs?
> 
> Who cares?  Insane ISA device configs anyway are not discoverable by
> guests, you need to teach the guest about the device manually.
> 
With proper ACPI they are discoverable. Since writing ACPI support for
multiple pvpanic devices is clear case of over-engineering it is a
courtesy to QEMU users to fail machine creation that cannot be properly
described by ACPI.

--
			Gleb.
Paolo Bonzini - March 14, 2013, 3:50 p.m.
Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
>>>>>> support is provided just for convenience of the OSPM.  In this case,
>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
>>>>>> support is similar to what we do for other on-board ISA devices, for
>>>>>> example serial ports (the serial ports use PIIX PCI configuration
>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
>>>>>> hw/acpi_piix4.c.
>>>>>
>>>>> Again I think there is a big difference between well knows device and
>>>>> PV devices that we add at random location. And if we make the later
>>>>> configurable i.e it may or may not be present and location where it is
>>>>> present can be changed then we better not make a guest to do guesses.
>>>>
>>>> No guesses here on part of the guest, and no probing in the firmware
>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
>>>> trick with the methods).
>>>
>>> That's the problem. The number is not hard coded in QEMU only DSDT.
>>
>> It is hard-coded where the board creates it, or at least as the default
>> value of the qdev property.
>
> Default value that can be changes is not hard coded.
> Why do you allow change in one place, but not the other?

I'm just following the model of other ISA devices, I don't think there's
any difference in this respect between well-known and pv devices (also
because in the end all modern guests will use ACPI to discover even
well-known devices).

The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
serial ports.

>>> If you hard code it in QEMU (make it non configurable) and make device mandatory
>>> static DSDT make sense if provided by QEMU.
>>
>> You cannot make it mandatory due to versioned machine types, but my plan
>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
>> makes sense to have a static DSDT.  Sorry if it was unclear.
> 
> And then you will have to have different DSDT for pre pc-1.5. Dynamic
> patching solves exactly that problem.

Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.

>>>> I think it's a nice compromise.

^^^ This still holds. :)

>>>>>> * ACPI support is a first-class part of the device.  Each instance of
>>>>>> the device should be there in the ACPI tables.  In this case the fw-cfg
>>>>>> data needs to be a list of ports, and it is probably simpler to combine
>>>>>> all the definitions in an SSDT that is dynamically-built (similar to
>>>>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
>>>>>> each instance of the device.
>>>>>>
>>>>>> I prefer the first, the second seems to be over-engineered.
>>>>>>
>>>>> Second is over-engineering indeed. The device should be singleton and
>>>>> fail if second instance is created. Do we have such capability in qdev?
>>>>
>>>> No, but why should it fail?
>>>>
>>> Why should it not? Guest cannot use more than on of them, why allow to
>>> create insane configs?
>>
>> Who cares?  Insane ISA device configs anyway are not discoverable by
>> guests, you need to teach the guest about the device manually.
>>
> With proper ACPI they are discoverable. Since writing ACPI support for
> multiple pvpanic devices is clear case of over-engineering it is a
> courtesy to QEMU users to fail machine creation that cannot be properly
> described by ACPI.

We don't fail machine creation if someone wants to place a serial port
at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
a reason to make pvpanic special in this respect.

Paolo
Gleb Natapov - March 14, 2013, 3:59 p.m.
On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> >>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> >>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
> >>>>>> support is provided just for convenience of the OSPM.  In this case,
> >>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
> >>>>>> support is similar to what we do for other on-board ISA devices, for
> >>>>>> example serial ports (the serial ports use PIIX PCI configuration
> >>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
> >>>>>> hw/acpi_piix4.c.
> >>>>>
> >>>>> Again I think there is a big difference between well knows device and
> >>>>> PV devices that we add at random location. And if we make the later
> >>>>> configurable i.e it may or may not be present and location where it is
> >>>>> present can be changed then we better not make a guest to do guesses.
> >>>>
> >>>> No guesses here on part of the guest, and no probing in the firmware
> >>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> >>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> >>>> trick with the methods).
> >>>
> >>> That's the problem. The number is not hard coded in QEMU only DSDT.
> >>
> >> It is hard-coded where the board creates it, or at least as the default
> >> value of the qdev property.
> >
> > Default value that can be changes is not hard coded.
> > Why do you allow change in one place, but not the other?
> 
> I'm just following the model of other ISA devices, I don't think there's
> any difference in this respect between well-known and pv devices (also
> because in the end all modern guests will use ACPI to discover even
> well-known devices).
> 
We are not there yet :)

> The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
> serial ports.
> 
> >>> If you hard code it in QEMU (make it non configurable) and make device mandatory
> >>> static DSDT make sense if provided by QEMU.
> >>
> >> You cannot make it mandatory due to versioned machine types, but my plan
> >> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
> >> makes sense to have a static DSDT.  Sorry if it was unclear.
> > 
> > And then you will have to have different DSDT for pre pc-1.5. Dynamic
> > patching solves exactly that problem.
> 
> Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.
> 
Yes, if you do not allow changing IO port patching _STA is enough, but
if you already patching it is easy to patch both.

> >>>> I think it's a nice compromise.
> 
> ^^^ This still holds. :)
If we would have found a reasonable way to go without patching at all
then it would have been worthwhile to consider compromises, but if
patching is inevitable I honestly do not see big difference between
patching one place or two.

> 
> >>>>>> * ACPI support is a first-class part of the device.  Each instance of
> >>>>>> the device should be there in the ACPI tables.  In this case the fw-cfg
> >>>>>> data needs to be a list of ports, and it is probably simpler to combine
> >>>>>> all the definitions in an SSDT that is dynamically-built (similar to
> >>>>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> >>>>>> each instance of the device.
> >>>>>>
> >>>>>> I prefer the first, the second seems to be over-engineered.
> >>>>>>
> >>>>> Second is over-engineering indeed. The device should be singleton and
> >>>>> fail if second instance is created. Do we have such capability in qdev?
> >>>>
> >>>> No, but why should it fail?
> >>>>
> >>> Why should it not? Guest cannot use more than on of them, why allow to
> >>> create insane configs?
> >>
> >> Who cares?  Insane ISA device configs anyway are not discoverable by
> >> guests, you need to teach the guest about the device manually.
> >>
> > With proper ACPI they are discoverable. Since writing ACPI support for
> > multiple pvpanic devices is clear case of over-engineering it is a
> > courtesy to QEMU users to fail machine creation that cannot be properly
> > described by ACPI.
> 
> We don't fail machine creation if someone wants to place a serial port
> at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
> a reason to make pvpanic special in this respect.
> 
Fine with me. That was just a suggestion. I thought we had singleton
qdev flag.

--
			Gleb.
Paolo Bonzini - March 14, 2013, 4:13 p.m.
Il 14/03/2013 16:59, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 15:23, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
>>>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
>>>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>>>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
>>>>>>>> support is provided just for convenience of the OSPM.  In this case,
>>>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
>>>>>>>> support is similar to what we do for other on-board ISA devices, for
>>>>>>>> example serial ports (the serial ports use PIIX PCI configuration
>>>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>>>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>>>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
>>>>>>>> hw/acpi_piix4.c.
>>>>>>>
>>>>>>> Again I think there is a big difference between well knows device and
>>>>>>> PV devices that we add at random location. And if we make the later
>>>>>>> configurable i.e it may or may not be present and location where it is
>>>>>>> present can be changed then we better not make a guest to do guesses.
>>>>>>
>>>>>> No guesses here on part of the guest, and no probing in the firmware
>>>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
>>>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
>>>>>> trick with the methods).
>>>>>
>>>>> That's the problem. The number is not hard coded in QEMU only DSDT.
>>>>
>>>> It is hard-coded where the board creates it, or at least as the default
>>>> value of the qdev property.
>>>
>>> Default value that can be changes is not hard coded.
>>> Why do you allow change in one place, but not the other?
>>
>> I'm just following the model of other ISA devices, I don't think there's
>> any difference in this respect between well-known and pv devices (also
>> because in the end all modern guests will use ACPI to discover even
>> well-known devices).
>>
> We are not there yet :)

Kind of... Windows will hide serial ports that return not-present for
_STA, for example.  Linux will just hide the PNPxxxx path and present it
under /sys/bus/platform instead.

>> The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
>> serial ports.
>>
>>>>> If you hard code it in QEMU (make it non configurable) and make device mandatory
>>>>> static DSDT make sense if provided by QEMU.
>>>>
>>>> You cannot make it mandatory due to versioned machine types, but my plan
>>>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
>>>> makes sense to have a static DSDT.  Sorry if it was unclear.
>>>
>>> And then you will have to have different DSDT for pre pc-1.5. Dynamic
>>> patching solves exactly that problem.
>>
>> Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.
>>
> Yes, if you do not allow changing IO port patching _STA is enough, but
> if you already patching it is easy to patch both.
> 
>>>>>> I think it's a nice compromise.
>>
>> ^^^ This still holds. :)
> If we would have found a reasonable way to go without patching at all
> then it would have been worthwhile to consider compromises, but if
> patching is inevitable I honestly do not see big difference between
> patching one place or two.

Hmm... can you do something like

        Name(PORT, 0xAAAA)
        OperationRegion(PEOR, SystemIO, PORT, 0x01)
        Field(PEOR, ByteAcc, NoLock, Preserve) {
            PEPT,   8,
        }

? i.e. use a Name inside an OperationRegion?

If so, then we can patch 0xAAAA to zero for not-present and the port for
present and indeed patch a single place.

If we have to patch 0x505 all over the place there's an advantage in
patching _STA only.  But if we can do the above it's a bit cleaner to
use the port for the patched value, indeed.

>> We don't fail machine creation if someone wants to place a serial port
>> at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
>> a reason to make pvpanic special in this respect.
>>
> Fine with me. That was just a suggestion. I thought we had singleton
> qdev flag.

We have no_user, but it's broken and not exactly a match for what you
want here.

Paolo
Gleb Natapov - March 15, 2013, 11:34 a.m.
On Thu, Mar 14, 2013 at 05:13:54PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 16:59, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> >>> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
> >>>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> >>>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> >>>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >>>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
> >>>>>>>> support is provided just for convenience of the OSPM.  In this case,
> >>>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
> >>>>>>>> support is similar to what we do for other on-board ISA devices, for
> >>>>>>>> example serial ports (the serial ports use PIIX PCI configuration
> >>>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >>>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >>>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
> >>>>>>>> hw/acpi_piix4.c.
> >>>>>>>
> >>>>>>> Again I think there is a big difference between well knows device and
> >>>>>>> PV devices that we add at random location. And if we make the later
> >>>>>>> configurable i.e it may or may not be present and location where it is
> >>>>>>> present can be changed then we better not make a guest to do guesses.
> >>>>>>
> >>>>>> No guesses here on part of the guest, and no probing in the firmware
> >>>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> >>>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> >>>>>> trick with the methods).
> >>>>>
> >>>>> That's the problem. The number is not hard coded in QEMU only DSDT.
> >>>>
> >>>> It is hard-coded where the board creates it, or at least as the default
> >>>> value of the qdev property.
> >>>
> >>> Default value that can be changes is not hard coded.
> >>> Why do you allow change in one place, but not the other?
> >>
> >> I'm just following the model of other ISA devices, I don't think there's
> >> any difference in this respect between well-known and pv devices (also
> >> because in the end all modern guests will use ACPI to discover even
> >> well-known devices).
> >>
> > We are not there yet :)
> 
> Kind of... Windows will hide serial ports that return not-present for
> _STA, for example.  Linux will just hide the PNPxxxx path and present it
> under /sys/bus/platform instead.
> 
> >> The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
> >> serial ports.
> >>
> >>>>> If you hard code it in QEMU (make it non configurable) and make device mandatory
> >>>>> static DSDT make sense if provided by QEMU.
> >>>>
> >>>> You cannot make it mandatory due to versioned machine types, but my plan
> >>>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
> >>>> makes sense to have a static DSDT.  Sorry if it was unclear.
> >>>
> >>> And then you will have to have different DSDT for pre pc-1.5. Dynamic
> >>> patching solves exactly that problem.
> >>
> >> Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.
> >>
> > Yes, if you do not allow changing IO port patching _STA is enough, but
> > if you already patching it is easy to patch both.
> > 
> >>>>>> I think it's a nice compromise.
> >>
> >> ^^^ This still holds. :)
> > If we would have found a reasonable way to go without patching at all
> > then it would have been worthwhile to consider compromises, but if
> > patching is inevitable I honestly do not see big difference between
> > patching one place or two.
> 
> Hmm... can you do something like
> 
>         Name(PORT, 0xAAAA)
>         OperationRegion(PEOR, SystemIO, PORT, 0x01)
>         Field(PEOR, ByteAcc, NoLock, Preserve) {
>             PEPT,   8,
>         }
> 
> ? i.e. use a Name inside an OperationRegion?
> 
> If so, then we can patch 0xAAAA to zero for not-present and the port for
> present and indeed patch a single place.
> 
> If we have to patch 0x505 all over the place there's an advantage in
> patching _STA only.  But if we can do the above it's a bit cleaner to
> use the port for the patched value, indeed.
> 
And patching Name() is as simple as putting
ACPI_EXTRACT_NAME_WORD_CONST above it.

> >> We don't fail machine creation if someone wants to place a serial port
> >> at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
> >> a reason to make pvpanic special in this respect.
> >>
> > Fine with me. That was just a suggestion. I thought we had singleton
> > qdev flag.
> 
> We have no_user, but it's broken and not exactly a match for what you
> want here.
> 
> Paolo

--
			Gleb.
Markus Armbruster - March 20, 2013, 9:15 a.m.
Hu Tao <hutao@cn.fujitsu.com> writes:

> pvevent device is used to send guest panic event from guest to qemu.
>
> When guest panic happens, pvevent device driver will write a event
> number to IO port 0x505(which is the IO port occupied by pvevent device,
> by default). On receiving the event, pvevent device will pause guest
> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>
> TODO: make the IO port configurable
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/Makefile.objs |   2 +
>  hw/pvevent.c     | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 hw/pvevent.c
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 40ebe46..edf499e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -218,5 +218,7 @@ obj-$(CONFIG_KVM) += ivshmem.o
>  obj-$(CONFIG_LINUX) += vfio_pci.o
>  endif
>  
> +common-obj-y += pvevent.o
> +
>  $(obj)/baum.o: QEMU_CFLAGS += $(SDL_CFLAGS) 
>  endif
> diff --git a/hw/pvevent.c b/hw/pvevent.c
> new file mode 100644
> index 0000000..c7df77b
> --- /dev/null
> +++ b/hw/pvevent.c
> @@ -0,0 +1,116 @@
> +/*
> + * QEMU simulated pvevent device.
> + *
> + * Copyright Fujitsu, Corp. 2013
> + *
> + * Authors:
> + *     Wen Congyang <wency@cn.fujitsu.com>
> + *     Hu Tao <hutao@cn.fujitsu.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <qapi/qmp/qobject.h>
> +#include <qapi/qmp/qjson.h>
> +#include <monitor/monitor.h>
> +#include <sysemu/sysemu.h>
> +#include <sysemu/kvm.h>
> +
> +/* The bit of supported pv event */
> +#define PVEVENT_F_PANICKED      0
> +
> +/* The pv event value */
> +#define PVEVENT_PANICKED        (1 << PVEVENT_F_PANICKED)

This looks like events are encoded as bits, which can be combined into a
set, but...

> +
> +#define TYPE_ISA_PVEVENT_DEVICE    "pvevent"
> +#define ISA_PVEVENT_DEVICE(obj)    \
> +    OBJECT_CHECK(PVEventState, (obj), TYPE_ISA_PVEVENT_DEVICE)
> +
> +static void panicked_mon_event(const char *action)
> +{
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'action': %s }", action);
> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
> +    qobject_decref(data);
> +}
> +
> +static void handle_event(int event)
> +{
> +    if (event == PVEVENT_PANICKED) {

... here you don't test bits.  Weird.  Intentional?

> +        panicked_mon_event("pause");
> +        vm_stop(RUN_STATE_GUEST_PANICKED);
> +        return;
> +    }
> +}

Should we log events we don't understand?

> +
> +#include "hw/isa.h"
> +
> +typedef struct PVEventState {
> +    ISADevice parent_obj;
> +
> +    MemoryRegion io;
> +    uint16_t ioport;
> +} PVEventState;
> +
> +/* return supported events on read */
> +static uint64_t pvevent_ioport_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return PVEVENT_PANICKED;
> +}
> +
> +static void pvevent_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> +                                 unsigned size)
> +{
> +    handle_event(val);
> +}
[...]
Markus Armbruster - March 20, 2013, 9:16 a.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 14/03/2013 16:59, Gleb Natapov ha scritto:
[...]
>> Fine with me. That was just a suggestion. I thought we had singleton
>> qdev flag.
>
> We have no_user, but it's broken and not exactly a match for what you
> want here.

For what it's worth, no_user is set for this device.
Markus Armbruster - March 20, 2013, 9:24 a.m.
Alexander Graf <agraf@suse.de> writes:

> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
>
>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>>> 
>>>>> When guest panic happens, pvevent device driver will write a event
>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>>> by default). On receiving the event, pvevent device will pause guest
>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>>> 
>>>>> TODO: make the IO port configurable
>>>> 
>>>> The port is already configurable as far as the device is concerned; when
>>>> you add the port to the PC boards you will have to wind up fw-cfg.
>>> 
>>> Why not add fw-cfg when device is created (with -device for instance)?
>> 
>> It depends on what we decide is the supported interface for the device:
>> 
>> * it can be an ISA device; the interface is the I/O port and ACPI
>
> Is there any particular reason it's an ISA device with a PIO port,
> rather than a platform / sysbus device with MMIO access? With the
> latter, we could easily reuse the device on other platforms (ppc, arm)
> and even the guest driver code for platforms that do ACPI (arm?).

As Paolo and Gleb observed, this is the most natural way to do such a
device on x86.  Other systems may want different connectors,
e.g. sysbus/MMIO.

We already have devices coming in variants with different connectors,
e.g. serial and fdc.  They share core code just fine.  Some boilerplate
gets duplicated, but it's not horrible.

There has been talk about modelling such things more nicely in QOM.
Opportunity for a QOM expert to tell us more about it :)

[...]

Patch

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 40ebe46..edf499e 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -218,5 +218,7 @@  obj-$(CONFIG_KVM) += ivshmem.o
 obj-$(CONFIG_LINUX) += vfio_pci.o
 endif
 
+common-obj-y += pvevent.o
+
 $(obj)/baum.o: QEMU_CFLAGS += $(SDL_CFLAGS) 
 endif
diff --git a/hw/pvevent.c b/hw/pvevent.c
new file mode 100644
index 0000000..c7df77b
--- /dev/null
+++ b/hw/pvevent.c
@@ -0,0 +1,116 @@ 
+/*
+ * QEMU simulated pvevent device.
+ *
+ * Copyright Fujitsu, Corp. 2013
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *     Hu Tao <hutao@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <qapi/qmp/qobject.h>
+#include <qapi/qmp/qjson.h>
+#include <monitor/monitor.h>
+#include <sysemu/sysemu.h>
+#include <sysemu/kvm.h>
+
+/* The bit of supported pv event */
+#define PVEVENT_F_PANICKED      0
+
+/* The pv event value */
+#define PVEVENT_PANICKED        (1 << PVEVENT_F_PANICKED)
+
+#define TYPE_ISA_PVEVENT_DEVICE    "pvevent"
+#define ISA_PVEVENT_DEVICE(obj)    \
+    OBJECT_CHECK(PVEventState, (obj), TYPE_ISA_PVEVENT_DEVICE)
+
+static void panicked_mon_event(const char *action)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'action': %s }", action);
+    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+    qobject_decref(data);
+}
+
+static void handle_event(int event)
+{
+    if (event == PVEVENT_PANICKED) {
+        panicked_mon_event("pause");
+        vm_stop(RUN_STATE_GUEST_PANICKED);
+        return;
+    }
+}
+
+#include "hw/isa.h"
+
+typedef struct PVEventState {
+    ISADevice parent_obj;
+
+    MemoryRegion io;
+    uint16_t ioport;
+} PVEventState;
+
+/* return supported events on read */
+static uint64_t pvevent_ioport_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return PVEVENT_PANICKED;
+}
+
+static void pvevent_ioport_write(void *opaque, hwaddr addr, uint64_t val,
+                                 unsigned size)
+{
+    handle_event(val);
+}
+
+static const MemoryRegionOps pvevent_ops = {
+    .read = pvevent_ioport_read,
+    .write = pvevent_ioport_write,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static int pvevent_isa_initfn(ISADevice *dev)
+{
+    PVEventState *s = ISA_PVEVENT_DEVICE(dev);
+
+    memory_region_init_io(&s->io, &pvevent_ops, s, "pvevent", 1);
+    isa_register_ioport(dev, &s->io, s->ioport);
+
+    return 0;
+}
+
+static Property pvevent_isa_properties[] = {
+    DEFINE_PROP_UINT16("ioport", PVEventState, ioport, 0x505),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pvevent_isa_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+
+    ic->init = pvevent_isa_initfn;
+    dc->no_user = 1;
+    dc->props = pvevent_isa_properties;
+}
+
+static TypeInfo pvevent_isa_info = {
+    .name          = TYPE_ISA_PVEVENT_DEVICE,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(PVEventState),
+    .class_init    = pvevent_isa_class_init,
+};
+
+static void pvevent_register_types(void)
+{
+    type_register_static(&pvevent_isa_info);
+}
+
+type_init(pvevent_register_types)