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

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

[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

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);
 }
-- 
2.21.3

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);
>  }
> 

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

[Philippe Mathieu-Daudé]

On 7/20/20 9:45 PM, Michael Tokarev wrote:
> 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

This is better to find the full list:

$ git grep -l isa_register_ioport
hw/audio/cs4231a.c
hw/audio/pcspk.c
hw/char/serial-isa.c
hw/i386/port92.c
hw/i386/vmport.c
hw/input/pckbd.c
hw/intc/i8259_common.c
hw/ipmi/isa_ipmi_bt.c
hw/ipmi/isa_ipmi_kcs.c
hw/isa/isa-bus.c
hw/isa/pc87312.c
hw/misc/applesmc.c
hw/misc/pvpanic.c
hw/net/ne2000-isa.c
hw/rtc/m48t59-isa.c
hw/rtc/mc146818rtc.c
hw/timer/i8254_common.c
include/hw/isa/isa.h

> 
> Thanks,
> 
> /mjt

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

[Michael Tokarev]

21.07.2020 15:29, Philippe Mathieu-Daudé wrote:
> On 7/20/20 9:45 PM, Michael Tokarev wrote:
...
>> 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.

Note: all of the entries below have min_access_size = max_access_size = 0
I dunno what it gives us, but it is a different issue.

>> 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
> 
> This is better to find the full list:

I found no other entries which restrict access to >1.

/mjt

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

[Michael S. Tsirkin]

On Mon, Jul 20, 2020 at 08:57:58PM +0200, Philippe Mathieu-Daudé wrote:
> 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.

Right, but I am not sure device is guaranteed to do something
sensible if the CPU does it.

Any examples where that is the case?

> 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


These all have to do with ACPI that Michael fixed, right?

> 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);
>  }
> -- 
> 2.21.3