[v4,12/30] hw/core: Introduce proxy-pic

Commit Message

Bernhard Beschow Dec. 21, 2022, 4:59 p.m. UTC

Having a proxy PIC allows for ISA PICs to be created and wired up in
southbridges. This is especially useful for PIIX3 for two reasons:
First, the southbridge doesn't need to care about the virtualization
technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
attached) and out-IRQs (which will trigger the IRQs of the respective
virtzalization technology) are separated. Second, since the in-IRQs are
populated with fully initialized qemu_irq's, they can already be wired
up inside PIIX3.

Signed-off-by: Bernhard Beschow <[e-mail]>
Reviewed-by: Michael S. Tsirkin <[e-mail]>
Message-Id: <[e-mail]>
---
 MAINTAINERS                 |  2 ++
 hw/core/Kconfig             |  3 ++
 hw/core/meson.build         |  1 +
 hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
 include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
 5 files changed, 130 insertions(+)
 create mode 100644 hw/core/proxy-pic.c
 create mode 100644 include/hw/core/proxy-pic.h

Comments

Philippe Mathieu-Daudé Jan. 4, 2023, 2:37 p.m. UTC | #1

On 21/12/22 17:59, Bernhard Beschow wrote:
> Having a proxy PIC allows for ISA PICs to be created and wired up in
> southbridges. This is especially useful for PIIX3 for two reasons:
> First, the southbridge doesn't need to care about the virtualization
> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
> attached) and out-IRQs (which will trigger the IRQs of the respective
> virtzalization technology) are separated. Second, since the in-IRQs are
> populated with fully initialized qemu_irq's, they can already be wired
> up inside PIIX3.
> 
> Signed-off-by: Bernhard Beschow <[e-mail]>
> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> Message-Id: <[e-mail]>
> ---
>   MAINTAINERS                 |  2 ++
>   hw/core/Kconfig             |  3 ++
>   hw/core/meson.build         |  1 +
>   hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>   include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>   5 files changed, 130 insertions(+)
>   create mode 100644 hw/core/proxy-pic.c
>   create mode 100644 include/hw/core/proxy-pic.h

Please enable scripts/git.orderfile.

> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
> new file mode 100644
> index 0000000000..0eb40c478a
> --- /dev/null
> +++ b/include/hw/core/proxy-pic.h
> @@ -0,0 +1,54 @@
> +/*
> + * Proxy interrupt controller device.
> + *
> + * Copyright (c) 2022 Bernhard Beschow <[e-mail]>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.

This is the MIT license right? Do you mind adding a SPDX tag along?

  * SPDX-License-Identifier: MIT

> + */
> +
> +#ifndef HW_PROXY_PIC_H
> +#define HW_PROXY_PIC_H
> +
> +#include "hw/qdev-core.h"
> +#include "qom/object.h"
> +#include "hw/irq.h"
> +
> +#define TYPE_PROXY_PIC "proxy-pic"
> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
> +
> +#define MAX_PROXY_PIC_LINES 16
> +
> +/**
> + * This is a simple device which has 16 pairs of GPIO input and output lines.
> + * Any change on an input line is forwarded to the respective output.
> + *
> + * QEMU interface:
> + *  + 16 unnamed GPIO inputs: the input lines
> + *  + 16 unnamed GPIO outputs: the output lines
> + */

Why restrict to 16 and not use a class property and allocate
on the heap? See TYPE_SPLIT_IRQ for example.

> +struct ProxyPICState {
> +    /*< private >*/
> +    struct DeviceState parent_obj;
> +    /*< public >*/
> +
> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
> +};
> +
> +#endif /* HW_PROXY_PIC_H */

Bernhard Beschow Jan. 4, 2023, 4:01 p.m. UTC | #2

Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>On 21/12/22 17:59, Bernhard Beschow wrote:
>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>> southbridges. This is especially useful for PIIX3 for two reasons:
>> First, the southbridge doesn't need to care about the virtualization
>> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
>> attached) and out-IRQs (which will trigger the IRQs of the respective
>> virtzalization technology) are separated. Second, since the in-IRQs are
>> populated with fully initialized qemu_irq's, they can already be wired
>> up inside PIIX3.
>> 
>> Signed-off-by: Bernhard Beschow <[e-mail]>
>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
>> Message-Id: <[e-mail]>
>> ---
>>   MAINTAINERS                 |  2 ++
>>   hw/core/Kconfig             |  3 ++
>>   hw/core/meson.build         |  1 +
>>   hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>   include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>   5 files changed, 130 insertions(+)
>>   create mode 100644 hw/core/proxy-pic.c
>>   create mode 100644 include/hw/core/proxy-pic.h
>
>Please enable scripts/git.orderfile.

Will do.

>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>> new file mode 100644
>> index 0000000000..0eb40c478a
>> --- /dev/null
>> +++ b/include/hw/core/proxy-pic.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * Proxy interrupt controller device.
>> + *
>> + * Copyright (c) 2022 Bernhard Beschow <[e-mail]>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>
>This is the MIT license right? Do you mind adding a SPDX tag along?

I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus preserved the license.

> * SPDX-License-Identifier: MIT

Or just replace the wall of text with this line? This should suffice, no?

>> + */
>> +
>> +#ifndef HW_PROXY_PIC_H
>> +#define HW_PROXY_PIC_H
>> +
>> +#include "hw/qdev-core.h"
>> +#include "qom/object.h"
>> +#include "hw/irq.h"
>> +
>> +#define TYPE_PROXY_PIC "proxy-pic"
>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>> +
>> +#define MAX_PROXY_PIC_LINES 16
>> +
>> +/**
>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>> + * Any change on an input line is forwarded to the respective output.
>> + *
>> + * QEMU interface:
>> + *  + 16 unnamed GPIO inputs: the input lines
>> + *  + 16 unnamed GPIO outputs: the output lines
>> + */
>
>Why restrict to 16 and not use a class property and allocate
>on the heap? See TYPE_SPLIT_IRQ for example.

TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)

I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.

Thanks,
Bernhard

>
>> +struct ProxyPICState {
>> +    /*< private >*/
>> +    struct DeviceState parent_obj;
>> +    /*< public >*/
>> +
>> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
>> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
>> +};
>> +
>> +#endif /* HW_PROXY_PIC_H */
>

Philippe Mathieu-Daudé Jan. 4, 2023, 4:35 p.m. UTC | #3

On 4/1/23 17:01, Bernhard Beschow wrote:
> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>> southbridges. This is especially useful for PIIX3 for two reasons:
>>> First, the southbridge doesn't need to care about the virtualization
>>> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
>>> attached) and out-IRQs (which will trigger the IRQs of the respective
>>> virtzalization technology) are separated. Second, since the in-IRQs are

Typo "virtualization".

>>> populated with fully initialized qemu_irq's, they can already be wired
>>> up inside PIIX3.
>>>
>>> Signed-off-by: Bernhard Beschow <[e-mail]>
>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
>>> Message-Id: <[e-mail]>
>>> ---
>>>    MAINTAINERS                 |  2 ++
>>>    hw/core/Kconfig             |  3 ++
>>>    hw/core/meson.build         |  1 +
>>>    hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>>    include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>>    5 files changed, 130 insertions(+)
>>>    create mode 100644 hw/core/proxy-pic.c
>>>    create mode 100644 include/hw/core/proxy-pic.h
>>
>> Please enable scripts/git.orderfile.
> 
> Will do.
> 
>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>> new file mode 100644
>>> index 0000000000..0eb40c478a
>>> --- /dev/null
>>> +++ b/include/hw/core/proxy-pic.h
>>> @@ -0,0 +1,54 @@
>>> +/*
>>> + * Proxy interrupt controller device.
>>> + *
>>> + * Copyright (c) 2022 Bernhard Beschow <[e-mail]>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to deal
>>> + * in the Software without restriction, including without limitation the rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>> + * THE SOFTWARE.
>>
>> This is the MIT license right? Do you mind adding a SPDX tag along?
> 
> I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus preserved the license.
> 
>> * SPDX-License-Identifier: MIT
> 
> Or just replace the wall of text with this line? This should suffice, no?

IIUC (IANAL) I can only suggest you to add a SPDX tag to the license you
chose, not ask you to remove the text; but since you ask/propose, the
tag suffices indeed. I suggest the tag use because it is clearer than
trying to match the full (often copy/pasted with typos) license text.

>>> + */
>>> +
>>> +#ifndef HW_PROXY_PIC_H
>>> +#define HW_PROXY_PIC_H
>>> +
>>> +#include "hw/qdev-core.h"
>>> +#include "qom/object.h"
>>> +#include "hw/irq.h"
>>> +
>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>> +
>>> +#define MAX_PROXY_PIC_LINES 16
>>> +
>>> +/**
>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>> + * Any change on an input line is forwarded to the respective output.
>>> + *
>>> + * QEMU interface:
>>> + *  + 16 unnamed GPIO inputs: the input lines
>>> + *  + 16 unnamed GPIO outputs: the output lines
>>> + */
>>
>> Why restrict to 16 and not use a class property and allocate
>> on the heap? See TYPE_SPLIT_IRQ for example.
> 
> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
> 
> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.

