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. > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel