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

[Bernhard Beschow]

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
>> 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?

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