Oh indeed. Well this model as is is OK, but it could be more useful
if able to proxy any range of IRQs.

I have the feeling we are cycling around this IRQ proxy:

22ec3283ef ("irq: introduce qemu_irq_proxy()")
078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in 
datasheet")
fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")

What is our problem? IRQ lines connect 2 devices in a fixed direction.
Current model expects one edge to be wired to a device before wiring
the other device, so device composition with IRQs in middle is
impossible? If so, this doesn't scale with dynamic machine creation.

Maybe the IRQ wiring should be another machine phase, after all
devices are instantiated?

Your approach is to create the 'IRQ proxy' first, like drawing the
wires on a board, then plug the devices, like soldering chips on
the printed board IRQs. So maybe devices shouldn't be the QOM owners
of IRQs, the board should...

Yeah, just thinking loudly...

Mark Cave-Ayland Jan. 4, 2023, 4:51 p.m. UTC | #4

On 04/01/2023 16:35, Philippe Mathieu-Daudé wrote:

> On 4/1/23 17:01, Bernhard Beschow wrote:
>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>> southbridges. This is especially useful for PIIX3 for two reasons:
>>>> First, the southbridge doesn't need to care about the virtualization
>>>> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
>>>> attached) and out-IRQs (which will trigger the IRQs of the respective
>>>> virtzalization technology) are separated. Second, since the in-IRQs are
> 
> Typo "virtualization".
> 
>>>> populated with fully initialized qemu_irq's, they can already be wired
>>>> up inside PIIX3.
>>>>
>>>> Signed-off-by: Bernhard Beschow <[e-mail]>
>>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
>>>> Message-Id: <[e-mail]>
>>>> ---
>>>>    MAINTAINERS                 |  2 ++
>>>>    hw/core/Kconfig             |  3 ++
>>>>    hw/core/meson.build         |  1 +
>>>>    hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>>>    include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>>>    5 files changed, 130 insertions(+)
>>>>    create mode 100644 hw/core/proxy-pic.c
>>>>    create mode 100644 include/hw/core/proxy-pic.h
>>>
>>> Please enable scripts/git.orderfile.
>>
>> Will do.
>>
>>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>>> new file mode 100644
>>>> index 0000000000..0eb40c478a
>>>> --- /dev/null
>>>> +++ b/include/hw/core/proxy-pic.h
>>>> @@ -0,0 +1,54 @@
>>>> +/*
>>>> + * Proxy interrupt controller device.
>>>> + *
>>>> + * Copyright (c) 2022 Bernhard Beschow <[e-mail]>
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>>
>>> This is the MIT license right? Do you mind adding a SPDX tag along?
>>
>> I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus 
>> preserved the license.
>>
>>> * SPDX-License-Identifier: MIT
>>
>> Or just replace the wall of text with this line? This should suffice, no?
> 
> IIUC (IANAL) I can only suggest you to add a SPDX tag to the license you
> chose, not ask you to remove the text; but since you ask/propose, the
> tag suffices indeed. I suggest the tag use because it is clearer than
> trying to match the full (often copy/pasted with typos) license text.
> 
>>>> + */
>>>> +
>>>> +#ifndef HW_PROXY_PIC_H
>>>> +#define HW_PROXY_PIC_H
>>>> +
>>>> +#include "hw/qdev-core.h"
>>>> +#include "qom/object.h"
>>>> +#include "hw/irq.h"
>>>> +
>>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>>> +
>>>> +#define MAX_PROXY_PIC_LINES 16
>>>> +
>>>> +/**
>>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>>> + * Any change on an input line is forwarded to the respective output.
>>>> + *
>>>> + * QEMU interface:
>>>> + *  + 16 unnamed GPIO inputs: the input lines
>>>> + *  + 16 unnamed GPIO outputs: the output lines
>>>> + */
>>>
>>> Why restrict to 16 and not use a class property and allocate
>>> on the heap? See TYPE_SPLIT_IRQ for example.
>>
>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of 
>> MAX_SPLIT_LINES which equals 16 ;)
>>
>> I was unsure on when to free the memory and how to dispose the elements so I went 
>> with this solution for simplicity. I'll look for inspitation in other device models 
>> and respin.
> 
> Oh indeed. Well this model as is is OK, but it could be more useful
> if able to proxy any range of IRQs.
> 
> I have the feeling we are cycling around this IRQ proxy:
> 
> 22ec3283ef ("irq: introduce qemu_irq_proxy()")
> 078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
> fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
> 
> What is our problem? IRQ lines connect 2 devices in a fixed direction.
> Current model expects one edge to be wired to a device before wiring
> the other device, so device composition with IRQs in middle is
> impossible? If so, this doesn't scale with dynamic machine creation.
> 
> Maybe the IRQ wiring should be another machine phase, after all
> devices are instantiated?
> 
> Your approach is to create the 'IRQ proxy' first, like drawing the
> wires on a board, then plug the devices, like soldering chips on
> the printed board IRQs. So maybe devices shouldn't be the QOM owners
> of IRQs, the board should...
> 
> Yeah, just thinking loudly...

