Re: [RFC PATCH-not-for-5.1? v2] hw/isa/isa-bus: Ensure ISA I/O regions are 8/16-bit accessible

[Michael Tokarev]

20.07.2020 21:57, Philippe Mathieu-Daudé пишет:
> Since commit 5d971f9e67 we don't accept mismatching sizes
> in memory_region_access_valid(). This gives troubles when
> a device is on an ISA bus, because the CPU is free to use
> 8/16-bit accesses on the bus (or up to 32-bit on EISA bus),
> regardless what range is valid for the device.
> 
> Add a check to ensure devices plugged on the ISA bus can
> accept 8/16-bits accesses.
> 
> Related bug reports:
> 
> - 
> https://lore.kernel.org/xen-devel/20200630170913.123646-1-anthony.perard@citrix.com/T/
> - https://bugs.debian.org/964793
> - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964247
> - https://bugs.launchpad.net/bugs/1886318

Here's the output (of a similar patch), after I fixed the 3 acpi core
devies, of one of my windows7 test VMs. I guess we need to fix either
these devices or the registration, before 5.1 is out, or else more
difficult-to-catch breakage like the above will pop up..

For now we don't have any released qemu version with this situation
so not many project enabled workarounds for broken qemu behavour
like the xen-devel link above.

qemu-system-x86_64: ISA device 'kvm-i8259' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'kvm-i8259' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'kvm-i8259' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'kvm-i8259' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'mc146818rtc' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'kvm-pit' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'isa-pcspk' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'isa-serial' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'i8042' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'i8042' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'vmport' requires I/O max_access_size of 2
qemu-system-x86_64: ISA device 'port92' requires I/O max_access_size of 2

Thanks,

/mjt

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> MST: I really don't like this approach, I think the ISA bus
>      should adjust the access.
> 
> since v1: only 8/16-bit accesses enforced
> ---
>  hw/isa/isa-bus.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 58fde178f9..e142eeef06 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -132,6 +132,20 @@ static inline void isa_init_ioport(ISADevice *dev, 
> uint16_t ioport)
>  
>  void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
>  {
> +    if (io->ops->valid.min_access_size > 1) {
> +        /*
> +         * To be backward compatible with IBM-PC bus, ISA bus must accept
> +         * 8-bit accesses.
> +         */
> +        error_report("ISA device '%s' requires I/O min_access_size of 1 
> (byte)",
> +                     object_get_typename(OBJECT(dev)));
> +        exit(1);
> +    } else if (io->ops->valid.max_access_size < 2) {
> +        /* ISA bus must accept 16-bit accesses (EISA accepts 32-bit) */
> +        error_report("ISA device '%s' requires I/O max_access_size of "
> +                     "at least 2 (bytes)", object_get_typename(OBJECT(dev)));
> +        exit(1);
> +    }
>      memory_region_add_subregion(isabus->address_space_io, start, io);
>      isa_init_ioport(dev, start);
>  }
>