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

[Bernhard Beschow]

Am 4. Januar 2023 16:35:57 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>On 4/1/23 17:01, Bernhard Beschow wrote:
>> Am 4. Januar 2023 14:37:29 UTC schrieb "Philippe Mathieu-Daudé" 
>> <philmd@linaro.org>:
>>> 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 <shentey@gmail.com>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>>>> ---
>>>>    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 <shentey@gmail.com>
>>>> + *
>>>> + * 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...