The main problem is that a lot of the old x86 code references QEMU IRQs directly 
instead of using qdev gpios, and so this proxy-pic device is a temporary glue which 
allows the x86 PIC board wiring to be done with qdev gpios, but still allow the 
various PIC implementations to access the QEMU IRQs directly as required.

One of my review comments from a previous version of the patch is that whilst this 
isn't a full qdev gpio conversion of all the x86 PIC implementations (which is likely 
a whole project in itself), there is a lot of good tidy-up work in this series. So as 
long as proxy-pic isn't directly exposed (for example, having qdev properties that 
are set by command line) I still think the series is worth merging in its current form.


ATB,

Mark.

Bernhard Beschow Jan. 4, 2023, 8:12 p.m. UTC | #5

Am 4. Januar 2023 16:35:57 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>On 4/1/23 17:01, Bernhard Beschow wrote:
>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>> southbridges. This is especially useful for PIIX3 for two reasons:
>>>> First, the southbridge doesn't need to care about the virtualization
>>>> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
>>>> attached) and out-IRQs (which will trigger the IRQs of the respective
>>>> virtzalization technology) are separated. Second, since the in-IRQs are
>
>Typo "virtualization".

Fixed...

>>>> populated with fully initialized qemu_irq's, they can already be wired
>>>> up inside PIIX3.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <[e-mail]>
>>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
>>>> Message-Id: <[e-mail]>
>>>> ---
>>>>    MAINTAINERS                 |  2 ++
>>>>    hw/core/Kconfig             |  3 ++
>>>>    hw/core/meson.build         |  1 +
>>>>    hw/core/proxy-pic.c         | 70 +++++++++++++++++++++++++++++++++++++
>>>>    include/hw/core/proxy-pic.h | 54 ++++++++++++++++++++++++++++
>>>>    5 files changed, 130 insertions(+)
>>>>    create mode 100644 hw/core/proxy-pic.c
>>>>    create mode 100644 include/hw/core/proxy-pic.h
>>> 
>>> Please enable scripts/git.orderfile.
>> 
>> Will do.
>> 
>>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>>> new file mode 100644
>>>> index 0000000000..0eb40c478a
>>>> --- /dev/null
>>>> +++ b/include/hw/core/proxy-pic.h
>>>> @@ -0,0 +1,54 @@
>>>> +/*
>>>> + * Proxy interrupt controller device.
>>>> + *
>>>> + * Copyright (c) 2022 Bernhard Beschow <[e-mail]>
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>> 
>>> This is the MIT license right? Do you mind adding a SPDX tag along?
>> 
>> I based my implementation on TYPE_SPLIT_IRQ as you suggested before and thus preserved the license.
>> 
>>> * SPDX-License-Identifier: MIT
>> 
>> Or just replace the wall of text with this line? This should suffice, no?
>
>IIUC (IANAL) I can only suggest you to add a SPDX tag to the license you
>chose, not ask you to remove the text; but since you ask/propose, the
>tag suffices indeed. I suggest the tag use because it is clearer than
>trying to match the full (often copy/pasted with typos) license text.

Changed...

>>>> + */
>>>> +
>>>> +#ifndef HW_PROXY_PIC_H
>>>> +#define HW_PROXY_PIC_H
>>>> +
>>>> +#include "hw/qdev-core.h"
>>>> +#include "qom/object.h"
>>>> +#include "hw/irq.h"
>>>> +
>>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>>> +
>>>> +#define MAX_PROXY_PIC_LINES 16
>>>> +
>>>> +/**
>>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>>> + * Any change on an input line is forwarded to the respective output.
>>>> + *
>>>> + * QEMU interface:
>>>> + *  + 16 unnamed GPIO inputs: the input lines
>>>> + *  + 16 unnamed GPIO outputs: the output lines
>>>> + */
>>> 
>>> Why restrict to 16 and not use a class property and allocate
>>> on the heap? See TYPE_SPLIT_IRQ for example.
>> 
>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
>> 
>> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.
>
>Oh indeed. Well this model as is is OK, but it could be more useful
>if able to proxy any range of IRQs.

