Re: [Qemu-devel] [Qemu-arm] [PATCH] virt: Suppress external aborts on virt-2.10 and earlier

[Philippe Mathieu-Daudé]

Hi Peter,

On 9/25/18 4:41 PM, Peter Maydell wrote:
> In commit c79c0a314c43b78 we enabled emulation of external aborts
> when the guest attempts to access a physical address with no
> mapped device. In commit 4672cbd7bed88dc6 we suppress this for
> most legacy boards to prevent breakage of previously working
> guests, but we didn't suppress it in the 'virt' board, with
> the rationale "we know that guests won't try to prod devices
> that we don't describe in the device tree or ACPI tables". This
> is mostly true, but we've had a report of a Linux guest image
> that this did break. The problem seems to be that the guest
> is (incorrectly) configured with a DEBUG_UART_PHYS value that
> tells it there is a uart at 0x10009000 (which is true for
> vexpress but not for virt), so in early bootup the kernel
> probes this bogus address.
> 
> This is a misconfigured guest, so we don't need to worry
> about it too much, but we can arrange that guests that ran
> on QEMU v2.10 (before c79c0a314c43b78) will still run on
> the "virt-2.10" board model, by suppressing external aborts
> only for that version and earlier. This seems a reasonable
> compromise.

I tried another approach to keep MEMTX_DECODE_ERROR delivered (out of
0x10009000) for 2.10, using the UNIMP device, but the code looks uglier
(create too much generic code for a single issue):

-- >8 --

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 4cc57a7ef6..217c9de6f6 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -101,6 +101,7 @@ typedef struct {
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
     bool no_highmem_ecam;
+    bool misconfigured_guest_debug_uart;
 } VirtMachineClass;

 typedef struct {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0b57f87abc..29bc9b9feb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -59,6 +59,7 @@
 #include "qapi/visitor.h"
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
+#include "hw/misc/unimp.h"

 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -1532,6 +1533,10 @@ static void machvirt_init(MachineState *machine)
     fdt_add_pmu_nodes(vms);

     create_uart(vms, pic, VIRT_UART, sysmem, serial_hd(0));
+    if (vmc->misconfigured_guest_debug_uart) {
+        /* kludge ... */
+        create_unimplemented_device("phantom-pl011", 0x10009000, 0x1000);
+    }

     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem);
@@ -1924,8 +1929,11 @@ static void virt_2_10_instance_init(Object *obj)

 static void virt_machine_2_10_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_2_11_options(mc);
     SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_10);
+    vmc->misconfigured_guest_debug_uart = true;
 }
 DEFINE_VIRT_MACHINE(2, 10)

-- 

I prefer your compromise.

> 
> Cc: address@hidden
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/arm/virt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 0b57f87abcb..3ba310a37b6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1926,6 +1926,7 @@ static void virt_machine_2_10_options(MachineClass *mc)
>  {
>      virt_machine_2_11_options(mc);
>      SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_10);

Can you add a short comment here?

       /* See https://bugs.launchpad.net/qemu/+bug/???:
        * Some misconfigured Linux guest poke for vexpress uart
        * at 0x10009000 */

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> +    mc->ignore_memory_transaction_failures = true;
>  }
>  DEFINE_VIRT_MACHINE(2, 10)
>