| Message ID | 1452612274-30218-1-git-send-email-shannon.zhao@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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;
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(-) -- 2.1.0