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

[Mark Cave-Ayland]

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é" 
> > <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".
>
> > > > 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.
>
> > > > + */
> > > > +
> > > > +#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.