Re: [PATCH v2 3/8] hw/mips/Kconfig: Remove ISA dependencies from MIPSsim board

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 8/2/24 09:11, Paolo Bonzini wrote:
> On Wed, Feb 7, 2024 at 7:58 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>> @@ -6,8 +6,7 @@ config MALTA
>>>
>>>    config MIPSSIM
>>>        bool
>>> -    select ISA_BUS
>>> -    select SERIAL_ISA
>>> +    select SERIAL
>>
>> Hmm there is an ISA bus which can be exposed with:
>>
>> -- >8 --
>> diff --git a/hw/mips/mipssim.c b/hw/mips/mipssim.c
>> index 16af31648e..a1a4688861 100644
>> --- a/hw/mips/mipssim.c
>> +++ b/hw/mips/mipssim.c
>> @@ -209,8 +209,9 @@ mips_mipssim_init(MachineState *machine)
>>        /* Register 64 KB of ISA IO space at 0x1fd00000. */
>>        memory_region_init_alias(isa, NULL, "isa_mmio",
>>                                 get_system_io(), 0, 0x00010000);
>>        memory_region_add_subregion(get_system_memory(), 0x1fd00000, isa);
>> +    isa_bus_new(NULL, get_system_memory(), get_system_io(), &error_abort);
> 
> Quoting you from
> https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg08752.html,
> "there is an ISA MMIO space mapped at 0x1fd00000, however this is not
> a real ISA bus (no ISA IRQ)".
> 
> If mipssim cannot support "-device isa-serial" as a replacement for
> "-serial", there's no reason for it to expose the bus. In the end,
> -device support is the main thing that an ISA bus provides over
> sysbus, and if it cannot work due to the missing interrupts, I think
> this patch is correct.
> 
> I can add a comment:
> 
> diff --git a/hw/mips/mipssim.c b/hw/mips/mipssim.c
> index 01e323904d9..47b37b454e7 100644
> --- a/hw/mips/mipssim.c
> +++ b/hw/mips/mipssim.c
> @@ -204,7 +204,11 @@ mips_mipssim_init(MachineState *machine)
>       cpu_mips_irq_init_cpu(cpu);
>       cpu_mips_clock_init(cpu);
> 
> -    /* Register 64 KB of ISA IO space at 0x1fd00000. */
> +    /*
> +     * Register 64 KB of ISA IO space at 0x1fd00000.  But without interrupts
> +     * (except for the hardcoded serial port interrupt) -device cannot work,
> +     * so do not expose the ISA bus to the user.
> +     */

Yes, thanks!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>       memory_region_init_alias(isa, NULL, "isa_mmio",
>                                get_system_io(), 0, 0x00010000);
>       memory_region_add_subregion(get_system_memory(), 0x1fd00000, isa);
> 
> Paolo
> 
>>        /*
>>         * A single 16450 sits at offset 0x3f8. It is attached to
>>         * MIPS CPU INT2, which is interrupt 4.
>> ---
>>
>