[Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Shannon Zhao]

When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
To DTB UEFI could call libfdt api to disable the RTC device node, but to
ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
device in QEMU when using UEFI.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/arm/virt-acpi-build.c         | 13 +++++++++++--
 hw/arm/virt.c                    |  5 ++++-
 include/hw/arm/virt-acpi-build.h |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 0caf5ce..cccec79 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -575,8 +575,17 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
     acpi_dsdt_add_cpus(scope, guest_info->smp_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
-    acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
-                      (irqmap[VIRT_RTC] + ARM_SPI_BASE));
+
+    /* When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
+     * To DTB UEFI could call libfdt api to disable the RTC device node, but to
+     * ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
+     * device here when using UEFI.
+     */
+    if (guest_info->acpi_rtc) {
+        acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
+                          (irqmap[VIRT_RTC] + ARM_SPI_BASE));
+    }
+
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index fd52b76..de12037 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1002,6 +1002,7 @@ static void machvirt_init(MachineState *machine)
     VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
     VirtGuestInfo *guest_info = &guest_info_state->info;
     char **cpustr;
+    bool firmware_loaded;
 
     if (!cpu_model) {
         cpu_model = "cortex-a15";
@@ -1124,12 +1125,14 @@ static void machvirt_init(MachineState *machine)
     create_fw_cfg(vbi, &address_space_memory);
     rom_set_fw(fw_cfg_find());
 
+    firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
     guest_info->smp_cpus = smp_cpus;
     guest_info->fw_cfg = fw_cfg_find();
     guest_info->memmap = vbi->memmap;
     guest_info->irqmap = vbi->irqmap;
     guest_info->use_highmem = vms->highmem;
     guest_info->gic_version = gic_version;
+    guest_info->acpi_rtc = !firmware_loaded;
     guest_info_state->machine_done.notify = virt_guest_info_machine_done;
     qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
 
@@ -1141,7 +1144,7 @@ static void machvirt_init(MachineState *machine)
     vbi->bootinfo.board_id = -1;
     vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
     vbi->bootinfo.get_dtb = machvirt_dtb;
-    vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
+    vbi->bootinfo.firmware_loaded = firmware_loaded;
     arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
 
     /*
diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
index 744b666..6f412a4 100644
--- a/include/hw/arm/virt-acpi-build.h
+++ b/include/hw/arm/virt-acpi-build.h
@@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
     const int *irqmap;
     bool use_highmem;
     int gic_version;
+    bool acpi_rtc;
 } VirtGuestInfo;
 
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Peter Maydell]

On 12 January 2016 at 15:24, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
> To DTB UEFI could call libfdt api to disable the RTC device node, but to
> ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
> device in QEMU when using UEFI.

I don't really understand this. I thought that if we were
using ACPI then we would always be doing it via UEFI?

Also I think if UEFI wants to take command of some of the
hardware it ought to be UEFI's job to adjust the tables
accordingly before it passes them on to the guest OS.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Shannon Zhao]

On 2016/1/12 23:30, Peter Maydell wrote:
> On 12 January 2016 at 15:24, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>> > When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
>> > To DTB UEFI could call libfdt api to disable the RTC device node, but to
>> > ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
>> > device in QEMU when using UEFI.
> I don't really understand this. I thought that if we were
> using ACPI then we would always be doing it via UEFI?
> 
Currently this is true and maybe for a long time this is also true.

> Also I think if UEFI wants to take command of some of the
> hardware it ought to be UEFI's job to adjust the tables
> accordingly before it passes them on to the guest OS.
Yes, the ideal method is adjusting the DSDT table in UEFI. But there is
almost no way to parse the DSDT table in UEFI. If we want to support
that it will introduce ACPI interpreter. This makes it more complex.

There is a discussion [1] about this on the edk2 list.

[1]https://www.mail-archive.com/edk2-devel@lists.01.org/msg06301.html

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Laszlo Ersek]

On 01/12/16 16:30, Peter Maydell wrote:
> On 12 January 2016 at 15:24, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>> When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
>> To DTB UEFI could call libfdt api to disable the RTC device node, but to
>> ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
>> device in QEMU when using UEFI.
> 
> I don't really understand this. I thought that if we were
> using ACPI then we would always be doing it via UEFI?

Yes.

Let my try to summarize here too:

- kernel booted without UEFI: consumes DTB, accesses RTC directly

- kernel booted with UEFI, consumes DTB: UEFI owns RTC, kernel uses UEFI
services, UEFI keeps kernel from directly accessing the RTC by disabling
the RTC node in the DTB, using libfdt

- kernel booted with UEFI, consumes ACPI: UEFI owns RTC, kernel uses
UEFI services, UEFI keeps kernel from directly accessing the RTC by...,
well, it can't, because we don't *parse* AML in UEFI.

> Also I think if UEFI wants to take command of some of the
> hardware it ought to be UEFI's job to adjust the tables
> accordingly before it passes them on to the guest OS.

In theory, maybe.

In practice, no; we have the ACPI linker/loader for that. Either the
generated AML must not contain the RTC node, or else some linker/loader
script command(s) have to be added that cause the guest firmware's
linker/loader client to patch the device out. Generally speaking
however, the linker/loader can only patch data tables, not definition
blocks (AML).

You might ask why the DTB is different then. Why aren't I suggesting, in
paralle, that the DTB generator behave similarly in QEMU? The answer is
that the firmware needs the RTC node in the DTB for its *own* purposes
as well, so the RTC node must be in the DTB in any case.

ACPI is different. The firmware downloads it, patches it blindly (=
processes the linker/loader script), then passes it to the OS. That's all.

Formatting AML is doable in the firmware; parsing / modifying AML that
was originally generated by QEMU is practically impossible. If you
recall the *original* introducion of the ACPI interpreter into the
kernel -- there was a huge uproar. Today Linux has a customized version
of the ACPI CA framework. edk2 doesn't, and shouldn't.

Plus, *intelligently* modifying AML in the firmware defeats the purpose
of the ACPI linker/loader -- which is to allow the firmware to remain
ignorant about ACPI.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Laszlo Ersek]

On 01/12/16 16:24, Shannon Zhao wrote:
> When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
> To DTB UEFI could call libfdt api to disable the RTC device node, but to
> ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
> device in QEMU when using UEFI.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c         | 13 +++++++++++--
>  hw/arm/virt.c                    |  5 ++++-
>  include/hw/arm/virt-acpi-build.h |  1 +
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0caf5ce..cccec79 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -575,8 +575,17 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>      acpi_dsdt_add_cpus(scope, guest_info->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> -    acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
> -                      (irqmap[VIRT_RTC] + ARM_SPI_BASE));
> +
> +    /* When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
> +     * To DTB UEFI could call libfdt api to disable the RTC device node, but to
> +     * ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
> +     * device here when using UEFI.
> +     */
> +    if (guest_info->acpi_rtc) {
> +        acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
> +                          (irqmap[VIRT_RTC] + ARM_SPI_BASE));
> +    }
> +
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index fd52b76..de12037 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1002,6 +1002,7 @@ static void machvirt_init(MachineState *machine)
>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
>      VirtGuestInfo *guest_info = &guest_info_state->info;
>      char **cpustr;
> +    bool firmware_loaded;
>  
>      if (!cpu_model) {
>          cpu_model = "cortex-a15";
> @@ -1124,12 +1125,14 @@ static void machvirt_init(MachineState *machine)
>      create_fw_cfg(vbi, &address_space_memory);
>      rom_set_fw(fw_cfg_find());
>  
> +    firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>      guest_info->smp_cpus = smp_cpus;
>      guest_info->fw_cfg = fw_cfg_find();
>      guest_info->memmap = vbi->memmap;
>      guest_info->irqmap = vbi->irqmap;
>      guest_info->use_highmem = vms->highmem;
>      guest_info->gic_version = gic_version;
> +    guest_info->acpi_rtc = !firmware_loaded;
>      guest_info_state->machine_done.notify = virt_guest_info_machine_done;
>      qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>  
> @@ -1141,7 +1144,7 @@ static void machvirt_init(MachineState *machine)
>      vbi->bootinfo.board_id = -1;
>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>      vbi->bootinfo.get_dtb = machvirt_dtb;
> -    vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> +    vbi->bootinfo.firmware_loaded = firmware_loaded;
>      arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>  
>      /*
> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
> index 744b666..6f412a4 100644
> --- a/include/hw/arm/virt-acpi-build.h
> +++ b/include/hw/arm/virt-acpi-build.h
> @@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
>      const int *irqmap;
>      bool use_highmem;
>      int gic_version;
> +    bool acpi_rtc;
>  } VirtGuestInfo;
>  
>  
> 

I realize that Peter is not buying the argument just yet, but I'd like
to offer a review here nonetheless.

I think the patch is good, except the location and the wording of the
code comment.

(1) The code comment should be located right above the

    guest_info->acpi_rtc = !firmware_loaded;

assignment.

(2) I think the code comment should simply use indicative mood:

    When booting the VM with UEFI, UEFI takes ownership of the RTC
    hardware. While UEFI can use libfdt to disable the RTC device node
    in the DTB that it passes to the OS, it cannot modify AML.
    Therefore, we won't generate the RTC ACPI device at all when using
    UEFI.

With those changes, I'm willing to R-b.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Ard Biesheuvel]

On 13 January 2016 at 11:18, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/12/16 16:24, Shannon Zhao wrote:
>> When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
>> To DTB UEFI could call libfdt api to disable the RTC device node, but to
>> ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
>> device in QEMU when using UEFI.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/arm/virt-acpi-build.c         | 13 +++++++++++--
>>  hw/arm/virt.c                    |  5 ++++-
>>  include/hw/arm/virt-acpi-build.h |  1 +
>>  3 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 0caf5ce..cccec79 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -575,8 +575,17 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>>      acpi_dsdt_add_cpus(scope, guest_info->smp_cpus);
>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>> -    acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
>> -                      (irqmap[VIRT_RTC] + ARM_SPI_BASE));
>> +
>> +    /* When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
>> +     * To DTB UEFI could call libfdt api to disable the RTC device node, but to
>> +     * ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
>> +     * device here when using UEFI.
>> +     */
>> +    if (guest_info->acpi_rtc) {
>> +        acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
>> +                          (irqmap[VIRT_RTC] + ARM_SPI_BASE));
>> +    }
>> +
>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index fd52b76..de12037 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1002,6 +1002,7 @@ static void machvirt_init(MachineState *machine)
>>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
>>      VirtGuestInfo *guest_info = &guest_info_state->info;
>>      char **cpustr;
>> +    bool firmware_loaded;
>>
>>      if (!cpu_model) {
>>          cpu_model = "cortex-a15";
>> @@ -1124,12 +1125,14 @@ static void machvirt_init(MachineState *machine)
>>      create_fw_cfg(vbi, &address_space_memory);
>>      rom_set_fw(fw_cfg_find());
>>
>> +    firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>>      guest_info->smp_cpus = smp_cpus;
>>      guest_info->fw_cfg = fw_cfg_find();
>>      guest_info->memmap = vbi->memmap;
>>      guest_info->irqmap = vbi->irqmap;
>>      guest_info->use_highmem = vms->highmem;
>>      guest_info->gic_version = gic_version;
>> +    guest_info->acpi_rtc = !firmware_loaded;
>>      guest_info_state->machine_done.notify = virt_guest_info_machine_done;
>>      qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>>
>> @@ -1141,7 +1144,7 @@ static void machvirt_init(MachineState *machine)
>>      vbi->bootinfo.board_id = -1;
>>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>>      vbi->bootinfo.get_dtb = machvirt_dtb;
>> -    vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>> +    vbi->bootinfo.firmware_loaded = firmware_loaded;
>>      arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>>
>>      /*
>> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
>> index 744b666..6f412a4 100644
>> --- a/include/hw/arm/virt-acpi-build.h
>> +++ b/include/hw/arm/virt-acpi-build.h
>> @@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
>>      const int *irqmap;
>>      bool use_highmem;
>>      int gic_version;
>> +    bool acpi_rtc;
>>  } VirtGuestInfo;
>>
>>
>>
>
> I realize that Peter is not buying the argument just yet, but I'd like
> to offer a review here nonetheless.
>

I am not buying it either, to be honest. In fact, I think it is
another reason why we should mandate UEFI when using ACPI (which is
already the case in practice). Then, we can simply omit the RTC ACPI
node entirely.


> I think the patch is good, except the location and the wording of the
> code comment.
>
> (1) The code comment should be located right above the
>
>     guest_info->acpi_rtc = !firmware_loaded;
>
> assignment.
>
> (2) I think the code comment should simply use indicative mood:
>
>     When booting the VM with UEFI, UEFI takes ownership of the RTC
>     hardware. While UEFI can use libfdt to disable the RTC device node
>     in the DTB that it passes to the OS, it cannot modify AML.
>     Therefore, we won't generate the RTC ACPI device at all when using
>     UEFI.
>
> With those changes, I'm willing to R-b.
>
> Thanks
> Laszlo

Re: [Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Laszlo Ersek]

On 01/13/16 11:20, Ard Biesheuvel wrote:
> On 13 January 2016 at 11:18, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/12/16 16:24, Shannon Zhao wrote:
>>> When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
>>> To DTB UEFI could call libfdt api to disable the RTC device node, but to
>>> ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
>>> device in QEMU when using UEFI.
>>>
>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>> ---
>>>  hw/arm/virt-acpi-build.c         | 13 +++++++++++--
>>>  hw/arm/virt.c                    |  5 ++++-
>>>  include/hw/arm/virt-acpi-build.h |  1 +
>>>  3 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 0caf5ce..cccec79 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -575,8 +575,17 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>>>      acpi_dsdt_add_cpus(scope, guest_info->smp_cpus);
>>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>>> -    acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
>>> -                      (irqmap[VIRT_RTC] + ARM_SPI_BASE));
>>> +
>>> +    /* When booting VM through UEFI, UEFI takes ownership of the RTC hardware.
>>> +     * To DTB UEFI could call libfdt api to disable the RTC device node, but to
>>> +     * ACPI it couldn't do that. Therefore, we don't generate the RTC ACPI
>>> +     * device here when using UEFI.
>>> +     */
>>> +    if (guest_info->acpi_rtc) {
>>> +        acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
>>> +                          (irqmap[VIRT_RTC] + ARM_SPI_BASE));
>>> +    }
>>> +
>>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>>>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>>>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index fd52b76..de12037 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1002,6 +1002,7 @@ static void machvirt_init(MachineState *machine)
>>>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
>>>      VirtGuestInfo *guest_info = &guest_info_state->info;
>>>      char **cpustr;
>>> +    bool firmware_loaded;
>>>
>>>      if (!cpu_model) {
>>>          cpu_model = "cortex-a15";
>>> @@ -1124,12 +1125,14 @@ static void machvirt_init(MachineState *machine)
>>>      create_fw_cfg(vbi, &address_space_memory);
>>>      rom_set_fw(fw_cfg_find());
>>>
>>> +    firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>>>      guest_info->smp_cpus = smp_cpus;
>>>      guest_info->fw_cfg = fw_cfg_find();
>>>      guest_info->memmap = vbi->memmap;
>>>      guest_info->irqmap = vbi->irqmap;
>>>      guest_info->use_highmem = vms->highmem;
>>>      guest_info->gic_version = gic_version;
>>> +    guest_info->acpi_rtc = !firmware_loaded;
>>>      guest_info_state->machine_done.notify = virt_guest_info_machine_done;
>>>      qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>>>
>>> @@ -1141,7 +1144,7 @@ static void machvirt_init(MachineState *machine)
>>>      vbi->bootinfo.board_id = -1;
>>>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>>>      vbi->bootinfo.get_dtb = machvirt_dtb;
>>> -    vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>>> +    vbi->bootinfo.firmware_loaded = firmware_loaded;
>>>      arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>>>
>>>      /*
>>> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
>>> index 744b666..6f412a4 100644
>>> --- a/include/hw/arm/virt-acpi-build.h
>>> +++ b/include/hw/arm/virt-acpi-build.h
>>> @@ -33,6 +33,7 @@ typedef struct VirtGuestInfo {
>>>      const int *irqmap;
>>>      bool use_highmem;
>>>      int gic_version;
>>> +    bool acpi_rtc;
>>>  } VirtGuestInfo;
>>>
>>>
>>>
>>
>> I realize that Peter is not buying the argument just yet, but I'd like
>> to offer a review here nonetheless.
>>
> 
> I am not buying it either, to be honest. In fact, I think it is
> another reason why we should mandate UEFI when using ACPI (which is
> already the case in practice). Then, we can simply omit the RTC ACPI
> node entirely.

Good point.

Laszlo

> 
> 
>> I think the patch is good, except the location and the wording of the
>> code comment.
>>
>> (1) The code comment should be located right above the
>>
>>     guest_info->acpi_rtc = !firmware_loaded;
>>
>> assignment.
>>
>> (2) I think the code comment should simply use indicative mood:
>>
>>     When booting the VM with UEFI, UEFI takes ownership of the RTC
>>     hardware. While UEFI can use libfdt to disable the RTC device node
>>     in the DTB that it passes to the OS, it cannot modify AML.
>>     Therefore, we won't generate the RTC ACPI device at all when using
>>     UEFI.
>>
>> With those changes, I'm willing to R-b.
>>
>> Thanks
>> Laszlo

Re: [Qemu-devel] [PATCH] ARM: Virt: Don't generate RTC ACPI node when using UEFI

[Peter Maydell]

On 13 January 2016 at 10:48, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/13/16 11:20, Ard Biesheuvel wrote:
>> I am not buying it either, to be honest. In fact, I think it is
>> another reason why we should mandate UEFI when using ACPI (which is
>> already the case in practice). Then, we can simply omit the RTC ACPI
>> node entirely.
>
> Good point.

Yes, a patch that simply removed the RTC device from the
ACPI table altogether would I think be better. That then
continues with the current approach that the tables provided
by QEMU are intended for use only with a UEFI bios in
the picture.

thanks
-- PMM