* [Qemu-devel] [PATCH v3 1/7] i8254: Do not raise IRQ level on reset
2012-01-31 17:41 [Qemu-devel] [PATCH v3 0/7] pit, hpet, pcspk: fixes & preparation for KVM Jan Kiszka
@ 2012-01-31 17:41 ` Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level Jan Kiszka
` (5 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 17:41 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel; +Cc: Blue Swirl, Marcelo Tosatti, Avi Kivity, kvm
Avoid changing the IRQ level to high on reset as it may trigger spurious
events. Instead, open-code the effects of pit_load_count(0) in the reset
handler.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/i8254.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/hw/i8254.c b/hw/i8254.c
index add1fab..d4a08d7 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -481,7 +481,13 @@ static void pit_reset(DeviceState *dev)
s = &pit->channels[i];
s->mode = 3;
s->gate = (i != 2);
- pit_load_count(s, 0);
+ s->count_load_time = qemu_get_clock_ns(vm_clock);
+ s->count = 0x10000;
+ if (i == 0) {
+ s->next_transition_time =
+ pit_get_next_transition_time(s, s->count_load_time);
+ qemu_mod_timer(s->irq_timer, s->next_transition_time);
+ }
}
}
--
1.7.3.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level
2012-01-31 17:41 [Qemu-devel] [PATCH v3 0/7] pit, hpet, pcspk: fixes & preparation for KVM Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 1/7] i8254: Do not raise IRQ level on reset Jan Kiszka
@ 2012-01-31 17:41 ` Jan Kiszka
2012-01-31 21:02 ` Anthony Liguori
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 3/7] i8254: Factor out interface header Jan Kiszka
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 17:41 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel; +Cc: Blue Swirl, Marcelo Tosatti, Avi Kivity, kvm
In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
8 but keeps track of the RTC output level and applies it when legacy
mode is turned off again. This value has to be preserved across save/
restore as it cannot be reconstructed otherwise.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/hpet.c | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/hw/hpet.c b/hw/hpet.c
index aba9ea9..39b291f 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int version_id)
return 0;
}
+static bool hpet_rtc_irq_level_needed(void *opaque)
+{
+ HPETState *s = opaque;
+
+ return s->rtc_irq_level != 0;
+}
+
+static const VMStateDescription vmstate_hpet_rtc_irq_level = {
+ .name = "hpet/rtc_irq_level",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(rtc_irq_level, HPETState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_hpet_timer = {
.name = "hpet_timer",
.version_id = 1,
@@ -273,6 +291,14 @@ static const VMStateDescription vmstate_hpet = {
VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
vmstate_hpet_timer, HPETTimer),
VMSTATE_END_OF_LIST()
+ },
+ .subsections = (VMStateSubsection[]) {
+ {
+ .vmsd = &vmstate_hpet_rtc_irq_level,
+ .needed = hpet_rtc_irq_level_needed,
+ }, {
+ /* empty */
+ }
}
};
--
1.7.3.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level Jan Kiszka
@ 2012-01-31 21:02 ` Anthony Liguori
2012-01-31 22:05 ` Jan Kiszka
0 siblings, 1 reply; 19+ messages in thread
From: Anthony Liguori @ 2012-01-31 21:02 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Blue Swirl, Marcelo Tosatti, qemu-devel, kvm, Avi Kivity
On 01/31/2012 11:41 AM, Jan Kiszka wrote:
> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
> 8 but keeps track of the RTC output level and applies it when legacy
> mode is turned off again. This value has to be preserved across save/
> restore as it cannot be reconstructed otherwise.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
> hw/hpet.c | 26 ++++++++++++++++++++++++++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/hw/hpet.c b/hw/hpet.c
> index aba9ea9..39b291f 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int version_id)
> return 0;
> }
>
> +static bool hpet_rtc_irq_level_needed(void *opaque)
> +{
> + HPETState *s = opaque;
> +
> + return s->rtc_irq_level != 0;
> +}
> +
> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
> + .name = "hpet/rtc_irq_level",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .minimum_version_id_old = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8(rtc_irq_level, HPETState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
This won't work. We don't clear rtc_irq_level on reset so rtc_irq_level may be
high or low after reset. As such, we can't use a subsection here. We need to
bump the savevm state.
Regards,
Anthony Liguori
> static const VMStateDescription vmstate_hpet_timer = {
> .name = "hpet_timer",
> .version_id = 1,
> @@ -273,6 +291,14 @@ static const VMStateDescription vmstate_hpet = {
> VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
> vmstate_hpet_timer, HPETTimer),
> VMSTATE_END_OF_LIST()
> + },
> + .subsections = (VMStateSubsection[]) {
> + {
> + .vmsd =&vmstate_hpet_rtc_irq_level,
> + .needed = hpet_rtc_irq_level_needed,
> + }, {
> + /* empty */
> + }
> }
> };
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level
2012-01-31 21:02 ` Anthony Liguori
@ 2012-01-31 22:05 ` Jan Kiszka
2012-01-31 22:38 ` Anthony Liguori
0 siblings, 1 reply; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 22:05 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Blue Swirl, Marcelo Tosatti, qemu-devel, kvm, Avi Kivity
[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]
On 2012-01-31 22:02, Anthony Liguori wrote:
> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
>> 8 but keeps track of the RTC output level and applies it when legacy
>> mode is turned off again. This value has to be preserved across save/
>> restore as it cannot be reconstructed otherwise.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>> hw/hpet.c | 26 ++++++++++++++++++++++++++
>> 1 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index aba9ea9..39b291f 100644
>> --- a/hw/hpet.c
>> +++ b/hw/hpet.c
>> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int
>> version_id)
>> return 0;
>> }
>>
>> +static bool hpet_rtc_irq_level_needed(void *opaque)
>> +{
>> + HPETState *s = opaque;
>> +
>> + return s->rtc_irq_level != 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>> + .name = "hpet/rtc_irq_level",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .minimum_version_id_old = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT8(rtc_irq_level, HPETState),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>
> This won't work. We don't clear rtc_irq_level on reset so rtc_irq_level
> may be high or low after reset. As such, we can't use a subsection
> here. We need to bump the savevm state.
rtc_irq_level is updated during reset by the connected RTC device. It
clears its IRQ line.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level
2012-01-31 22:05 ` Jan Kiszka
@ 2012-01-31 22:38 ` Anthony Liguori
2012-01-31 22:39 ` Jan Kiszka
0 siblings, 1 reply; 19+ messages in thread
From: Anthony Liguori @ 2012-01-31 22:38 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Blue Swirl, Marcelo Tosatti, qemu-devel, kvm, Avi Kivity
On 01/31/2012 04:05 PM, Jan Kiszka wrote:
> On 2012-01-31 22:02, Anthony Liguori wrote:
>> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>>> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
>>> 8 but keeps track of the RTC output level and applies it when legacy
>>> mode is turned off again. This value has to be preserved across save/
>>> restore as it cannot be reconstructed otherwise.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>> ---
>>> hw/hpet.c | 26 ++++++++++++++++++++++++++
>>> 1 files changed, 26 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>> index aba9ea9..39b291f 100644
>>> --- a/hw/hpet.c
>>> +++ b/hw/hpet.c
>>> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int
>>> version_id)
>>> return 0;
>>> }
>>>
>>> +static bool hpet_rtc_irq_level_needed(void *opaque)
>>> +{
>>> + HPETState *s = opaque;
>>> +
>>> + return s->rtc_irq_level != 0;
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>>> + .name = "hpet/rtc_irq_level",
>>> + .version_id = 1,
>>> + .minimum_version_id = 1,
>>> + .minimum_version_id_old = 1,
>>> + .fields = (VMStateField[]) {
>>> + VMSTATE_UINT8(rtc_irq_level, HPETState),
>>> + VMSTATE_END_OF_LIST()
>>> + }
>>> +};
>>> +
>>
>> This won't work. We don't clear rtc_irq_level on reset so rtc_irq_level
>> may be high or low after reset. As such, we can't use a subsection
>> here. We need to bump the savevm state.
>
> rtc_irq_level is updated during reset by the connected RTC device. It
> clears its IRQ line.
Ah, that's subtle and warrants a comment :-)
Probably worth adding an s->rtc_irq_level = 0 in reset just to be extra safe.
Regards,
Anthony Liguori
>
> Jan
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level
2012-01-31 22:38 ` Anthony Liguori
@ 2012-01-31 22:39 ` Jan Kiszka
0 siblings, 0 replies; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 22:39 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Blue Swirl, Marcelo Tosatti, qemu-devel, kvm, Avi Kivity
[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]
On 2012-01-31 23:38, Anthony Liguori wrote:
> On 01/31/2012 04:05 PM, Jan Kiszka wrote:
>> On 2012-01-31 22:02, Anthony Liguori wrote:
>>> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>>>> In legacy mode, the HPET suppresses the RTC interrupt delivery via IRQ
>>>> 8 but keeps track of the RTC output level and applies it when legacy
>>>> mode is turned off again. This value has to be preserved across save/
>>>> restore as it cannot be reconstructed otherwise.
>>>>
>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>> ---
>>>> hw/hpet.c | 26 ++++++++++++++++++++++++++
>>>> 1 files changed, 26 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>>> index aba9ea9..39b291f 100644
>>>> --- a/hw/hpet.c
>>>> +++ b/hw/hpet.c
>>>> @@ -240,6 +240,24 @@ static int hpet_post_load(void *opaque, int
>>>> version_id)
>>>> return 0;
>>>> }
>>>>
>>>> +static bool hpet_rtc_irq_level_needed(void *opaque)
>>>> +{
>>>> + HPETState *s = opaque;
>>>> +
>>>> + return s->rtc_irq_level != 0;
>>>> +}
>>>> +
>>>> +static const VMStateDescription vmstate_hpet_rtc_irq_level = {
>>>> + .name = "hpet/rtc_irq_level",
>>>> + .version_id = 1,
>>>> + .minimum_version_id = 1,
>>>> + .minimum_version_id_old = 1,
>>>> + .fields = (VMStateField[]) {
>>>> + VMSTATE_UINT8(rtc_irq_level, HPETState),
>>>> + VMSTATE_END_OF_LIST()
>>>> + }
>>>> +};
>>>> +
>>>
>>> This won't work. We don't clear rtc_irq_level on reset so rtc_irq_level
>>> may be high or low after reset. As such, we can't use a subsection
>>> here. We need to bump the savevm state.
>>
>> rtc_irq_level is updated during reset by the connected RTC device. It
>> clears its IRQ line.
>
> Ah, that's subtle and warrants a comment :-)
>
> Probably worth adding an s->rtc_irq_level = 0 in reset just to be extra
> safe.
Agreed. Will do.
Thanks,
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 3/7] i8254: Factor out interface header
2012-01-31 17:41 [Qemu-devel] [PATCH v3 0/7] pit, hpet, pcspk: fixes & preparation for KVM Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 1/7] i8254: Do not raise IRQ level on reset Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 2/7] hpet: Save/restore cached RTC IRQ level Jan Kiszka
@ 2012-01-31 17:41 ` Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 4/7] i8254: Pass alternative IRQ output object on initialization Jan Kiszka
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 17:41 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel; +Cc: Blue Swirl, Marcelo Tosatti, Avi Kivity, kvm
Move the public interface of the PIT into its own header file and update
all users.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/alpha_dp264.c | 1 +
hw/hpet.c | 1 +
hw/i82378.c | 1 +
hw/i8254.c | 1 +
hw/i8254.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/mips_fulong2e.c | 1 +
hw/mips_jazz.c | 1 +
hw/mips_malta.c | 1 +
hw/mips_r4k.c | 1 +
hw/pc.c | 1 +
hw/pc.h | 25 ------------------------
hw/pcspk.c | 1 +
12 files changed, 64 insertions(+), 25 deletions(-)
create mode 100644 hw/i8254.h
diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
index 876335a..4c0efd3 100644
--- a/hw/alpha_dp264.c
+++ b/hw/alpha_dp264.c
@@ -14,6 +14,7 @@
#include "sysemu.h"
#include "mc146818rtc.h"
#include "ide.h"
+#include "i8254.h"
#define MAX_IDE_BUS 2
diff --git a/hw/hpet.c b/hw/hpet.c
index 39b291f..c921ec9 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -31,6 +31,7 @@
#include "hpet_emul.h"
#include "sysbus.h"
#include "mc146818rtc.h"
+#include "i8254.h"
//#define HPET_DEBUG
#ifdef HPET_DEBUG
diff --git a/hw/i82378.c b/hw/i82378.c
index 99b453a..652970e 100644
--- a/hw/i82378.c
+++ b/hw/i82378.c
@@ -19,6 +19,7 @@
#include "pci.h"
#include "pc.h"
+#include "i8254.h"
//#define DEBUG_I82378
diff --git a/hw/i8254.c b/hw/i8254.c
index d4a08d7..df83880 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -25,6 +25,7 @@
#include "pc.h"
#include "isa.h"
#include "qemu-timer.h"
+#include "i8254.h"
//#define DEBUG_PIT
diff --git a/hw/i8254.h b/hw/i8254.h
new file mode 100644
index 0000000..cd3111c
--- /dev/null
+++ b/hw/i8254.h
@@ -0,0 +1,54 @@
+/*
+ * QEMU 8253/8254 interval timer emulation
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_I8254_H
+#define HW_I8254_H
+
+#include "hw.h"
+#include "isa.h"
+
+#define PIT_FREQ 1193182
+
+static inline ISADevice *pit_init(ISABus *bus, int base, int irq)
+{
+ ISADevice *dev;
+
+ dev = isa_create(bus, "isa-pit");
+ qdev_prop_set_uint32(&dev->qdev, "iobase", base);
+ qdev_prop_set_uint32(&dev->qdev, "irq", irq);
+ qdev_init_nofail(&dev->qdev);
+
+ return dev;
+}
+
+void pit_set_gate(ISADevice *dev, int channel, int val);
+int pit_get_gate(ISADevice *dev, int channel);
+int pit_get_initial_count(ISADevice *dev, int channel);
+int pit_get_mode(ISADevice *dev, int channel);
+int pit_get_out(ISADevice *dev, int channel, int64_t current_time);
+
+void hpet_pit_disable(void);
+void hpet_pit_enable(void);
+
+#endif /* !HW_I8254_H */
diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
index 163a668..ead72ae 100644
--- a/hw/mips_fulong2e.c
+++ b/hw/mips_fulong2e.c
@@ -40,6 +40,7 @@
#include "elf.h"
#include "vt82c686.h"
#include "mc146818rtc.h"
+#include "i8254.h"
#include "blockdev.h"
#include "exec-memory.h"
diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
index 63165b9..61dee4d 100644
--- a/hw/mips_jazz.c
+++ b/hw/mips_jazz.c
@@ -36,6 +36,7 @@
#include "mips-bios.h"
#include "loader.h"
#include "mc146818rtc.h"
+#include "i8254.h"
#include "blockdev.h"
#include "sysbus.h"
#include "exec-memory.h"
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index 64603ce..32f9f65 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -45,6 +45,7 @@
#include "loader.h"
#include "elf.h"
#include "mc146818rtc.h"
+#include "i8254.h"
#include "blockdev.h"
#include "exec-memory.h"
#include "sysbus.h" /* SysBusDevice */
diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
index 1c0615c..1b3ec2d 100644
--- a/hw/mips_r4k.c
+++ b/hw/mips_r4k.c
@@ -22,6 +22,7 @@
#include "loader.h"
#include "elf.h"
#include "mc146818rtc.h"
+#include "i8254.h"
#include "blockdev.h"
#include "exec-memory.h"
diff --git a/hw/pc.c b/hw/pc.c
index 31608d3..080a731 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -36,6 +36,7 @@
#include "elf.h"
#include "multiboot.h"
#include "mc146818rtc.h"
+#include "i8254.h"
#include "msi.h"
#include "sysbus.h"
#include "sysemu.h"
diff --git a/hw/pc.h b/hw/pc.h
index c666ec9..b08708d 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -81,31 +81,6 @@ typedef struct GSIState {
void gsi_handler(void *opaque, int n, int level);
-/* i8254.c */
-
-#define PIT_FREQ 1193182
-
-static inline ISADevice *pit_init(ISABus *bus, int base, int irq)
-{
- ISADevice *dev;
-
- dev = isa_create(bus, "isa-pit");
- qdev_prop_set_uint32(&dev->qdev, "iobase", base);
- qdev_prop_set_uint32(&dev->qdev, "irq", irq);
- qdev_init_nofail(&dev->qdev);
-
- return dev;
-}
-
-void pit_set_gate(ISADevice *dev, int channel, int val);
-int pit_get_gate(ISADevice *dev, int channel);
-int pit_get_initial_count(ISADevice *dev, int channel);
-int pit_get_mode(ISADevice *dev, int channel);
-int pit_get_out(ISADevice *dev, int channel, int64_t current_time);
-
-void hpet_pit_disable(void);
-void hpet_pit_enable(void);
-
/* vmport.c */
static inline void vmport_init(ISABus *bus)
{
diff --git a/hw/pcspk.c b/hw/pcspk.c
index acb0167..43df818 100644
--- a/hw/pcspk.c
+++ b/hw/pcspk.c
@@ -27,6 +27,7 @@
#include "isa.h"
#include "audio/audio.h"
#include "qemu-timer.h"
+#include "i8254.h"
#define PCSPK_BUF_LEN 1792
#define PCSPK_SAMPLE_RATE 32000
--
1.7.3.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH v3 4/7] i8254: Pass alternative IRQ output object on initialization
2012-01-31 17:41 [Qemu-devel] [PATCH v3 0/7] pit, hpet, pcspk: fixes & preparation for KVM Jan Kiszka
` (2 preceding siblings ...)
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 3/7] i8254: Factor out interface header Jan Kiszka
@ 2012-01-31 17:41 ` Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 5/7] i8254: Rework & fix interaction with HPET in legacy mode Jan Kiszka
` (2 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 17:41 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel; +Cc: Blue Swirl, Marcelo Tosatti, Avi Kivity, kvm
HPET legacy emulation will require control over the PIT IRQ output. To
enable this, add support for an alternative IRQ output object to the PIT
factory function. If the isa_irq number is < 0, this object will be
used.
This also removes the IRQ number property from the PIT class as we now
use a generic GPIO output pin that is connected by the factory function.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/alpha_dp264.c | 2 +-
hw/i82378.c | 2 +-
hw/i8254.c | 4 +---
hw/i8254.h | 6 ++++--
hw/mips_fulong2e.c | 2 +-
hw/mips_jazz.c | 2 +-
hw/mips_malta.c | 2 +-
hw/mips_r4k.c | 2 +-
hw/pc.c | 2 +-
9 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
index 4c0efd3..ea0fd95 100644
--- a/hw/alpha_dp264.c
+++ b/hw/alpha_dp264.c
@@ -73,7 +73,7 @@ static void clipper_init(ram_addr_t ram_size,
clipper_pci_map_irq);
rtc_init(isa_bus, 1980, rtc_irq);
- pit_init(isa_bus, 0x40, 0);
+ pit_init(isa_bus, 0x40, 0, NULL);
isa_create_simple(isa_bus, "i8042");
/* VGA setup. Don't bother loading the bios. */
diff --git a/hw/i82378.c b/hw/i82378.c
index 652970e..127cadf 100644
--- a/hw/i82378.c
+++ b/hw/i82378.c
@@ -192,7 +192,7 @@ static void i82378_init(DeviceState *dev, I82378State *s)
isa_bus_irqs(isabus, s->i8259);
/* 1 82C54 (pit) */
- pit = pit_init(isabus, 0x40, 0);
+ pit = pit_init(isabus, 0x40, 0, NULL);
/* speaker */
pcspk_init(pit);
diff --git a/hw/i8254.c b/hw/i8254.c
index df83880..326211a 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -57,7 +57,6 @@ typedef struct PITChannelState {
typedef struct PITState {
ISADevice dev;
MemoryRegion ioports;
- uint32_t irq;
uint32_t iobase;
PITChannelState channels[3];
} PITState;
@@ -532,7 +531,7 @@ static int pit_initfn(ISADevice *dev)
s = &pit->channels[0];
/* the timer 0 is connected to an IRQ */
s->irq_timer = qemu_new_timer_ns(vm_clock, pit_irq_timer, s);
- s->irq = isa_get_irq(dev, pit->irq);
+ qdev_init_gpio_out(&dev->qdev, &s->irq, 1);
memory_region_init_io(&pit->ioports, &pit_ioport_ops, pit, "pit", 4);
isa_register_ioport(dev, &pit->ioports, pit->iobase);
@@ -556,7 +555,6 @@ static DeviceInfo pit_info = {
.no_user = 1,
.class_init = pit_class_initfn,
.props = (Property[]) {
- DEFINE_PROP_UINT32("irq", PITState, irq, -1),
DEFINE_PROP_HEX32("iobase", PITState, iobase, -1),
DEFINE_PROP_END_OF_LIST(),
},
diff --git a/hw/i8254.h b/hw/i8254.h
index cd3111c..fc64a63 100644
--- a/hw/i8254.h
+++ b/hw/i8254.h
@@ -30,14 +30,16 @@
#define PIT_FREQ 1193182
-static inline ISADevice *pit_init(ISABus *bus, int base, int irq)
+static inline ISADevice *pit_init(ISABus *bus, int base, int isa_irq,
+ qemu_irq alt_irq)
{
ISADevice *dev;
dev = isa_create(bus, "isa-pit");
qdev_prop_set_uint32(&dev->qdev, "iobase", base);
- qdev_prop_set_uint32(&dev->qdev, "irq", irq);
qdev_init_nofail(&dev->qdev);
+ qdev_connect_gpio_out(&dev->qdev, 0,
+ isa_irq >= 0 ? isa_get_irq(dev, isa_irq) : alt_irq);
return dev;
}
diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
index ead72ae..e3ba9dd 100644
--- a/hw/mips_fulong2e.c
+++ b/hw/mips_fulong2e.c
@@ -364,7 +364,7 @@ static void mips_fulong2e_init(ram_addr_t ram_size, const char *boot_device,
smbus_eeprom_init(smbus, 1, eeprom_spd, sizeof(eeprom_spd));
/* init other devices */
- pit = pit_init(isa_bus, 0x40, 0);
+ pit = pit_init(isa_bus, 0x40, 0, NULL);
cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
DMA_init(0, cpu_exit_irq);
diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
index 61dee4d..b61b218 100644
--- a/hw/mips_jazz.c
+++ b/hw/mips_jazz.c
@@ -192,7 +192,7 @@ static void mips_jazz_init(MemoryRegion *address_space,
isa_bus_irqs(isa_bus, i8259);
cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
DMA_init(0, cpu_exit_irq);
- pit = pit_init(isa_bus, 0x40, 0);
+ pit = pit_init(isa_bus, 0x40, 0, NULL);
pcspk_init(pit);
/* ISA IO space at 0x90000000 */
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index 32f9f65..f611701 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -970,7 +970,7 @@ void mips_malta_init (ram_addr_t ram_size,
isa_get_irq(NULL, 9), NULL, NULL, 0);
/* TODO: Populate SPD eeprom data. */
smbus_eeprom_init(smbus, 8, NULL, 0);
- pit = pit_init(isa_bus, 0x40, 0);
+ pit = pit_init(isa_bus, 0x40, 0, NULL);
cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
DMA_init(0, cpu_exit_irq);
diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
index 1b3ec2d..83401f0 100644
--- a/hw/mips_r4k.c
+++ b/hw/mips_r4k.c
@@ -270,7 +270,7 @@ void mips_r4k_init (ram_addr_t ram_size,
isa_mmio_init(0x14000000, 0x00010000);
isa_mem_base = 0x10000000;
- pit = pit_init(isa_bus, 0x40, 0);
+ pit = pit_init(isa_bus, 0x40, 0, NULL);
for(i = 0; i < MAX_SERIAL_PORTS; i++) {
if (serial_hds[i]) {
diff --git a/hw/pc.c b/hw/pc.c
index 080a731..223678d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1159,7 +1159,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
qemu_register_boot_set(pc_boot_set, *rtc_state);
- pit = pit_init(isa_bus, 0x40, 0);
+ pit = pit_init(isa_bus, 0x40, 0, NULL);
pcspk_init(pit);
for(i = 0; i < MAX_SERIAL_PORTS; i++) {
--
1.7.3.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH v3 5/7] i8254: Rework & fix interaction with HPET in legacy mode
2012-01-31 17:41 [Qemu-devel] [PATCH v3 0/7] pit, hpet, pcspk: fixes & preparation for KVM Jan Kiszka
` (3 preceding siblings ...)
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 4/7] i8254: Pass alternative IRQ output object on initialization Jan Kiszka
@ 2012-01-31 17:41 ` Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev Jan Kiszka
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 7/7] i8254: Factor out pit_get_channel_info Jan Kiszka
6 siblings, 0 replies; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 17:41 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel; +Cc: Blue Swirl, Marcelo Tosatti, Avi Kivity, kvm
When the HPET enters legacy mode, the IRQ output of the PIT is
suppressed and replaced by the HPET timer 0. But the current code to
emulate this was broken in many ways. It reset the PIT state after
re-enabling, it worked against a stale static PIT structure, and it did
not properly saved/restored the IRQ output mask in the PIT vmstate.
This patch solves the PIT IRQ control in a different way. On x86, it
both redirects the PIT IRQ to the HPET, just like the RTC. But it also
keeps the control line from the HPET to the PIT. This allows to disable
the PIT QEMU timer when it is not needed. The PIT's view on the control
line state is now saved in the same format that qemu-kvm is already
using.
Note that, in contrast to the suppressed RTC IRQ line, we do not need to
save/restore the PIT line state in the HPET. As we trigger a PIT IRQ
update via the control line, the line state is reconstructed on mode
switch.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/hpet.c | 38 ++++++++++++++++++++------------------
hw/hpet_emul.h | 3 +++
hw/i8254.c | 46 ++++++++++++++++++++++------------------------
hw/i8254.h | 3 ---
hw/pc.c | 15 ++++++++++++---
5 files changed, 57 insertions(+), 48 deletions(-)
diff --git a/hw/hpet.c b/hw/hpet.c
index c921ec9..9fbbd47 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -65,6 +65,7 @@ typedef struct HPETState {
qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
uint32_t flags;
uint8_t rtc_irq_level;
+ qemu_irq pit_enabled;
uint8_t num_timers;
HPETTimer timer[HPET_MAX_TIMERS];
@@ -573,12 +574,15 @@ static void hpet_ram_write(void *opaque, target_phys_addr_t addr,
hpet_del_timer(&s->timer[i]);
}
}
- /* i8254 and RTC are disabled when HPET is in legacy mode */
+ /* i8254 and RTC output pins are disabled
+ * when HPET is in legacy mode */
if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
- hpet_pit_disable();
+ qemu_set_irq(s->pit_enabled, 0);
+ qemu_irq_lower(s->irqs[0]);
qemu_irq_lower(s->irqs[RTC_ISA_IRQ]);
} else if (deactivating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
- hpet_pit_enable();
+ qemu_irq_lower(s->irqs[0]);
+ qemu_set_irq(s->pit_enabled, 1);
qemu_set_irq(s->irqs[RTC_ISA_IRQ], s->rtc_irq_level);
}
break;
@@ -632,7 +636,6 @@ static void hpet_reset(DeviceState *d)
{
HPETState *s = FROM_SYSBUS(HPETState, sysbus_from_qdev(d));
int i;
- static int count = 0;
for (i = 0; i < s->num_timers; i++) {
HPETTimer *timer = &s->timer[i];
@@ -649,29 +652,27 @@ static void hpet_reset(DeviceState *d)
timer->wrap_flag = 0;
}
+ qemu_set_irq(s->pit_enabled, 1);
s->hpet_counter = 0ULL;
s->hpet_offset = 0ULL;
s->config = 0ULL;
- if (count > 0) {
- /* we don't enable pit when hpet_reset is first called (by hpet_init)
- * because hpet is taking over for pit here. On subsequent invocations,
- * hpet_reset is called due to system reset. At this point control must
- * be returned to pit until SW reenables hpet.
- */
- hpet_pit_enable();
- }
hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
hpet_cfg.hpet[s->hpet_id].address = sysbus_from_qdev(d)->mmio[0].addr;
- count = 1;
}
-static void hpet_handle_rtc_irq(void *opaque, int n, int level)
+static void hpet_handle_legacy_irq(void *opaque, int n, int level)
{
HPETState *s = FROM_SYSBUS(HPETState, opaque);
- s->rtc_irq_level = level;
- if (!hpet_in_legacy_mode(s)) {
- qemu_set_irq(s->irqs[RTC_ISA_IRQ], level);
+ if (n == HPET_LEGACY_PIT_INT) {
+ if (!hpet_in_legacy_mode(s)) {
+ qemu_set_irq(s->irqs[0], level);
+ }
+ } else {
+ s->rtc_irq_level = level;
+ if (!hpet_in_legacy_mode(s)) {
+ qemu_set_irq(s->irqs[RTC_ISA_IRQ], level);
+ }
}
}
@@ -714,7 +715,8 @@ static int hpet_init(SysBusDevice *dev)
s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
s->capability |= ((HPET_CLK_PERIOD) << 32);
- qdev_init_gpio_in(&dev->qdev, hpet_handle_rtc_irq, 1);
+ qdev_init_gpio_in(&dev->qdev, hpet_handle_legacy_irq, 2);
+ qdev_init_gpio_out(&dev->qdev, &s->pit_enabled, 1);
/* HPET Area */
memory_region_init_io(&s->iomem, &hpet_ram_ops, s, "hpet", 0x400);
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 6128702..757f79f 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -22,6 +22,9 @@
#define HPET_NUM_IRQ_ROUTES 32
+#define HPET_LEGACY_PIT_INT 0
+#define HPET_LEGACY_RTC_INT 1
+
#define HPET_CFG_ENABLE 0x001
#define HPET_CFG_LEGACY 0x002
diff --git a/hw/i8254.c b/hw/i8254.c
index 326211a..6412277 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -52,6 +52,7 @@ typedef struct PITChannelState {
int64_t next_transition_time;
QEMUTimer *irq_timer;
qemu_irq irq;
+ uint32_t irq_disabled;
} PITChannelState;
typedef struct PITState {
@@ -61,8 +62,6 @@ typedef struct PITState {
PITChannelState channels[3];
} PITState;
-static PITState pit_state;
-
static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
static int pit_get_count(PITChannelState *s)
@@ -378,8 +377,9 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
int64_t expire_time;
int irq_level;
- if (!s->irq_timer)
+ if (!s->irq_timer || s->irq_disabled) {
return;
+ }
expire_time = pit_get_next_transition_time(s, current_time);
irq_level = pit_get_out1(s, current_time);
qemu_set_irq(s->irq, irq_level);
@@ -450,6 +450,7 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
qemu_get_8s(f, &s->bcd);
qemu_get_8s(f, &s->gate);
s->count_load_time=qemu_get_be64(f);
+ s->irq_disabled = 0;
if (s->irq_timer) {
s->next_transition_time=qemu_get_be64(f);
qemu_get_timer(f, s->irq_timer);
@@ -460,11 +461,12 @@ static int pit_load_old(QEMUFile *f, void *opaque, int version_id)
static const VMStateDescription vmstate_pit = {
.name = "i8254",
- .version_id = 2,
+ .version_id = 3,
.minimum_version_id = 2,
.minimum_version_id_old = 1,
.load_state_old = pit_load_old,
.fields = (VMStateField []) {
+ VMSTATE_UINT32_V(channels[0].irq_disabled, PITState, 3),
VMSTATE_STRUCT_ARRAY(channels, PITState, 3, 2, vmstate_pit_channel, PITChannelState),
VMSTATE_TIMER(channels[0].irq_timer, PITState),
VMSTATE_END_OF_LIST()
@@ -483,7 +485,7 @@ static void pit_reset(DeviceState *dev)
s->gate = (i != 2);
s->count_load_time = qemu_get_clock_ns(vm_clock);
s->count = 0x10000;
- if (i == 0) {
+ if (i == 0 && !s->irq_disabled) {
s->next_transition_time =
pit_get_next_transition_time(s, s->count_load_time);
qemu_mod_timer(s->irq_timer, s->next_transition_time);
@@ -491,26 +493,20 @@ static void pit_reset(DeviceState *dev)
}
}
-/* When HPET is operating in legacy mode, i8254 timer0 is disabled */
-void hpet_pit_disable(void) {
- PITChannelState *s;
- s = &pit_state.channels[0];
- if (s->irq_timer)
- qemu_del_timer(s->irq_timer);
-}
-
-/* When HPET is reset or leaving legacy mode, it must reenable i8254
- * timer 0
- */
-
-void hpet_pit_enable(void)
+/* When HPET is operating in legacy mode, suppress the ignored timer IRQ,
+ * reenable it when legacy mode is left again. */
+static void pit_irq_control(void *opaque, int n, int enable)
{
- PITState *pit = &pit_state;
- PITChannelState *s;
- s = &pit->channels[0];
- s->mode = 3;
- s->gate = 1;
- pit_load_count(s, 0);
+ PITState *pit = opaque;
+ PITChannelState *s = &pit->channels[0];
+
+ if (enable) {
+ s->irq_disabled = 0;
+ pit_irq_timer_update(s, qemu_get_clock_ns(vm_clock));
+ } else {
+ s->irq_disabled = 1;
+ qemu_del_timer(s->irq_timer);
+ }
}
static const MemoryRegionPortio pit_portio[] = {
@@ -536,6 +532,8 @@ static int pit_initfn(ISADevice *dev)
memory_region_init_io(&pit->ioports, &pit_ioport_ops, pit, "pit", 4);
isa_register_ioport(dev, &pit->ioports, pit->iobase);
+ qdev_init_gpio_in(&dev->qdev, pit_irq_control, 1);
+
qdev_set_legacy_instance_id(&dev->qdev, pit->iobase, 2);
return 0;
diff --git a/hw/i8254.h b/hw/i8254.h
index fc64a63..8ad8e07 100644
--- a/hw/i8254.h
+++ b/hw/i8254.h
@@ -50,7 +50,4 @@ int pit_get_initial_count(ISADevice *dev, int channel);
int pit_get_mode(ISADevice *dev, int channel);
int pit_get_out(ISADevice *dev, int channel, int64_t current_time);
-void hpet_pit_disable(void);
-void hpet_pit_enable(void);
-
#endif /* !HW_I8254_H */
diff --git a/hw/pc.c b/hw/pc.c
index 223678d..6fb1de7 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1136,6 +1136,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
{
int i;
DriveInfo *fd[MAX_FD];
+ DeviceState *hpet = NULL;
+ int pit_isa_irq = 0;
+ qemu_irq pit_alt_irq = NULL;
qemu_irq rtc_irq = NULL;
qemu_irq *a20_line;
ISADevice *i8042, *port92, *vmmouse, *pit;
@@ -1146,20 +1149,26 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
if (!no_hpet) {
- DeviceState *hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
+ hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
if (hpet) {
for (i = 0; i < GSI_NUM_PINS; i++) {
sysbus_connect_irq(sysbus_from_qdev(hpet), i, gsi[i]);
}
- rtc_irq = qdev_get_gpio_in(hpet, 0);
+ pit_isa_irq = -1;
+ pit_alt_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_PIT_INT);
+ rtc_irq = qdev_get_gpio_in(hpet, HPET_LEGACY_RTC_INT);
}
}
*rtc_state = rtc_init(isa_bus, 2000, rtc_irq);
qemu_register_boot_set(pc_boot_set, *rtc_state);
- pit = pit_init(isa_bus, 0x40, 0, NULL);
+ pit = pit_init(isa_bus, 0x40, pit_isa_irq, pit_alt_irq);
+ if (hpet) {
+ /* connect PIT to output control line of the HPET */
+ qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0));
+ }
pcspk_init(pit);
for(i = 0; i < MAX_SERIAL_PORTS; i++) {
--
1.7.3.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-01-31 17:41 [Qemu-devel] [PATCH v3 0/7] pit, hpet, pcspk: fixes & preparation for KVM Jan Kiszka
` (4 preceding siblings ...)
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 5/7] i8254: Rework & fix interaction with HPET in legacy mode Jan Kiszka
@ 2012-01-31 17:41 ` Jan Kiszka
2012-01-31 20:49 ` Anthony Liguori
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 7/7] i8254: Factor out pit_get_channel_info Jan Kiszka
6 siblings, 1 reply; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 17:41 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel; +Cc: Blue Swirl, Marcelo Tosatti, Avi Kivity, kvm
Convert the PC speaker device to a qdev ISA model. Move the public
interface to a dedicated header file at this chance.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
arch_init.c | 1 +
hw/i82378.c | 3 +-
hw/mips_jazz.c | 3 +-
hw/pc.c | 3 +-
hw/pc.h | 4 ---
hw/pcspk.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++---------
hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++
7 files changed, 104 insertions(+), 17 deletions(-)
create mode 100644 hw/pcspk.h
diff --git a/arch_init.c b/arch_init.c
index 2366511..a45485b 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -42,6 +42,7 @@
#include "gdbstub.h"
#include "hw/smbios.h"
#include "exec-memory.h"
+#include "hw/pcspk.h"
#ifdef TARGET_SPARC
int graphic_width = 1024;
diff --git a/hw/i82378.c b/hw/i82378.c
index 127cadf..79fa8b7 100644
--- a/hw/i82378.c
+++ b/hw/i82378.c
@@ -20,6 +20,7 @@
#include "pci.h"
#include "pc.h"
#include "i8254.h"
+#include "pcspk.h"
//#define DEBUG_I82378
@@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, I82378State *s)
pit = pit_init(isabus, 0x40, 0, NULL);
/* speaker */
- pcspk_init(pit);
+ pcspk_init(isabus, pit);
/* 2 82C37 (dma) */
DMA_init(1, &s->out[1]);
diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
index b61b218..65608dc 100644
--- a/hw/mips_jazz.c
+++ b/hw/mips_jazz.c
@@ -37,6 +37,7 @@
#include "loader.h"
#include "mc146818rtc.h"
#include "i8254.h"
+#include "pcspk.h"
#include "blockdev.h"
#include "sysbus.h"
#include "exec-memory.h"
@@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion *address_space,
cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
DMA_init(0, cpu_exit_irq);
pit = pit_init(isa_bus, 0x40, 0, NULL);
- pcspk_init(pit);
+ pcspk_init(isa_bus, pit);
/* ISA IO space at 0x90000000 */
isa_mmio_init(0x90000000, 0x01000000);
diff --git a/hw/pc.c b/hw/pc.c
index 6fb1de7..f6a91a9 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -37,6 +37,7 @@
#include "multiboot.h"
#include "mc146818rtc.h"
#include "i8254.h"
+#include "pcspk.h"
#include "msi.h"
#include "sysbus.h"
#include "sysemu.h"
@@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
/* connect PIT to output control line of the HPET */
qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0));
}
- pcspk_init(pit);
+ pcspk_init(isa_bus, pit);
for(i = 0; i < MAX_SERIAL_PORTS; i++) {
if (serial_hds[i]) {
diff --git a/hw/pc.h b/hw/pc.h
index b08708d..1b47bbd 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
/* hpet.c */
extern int no_hpet;
-/* pcspk.c */
-void pcspk_init(ISADevice *pit);
-int pcspk_audio_init(ISABus *bus);
-
/* piix_pci.c */
struct PCII440FXState;
typedef struct PCII440FXState PCII440FXState;
diff --git a/hw/pcspk.c b/hw/pcspk.c
index 43df818..5bd9e32 100644
--- a/hw/pcspk.c
+++ b/hw/pcspk.c
@@ -28,6 +28,7 @@
#include "audio/audio.h"
#include "qemu-timer.h"
#include "i8254.h"
+#include "pcspk.h"
#define PCSPK_BUF_LEN 1792
#define PCSPK_SAMPLE_RATE 32000
@@ -35,10 +36,13 @@
#define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / PCSPK_MAX_FREQ)
typedef struct {
+ ISADevice dev;
+ MemoryRegion ioport;
+ uint32_t iobase;
uint8_t sample_buf[PCSPK_BUF_LEN];
QEMUSoundCard card;
SWVoiceOut *voice;
- ISADevice *pit;
+ void *pit;
unsigned int pit_count;
unsigned int samples;
unsigned int play_pos;
@@ -47,7 +51,7 @@ typedef struct {
} PCSpkState;
static const char *s_spk = "pcspk";
-static PCSpkState pcspk_state;
+static PCSpkState *pcspk_state;
static inline void generate_samples(PCSpkState *s)
{
@@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free)
int pcspk_audio_init(ISABus *bus)
{
- PCSpkState *s = &pcspk_state;
+ PCSpkState *s = pcspk_state;
struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0};
AUD_register_card(s_spk, &s->card);
@@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus)
return 0;
}
-static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
+static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr,
+ unsigned size)
{
PCSpkState *s = opaque;
int out;
@@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
return pit_get_gate(s->pit, 2) | (s->data_on << 1) | s->dummy_refresh_clock | out;
}
-static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val)
+static void pcspk_io_write(void *opaque, target_phys_addr_t addr, uint64_t val,
+ unsigned size)
{
PCSpkState *s = opaque;
const int gate = val & 1;
@@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val)
}
}
-void pcspk_init(ISADevice *pit)
+static const MemoryRegionOps pcspk_io_ops = {
+ .read = pcspk_io_read,
+ .write = pcspk_io_write,
+ .impl = {
+ .min_access_size = 1,
+ .max_access_size = 1,
+ },
+};
+
+static int pcspk_initfn(ISADevice *dev)
+{
+ PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
+
+ memory_region_init_io(&s->ioport, &pcspk_io_ops, s, "elcr", 1);
+ isa_register_ioport(NULL, &s->ioport, s->iobase);
+
+ pcspk_state = s;
+
+ return 0;
+}
+
+static void pcspk_class_initfn(ObjectClass *klass, void *data)
{
- PCSpkState *s = &pcspk_state;
+ ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+ ic->init = pcspk_initfn;
+}
- s->pit = pit;
- register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s);
- register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s);
+static DeviceInfo pcspk_info = {
+ .name = "isa-pcspk",
+ .size = sizeof(PCSpkState),
+ .no_user = 1,
+ .props = (Property[]) {
+ DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1),
+ DEFINE_PROP_PTR("pit", PCSpkState, pit),
+ DEFINE_PROP_END_OF_LIST(),
+ },
+ .class_init = pcspk_class_initfn,
+};
+
+static void pcspk_register(void)
+{
+ isa_qdev_register(&pcspk_info);
}
+device_init(pcspk_register)
diff --git a/hw/pcspk.h b/hw/pcspk.h
new file mode 100644
index 0000000..7f42bac
--- /dev/null
+++ b/hw/pcspk.h
@@ -0,0 +1,45 @@
+/*
+ * QEMU PC speaker emulation
+ *
+ * Copyright (c) 2006 Joachim Henke
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_PCSPK_H
+#define HW_PCSPK_H
+
+#include "hw.h"
+#include "isa.h"
+
+static inline ISADevice *pcspk_init(ISABus *bus, ISADevice *pit)
+{
+ ISADevice *dev;
+
+ dev = isa_create(bus, "isa-pcspk");
+ qdev_prop_set_uint32(&dev->qdev, "iobase", 0x61);
+ qdev_prop_set_ptr(&dev->qdev, "pit", pit);
+ qdev_init_nofail(&dev->qdev);
+
+ return dev;
+}
+
+int pcspk_audio_init(ISABus *bus);
+
+#endif /* !HW_PCSPK_H */
--
1.7.3.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev Jan Kiszka
@ 2012-01-31 20:49 ` Anthony Liguori
2012-01-31 22:00 ` Jan Kiszka
2012-02-01 7:29 ` Paolo Bonzini
0 siblings, 2 replies; 19+ messages in thread
From: Anthony Liguori @ 2012-01-31 20:49 UTC (permalink / raw)
To: Jan Kiszka
Cc: Anthony Liguori, kvm, Marcelo Tosatti, qemu-devel, Blue Swirl,
Avi Kivity
On 01/31/2012 11:41 AM, Jan Kiszka wrote:
> Convert the PC speaker device to a qdev ISA model. Move the public
> interface to a dedicated header file at this chance.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
Heh, I did this too more or less the same way. Some comments below:
> ---
> arch_init.c | 1 +
> hw/i82378.c | 3 +-
> hw/mips_jazz.c | 3 +-
> hw/pc.c | 3 +-
> hw/pc.h | 4 ---
> hw/pcspk.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++---------
> hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 104 insertions(+), 17 deletions(-)
> create mode 100644 hw/pcspk.h
>
> diff --git a/arch_init.c b/arch_init.c
> index 2366511..a45485b 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -42,6 +42,7 @@
> #include "gdbstub.h"
> #include "hw/smbios.h"
> #include "exec-memory.h"
> +#include "hw/pcspk.h"
>
> #ifdef TARGET_SPARC
> int graphic_width = 1024;
> diff --git a/hw/i82378.c b/hw/i82378.c
> index 127cadf..79fa8b7 100644
> --- a/hw/i82378.c
> +++ b/hw/i82378.c
> @@ -20,6 +20,7 @@
> #include "pci.h"
> #include "pc.h"
> #include "i8254.h"
> +#include "pcspk.h"
>
> //#define DEBUG_I82378
>
> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev, I82378State *s)
> pit = pit_init(isabus, 0x40, 0, NULL);
>
> /* speaker */
> - pcspk_init(pit);
> + pcspk_init(isabus, pit);
>
> /* 2 82C37 (dma) */
> DMA_init(1,&s->out[1]);
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index b61b218..65608dc 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -37,6 +37,7 @@
> #include "loader.h"
> #include "mc146818rtc.h"
> #include "i8254.h"
> +#include "pcspk.h"
> #include "blockdev.h"
> #include "sysbus.h"
> #include "exec-memory.h"
> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion *address_space,
> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
> DMA_init(0, cpu_exit_irq);
> pit = pit_init(isa_bus, 0x40, 0, NULL);
> - pcspk_init(pit);
> + pcspk_init(isa_bus, pit);
>
> /* ISA IO space at 0x90000000 */
> isa_mmio_init(0x90000000, 0x01000000);
> diff --git a/hw/pc.c b/hw/pc.c
> index 6fb1de7..f6a91a9 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -37,6 +37,7 @@
> #include "multiboot.h"
> #include "mc146818rtc.h"
> #include "i8254.h"
> +#include "pcspk.h"
> #include "msi.h"
> #include "sysbus.h"
> #include "sysemu.h"
> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> /* connect PIT to output control line of the HPET */
> qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev, 0));
> }
> - pcspk_init(pit);
> + pcspk_init(isa_bus, pit);
>
> for(i = 0; i< MAX_SERIAL_PORTS; i++) {
> if (serial_hds[i]) {
> diff --git a/hw/pc.h b/hw/pc.h
> index b08708d..1b47bbd 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
> /* hpet.c */
> extern int no_hpet;
>
> -/* pcspk.c */
> -void pcspk_init(ISADevice *pit);
> -int pcspk_audio_init(ISABus *bus);
> -
> /* piix_pci.c */
> struct PCII440FXState;
> typedef struct PCII440FXState PCII440FXState;
> diff --git a/hw/pcspk.c b/hw/pcspk.c
> index 43df818..5bd9e32 100644
> --- a/hw/pcspk.c
> +++ b/hw/pcspk.c
> @@ -28,6 +28,7 @@
> #include "audio/audio.h"
> #include "qemu-timer.h"
> #include "i8254.h"
> +#include "pcspk.h"
>
> #define PCSPK_BUF_LEN 1792
> #define PCSPK_SAMPLE_RATE 32000
> @@ -35,10 +36,13 @@
> #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) / PCSPK_MAX_FREQ)
>
> typedef struct {
> + ISADevice dev;
> + MemoryRegion ioport;
> + uint32_t iobase;
> uint8_t sample_buf[PCSPK_BUF_LEN];
> QEMUSoundCard card;
> SWVoiceOut *voice;
> - ISADevice *pit;
> + void *pit;
> unsigned int pit_count;
> unsigned int samples;
> unsigned int play_pos;
> @@ -47,7 +51,7 @@ typedef struct {
> } PCSpkState;
>
> static const char *s_spk = "pcspk";
> -static PCSpkState pcspk_state;
> +static PCSpkState *pcspk_state;
>
> static inline void generate_samples(PCSpkState *s)
> {
> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free)
>
> int pcspk_audio_init(ISABus *bus)
> {
> - PCSpkState *s =&pcspk_state;
> + PCSpkState *s = pcspk_state;
> struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0};
This can be a follow up, but what this is really doing is enabling audio for
this device. ISABus is unused as a parameter.
I think it would be better to make this a qdev bool property (audio_enabled) and
then modify the code that calls this function to either set the property as a
global, or use the QOM path to specifically set it on this device.
Either way, I think setting an audio_enabled flag via qdev makes a whole lot
more sense conceptionally than using the -soundhw option.
>
> AUD_register_card(s_spk,&s->card);
> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus)
> return 0;
> }
>
> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr,
> + unsigned size)
> {
> PCSpkState *s = opaque;
> int out;
> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
> return pit_get_gate(s->pit, 2) | (s->data_on<< 1) | s->dummy_refresh_clock | out;
> }
>
> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr, uint64_t val,
> + unsigned size)
> {
> PCSpkState *s = opaque;
> const int gate = val& 1;
> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> }
> }
>
> -void pcspk_init(ISADevice *pit)
> +static const MemoryRegionOps pcspk_io_ops = {
> + .read = pcspk_io_read,
> + .write = pcspk_io_write,
> + .impl = {
> + .min_access_size = 1,
> + .max_access_size = 1,
> + },
> +};
> +
> +static int pcspk_initfn(ISADevice *dev)
> +{
> + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
> +
> + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1);
> + isa_register_ioport(NULL,&s->ioport, s->iobase);
Should pass dev as the first argument to isa_register_ioport. Otherwise the
resource won't be cleaned up during destruction.
> +
> + pcspk_state = s;
> +
> + return 0;
> +}
> +
> +static void pcspk_class_initfn(ObjectClass *klass, void *data)
> {
> - PCSpkState *s =&pcspk_state;
> + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> + ic->init = pcspk_initfn;
> +}
>
> - s->pit = pit;
> - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s);
> - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s);
> +static DeviceInfo pcspk_info = {
> + .name = "isa-pcspk",
> + .size = sizeof(PCSpkState),
> + .no_user = 1,
> + .props = (Property[]) {
> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1),
> + DEFINE_PROP_PTR("pit", PCSpkState, pit),
Please don't introduce a pointer property here. They cannot be used in a
meaningful way in qdev. Why not register a link<TYPE_PIT> in instance_init?
Regards,
Anthony Liguori
> + DEFINE_PROP_END_OF_LIST(),
> + },
> + .class_init = pcspk_class_initfn,
> +};
> +
> +static void pcspk_register(void)
> +{
> + isa_qdev_register(&pcspk_info);
> }
> +device_init(pcspk_register)
> diff --git a/hw/pcspk.h b/hw/pcspk.h
> new file mode 100644
> index 0000000..7f42bac
> --- /dev/null
> +++ b/hw/pcspk.h
> @@ -0,0 +1,45 @@
> +/*
> + * QEMU PC speaker emulation
> + *
> + * Copyright (c) 2006 Joachim Henke
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_PCSPK_H
> +#define HW_PCSPK_H
> +
> +#include "hw.h"
> +#include "isa.h"
> +
> +static inline ISADevice *pcspk_init(ISABus *bus, ISADevice *pit)
> +{
> + ISADevice *dev;
> +
> + dev = isa_create(bus, "isa-pcspk");
> + qdev_prop_set_uint32(&dev->qdev, "iobase", 0x61);
> + qdev_prop_set_ptr(&dev->qdev, "pit", pit);
> + qdev_init_nofail(&dev->qdev);
> +
> + return dev;
> +}
> +
> +int pcspk_audio_init(ISABus *bus);
> +
> +#endif /* !HW_PCSPK_H */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-01-31 20:49 ` Anthony Liguori
@ 2012-01-31 22:00 ` Jan Kiszka
2012-01-31 22:40 ` Anthony Liguori
2012-02-01 7:29 ` Paolo Bonzini
1 sibling, 1 reply; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 22:00 UTC (permalink / raw)
To: Anthony Liguori
Cc: Anthony Liguori, kvm, Marcelo Tosatti, qemu-devel, Blue Swirl,
Avi Kivity
[-- Attachment #1: Type: text/plain, Size: 8453 bytes --]
On 2012-01-31 21:49, Anthony Liguori wrote:
> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>> Convert the PC speaker device to a qdev ISA model. Move the public
>> interface to a dedicated header file at this chance.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>
> Heh, I did this too more or less the same way. Some comments below:
>
>> ---
>> arch_init.c | 1 +
>> hw/i82378.c | 3 +-
>> hw/mips_jazz.c | 3 +-
>> hw/pc.c | 3 +-
>> hw/pc.h | 4 ---
>> hw/pcspk.c | 62
>> ++++++++++++++++++++++++++++++++++++++++++++++---------
>> hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++
>> 7 files changed, 104 insertions(+), 17 deletions(-)
>> create mode 100644 hw/pcspk.h
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 2366511..a45485b 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -42,6 +42,7 @@
>> #include "gdbstub.h"
>> #include "hw/smbios.h"
>> #include "exec-memory.h"
>> +#include "hw/pcspk.h"
>>
>> #ifdef TARGET_SPARC
>> int graphic_width = 1024;
>> diff --git a/hw/i82378.c b/hw/i82378.c
>> index 127cadf..79fa8b7 100644
>> --- a/hw/i82378.c
>> +++ b/hw/i82378.c
>> @@ -20,6 +20,7 @@
>> #include "pci.h"
>> #include "pc.h"
>> #include "i8254.h"
>> +#include "pcspk.h"
>>
>> //#define DEBUG_I82378
>>
>> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev,
>> I82378State *s)
>> pit = pit_init(isabus, 0x40, 0, NULL);
>>
>> /* speaker */
>> - pcspk_init(pit);
>> + pcspk_init(isabus, pit);
>>
>> /* 2 82C37 (dma) */
>> DMA_init(1,&s->out[1]);
>> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
>> index b61b218..65608dc 100644
>> --- a/hw/mips_jazz.c
>> +++ b/hw/mips_jazz.c
>> @@ -37,6 +37,7 @@
>> #include "loader.h"
>> #include "mc146818rtc.h"
>> #include "i8254.h"
>> +#include "pcspk.h"
>> #include "blockdev.h"
>> #include "sysbus.h"
>> #include "exec-memory.h"
>> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion
>> *address_space,
>> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
>> DMA_init(0, cpu_exit_irq);
>> pit = pit_init(isa_bus, 0x40, 0, NULL);
>> - pcspk_init(pit);
>> + pcspk_init(isa_bus, pit);
>>
>> /* ISA IO space at 0x90000000 */
>> isa_mmio_init(0x90000000, 0x01000000);
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 6fb1de7..f6a91a9 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -37,6 +37,7 @@
>> #include "multiboot.h"
>> #include "mc146818rtc.h"
>> #include "i8254.h"
>> +#include "pcspk.h"
>> #include "msi.h"
>> #include "sysbus.h"
>> #include "sysemu.h"
>> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus,
>> qemu_irq *gsi,
>> /* connect PIT to output control line of the HPET */
>> qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev,
>> 0));
>> }
>> - pcspk_init(pit);
>> + pcspk_init(isa_bus, pit);
>>
>> for(i = 0; i< MAX_SERIAL_PORTS; i++) {
>> if (serial_hds[i]) {
>> diff --git a/hw/pc.h b/hw/pc.h
>> index b08708d..1b47bbd 100644
>> --- a/hw/pc.h
>> +++ b/hw/pc.h
>> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice
>> *dev, uint8_t addr);
>> /* hpet.c */
>> extern int no_hpet;
>>
>> -/* pcspk.c */
>> -void pcspk_init(ISADevice *pit);
>> -int pcspk_audio_init(ISABus *bus);
>> -
>> /* piix_pci.c */
>> struct PCII440FXState;
>> typedef struct PCII440FXState PCII440FXState;
>> diff --git a/hw/pcspk.c b/hw/pcspk.c
>> index 43df818..5bd9e32 100644
>> --- a/hw/pcspk.c
>> +++ b/hw/pcspk.c
>> @@ -28,6 +28,7 @@
>> #include "audio/audio.h"
>> #include "qemu-timer.h"
>> #include "i8254.h"
>> +#include "pcspk.h"
>>
>> #define PCSPK_BUF_LEN 1792
>> #define PCSPK_SAMPLE_RATE 32000
>> @@ -35,10 +36,13 @@
>> #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) /
>> PCSPK_MAX_FREQ)
>>
>> typedef struct {
>> + ISADevice dev;
>> + MemoryRegion ioport;
>> + uint32_t iobase;
>> uint8_t sample_buf[PCSPK_BUF_LEN];
>> QEMUSoundCard card;
>> SWVoiceOut *voice;
>> - ISADevice *pit;
>> + void *pit;
>> unsigned int pit_count;
>> unsigned int samples;
>> unsigned int play_pos;
>> @@ -47,7 +51,7 @@ typedef struct {
>> } PCSpkState;
>>
>> static const char *s_spk = "pcspk";
>> -static PCSpkState pcspk_state;
>> +static PCSpkState *pcspk_state;
>>
>> static inline void generate_samples(PCSpkState *s)
>> {
>> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free)
>>
>> int pcspk_audio_init(ISABus *bus)
>> {
>> - PCSpkState *s =&pcspk_state;
>> + PCSpkState *s = pcspk_state;
>> struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0};
>
>
> This can be a follow up, but what this is really doing is enabling audio
> for this device. ISABus is unused as a parameter.
Yes, but this is a callback for the audio subsystem, look at
arch_init.c. Nothing we can do here, only if we change that callback
prototype.
>
> I think it would be better to make this a qdev bool property
> (audio_enabled) and then modify the code that calls this function to
> either set the property as a global, or use the QOM path to specifically
> set it on this device.
>
> Either way, I think setting an audio_enabled flag via qdev makes a whole
> lot more sense conceptionally than using the -soundhw option.
Yep, there is room for improvements. The audio system is just another
backend, like a chardev or netdev. It should once we handled like this I
guess.
>
>>
>> AUD_register_card(s_spk,&s->card);
>> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus)
>> return 0;
>> }
>>
>> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
>> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr,
>> + unsigned size)
>> {
>> PCSpkState *s = opaque;
>> int out;
>> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque,
>> uint32_t addr)
>> return pit_get_gate(s->pit, 2) | (s->data_on<< 1) |
>> s->dummy_refresh_clock | out;
>> }
>>
>> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t
>> val)
>> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr,
>> uint64_t val,
>> + unsigned size)
>> {
>> PCSpkState *s = opaque;
>> const int gate = val& 1;
>> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque,
>> uint32_t addr, uint32_t val)
>> }
>> }
>>
>> -void pcspk_init(ISADevice *pit)
>> +static const MemoryRegionOps pcspk_io_ops = {
>> + .read = pcspk_io_read,
>> + .write = pcspk_io_write,
>> + .impl = {
>> + .min_access_size = 1,
>> + .max_access_size = 1,
>> + },
>> +};
>> +
>> +static int pcspk_initfn(ISADevice *dev)
>> +{
>> + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
>> +
>> + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1);
>> + isa_register_ioport(NULL,&s->ioport, s->iobase);
>
> Should pass dev as the first argument to isa_register_ioport. Otherwise
> the resource won't be cleaned up during destruction.
Oops, will fix.
>
>> +
>> + pcspk_state = s;
>> +
>> + return 0;
>> +}
>> +
>> +static void pcspk_class_initfn(ObjectClass *klass, void *data)
>> {
>> - PCSpkState *s =&pcspk_state;
>> + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
>> + ic->init = pcspk_initfn;
>> +}
>>
>> - s->pit = pit;
>> - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s);
>> - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s);
>> +static DeviceInfo pcspk_info = {
>> + .name = "isa-pcspk",
>> + .size = sizeof(PCSpkState),
>> + .no_user = 1,
>> + .props = (Property[]) {
>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1),
>> + DEFINE_PROP_PTR("pit", PCSpkState, pit),
>
> Please don't introduce a pointer property here. They cannot be used in
> a meaningful way in qdev. Why not register a link<TYPE_PIT> in
> instance_init?
Once it's properly usable, I will do so. So far I see now in-tree -
ideally type-checking - replacement for qdev_prop_set_ptr.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-01-31 22:00 ` Jan Kiszka
@ 2012-01-31 22:40 ` Anthony Liguori
2012-01-31 22:48 ` Jan Kiszka
0 siblings, 1 reply; 19+ messages in thread
From: Anthony Liguori @ 2012-01-31 22:40 UTC (permalink / raw)
To: Jan Kiszka; +Cc: kvm, Marcelo Tosatti, qemu-devel, Blue Swirl, Avi Kivity
On 01/31/2012 04:00 PM, Jan Kiszka wrote:
> On 2012-01-31 21:49, Anthony Liguori wrote:
>> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>>> Convert the PC speaker device to a qdev ISA model. Move the public
>>> interface to a dedicated header file at this chance.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> Heh, I did this too more or less the same way. Some comments below:
>>
>>> ---
>>> arch_init.c | 1 +
>>> hw/i82378.c | 3 +-
>>> hw/mips_jazz.c | 3 +-
>>> hw/pc.c | 3 +-
>>> hw/pc.h | 4 ---
>>> hw/pcspk.c | 62
>>> ++++++++++++++++++++++++++++++++++++++++++++++---------
>>> hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++
>>> 7 files changed, 104 insertions(+), 17 deletions(-)
>>> create mode 100644 hw/pcspk.h
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 2366511..a45485b 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -42,6 +42,7 @@
>>> #include "gdbstub.h"
>>> #include "hw/smbios.h"
>>> #include "exec-memory.h"
>>> +#include "hw/pcspk.h"
>>>
>>> #ifdef TARGET_SPARC
>>> int graphic_width = 1024;
>>> diff --git a/hw/i82378.c b/hw/i82378.c
>>> index 127cadf..79fa8b7 100644
>>> --- a/hw/i82378.c
>>> +++ b/hw/i82378.c
>>> @@ -20,6 +20,7 @@
>>> #include "pci.h"
>>> #include "pc.h"
>>> #include "i8254.h"
>>> +#include "pcspk.h"
>>>
>>> //#define DEBUG_I82378
>>>
>>> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev,
>>> I82378State *s)
>>> pit = pit_init(isabus, 0x40, 0, NULL);
>>>
>>> /* speaker */
>>> - pcspk_init(pit);
>>> + pcspk_init(isabus, pit);
>>>
>>> /* 2 82C37 (dma) */
>>> DMA_init(1,&s->out[1]);
>>> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
>>> index b61b218..65608dc 100644
>>> --- a/hw/mips_jazz.c
>>> +++ b/hw/mips_jazz.c
>>> @@ -37,6 +37,7 @@
>>> #include "loader.h"
>>> #include "mc146818rtc.h"
>>> #include "i8254.h"
>>> +#include "pcspk.h"
>>> #include "blockdev.h"
>>> #include "sysbus.h"
>>> #include "exec-memory.h"
>>> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion
>>> *address_space,
>>> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
>>> DMA_init(0, cpu_exit_irq);
>>> pit = pit_init(isa_bus, 0x40, 0, NULL);
>>> - pcspk_init(pit);
>>> + pcspk_init(isa_bus, pit);
>>>
>>> /* ISA IO space at 0x90000000 */
>>> isa_mmio_init(0x90000000, 0x01000000);
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index 6fb1de7..f6a91a9 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -37,6 +37,7 @@
>>> #include "multiboot.h"
>>> #include "mc146818rtc.h"
>>> #include "i8254.h"
>>> +#include "pcspk.h"
>>> #include "msi.h"
>>> #include "sysbus.h"
>>> #include "sysemu.h"
>>> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus,
>>> qemu_irq *gsi,
>>> /* connect PIT to output control line of the HPET */
>>> qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev,
>>> 0));
>>> }
>>> - pcspk_init(pit);
>>> + pcspk_init(isa_bus, pit);
>>>
>>> for(i = 0; i< MAX_SERIAL_PORTS; i++) {
>>> if (serial_hds[i]) {
>>> diff --git a/hw/pc.h b/hw/pc.h
>>> index b08708d..1b47bbd 100644
>>> --- a/hw/pc.h
>>> +++ b/hw/pc.h
>>> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice
>>> *dev, uint8_t addr);
>>> /* hpet.c */
>>> extern int no_hpet;
>>>
>>> -/* pcspk.c */
>>> -void pcspk_init(ISADevice *pit);
>>> -int pcspk_audio_init(ISABus *bus);
>>> -
>>> /* piix_pci.c */
>>> struct PCII440FXState;
>>> typedef struct PCII440FXState PCII440FXState;
>>> diff --git a/hw/pcspk.c b/hw/pcspk.c
>>> index 43df818..5bd9e32 100644
>>> --- a/hw/pcspk.c
>>> +++ b/hw/pcspk.c
>>> @@ -28,6 +28,7 @@
>>> #include "audio/audio.h"
>>> #include "qemu-timer.h"
>>> #include "i8254.h"
>>> +#include "pcspk.h"
>>>
>>> #define PCSPK_BUF_LEN 1792
>>> #define PCSPK_SAMPLE_RATE 32000
>>> @@ -35,10 +36,13 @@
>>> #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) /
>>> PCSPK_MAX_FREQ)
>>>
>>> typedef struct {
>>> + ISADevice dev;
>>> + MemoryRegion ioport;
>>> + uint32_t iobase;
>>> uint8_t sample_buf[PCSPK_BUF_LEN];
>>> QEMUSoundCard card;
>>> SWVoiceOut *voice;
>>> - ISADevice *pit;
>>> + void *pit;
>>> unsigned int pit_count;
>>> unsigned int samples;
>>> unsigned int play_pos;
>>> @@ -47,7 +51,7 @@ typedef struct {
>>> } PCSpkState;
>>>
>>> static const char *s_spk = "pcspk";
>>> -static PCSpkState pcspk_state;
>>> +static PCSpkState *pcspk_state;
>>>
>>> static inline void generate_samples(PCSpkState *s)
>>> {
>>> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free)
>>>
>>> int pcspk_audio_init(ISABus *bus)
>>> {
>>> - PCSpkState *s =&pcspk_state;
>>> + PCSpkState *s = pcspk_state;
>>> struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0};
>>
>>
>> This can be a follow up, but what this is really doing is enabling audio
>> for this device. ISABus is unused as a parameter.
>
> Yes, but this is a callback for the audio subsystem, look at
> arch_init.c. Nothing we can do here, only if we change that callback
> prototype.
>
>>
>> I think it would be better to make this a qdev bool property
>> (audio_enabled) and then modify the code that calls this function to
>> either set the property as a global, or use the QOM path to specifically
>> set it on this device.
>>
>> Either way, I think setting an audio_enabled flag via qdev makes a whole
>> lot more sense conceptionally than using the -soundhw option.
>
> Yep, there is room for improvements. The audio system is just another
> backend, like a chardev or netdev. It should once we handled like this I
> guess.
>
>>
>>>
>>> AUD_register_card(s_spk,&s->card);
>>> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus)
>>> return 0;
>>> }
>>>
>>> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
>>> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr,
>>> + unsigned size)
>>> {
>>> PCSpkState *s = opaque;
>>> int out;
>>> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque,
>>> uint32_t addr)
>>> return pit_get_gate(s->pit, 2) | (s->data_on<< 1) |
>>> s->dummy_refresh_clock | out;
>>> }
>>>
>>> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t
>>> val)
>>> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr,
>>> uint64_t val,
>>> + unsigned size)
>>> {
>>> PCSpkState *s = opaque;
>>> const int gate = val& 1;
>>> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque,
>>> uint32_t addr, uint32_t val)
>>> }
>>> }
>>>
>>> -void pcspk_init(ISADevice *pit)
>>> +static const MemoryRegionOps pcspk_io_ops = {
>>> + .read = pcspk_io_read,
>>> + .write = pcspk_io_write,
>>> + .impl = {
>>> + .min_access_size = 1,
>>> + .max_access_size = 1,
>>> + },
>>> +};
>>> +
>>> +static int pcspk_initfn(ISADevice *dev)
>>> +{
>>> + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
>>> +
>>> + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1);
>>> + isa_register_ioport(NULL,&s->ioport, s->iobase);
>>
>> Should pass dev as the first argument to isa_register_ioport. Otherwise
>> the resource won't be cleaned up during destruction.
>
> Oops, will fix.
>
>>
>>> +
>>> + pcspk_state = s;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void pcspk_class_initfn(ObjectClass *klass, void *data)
>>> {
>>> - PCSpkState *s =&pcspk_state;
>>> + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
>>> + ic->init = pcspk_initfn;
>>> +}
>>>
>>> - s->pit = pit;
>>> - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s);
>>> - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s);
>>> +static DeviceInfo pcspk_info = {
>>> + .name = "isa-pcspk",
>>> + .size = sizeof(PCSpkState),
>>> + .no_user = 1,
>>> + .props = (Property[]) {
>>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1),
>>> + DEFINE_PROP_PTR("pit", PCSpkState, pit),
>>
>> Please don't introduce a pointer property here. They cannot be used in
>> a meaningful way in qdev. Why not register a link<TYPE_PIT> in
>> instance_init?
>
> Once it's properly usable, I will do so. So far I see now in-tree -
> ideally type-checking - replacement for qdev_prop_set_ptr.
Why is what's in the tree not usable?
Just don't do pcspk_init as a static inline (which is not that nice to do
anyway) and you don't need to worry about the availability of an accessor.
And BTW, there is strict type checking now, which makes it already an
improvement over property pointers.
Regards,
Anthony Liguori
>
> Jan
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-01-31 22:40 ` Anthony Liguori
@ 2012-01-31 22:48 ` Jan Kiszka
2012-01-31 23:23 ` Anthony Liguori
0 siblings, 1 reply; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 22:48 UTC (permalink / raw)
To: Anthony Liguori; +Cc: kvm, Marcelo Tosatti, qemu-devel, Blue Swirl, Avi Kivity
[-- Attachment #1: Type: text/plain, Size: 9979 bytes --]
On 2012-01-31 23:40, Anthony Liguori wrote:
> On 01/31/2012 04:00 PM, Jan Kiszka wrote:
>> On 2012-01-31 21:49, Anthony Liguori wrote:
>>> On 01/31/2012 11:41 AM, Jan Kiszka wrote:
>>>> Convert the PC speaker device to a qdev ISA model. Move the public
>>>> interface to a dedicated header file at this chance.
>>>>
>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>
>>> Heh, I did this too more or less the same way. Some comments below:
>>>
>>>> ---
>>>> arch_init.c | 1 +
>>>> hw/i82378.c | 3 +-
>>>> hw/mips_jazz.c | 3 +-
>>>> hw/pc.c | 3 +-
>>>> hw/pc.h | 4 ---
>>>> hw/pcspk.c | 62
>>>> ++++++++++++++++++++++++++++++++++++++++++++++---------
>>>> hw/pcspk.h | 45 ++++++++++++++++++++++++++++++++++++++++
>>>> 7 files changed, 104 insertions(+), 17 deletions(-)
>>>> create mode 100644 hw/pcspk.h
>>>>
>>>> diff --git a/arch_init.c b/arch_init.c
>>>> index 2366511..a45485b 100644
>>>> --- a/arch_init.c
>>>> +++ b/arch_init.c
>>>> @@ -42,6 +42,7 @@
>>>> #include "gdbstub.h"
>>>> #include "hw/smbios.h"
>>>> #include "exec-memory.h"
>>>> +#include "hw/pcspk.h"
>>>>
>>>> #ifdef TARGET_SPARC
>>>> int graphic_width = 1024;
>>>> diff --git a/hw/i82378.c b/hw/i82378.c
>>>> index 127cadf..79fa8b7 100644
>>>> --- a/hw/i82378.c
>>>> +++ b/hw/i82378.c
>>>> @@ -20,6 +20,7 @@
>>>> #include "pci.h"
>>>> #include "pc.h"
>>>> #include "i8254.h"
>>>> +#include "pcspk.h"
>>>>
>>>> //#define DEBUG_I82378
>>>>
>>>> @@ -195,7 +196,7 @@ static void i82378_init(DeviceState *dev,
>>>> I82378State *s)
>>>> pit = pit_init(isabus, 0x40, 0, NULL);
>>>>
>>>> /* speaker */
>>>> - pcspk_init(pit);
>>>> + pcspk_init(isabus, pit);
>>>>
>>>> /* 2 82C37 (dma) */
>>>> DMA_init(1,&s->out[1]);
>>>> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
>>>> index b61b218..65608dc 100644
>>>> --- a/hw/mips_jazz.c
>>>> +++ b/hw/mips_jazz.c
>>>> @@ -37,6 +37,7 @@
>>>> #include "loader.h"
>>>> #include "mc146818rtc.h"
>>>> #include "i8254.h"
>>>> +#include "pcspk.h"
>>>> #include "blockdev.h"
>>>> #include "sysbus.h"
>>>> #include "exec-memory.h"
>>>> @@ -193,7 +194,7 @@ static void mips_jazz_init(MemoryRegion
>>>> *address_space,
>>>> cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
>>>> DMA_init(0, cpu_exit_irq);
>>>> pit = pit_init(isa_bus, 0x40, 0, NULL);
>>>> - pcspk_init(pit);
>>>> + pcspk_init(isa_bus, pit);
>>>>
>>>> /* ISA IO space at 0x90000000 */
>>>> isa_mmio_init(0x90000000, 0x01000000);
>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>> index 6fb1de7..f6a91a9 100644
>>>> --- a/hw/pc.c
>>>> +++ b/hw/pc.c
>>>> @@ -37,6 +37,7 @@
>>>> #include "multiboot.h"
>>>> #include "mc146818rtc.h"
>>>> #include "i8254.h"
>>>> +#include "pcspk.h"
>>>> #include "msi.h"
>>>> #include "sysbus.h"
>>>> #include "sysemu.h"
>>>> @@ -1169,7 +1170,7 @@ void pc_basic_device_init(ISABus *isa_bus,
>>>> qemu_irq *gsi,
>>>> /* connect PIT to output control line of the HPET */
>>>> qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(&pit->qdev,
>>>> 0));
>>>> }
>>>> - pcspk_init(pit);
>>>> + pcspk_init(isa_bus, pit);
>>>>
>>>> for(i = 0; i< MAX_SERIAL_PORTS; i++) {
>>>> if (serial_hds[i]) {
>>>> diff --git a/hw/pc.h b/hw/pc.h
>>>> index b08708d..1b47bbd 100644
>>>> --- a/hw/pc.h
>>>> +++ b/hw/pc.h
>>>> @@ -149,10 +149,6 @@ void piix4_smbus_register_device(SMBusDevice
>>>> *dev, uint8_t addr);
>>>> /* hpet.c */
>>>> extern int no_hpet;
>>>>
>>>> -/* pcspk.c */
>>>> -void pcspk_init(ISADevice *pit);
>>>> -int pcspk_audio_init(ISABus *bus);
>>>> -
>>>> /* piix_pci.c */
>>>> struct PCII440FXState;
>>>> typedef struct PCII440FXState PCII440FXState;
>>>> diff --git a/hw/pcspk.c b/hw/pcspk.c
>>>> index 43df818..5bd9e32 100644
>>>> --- a/hw/pcspk.c
>>>> +++ b/hw/pcspk.c
>>>> @@ -28,6 +28,7 @@
>>>> #include "audio/audio.h"
>>>> #include "qemu-timer.h"
>>>> #include "i8254.h"
>>>> +#include "pcspk.h"
>>>>
>>>> #define PCSPK_BUF_LEN 1792
>>>> #define PCSPK_SAMPLE_RATE 32000
>>>> @@ -35,10 +36,13 @@
>>>> #define PCSPK_MIN_COUNT ((PIT_FREQ + PCSPK_MAX_FREQ - 1) /
>>>> PCSPK_MAX_FREQ)
>>>>
>>>> typedef struct {
>>>> + ISADevice dev;
>>>> + MemoryRegion ioport;
>>>> + uint32_t iobase;
>>>> uint8_t sample_buf[PCSPK_BUF_LEN];
>>>> QEMUSoundCard card;
>>>> SWVoiceOut *voice;
>>>> - ISADevice *pit;
>>>> + void *pit;
>>>> unsigned int pit_count;
>>>> unsigned int samples;
>>>> unsigned int play_pos;
>>>> @@ -47,7 +51,7 @@ typedef struct {
>>>> } PCSpkState;
>>>>
>>>> static const char *s_spk = "pcspk";
>>>> -static PCSpkState pcspk_state;
>>>> +static PCSpkState *pcspk_state;
>>>>
>>>> static inline void generate_samples(PCSpkState *s)
>>>> {
>>>> @@ -99,7 +103,7 @@ static void pcspk_callback(void *opaque, int free)
>>>>
>>>> int pcspk_audio_init(ISABus *bus)
>>>> {
>>>> - PCSpkState *s =&pcspk_state;
>>>> + PCSpkState *s = pcspk_state;
>>>> struct audsettings as = {PCSPK_SAMPLE_RATE, 1, AUD_FMT_U8, 0};
>>>
>>>
>>> This can be a follow up, but what this is really doing is enabling audio
>>> for this device. ISABus is unused as a parameter.
>>
>> Yes, but this is a callback for the audio subsystem, look at
>> arch_init.c. Nothing we can do here, only if we change that callback
>> prototype.
>>
>>>
>>> I think it would be better to make this a qdev bool property
>>> (audio_enabled) and then modify the code that calls this function to
>>> either set the property as a global, or use the QOM path to specifically
>>> set it on this device.
>>>
>>> Either way, I think setting an audio_enabled flag via qdev makes a whole
>>> lot more sense conceptionally than using the -soundhw option.
>>
>> Yep, there is room for improvements. The audio system is just another
>> backend, like a chardev or netdev. It should once we handled like this I
>> guess.
>>
>>>
>>>>
>>>> AUD_register_card(s_spk,&s->card);
>>>> @@ -113,7 +117,8 @@ int pcspk_audio_init(ISABus *bus)
>>>> return 0;
>>>> }
>>>>
>>>> -static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
>>>> +static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr,
>>>> + unsigned size)
>>>> {
>>>> PCSpkState *s = opaque;
>>>> int out;
>>>> @@ -124,7 +129,8 @@ static uint32_t pcspk_ioport_read(void *opaque,
>>>> uint32_t addr)
>>>> return pit_get_gate(s->pit, 2) | (s->data_on<< 1) |
>>>> s->dummy_refresh_clock | out;
>>>> }
>>>>
>>>> -static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t
>>>> val)
>>>> +static void pcspk_io_write(void *opaque, target_phys_addr_t addr,
>>>> uint64_t val,
>>>> + unsigned size)
>>>> {
>>>> PCSpkState *s = opaque;
>>>> const int gate = val& 1;
>>>> @@ -138,11 +144,47 @@ static void pcspk_ioport_write(void *opaque,
>>>> uint32_t addr, uint32_t val)
>>>> }
>>>> }
>>>>
>>>> -void pcspk_init(ISADevice *pit)
>>>> +static const MemoryRegionOps pcspk_io_ops = {
>>>> + .read = pcspk_io_read,
>>>> + .write = pcspk_io_write,
>>>> + .impl = {
>>>> + .min_access_size = 1,
>>>> + .max_access_size = 1,
>>>> + },
>>>> +};
>>>> +
>>>> +static int pcspk_initfn(ISADevice *dev)
>>>> +{
>>>> + PCSpkState *s = DO_UPCAST(PCSpkState, dev, dev);
>>>> +
>>>> + memory_region_init_io(&s->ioport,&pcspk_io_ops, s, "elcr", 1);
>>>> + isa_register_ioport(NULL,&s->ioport, s->iobase);
>>>
>>> Should pass dev as the first argument to isa_register_ioport. Otherwise
>>> the resource won't be cleaned up during destruction.
>>
>> Oops, will fix.
>>
>>>
>>>> +
>>>> + pcspk_state = s;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void pcspk_class_initfn(ObjectClass *klass, void *data)
>>>> {
>>>> - PCSpkState *s =&pcspk_state;
>>>> + ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
>>>> + ic->init = pcspk_initfn;
>>>> +}
>>>>
>>>> - s->pit = pit;
>>>> - register_ioport_read(0x61, 1, 1, pcspk_ioport_read, s);
>>>> - register_ioport_write(0x61, 1, 1, pcspk_ioport_write, s);
>>>> +static DeviceInfo pcspk_info = {
>>>> + .name = "isa-pcspk",
>>>> + .size = sizeof(PCSpkState),
>>>> + .no_user = 1,
>>>> + .props = (Property[]) {
>>>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1),
>>>> + DEFINE_PROP_PTR("pit", PCSpkState, pit),
>>>
>>> Please don't introduce a pointer property here. They cannot be used in
>>> a meaningful way in qdev. Why not register a link<TYPE_PIT> in
>>> instance_init?
>>
>> Once it's properly usable, I will do so. So far I see now in-tree -
>> ideally type-checking - replacement for qdev_prop_set_ptr.
>
> Why is what's in the tree not usable?
>
> Just don't do pcspk_init as a static inline (which is not that nice to
> do anyway) and you don't need to worry about the availability of an
> accessor.
The current pattern requested by some reviewers used to be the one I
applied here. I dislike it as well when the device can't be seriously
configured out. But I can switch over, no problem.
>
> And BTW, there is strict type checking now, which makes it already an
> improvement over property pointers.
OK, for my slow understanding: I use qdev_property_add_link in the
device init function to create the property, letting it point to a state
field. But what should I call from the outside to actually set its
value? And what C type does this value have? A DeviceState * or a char *
(path)?
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-01-31 22:48 ` Jan Kiszka
@ 2012-01-31 23:23 ` Anthony Liguori
0 siblings, 0 replies; 19+ messages in thread
From: Anthony Liguori @ 2012-01-31 23:23 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Blue Swirl, Marcelo Tosatti, qemu-devel, kvm, Avi Kivity
On 01/31/2012 04:48 PM, Jan Kiszka wrote:
> On 2012-01-31 23:40, Anthony Liguori wrote:
>> Why is what's in the tree not usable?
>>
>> Just don't do pcspk_init as a static inline (which is not that nice to
>> do anyway) and you don't need to worry about the availability of an
>> accessor.
>
> The current pattern requested by some reviewers used to be the one I
> applied here. I dislike it as well when the device can't be seriously
> configured out. But I can switch over, no problem.
>
>>
>> And BTW, there is strict type checking now, which makes it already an
>> improvement over property pointers.
>
> OK, for my slow understanding: I use qdev_property_add_link in the
> device init function to create the property, letting it point to a state
> field. But what should I call from the outside to actually set its
> value?
You have a few options:
1) you can add a generic set_child function to qdev... but don't do this, I'll
fix it right in my future series
2) You can take advantage of the fact that the only thing that ever sets this is
pcspk_init, move pcspk_init to pcspk.c, and then since you have access to the
state structure, directly assign it there.
3) Introduce a pcspk_set_pit() function that just does the same thing as (2).
4) Introduce (1) and then have a pcspk_set_pit() that just calls (1). This is
what I think we should do moving forward and I plan to do this in my future
refactorings.
That all said, I think this isn't going to work for you until my next series is
merged. Right now, the link properties have strict type checking. You really
need a link<PITCommon> in order to be able to accept either a KVMPIT or a normal
PIT. My series on the list relaxes the type checking to use implements instead
of a strict equality check.
So maybe the best thing to do is drop the ptr type entirely, make pcspk_init()
be in pcspk.c and touch the private state, and then I'll refactor it later to
use a link<> property.
> And what C type does this value have?
PITState *.
Regards,
Anthony Liguroi
A DeviceState * or a char *
> (path)?
>
> Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-01-31 20:49 ` Anthony Liguori
2012-01-31 22:00 ` Jan Kiszka
@ 2012-02-01 7:29 ` Paolo Bonzini
2012-02-01 9:18 ` Jan Kiszka
1 sibling, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2012-02-01 7:29 UTC (permalink / raw)
To: Anthony Liguori
Cc: Anthony Liguori, kvm, Jan Kiszka, Marcelo Tosatti, qemu-devel,
Blue Swirl, Avi Kivity
On 01/31/2012 09:49 PM, Anthony Liguori wrote:
>>
>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1),
>> + DEFINE_PROP_PTR("pit", PCSpkState, pit),
>
> Please don't introduce a pointer property here. They cannot be used in
> a meaningful way in qdev. Why not register a link<TYPE_PIT> in
> instance_init?
I'm going to clean this up, you can leave the PTR for now.
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev
2012-02-01 7:29 ` Paolo Bonzini
@ 2012-02-01 9:18 ` Jan Kiszka
0 siblings, 0 replies; 19+ messages in thread
From: Jan Kiszka @ 2012-02-01 9:18 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Anthony Liguori, kvm, Marcelo Tosatti, qemu-devel, Blue Swirl,
Avi Kivity
On 2012-02-01 08:29, Paolo Bonzini wrote:
> On 01/31/2012 09:49 PM, Anthony Liguori wrote:
>>>
>>> + DEFINE_PROP_HEX32("iobase", PCSpkState, iobase, -1),
>>> + DEFINE_PROP_PTR("pit", PCSpkState, pit),
>>
>> Please don't introduce a pointer property here. They cannot be used in
>> a meaningful way in qdev. Why not register a link<TYPE_PIT> in
>> instance_init?
>
> I'm going to clean this up, you can leave the PTR for now.
OK, will then ship v4 with this property still in place and count on you.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 7/7] i8254: Factor out pit_get_channel_info
2012-01-31 17:41 [Qemu-devel] [PATCH v3 0/7] pit, hpet, pcspk: fixes & preparation for KVM Jan Kiszka
` (5 preceding siblings ...)
2012-01-31 17:41 ` [Qemu-devel] [PATCH v3 6/7] pcspk: Convert to qdev Jan Kiszka
@ 2012-01-31 17:41 ` Jan Kiszka
6 siblings, 0 replies; 19+ messages in thread
From: Jan Kiszka @ 2012-01-31 17:41 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel; +Cc: Blue Swirl, Marcelo Tosatti, Avi Kivity, kvm
Instead of providing 4 individual query functions for mode, gate, output
and initial counter state, introduce a service that queries all
information at once. This comes with tiny additional costs for
pcspk_callback but with a much cleaner interface. Also, it will simplify
the implementation of the KVM in-kernel PIT model.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
hw/i8254.c | 35 ++++++++++-------------------------
hw/i8254.h | 12 ++++++++----
hw/pcspk.c | 16 +++++++++++-----
3 files changed, 29 insertions(+), 34 deletions(-)
diff --git a/hw/i8254.c b/hw/i8254.c
index 6412277..ff253bb 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -90,7 +90,7 @@ static int pit_get_count(PITChannelState *s)
}
/* get pit output bit */
-static int pit_get_out1(PITChannelState *s, int64_t current_time)
+static int pit_get_out(PITChannelState *s, int64_t current_time)
{
uint64_t d;
int out;
@@ -122,13 +122,6 @@ static int pit_get_out1(PITChannelState *s, int64_t current_time)
return out;
}
-int pit_get_out(ISADevice *dev, int channel, int64_t current_time)
-{
- PITState *pit = DO_UPCAST(PITState, dev, dev);
- PITChannelState *s = &pit->channels[channel];
- return pit_get_out1(s, current_time);
-}
-
/* return -1 if no transition will occur. */
static int64_t pit_get_next_transition_time(PITChannelState *s,
int64_t current_time)
@@ -215,25 +208,15 @@ void pit_set_gate(ISADevice *dev, int channel, int val)
s->gate = val;
}
-int pit_get_gate(ISADevice *dev, int channel)
-{
- PITState *pit = DO_UPCAST(PITState, dev, dev);
- PITChannelState *s = &pit->channels[channel];
- return s->gate;
-}
-
-int pit_get_initial_count(ISADevice *dev, int channel)
+void pit_get_channel_info(ISADevice *dev, int channel, PITChannelInfo *info)
{
PITState *pit = DO_UPCAST(PITState, dev, dev);
PITChannelState *s = &pit->channels[channel];
- return s->count;
-}
-int pit_get_mode(ISADevice *dev, int channel)
-{
- PITState *pit = DO_UPCAST(PITState, dev, dev);
- PITChannelState *s = &pit->channels[channel];
- return s->mode;
+ info->gate = s->gate;
+ info->mode = s->mode;
+ info->initial_count = s->count;
+ info->out = pit_get_out(s, qemu_get_clock_ns(vm_clock));
}
static inline void pit_load_count(PITChannelState *s, int val)
@@ -274,7 +257,9 @@ static void pit_ioport_write(void *opaque, uint32_t addr, uint32_t val)
if (!(val & 0x10) && !s->status_latched) {
/* status latch */
/* XXX: add BCD and null count */
- s->status = (pit_get_out1(s, qemu_get_clock_ns(vm_clock)) << 7) |
+ s->status =
+ (pit_get_out(s,
+ qemu_get_clock_ns(vm_clock)) << 7) |
(s->rw_mode << 4) |
(s->mode << 1) |
s->bcd;
@@ -381,7 +366,7 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
return;
}
expire_time = pit_get_next_transition_time(s, current_time);
- irq_level = pit_get_out1(s, current_time);
+ irq_level = pit_get_out(s, current_time);
qemu_set_irq(s->irq, irq_level);
#ifdef DEBUG_PIT
printf("irq_level=%d next_delay=%f\n",
diff --git a/hw/i8254.h b/hw/i8254.h
index 8ad8e07..a1d2e98 100644
--- a/hw/i8254.h
+++ b/hw/i8254.h
@@ -30,6 +30,13 @@
#define PIT_FREQ 1193182
+typedef struct PITChannelInfo {
+ int gate;
+ int mode;
+ int initial_count;
+ int out;
+} PITChannelInfo;
+
static inline ISADevice *pit_init(ISABus *bus, int base, int isa_irq,
qemu_irq alt_irq)
{
@@ -45,9 +52,6 @@ static inline ISADevice *pit_init(ISABus *bus, int base, int isa_irq,
}
void pit_set_gate(ISADevice *dev, int channel, int val);
-int pit_get_gate(ISADevice *dev, int channel);
-int pit_get_initial_count(ISADevice *dev, int channel);
-int pit_get_mode(ISADevice *dev, int channel);
-int pit_get_out(ISADevice *dev, int channel, int64_t current_time);
+void pit_get_channel_info(ISADevice *dev, int channel, PITChannelInfo *info);
#endif /* !HW_I8254_H */
diff --git a/hw/pcspk.c b/hw/pcspk.c
index 5bd9e32..834fb0c 100644
--- a/hw/pcspk.c
+++ b/hw/pcspk.c
@@ -75,12 +75,16 @@ static inline void generate_samples(PCSpkState *s)
static void pcspk_callback(void *opaque, int free)
{
PCSpkState *s = opaque;
+ PITChannelInfo ch;
unsigned int n;
- if (pit_get_mode(s->pit, 2) != 3)
+ pit_get_channel_info(s->pit, 2, &ch);
+
+ if (ch.mode != 3) {
return;
+ }
- n = pit_get_initial_count(s->pit, 2);
+ n = ch.initial_count;
/* avoid frequencies that are not reproducible with sample rate */
if (n < PCSPK_MIN_COUNT)
n = 0;
@@ -121,12 +125,14 @@ static uint64_t pcspk_io_read(void *opaque, target_phys_addr_t addr,
unsigned size)
{
PCSpkState *s = opaque;
- int out;
+ PITChannelInfo ch;
+
+ pit_get_channel_info(s->pit, 2, &ch);
s->dummy_refresh_clock ^= (1 << 4);
- out = pit_get_out(s->pit, 2, qemu_get_clock_ns(vm_clock)) << 5;
- return pit_get_gate(s->pit, 2) | (s->data_on << 1) | s->dummy_refresh_clock | out;
+ return ch.gate | (s->data_on << 1) | s->dummy_refresh_clock |
+ (ch.out << 5);
}
static void pcspk_io_write(void *opaque, target_phys_addr_t addr, uint64_t val,
--
1.7.3.4
^ permalink raw reply related [flat|nested] 19+ messages in thread