On 01/13/16 11:20, Ard Biesheuvel wrote: > On 13 January 2016 at 11:18, Laszlo Ersek <ler...@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.z...@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