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

[Laszlo Ersek <lersek@redhat.com>]

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