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 <address@hidden> 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 <address@hidden>
>> ---
>>  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