I've responded with a new, single patch to this patch. Is that okay or shall I respin the whole series? Is anything missing? IIUC we can make the proxy-pic dynamic in a follow-up?

>I have the feeling we are cycling around this IRQ proxy:
>
>22ec3283ef ("irq: introduce qemu_irq_proxy()")
>078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
>fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
>
>What is our problem? IRQ lines connect 2 devices in a fixed direction.
>Current model expects one edge to be wired to a device before wiring
>the other device, so device composition with IRQs in middle is
>impossible? If so, this doesn't scale with dynamic machine creation.

My PIIX consolidation series and even more so my effort to make the VT82xx south bridges work with the PC machine are indeed bottom-up explorations of dynamic/flexible machine creation.

Best regards,
Bernhard

>Maybe the IRQ wiring should be another machine phase, after all
>devices are instantiated?
>
>Your approach is to create the 'IRQ proxy' first, like drawing the
>wires on a board, then plug the devices, like soldering chips on
>the printed board IRQs. So maybe devices shouldn't be the QOM owners
>of IRQs, the board should...
>
>Yeah, just thinking loudly...

Philippe Mathieu-Daudé Jan. 4, 2023, 8:31 p.m. UTC | #6

On 4/1/23 21:12, Bernhard Beschow wrote:
> 
> 
> Am 4. Januar 2023 16:35:57 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>> On 4/1/23 17:01, Bernhard Beschow wrote:
>>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>>> southbridges. This is especially useful for PIIX3 for two reasons:
>>>>> First, the southbridge doesn't need to care about the virtualization
>>>>> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
>>>>> attached) and out-IRQs (which will trigger the IRQs of the respective
>>>>> virtzalization technology) are separated. Second, since the in-IRQs are
>>
>> Typo "virtualization".
> 
> Fixed...


>>>> Why restrict to 16 and not use a class property and allocate
>>>> on the heap? See TYPE_SPLIT_IRQ for example.
>>>
>>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
>>>
>>> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.
>>
>> Oh indeed. Well this model as is is OK, but it could be more useful
>> if able to proxy any range of IRQs.
> 
> I've responded with a new, single patch to this patch. Is that okay or shall I respin the whole series? Is anything missing? IIUC we can make the proxy-pic dynamic in a follow-up?

