Re: [PATCH v2 1/3] vt82c686.c: don't raise SCI when PCI_INTERRUPT_PIN isn't setup

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 4255 bytes --]

On Tue, 23 Mar 2021, Philippe Mathieu-Daudé wrote:
> Hi Isaku,
>
> On 3/23/21 6:24 PM, Isaku Yamahata wrote:
>> Without this patch, the following patch will triger clan runtime
>> sanitizer warnings as follows. This patch proactively works around it.
>> I let v582c686.c maintainer address a correct fix as I'm not sure
>> about fuloong2e device model.
>>
>>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
>>> QTEST_QEMU_IMG=./qemu-img
>>> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
>>> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/qom-test --tap -k
>>> PASS 1 qtest-mips64el/qom-test /mips64el/qom/loongson3-virt
>>> PASS 2 qtest-mips64el/qom-test /mips64el/qom/none
>>> PASS 3 qtest-mips64el/qom-test /mips64el/qom/magnum
>>> PASS 4 qtest-mips64el/qom-test /mips64el/qom/mipssim
>>> PASS 5 qtest-mips64el/qom-test /mips64el/qom/malta
>>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
>>> PASS 6 qtest-mips64el/qom-test /mips64el/qom/fuloong2e
>>> PASS 7 qtest-mips64el/qom-test /mips64el/qom/boston
>>> PASS 8 qtest-mips64el/qom-test /mips64el/qom/pica61
>>>
>>> and similarly for eg
>>>
>>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
>>> QTEST_QEMU_IMG=./qemu-img
>>> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
>>> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/endianness-test
>>> --tap -k
>>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
>>> PASS 1 qtest-mips64el/endianness-test /mips64el/endianness/fuloong2e
>>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
>>> PASS 2 qtest-mips64el/endianness-test /mips64el/endianness/split/fuloong2e
>>> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
>>> PASS 3 qtest-mips64el/endianness-test /mips64el/endianness/combine/fuloong2e
>>
>> Cc: Huacai Chen <chenhuacai@kernel.org>
>> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
>> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> Reported-by: Peter Maydell <Peter.maydel@linaro.org>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> ---
>>  hw/isa/vt82c686.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 05d084f698..f0fb309f12 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -144,7 +144,18 @@ static void pm_update_sci(ViaPMState *s)
>>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>>                     ACPI_BITMASK_TIMER_ENABLE)) != 0);
>> -    pci_set_irq(&s->dev, sci_level);
>> +    if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) {
>> +        /*
>> +         * FIXME:
>> +         * Fix device model that realizes this PM device and remove
>> +         * this work around.
>> +         * The device model should wire SCI and setup
>> +         * PCI_INTERRUPT_PIN properly.
>> +         * If PIN# = 0(interrupt pin isn't used), don't raise SCI as
>> +         * work around.
>> +         */
>> +        pci_set_irq(&s->dev, sci_level);
>
> I'll defer this to Zoltan.

I don't know anything about this, this was there well before I've touched 
this device model:

https://git.qemu.org/?p=qemu.git;a=blame;f=hw/isa/vt82c686.c;hb=8063396bf3459a810d24e3efd6110b8480f0de5b

> Personally I wouldn't care about SCI_EN on the vt82c686, as
> it is not used by x86 machines (IOW, I'd not modify via_pm_reset
> and KISS).

I'm not sure but maybe then you could also just remove the PM parts from 
the device model as it probably does not work correctly anyway at the 
moment as it may not be correctly wired up to config registers. I'm not 
sure if it's needed by any guests but it was there for some reason and 
maybe better to fix it if possible than dropping it. As a workaround I'm 
OK with the proposed patch, I don't think it would break anything but 
haven't tested it.

Regards,
BALATON Zoltan

>> +    }
>>      /* schedule a timer interruption if needed */
>>      acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
>>                         !(pmsts & ACPI_BITMASK_TIMER_STATUS));
>>
>
>