* [PATCH 01/11] mos6522: add defines for IFR bit flags
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
@ 2022-01-27 20:53 ` Mark Cave-Ayland
2022-01-27 23:16 ` BALATON Zoltan
2022-01-27 20:53 ` [PATCH 02/11] mac_via: use IFR bit flag constants for VIA1 IRQs Mark Cave-Ayland
` (9 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:53 UTC (permalink / raw)
To: laurent, qemu-devel
These are intended to make it easier to see how the physical control lines
are wired for each instance.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
include/hw/misc/mos6522.h | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index fc95d22b0f..12abd8b8d2 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -41,13 +41,21 @@
#define IER_SET 0x80 /* set bits in IER */
#define IER_CLR 0 /* clear bits in IER */
-#define CA2_INT 0x01
-#define CA1_INT 0x02
-#define SR_INT 0x04 /* Shift register full/empty */
-#define CB2_INT 0x08
-#define CB1_INT 0x10
-#define T2_INT 0x20 /* Timer 2 interrupt */
-#define T1_INT 0x40 /* Timer 1 interrupt */
+#define CA2_INT_BIT 0
+#define CA1_INT_BIT 1
+#define SR_INT_BIT 2 /* Shift register full/empty */
+#define CB2_INT_BIT 3
+#define CB1_INT_BIT 4
+#define T2_INT_BIT 5 /* Timer 2 interrupt */
+#define T1_INT_BIT 6 /* Timer 1 interrupt */
+
+#define CA2_INT (1 << CA2_INT_BIT)
+#define CA1_INT (1 << CA1_INT_BIT)
+#define SR_INT (1 << SR_INT_BIT)
+#define CB2_INT (1 << CB2_INT_BIT)
+#define CB1_INT (1 << CB1_INT_BIT)
+#define T2_INT (1 << T2_INT_BIT)
+#define T1_INT (1 << T1_INT_BIT)
/* Bits in ACR */
#define T1MODE 0xc0 /* Timer 1 mode */
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 01/11] mos6522: add defines for IFR bit flags
2022-01-27 20:53 ` [PATCH 01/11] mos6522: add defines for IFR bit flags Mark Cave-Ayland
@ 2022-01-27 23:16 ` BALATON Zoltan
2022-02-05 10:51 ` Mark Cave-Ayland
0 siblings, 1 reply; 45+ messages in thread
From: BALATON Zoltan @ 2022-01-27 23:16 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: laurent, qemu-devel
On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
> These are intended to make it easier to see how the physical control lines
> are wired for each instance.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index fc95d22b0f..12abd8b8d2 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -41,13 +41,21 @@
> #define IER_SET 0x80 /* set bits in IER */
> #define IER_CLR 0 /* clear bits in IER */
>
> -#define CA2_INT 0x01
> -#define CA1_INT 0x02
> -#define SR_INT 0x04 /* Shift register full/empty */
> -#define CB2_INT 0x08
> -#define CB1_INT 0x10
> -#define T2_INT 0x20 /* Timer 2 interrupt */
> -#define T1_INT 0x40 /* Timer 1 interrupt */
> +#define CA2_INT_BIT 0
> +#define CA1_INT_BIT 1
> +#define SR_INT_BIT 2 /* Shift register full/empty */
> +#define CB2_INT_BIT 3
> +#define CB1_INT_BIT 4
> +#define T2_INT_BIT 5 /* Timer 2 interrupt */
> +#define T1_INT_BIT 6 /* Timer 1 interrupt */
> +
> +#define CA2_INT (1 << CA2_INT_BIT)
> +#define CA1_INT (1 << CA1_INT_BIT)
> +#define SR_INT (1 << SR_INT_BIT)
> +#define CB2_INT (1 << CB2_INT_BIT)
> +#define CB1_INT (1 << CB1_INT_BIT)
> +#define T2_INT (1 << T2_INT_BIT)
> +#define T1_INT (1 << T1_INT_BIT)
Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT)
instead of the second set of #defines which would provide same readability
but with less #defines needed.
Regards,
BALATON Zoltan
> /* Bits in ACR */
> #define T1MODE 0xc0 /* Timer 1 mode */
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 01/11] mos6522: add defines for IFR bit flags
2022-01-27 23:16 ` BALATON Zoltan
@ 2022-02-05 10:51 ` Mark Cave-Ayland
2022-02-05 11:16 ` Philippe Mathieu-Daudé via
0 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-05 10:51 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: laurent, qemu-devel
On 27/01/2022 23:16, BALATON Zoltan wrote:
> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>> These are intended to make it easier to see how the physical control lines
>> are wired for each instance.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>> index fc95d22b0f..12abd8b8d2 100644
>> --- a/include/hw/misc/mos6522.h
>> +++ b/include/hw/misc/mos6522.h
>> @@ -41,13 +41,21 @@
>> #define IER_SET 0x80 /* set bits in IER */
>> #define IER_CLR 0 /* clear bits in IER */
>>
>> -#define CA2_INT 0x01
>> -#define CA1_INT 0x02
>> -#define SR_INT 0x04 /* Shift register full/empty */
>> -#define CB2_INT 0x08
>> -#define CB1_INT 0x10
>> -#define T2_INT 0x20 /* Timer 2 interrupt */
>> -#define T1_INT 0x40 /* Timer 1 interrupt */
>> +#define CA2_INT_BIT 0
>> +#define CA1_INT_BIT 1
>> +#define SR_INT_BIT 2 /* Shift register full/empty */
>> +#define CB2_INT_BIT 3
>> +#define CB1_INT_BIT 4
>> +#define T2_INT_BIT 5 /* Timer 2 interrupt */
>> +#define T1_INT_BIT 6 /* Timer 1 interrupt */
>> +
>> +#define CA2_INT (1 << CA2_INT_BIT)
>> +#define CA1_INT (1 << CA1_INT_BIT)
>> +#define SR_INT (1 << SR_INT_BIT)
>> +#define CB2_INT (1 << CB2_INT_BIT)
>> +#define CB1_INT (1 << CB1_INT_BIT)
>> +#define T2_INT (1 << T2_INT_BIT)
>> +#define T1_INT (1 << T1_INT_BIT)
>
> Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT) instead of
> the second set of #defines which would provide same readability but with less
> #defines needed.
I'm not so keen on removing the _INT defines since that would require updating all
users to use BIT() everywhere which I don't think gains us much. I could certainly
replace these definitions with BIT(FOO) instead of (1 << FOO) if that helps
readability though.
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 01/11] mos6522: add defines for IFR bit flags
2022-02-05 10:51 ` Mark Cave-Ayland
@ 2022-02-05 11:16 ` Philippe Mathieu-Daudé via
2022-02-05 12:06 ` BALATON Zoltan
0 siblings, 1 reply; 45+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-05 11:16 UTC (permalink / raw)
To: Mark Cave-Ayland, BALATON Zoltan; +Cc: laurent, qemu-devel
On 5/2/22 11:51, Mark Cave-Ayland wrote:
> On 27/01/2022 23:16, BALATON Zoltan wrote:
>
>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>> These are intended to make it easier to see how the physical control
>>> lines
>>> are wired for each instance.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>> index fc95d22b0f..12abd8b8d2 100644
>>> --- a/include/hw/misc/mos6522.h
>>> +++ b/include/hw/misc/mos6522.h
>>> @@ -41,13 +41,21 @@
>>> #define IER_SET 0x80 /* set bits in IER */
>>> #define IER_CLR 0 /* clear bits in IER */
>>>
>>> -#define CA2_INT 0x01
>>> -#define CA1_INT 0x02
>>> -#define SR_INT 0x04 /* Shift register full/empty */
>>> -#define CB2_INT 0x08
>>> -#define CB1_INT 0x10
>>> -#define T2_INT 0x20 /* Timer 2 interrupt */
>>> -#define T1_INT 0x40 /* Timer 1 interrupt */
>>> +#define CA2_INT_BIT 0
>>> +#define CA1_INT_BIT 1
>>> +#define SR_INT_BIT 2 /* Shift register full/empty */
>>> +#define CB2_INT_BIT 3
>>> +#define CB1_INT_BIT 4
>>> +#define T2_INT_BIT 5 /* Timer 2 interrupt */
>>> +#define T1_INT_BIT 6 /* Timer 1 interrupt */
>>> +
>>> +#define CA2_INT (1 << CA2_INT_BIT)
>>> +#define CA1_INT (1 << CA1_INT_BIT)
>>> +#define SR_INT (1 << SR_INT_BIT)
>>> +#define CB2_INT (1 << CB2_INT_BIT)
>>> +#define CB1_INT (1 << CB1_INT_BIT)
>>> +#define T2_INT (1 << T2_INT_BIT)
>>> +#define T1_INT (1 << T1_INT_BIT)
>>
>> Maybe you could leave the #defines called XX_INT and then use
>> BIT(XX_INT) instead of the second set of #defines which would provide
>> same readability but with less #defines needed.
>
> I'm not so keen on removing the _INT defines since that would require
> updating all users to use BIT() everywhere which I don't think gains us
> much. I could certainly replace these definitions with BIT(FOO) instead
> of (1 << FOO) if that helps readability though.
Do you mean simply doing this?
-#define T1_INT 0x40 /* Timer 1 interrupt */
+#define T1_INT BIT(6) /* Timer 1 interrupt */
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 01/11] mos6522: add defines for IFR bit flags
2022-02-05 11:16 ` Philippe Mathieu-Daudé via
@ 2022-02-05 12:06 ` BALATON Zoltan
2022-02-20 10:53 ` Mark Cave-Ayland
0 siblings, 1 reply; 45+ messages in thread
From: BALATON Zoltan @ 2022-02-05 12:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Mark Cave-Ayland, laurent, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3449 bytes --]
On Sat, 5 Feb 2022, Philippe Mathieu-Daudé wrote:
> On 5/2/22 11:51, Mark Cave-Ayland wrote:
>> On 27/01/2022 23:16, BALATON Zoltan wrote:
>>
>>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>>> These are intended to make it easier to see how the physical control
>>>> lines
>>>> are wired for each instance.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>>> index fc95d22b0f..12abd8b8d2 100644
>>>> --- a/include/hw/misc/mos6522.h
>>>> +++ b/include/hw/misc/mos6522.h
>>>> @@ -41,13 +41,21 @@
>>>> #define IER_SET 0x80 /* set bits in IER */
>>>> #define IER_CLR 0 /* clear bits in IER */
>>>>
>>>> -#define CA2_INT 0x01
>>>> -#define CA1_INT 0x02
>>>> -#define SR_INT 0x04 /* Shift register full/empty */
>>>> -#define CB2_INT 0x08
>>>> -#define CB1_INT 0x10
>>>> -#define T2_INT 0x20 /* Timer 2 interrupt */
>>>> -#define T1_INT 0x40 /* Timer 1 interrupt */
>>>> +#define CA2_INT_BIT 0
>>>> +#define CA1_INT_BIT 1
>>>> +#define SR_INT_BIT 2 /* Shift register full/empty */
>>>> +#define CB2_INT_BIT 3
>>>> +#define CB1_INT_BIT 4
>>>> +#define T2_INT_BIT 5 /* Timer 2 interrupt */
>>>> +#define T1_INT_BIT 6 /* Timer 1 interrupt */
>>>> +
>>>> +#define CA2_INT (1 << CA2_INT_BIT)
>>>> +#define CA1_INT (1 << CA1_INT_BIT)
>>>> +#define SR_INT (1 << SR_INT_BIT)
>>>> +#define CB2_INT (1 << CB2_INT_BIT)
>>>> +#define CB1_INT (1 << CB1_INT_BIT)
>>>> +#define T2_INT (1 << T2_INT_BIT)
>>>> +#define T1_INT (1 << T1_INT_BIT)
>>>
>>> Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT)
>>> instead of the second set of #defines which would provide same readability
>>> but with less #defines needed.
>>
>> I'm not so keen on removing the _INT defines since that would require
>> updating all users to use BIT() everywhere which I don't think gains us
>> much. I could certainly replace these definitions with BIT(FOO) instead of
>> (1 << FOO) if that helps readability though.
>
> Do you mean simply doing this?
>
> -#define T1_INT 0x40 /* Timer 1 interrupt */
> +#define T1_INT BIT(6) /* Timer 1 interrupt */
I meant:
#define T1_INT 6
and then replace current usage of T1_INT with BIT(T1_INT) that way we
don't need both T1_INT_BIT and T1_INT defines which seems redundant as
BIT(T1_INT) is not much longer and still clear what it refers to. It's
true that this needs more changes but the result is more readable IMO than
introducing another set of defines that ome has to look up when encounters
them as the meaning might not be clear. That's why I think one set of
defines for bit numbers and using existing BIT(num) for values is better
but it's just an idea, I don't care that much.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 01/11] mos6522: add defines for IFR bit flags
2022-02-05 12:06 ` BALATON Zoltan
@ 2022-02-20 10:53 ` Mark Cave-Ayland
2022-02-20 19:21 ` BALATON Zoltan
0 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-20 10:53 UTC (permalink / raw)
To: BALATON Zoltan, Philippe Mathieu-Daudé; +Cc: laurent, qemu-devel
On 05/02/2022 12:06, BALATON Zoltan wrote:
> On Sat, 5 Feb 2022, Philippe Mathieu-Daudé wrote:
>> On 5/2/22 11:51, Mark Cave-Ayland wrote:
>>> On 27/01/2022 23:16, BALATON Zoltan wrote:
>>>
>>>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>>>> These are intended to make it easier to see how the physical control lines
>>>>> are wired for each instance.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>>>> index fc95d22b0f..12abd8b8d2 100644
>>>>> --- a/include/hw/misc/mos6522.h
>>>>> +++ b/include/hw/misc/mos6522.h
>>>>> @@ -41,13 +41,21 @@
>>>>> #define IER_SET 0x80 /* set bits in IER */
>>>>> #define IER_CLR 0 /* clear bits in IER */
>>>>>
>>>>> -#define CA2_INT 0x01
>>>>> -#define CA1_INT 0x02
>>>>> -#define SR_INT 0x04 /* Shift register full/empty */
>>>>> -#define CB2_INT 0x08
>>>>> -#define CB1_INT 0x10
>>>>> -#define T2_INT 0x20 /* Timer 2 interrupt */
>>>>> -#define T1_INT 0x40 /* Timer 1 interrupt */
>>>>> +#define CA2_INT_BIT 0
>>>>> +#define CA1_INT_BIT 1
>>>>> +#define SR_INT_BIT 2 /* Shift register full/empty */
>>>>> +#define CB2_INT_BIT 3
>>>>> +#define CB1_INT_BIT 4
>>>>> +#define T2_INT_BIT 5 /* Timer 2 interrupt */
>>>>> +#define T1_INT_BIT 6 /* Timer 1 interrupt */
>>>>> +
>>>>> +#define CA2_INT (1 << CA2_INT_BIT)
>>>>> +#define CA1_INT (1 << CA1_INT_BIT)
>>>>> +#define SR_INT (1 << SR_INT_BIT)
>>>>> +#define CB2_INT (1 << CB2_INT_BIT)
>>>>> +#define CB1_INT (1 << CB1_INT_BIT)
>>>>> +#define T2_INT (1 << T2_INT_BIT)
>>>>> +#define T1_INT (1 << T1_INT_BIT)
>>>>
>>>> Maybe you could leave the #defines called XX_INT and then use BIT(XX_INT) instead
>>>> of the second set of #defines which would provide same readability but with less
>>>> #defines needed.
>>>
>>> I'm not so keen on removing the _INT defines since that would require updating all
>>> users to use BIT() everywhere which I don't think gains us much. I could certainly
>>> replace these definitions with BIT(FOO) instead of (1 << FOO) if that helps
>>> readability though.
>>
>> Do you mean simply doing this?
>>
>> -#define T1_INT 0x40 /* Timer 1 interrupt */
>> +#define T1_INT BIT(6) /* Timer 1 interrupt */
>
> I meant:
>
> #define T1_INT 6
>
> and then replace current usage of T1_INT with BIT(T1_INT) that way we don't need both
> T1_INT_BIT and T1_INT defines which seems redundant as BIT(T1_INT) is not much longer
> and still clear what it refers to. It's true that this needs more changes but the
> result is more readable IMO than introducing another set of defines that ome has to
> look up when encounters them as the meaning might not be clear. That's why I think
> one set of defines for bit numbers and using existing BIT(num) for values is better
> but it's just an idea, I don't care that much.
I think the best solution here is to just use BIT() for the final shifted values like
this:
#define CA2_INT_BIT 0
...
...
#define CA2_INT BIT(CA2_INT_BIT)
Otherwise I can see there being confusion given that the BIT() macro is used for
defines without a _BIT suffix which are also being used elsewhere. I'll update this
in v2 accordingly.
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 01/11] mos6522: add defines for IFR bit flags
2022-02-20 10:53 ` Mark Cave-Ayland
@ 2022-02-20 19:21 ` BALATON Zoltan
0 siblings, 0 replies; 45+ messages in thread
From: BALATON Zoltan @ 2022-02-20 19:21 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, Philippe Mathieu-Daudé, laurent
[-- Attachment #1: Type: text/plain, Size: 5258 bytes --]
On Sun, 20 Feb 2022, Mark Cave-Ayland wrote:
> On 05/02/2022 12:06, BALATON Zoltan wrote:
>> On Sat, 5 Feb 2022, Philippe Mathieu-Daudé wrote:
>>> On 5/2/22 11:51, Mark Cave-Ayland wrote:
>>>> On 27/01/2022 23:16, BALATON Zoltan wrote:
>>>>
>>>>> On Thu, 27 Jan 2022, Mark Cave-Ayland wrote:
>>>>>> These are intended to make it easier to see how the physical control
>>>>>> lines
>>>>>> are wired for each instance.
>>>>>>
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>> ---
>>>>>> include/hw/misc/mos6522.h | 22 +++++++++++++++-------
>>>>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>>>>>> index fc95d22b0f..12abd8b8d2 100644
>>>>>> --- a/include/hw/misc/mos6522.h
>>>>>> +++ b/include/hw/misc/mos6522.h
>>>>>> @@ -41,13 +41,21 @@
>>>>>> #define IER_SET 0x80 /* set bits in IER */
>>>>>> #define IER_CLR 0 /* clear bits in IER */
>>>>>>
>>>>>> -#define CA2_INT 0x01
>>>>>> -#define CA1_INT 0x02
>>>>>> -#define SR_INT 0x04 /* Shift register full/empty */
>>>>>> -#define CB2_INT 0x08
>>>>>> -#define CB1_INT 0x10
>>>>>> -#define T2_INT 0x20 /* Timer 2 interrupt */
>>>>>> -#define T1_INT 0x40 /* Timer 1 interrupt */
>>>>>> +#define CA2_INT_BIT 0
>>>>>> +#define CA1_INT_BIT 1
>>>>>> +#define SR_INT_BIT 2 /* Shift register full/empty */
>>>>>> +#define CB2_INT_BIT 3
>>>>>> +#define CB1_INT_BIT 4
>>>>>> +#define T2_INT_BIT 5 /* Timer 2 interrupt */
>>>>>> +#define T1_INT_BIT 6 /* Timer 1 interrupt */
>>>>>> +
>>>>>> +#define CA2_INT (1 << CA2_INT_BIT)
>>>>>> +#define CA1_INT (1 << CA1_INT_BIT)
>>>>>> +#define SR_INT (1 << SR_INT_BIT)
>>>>>> +#define CB2_INT (1 << CB2_INT_BIT)
>>>>>> +#define CB1_INT (1 << CB1_INT_BIT)
>>>>>> +#define T2_INT (1 << T2_INT_BIT)
>>>>>> +#define T1_INT (1 << T1_INT_BIT)
>>>>>
>>>>> Maybe you could leave the #defines called XX_INT and then use
>>>>> BIT(XX_INT) instead of the second set of #defines which would provide
>>>>> same readability but with less #defines needed.
>>>>
>>>> I'm not so keen on removing the _INT defines since that would require
>>>> updating all users to use BIT() everywhere which I don't think gains us
>>>> much. I could certainly replace these definitions with BIT(FOO) instead
>>>> of (1 << FOO) if that helps readability though.
>>>
>>> Do you mean simply doing this?
>>>
>>> -#define T1_INT 0x40 /* Timer 1 interrupt */
>>> +#define T1_INT BIT(6) /* Timer 1 interrupt */
>>
>> I meant:
>>
>> #define T1_INT 6
>>
>> and then replace current usage of T1_INT with BIT(T1_INT) that way we don't
>> need both T1_INT_BIT and T1_INT defines which seems redundant as
>> BIT(T1_INT) is not much longer and still clear what it refers to. It's true
>> that this needs more changes but the result is more readable IMO than
>> introducing another set of defines that ome has to look up when encounters
>> them as the meaning might not be clear. That's why I think one set of
>> defines for bit numbers and using existing BIT(num) for values is better
>> but it's just an idea, I don't care that much.
>
> I think the best solution here is to just use BIT() for the final shifted
> values like this:
>
> #define CA2_INT_BIT 0
> ...
> ...
> #define CA2_INT BIT(CA2_INT_BIT)
That does not really help much as the idea was to avoid having two set of
defines and only have one set for the bit numbers then use the BIT() macro
instead of the current values. Using the BIT() macro in the second set of
defines does not help reduce the number of defines in code which the
reader will have to look up in this header. IMO having defines only for
bit numbers and always using BIT(whatever) for values is less confusing
assuming one is familiar with what the BIT() macro does.
> Otherwise I can see there being confusion given that the BIT() macro is used
> for defines without a _BIT suffix which are also being used elsewhere.
Maybe it's only confusing to you as you've named the bit numbers *_BIT and
the values without BIT and my proposal was to name the bit numbers as the
simple names and use BIT(name) for the value which looks kind of opposite
naming but it's the simplest. I guess you could also have bit numbers
named *_BIT and then use BIT(CA2_INT_BIT) instead of the second set of
defines for the values but that looks a bit redundant and maybe more
confusing than just using BIT(CA2_INT).
> I'll update this in v2 accordingly.
I don't have a strong opinion on this so if you prefer the way it is now
or using the BIT() macro only for the separate defines then do that. I
don't mind either way.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 02/11] mac_via: use IFR bit flag constants for VIA1 IRQs
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
2022-01-27 20:53 ` [PATCH 01/11] mos6522: add defines for IFR bit flags Mark Cave-Ayland
@ 2022-01-27 20:53 ` Mark Cave-Ayland
2022-01-27 20:53 ` [PATCH 03/11] mac_via: use IFR bit flag constants for VIA2 IRQs Mark Cave-Ayland
` (8 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:53 UTC (permalink / raw)
To: laurent, qemu-devel
This allows us to easily see how the physical control lines are mapped to the
IFR bit flags.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
include/hw/misc/mac_via.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index b445565866..b0c3825c9b 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -18,11 +18,11 @@
#define VIA_SIZE 0x2000
/* VIA 1 */
-#define VIA1_IRQ_ONE_SECOND_BIT 0
-#define VIA1_IRQ_60HZ_BIT 1
-#define VIA1_IRQ_ADB_READY_BIT 2
-#define VIA1_IRQ_ADB_DATA_BIT 3
-#define VIA1_IRQ_ADB_CLOCK_BIT 4
+#define VIA1_IRQ_ONE_SECOND_BIT CA2_INT_BIT
+#define VIA1_IRQ_60HZ_BIT CA1_INT_BIT
+#define VIA1_IRQ_ADB_READY_BIT SR_INT_BIT
+#define VIA1_IRQ_ADB_DATA_BIT CB2_INT_BIT
+#define VIA1_IRQ_ADB_CLOCK_BIT CB1_INT_BIT
#define VIA1_IRQ_NB 8
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 03/11] mac_via: use IFR bit flag constants for VIA2 IRQs
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
2022-01-27 20:53 ` [PATCH 01/11] mos6522: add defines for IFR bit flags Mark Cave-Ayland
2022-01-27 20:53 ` [PATCH 02/11] mac_via: use IFR bit flag constants for VIA1 IRQs Mark Cave-Ayland
@ 2022-01-27 20:53 ` Mark Cave-Ayland
2022-01-27 20:53 ` [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs Mark Cave-Ayland
` (7 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:53 UTC (permalink / raw)
To: laurent, qemu-devel
This allows us to easily see how the physical control lines are mapped to the
IFR bit flags.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
include/hw/misc/mac_via.h | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index b0c3825c9b..2df1ab01b6 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -80,11 +80,10 @@ struct MOS6522Q800VIA1State {
/* VIA 2 */
-#define VIA2_IRQ_SCSI_DATA_BIT 0
-#define VIA2_IRQ_NUBUS_BIT 1
-#define VIA2_IRQ_UNUSED_BIT 2
-#define VIA2_IRQ_SCSI_BIT 3
-#define VIA2_IRQ_ASC_BIT 4
+#define VIA2_IRQ_SCSI_DATA_BIT CA2_INT_BIT
+#define VIA2_IRQ_NUBUS_BIT CA1_INT_BIT
+#define VIA2_IRQ_SCSI_BIT CB2_INT_BIT
+#define VIA2_IRQ_ASC_BIT CB1_INT_BIT
#define VIA2_IRQ_NB 8
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (2 preceding siblings ...)
2022-01-27 20:53 ` [PATCH 03/11] mac_via: use IFR bit flag constants for VIA2 IRQs Mark Cave-Ayland
@ 2022-01-27 20:53 ` Mark Cave-Ayland
2022-02-07 19:29 ` Peter Maydell
2022-01-27 20:53 ` [PATCH 05/11] mos6522: remove update_irq() and set_sr_int() methods from MOS6522DeviceClass Mark Cave-Ayland
` (6 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:53 UTC (permalink / raw)
To: laurent, qemu-devel
For historical reasons each mos6522 instance implements its own setting and
update of the IFR flag bits using methods exposed by MOS6522DeviceClass. As
of today this is no longer required, and it is now possible to implement
the mos6522 IRQs as standard qdev gpios.
Switch over to use qdev gpios for the mos6522 device and update all instances
accordingly.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/mac_via.c | 56 +++++++--------------------------------
hw/misc/macio/cuda.c | 5 ++--
hw/misc/macio/pmu.c | 4 +--
hw/misc/mos6522.c | 15 +++++++++++
include/hw/misc/mac_via.h | 6 +----
include/hw/misc/mos6522.h | 2 ++
6 files changed, 32 insertions(+), 56 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b378e6b305..0756374f1b 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -325,10 +325,9 @@ static void via1_sixty_hz(void *opaque)
{
MOS6522Q800VIA1State *v1s = opaque;
MOS6522State *s = MOS6522(v1s);
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+ qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_60HZ_BIT);
- s->ifr |= VIA1_IRQ_60HZ;
- mdc->update_irq(s);
+ qemu_set_irq(irq, 1);
via1_sixty_hz_update(v1s);
}
@@ -337,44 +336,13 @@ static void via1_one_second(void *opaque)
{
MOS6522Q800VIA1State *v1s = opaque;
MOS6522State *s = MOS6522(v1s);
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+ qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_ONE_SECOND_BIT);
- s->ifr |= VIA1_IRQ_ONE_SECOND;
- mdc->update_irq(s);
+ qemu_set_irq(irq, 1);
via1_one_second_update(v1s);
}
-static void via1_irq_request(void *opaque, int irq, int level)
-{
- MOS6522Q800VIA1State *v1s = opaque;
- MOS6522State *s = MOS6522(v1s);
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
- if (level) {
- s->ifr |= 1 << irq;
- } else {
- s->ifr &= ~(1 << irq);
- }
-
- mdc->update_irq(s);
-}
-
-static void via2_irq_request(void *opaque, int irq, int level)
-{
- MOS6522Q800VIA2State *v2s = opaque;
- MOS6522State *s = MOS6522(v2s);
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
- if (level) {
- s->ifr |= 1 << irq;
- } else {
- s->ifr &= ~(1 << irq);
- }
-
- mdc->update_irq(s);
-}
-
static void pram_update(MOS6522Q800VIA1State *v1s)
{
@@ -1061,8 +1029,6 @@ static void mos6522_q800_via1_init(Object *obj)
qbus_init((BusState *)&v1s->adb_bus, sizeof(v1s->adb_bus),
TYPE_ADB_BUS, DEVICE(v1s), "adb.0");
- qdev_init_gpio_in(DEVICE(obj), via1_irq_request, VIA1_IRQ_NB);
-
/* A/UX mode */
qdev_init_gpio_out(DEVICE(obj), &v1s->auxmode_irq, 1);
}
@@ -1150,22 +1116,20 @@ static void mos6522_q800_via2_reset(DeviceState *dev)
ms->a = 0x7f;
}
-static void via2_nubus_irq_request(void *opaque, int irq, int level)
+static void via2_nubus_irq_request(void *opaque, int n, int level)
{
MOS6522Q800VIA2State *v2s = opaque;
MOS6522State *s = MOS6522(v2s);
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+ qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA2_IRQ_NUBUS_BIT);
if (level) {
/* Port A nubus IRQ inputs are active LOW */
- s->a &= ~(1 << irq);
- s->ifr |= 1 << VIA2_IRQ_NUBUS_BIT;
+ s->a &= ~(1 << n);
} else {
- s->a |= (1 << irq);
- s->ifr &= ~(1 << VIA2_IRQ_NUBUS_BIT);
+ s->a |= (1 << n);
}
- mdc->update_irq(s);
+ qemu_set_irq(irq, level);
}
static void mos6522_q800_via2_init(Object *obj)
@@ -1177,8 +1141,6 @@ static void mos6522_q800_via2_init(Object *obj)
"via2", VIA_SIZE);
sysbus_init_mmio(sbd, &v2s->via_mem);
- qdev_init_gpio_in(DEVICE(obj), via2_irq_request, VIA2_IRQ_NB);
-
qdev_init_gpio_in_named(DEVICE(obj), via2_nubus_irq_request, "nubus-irq",
VIA2_NUBUS_IRQ_NB);
}
diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index e917a6a095..f76d9227d3 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include "qemu-common.h"
+#include "hw/irq.h"
#include "hw/ppc/mac.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
@@ -96,9 +97,9 @@ static void cuda_set_sr_int(void *opaque)
CUDAState *s = opaque;
MOS6522CUDAState *mcs = &s->mos6522_cuda;
MOS6522State *ms = MOS6522(mcs);
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(ms);
+ qemu_irq irq = qdev_get_gpio_in(DEVICE(ms), SR_INT_BIT);
- mdc->set_sr_int(ms);
+ qemu_set_irq(irq, 1);
}
static void cuda_delay_set_sr_int(CUDAState *s)
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index eb39c64694..6e80fe1cfa 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -75,9 +75,9 @@ static void via_set_sr_int(void *opaque)
PMUState *s = opaque;
MOS6522PMUState *mps = MOS6522_PMU(&s->mos6522_pmu);
MOS6522State *ms = MOS6522(mps);
- MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(ms);
+ qemu_irq irq = qdev_get_gpio_in(DEVICE(ms), SR_INT_BIT);
- mdc->set_sr_int(ms);
+ qemu_set_irq(irq, 1);
}
static void pmu_update_extirq(PMUState *s)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1c57332b40..6be6853dc2 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -52,6 +52,19 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
+static void mos6522_set_irq(void *opaque, int n, int level)
+{
+ MOS6522State *s = MOS6522(opaque);
+
+ if (level) {
+ s->ifr |= 1 << n;
+ } else {
+ s->ifr &= ~(1 << n);
+ }
+
+ mos6522_update_irq(s);
+}
+
static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
{
MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
@@ -488,6 +501,8 @@ static void mos6522_init(Object *obj)
s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
+
+ qdev_init_gpio_in(DEVICE(obj), mos6522_set_irq, VIA_NUM_INTS);
}
static void mos6522_finalize(Object *obj)
diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index 2df1ab01b6..bffeba38ee 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -24,8 +24,6 @@
#define VIA1_IRQ_ADB_DATA_BIT CB2_INT_BIT
#define VIA1_IRQ_ADB_CLOCK_BIT CB1_INT_BIT
-#define VIA1_IRQ_NB 8
-
#define VIA1_IRQ_ONE_SECOND (1 << VIA1_IRQ_ONE_SECOND_BIT)
#define VIA1_IRQ_60HZ (1 << VIA1_IRQ_60HZ_BIT)
#define VIA1_IRQ_ADB_READY (1 << VIA1_IRQ_ADB_READY_BIT)
@@ -42,7 +40,7 @@ struct MOS6522Q800VIA1State {
MemoryRegion via_mem;
- qemu_irq irqs[VIA1_IRQ_NB];
+ qemu_irq irqs[VIA_NUM_INTS];
qemu_irq auxmode_irq;
uint8_t last_b;
@@ -85,8 +83,6 @@ struct MOS6522Q800VIA1State {
#define VIA2_IRQ_SCSI_BIT CB2_INT_BIT
#define VIA2_IRQ_ASC_BIT CB1_INT_BIT
-#define VIA2_IRQ_NB 8
-
#define VIA2_IRQ_SCSI_DATA (1 << VIA2_IRQ_SCSI_DATA_BIT)
#define VIA2_IRQ_NUBUS (1 << VIA2_IRQ_NUBUS_BIT)
#define VIA2_IRQ_UNUSED (1 << VIA2_IRQ_SCSI_BIT)
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 12abd8b8d2..ced8a670bf 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -57,6 +57,8 @@
#define T2_INT (1 << T2_INT_BIT)
#define T1_INT (1 << T1_INT_BIT)
+#define VIA_NUM_INTS 5
+
/* Bits in ACR */
#define T1MODE 0xc0 /* Timer 1 mode */
#define T1MODE_CONT 0x40 /* continuous interrupts */
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs
2022-01-27 20:53 ` [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs Mark Cave-Ayland
@ 2022-02-07 19:29 ` Peter Maydell
2022-02-20 11:01 ` Mark Cave-Ayland
0 siblings, 1 reply; 45+ messages in thread
From: Peter Maydell @ 2022-02-07 19:29 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: Laurent, qemu-devel
On Thu, 27 Jan 2022 at 21:01, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> For historical reasons each mos6522 instance implements its own setting and
> update of the IFR flag bits using methods exposed by MOS6522DeviceClass. As
> of today this is no longer required, and it is now possible to implement
> the mos6522 IRQs as standard qdev gpios.
>
> Switch over to use qdev gpios for the mos6522 device and update all instances
> accordingly.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/misc/mac_via.c | 56 +++++++--------------------------------
> hw/misc/macio/cuda.c | 5 ++--
> hw/misc/macio/pmu.c | 4 +--
> hw/misc/mos6522.c | 15 +++++++++++
> include/hw/misc/mac_via.h | 6 +----
> include/hw/misc/mos6522.h | 2 ++
> 6 files changed, 32 insertions(+), 56 deletions(-)
> -static void via2_nubus_irq_request(void *opaque, int irq, int level)
> +static void via2_nubus_irq_request(void *opaque, int n, int level)
> {
> MOS6522Q800VIA2State *v2s = opaque;
> MOS6522State *s = MOS6522(v2s);
> - MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
> + qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA2_IRQ_NUBUS_BIT);
>
> if (level) {
> /* Port A nubus IRQ inputs are active LOW */
> - s->a &= ~(1 << irq);
> - s->ifr |= 1 << VIA2_IRQ_NUBUS_BIT;
> + s->a &= ~(1 << n);
> } else {
> - s->a |= (1 << irq);
> - s->ifr &= ~(1 << VIA2_IRQ_NUBUS_BIT);
> + s->a |= (1 << n);
> }
>
> - mdc->update_irq(s);
> + qemu_set_irq(irq, level);
> }
It feels a bit inconsistent here that we're still reaching into
the MOS6522State to set s->a, but I guess this is still
better than what we had before.
> -#define VIA1_IRQ_NB 8
> -
> #define VIA1_IRQ_ONE_SECOND (1 << VIA1_IRQ_ONE_SECOND_BIT)
> #define VIA1_IRQ_60HZ (1 << VIA1_IRQ_60HZ_BIT)
> #define VIA1_IRQ_ADB_READY (1 << VIA1_IRQ_ADB_READY_BIT)
> @@ -42,7 +40,7 @@ struct MOS6522Q800VIA1State {
>
> MemoryRegion via_mem;
>
> - qemu_irq irqs[VIA1_IRQ_NB];
> + qemu_irq irqs[VIA_NUM_INTS];
This irqs[] array appears to be entirely unused. You could
delete it as a separate patch before this one.
> qemu_irq auxmode_irq;
> uint8_t last_b;
>
> @@ -85,8 +83,6 @@ struct MOS6522Q800VIA1State {
> #define VIA2_IRQ_SCSI_BIT CB2_INT_BIT
> #define VIA2_IRQ_ASC_BIT CB1_INT_BIT
>
> -#define VIA2_IRQ_NB 8
> -
> #define VIA2_IRQ_SCSI_DATA (1 << VIA2_IRQ_SCSI_DATA_BIT)
> #define VIA2_IRQ_NUBUS (1 << VIA2_IRQ_NUBUS_BIT)
> #define VIA2_IRQ_UNUSED (1 << VIA2_IRQ_SCSI_BIT)
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index 12abd8b8d2..ced8a670bf 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -57,6 +57,8 @@
> #define T2_INT (1 << T2_INT_BIT)
> #define T1_INT (1 << T1_INT_BIT)
>
> +#define VIA_NUM_INTS 5
Were we not using 5,6,7 previously ?
Anyway,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs
2022-02-07 19:29 ` Peter Maydell
@ 2022-02-20 11:01 ` Mark Cave-Ayland
0 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-20 11:01 UTC (permalink / raw)
To: Peter Maydell; +Cc: Laurent, qemu-devel
On 07/02/2022 19:29, Peter Maydell wrote:
> On Thu, 27 Jan 2022 at 21:01, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> For historical reasons each mos6522 instance implements its own setting and
>> update of the IFR flag bits using methods exposed by MOS6522DeviceClass. As
>> of today this is no longer required, and it is now possible to implement
>> the mos6522 IRQs as standard qdev gpios.
>>
>> Switch over to use qdev gpios for the mos6522 device and update all instances
>> accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/misc/mac_via.c | 56 +++++++--------------------------------
>> hw/misc/macio/cuda.c | 5 ++--
>> hw/misc/macio/pmu.c | 4 +--
>> hw/misc/mos6522.c | 15 +++++++++++
>> include/hw/misc/mac_via.h | 6 +----
>> include/hw/misc/mos6522.h | 2 ++
>> 6 files changed, 32 insertions(+), 56 deletions(-)
>
>
>> -static void via2_nubus_irq_request(void *opaque, int irq, int level)
>> +static void via2_nubus_irq_request(void *opaque, int n, int level)
>> {
>> MOS6522Q800VIA2State *v2s = opaque;
>> MOS6522State *s = MOS6522(v2s);
>> - MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
>> + qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA2_IRQ_NUBUS_BIT);
>>
>> if (level) {
>> /* Port A nubus IRQ inputs are active LOW */
>> - s->a &= ~(1 << irq);
>> - s->ifr |= 1 << VIA2_IRQ_NUBUS_BIT;
>> + s->a &= ~(1 << n);
>> } else {
>> - s->a |= (1 << irq);
>> - s->ifr &= ~(1 << VIA2_IRQ_NUBUS_BIT);
>> + s->a |= (1 << n);
>> }
>>
>> - mdc->update_irq(s);
>> + qemu_set_irq(irq, level);
>> }
>
> It feels a bit inconsistent here that we're still reaching into
> the MOS6522State to set s->a, but I guess this is still
> better than what we had before.
Yeah it's a little bit messy. On the Mac the Nubus IRQs are ORd to produce the VIA
interrupt line, but the individual Nubus IRQ lines are wired to the VIA port A to
allow the state of each line to be determined. The VIA port IO lines are
bi-directional, so rather than try and implement this with GPIOs it's easiest to have
a separate VIA2 Nubus GPIOs that also handle the port IO lines as above.
>> -#define VIA1_IRQ_NB 8
>> -
>> #define VIA1_IRQ_ONE_SECOND (1 << VIA1_IRQ_ONE_SECOND_BIT)
>> #define VIA1_IRQ_60HZ (1 << VIA1_IRQ_60HZ_BIT)
>> #define VIA1_IRQ_ADB_READY (1 << VIA1_IRQ_ADB_READY_BIT)
>> @@ -42,7 +40,7 @@ struct MOS6522Q800VIA1State {
>>
>> MemoryRegion via_mem;
>>
>> - qemu_irq irqs[VIA1_IRQ_NB];
>> + qemu_irq irqs[VIA_NUM_INTS];
>
> This irqs[] array appears to be entirely unused. You could
> delete it as a separate patch before this one.
Ah yes, before this patch each VIA had its own GPIOs which used the methods in
MOS6522DeviceClass to manipulate the IFR bit. Since the Q800 VIAs have the MOS6522
device as a parent class, the GPIOs are inherited from that and so this array should
actually be removed as part of this patch.
>> qemu_irq auxmode_irq;
>> uint8_t last_b;
>>
>> @@ -85,8 +83,6 @@ struct MOS6522Q800VIA1State {
>> #define VIA2_IRQ_SCSI_BIT CB2_INT_BIT
>> #define VIA2_IRQ_ASC_BIT CB1_INT_BIT
>>
>> -#define VIA2_IRQ_NB 8
>> -
>> #define VIA2_IRQ_SCSI_DATA (1 << VIA2_IRQ_SCSI_DATA_BIT)
>> #define VIA2_IRQ_NUBUS (1 << VIA2_IRQ_NUBUS_BIT)
>> #define VIA2_IRQ_UNUSED (1 << VIA2_IRQ_SCSI_BIT)
>> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
>> index 12abd8b8d2..ced8a670bf 100644
>> --- a/include/hw/misc/mos6522.h
>> +++ b/include/hw/misc/mos6522.h
>> @@ -57,6 +57,8 @@
>> #define T2_INT (1 << T2_INT_BIT)
>> #define T1_INT (1 << T1_INT_BIT)
>>
>> +#define VIA_NUM_INTS 5
>
> Were we not using 5,6,7 previously ?
5 and 6 are for the in-built timers, and 7 is a set/clear flag so they are not
externally accessible.
> Anyway,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 05/11] mos6522: remove update_irq() and set_sr_int() methods from MOS6522DeviceClass
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (3 preceding siblings ...)
2022-01-27 20:53 ` [PATCH 04/11] mos6522: switch over to use qdev gpios for IRQs Mark Cave-Ayland
@ 2022-01-27 20:53 ` Mark Cave-Ayland
2022-02-07 19:30 ` Peter Maydell
2022-01-27 20:54 ` [PATCH 06/11] mos6522: use device_class_set_parent_reset() to propagate reset to parent Mark Cave-Ayland
` (5 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:53 UTC (permalink / raw)
To: laurent, qemu-devel
Now that the mos6522 IRQs are managed using standard qdev gpios these methods
are no longer required.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/mos6522.c | 9 ---------
include/hw/misc/mos6522.h | 2 --
2 files changed, 11 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 6be6853dc2..4c3147a7d1 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -208,13 +208,6 @@ static void mos6522_timer2(void *opaque)
mos6522_update_irq(s);
}
-static void mos6522_set_sr_int(MOS6522State *s)
-{
- trace_mos6522_set_sr_int();
- s->ifr |= SR_INT;
- mos6522_update_irq(s);
-}
-
static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
{
return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
@@ -527,10 +520,8 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
dc->vmsd = &vmstate_mos6522;
device_class_set_props(dc, mos6522_properties);
mdc->parent_reset = dc->reset;
- mdc->set_sr_int = mos6522_set_sr_int;
mdc->portB_write = mos6522_portB_write;
mdc->portA_write = mos6522_portA_write;
- mdc->update_irq = mos6522_update_irq;
mdc->get_timer1_counter_value = mos6522_get_counter_value;
mdc->get_timer2_counter_value = mos6522_get_counter_value;
mdc->get_timer1_load_time = mos6522_get_load_time;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index ced8a670bf..f6b513a3a2 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -140,10 +140,8 @@ struct MOS6522DeviceClass {
DeviceClass parent_class;
DeviceReset parent_reset;
- void (*set_sr_int)(MOS6522State *dev);
void (*portB_write)(MOS6522State *dev);
void (*portA_write)(MOS6522State *dev);
- void (*update_irq)(MOS6522State *dev);
/* These are used to influence the CUDA MacOS timebase calibration */
uint64_t (*get_timer1_counter_value)(MOS6522State *dev, MOS6522Timer *ti);
uint64_t (*get_timer2_counter_value)(MOS6522State *dev, MOS6522Timer *ti);
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 06/11] mos6522: use device_class_set_parent_reset() to propagate reset to parent
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (4 preceding siblings ...)
2022-01-27 20:53 ` [PATCH 05/11] mos6522: remove update_irq() and set_sr_int() methods from MOS6522DeviceClass Mark Cave-Ayland
@ 2022-01-27 20:54 ` Mark Cave-Ayland
2022-02-07 19:31 ` Peter Maydell
2022-01-27 20:54 ` [PATCH 07/11] mos6522: add register names to register read/write trace events Mark Cave-Ayland
` (4 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:54 UTC (permalink / raw)
To: laurent, qemu-devel
Switch from using a legacy approach to the more formal approach for propagating
device reset to the parent.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/mac_via.c | 7 +++++--
hw/misc/macio/cuda.c | 3 ++-
hw/misc/macio/pmu.c | 3 ++-
hw/misc/mos6522.c | 1 -
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 0756374f1b..95cf2e03d9 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -1076,9 +1076,11 @@ static Property mos6522_q800_via1_properties[] = {
static void mos6522_q800_via1_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
+ MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
dc->realize = mos6522_q800_via1_realize;
- dc->reset = mos6522_q800_via1_reset;
+ device_class_set_parent_reset(dc, mos6522_q800_via1_reset,
+ &mdc->parent_reset);
dc->vmsd = &vmstate_q800_via1;
device_class_set_props(dc, mos6522_q800_via1_properties);
}
@@ -1161,7 +1163,8 @@ static void mos6522_q800_via2_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
- dc->reset = mos6522_q800_via2_reset;
+ device_class_set_parent_reset(dc, mos6522_q800_via2_reset,
+ &mdc->parent_reset);
dc->vmsd = &vmstate_q800_via2;
mdc->portB_write = mos6522_q800_via2_portB_write;
}
diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index f76d9227d3..4ced31c2c5 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -606,7 +606,8 @@ static void mos6522_cuda_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
- dc->reset = mos6522_cuda_reset;
+ device_class_set_parent_reset(dc, mos6522_cuda_reset,
+ &mdc->parent_reset);
mdc->portB_write = mos6522_cuda_portB_write;
mdc->get_timer1_counter_value = cuda_get_counter_value;
mdc->get_timer2_counter_value = cuda_get_counter_value;
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index 6e80fe1cfa..a5dd0a4734 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -850,7 +850,8 @@ static void mos6522_pmu_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
MOS6522DeviceClass *mdc = MOS6522_CLASS(oc);
- dc->reset = mos6522_pmu_reset;
+ device_class_set_parent_reset(dc, mos6522_pmu_reset,
+ &mdc->parent_reset);
mdc->portB_write = mos6522_pmu_portB_write;
mdc->portA_write = mos6522_pmu_portA_write;
}
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 4c3147a7d1..093cc83dcf 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -519,7 +519,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
dc->reset = mos6522_reset;
dc->vmsd = &vmstate_mos6522;
device_class_set_props(dc, mos6522_properties);
- mdc->parent_reset = dc->reset;
mdc->portB_write = mos6522_portB_write;
mdc->portA_write = mos6522_portA_write;
mdc->get_timer1_counter_value = mos6522_get_counter_value;
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 06/11] mos6522: use device_class_set_parent_reset() to propagate reset to parent
2022-01-27 20:54 ` [PATCH 06/11] mos6522: use device_class_set_parent_reset() to propagate reset to parent Mark Cave-Ayland
@ 2022-02-07 19:31 ` Peter Maydell
0 siblings, 0 replies; 45+ messages in thread
From: Peter Maydell @ 2022-02-07 19:31 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: Laurent, qemu-devel
On Thu, 27 Jan 2022 at 21:04, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Switch from using a legacy approach to the more formal approach for propagating
> device reset to the parent.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/misc/mac_via.c | 7 +++++--
> hw/misc/macio/cuda.c | 3 ++-
> hw/misc/macio/pmu.c | 3 ++-
> hw/misc/mos6522.c | 1 -
> 4 files changed, 9 insertions(+), 5 deletions(-)
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 07/11] mos6522: add register names to register read/write trace events
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (5 preceding siblings ...)
2022-01-27 20:54 ` [PATCH 06/11] mos6522: use device_class_set_parent_reset() to propagate reset to parent Mark Cave-Ayland
@ 2022-01-27 20:54 ` Mark Cave-Ayland
2022-02-07 19:32 ` Peter Maydell
2022-01-27 20:54 ` [PATCH 08/11] mos6522: add "info via" HMP command for debugging Mark Cave-Ayland
` (3 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:54 UTC (permalink / raw)
To: laurent, qemu-devel
This helps to follow how the guest is programming the mos6522 when debugging.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/mos6522.c | 10 ++++++++--
hw/misc/trace-events | 4 ++--
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 093cc83dcf..aaae195d63 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -36,6 +36,12 @@
#include "qemu/module.h"
#include "trace.h"
+
+static const char *mos6522_reg_names[16] = {
+ "ORB", "ORA", "DDRB", "DDRA", "T1CL", "T1CH", "T1LL", "T1LH",
+ "T2CL", "T2CH", "SR", "ACR", "PCR", "IFR", "IER", "ANH"
+};
+
/* XXX: implement all timer modes */
static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
@@ -310,7 +316,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
}
if (addr != VIA_REG_IFR || val != 0) {
- trace_mos6522_read(addr, val);
+ trace_mos6522_read(addr, mos6522_reg_names[addr], val);
}
return val;
@@ -321,7 +327,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
MOS6522State *s = opaque;
MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
- trace_mos6522_write(addr, val);
+ trace_mos6522_write(addr, mos6522_reg_names[addr], val);
switch (addr) {
case VIA_REG_B:
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 1c373dd0a4..c1ea57de31 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -95,8 +95,8 @@ imx7_gpr_write(uint64_t offset, uint64_t value) "addr 0x%08" PRIx64 "value 0x%08
mos6522_set_counter(int index, unsigned int val) "T%d.counter=%d"
mos6522_get_next_irq_time(uint16_t latch, int64_t d, int64_t delta) "latch=%d counter=0x%"PRId64 " delta_next=0x%"PRId64
mos6522_set_sr_int(void) "set sr_int"
-mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
-mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
+mos6522_write(uint64_t addr, const char *name, uint64_t val) "reg=0x%"PRIx64 " [%s] val=0x%"PRIx64
+mos6522_read(uint64_t addr, const char *name, unsigned val) "reg=0x%"PRIx64 " [%s] val=0x%x"
# npcm7xx_clk.c
npcm7xx_clk_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (6 preceding siblings ...)
2022-01-27 20:54 ` [PATCH 07/11] mos6522: add register names to register read/write trace events Mark Cave-Ayland
@ 2022-01-27 20:54 ` Mark Cave-Ayland
2022-02-07 19:34 ` Peter Maydell
2022-02-08 11:38 ` Daniel P. Berrangé
2022-01-27 20:54 ` [PATCH 09/11] mos6522: record last_irq_levels in mos6522_set_irq() Mark Cave-Ayland
` (2 subsequent siblings)
10 siblings, 2 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:54 UTC (permalink / raw)
To: laurent, qemu-devel
This displays detailed information about the device registers and timers to aid
debugging problems with timers and interrupts.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hmp-commands-info.hx | 12 ++++++
hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 104 insertions(+)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index e90f20a107..4e714e79a2 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -879,3 +879,15 @@ SRST
``info sgx``
Show intel SGX information.
ERST
+
+ {
+ .name = "via",
+ .args_type = "",
+ .params = "",
+ .help = "show guest 6522 VIA devices",
+ },
+
+SRST
+ ``info via``
+ Show guest 6522 VIA devices.
+ERST
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index aaae195d63..cfa6a9c44b 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -30,6 +30,8 @@
#include "hw/misc/mos6522.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
+#include "monitor/monitor.h"
+#include "qapi/type-helpers.h"
#include "qemu/timer.h"
#include "qemu/cutils.h"
#include "qemu/log.h"
@@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
}
}
+static int qmp_x_query_via_foreach(Object *obj, void *opaque)
+{
+ GString *buf = opaque;
+
+ if (object_dynamic_cast(obj, TYPE_MOS6522)) {
+ MOS6522State *s = MOS6522(obj);
+ int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ uint16_t t1counter = get_counter(s, &s->timers[0]);
+ uint16_t t2counter = get_counter(s, &s->timers[1]);
+
+ g_string_append_printf(buf, "%s:\n", object_get_typename(obj));
+
+ g_string_append_printf(buf, " Registers:\n");
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[0], s->b);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[1], s->a);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[2], s->dirb);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[3], s->dira);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[4], t1counter & 0xff);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[5], t1counter >> 8);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[6],
+ s->timers[0].latch & 0xff);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[7],
+ s->timers[0].latch >> 8);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[8], t2counter & 0xff);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[9], t2counter >> 8);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[10], s->sr);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[11], s->acr);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[12], s->pcr);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[13], s->ifr);
+ g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
+ mos6522_reg_names[14], s->ier);
+
+ g_string_append_printf(buf, " Timers:\n");
+ g_string_append_printf(buf, " Using current time now(ns)=%"PRId64
+ "\n", now);
+ g_string_append_printf(buf, " T1 freq(hz)=%"PRId64
+ " mode=%s"
+ " counter=0x%x"
+ " latch=0x%x\n"
+ " load_time(ns)=%"PRId64
+ " next_irq_time(ns)=%"PRId64 "\n",
+ s->timers[0].frequency,
+ ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous"
+ : "one-shot",
+ t1counter,
+ s->timers[0].latch,
+ s->timers[0].load_time,
+ get_next_irq_time(s, &s->timers[0], now));
+ g_string_append_printf(buf, " T2 freq(hz)=%"PRId64
+ " mode=%s"
+ " counter=0x%x"
+ " latch=0x%x\n"
+ " load_time(ns)=%"PRId64
+ " next_irq_time(ns)=%"PRId64 "\n",
+ s->timers[1].frequency,
+ "one-shot",
+ t2counter,
+ s->timers[1].latch,
+ s->timers[1].load_time,
+ get_next_irq_time(s, &s->timers[1], now));
+ }
+
+ return 0;
+}
+
+static HumanReadableText *qmp_x_query_via(Error **errp)
+{
+ g_autoptr(GString) buf = g_string_new("");
+
+ object_child_foreach_recursive(object_get_root(),
+ qmp_x_query_via_foreach, buf);
+
+ return human_readable_text_from_str(buf);
+}
+
static const MemoryRegionOps mos6522_ops = {
.read = mos6522_read,
.write = mos6522_write,
@@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
static void mos6522_register_types(void)
{
type_register_static(&mos6522_type_info);
+ monitor_register_hmp_info_hrt("via", qmp_x_query_via);
}
type_init(mos6522_register_types)
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-01-27 20:54 ` [PATCH 08/11] mos6522: add "info via" HMP command for debugging Mark Cave-Ayland
@ 2022-02-07 19:34 ` Peter Maydell
2022-02-08 5:14 ` Philippe Mathieu-Daudé via
2022-02-08 11:38 ` Daniel P. Berrangé
1 sibling, 1 reply; 45+ messages in thread
From: Peter Maydell @ 2022-02-07 19:34 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: Markus Armbruster, Laurent, qemu-devel
On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This displays detailed information about the device registers and timers to aid
> debugging problems with timers and interrupts.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hmp-commands-info.hx | 12 ++++++
> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 104 insertions(+)
I'm not sure how keen we are on adding new device-specific
HMP info commands, but it's not my area of expertise. Markus ?
(patch below for context)
thanks
-- PMM
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index e90f20a107..4e714e79a2 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -879,3 +879,15 @@ SRST
> ``info sgx``
> Show intel SGX information.
> ERST
> +
> + {
> + .name = "via",
> + .args_type = "",
> + .params = "",
> + .help = "show guest 6522 VIA devices",
> + },
> +
> +SRST
> + ``info via``
> + Show guest 6522 VIA devices.
> +ERST
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index aaae195d63..cfa6a9c44b 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -30,6 +30,8 @@
> #include "hw/misc/mos6522.h"
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> +#include "monitor/monitor.h"
> +#include "qapi/type-helpers.h"
> #include "qemu/timer.h"
> #include "qemu/cutils.h"
> #include "qemu/log.h"
> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> }
> }
>
> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
> +{
> + GString *buf = opaque;
> +
> + if (object_dynamic_cast(obj, TYPE_MOS6522)) {
> + MOS6522State *s = MOS6522(obj);
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + uint16_t t1counter = get_counter(s, &s->timers[0]);
> + uint16_t t2counter = get_counter(s, &s->timers[1]);
> +
> + g_string_append_printf(buf, "%s:\n", object_get_typename(obj));
> +
> + g_string_append_printf(buf, " Registers:\n");
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[0], s->b);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[1], s->a);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[2], s->dirb);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[3], s->dira);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[4], t1counter & 0xff);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[5], t1counter >> 8);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[6],
> + s->timers[0].latch & 0xff);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[7],
> + s->timers[0].latch >> 8);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[8], t2counter & 0xff);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[9], t2counter >> 8);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[10], s->sr);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[11], s->acr);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[12], s->pcr);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[13], s->ifr);
> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
> + mos6522_reg_names[14], s->ier);
> +
> + g_string_append_printf(buf, " Timers:\n");
> + g_string_append_printf(buf, " Using current time now(ns)=%"PRId64
> + "\n", now);
> + g_string_append_printf(buf, " T1 freq(hz)=%"PRId64
> + " mode=%s"
> + " counter=0x%x"
> + " latch=0x%x\n"
> + " load_time(ns)=%"PRId64
> + " next_irq_time(ns)=%"PRId64 "\n",
> + s->timers[0].frequency,
> + ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous"
> + : "one-shot",
> + t1counter,
> + s->timers[0].latch,
> + s->timers[0].load_time,
> + get_next_irq_time(s, &s->timers[0], now));
> + g_string_append_printf(buf, " T2 freq(hz)=%"PRId64
> + " mode=%s"
> + " counter=0x%x"
> + " latch=0x%x\n"
> + " load_time(ns)=%"PRId64
> + " next_irq_time(ns)=%"PRId64 "\n",
> + s->timers[1].frequency,
> + "one-shot",
> + t2counter,
> + s->timers[1].latch,
> + s->timers[1].load_time,
> + get_next_irq_time(s, &s->timers[1], now));
> + }
> +
> + return 0;
> +}
> +
> +static HumanReadableText *qmp_x_query_via(Error **errp)
> +{
> + g_autoptr(GString) buf = g_string_new("");
> +
> + object_child_foreach_recursive(object_get_root(),
> + qmp_x_query_via_foreach, buf);
> +
> + return human_readable_text_from_str(buf);
> +}
> +
> static const MemoryRegionOps mos6522_ops = {
> .read = mos6522_read,
> .write = mos6522_write,
> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
> static void mos6522_register_types(void)
> {
> type_register_static(&mos6522_type_info);
> + monitor_register_hmp_info_hrt("via", qmp_x_query_via);
> }
>
> type_init(mos6522_register_types)
> --
> 2.20.1
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-07 19:34 ` Peter Maydell
@ 2022-02-08 5:14 ` Philippe Mathieu-Daudé via
2022-02-08 8:10 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-08 5:14 UTC (permalink / raw)
To: Peter Maydell, Mark Cave-Ayland, Dr. David Alan Gilbert
Cc: Markus Armbruster, Laurent, qemu-devel
On 7/2/22 20:34, Peter Maydell wrote:
> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This displays detailed information about the device registers and timers to aid
>> debugging problems with timers and interrupts.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hmp-commands-info.hx | 12 ++++++
>> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 104 insertions(+)
>
>
> I'm not sure how keen we are on adding new device-specific
> HMP info commands, but it's not my area of expertise. Markus ?
HMP is David :) IIRC it is OK as long as HMP is a QMP wrapper.
>
> (patch below for context)
>
> thanks
> -- PMM
>
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index e90f20a107..4e714e79a2 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -879,3 +879,15 @@ SRST
>> ``info sgx``
>> Show intel SGX information.
>> ERST
>> +
>> + {
>> + .name = "via",
>> + .args_type = "",
>> + .params = "",
>> + .help = "show guest 6522 VIA devices",
>> + },
>> +
>> +SRST
>> + ``info via``
>> + Show guest 6522 VIA devices.
>> +ERST
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index aaae195d63..cfa6a9c44b 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -30,6 +30,8 @@
>> #include "hw/misc/mos6522.h"
>> #include "hw/qdev-properties.h"
>> #include "migration/vmstate.h"
>> +#include "monitor/monitor.h"
>> +#include "qapi/type-helpers.h"
>> #include "qemu/timer.h"
>> #include "qemu/cutils.h"
>> #include "qemu/log.h"
>> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> }
>> }
>>
>> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
>> +{
>> + GString *buf = opaque;
>> +
>> + if (object_dynamic_cast(obj, TYPE_MOS6522)) {
>> + MOS6522State *s = MOS6522(obj);
>> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> + uint16_t t1counter = get_counter(s, &s->timers[0]);
>> + uint16_t t2counter = get_counter(s, &s->timers[1]);
>> +
>> + g_string_append_printf(buf, "%s:\n", object_get_typename(obj));
>> +
>> + g_string_append_printf(buf, " Registers:\n");
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[0], s->b);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[1], s->a);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[2], s->dirb);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[3], s->dira);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[4], t1counter & 0xff);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[5], t1counter >> 8);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[6],
>> + s->timers[0].latch & 0xff);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[7],
>> + s->timers[0].latch >> 8);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[8], t2counter & 0xff);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[9], t2counter >> 8);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[10], s->sr);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[11], s->acr);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[12], s->pcr);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[13], s->ifr);
>> + g_string_append_printf(buf, " %-*s: 0x%x\n", 4,
>> + mos6522_reg_names[14], s->ier);
>> +
>> + g_string_append_printf(buf, " Timers:\n");
>> + g_string_append_printf(buf, " Using current time now(ns)=%"PRId64
>> + "\n", now);
>> + g_string_append_printf(buf, " T1 freq(hz)=%"PRId64
>> + " mode=%s"
>> + " counter=0x%x"
>> + " latch=0x%x\n"
>> + " load_time(ns)=%"PRId64
>> + " next_irq_time(ns)=%"PRId64 "\n",
>> + s->timers[0].frequency,
>> + ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous"
>> + : "one-shot",
>> + t1counter,
>> + s->timers[0].latch,
>> + s->timers[0].load_time,
>> + get_next_irq_time(s, &s->timers[0], now));
>> + g_string_append_printf(buf, " T2 freq(hz)=%"PRId64
>> + " mode=%s"
>> + " counter=0x%x"
>> + " latch=0x%x\n"
>> + " load_time(ns)=%"PRId64
>> + " next_irq_time(ns)=%"PRId64 "\n",
>> + s->timers[1].frequency,
>> + "one-shot",
>> + t2counter,
>> + s->timers[1].latch,
>> + s->timers[1].load_time,
>> + get_next_irq_time(s, &s->timers[1], now));
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static HumanReadableText *qmp_x_query_via(Error **errp)
>> +{
>> + g_autoptr(GString) buf = g_string_new("");
>> +
>> + object_child_foreach_recursive(object_get_root(),
>> + qmp_x_query_via_foreach, buf);
>> +
>> + return human_readable_text_from_str(buf);
>> +}
>> +
>> static const MemoryRegionOps mos6522_ops = {
>> .read = mos6522_read,
>> .write = mos6522_write,
>> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
>> static void mos6522_register_types(void)
>> {
>> type_register_static(&mos6522_type_info);
>> + monitor_register_hmp_info_hrt("via", qmp_x_query_via);
>> }
>>
>> type_init(mos6522_register_types)
>> --
>> 2.20.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 5:14 ` Philippe Mathieu-Daudé via
@ 2022-02-08 8:10 ` Markus Armbruster
2022-02-08 10:29 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2022-02-08 8:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Mark Cave-Ayland, qemu-devel,
Dr. David Alan Gilbert, Gerd Hoffmann, Laurent
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> On 7/2/22 20:34, Peter Maydell wrote:
>> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>
>>> This displays detailed information about the device registers and timers to aid
>>> debugging problems with timers and interrupts.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> hmp-commands-info.hx | 12 ++++++
>>> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 104 insertions(+)
>>
>> I'm not sure how keen we are on adding new device-specific
>> HMP info commands, but it's not my area of expertise. Markus ?
>
> HMP is David :)
Yes.
> IIRC it is OK as long as HMP is a QMP wrapper.
That's "how to do it", and I'll get back to it in a jiffie, but Peter
was wondering about the "whether to do it".
Most HMP commands are always there.
We have a few specific to compile-time configurable features: TCG, VNC,
Spice, Slirp, Linux. Does not apply here.
We have a few specific to targets, such as dump-skeys and info skeys for
s390. Target-specific is not quite the same as device-specific.
We have no device-specific commands so far. However, dump-skeys and
info skeys appear to be about the skeys *device*, not the s390 target.
Perhaps any s390 target has such a device? I don't know. My point is
we already have device-specific commands, they're just masquerading as
target-specific commands.
The proposed device-specific command uses a mechanism originally made
for modules instead (more on that below).
I think we should make up our minds which way we want device-specific
commands done, then do *all* of them that way.
On to "how to do it", part 1.
Most of the time, the command handler is declared with the command in
hmp-commands{,-info}.hx, possibly with compile-time conditionals.
But it can also be left null there, and set with monitor_register_hmp()
or monitor_register_hmp_info_hrt(). This is intended for modules; see
commit f0e48cbd791^..bca6eb34f03.
Aside: can modules be unloaded? If yes, we better zap the handler
then.
The proposed "info via" uses monitor_register_hmp_info_hrt(). No
objection from me, requires David's ACK.
"How to do it", part 2, in reply to Philippe's remark.
Ideally, HMP commands wrap around QMP commands, but we accept exceptions
for certain use cases where the wrapping is more trouble than it's
worth, with justification. I've explained this several times, and I'm
happy to dig up a reference or explain it again if there's a need.
Justifying an exception is bothersome, too. Daniel Berrangé recently
created a way to reduce the wrapping trouble (merge commit
e86e00a2493). The proposed "info via" makes use of it.
>> (patch below for context)
>> thanks
>> -- PMM
>>
>>>
>>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>>> index e90f20a107..4e714e79a2 100644
>>> --- a/hmp-commands-info.hx
>>> +++ b/hmp-commands-info.hx
>>> @@ -879,3 +879,15 @@ SRST
>>> ``info sgx``
>>> Show intel SGX information.
>>> ERST
>>> +
>>> + {
>>> + .name = "via",
>>> + .args_type = "",
>>> + .params = "",
>>> + .help = "show guest 6522 VIA devices",
>>> + },
>>> +
>>> +SRST
>>> + ``info via``
>>> + Show guest 6522 VIA devices.
>>> +ERST
Should this be conditional on the targets where we actually link the
device, like info skeys?
[...]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 8:10 ` Markus Armbruster
@ 2022-02-08 10:29 ` Dr. David Alan Gilbert
2022-02-08 11:52 ` Daniel P. Berrangé
2022-02-08 12:32 ` Mark Cave-Ayland
0 siblings, 2 replies; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2022-02-08 10:29 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Mark Cave-Ayland, qemu-devel,
Philippe Mathieu-Daudé,
Gerd Hoffmann, Laurent
* Markus Armbruster (armbru@redhat.com) wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
> > On 7/2/22 20:34, Peter Maydell wrote:
> >> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> >> <mark.cave-ayland@ilande.co.uk> wrote:
> >>>
> >>> This displays detailed information about the device registers and timers to aid
> >>> debugging problems with timers and interrupts.
> >>>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>> ---
> >>> hmp-commands-info.hx | 12 ++++++
> >>> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> >>> 2 files changed, 104 insertions(+)
> >>
> >> I'm not sure how keen we are on adding new device-specific
> >> HMP info commands, but it's not my area of expertise. Markus ?
> >
> > HMP is David :)
>
> Yes.
So let me start with an:
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
(If it's useful info for the author of the device, then I'm happy for
HMP to have that), but then - (moving the reply around a bit):
> Should this be conditional on the targets where we actually link the
> device, like info skeys?
>
Yes, I think so; it's a reasonably old/obscure device, there's no reason
everyone having it built in.
> > IIRC it is OK as long as HMP is a QMP wrapper.
>
> That's "how to do it", and I'll get back to it in a jiffie, but Peter
> was wondering about the "whether to do it".
>
> Most HMP commands are always there.
>
> We have a few specific to compile-time configurable features: TCG, VNC,
> Spice, Slirp, Linux. Does not apply here.
>
> We have a few specific to targets, such as dump-skeys and info skeys for
> s390. Target-specific is not quite the same as device-specific.
>
> We have no device-specific commands so far. However, dump-skeys and
> info skeys appear to be about the skeys *device*, not the s390 target.
> Perhaps any s390 target has such a device? I don't know. My point is
> we already have device-specific commands, they're just masquerading as
> target-specific commands.
Yeh we've got info lapic/ioapic as well.
> The proposed device-specific command uses a mechanism originally made
> for modules instead (more on that below).
>
> I think we should make up our minds which way we want device-specific
> commands done, then do *all* of them that way.
I think device specific commands make sense, but I think it would
probably be better if we had an 'info dev $name' and that a method on
the device rather than registering each one separately.
I'd assume that this would be a QMP level thing that got unwrapped at
HMP.
But that's not a problem for this contribution; someone else can figure
that out later.
Dave
>
> On to "how to do it", part 1.
>
> Most of the time, the command handler is declared with the command in
> hmp-commands{,-info}.hx, possibly with compile-time conditionals.
>
> But it can also be left null there, and set with monitor_register_hmp()
> or monitor_register_hmp_info_hrt(). This is intended for modules; see
> commit f0e48cbd791^..bca6eb34f03.
>
> Aside: can modules be unloaded? If yes, we better zap the handler
> then.
>
> The proposed "info via" uses monitor_register_hmp_info_hrt(). No
> objection from me, requires David's ACK.
>
>
> "How to do it", part 2, in reply to Philippe's remark.
>
> Ideally, HMP commands wrap around QMP commands, but we accept exceptions
> for certain use cases where the wrapping is more trouble than it's
> worth, with justification. I've explained this several times, and I'm
> happy to dig up a reference or explain it again if there's a need.
>
> Justifying an exception is bothersome, too. Daniel Berrangé recently
> created a way to reduce the wrapping trouble (merge commit
> e86e00a2493). The proposed "info via" makes use of it.
>
> >> (patch below for context)
> >> thanks
> >> -- PMM
> >>
> >>>
> >>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> >>> index e90f20a107..4e714e79a2 100644
> >>> --- a/hmp-commands-info.hx
> >>> +++ b/hmp-commands-info.hx
> >>> @@ -879,3 +879,15 @@ SRST
> >>> ``info sgx``
> >>> Show intel SGX information.
> >>> ERST
> >>> +
> >>> + {
> >>> + .name = "via",
> >>> + .args_type = "",
> >>> + .params = "",
> >>> + .help = "show guest 6522 VIA devices",
> >>> + },
> >>> +
> >>> +SRST
> >>> + ``info via``
> >>> + Show guest 6522 VIA devices.
> >>> +ERST
>
> [...]
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 10:29 ` Dr. David Alan Gilbert
@ 2022-02-08 11:52 ` Daniel P. Berrangé
2022-02-08 12:43 ` Mark Cave-Ayland
2022-02-08 15:13 ` Markus Armbruster
2022-02-08 12:32 ` Mark Cave-Ayland
1 sibling, 2 replies; 45+ messages in thread
From: Daniel P. Berrangé @ 2022-02-08 11:52 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Peter Maydell, qemu-devel, Mark Cave-Ayland,
Philippe Mathieu-Daudé,
Markus Armbruster, Gerd Hoffmann, Laurent
On Tue, Feb 08, 2022 at 10:29:09AM +0000, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> >
> > > On 7/2/22 20:34, Peter Maydell wrote:
> > >> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> > >> <mark.cave-ayland@ilande.co.uk> wrote:
> > >>>
> > >>> This displays detailed information about the device registers and timers to aid
> > >>> debugging problems with timers and interrupts.
> > >>>
> > >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > >>> ---
> > >>> hmp-commands-info.hx | 12 ++++++
> > >>> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> > >>> 2 files changed, 104 insertions(+)
> > >>
> > >> I'm not sure how keen we are on adding new device-specific
> > >> HMP info commands, but it's not my area of expertise. Markus ?
> > >
> > > HMP is David :)
> >
> > Yes.
>
> So let me start with an:
>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> (If it's useful info for the author of the device, then I'm happy for
> HMP to have that), but then - (moving the reply around a bit):
>
>
> > Should this be conditional on the targets where we actually link the
> > device, like info skeys?
> >
>
> Yes, I think so; it's a reasonably old/obscure device, there's no reason
> everyone having it built in.
>
> > > IIRC it is OK as long as HMP is a QMP wrapper.
> >
> > That's "how to do it", and I'll get back to it in a jiffie, but Peter
> > was wondering about the "whether to do it".
> >
> > Most HMP commands are always there.
> >
> > We have a few specific to compile-time configurable features: TCG, VNC,
> > Spice, Slirp, Linux. Does not apply here.
> >
> > We have a few specific to targets, such as dump-skeys and info skeys for
> > s390. Target-specific is not quite the same as device-specific.
> >
> > We have no device-specific commands so far. However, dump-skeys and
> > info skeys appear to be about the skeys *device*, not the s390 target.
> > Perhaps any s390 target has such a device? I don't know. My point is
> > we already have device-specific commands, they're just masquerading as
> > target-specific commands.
>
> Yeh we've got info lapic/ioapic as well.
>
> > The proposed device-specific command uses a mechanism originally made
> > for modules instead (more on that below).
> >
> > I think we should make up our minds which way we want device-specific
> > commands done, then do *all* of them that way.
>
> I think device specific commands make sense, but I think it would
> probably be better if we had an 'info dev $name' and that a method on
> the device rather than registering each one separately.
> I'd assume that this would be a QMP level thing that got unwrapped at
> HMP.
>
> But that's not a problem for this contribution; someone else can figure
> that out later.
Actually I think this would solve a problem with this contribution.
This patch implements a QMP command but never registers it, so it
isn't actually accessible via QMP. It only registers the HMP wrapper
which rather defeats the point of doing it via the QMP HumanReadableText
approach.
I'm guessing this was done because we don't have ability to dynamically
register QMP commands at runtime. I don't know how hard/easy it is
to address this, and perhaps we don't even want to. It was a problem
for me when previously converting HMP info commands to QMP and I
didn't get a solution, so didn't convert anything where the command
impl was dynamically registered at runtime. This basically excluded
converting devices that have been split into loadable modules.
If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug'
commands, then we could side-step the QMP limitation, as both thue
HMP and QMP comamnds could be statically registered. The devices
then only need to register a callback at runtime which should be
easier to deal with.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 11:52 ` Daniel P. Berrangé
@ 2022-02-08 12:43 ` Mark Cave-Ayland
2022-02-08 13:03 ` Dr. David Alan Gilbert
2022-02-08 15:13 ` Markus Armbruster
1 sibling, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-08 12:43 UTC (permalink / raw)
To: Daniel P. Berrangé, Dr. David Alan Gilbert
Cc: Peter Maydell, qemu-devel, Markus Armbruster, Laurent,
Philippe Mathieu-Daudé,
Gerd Hoffmann
On 08/02/2022 11:52, Daniel P. Berrangé wrote:
(cut)
>>> The proposed device-specific command uses a mechanism originally made
>>> for modules instead (more on that below).
>>>
>>> I think we should make up our minds which way we want device-specific
>>> commands done, then do *all* of them that way.
>>
>> I think device specific commands make sense, but I think it would
>> probably be better if we had an 'info dev $name' and that a method on
>> the device rather than registering each one separately.
>> I'd assume that this would be a QMP level thing that got unwrapped at
>> HMP.
>>
>> But that's not a problem for this contribution; someone else can figure
>> that out later.
>
> Actually I think this would solve a problem with this contribution.
> This patch implements a QMP command but never registers it, so it
> isn't actually accessible via QMP. It only registers the HMP wrapper
> which rather defeats the point of doing it via the QMP HumanReadableText
> approach.
>
> I'm guessing this was done because we don't have ability to dynamically
> register QMP commands at runtime. I don't know how hard/easy it is
> to address this, and perhaps we don't even want to. It was a problem
> for me when previously converting HMP info commands to QMP and I
> didn't get a solution, so didn't convert anything where the command
> impl was dynamically registered at runtime. This basically excluded
> converting devices that have been split into loadable modules.
>
> If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug'
> commands, then we could side-step the QMP limitation, as both thue
> HMP and QMP comamnds could be statically registered. The devices
> then only need to register a callback at runtime which should be
> easier to deal with.
That could work, or even just a "debug via" without using "info" for brevity.
You could even add the callback to DeviceClass so that the registration takes place
as part of class_init() using a function such as device_class_register_debug("name",
callback).
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 12:43 ` Mark Cave-Ayland
@ 2022-02-08 13:03 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2022-02-08 13:03 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Peter Maydell, Daniel P. Berrangé,
qemu-devel, Philippe Mathieu-Daudé,
Markus Armbruster, Gerd Hoffmann, Laurent
* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
> On 08/02/2022 11:52, Daniel P. Berrangé wrote:
>
> (cut)
>
> > > > The proposed device-specific command uses a mechanism originally made
> > > > for modules instead (more on that below).
> > > >
> > > > I think we should make up our minds which way we want device-specific
> > > > commands done, then do *all* of them that way.
> > >
> > > I think device specific commands make sense, but I think it would
> > > probably be better if we had an 'info dev $name' and that a method on
> > > the device rather than registering each one separately.
> > > I'd assume that this would be a QMP level thing that got unwrapped at
> > > HMP.
> > >
> > > But that's not a problem for this contribution; someone else can figure
> > > that out later.
> >
> > Actually I think this would solve a problem with this contribution.
> > This patch implements a QMP command but never registers it, so it
> > isn't actually accessible via QMP. It only registers the HMP wrapper
> > which rather defeats the point of doing it via the QMP HumanReadableText
> > approach.
> >
> > I'm guessing this was done because we don't have ability to dynamically
> > register QMP commands at runtime. I don't know how hard/easy it is
> > to address this, and perhaps we don't even want to. It was a problem
> > for me when previously converting HMP info commands to QMP and I
> > didn't get a solution, so didn't convert anything where the command
> > impl was dynamically registered at runtime. This basically excluded
> > converting devices that have been split into loadable modules.
> >
> > If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug'
> > commands, then we could side-step the QMP limitation, as both thue
> > HMP and QMP comamnds could be statically registered. The devices
> > then only need to register a callback at runtime which should be
> > easier to deal with.
>
> That could work, or even just a "debug via" without using "info" for brevity.
>
> You could even add the callback to DeviceClass so that the registration
> takes place as part of class_init() using a function such as
> device_class_register_debug("name", callback).
Yes, that was what I was imagining
Dave
>
> ATB,
>
> Mark.
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 11:52 ` Daniel P. Berrangé
2022-02-08 12:43 ` Mark Cave-Ayland
@ 2022-02-08 15:13 ` Markus Armbruster
1 sibling, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2022-02-08 15:13 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Peter Maydell, qemu-devel, Mark Cave-Ayland,
Philippe Mathieu-Daudé,
Dr. David Alan Gilbert, Gerd Hoffmann, Laurent
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Tue, Feb 08, 2022 at 10:29:09AM +0000, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>> > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> >
>> > > On 7/2/22 20:34, Peter Maydell wrote:
>> > >> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
>> > >> <mark.cave-ayland@ilande.co.uk> wrote:
>> > >>>
>> > >>> This displays detailed information about the device registers and timers to aid
>> > >>> debugging problems with timers and interrupts.
>> > >>>
>> > >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> > >>> ---
>> > >>> hmp-commands-info.hx | 12 ++++++
>> > >>> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
>> > >>> 2 files changed, 104 insertions(+)
>> > >>
>> > >> I'm not sure how keen we are on adding new device-specific
>> > >> HMP info commands, but it's not my area of expertise. Markus ?
>> > >
>> > > HMP is David :)
>> >
>> > Yes.
>>
>> So let me start with an:
>>
>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> (If it's useful info for the author of the device, then I'm happy for
>> HMP to have that), but then - (moving the reply around a bit):
>>
>>
>> > Should this be conditional on the targets where we actually link the
>> > device, like info skeys?
>> >
>>
>> Yes, I think so; it's a reasonably old/obscure device, there's no reason
>> everyone having it built in.
>>
>> > > IIRC it is OK as long as HMP is a QMP wrapper.
>> >
>> > That's "how to do it", and I'll get back to it in a jiffie, but Peter
>> > was wondering about the "whether to do it".
>> >
>> > Most HMP commands are always there.
>> >
>> > We have a few specific to compile-time configurable features: TCG, VNC,
>> > Spice, Slirp, Linux. Does not apply here.
>> >
>> > We have a few specific to targets, such as dump-skeys and info skeys for
>> > s390. Target-specific is not quite the same as device-specific.
>> >
>> > We have no device-specific commands so far. However, dump-skeys and
>> > info skeys appear to be about the skeys *device*, not the s390 target.
>> > Perhaps any s390 target has such a device? I don't know. My point is
>> > we already have device-specific commands, they're just masquerading as
>> > target-specific commands.
>>
>> Yeh we've got info lapic/ioapic as well.
>>
>> > The proposed device-specific command uses a mechanism originally made
>> > for modules instead (more on that below).
>> >
>> > I think we should make up our minds which way we want device-specific
>> > commands done, then do *all* of them that way.
>>
>> I think device specific commands make sense, but I think it would
>> probably be better if we had an 'info dev $name' and that a method on
>> the device rather than registering each one separately.
>> I'd assume that this would be a QMP level thing that got unwrapped at
>> HMP.
>>
>> But that's not a problem for this contribution; someone else can figure
>> that out later.
>
> Actually I think this would solve a problem with this contribution.
> This patch implements a QMP command but never registers it, so it
> isn't actually accessible via QMP. It only registers the HMP wrapper
> which rather defeats the point of doing it via the QMP HumanReadableText
> approach.
>
> I'm guessing this was done because we don't have ability to dynamically
> register QMP commands at runtime.
Correct. I pointed this out in review, actually:
Message-ID: <87tuk1k0de.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg01236.html
Gerd replied "when going forward with building more code modular we
might need this for qmp too at some point".
> I don't know how hard/easy it is
> to address this, and perhaps we don't even want to.
Depends on requirements.
Say all we want is "command fails unless the module is loaded" (or
whatever the condition is). Should be fairly easy.
But if we want introspection to reflect the state of things, that's
harder.
> It was a problem
> for me when previously converting HMP info commands to QMP and I
> didn't get a solution, so didn't convert anything where the command
> impl was dynamically registered at runtime. This basically excluded
> converting devices that have been split into loadable modules.
Understand; you series was big enough as it was.
> If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug'
> commands, then we could side-step the QMP limitation, as both thue
> HMP and QMP comamnds could be statically registered. The devices
> then only need to register a callback at runtime which should be
> easier to deal with.
I like it.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 10:29 ` Dr. David Alan Gilbert
2022-02-08 11:52 ` Daniel P. Berrangé
@ 2022-02-08 12:32 ` Mark Cave-Ayland
2022-02-08 13:04 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-08 12:32 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Markus Armbruster
Cc: qemu-devel, Peter Maydell, Gerd Hoffmann,
Philippe Mathieu-Daudé,
Laurent
On 08/02/2022 10:29, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>
>>> On 7/2/22 20:34, Peter Maydell wrote:
>>>> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
>>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>>
>>>>> This displays detailed information about the device registers and timers to aid
>>>>> debugging problems with timers and interrupts.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>> hmp-commands-info.hx | 12 ++++++
>>>>> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> 2 files changed, 104 insertions(+)
>>>>
>>>> I'm not sure how keen we are on adding new device-specific
>>>> HMP info commands, but it's not my area of expertise. Markus ?
>>>
>>> HMP is David :)
>>
>> Yes.
>
> So let me start with an:
>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> (If it's useful info for the author of the device, then I'm happy for
> HMP to have that), but then - (moving the reply around a bit):
>
>
>> Should this be conditional on the targets where we actually link the
>> device, like info skeys?
>>
>
> Yes, I think so; it's a reasonably old/obscure device, there's no reason
> everyone having it built in.
Unfortunately that doesn't seem to work: whilst the target CONFIG_* defines are
declared when processing hmp-commands-info.hx it seems the the device CONFIG_*
defines are missing?
>>> IIRC it is OK as long as HMP is a QMP wrapper.
>>
>> That's "how to do it", and I'll get back to it in a jiffie, but Peter
>> was wondering about the "whether to do it".
>>
>> Most HMP commands are always there.
>>
>> We have a few specific to compile-time configurable features: TCG, VNC,
>> Spice, Slirp, Linux. Does not apply here.
>>
>> We have a few specific to targets, such as dump-skeys and info skeys for
>> s390. Target-specific is not quite the same as device-specific.
>>
>> We have no device-specific commands so far. However, dump-skeys and
>> info skeys appear to be about the skeys *device*, not the s390 target.
>> Perhaps any s390 target has such a device? I don't know. My point is
>> we already have device-specific commands, they're just masquerading as
>> target-specific commands.
>
> Yeh we've got info lapic/ioapic as well.
... which I suspect is a workaround for only the target CONFIG_* defines being declared.
>> The proposed device-specific command uses a mechanism originally made
>> for modules instead (more on that below).
>>
>> I think we should make up our minds which way we want device-specific
>> commands done, then do *all* of them that way.
>
> I think device specific commands make sense, but I think it would
> probably be better if we had an 'info dev $name' and that a method on
> the device rather than registering each one separately.
> I'd assume that this would be a QMP level thing that got unwrapped at
> HMP.
>
> But that's not a problem for this contribution; someone else can figure
> that out later.
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 12:32 ` Mark Cave-Ayland
@ 2022-02-08 13:04 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2022-02-08 13:04 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Peter Maydell, Markus Armbruster, qemu-devel, Laurent,
Gerd Hoffmann, Philippe Mathieu-Daudé
* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
> On 08/02/2022 10:29, Dr. David Alan Gilbert wrote:
>
> > * Markus Armbruster (armbru@redhat.com) wrote:
> > > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > >
> > > > On 7/2/22 20:34, Peter Maydell wrote:
> > > > > On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> > > > > <mark.cave-ayland@ilande.co.uk> wrote:
> > > > > >
> > > > > > This displays detailed information about the device registers and timers to aid
> > > > > > debugging problems with timers and interrupts.
> > > > > >
> > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > > > ---
> > > > > > hmp-commands-info.hx | 12 ++++++
> > > > > > hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > 2 files changed, 104 insertions(+)
> > > > >
> > > > > I'm not sure how keen we are on adding new device-specific
> > > > > HMP info commands, but it's not my area of expertise. Markus ?
> > > >
> > > > HMP is David :)
> > >
> > > Yes.
> >
> > So let me start with an:
> >
> > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > (If it's useful info for the author of the device, then I'm happy for
> > HMP to have that), but then - (moving the reply around a bit):
> >
> >
> > > Should this be conditional on the targets where we actually link the
> > > device, like info skeys?
> > >
> >
> > Yes, I think so; it's a reasonably old/obscure device, there's no reason
> > everyone having it built in.
>
> Unfortunately that doesn't seem to work: whilst the target CONFIG_* defines
> are declared when processing hmp-commands-info.hx it seems the the device
> CONFIG_* defines are missing?
I'd be happy to take it down to the target level.
Dave
> > > > IIRC it is OK as long as HMP is a QMP wrapper.
> > >
> > > That's "how to do it", and I'll get back to it in a jiffie, but Peter
> > > was wondering about the "whether to do it".
> > >
> > > Most HMP commands are always there.
> > >
> > > We have a few specific to compile-time configurable features: TCG, VNC,
> > > Spice, Slirp, Linux. Does not apply here.
> > >
> > > We have a few specific to targets, such as dump-skeys and info skeys for
> > > s390. Target-specific is not quite the same as device-specific.
> > >
> > > We have no device-specific commands so far. However, dump-skeys and
> > > info skeys appear to be about the skeys *device*, not the s390 target.
> > > Perhaps any s390 target has such a device? I don't know. My point is
> > > we already have device-specific commands, they're just masquerading as
> > > target-specific commands.
> >
> > Yeh we've got info lapic/ioapic as well.
>
> ... which I suspect is a workaround for only the target CONFIG_* defines being declared.
>
> > > The proposed device-specific command uses a mechanism originally made
> > > for modules instead (more on that below).
> > >
> > > I think we should make up our minds which way we want device-specific
> > > commands done, then do *all* of them that way.
> >
> > I think device specific commands make sense, but I think it would
> > probably be better if we had an 'info dev $name' and that a method on
> > the device rather than registering each one separately.
> > I'd assume that this would be a QMP level thing that got unwrapped at
> > HMP.
> >
> > But that's not a problem for this contribution; someone else can figure
> > that out later.
>
>
> ATB,
>
> Mark.
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-01-27 20:54 ` [PATCH 08/11] mos6522: add "info via" HMP command for debugging Mark Cave-Ayland
2022-02-07 19:34 ` Peter Maydell
@ 2022-02-08 11:38 ` Daniel P. Berrangé
2022-02-08 12:39 ` Mark Cave-Ayland
1 sibling, 1 reply; 45+ messages in thread
From: Daniel P. Berrangé @ 2022-02-08 11:38 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: laurent, qemu-devel
On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote:
> This displays detailed information about the device registers and timers to aid
> debugging problems with timers and interrupts.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hmp-commands-info.hx | 12 ++++++
> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 104 insertions(+)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index e90f20a107..4e714e79a2 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -879,3 +879,15 @@ SRST
> ``info sgx``
> Show intel SGX information.
> ERST
> +
> + {
> + .name = "via",
> + .args_type = "",
> + .params = "",
> + .help = "show guest 6522 VIA devices",
> + },
> +
> +SRST
> + ``info via``
> + Show guest 6522 VIA devices.
> +ERST
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index aaae195d63..cfa6a9c44b 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -30,6 +30,8 @@
> #include "hw/misc/mos6522.h"
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> +#include "monitor/monitor.h"
> +#include "qapi/type-helpers.h"
> #include "qemu/timer.h"
> #include "qemu/cutils.h"
> #include "qemu/log.h"
> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> }
> }
>
> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
> +
> +static HumanReadableText *qmp_x_query_via(Error **errp)
> +{
> + g_autoptr(GString) buf = g_string_new("");
> +
> + object_child_foreach_recursive(object_get_root(),
> + qmp_x_query_via_foreach, buf);
> +
> + return human_readable_text_from_str(buf);
> +}
This provides a code handler for a QMP command which is good,
but doesn't ever define the QMP command in the qapi/ schema.
> static const MemoryRegionOps mos6522_ops = {
> .read = mos6522_read,
> .write = mos6522_write,
> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
> static void mos6522_register_types(void)
> {
> type_register_static(&mos6522_type_info);
> + monitor_register_hmp_info_hrt("via", qmp_x_query_via);
This only registers the HMP counterpart.
The idea of the HumanReadableText handler is that it is calling
a QMP command that is exposed to apps.
> }
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 11:38 ` Daniel P. Berrangé
@ 2022-02-08 12:39 ` Mark Cave-Ayland
2022-02-08 12:49 ` Daniel P. Berrangé
0 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-08 12:39 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: laurent, qemu-devel
On 08/02/2022 11:38, Daniel P. Berrangé wrote:
> On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote:
>> This displays detailed information about the device registers and timers to aid
>> debugging problems with timers and interrupts.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hmp-commands-info.hx | 12 ++++++
>> hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 104 insertions(+)
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index e90f20a107..4e714e79a2 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -879,3 +879,15 @@ SRST
>> ``info sgx``
>> Show intel SGX information.
>> ERST
>> +
>> + {
>> + .name = "via",
>> + .args_type = "",
>> + .params = "",
>> + .help = "show guest 6522 VIA devices",
>> + },
>> +
>> +SRST
>> + ``info via``
>> + Show guest 6522 VIA devices.
>> +ERST
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index aaae195d63..cfa6a9c44b 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -30,6 +30,8 @@
>> #include "hw/misc/mos6522.h"
>> #include "hw/qdev-properties.h"
>> #include "migration/vmstate.h"
>> +#include "monitor/monitor.h"
>> +#include "qapi/type-helpers.h"
>> #include "qemu/timer.h"
>> #include "qemu/cutils.h"
>> #include "qemu/log.h"
>> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>> }
>> }
>>
>> +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
>
>
>> +
>> +static HumanReadableText *qmp_x_query_via(Error **errp)
>> +{
>> + g_autoptr(GString) buf = g_string_new("");
>> +
>> + object_child_foreach_recursive(object_get_root(),
>> + qmp_x_query_via_foreach, buf);
>> +
>> + return human_readable_text_from_str(buf);
>> +}
>
> This provides a code handler for a QMP command which is good,
> but doesn't ever define the QMP command in the qapi/ schema.
First of all, thank you for writing the docs at
https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text
which were really useful when writing this patch.
I was under the impression that monitor_register_hmp_info_hrt() does all the magic
here i.e. it declares the underlying QMP command with an x- prefix and effectively
encapsulates the text field in a way that says "this is an unreliable text opaque for
humans"?
If a qapi/ schema is needed could you explain what it should look like for this
example and where it should go? Looking at the existing .json files I can't
immediately see one which is the right place for this to live.
>> static const MemoryRegionOps mos6522_ops = {
>> .read = mos6522_read,
>> .write = mos6522_write,
>> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
>> static void mos6522_register_types(void)
>> {
>> type_register_static(&mos6522_type_info);
>> + monitor_register_hmp_info_hrt("via", qmp_x_query_via);
>
> This only registers the HMP counterpart.
>
> The idea of the HumanReadableText handler is that it is calling
> a QMP command that is exposed to apps.
>
>> }
>
> Regards,
> Daniel
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 12:39 ` Mark Cave-Ayland
@ 2022-02-08 12:49 ` Daniel P. Berrangé
2022-02-08 13:06 ` Mark Cave-Ayland
0 siblings, 1 reply; 45+ messages in thread
From: Daniel P. Berrangé @ 2022-02-08 12:49 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: laurent, qemu-devel
On Tue, Feb 08, 2022 at 12:39:04PM +0000, Mark Cave-Ayland wrote:
> On 08/02/2022 11:38, Daniel P. Berrangé wrote:
>
> > On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote:
> > > This displays detailed information about the device registers and timers o aid
> > > debugging problems with timers and interrupts.
> > >
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > > hmp-commands-info.hx | 12 ++++++
> > > hw/misc/mos6522.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 104 insertions(+)
> > >
> > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > index e90f20a107..4e714e79a2 100644
> > > --- a/hmp-commands-info.hx
> > > +++ b/hmp-commands-info.hx
> > > @@ -879,3 +879,15 @@ SRST
> > > ``info sgx``
> > > Show intel SGX information.
> > > ERST
> > > +
> > > + {
> > > + .name = "via",
> > > + .args_type = "",
> > > + .params = "",
> > > + .help = "show guest 6522 VIA devices",
> > > + },
> > > +
> > > +SRST
> > > + ``info via``
> > > + Show guest 6522 VIA devices.
> > > +ERST
> > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > > index aaae195d63..cfa6a9c44b 100644
> > > --- a/hw/misc/mos6522.c
> > > +++ b/hw/misc/mos6522.c
> > > @@ -30,6 +30,8 @@
> > > #include "hw/misc/mos6522.h"
> > > #include "hw/qdev-properties.h"
> > > #include "migration/vmstate.h"
> > > +#include "monitor/monitor.h"
> > > +#include "qapi/type-helpers.h"
> > > #include "qemu/timer.h"
> > > #include "qemu/cutils.h"
> > > #include "qemu/log.h"
> > > @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> > > }
> > > }
> > > +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
> >
> >
> > > +
> > > +static HumanReadableText *qmp_x_query_via(Error **errp)
> > > +{
> > > + g_autoptr(GString) buf = g_string_new("");
> > > +
> > > + object_child_foreach_recursive(object_get_root(),
> > > + qmp_x_query_via_foreach, buf);
> > > +
> > > + return human_readable_text_from_str(buf);
> > > +}
> >
> > This provides a code handler for a QMP command which is good,
> > but doesn't ever define the QMP command in the qapi/ schema.
>
> First of all, thank you for writing the docs at https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text
> which were really useful when writing this patch.
>
> I was under the impression that monitor_register_hmp_info_hrt() does all the
> magic here i.e. it declares the underlying QMP command with an x- prefix and
> effectively encapsulates the text field in a way that says "this is an
> unreliable text opaque for humans"?
The monitor_register_hmp_info_hrt only does the HMP glue side, and
that's only needed if you must dynamically register the HMP command.
For statically registered commands set '.cmd_info_hrt' directly in
the hml-commands-info.hx for the HMP side.
> If a qapi/ schema is needed could you explain what it should look like for
> this example and where it should go? Looking at the existing .json files I
> can't immediately see one which is the right place for this to live.
Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
there. The QAPI bit is fairly simple.
if you want to see an illustration of what's different from a previous
pure HMP impl, look at:
commit dd98234c059e6bdb05a52998270df6d3d990332e
Author: Daniel P. Berrangé <berrange@redhat.com>
Date: Wed Sep 8 10:35:43 2021 +0100
qapi: introduce x-query-roms QMP command
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 12:49 ` Daniel P. Berrangé
@ 2022-02-08 13:06 ` Mark Cave-Ayland
2022-02-08 13:10 ` Daniel P. Berrangé
0 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-08 13:06 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: laurent, qemu-devel
On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>> effectively encapsulates the text field in a way that says "this is an
>> unreliable text opaque for humans"?
>
> The monitor_register_hmp_info_hrt only does the HMP glue side, and
> that's only needed if you must dynamically register the HMP command.
> For statically registered commands set '.cmd_info_hrt' directly in
> the hml-commands-info.hx for the HMP side.
>
>> If a qapi/ schema is needed could you explain what it should look like for
>> this example and where it should go? Looking at the existing .json files I
>> can't immediately see one which is the right place for this to live.
>
> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> there. The QAPI bit is fairly simple.
>
> if you want to see an illustration of what's different from a previous
> pure HMP impl, look at:
>
> commit dd98234c059e6bdb05a52998270df6d3d990332e
> Author: Daniel P. Berrangé <berrange@redhat.com>
> Date: Wed Sep 8 10:35:43 2021 +0100
>
> qapi: introduce x-query-roms QMP command
I see, thanks for the reference. So qapi/machine.json would be the right place to
declare the QMP part even for a specific device?
Even this approach still wouldn't work in its current form though, since as mentioned
in my previous email it seems that only the target CONFIG_* defines and not the
device CONFIG_* defines are present when processing hmp-commands-info.hx.
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 13:06 ` Mark Cave-Ayland
@ 2022-02-08 13:10 ` Daniel P. Berrangé
2022-02-20 17:18 ` Mark Cave-Ayland
0 siblings, 1 reply; 45+ messages in thread
From: Daniel P. Berrangé @ 2022-02-08 13:10 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: laurent, qemu-devel
On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>
> > > I was under the impression that monitor_register_hmp_info_hrt() does all the
> > > magic here i.e. it declares the underlying QMP command with an x- prefix and
> > > effectively encapsulates the text field in a way that says "this is an
> > > unreliable text opaque for humans"?
> >
> > The monitor_register_hmp_info_hrt only does the HMP glue side, and
> > that's only needed if you must dynamically register the HMP command.
> > For statically registered commands set '.cmd_info_hrt' directly in
> > the hml-commands-info.hx for the HMP side.
> >
> > > If a qapi/ schema is needed could you explain what it should look like for
> > > this example and where it should go? Looking at the existing .json files I
> > > can't immediately see one which is the right place for this to live.
> >
> > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> > there. The QAPI bit is fairly simple.
> >
> > if you want to see an illustration of what's different from a previous
> > pure HMP impl, look at:
> >
> > commit dd98234c059e6bdb05a52998270df6d3d990332e
> > Author: Daniel P. Berrangé <berrange@redhat.com>
> > Date: Wed Sep 8 10:35:43 2021 +0100
> >
> > qapi: introduce x-query-roms QMP command
>
> I see, thanks for the reference. So qapi/machine.json would be the right
> place to declare the QMP part even for a specific device?
>
> Even this approach still wouldn't work in its current form though, since as
> mentioned in my previous email it seems that only the target CONFIG_*
> defines and not the device CONFIG_* defines are present when processing
> hmp-commands-info.hx.
Yeah, that's where the pain comes in. While QAPI schema can be made
conditional on a few CONFIG_* parameters - basically those derived
from global configure time options, it is impossible for this to be
with with target specific options like the device CONFIG_* defines.
This is why I suggested in my othuer reply that it would need to be
done with a generic 'info dev-debug' / 'x-query-dev-debug' command
that can be registered unconditionally, and then individual devices
plug into it.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-08 13:10 ` Daniel P. Berrangé
@ 2022-02-20 17:18 ` Mark Cave-Ayland
2022-02-21 12:20 ` Philippe Mathieu-Daudé
2022-02-21 17:11 ` Daniel P. Berrangé
0 siblings, 2 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-20 17:18 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: laurent, Dr. David Alan Gilbert (git), qemu-devel
On 08/02/2022 13:10, Daniel P. Berrangé wrote:
> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>
>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>> effectively encapsulates the text field in a way that says "this is an
>>>> unreliable text opaque for humans"?
>>>
>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>> that's only needed if you must dynamically register the HMP command.
>>> For statically registered commands set '.cmd_info_hrt' directly in
>>> the hml-commands-info.hx for the HMP side.
>>>
>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>> this example and where it should go? Looking at the existing .json files I
>>>> can't immediately see one which is the right place for this to live.
>>>
>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>> there. The QAPI bit is fairly simple.
>>>
>>> if you want to see an illustration of what's different from a previous
>>> pure HMP impl, look at:
>>>
>>> commit dd98234c059e6bdb05a52998270df6d3d990332e
>>> Author: Daniel P. Berrangé <berrange@redhat.com>
>>> Date: Wed Sep 8 10:35:43 2021 +0100
>>>
>>> qapi: introduce x-query-roms QMP command
>>
>> I see, thanks for the reference. So qapi/machine.json would be the right
>> place to declare the QMP part even for a specific device?
>>
>> Even this approach still wouldn't work in its current form though, since as
>> mentioned in my previous email it seems that only the target CONFIG_*
>> defines and not the device CONFIG_* defines are present when processing
>> hmp-commands-info.hx.
>
> Yeah, that's where the pain comes in. While QAPI schema can be made
> conditional on a few CONFIG_* parameters - basically those derived
> from global configure time options, it is impossible for this to be
> with with target specific options like the device CONFIG_* defines.
>
> This is why I suggested in my othuer reply that it would need to be
> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
> that can be registered unconditionally, and then individual devices
> plug into it.
After some more experiments this afternoon I still seem to be falling through the
gaps on this one. This is based upon my understanding that all new HMP commands
should use a QMP HumanReadableText implementation and the new command should be
restricted according to target.
Currently I am working with this change to hmp-commands-info.hx and
qapi/misc-target.json:
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 7e6bd30395..aac86d5473 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -880,15 +880,15 @@ SRST
Show intel SGX information.
ERST
#if defined(TARGET_M68K) || defined(TARGET_PPC)
{
.name = "via",
.args_type = "",
.params = "",
.help = "show guest mos6522 VIA devices",
.cmd_info_hrt = qmp_x_query_via,
},
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4bc45d2474..72bf71888e 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -2,6 +2,8 @@
# vim: filetype=python
#
+{ 'include': 'common.json' }
+
##
# @RTC_CHANGE:
#
@@ -424,3 +426,19 @@
#
##
{ 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
+
+##
+# @x-query-via:
+#
+# Query information on the registered mos6522 VIAs
+#
+# Features:
+# @unstable: This command is meant for debugging.
+#
+# Returns: internal mos6522 information
+#
+# Since: 7.0
+##
+{ 'command': 'x-query-via',
+ 'returns': 'HumanReadableText',
+ 'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } }
The main problem with trying to restrict the new command to a target (or targets) is
that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be
included in a device header such as mos6522.h without getting poison errors like
below (which does actually make sense):
In file included from ./qapi/qapi-commands-misc-target.h:17,
from /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
from ../hw/misc/mos6522.c:30:
./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned "TARGET_ALPHA"
I can work around that by declaring the prototype for qmp_x_query_via() manually i.e.:
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 9c21da2ddd..9677293ad0 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -30,7 +30,7 @@
#include "exec/memory.h"
#include "hw/sysbus.h"
#include "hw/input/adb.h"
+#include "qapi/qapi-commands-common.h"
#include "qom/object.h"
/* Bits in ACR */
@@ -156,4 +156,6 @@ extern const VMStateDescription vmstate_mos6522;
uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);
+HumanReadableText *qmp_x_query_via(Error **errp);
+
#endif /* MOS6522_H */
This works fine for targets where CONFIG_MOS6522 is defined, but if I try a target
such as x86_64 where the device isn't used then I hit another compilation error:
qapi/qapi-commands-misc-target.c:598:13: error:
‘qmp_marshal_output_HumanReadableText’ defined but not used [-Werror=unused-function]
static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Looking at the generated qapi/qapi-commands-misc-target.c file in question I see this:
static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
QObject **ret_out, Error **errp)
{
Visitor *v;
v = qobject_output_visitor_new_qmp(ret_out);
if (visit_type_HumanReadableText(v, "unused", &ret_in, errp)) {
visit_complete(v, ret_out);
}
visit_free(v);
v = qapi_dealloc_visitor_new();
visit_type_HumanReadableText(v, "unused", &ret_in, NULL);
visit_free(v);
}
#if defined(TARGET_M68K) || defined(TARGET_PPC)
void qmp_marshal_x_query_via(QDict *args, QObject **ret, Error **errp)
{
...
...
}
#endif
i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if TARGET guards
and since HumanReadableText is only used by the new qmp_x_query_via() functionality
then the compiler complains and aborts the compilation.
Possibly this is an error in the QAPI generator for types hidden behind commands
using "if"? Otherwise I'm not sure what is the best way to proceed, so I'd be
grateful for some further pointers.
ATB,
Mark.
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-20 17:18 ` Mark Cave-Ayland
@ 2022-02-21 12:20 ` Philippe Mathieu-Daudé
2022-02-21 22:27 ` Mark Cave-Ayland
2022-02-21 17:11 ` Daniel P. Berrangé
1 sibling, 1 reply; 45+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-02-21 12:20 UTC (permalink / raw)
To: Mark Cave-Ayland, Daniel P. Berrangé, Thomas Huth
Cc: qemu-devel, laurent, Dr. David Alan Gilbert (git)
+Thomas
On 20/2/22 18:18, Mark Cave-Ayland wrote:
> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>
>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>
>>>>> I was under the impression that monitor_register_hmp_info_hrt()
>>>>> does all the
>>>>> magic here i.e. it declares the underlying QMP command with an x-
>>>>> prefix and
>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>> unreliable text opaque for humans"?
>>>>
>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>> that's only needed if you must dynamically register the HMP command.
>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>> the hml-commands-info.hx for the HMP side.
>>>>
>>>>> If a qapi/ schema is needed could you explain what it should look
>>>>> like for
>>>>> this example and where it should go? Looking at the existing .json
>>>>> files I
>>>>> can't immediately see one which is the right place for this to live.
>>>>
>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>> there. The QAPI bit is fairly simple.
>>>>
>>>> if you want to see an illustration of what's different from a previous
>>>> pure HMP impl, look at:
>>>>
>>>> commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>> Author: Daniel P. Berrangé <berrange@redhat.com>
>>>> Date: Wed Sep 8 10:35:43 2021 +0100
>>>>
>>>> qapi: introduce x-query-roms QMP command
>>>
>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>> place to declare the QMP part even for a specific device?
>>>
>>> Even this approach still wouldn't work in its current form though,
>>> since as
>>> mentioned in my previous email it seems that only the target CONFIG_*
>>> defines and not the device CONFIG_* defines are present when processing
>>> hmp-commands-info.hx.
>>
>> Yeah, that's where the pain comes in. While QAPI schema can be made
>> conditional on a few CONFIG_* parameters - basically those derived
>> from global configure time options, it is impossible for this to be
>> with with target specific options like the device CONFIG_* defines.
>>
>> This is why I suggested in my othuer reply that it would need to be
>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>> that can be registered unconditionally, and then individual devices
>> plug into it.
>
> After some more experiments this afternoon I still seem to be falling
> through the gaps on this one. This is based upon my understanding that
> all new HMP commands should use a QMP HumanReadableText implementation
> and the new command should be restricted according to target.
>
> Currently I am working with this change to hmp-commands-info.hx and
> qapi/misc-target.json:
>
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 7e6bd30395..aac86d5473 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -880,15 +880,15 @@ SRST
> Show intel SGX information.
> ERST
>
> #if defined(TARGET_M68K) || defined(TARGET_PPC)
> {
> .name = "via",
> .args_type = "",
> .params = "",
> .help = "show guest mos6522 VIA devices",
> .cmd_info_hrt = qmp_x_query_via,
> },
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 4bc45d2474..72bf71888e 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -2,6 +2,8 @@
> # vim: filetype=python
> #
>
> +{ 'include': 'common.json' }
> +
> ##
> # @RTC_CHANGE:
> #
> @@ -424,3 +426,19 @@
> #
> ##
> { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if':
> 'TARGET_I386' }
> +
> +##
> +# @x-query-via:
> +#
> +# Query information on the registered mos6522 VIAs
> +#
> +# Features:
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: internal mos6522 information
> +#
> +# Since: 7.0
> +##
> +{ 'command': 'x-query-via',
> + 'returns': 'HumanReadableText',
> + 'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K',
> 'TARGET_PPC' ] } }
>
>
> The main problem with trying to restrict the new command to a target (or
> targets) is that the autogenerated qapi/qapi-commands-misc-target.h QAPI
> header cannot be included in a device header such as mos6522.h without
> getting poison errors like below (which does actually make sense):
>
>
> In file included from ./qapi/qapi-commands-misc-target.h:17,
> from
> /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
> from ../hw/misc/mos6522.c:30:
> ./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned
> "TARGET_ALPHA"
Can be kludged by making this device target-specific:
-- >8 --
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 6dcbe044f3..65837b1dfe 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true:
files('armv7m_ras.c'))
-softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
+specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
---
I'd rather keep devices generic, but sometime we can't. In this case
VIA is only used on m68k so it could be acceptable.
> I can work around that by declaring the prototype for qmp_x_query_via()
> manually i.e.:
>
>
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index 9c21da2ddd..9677293ad0 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -30,7 +30,7 @@
> #include "exec/memory.h"
> #include "hw/sysbus.h"
> #include "hw/input/adb.h"
> +#include "qapi/qapi-commands-common.h"
> #include "qom/object.h"
>
> /* Bits in ACR */
> @@ -156,4 +156,6 @@ extern const VMStateDescription vmstate_mos6522;
> uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
> void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned
> size);
>
> +HumanReadableText *qmp_x_query_via(Error **errp);
> +
> #endif /* MOS6522_H */
>
>
> This works fine for targets where CONFIG_MOS6522 is defined, but if I
> try a target such as x86_64 where the device isn't used then I hit
> another compilation error:
>
>
> qapi/qapi-commands-misc-target.c:598:13: error:
> ‘qmp_marshal_output_HumanReadableText’ defined but not used
> [-Werror=unused-function]
> static void qmp_marshal_output_HumanReadableText(HumanReadableText
> *ret_in,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
>
> Looking at the generated qapi/qapi-commands-misc-target.c file in
> question I see this:
>
>
> static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in,
> QObject **ret_out, Error **errp)
> {
> Visitor *v;
>
> v = qobject_output_visitor_new_qmp(ret_out);
> if (visit_type_HumanReadableText(v, "unused", &ret_in, errp)) {
> visit_complete(v, ret_out);
> }
> visit_free(v);
> v = qapi_dealloc_visitor_new();
> visit_type_HumanReadableText(v, "unused", &ret_in, NULL);
> visit_free(v);
> }
>
> #if defined(TARGET_M68K) || defined(TARGET_PPC)
> void qmp_marshal_x_query_via(QDict *args, QObject **ret, Error **errp)
> {
> ...
> ...
> }
> #endif
>
> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
> TARGET guards and since HumanReadableText is only used by the new
> qmp_x_query_via() functionality then the compiler complains and aborts
> the compilation.
>
> Possibly this is an error in the QAPI generator for types hidden behind
> commands using "if"? Otherwise I'm not sure what is the best way to
> proceed, so I'd be grateful for some further pointers.
>
>
> ATB,
>
> Mark.
>
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-21 12:20 ` Philippe Mathieu-Daudé
@ 2022-02-21 22:27 ` Mark Cave-Ayland
0 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-21 22:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Daniel P. Berrangé, Thomas Huth
Cc: qemu-devel, laurent, Dr. David Alan Gilbert (git)
On 21/02/2022 12:20, Philippe Mathieu-Daudé wrote:
> +Thomas
>
> On 20/2/22 18:18, Mark Cave-Ayland wrote:
>> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>>
>>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>>
>>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>>> unreliable text opaque for humans"?
>>>>>
>>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>>> that's only needed if you must dynamically register the HMP command.
>>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>>> the hml-commands-info.hx for the HMP side.
>>>>>
>>>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>>>> this example and where it should go? Looking at the existing .json files I
>>>>>> can't immediately see one which is the right place for this to live.
>>>>>
>>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>>> there. The QAPI bit is fairly simple.
>>>>>
>>>>> if you want to see an illustration of what's different from a previous
>>>>> pure HMP impl, look at:
>>>>>
>>>>> commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>>> Author: Daniel P. Berrangé <berrange@redhat.com>
>>>>> Date: Wed Sep 8 10:35:43 2021 +0100
>>>>>
>>>>> qapi: introduce x-query-roms QMP command
>>>>
>>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>>> place to declare the QMP part even for a specific device?
>>>>
>>>> Even this approach still wouldn't work in its current form though, since as
>>>> mentioned in my previous email it seems that only the target CONFIG_*
>>>> defines and not the device CONFIG_* defines are present when processing
>>>> hmp-commands-info.hx.
>>>
>>> Yeah, that's where the pain comes in. While QAPI schema can be made
>>> conditional on a few CONFIG_* parameters - basically those derived
>>> from global configure time options, it is impossible for this to be
>>> with with target specific options like the device CONFIG_* defines.
>>>
>>> This is why I suggested in my othuer reply that it would need to be
>>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>>> that can be registered unconditionally, and then individual devices
>>> plug into it.
>>
>> After some more experiments this afternoon I still seem to be falling through the
>> gaps on this one. This is based upon my understanding that all new HMP commands
>> should use a QMP HumanReadableText implementation and the new command should be
>> restricted according to target.
>>
>> Currently I am working with this change to hmp-commands-info.hx and
>> qapi/misc-target.json:
>>
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index 7e6bd30395..aac86d5473 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -880,15 +880,15 @@ SRST
>> Show intel SGX information.
>> ERST
>>
>> #if defined(TARGET_M68K) || defined(TARGET_PPC)
>> {
>> .name = "via",
>> .args_type = "",
>> .params = "",
>> .help = "show guest mos6522 VIA devices",
>> .cmd_info_hrt = qmp_x_query_via,
>> },
>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>> index 4bc45d2474..72bf71888e 100644
>> --- a/qapi/misc-target.json
>> +++ b/qapi/misc-target.json
>> @@ -2,6 +2,8 @@
>> # vim: filetype=python
>> #
>>
>> +{ 'include': 'common.json' }
>> +
>> ##
>> # @RTC_CHANGE:
>> #
>> @@ -424,3 +426,19 @@
>> #
>> ##
>> { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
>> +
>> +##
>> +# @x-query-via:
>> +#
>> +# Query information on the registered mos6522 VIAs
>> +#
>> +# Features:
>> +# @unstable: This command is meant for debugging.
>> +#
>> +# Returns: internal mos6522 information
>> +#
>> +# Since: 7.0
>> +##
>> +{ 'command': 'x-query-via',
>> + 'returns': 'HumanReadableText',
>> + 'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } }
>>
>>
>> The main problem with trying to restrict the new command to a target (or targets)
>> is that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be
>> included in a device header such as mos6522.h without getting poison errors like
>> below (which does actually make sense):
>>
>>
>> In file included from ./qapi/qapi-commands-misc-target.h:17,
>> from /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35,
>> from ../hw/misc/mos6522.c:30:
>> ./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned "TARGET_ALPHA"
>
> Can be kludged by making this device target-specific:
>
> -- >8 --
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 6dcbe044f3..65837b1dfe 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_ras.c'))
> -softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
> +specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
> ---
>
> I'd rather keep devices generic, but sometime we can't. In this case
> VIA is only used on m68k so it could be acceptable.
That's not quite true though, it's also used for the Mac machines. From what I can
see everyone agrees that there needs to be a better way to do this, but it requires
someone with time and interest to implement the suggested changes.
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-20 17:18 ` Mark Cave-Ayland
2022-02-21 12:20 ` Philippe Mathieu-Daudé
@ 2022-02-21 17:11 ` Daniel P. Berrangé
2022-02-21 22:29 ` Mark Cave-Ayland
1 sibling, 1 reply; 45+ messages in thread
From: Daniel P. Berrangé @ 2022-02-21 17:11 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: laurent, Dr. David Alan Gilbert (git), qemu-devel
On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>
> > On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
> > > On 08/02/2022 12:49, Daniel P. Berrangé wrote:
> > >
> > > > > I was under the impression that monitor_register_hmp_info_hrt() does all the
> > > > > magic here i.e. it declares the underlying QMP command with an x- prefix and
> > > > > effectively encapsulates the text field in a way that says "this is an
> > > > > unreliable text opaque for humans"?
> > > >
> > > > The monitor_register_hmp_info_hrt only does the HMP glue side, and
> > > > that's only needed if you must dynamically register the HMP command.
> > > > For statically registered commands set '.cmd_info_hrt' directly in
> > > > the hml-commands-info.hx for the HMP side.
> > > >
> > > > > If a qapi/ schema is needed could you explain what it should look like for
> > > > > this example and where it should go? Looking at the existing .json files I
> > > > > can't immediately see one which is the right place for this to live.
> > > >
> > > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> > > > there. The QAPI bit is fairly simple.
> > > >
> > > > if you want to see an illustration of what's different from a previous
> > > > pure HMP impl, look at:
> > > >
> > > > commit dd98234c059e6bdb05a52998270df6d3d990332e
> > > > Author: Daniel P. Berrangé <berrange@redhat.com>
> > > > Date: Wed Sep 8 10:35:43 2021 +0100
> > > >
> > > > qapi: introduce x-query-roms QMP command
> > >
> > > I see, thanks for the reference. So qapi/machine.json would be the right
> > > place to declare the QMP part even for a specific device?
> > >
> > > Even this approach still wouldn't work in its current form though, since as
> > > mentioned in my previous email it seems that only the target CONFIG_*
> > > defines and not the device CONFIG_* defines are present when processing
> > > hmp-commands-info.hx.
> >
> > Yeah, that's where the pain comes in. While QAPI schema can be made
> > conditional on a few CONFIG_* parameters - basically those derived
> > from global configure time options, it is impossible for this to be
> > with with target specific options like the device CONFIG_* defines.
> >
> > This is why I suggested in my othuer reply that it would need to be
> > done with a generic 'info dev-debug' / 'x-query-dev-debug' command
> > that can be registered unconditionally, and then individual devices
> > plug into it.
>
> After some more experiments this afternoon I still seem to be falling
> through the gaps on this one. This is based upon my understanding that all
> new HMP commands should use a QMP HumanReadableText implementation and the
> new command should be restricted according to target.
>
> Currently I am working with this change to hmp-commands-info.hx and
> qapi/misc-target.json:
[snip]
> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
> TARGET guards and since HumanReadableText is only used by the new
> qmp_x_query_via() functionality then the compiler complains and aborts the
> compilation.
>
> Possibly this is an error in the QAPI generator for types hidden behind
> commands using "if"? Otherwise I'm not sure what is the best way to proceed,
> so I'd be grateful for some further pointers.
Yes, this is pretty much what I expect and exactly what I hit with
other target specific commands.
That's why I suggested something like a general 'x-device-debug' command
that is NOT conditionalized in QAPI, against which dev impls can register
a callback to provide detailed reporting, instead of a device type specific
command.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-21 17:11 ` Daniel P. Berrangé
@ 2022-02-21 22:29 ` Mark Cave-Ayland
2022-02-22 15:03 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-21 22:29 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: laurent, Dr. David Alan Gilbert (git), qemu-devel
On 21/02/2022 17:11, Daniel P. Berrangé wrote:
> On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
>> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>>
>>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>>
>>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>>> unreliable text opaque for humans"?
>>>>>
>>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>>> that's only needed if you must dynamically register the HMP command.
>>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>>> the hml-commands-info.hx for the HMP side.
>>>>>
>>>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>>>> this example and where it should go? Looking at the existing .json files I
>>>>>> can't immediately see one which is the right place for this to live.
>>>>>
>>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>>> there. The QAPI bit is fairly simple.
>>>>>
>>>>> if you want to see an illustration of what's different from a previous
>>>>> pure HMP impl, look at:
>>>>>
>>>>> commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>>> Author: Daniel P. Berrangé <berrange@redhat.com>
>>>>> Date: Wed Sep 8 10:35:43 2021 +0100
>>>>>
>>>>> qapi: introduce x-query-roms QMP command
>>>>
>>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>>> place to declare the QMP part even for a specific device?
>>>>
>>>> Even this approach still wouldn't work in its current form though, since as
>>>> mentioned in my previous email it seems that only the target CONFIG_*
>>>> defines and not the device CONFIG_* defines are present when processing
>>>> hmp-commands-info.hx.
>>>
>>> Yeah, that's where the pain comes in. While QAPI schema can be made
>>> conditional on a few CONFIG_* parameters - basically those derived
>>> from global configure time options, it is impossible for this to be
>>> with with target specific options like the device CONFIG_* defines.
>>>
>>> This is why I suggested in my othuer reply that it would need to be
>>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>>> that can be registered unconditionally, and then individual devices
>>> plug into it.
>>
>> After some more experiments this afternoon I still seem to be falling
>> through the gaps on this one. This is based upon my understanding that all
>> new HMP commands should use a QMP HumanReadableText implementation and the
>> new command should be restricted according to target.
>>
>> Currently I am working with this change to hmp-commands-info.hx and
>> qapi/misc-target.json:
>
> [snip]
>
>> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
>> TARGET guards and since HumanReadableText is only used by the new
>> qmp_x_query_via() functionality then the compiler complains and aborts the
>> compilation.
>>
>> Possibly this is an error in the QAPI generator for types hidden behind
>> commands using "if"? Otherwise I'm not sure what is the best way to proceed,
>> so I'd be grateful for some further pointers.
>
> Yes, this is pretty much what I expect and exactly what I hit with
> other target specific commands.
>
> That's why I suggested something like a general 'x-device-debug' command
> that is NOT conditionalized in QAPI, against which dev impls can register
> a callback to provide detailed reporting, instead of a device type specific
> command.
Ah so this is a known issue with this approach then. David mentioned earlier in the
thread that he'd be okay with a HMP command if it was useful and restricted to the
required targets, so would it be okay to add "info via" for now as just a (non-QMP
wrapped) HMP info command if I can get that to work?
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-21 22:29 ` Mark Cave-Ayland
@ 2022-02-22 15:03 ` Dr. David Alan Gilbert
2022-02-24 12:26 ` Mark Cave-Ayland
0 siblings, 1 reply; 45+ messages in thread
From: Dr. David Alan Gilbert @ 2022-02-22 15:03 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: Daniel P. Berrangé, laurent, qemu-devel
* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
> On 21/02/2022 17:11, Daniel P. Berrangé wrote:
>
> > On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
> > > On 08/02/2022 13:10, Daniel P. Berrangé wrote:
> > >
> > > > On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
> > > > > On 08/02/2022 12:49, Daniel P. Berrangé wrote:
> > > > >
> > > > > > > I was under the impression that monitor_register_hmp_info_hrt() does all the
> > > > > > > magic here i.e. it declares the underlying QMP command with an x- prefix and
> > > > > > > effectively encapsulates the text field in a way that says "this is an
> > > > > > > unreliable text opaque for humans"?
> > > > > >
> > > > > > The monitor_register_hmp_info_hrt only does the HMP glue side, and
> > > > > > that's only needed if you must dynamically register the HMP command.
> > > > > > For statically registered commands set '.cmd_info_hrt' directly in
> > > > > > the hml-commands-info.hx for the HMP side.
> > > > > >
> > > > > > > If a qapi/ schema is needed could you explain what it should look like for
> > > > > > > this example and where it should go? Looking at the existing .json files I
> > > > > > > can't immediately see one which is the right place for this to live.
> > > > > >
> > > > > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
> > > > > > there. The QAPI bit is fairly simple.
> > > > > >
> > > > > > if you want to see an illustration of what's different from a previous
> > > > > > pure HMP impl, look at:
> > > > > >
> > > > > > commit dd98234c059e6bdb05a52998270df6d3d990332e
> > > > > > Author: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > Date: Wed Sep 8 10:35:43 2021 +0100
> > > > > >
> > > > > > qapi: introduce x-query-roms QMP command
> > > > >
> > > > > I see, thanks for the reference. So qapi/machine.json would be the right
> > > > > place to declare the QMP part even for a specific device?
> > > > >
> > > > > Even this approach still wouldn't work in its current form though, since as
> > > > > mentioned in my previous email it seems that only the target CONFIG_*
> > > > > defines and not the device CONFIG_* defines are present when processing
> > > > > hmp-commands-info.hx.
> > > >
> > > > Yeah, that's where the pain comes in. While QAPI schema can be made
> > > > conditional on a few CONFIG_* parameters - basically those derived
> > > > from global configure time options, it is impossible for this to be
> > > > with with target specific options like the device CONFIG_* defines.
> > > >
> > > > This is why I suggested in my othuer reply that it would need to be
> > > > done with a generic 'info dev-debug' / 'x-query-dev-debug' command
> > > > that can be registered unconditionally, and then individual devices
> > > > plug into it.
> > >
> > > After some more experiments this afternoon I still seem to be falling
> > > through the gaps on this one. This is based upon my understanding that all
> > > new HMP commands should use a QMP HumanReadableText implementation and the
> > > new command should be restricted according to target.
> > >
> > > Currently I am working with this change to hmp-commands-info.hx and
> > > qapi/misc-target.json:
> >
> > [snip]
> > > i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
> > > TARGET guards and since HumanReadableText is only used by the new
> > > qmp_x_query_via() functionality then the compiler complains and aborts the
> > > compilation.
> > >
> > > Possibly this is an error in the QAPI generator for types hidden behind
> > > commands using "if"? Otherwise I'm not sure what is the best way to proceed,
> > > so I'd be grateful for some further pointers.
> >
> > Yes, this is pretty much what I expect and exactly what I hit with
> > other target specific commands.
> >
> > That's why I suggested something like a general 'x-device-debug' command
> > that is NOT conditionalized in QAPI, against which dev impls can register
> > a callback to provide detailed reporting, instead of a device type specific
> > command.
>
> Ah so this is a known issue with this approach then. David mentioned earlier
> in the thread that he'd be okay with a HMP command if it was useful and
> restricted to the required targets, so would it be okay to add "info via"
> for now as just a (non-QMP wrapped) HMP info command if I can get that to
> work?
I still am from an HMP point of view; it sounds like the right way in
the future is to get the info devices or whatever; I suggest you keep
it as close to a QMP implementation as possible, still with the
HumanReadableText stuff.
(Others might still be nervous about an HMP special; but I don't see
it's worth holding this trivial stuff up for it).
Dave
>
> ATB,
>
> Mark.
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging
2022-02-22 15:03 ` Dr. David Alan Gilbert
@ 2022-02-24 12:26 ` Mark Cave-Ayland
0 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-02-24 12:26 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Daniel P. Berrangé, laurent, qemu-devel
On 22/02/2022 15:03, Dr. David Alan Gilbert wrote:
> * Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
>> On 21/02/2022 17:11, Daniel P. Berrangé wrote:
>>
>>> On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote:
>>>> On 08/02/2022 13:10, Daniel P. Berrangé wrote:
>>>>
>>>>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote:
>>>>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote:
>>>>>>
>>>>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the
>>>>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and
>>>>>>>> effectively encapsulates the text field in a way that says "this is an
>>>>>>>> unreliable text opaque for humans"?
>>>>>>>
>>>>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and
>>>>>>> that's only needed if you must dynamically register the HMP command.
>>>>>>> For statically registered commands set '.cmd_info_hrt' directly in
>>>>>>> the hml-commands-info.hx for the HMP side.
>>>>>>>
>>>>>>>> If a qapi/ schema is needed could you explain what it should look like for
>>>>>>>> this example and where it should go? Looking at the existing .json files I
>>>>>>>> can't immediately see one which is the right place for this to live.
>>>>>>>
>>>>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
>>>>>>> there. The QAPI bit is fairly simple.
>>>>>>>
>>>>>>> if you want to see an illustration of what's different from a previous
>>>>>>> pure HMP impl, look at:
>>>>>>>
>>>>>>> commit dd98234c059e6bdb05a52998270df6d3d990332e
>>>>>>> Author: Daniel P. Berrangé <berrange@redhat.com>
>>>>>>> Date: Wed Sep 8 10:35:43 2021 +0100
>>>>>>>
>>>>>>> qapi: introduce x-query-roms QMP command
>>>>>>
>>>>>> I see, thanks for the reference. So qapi/machine.json would be the right
>>>>>> place to declare the QMP part even for a specific device?
>>>>>>
>>>>>> Even this approach still wouldn't work in its current form though, since as
>>>>>> mentioned in my previous email it seems that only the target CONFIG_*
>>>>>> defines and not the device CONFIG_* defines are present when processing
>>>>>> hmp-commands-info.hx.
>>>>>
>>>>> Yeah, that's where the pain comes in. While QAPI schema can be made
>>>>> conditional on a few CONFIG_* parameters - basically those derived
>>>>> from global configure time options, it is impossible for this to be
>>>>> with with target specific options like the device CONFIG_* defines.
>>>>>
>>>>> This is why I suggested in my othuer reply that it would need to be
>>>>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command
>>>>> that can be registered unconditionally, and then individual devices
>>>>> plug into it.
>>>>
>>>> After some more experiments this afternoon I still seem to be falling
>>>> through the gaps on this one. This is based upon my understanding that all
>>>> new HMP commands should use a QMP HumanReadableText implementation and the
>>>> new command should be restricted according to target.
>>>>
>>>> Currently I am working with this change to hmp-commands-info.hx and
>>>> qapi/misc-target.json:
>>>
>>> [snip]
>>>> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if
>>>> TARGET guards and since HumanReadableText is only used by the new
>>>> qmp_x_query_via() functionality then the compiler complains and aborts the
>>>> compilation.
>>>>
>>>> Possibly this is an error in the QAPI generator for types hidden behind
>>>> commands using "if"? Otherwise I'm not sure what is the best way to proceed,
>>>> so I'd be grateful for some further pointers.
>>>
>>> Yes, this is pretty much what I expect and exactly what I hit with
>>> other target specific commands.
>>>
>>> That's why I suggested something like a general 'x-device-debug' command
>>> that is NOT conditionalized in QAPI, against which dev impls can register
>>> a callback to provide detailed reporting, instead of a device type specific
>>> command.
>>
>> Ah so this is a known issue with this approach then. David mentioned earlier
>> in the thread that he'd be okay with a HMP command if it was useful and
>> restricted to the required targets, so would it be okay to add "info via"
>> for now as just a (non-QMP wrapped) HMP info command if I can get that to
>> work?
>
> I still am from an HMP point of view; it sounds like the right way in
> the future is to get the info devices or whatever; I suggest you keep
> it as close to a QMP implementation as possible, still with the
> HumanReadableText stuff.
> (Others might still be nervous about an HMP special; but I don't see
> it's worth holding this trivial stuff up for it).
I've just posted a v2 and what I've done there is to manually add a hmp_info_via()
wrapper (taken almost verbatim from
https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#id1) and added
it to both include/monitor/hmp-target.h and include/hw/misc/mos6522.h which passes a
Gitlab run.
I think it's worth having as an in-tree reference for when a more formal HMP/QMP
per-device as opposed to per-target infrastructure arrives.
ATB,
Mark.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 09/11] mos6522: record last_irq_levels in mos6522_set_irq()
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (7 preceding siblings ...)
2022-01-27 20:54 ` [PATCH 08/11] mos6522: add "info via" HMP command for debugging Mark Cave-Ayland
@ 2022-01-27 20:54 ` Mark Cave-Ayland
2022-01-27 20:54 ` [PATCH 10/11] mos6522: implement edge-triggering for CA1/2 and CB1/2 control line IRQs Mark Cave-Ayland
2022-01-27 20:54 ` [PATCH 11/11] macio/pmu.c: remove redundant code Mark Cave-Ayland
10 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:54 UTC (permalink / raw)
To: laurent, qemu-devel
To detect edge-triggered IRQs it is necessary to store the last state of each
IRQ in a last_irq_levels bitmap.
Note: this is a migration break for machines which use mos6522 instances which
are g3beige/mac99 (PPC) and q800 (m68k).
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/mos6522.c | 11 +++++++++--
include/hw/misc/mos6522.h | 1 +
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index cfa6a9c44b..6eed751726 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -71,6 +71,12 @@ static void mos6522_set_irq(void *opaque, int n, int level)
}
mos6522_update_irq(s);
+
+ if (level) {
+ s->last_irq_levels |= 1 << n;
+ } else {
+ s->last_irq_levels &= ~(1 << n);
+ }
}
static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
@@ -532,8 +538,8 @@ static const VMStateDescription vmstate_mos6522_timer = {
const VMStateDescription vmstate_mos6522 = {
.name = "mos6522",
- .version_id = 0,
- .minimum_version_id = 0,
+ .version_id = 1,
+ .minimum_version_id = 1,
.fields = (VMStateField[]) {
VMSTATE_UINT8(a, MOS6522State),
VMSTATE_UINT8(b, MOS6522State),
@@ -544,6 +550,7 @@ const VMStateDescription vmstate_mos6522 = {
VMSTATE_UINT8(pcr, MOS6522State),
VMSTATE_UINT8(ifr, MOS6522State),
VMSTATE_UINT8(ier, MOS6522State),
+ VMSTATE_UINT8(last_irq_levels, MOS6522State),
VMSTATE_STRUCT_ARRAY(timers, MOS6522State, 2, 0,
vmstate_mos6522_timer, MOS6522Timer),
VMSTATE_END_OF_LIST()
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index f6b513a3a2..6c40a8fcd3 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -131,6 +131,7 @@ struct MOS6522State {
uint64_t frequency;
qemu_irq irq;
+ uint8_t last_irq_levels;
};
#define TYPE_MOS6522 "mos6522"
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 10/11] mos6522: implement edge-triggering for CA1/2 and CB1/2 control line IRQs
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (8 preceding siblings ...)
2022-01-27 20:54 ` [PATCH 09/11] mos6522: record last_irq_levels in mos6522_set_irq() Mark Cave-Ayland
@ 2022-01-27 20:54 ` Mark Cave-Ayland
2022-01-27 20:54 ` [PATCH 11/11] macio/pmu.c: remove redundant code Mark Cave-Ayland
10 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:54 UTC (permalink / raw)
To: laurent, qemu-devel
The mos6522 datasheet describes how the control lines IRQs are edge-triggered
according to the configuration in the PCR register. Implement the logic according
to the datasheet so that the interrupt bits in IFR are latched when the edge is
detected, and cleared when reading portA/portB or writing to IFR as necessary.
To maintain bisectibility this change also updates the SCSI, SCSI data, Nubus
and VIA2 60Hz/1Hz clocks in the q800 machine to be negative edge-triggered as
confirmed by the PCR programming in all of Linux, NetBSD and MacOS.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/m68k/q800.c | 9 +++--
hw/misc/mac_via.c | 11 ++++--
hw/misc/mos6522.c | 82 +++++++++++++++++++++++++++++++++++++--
include/hw/misc/mos6522.h | 15 +++++++
4 files changed, 106 insertions(+), 11 deletions(-)
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 55dfe5036f..66ca5c0df6 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -533,10 +533,11 @@ static void q800_init(MachineState *machine)
sysbus = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(sysbus, &error_fatal);
- sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(via2_dev,
- VIA2_IRQ_SCSI_BIT));
- sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(via2_dev,
- VIA2_IRQ_SCSI_DATA_BIT));
+ /* SCSI and SCSI data IRQs are negative edge triggered */
+ sysbus_connect_irq(sysbus, 0, qemu_irq_invert(qdev_get_gpio_in(via2_dev,
+ VIA2_IRQ_SCSI_BIT)));
+ sysbus_connect_irq(sysbus, 1, qemu_irq_invert(qdev_get_gpio_in(via2_dev,
+ VIA2_IRQ_SCSI_DATA_BIT)));
sysbus_mmio_map(sysbus, 0, ESP_BASE);
sysbus_mmio_map(sysbus, 1, ESP_PDMA);
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 95cf2e03d9..d9ab5b3839 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -327,7 +327,9 @@ static void via1_sixty_hz(void *opaque)
MOS6522State *s = MOS6522(v1s);
qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_60HZ_BIT);
- qemu_set_irq(irq, 1);
+ /* Negative edge trigger */
+ qemu_irq_lower(irq);
+ qemu_irq_raise(irq);
via1_sixty_hz_update(v1s);
}
@@ -338,7 +340,9 @@ static void via1_one_second(void *opaque)
MOS6522State *s = MOS6522(v1s);
qemu_irq irq = qdev_get_gpio_in(DEVICE(s), VIA1_IRQ_ONE_SECOND_BIT);
- qemu_set_irq(irq, 1);
+ /* Negative edge trigger */
+ qemu_irq_lower(irq);
+ qemu_irq_raise(irq);
via1_one_second_update(v1s);
}
@@ -1131,7 +1135,8 @@ static void via2_nubus_irq_request(void *opaque, int n, int level)
s->a |= (1 << n);
}
- qemu_set_irq(irq, level);
+ /* Negative edge trigger */
+ qemu_set_irq(irq, !level);
}
static void mos6522_q800_via2_init(Object *obj)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 6eed751726..84b2d54ef3 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,14 +63,62 @@ static void mos6522_update_irq(MOS6522State *s)
static void mos6522_set_irq(void *opaque, int n, int level)
{
MOS6522State *s = MOS6522(opaque);
+ int last_level = !!(s->last_irq_levels & (1 << n));
+ uint8_t last_ifr = s->ifr;
+ bool positive_edge = true;
+ int ctrl;
+
+ /*
+ * SR_INT is managed by mos6522 instances and cleared upon SR
+ * read. It is only the external CA1/2 and CB1/2 lines that
+ * are edge-triggered and latched in IFR
+ */
+ if (n != SR_INT_BIT && level == last_level) {
+ return;
+ }
- if (level) {
+ /* Detect negative edge trigger */
+ if (last_level == 1 && level == 0) {
+ positive_edge = false;
+ }
+
+ switch (n) {
+ case CA2_INT_BIT:
+ ctrl = (s->pcr & CA2_CTRL_MASK) >> CA2_CTRL_SHIFT;
+ if ((positive_edge && (ctrl & C2_POS)) ||
+ (!positive_edge && !(ctrl & C2_POS))) {
+ s->ifr |= 1 << n;
+ }
+ break;
+ case CA1_INT_BIT:
+ ctrl = (s->pcr & CA1_CTRL_MASK) >> CA1_CTRL_SHIFT;
+ if ((positive_edge && (ctrl & C1_POS)) ||
+ (!positive_edge && !(ctrl & C1_POS))) {
+ s->ifr |= 1 << n;
+ }
+ break;
+ case SR_INT_BIT:
s->ifr |= 1 << n;
- } else {
- s->ifr &= ~(1 << n);
+ break;
+ case CB2_INT_BIT:
+ ctrl = (s->pcr & CB2_CTRL_MASK) >> CB2_CTRL_SHIFT;
+ if ((positive_edge && (ctrl & C2_POS)) ||
+ (!positive_edge && !(ctrl & C2_POS))) {
+ s->ifr |= 1 << n;
+ }
+ break;
+ case CB1_INT_BIT:
+ ctrl = (s->pcr & CB1_CTRL_MASK) >> CB1_CTRL_SHIFT;
+ if ((positive_edge && (ctrl & C1_POS)) ||
+ (!positive_edge && !(ctrl & C1_POS))) {
+ s->ifr |= 1 << n;
+ }
+ break;
}
- mos6522_update_irq(s);
+ if (s->ifr != last_ifr) {
+ mos6522_update_irq(s);
+ }
if (level) {
s->last_irq_levels |= 1 << n;
@@ -249,6 +297,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
{
MOS6522State *s = opaque;
uint32_t val;
+ int ctrl;
int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
if (now >= s->timers[0].next_irq_time) {
@@ -262,12 +311,24 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size)
switch (addr) {
case VIA_REG_B:
val = s->b;
+ ctrl = (s->pcr & CB2_CTRL_MASK) >> CB2_CTRL_SHIFT;
+ if (!(ctrl & C2_IND)) {
+ s->ifr &= ~CB2_INT;
+ }
+ s->ifr &= ~CB1_INT;
+ mos6522_update_irq(s);
break;
case VIA_REG_A:
qemu_log_mask(LOG_UNIMP, "Read access to register A with handshake");
/* fall through */
case VIA_REG_ANH:
val = s->a;
+ ctrl = (s->pcr & CA2_CTRL_MASK) >> CA2_CTRL_SHIFT;
+ if (!(ctrl & C2_IND)) {
+ s->ifr &= ~CA2_INT;
+ }
+ s->ifr &= ~CA1_INT;
+ mos6522_update_irq(s);
break;
case VIA_REG_DIRB:
val = s->dirb;
@@ -334,6 +395,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
{
MOS6522State *s = opaque;
MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+ int ctrl;
trace_mos6522_write(addr, mos6522_reg_names[addr], val);
@@ -341,6 +403,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
case VIA_REG_B:
s->b = (s->b & ~s->dirb) | (val & s->dirb);
mdc->portB_write(s);
+ ctrl = (s->pcr & CB2_CTRL_MASK) >> CB2_CTRL_SHIFT;
+ if (!(ctrl & C2_IND)) {
+ s->ifr &= ~CB2_INT;
+ }
+ s->ifr &= ~CB1_INT;
+ mos6522_update_irq(s);
break;
case VIA_REG_A:
qemu_log_mask(LOG_UNIMP, "Write access to register A with handshake");
@@ -348,6 +416,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
case VIA_REG_ANH:
s->a = (s->a & ~s->dira) | (val & s->dira);
mdc->portA_write(s);
+ ctrl = (s->pcr & CA2_CTRL_MASK) >> CA2_CTRL_SHIFT;
+ if (!(ctrl & C2_IND)) {
+ s->ifr &= ~CA2_INT;
+ }
+ s->ifr &= ~CA1_INT;
+ mos6522_update_irq(s);
break;
case VIA_REG_DIRB:
s->dirb = val;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index 6c40a8fcd3..a9afb539b2 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -63,6 +63,21 @@
#define T1MODE 0xc0 /* Timer 1 mode */
#define T1MODE_CONT 0x40 /* continuous interrupts */
+/* Bits in PCR */
+#define CB2_CTRL_MASK 0xe0
+#define CB2_CTRL_SHIFT 5
+#define CB1_CTRL_MASK 0x10
+#define CB1_CTRL_SHIFT 4
+#define CA2_CTRL_MASK 0x0e
+#define CA2_CTRL_SHIFT 1
+#define CA1_CTRL_MASK 0x1
+#define CA1_CTRL_SHIFT 0
+
+#define C2_POS 0x2
+#define C2_IND 0x1
+
+#define C1_POS 0x1
+
/* VIA registers */
#define VIA_REG_B 0x00
#define VIA_REG_A 0x01
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH 11/11] macio/pmu.c: remove redundant code
2022-01-27 20:53 [PATCH 00/11] mos6522: switch to gpios, add control line edge-triggering and extra debugging Mark Cave-Ayland
` (9 preceding siblings ...)
2022-01-27 20:54 ` [PATCH 10/11] mos6522: implement edge-triggering for CA1/2 and CB1/2 control line IRQs Mark Cave-Ayland
@ 2022-01-27 20:54 ` Mark Cave-Ayland
10 siblings, 0 replies; 45+ messages in thread
From: Mark Cave-Ayland @ 2022-01-27 20:54 UTC (permalink / raw)
To: laurent, qemu-devel
Now that the logic related to edge-triggered interrupts is all contained within
the mos6522 device the redundant implementation for the mac99 PMU device can
be removed.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/misc/macio/pmu.c | 33 ---------------------------------
include/hw/misc/macio/pmu.h | 2 --
2 files changed, 35 deletions(-)
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index a5dd0a4734..a16841e758 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -57,19 +57,6 @@
#define VIA_TIMER_FREQ (4700000 / 6)
-static void via_update_irq(PMUState *s)
-{
- MOS6522PMUState *mps = MOS6522_PMU(&s->mos6522_pmu);
- MOS6522State *ms = MOS6522(mps);
-
- bool new_state = !!(ms->ifr & ms->ier & (SR_INT | T1_INT | T2_INT));
-
- if (new_state != s->via_irq_state) {
- s->via_irq_state = new_state;
- qemu_set_irq(s->via_irq, new_state);
- }
-}
-
static void via_set_sr_int(void *opaque)
{
PMUState *s = opaque;
@@ -808,28 +795,9 @@ static void mos6522_pmu_portB_write(MOS6522State *s)
MOS6522PMUState *mps = container_of(s, MOS6522PMUState, parent_obj);
PMUState *ps = container_of(mps, PMUState, mos6522_pmu);
- if ((s->pcr & 0xe0) == 0x20 || (s->pcr & 0xe0) == 0x60) {
- s->ifr &= ~CB2_INT;
- }
- s->ifr &= ~CB1_INT;
-
- via_update_irq(ps);
pmu_update(ps);
}
-static void mos6522_pmu_portA_write(MOS6522State *s)
-{
- MOS6522PMUState *mps = container_of(s, MOS6522PMUState, parent_obj);
- PMUState *ps = container_of(mps, PMUState, mos6522_pmu);
-
- if ((s->pcr & 0x0e) == 0x02 || (s->pcr & 0x0e) == 0x06) {
- s->ifr &= ~CA2_INT;
- }
- s->ifr &= ~CA1_INT;
-
- via_update_irq(ps);
-}
-
static void mos6522_pmu_reset(DeviceState *dev)
{
MOS6522State *ms = MOS6522(dev);
@@ -853,7 +821,6 @@ static void mos6522_pmu_class_init(ObjectClass *oc, void *data)
device_class_set_parent_reset(dc, mos6522_pmu_reset,
&mdc->parent_reset);
mdc->portB_write = mos6522_pmu_portB_write;
- mdc->portA_write = mos6522_pmu_portA_write;
}
static const TypeInfo mos6522_pmu_type_info = {
diff --git a/include/hw/misc/macio/pmu.h b/include/hw/misc/macio/pmu.h
index 78237d99a2..00fcdd23f5 100644
--- a/include/hw/misc/macio/pmu.h
+++ b/include/hw/misc/macio/pmu.h
@@ -193,8 +193,6 @@ struct PMUState {
MemoryRegion mem;
uint64_t frequency;
- qemu_irq via_irq;
- bool via_irq_state;
/* PMU state */
MOS6522PMUState mos6522_pmu;
--
2.20.1
^ permalink raw reply related [flat|nested] 45+ messages in thread