I think we are good :) If you can point me to a branch with all your 
patches, I could verify everything is properly applied locally.

>> I have the feeling we are cycling around this IRQ proxy:
>>
>> 22ec3283ef ("irq: introduce qemu_irq_proxy()")
>> 078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
>> fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
>>
>> What is our problem? IRQ lines connect 2 devices in a fixed direction.
>> Current model expects one edge to be wired to a device before wiring
>> the other device, so device composition with IRQs in middle is
>> impossible? If so, this doesn't scale with dynamic machine creation.
> 
> My PIIX consolidation series and even more so my effort to make the VT82xx south bridges work with the PC machine are indeed bottom-up explorations of dynamic/flexible machine creation.

Yeah (I have been there too...). Also Mark Cave-Ayland confirmed
elsewhere in this thread that yourv effort points toward the right
direction :)

Regards,

Phil.

Bernhard Beschow Jan. 4, 2023, 8:57 p.m. UTC | #7

Am 4. Januar 2023 20:31:15 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>On 4/1/23 21:12, Bernhard Beschow wrote:
>> 
>> 
>> Am 4. Januar 2023 16:35:57 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>>> On 4/1/23 17:01, Bernhard Beschow wrote:
>>>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" <[e-mail]>:
>>>>> On 21/12/22 17:59, Bernhard Beschow wrote:
>>>>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>>>>> southbridges. This is especially useful for PIIX3 for two reasons:
>>>>>> First, the southbridge doesn't need to care about the virtualization
>>>>>> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
>>>>>> attached) and out-IRQs (which will trigger the IRQs of the respective
>>>>>> virtzalization technology) are separated. Second, since the in-IRQs are
>>> 
>>> Typo "virtualization".
>> 
>> Fixed...
>
>
>>>>> Why restrict to 16 and not use a class property and allocate
>>>>> on the heap? See TYPE_SPLIT_IRQ for example.
>>>> 
>>>> TYPE_SPLIT_IRQ doesn't allocate on the heap and instead has a hardcoded limit of MAX_SPLIT_LINES which equals 16 ;)
>>>> 
>>>> I was unsure on when to free the memory and how to dispose the elements so I went with this solution for simplicity. I'll look for inspitation in other device models and respin.
>>> 
>>> Oh indeed. Well this model as is is OK, but it could be more useful
>>> if able to proxy any range of IRQs.
>> 
>> I've responded with a new, single patch to this patch. Is that okay or shall I respin the whole series? Is anything missing? IIUC we can make the proxy-pic dynamic in a follow-up?
>
>I think we are good :) If you can point me to a branch with all your patches, I could verify everything is properly applied locally.

Sure, here we go: https://github.com/shentok/qemu/commits/piix-consolidate

Thanks for your help and for picking up this beast ;)

>
>>> I have the feeling we are cycling around this IRQ proxy:
>>> 
>>> 22ec3283ef ("irq: introduce qemu_irq_proxy()")
>>> 078778c5a5 ("piix4: Add an i8259 Interrupt Controller as specified in datasheet")
>>> fc531e7cab ("Revert "irq: introduce qemu_irq_proxy()"")
>>> 
>>> What is our problem? IRQ lines connect 2 devices in a fixed direction.
>>> Current model expects one edge to be wired to a device before wiring
>>> the other device, so device composition with IRQs in middle is
>>> impossible? If so, this doesn't scale with dynamic machine creation.
>> 
>> My PIIX consolidation series and even more so my effort to make the VT82xx south bridges work with the PC machine are indeed bottom-up explorations of dynamic/flexible machine creation.
>
>Yeah (I have been there too...).

I've seen it. Eventually I'll also pick up your work of eliminating the isabus global...

Best regards,
Bernhard

> Also Mark Cave-Ayland confirmed
>elsewhere in this thread that yourv effort points toward the right
>direction :)
>
>Regards,
>
>Phil.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 716d5a24ad..f862bfc7d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1675,6 +1675,7 @@  S: Supported
 F: hw/char/debugcon.c
 F: hw/char/parallel*
 F: hw/char/serial*
+F: hw/core/proxy-pic.c
 F: hw/dma/i8257*
 F: hw/i2c/pm_smbus.c
 F: hw/input/pckbd.c
@@ -1691,6 +1692,7 @@  F: hw/watchdog/wdt_ib700.c
 F: hw/watchdog/wdt_i6300esb.c
 F: include/hw/display/vga.h
 F: include/hw/char/parallel.h
+F: include/hw/core/proxy-pic.h
 F: include/hw/dma/i8257.h
 F: include/hw/i2c/pm_smbus.h
 F: include/hw/input/i8042.h
diff --git a/hw/core/Kconfig b/hw/core/Kconfig
index 9397503656..a7224f4ca0 100644
--- a/hw/core/Kconfig
+++ b/hw/core/Kconfig
@@ -22,6 +22,9 @@  config OR_IRQ
 config PLATFORM_BUS
     bool
 
+config PROXY_PIC
+    bool
+
 config REGISTER
     bool
 
diff --git a/hw/core/meson.build b/hw/core/meson.build
index 7a4d02b6c0..e86aef6ec3 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -30,6 +30,7 @@  softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.
 softmmu_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
 softmmu_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
 softmmu_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
+softmmu_ss.add(when: 'CONFIG_PROXY_PIC', if_true: files('proxy-pic.c'))
 softmmu_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
 softmmu_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
 softmmu_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
diff --git a/hw/core/proxy-pic.c b/hw/core/proxy-pic.c
new file mode 100644
index 0000000000..3251727d19
--- /dev/null
+++ b/hw/core/proxy-pic.c
@@ -0,0 +1,70 @@ 
+/*
+ * Proxy interrupt controller device.
+ *
+ * Copyright (c) 2022 Bernhard Beschow <[e-mail]>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/proxy-pic.h"
+
+static void proxy_pic_set_irq(void *opaque, int irq, int level)
+{
+    ProxyPICState *s = opaque;
+
+    qemu_set_irq(s->out_irqs[irq], level);
+}
+
+static void proxy_pic_realize(DeviceState *dev, Error **errp)
+{
+    ProxyPICState *s = PROXY_PIC(dev);
+
+    qdev_init_gpio_in(DEVICE(s), proxy_pic_set_irq, MAX_PROXY_PIC_LINES);
+    qdev_init_gpio_out(DEVICE(s), s->out_irqs, MAX_PROXY_PIC_LINES);
+
+    for (int i = 0; i < MAX_PROXY_PIC_LINES; ++i) {
+        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
+    }
+}
+
+static void proxy_pic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    /* No state to reset or migrate */
+    dc->realize = proxy_pic_realize;
+
+    /* Reason: Needs to be wired up to work */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo proxy_pic_info = {
+    .name          = TYPE_PROXY_PIC,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(ProxyPICState),
+    .class_init = proxy_pic_class_init,
+};
+
+static void split_irq_register_types(void)
+{
+    type_register_static(&proxy_pic_info);
+}
+
+type_init(split_irq_register_types)
diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
new file mode 100644
index 0000000000..0eb40c478a
--- /dev/null
+++ b/include/hw/core/proxy-pic.h
@@ -0,0 +1,54 @@ 
+/*
+ * Proxy interrupt controller device.
+ *
+ * Copyright (c) 2022 Bernhard Beschow <[e-mail]>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_PROXY_PIC_H
+#define HW_PROXY_PIC_H
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+#include "hw/irq.h"
+
+#define TYPE_PROXY_PIC "proxy-pic"
+OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
+
+#define MAX_PROXY_PIC_LINES 16
+
+/**
+ * This is a simple device which has 16 pairs of GPIO input and output lines.
+ * Any change on an input line is forwarded to the respective output.
+ *
+ * QEMU interface:
+ *  + 16 unnamed GPIO inputs: the input lines
+ *  + 16 unnamed GPIO outputs: the output lines
+ */
+struct ProxyPICState {
+    /*< private >*/
+    struct DeviceState parent_obj;
+    /*< public >*/
+
+    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
+    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
+};
+
+#endif /* HW_PROXY_PIC_H */