[RFC 1/1] hw/input: add basic support for a PCI PS/2 controller
The linux pcips2 driver is responsible for driving this device.
Signed-off-by: Liav Albani
---
hw/i386/Kconfig | 1 +
hw/input/Kconfig | 5 +
hw/input/meson.build | 2 +
hw/input/ps2pci.c | 282 ++
include/hw/input/ps2pci.h | 52 +++
5 files changed, 342 insertions(+)
create mode 100644 hw/input/ps2pci.c
create mode 100644 include/hw/input/ps2pci.h
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d40802d..dad6188 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -36,6 +36,7 @@ config PC
select I8259
select I8254
select PCKBD
+select PS2PCI
select PCSPK
select I8257
select MC146818RTC
diff --git a/hw/input/Kconfig b/hw/input/Kconfig
index 55865bb..04c97e8 100644
--- a/hw/input/Kconfig
+++ b/hw/input/Kconfig
@@ -17,6 +17,11 @@ config PL050
bool
select PS2
+config PS2PCI
+bool
+depends on PCI
+select PS2
+
config PS2
bool
diff --git a/hw/input/meson.build b/hw/input/meson.build
index 8deb011..4fa1462 100644
--- a/hw/input/meson.build
+++ b/hw/input/meson.build
@@ -16,3 +16,5 @@ softmmu_ss.add(when: 'CONFIG_VHOST_USER_INPUT', if_true:
files('vhost-user-input
softmmu_ss.add(when: 'CONFIG_PXA2XX', if_true: files('pxa2xx_keypad.c'))
softmmu_ss.add(when: 'CONFIG_TSC210X', if_true: files('tsc210x.c'))
softmmu_ss.add(when: 'CONFIG_LASIPS2', if_true: files('lasips2.c'))
+
+softmmu_ss.add(when: 'CONFIG_PS2PCI', if_true: files('ps2pci.c'))
diff --git a/hw/input/ps2pci.c b/hw/input/ps2pci.c
new file mode 100644
index 000..4340af1
--- /dev/null
+++ b/hw/input/ps2pci.c
@@ -0,0 +1,282 @@
+/*
+ * QEMU PCI PS/2 adapter.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/units.h"
+#include "hw/pci/pci_device.h"
+#include "hw/input/ps2pci.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "hw/irq.h"
+#include "qapi/error.h"
+#include "qom/object.h"
+#include "qemu/log.h"
+
+static const VMStateDescription vmstate_ps2_pci = {
+.name = "ps2-pci",
+.fields = (VMStateField[]) {
+VMSTATE_PCI_DEVICE(parent_obj, PS2PCIState),
+VMSTATE_END_OF_LIST()
+}
+};
+
+#define PS2_CTRL (0)
+#define PS2_STATUS (1)
+#define PS2_DATA (2)
+
+#define PS2_CTRL_CLK (1<<0)
+#define PS2_CTRL_DAT (1<<1)
+#define PS2_CTRL_TXIRQ (1<<2)
+#define PS2_CTRL_ENABLE(1<<3)
+#define PS2_CTRL_RXIRQ (1<<4)
+
+#define PS2_STAT_CLK (1<<0)
+#define PS2_STAT_DAT (1<<1)
+#define PS2_STAT_PARITY(1<<2)
+#define PS2_STAT_RXFULL(1<<5)
+#define PS2_STAT_TXBUSY(1<<6)
+#define PS2_STAT_TXEMPTY (1<<7)
+
+static void ps2_pci_update_irq(PS2PCIState *s)
+{
+int level = (s->pending && (s->cr & PS2_CTRL_RXIRQ) != 0)
+ || (s->cr & PS2_CTRL_TXIRQ) != 0;
+
+qemu_set_irq(s->irq, level);
+}
+
+static void ps2_pci_set_irq(void *opaque, int n, int level)
+{
+PS2PCIState *s = (PS2PCIState *)opaque;
+
+s->pending = level;
+ps2_pci_update_irq(s);
+}
+
+static uint64_t ps2_pci_io_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+PS2PCIState *s = (PS2PCIState*)opaque;
+switch (offset) {
+case PS2_CTRL: /* CTRL */
+return s->cr;
+case PS2_STATUS: /* STATUS */
+{
+uint32_t stat = 0;
+if (s->pending)
+stat = PS2_STAT_RXFULL;
+else
+stat = PS2_STAT_TXEMPTY;
+uint8_t val = 0;
+val = s->last;
+val = val ^ (val >> 4);
+val = val ^ (val >> 2);
+val = (val ^ (val >> 1)) & 1;
+if (val) {
+stat |= PS2_STAT_PARITY;
+}
+return stat;
+}
+case PS2_DATA: /* DATA */
+if (s->pending && s->cr & PS2_CTRL_ENABLE) {
+s->last = ps2_read_data(s->ps2dev);
+if (ps2_queue_empty(s->ps2dev))
+s->pending = 0;
+} else {
+return 0;
+}
+return s->last;
+default:
+qemu_log_mask(LOG_GUEST_ERROR,
+ "ps2_pci_io_read: Bad offset %x\n", (int)offset);
+return 0;
+}
+}
+
+static void ps2_pci_io_write(void *opaque, hwaddr offset,
+uint64_t value, unsigned size)
+{
+PS2PCIState *s = (PS2PCIState *)opaque;
+switch (offset) {
+case PS2_CTRL: /*
[RFC 0/1] hw/input: add basic support for a PCI PS/2 controller
This small patch I wrote might raise few questions on the motivation to why someone would write such thing like this - I'll try my best to be descriptive about this. I am SerenityOS developer. In that project I mainly work on kernel stuff for the most part. Recently I started to fix issues with the HID code in the SerenityOS kernel and I stumbled upon fixing some issues with our PS/2 abstractions in relation to hardware implementations (such as the i8042) and the rest of the HID subsystem. I figured that it would be nice that with strong abstractions in place that I could write more drivers for other PS2 serial IO controllers. That's where I started to look into the Linux source code and I found that sometime in the past, someone wrote a driver for PS/2 controller, which is connected over the PCI bus. This driver is called pcips2 and almost nobody touched it since its initial write in 2003 (by now it's 20 years old, so getting hardware to test it might be even harder today!). Therefore, my goal was to see if I could actually use that driver, and so I wrote this small patch for letting QEMU to emulate that very old PS/2 PCI card. If this ever goes into the tree, a SerenityOS kernel driver could be written too, and will probably be used someday in our upcoming aarch64 port (probably for the virt machine as it supports the PCI bus). As for what it gives, keyboard works perfectly fine. I had some struggle with interrupts initially (mainly getting "bogus" interrupts). As for the mouse support - I tried to boot Manjaro live-CD on QEMU with either both of the "pci-ps2-keyboard" and "pci-ps2-mouse" devices and also with only "pci-ps2-mouse" - both options resulted in a kernel panic a few seconds after booting. The panic seemed to be some sort of nullptr dereference in the linux PS/2 core code, which I still need to debug. In general, this potentially could benefit multiple projects, which is the best case I'd imagine - more emulation options for QEMU, more drivers for SerenityOS, and maybe even fixing issues in PS/2 core (in Linux) and improving the pcips2 driver code in Linux as well. Liav Albani (1): hw/input: add basic support for a PCI PS/2 controller hw/i386/Kconfig | 1 + hw/input/Kconfig | 5 + hw/input/meson.build | 2 + hw/input/ps2pci.c | 282 ++ include/hw/input/ps2pci.h | 52 +++ 5 files changed, 342 insertions(+) create mode 100644 hw/input/ps2pci.c create mode 100644 include/hw/input/ps2pci.h -- 2.40.1
Re: [PATCH 0/1] hw/ide: share bmdma read and write functions
On 1/16/23 22:29, John Snow wrote: On Fri, Jan 13, 2023 at 9:10 AM Liav Albani wrote: On 1/11/23 01:07, Bernhard Beschow wrote: Am 9. Januar 2023 19:24:16 UTC schrieb John Snow : On Tue, Sep 6, 2022 at 10:27 AM Bernhard Beschow wrote: Am 19. Februar 2022 08:08:17 UTC schrieb Liav Albani : This is a preparation before I send v3 of ich6-ide controller emulation patch. I figured that it's more trivial to split the changes this way, by extracting the bmdma functions from via.c and piix.c and sharing them together. Then, I could easily put these into use when I send v3 of the ich6-ide patch by just using the already separated functions. This was suggested by BALATON Zoltan when he submitted a code review on my ich6-ide controller emulation patch. Ping. Any news? *cough*. Has this been folded into subsequent series, or does this still need attention? Both piix and via still have their own bmdma implementations. This patch might be worth having. Best regards, Bernhard I see. Since you are still interested, I will try to see what was the outcome of that patch as I really don't remember if it passed the CI tests, etc. If applicable, I will send this as v2, or if it's already approved, then I guess we could just let it be merged to the tree? I was just going to run some smoke tests on it and as long as it didn't hurt anything, I'd wave it in. If you want it alongside other patches that I also should stage, you can bundle them if you'd like. Just let me know what you plan on doing. --js For now I rather let the BMDMA patches (sharing read & write functions) to get in, and work on the ich6-ide patches later. Thank you for picking this (I myself forgot about that)! Let me know about any problem being raised in the tests that you mentioned you will do for this patch. Best regards, Liav
Re: [PATCH 0/1] hw/ide: share bmdma read and write functions
On 1/11/23 01:07, Bernhard Beschow wrote: Am 9. Januar 2023 19:24:16 UTC schrieb John Snow : On Tue, Sep 6, 2022 at 10:27 AM Bernhard Beschow wrote: Am 19. Februar 2022 08:08:17 UTC schrieb Liav Albani : This is a preparation before I send v3 of ich6-ide controller emulation patch. I figured that it's more trivial to split the changes this way, by extracting the bmdma functions from via.c and piix.c and sharing them together. Then, I could easily put these into use when I send v3 of the ich6-ide patch by just using the already separated functions. This was suggested by BALATON Zoltan when he submitted a code review on my ich6-ide controller emulation patch. Ping. Any news? *cough*. Has this been folded into subsequent series, or does this still need attention? Both piix and via still have their own bmdma implementations. This patch might be worth having. Best regards, Bernhard I see. Since you are still interested, I will try to see what was the outcome of that patch as I really don't remember if it passed the CI tests, etc. If applicable, I will send this as v2, or if it's already approved, then I guess we could just let it be merged to the tree? Best regards, Liav Liav Albani (1): hw/ide: share bmdma read and write functions between piix.c and via.c hw/ide/pci.c | 47 hw/ide/piix.c| 50 ++- hw/ide/via.c | 51 ++-- include/hw/ide/pci.h | 4 4 files changed, 55 insertions(+), 97 deletions(-)
Re: [PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers
On 9/21/22 09:14, Gerd Hoffmann wrote: Nope. Even if you fix the framebuffer address conflict you still have the io address conflict. Yeah, that is why I explicitly said that this is needed to be fixed as well in later patches. Yep. That's why isa-pc is pretty much unused these days. Well, I can't say I use it frequently, but I do test it with the SerenityOS kernel and it works pretty well. The SerenityOS kernel is able to drive an isa-vga device just fine after my patches were merged yesterday (in the GitHub pull request I provided a link to), so I do see that machine type as a valuable test platform. When you want build a sysbus variant of the bochs-display device and make that discoverable by the guest somehow (dt or acpi) you can expose both io ports and framebuffer address that way. No need to touch the bochs dispi interface for that. This is an interesting idea. A sysbus-bochs-display device might work well as you suggested, instead of using an isa-vga device. This idea is quite neat in my opinion, because it could speed up the boot of such VM while still providing sufficient display capabilities for those we need them. It could help developers to test their OSes on such hardware setups to ensure multi-monitor configuration works reliably when there's no PCI bus at all but many framebuffer devices being used in one VM. Why not just use virtio-gpu? Trying to run this command: qemu-system-x86_64 -M microvm -m 2048 -device virtio-gpu Results in: qemu-system-x86_64: -device virtio-gpu: No 'PCI' bus found for device 'virtio-gpu-pci' The idea was to not use PCI at all but still to have multiple framebuffer devices. So clearly virtio-gpu is not usable in this situation. 2. This is more related to the SerenityOS project - I prefer to not hardcode physical addresses at all wherever I can do so. This makes the code cleaner and more understandable as far as I observe this from the angle of the person which is not me, that tries to make sense from the code flow. Yea, fully agree, but why continue to use non-discoverable isa bus devices then? On the ISA-PC machine, I still want to be able to boot into a graphical environment with the SerenityOS kernel. The only option is to use the isa-vga device, which works OK. On the microvm machine, it is really not that important if I use the isa-vga device or a sysbus-bochs-display device (when I implement that device). I just want to support as many x86 platform configurations as possible - modern non-PCI machines, ISA-PC machines and regular i440fx/Q35 machines. 3. The costs of adding this feature are pretty negligible compared to the possible value of this, especially if we apply the idea of running multiple ISA-VGA devices on one microvm machine. Still, the only major "issue" that one can point to is the fact that I bump up the Bochs VBE version number, which could be questionable with how the feature might be insignificant for many guest OSes out there. Touching isa-vga at this point doesn't make sense at all. We simply can't move around the framebuffer without screwing up users. That's an issue I didn't consider, but this is not a real problem on the microvm machine if you use the device tree approach to expose the resources of the device, which in some sense makes it unnecessary to use the bochs dispi interface to expose the framebuffer physical address. Also I don't see any actual value in this. Even considering the multiple devices case the patch is a partial solution only (handles the framebuffer but not the ioports) which is pointless. Considering the possibility of exposing the framebuffer address within the device tree blob, it is indeed not making more value to go with this approach. I'm still fond of the idea to create a sysbus variant of the bochs-display device, so I might work on that instead :) Best regards, Liav
[PATCH 1/1] hw/display: expose linear framebuffer address in Bochs VBE registers
This is quite useful on the isa-vga device, because it lets guest drivers
to determine where is the framebuffer located in physical memory instead
of blindly hardcoding an address. It also allows future movements of the
framebuffer to other locations.
Signed-off-by: Liav Albani
---
hw/display/bochs-display.c | 10 +-
hw/display/vga.c | 13 +++--
include/hw/display/bochs-vbe.h | 7 ++-
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 8ed734b195..aa3aa51e2f 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -77,9 +77,17 @@ static uint64_t bochs_display_vbe_read(void *ptr, hwaddr
addr,
switch (index) {
case VBE_DISPI_INDEX_ID:
-return VBE_DISPI_ID5;
+return VBE_DISPI_ID6;
case VBE_DISPI_INDEX_VIDEO_MEMORY_64K:
return s->vgamem / (64 * KiB);
+case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS1:
+return (s->vram.addr) & 0x;
+case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS2:
+return (s->vram.addr >> 16) & 0x;
+case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS3:
+return (s->vram.addr >> 32) & 0x;
+case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS4:
+return (s->vram.addr >> 48) & 0x;
}
if (index >= ARRAY_SIZE(s->vbe_regs)) {
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 50ecb1ad02..9d91946987 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -727,6 +727,14 @@ uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
}
} else if (s->vbe_index == VBE_DISPI_INDEX_VIDEO_MEMORY_64K) {
val = s->vbe_size / (64 * KiB);
+} else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS1) {
+val = (s->vram.addr) & 0x;
+} else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS2) {
+val = (s->vram.addr >> 16) & 0x;
+} else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS3) {
+val = (s->vram.addr >> 32) & 0x;
+} else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS4) {
+val = (s->vram.addr >> 48) & 0x;
} else {
val = 0;
}
@@ -753,7 +761,8 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr,
uint32_t val)
val == VBE_DISPI_ID2 ||
val == VBE_DISPI_ID3 ||
val == VBE_DISPI_ID4 ||
-val == VBE_DISPI_ID5) {
+val == VBE_DISPI_ID5 ||
+val == VBE_DISPI_ID6) {
s->vbe_regs[s->vbe_index] = val;
}
break;
@@ -1830,7 +1839,7 @@ void vga_common_reset(VGACommonState *s)
s->bank_offset = 0;
s->vbe_index = 0;
memset(s->vbe_regs, '\0', sizeof(s->vbe_regs));
-s->vbe_regs[VBE_DISPI_INDEX_ID] = VBE_DISPI_ID5;
+s->vbe_regs[VBE_DISPI_INDEX_ID] = VBE_DISPI_ID6;
s->vbe_start_addr = 0;
s->vbe_line_offset = 0;
s->vbe_bank_mask = (s->vram_size >> 16) - 1;
diff --git a/include/hw/display/bochs-vbe.h b/include/hw/display/bochs-vbe.h
index bc2f046eee..383474b9d1 100644
--- a/include/hw/display/bochs-vbe.h
+++ b/include/hw/display/bochs-vbe.h
@@ -19,8 +19,12 @@
#define VBE_DISPI_INDEX_VIRT_HEIGHT 0x7
#define VBE_DISPI_INDEX_X_OFFSET0x8
#define VBE_DISPI_INDEX_Y_OFFSET0x9
-#define VBE_DISPI_INDEX_NB 0xa /* size of vbe_regs[] */
#define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS1 0xb /* read-only, not in
vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS2 0xc /* read-only, not in
vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS3 0xd /* read-only, not in
vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS4 0xe /* read-only, not in
vbe_regs */
+#define VBE_DISPI_INDEX_NB 0xe /* size of vbe_regs[] */
/* VBE_DISPI_INDEX_ID */
#define VBE_DISPI_ID0 0xB0C0
@@ -29,6 +33,7 @@
#define VBE_DISPI_ID3 0xB0C3
#define VBE_DISPI_ID4 0xB0C4
#define VBE_DISPI_ID5 0xB0C5
+#define VBE_DISPI_ID6 0xB0C6
/* VBE_DISPI_INDEX_ENABLE */
#define VBE_DISPI_DISABLED 0x00
--
2.37.3
[PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers
Recently I submitted patches to the SerenityOS project in regard to enhancing the overall abstractions of x86-specific hardware support code in the SerenityOS kernel in preparation for aarch64 support. Then, I moved on to submit another patch to introduce support of the isa-vga device, as we currently allow people to run an ISA-PC machine with SerenityOS without any GUI (see this link for more details - https://github.com/SerenityOS/serenity/pull/15259). This all worked pretty well and with the patches being applied it is possible to boot into a graphical environment. However, not all things are perfect (yet). To ensure we only create a driver instance for an isa-vga device, I try to ensure that PCI was disabled due to failed hardware test and not because of a user decision, to ensure that we do not try to drive a PCI stdvga device with an ISA device-targeted HW parameters. This worked well too, but one problem is left now - I still had to hardcode the framebuffer address to 0xE000 in the driver. Honestly, it's not a big issue of its own - many devices are assumed to exist at well-defined physical memory addresses, especially when it is related to plain old ISA devices. However, I still want to fix this, for these reasons: 1. Although it is reasonable to assume no more than one isa-vga device will exist in one machine, this could be changed easily later on. As it stands now, on an ISA-PC machine with no PCI bus, there are basically zero methods to detect hardware - you have to assume the hardware is there, or just to probe for it and hope for the best. Relying on the BIOS to do hardware detection is also a possibility as it knows the best upon the platform it being used on. In the SerenityOS project we decided for the time being to not use the BIOS as that will require doing hacky stuff to use 16-bit code segments. Also, the BIOS is not perfect and of course it does not give you every little detail you might want, as long as we consider the standard services of an x86 BIOS these days. For an ISA-PC machine, the assumption of one isa-vga device at most is OK and will be true in the future as well. However, for other machines, and the one I am especially interested in, the microvm machine, this claim could be easily revoked as the microvm machine exposes a device tree - we could easily place many ISA-VGA devices on the "System bus" of a virtual machine, essentially having multiple framebuffer devices on such machine setup, with no PCI bus being involved at all. Of course, we will need to figure out how to make some sort of an ISA-VGA device that resembles a bochs-display device - it should not have VGA capabilities because otherwise the devices' resources will be in conflict for VGA control of the VGA IO space. The Bochs VBE registers will also need to be located in different IO ports too for each device. This idea is quite neat in my opinion, because it could speed up the boot of such VM while still providing sufficient display capabilities for those we need them. It could help developers to test their OSes on such hardware setups to ensure multi-monitor configuration works reliably when there's no PCI bus at all but many framebuffer devices being used in one VM. 2. This is more related to the SerenityOS project - I prefer to not hardcode physical addresses at all wherever I can do so. This makes the code cleaner and more understandable as far as I observe this from the angle of the person which is not me, that tries to make sense from the code flow. 3. The costs of adding this feature are pretty negligible compared to the possible value of this, especially if we apply the idea of running multiple ISA-VGA devices on one microvm machine. Still, the only major "issue" that one can point to is the fact that I bump up the Bochs VBE version number, which could be questionable with how the feature might be insignificant for many guest OSes out there. Liav Albani (1): hw/display: expose linear framebuffer address in Bochs VBE registers hw/display/bochs-display.c | 10 +- hw/display/vga.c | 13 +++-- include/hw/display/bochs-vbe.h | 7 ++- 3 files changed, 26 insertions(+), 4 deletions(-) -- 2.37.3
Re: [PATCH v2] hw/display: load the correct ROM file for isa-vga device
On 9/17/22 17:40, Liav Albani wrote: On 9/17/22 17:32, Liav Albani wrote: diff --git a/pc-bios/meson.build b/pc-bios/meson.build index 388e0db6e4..6af94a4a0a 100644 --- a/pc-bios/meson.build +++ b/pc-bios/meson.build @@ -29,7 +29,7 @@ blobs = [ 'bios-microvm.bin', 'qboot.rom', 'sgabios.bin', - 'vgabios.bin', + 'vgabios-isavga.bin', 'vgabios-cirrus.bin', Well, it seems like this one doesn't want to be compiled now, so I'll need to dive deeper to figure out how to ensure it always produces the requested file. So apparently the problem on my development machine is that for some odd reason I don't have the "vgabios.bin" file, but I do have the "vgabios-isavga.bin" file. According to my package manager (pacman), it is owned by the SeaBIOS package: usr/share/qemu/vgabios-isavga.bin is owned by extra/seabios 1.15.0-1 Other files in that directory are owned by the same package as well, for example: usr/share/qemu/vgabios-stdvga.bin is owned by extra/seabios 1.15.0-1 usr/share/qemu/vgabios-virtio.bin is owned by extra/seabios 1.15.0-1 usr/share/qemu/vgabios-vmware.bin is owned by extra/seabios 1.15.0-1 So I'm not sure what is the best approach here to fix this. It is definitely not a problem in QEMU because when I compile it from source I do get a "vgabios.bin" file in the build directory, but I do think that QEMU should try to use the "vgabios-isavga.bin" file when using the isa-vga device, so in some way we could fix it in QEMU, which also makes sense to ensure the filename is "vgabios-isavga.bin" for the isa-vga device and not plain "vgabios.bin", which really doesn't say much about the type of the graphics device. 'vgabios-stdvga.bin', 'vgabios-vmware.bin',
Re: [PATCH v2] hw/display: load the correct ROM file for isa-vga device
On 9/17/22 17:32, Liav Albani wrote: diff --git a/pc-bios/meson.build b/pc-bios/meson.build index 388e0db6e4..6af94a4a0a 100644 --- a/pc-bios/meson.build +++ b/pc-bios/meson.build @@ -29,7 +29,7 @@ blobs = [ 'bios-microvm.bin', 'qboot.rom', 'sgabios.bin', - 'vgabios.bin', + 'vgabios-isavga.bin', 'vgabios-cirrus.bin', Well, it seems like this one doesn't want to be compiled now, so I'll need to dive deeper to figure out how to ensure it always produces the requested file. 'vgabios-stdvga.bin', 'vgabios-vmware.bin',
[PATCH v2] hw/display: load the correct ROM file for isa-vga device
Apparently QEMU didn't load the correct ROM file when using the isa-vga
device on my development machine, which resulted in a display waiting to
be initialized by a guest OS kernel. With this fix, SeaBIOS is able to
print vital data to a text mode console during boot, which is useful in
case of failing to continue booting.
The build name of the vgabios.bin is changed too, to vgabios-isavga.bin
to ensure we always have that file when QEMU is installed as a package
or compiled from source.
Signed-off-by: Liav Albani
---
hw/display/vga-isa.c | 2 +-
hw/display/vga_int.h | 2 +-
pc-bios/meson.build | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 46abbc5653..bcf646d012 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -84,7 +84,7 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
VBE_DISPI_LFB_PHYSICAL_ADDRESS,
>vram);
/* ROM BIOS */
-rom_add_vga(VGABIOS_FILENAME);
+rom_add_vga(VGABIOS_ISAVGA_FILENAME);
}
static Property vga_isa_properties[] = {
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 305e700014..b63788e809 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -183,7 +183,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr,
uint32_t val);
extern const uint8_t sr_mask[8];
extern const uint8_t gr_mask[16];
-#define VGABIOS_FILENAME "vgabios.bin"
+#define VGABIOS_ISAVGA_FILENAME "vgabios-isavga.bin"
#define VGABIOS_CIRRUS_FILENAME "vgabios-cirrus.bin"
extern const MemoryRegionOps vga_mem_ops;
diff --git a/pc-bios/meson.build b/pc-bios/meson.build
index 388e0db6e4..6af94a4a0a 100644
--- a/pc-bios/meson.build
+++ b/pc-bios/meson.build
@@ -29,7 +29,7 @@ blobs = [
'bios-microvm.bin',
'qboot.rom',
'sgabios.bin',
- 'vgabios.bin',
+ 'vgabios-isavga.bin',
'vgabios-cirrus.bin',
'vgabios-stdvga.bin',
'vgabios-vmware.bin',
--
2.37.3
Re: [PATCH] hw/display: load the correct ROM file for isa-vga device
On 9/17/22 17:06, Liav Albani wrote:
Apparently we didn't load the correct ROM file when using the isa-vga
device, which resulted in a display waiting to be initialized by a guest
OS kernel. With this fix, SeaBIOS is able to print vital data to a text
mode console during boot, which is useful in case of failing to continue
booting.
Signed-off-by: Liav Albani
---
hw/display/vga-isa.c | 2 +-
hw/display/vga_int.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 46abbc5653..bcf646d012 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -84,7 +84,7 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
VBE_DISPI_LFB_PHYSICAL_ADDRESS,
>vram);
/* ROM BIOS */
-rom_add_vga(VGABIOS_FILENAME);
+rom_add_vga(VGABIOS_ISAVGA_FILENAME);
}
static Property vga_isa_properties[] = {
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 305e700014..b5e803ac51 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -183,7 +183,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr,
uint32_t val);
extern const uint8_t sr_mask[8];
extern const uint8_t gr_mask[16];
-#define VGABIOS_FILENAME "vgabios.bin"
+#define VGABIOS_ISAVGA_FILENAME "vgabios-isa.bin"
This should be an "vgabios-isavga.bin" actually, at least this is how it
is named on my development machine. I'll send a v2 shortly.
#define VGABIOS_CIRRUS_FILENAME "vgabios-cirrus.bin"
extern const MemoryRegionOps vga_mem_ops;
[PATCH] hw/display: load the correct ROM file for isa-vga device
Apparently we didn't load the correct ROM file when using the isa-vga
device, which resulted in a display waiting to be initialized by a guest
OS kernel. With this fix, SeaBIOS is able to print vital data to a text
mode console during boot, which is useful in case of failing to continue
booting.
Signed-off-by: Liav Albani
---
hw/display/vga-isa.c | 2 +-
hw/display/vga_int.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 46abbc5653..bcf646d012 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -84,7 +84,7 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
VBE_DISPI_LFB_PHYSICAL_ADDRESS,
>vram);
/* ROM BIOS */
-rom_add_vga(VGABIOS_FILENAME);
+rom_add_vga(VGABIOS_ISAVGA_FILENAME);
}
static Property vga_isa_properties[] = {
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 305e700014..b5e803ac51 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -183,7 +183,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr,
uint32_t val);
extern const uint8_t sr_mask[8];
extern const uint8_t gr_mask[16];
-#define VGABIOS_FILENAME "vgabios.bin"
+#define VGABIOS_ISAVGA_FILENAME "vgabios-isa.bin"
#define VGABIOS_CIRRUS_FILENAME "vgabios-cirrus.bin"
extern const MemoryRegionOps vga_mem_ops;
--
2.37.3
Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
but I feel quoting spec and including table name is a good idea actually, but pls quote verbatim: I don't do that and don't ask it from others. The reason being that pointing where to look in spec and having verbatim copy of field name is sufficient for looking it up and QEMU does not endup with half of spec copied in (+unintentional mistakes). (As reviewer I will check if whatever written in patch actually matches spec anyways) That's why I typically use 'spec ver, verbatim field name[, chapter/table name]' policy. The later optional part is usually used for pointing to values description. Ok but here the field name was not listed verbatim, and table name is missing. It is actually 8042 and table name is Fixed ACPI Description Table Boot Architecture Flags. So, in which route should I go with this? I could add a reference to the ACPI spec, but can write and explain more if you want me to.
Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
On 3/2/22 14:42, Michael S. Tsirkin wrote: On Wed, Mar 02, 2022 at 10:44:03AM +0530, Ani Sinha wrote: On Wed, Mar 2, 2022 at 12:50 AM Liav Albani wrote: On 3/1/22 11:52, Ani Sinha wrote: On Tue, 1 Mar 2022, Igor Mammedov wrote: On Mon, 28 Feb 2022 22:17:32 +0200 Liav Albani wrote: This can allow the guest OS to determine more easily if i8042 controller is present in the system or not, so it doesn't need to do probing of the controller, but just initialize it immediately, before enumerating the ACPI AML namespace. This change only applies to the x86/q35 machine type, as it uses FACP ACPI table with revision higher than 1, which should implement at least ACPI 2.0 features within the table, hence it can also set the IA-PC boot flags register according to the ACPI 2.0 specification. Signed-off-by: Liav Albani --- hw/acpi/aml-build.c | 11 ++- hw/i386/acpi-build.c| 9 + hw/i386/acpi-microvm.c | 9 + commit message says it's q35 specific, so wy it touched microvm anc piix4? Igor is correct. Although I see that currently there are no 8042 devices for microvms, maybe we should be conservative and add the code to detect the device anyway. In that case, the change could affect microvms too when such devices get added in the future. echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm -monitor stdio 2>/dev/null | grep 8042 What about this? echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm -device i8042 -monitor stdio 2>/dev/null | grep 8042 Or this? echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm -device i8042 -monitor stdio 2>/dev/null | grep 8042 On both occasions you are explicitly adding the device. Yes of course. It seems a bit cleaner to have -device i8042 -monitor stdio give us the correct ACPI table even if there's no pressing need for this ATM, simply because it's not much more code, and because if we don't we risk guests trying to work around incorrect ACPI tables. Let us however do this in a patch by its own with proper documentation and motivation. So if I understand how to do this now - I should drop the code for the MicroVM ACPI for now, letting only to change the Q35 FACP table, right? So if that's the case I should send it in a separate patch? If that's the case, as suggested by you and Ani, I'll not add a separate function to reduce code duplication as there's no such thing in such case...
Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
On 3/1/22 11:52, Ani Sinha wrote: On Tue, 1 Mar 2022, Igor Mammedov wrote: On Mon, 28 Feb 2022 22:17:32 +0200 Liav Albani wrote: This can allow the guest OS to determine more easily if i8042 controller is present in the system or not, so it doesn't need to do probing of the controller, but just initialize it immediately, before enumerating the ACPI AML namespace. This change only applies to the x86/q35 machine type, as it uses FACP ACPI table with revision higher than 1, which should implement at least ACPI 2.0 features within the table, hence it can also set the IA-PC boot flags register according to the ACPI 2.0 specification. Signed-off-by: Liav Albani --- hw/acpi/aml-build.c | 11 ++- hw/i386/acpi-build.c| 9 + hw/i386/acpi-microvm.c | 9 + commit message says it's q35 specific, so wy it touched microvm anc piix4? Igor is correct. Although I see that currently there are no 8042 devices for microvms, maybe we should be conservative and add the code to detect the device anyway. In that case, the change could affect microvms too when such devices get added in the future. echo -e "info qtree\r\nquit\r\n" | ./qemu-system-x86_64 -machine microvm -monitor stdio 2>/dev/null | grep 8042 What about this? echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm -device i8042 -monitor stdio 2>/dev/null | grep 8042 Or this? echo -e "info mtree\r\nquit\r\n" | qemu-system-x86_64 -machine microvm -device i8042 -monitor stdio 2>/dev/null | grep 8042
Re: [PATCH v4 3/3] tests/acpi: i386: update FACP table differences
On 3/1/22 13:21, Michael S. Tsirkin wrote: On Tue, Mar 01, 2022 at 08:29:57AM +0530, Ani Sinha wrote: On Mon, 28 Feb 2022, Liav Albani wrote: After changing the IAPC boot flags register to indicate support of i8042 in the machine chipset to help the guest OS to determine its existence "faster", we need to have the updated FACP ACPI binary images in tree. @@ -1,32 +1,32 @@ /* * Intel ACPI Component Architecture * AML/ASL+ Disassembler version 20211217 (64-bit version) * Copyright (c) 2000 - 2021 Intel Corporation * - * Disassembly of tests/data/acpi/q35/FACP, Wed Feb 23 22:37:39 2022 + * Disassembly of /tmp/aml-BBFBI1, Wed Feb 23 22:37:39 2022 cut this out pls I see, this is indeed not very useful... * * ACPI Data Table [FACP] * * Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex) */ [000h 4]Signature : "FACP"[Fixed ACPI Description Table (FADT)] [004h 0004 4] Table Length : 00F4 [008h 0008 1] Revision : 03 -[009h 0009 1] Checksum : B9 +[009h 0009 1] Checksum : B7 and this [00Ah 0010 6] Oem ID : "BOCHS " [010h 0016 8] Oem Table ID : "BXPC" [018h 0024 4] Oem Revision : 0001 [01Ch 0028 4] Asl Compiler ID : "BXPC" [020h 0032 4]Asl Compiler Revision : 0001 [024h 0036 4] FACS Address : [028h 0040 4] DSDT Address : [02Ch 0044 1]Model : 01 [02Dh 0045 1] PM Profile : 00 [Unspecified] [02Eh 0046 2]SCI Interrupt : 0009 [030h 0048 4] SMI Command Port : 00B2 [034h 0052 1]ACPI Enable Value : 02 [035h 0053 1] ACPI Disable Value : 03 [036h 0054 1] S4BIOS Command : 00 [037h 0055 1] P-State Control : 00 @@ -42,35 +42,35 @@ [059h 0089 1] PM1 Control Block Length : 02 [05Ah 0090 1] PM2 Control Block Length : 00 [05Bh 0091 1]PM Timer Block Length : 04 [05Ch 0092 1]GPE0 Block Length : 10 [05Dh 0093 1]GPE1 Block Length : 00 [05Eh 0094 1] GPE1 Base Offset : 00 [05Fh 0095 1] _CST Support : 00 [060h 0096 2] C2 Latency : 0FFF [062h 0098 2] C3 Latency : 0FFF [064h 0100 2] CPU Cache Size : [066h 0102 2] Cache Flush Stride : [068h 0104 1]Duty Cycle Offset : 00 [069h 0105 1] Duty Cycle Width : 00 [06Ah 0106 1] RTC Day Alarm Index : 00 [06Bh 0107 1]RTC Month Alarm Index : 00 [06Ch 0108 1]RTC Century Index : 32 -[06Dh 0109 2] Boot Flags (decoded below) : +[06Dh 0109 2] Boot Flags (decoded below) : 0002 Legacy Devices Supported (V2) : 0 -8042 Present on ports 60/64 (V2) : 0 +8042 Present on ports 60/64 (V2) : 1 VGA Not Present (V4) : 0 MSI Not Supported (V4) : 0 PCIe ASPM Not Supported (V4) : 0 CMOS RTC Not Present (V5) : 0 leaving just this It will be fixed in version 5. [06Fh 0111 1] Reserved : 00 [070h 0112 4]Flags (decoded below) : 84A5 WBINVD instruction is operational (V1) : 1 WBINVD flushes all caches (V1) : 0 All CPUs support C1 (V1) : 1 C2 works on MP system (V1) : 0 Control Method Power Button (V1) : 0 Control Method Sleep Button (V1) : 1 RTC wake not in fixed reg space (V1) : 0 RTC can wake system from S4 (V1) : 1 32-bit PM Timer (V1) : 0 Docking Supported (V1) : 0 @@ -148,32 +148,32 @@ [0DCh 0220 1] Space ID : 01 [SystemIO] [0DDh 0221 1]Bit Width : 80 [0DEh 0222 1] Bit Offset : 00 [0DFh 0223 1] Encoded Access Width : 00 [Undefined/Legacy] [0E0h 0224 8] Address : 0620 [0E8h 0232 12] GPE1 Block : [Generic Address Structure] [0E8h 0232 1] Space ID : 00 [SystemMemory] [0E9h 0233 1]Bit Width : 00 [0EAh 0234 1] Bit Offset : 00 [0EBh 0235 1] Encoded Access Width : 00 [Undefined/Legacy] [0ECh 0236 8] Address : Raw Table Data: Length 244 (0xF4) -: 46 41 43 50 F4 00 00 00 03 B9 42 4F 43 48 53 20 // FACP..BOCHS +: 46 41 43 50 F4 00 00 00 03 B7 42 4F 43 48 53 20 // FACP..BOCHS 001
Re: [PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
On 3/1/22 13:19, Michael S. Tsirkin wrote:
On Tue, Mar 01, 2022 at 09:43:54AM +0100, Igor Mammedov wrote:
On Mon, 28 Feb 2022 22:17:32 +0200
Liav Albani wrote:
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
This change only applies to the x86/q35 machine type, as it uses FACP
ACPI table with revision higher than 1, which should implement at least
ACPI 2.0 features within the table, hence it can also set the IA-PC boot
flags register according to the ACPI 2.0 specification.
Signed-off-by: Liav Albani
---
hw/acpi/aml-build.c | 11 ++-
hw/i386/acpi-build.c| 9 +
hw/i386/acpi-microvm.c | 9 +
commit message says it's q35 specific, so wy it touched microvm anc piix4?
This affect only q35 machine type for now, but what happens if the
MicroVM ACPI FACP table is updated to use a revision that supports IA-PC
boot flags?
include/hw/acpi/acpi-defs.h | 1 +
4 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..2085905b83 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,16 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+/*
+ * This register is not defined in ACPI spec version 1.0, where the FACP
+ * revision == 1 also applies. Therefore, just ignore setting this
register.
+ */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
build_append_int_noprefix(tbl, 0, 1); /* Reserved */
build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..c72c7bb9bb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,7 @@
#include "hw/nvram/fw_cfg.h"
#include "hw/acpi/bios-linker-loader.h"
#include "hw/isa/isa.h"
+#include "hw/input/i8042.h"
#include "hw/block/fdc.h"
#include "hw/acpi/memory_hotplug.h"
#include "sysemu/tpm.h"
@@ -192,6 +193,14 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK,
NULL)
},
};
+/*
+ * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
+ * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
/* APCI spec version 2.0, Table 5-10 */
is sufficient, the rest could be read from spec/
ACPI though, not APCI.
It will be fixed in version 5.
The comment can be shorter and more clearer, but I feel quoting spec
and including table name is a good idea actually, but pls quote verbatim:
/* ACPI spec version 2.0, Table 5-10: Fixed ACPI Description Table Boot
Architecture Flags */
/* Bit offset 1 - port 60 and 64 based keyboard controller, usually
implemented as an 8042 or equivalent micro-controller. */
+ */
+fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
+0x0002 : 0x;
and make it 0x1 << 1 - clearer that this is bit 1. Leading zeroes are
not helpful since compiler does not check there's a correct number of
these.
It will be fixed in version 5.
+
*data = fadt;
}
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..4bc72b1672 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -31,6 +31,7 @@
#include "hw/acpi/generic_event_device.h"
#include "hw/acpi/utils.h"
#include "hw/acpi/erst.h"
+#include "hw/input/i8042.h"
#include "hw/i386/fw_cfg.h"
#include "hw/i386/microvm.h"
#include "hw/pci/pci.h"
@@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
.reset_val = ACPI_GED_RESET_VALUE,
};
+/*
+ * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
+ * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+pmfadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
+0x0002 : 0x;
+
let's avoid code duplication pls.
What do you suggest to do with this? Creating a separate functi
[PATCH v4 3/3] tests/acpi: i386: update FACP table differences
04 10 00 00 00 // ... -0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // 2... +0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // 2... 0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // 0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // 0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // . .. 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // 00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // 00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . .. 00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ... 00F0: 00 00 00 00 // ** Signed-off-by: Liav Albani --- tests/data/acpi/q35/FACP| Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes tests/qtest/bios-tables-test-allowed-diff.h | 4 5 files changed, 4 deletions(-) diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644 GIT binary patch delta 23 ecmeyu_=S
[PATCH v4 2/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
This change only applies to the x86/q35 machine type, as it uses FACP
ACPI table with revision higher than 1, which should implement at least
ACPI 2.0 features within the table, hence it can also set the IA-PC boot
flags register according to the ACPI 2.0 specification.
Signed-off-by: Liav Albani
---
hw/acpi/aml-build.c | 11 ++-
hw/i386/acpi-build.c| 9 +
hw/i386/acpi-microvm.c | 9 +
include/hw/acpi/acpi-defs.h | 1 +
4 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..2085905b83 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,16 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+/*
+ * This register is not defined in ACPI spec version 1.0, where the FACP
+ * revision == 1 also applies. Therefore, just ignore setting this
register.
+ */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
build_append_int_noprefix(tbl, 0, 1); /* Reserved */
build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..c72c7bb9bb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,6 +38,7 @@
#include "hw/nvram/fw_cfg.h"
#include "hw/acpi/bios-linker-loader.h"
#include "hw/isa/isa.h"
+#include "hw/input/i8042.h"
#include "hw/block/fdc.h"
#include "hw/acpi/memory_hotplug.h"
#include "sysemu/tpm.h"
@@ -192,6 +193,14 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
},
};
+/*
+ * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
+ * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+fadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
+0x0002 : 0x;
+
*data = fadt;
}
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..4bc72b1672 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -31,6 +31,7 @@
#include "hw/acpi/generic_event_device.h"
#include "hw/acpi/utils.h"
#include "hw/acpi/erst.h"
+#include "hw/input/i8042.h"
#include "hw/i386/fw_cfg.h"
#include "hw/i386/microvm.h"
#include "hw/pci/pci.h"
@@ -189,6 +190,14 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
.reset_val = ACPI_GED_RESET_VALUE,
};
+/*
+ * second bit of 16 of the IAPC_BOOT_ARCH register indicates i8042 presence
+ * or equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+pmfadt.iapc_boot_arch = object_resolve_path_type("", TYPE_I8042, NULL) ?
+0x0002 : 0x;
+
table_offsets = g_array_new(false, true /* clear */,
sizeof(uint32_t));
bios_linker_loader_alloc(tables->linker,
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c97e8633ad..2b42e4192b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
uint16_t plvl2_lat;/* P_LVL2_LAT */
uint16_t plvl3_lat;/* P_LVL3_LAT */
uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
+uint16_t iapc_boot_arch; /* IAPC_BOOT_ARCH */
uint8_t minor_ver; /* FADT Minor Version */
/*
--
2.35.1
[PATCH v4 1/3] tests/acpi: i386: allow FACP acpi table changes
The FACP table is going to be changed for x86/q35 machines. To be sure the following changes are not breaking any QEMU test this change follows step 2 from the bios-tables-test.c guide on changes that affect ACPI tables. Signed-off-by: Liav Albani --- tests/qtest/bios-tables-test-allowed-diff.h | 4 1 file changed, 4 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..7570e39369 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,5 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/q35/FACP", +"tests/data/acpi/q35/FACP.nosmm", +"tests/data/acpi/q35/FACP.slic", +"tests/data/acpi/q35/FACP.xapic", -- 2.35.1
[PATCH v4 0/3] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller is present in the system or not, so it doesn't need to do probing of the controller, but just initialize it immediately, before enumerating the ACPI AML namespace. To allow "flexible" indication, I don't hardcode the bit at location 1 as on in the IA-PC boot flags, but try to search for i8042 on the ISA bus to verify it exists in the system. Why this is useful you might ask - this patch allows the guest OS to probe and use the i8042 controller without decoding the ACPI AML blob at all. For example, as a developer of the SerenityOS kernel, I might want to allow people to not try to decode the ACPI AML namespace (for now, we still don't support ACPI AML as it's a work in progress), but still to not probe for the i8042 but just use it after looking in the IA-PC boot flags in the ACPI FADT table. Liav Albani (3): tests/acpi: i386: allow FACP acpi table changes hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table tests/acpi: i386: update FACP table differences hw/acpi/aml-build.c| 11 ++- hw/i386/acpi-build.c | 9 + hw/i386/acpi-microvm.c | 9 + include/hw/acpi/acpi-defs.h| 1 + tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes 8 files changed, 29 insertions(+), 1 deletion(-) -- 2.35.1
Re: [PATCH v3 1/4] hw/isa: add function to check for existence of device by its type
On 2/27/22 09:27, Ani Sinha wrote:
On Sat, 26 Feb 2022, Liav Albani wrote:
This function enumerates all attached ISA devices in the machine, and
tries to compare a given device type name to the enumerated devices.
For example, this can help other code to determine if a i8042 controller
exists in the machine.
Signed-off-by: Liav Albani
---
hw/isa/isa-bus.c | 23 +++
include/hw/isa/isa.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 6c31398dda..663aa36d29 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -222,6 +222,29 @@ void isa_build_aml(ISABus *bus, Aml *scope)
}
}
+bool isa_check_device_existence(const char *typename)
+{
+/*
+ * If there's no ISA bus, we know for sure that the checked ISA device type
+ * doesn't exist in the machine.
+ */
+if (isabus == NULL) {
nit: I would do if (!isabus) instead to keep uniformity with other parts
of the code.
Hmm, OK, I'll change it because it seems really fine to do that this way
too :)
+return false;
+}
+
+BusChild *kid;
+ISADevice *dev;
+
+QTAILQ_FOREACH(kid, >parent_obj.children, sibling) {
+dev = ISA_DEVICE(kid->child);
+const char *object_type = object_get_typename(OBJECT(dev));
+if (object_type && strcmp(object_type, typename) == 0) {
nit: I would do !strcmp() instead.
Hmm, OK, I'll change it because it seems really fine to do that this way
too :)
+return true;
+}
+}
+return false;
+}
+
static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
{
ISADevice *d = ISA_DEVICE(dev);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index d4417b34b6..65f0c7e28c 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
MemoryRegion *isa_address_space(ISADevice *dev);
MemoryRegion *isa_address_space_io(ISADevice *dev);
ISADevice *isa_new(const char *name);
+bool isa_check_device_existence(const char *typename);
Please provide documentation for this function in line with other
functions like isa_register_ioport() and isa_register_portio_list() in
the same header.
Ah, I see what you mean - I'll write short descriptive documentation
like what there's for other functions :)
Thanks for the suggestions!
Best regards,
Liav
Re: [PATCH v3 3/4] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
On 2/27/22 08:56, Ani Sinha wrote:
On Sat, 26 Feb 2022, Liav Albani wrote:
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
Signed-off-by: Liav Albani
---
hw/acpi/aml-build.c | 7 ++-
hw/i386/acpi-build.c| 8
hw/i386/acpi-microvm.c | 9 +
include/hw/acpi/acpi-defs.h | 1 +
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..ef5f4cad87 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
Like a suggested in the previous iteration, please add a comment here to
specify why you are adding this only for rev !=1 . Also please mention
that your change only affects q35 machines since i440fx uses rev 1 (ref to
acpi_get_pm_info()).
build_append_int_noprefix(tbl, 0, 1); /* Reserved */
build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..65dbc1ec36 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,14 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK,
NULL)
},
};
+/*
+ * second bit of 16 but
wow, you even retained my typo here! :-)
Yeah, I figured this after I sent the patch series, sorry for that mistake!
IAPC_BOOT_ARCH indicates presence of 8042 or
+ * equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+
+fadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002 :
0x;
+
*data = fadt;
}
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..e5f89164be 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -189,6 +189,15 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
.reset_val = ACPI_GED_RESET_VALUE,
};
+/*
+ * second bit of 16 but IAPC_BOOT_ARCH indicates presence of 8042 or
ditto as above.
No problem, I'll send a v4 soon :)
+ * equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+
+pmfadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002
+: 0x;
+
table_offsets = g_array_new(false, true /* clear */,
sizeof(uint32_t));
bios_linker_loader_alloc(tables->linker,
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c97e8633ad..2b42e4192b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
uint16_t plvl2_lat;/* P_LVL2_LAT */
uint16_t plvl3_lat;/* P_LVL3_LAT */
uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
+uint16_t iapc_boot_arch; /* IAPC_BOOT_ARCH */
uint8_t minor_ver; /* FADT Minor Version */
/*
--
2.35.1
Re: [PATCH v3 3/4] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
On 2/27/22 12:48, Bernhard Beschow wrote:
Am 26. Februar 2022 06:30:18 UTC schrieb Liav Albani :
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
Signed-off-by: Liav Albani
---
hw/acpi/aml-build.c | 7 ++-
hw/i386/acpi-build.c| 8
hw/i386/acpi-microvm.c | 9 +
include/hw/acpi/acpi-defs.h | 1 +
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..ef5f4cad87 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
build_append_int_noprefix(tbl, 0, 1); /* Reserved */
build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..65dbc1ec36 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,14 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
},
};
+/*
+ * second bit of 16 but IAPC_BOOT_ARCH indicates presence of 8042 or
+ * equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+
+fadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002 :
0x;
Couldn't qdev_find_recursive() be used here instead? This would also make patch
1 unneccessary. Same below.
Best regards
Bernhard
I tried it first, but because it tries to find the ID of a device
instead of a type (I look for i8042 type which is a string of the device
type), it didn't work as expected. We don't compare DeviceState id, but
ObjectClass type->name here :)
With my patch we could just find the device without any problem whatsoever.
Best regards,
Liav
[PATCH v3 4/4] tests/acpi: i386: update FACP table differences
04 10 00 00 00 // ... -0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 00 00 00 // 2... +0060: FF 0F FF 0F 00 00 00 00 00 00 00 00 32 02 00 00 // 2... 0070: A5 84 00 00 01 08 00 00 F9 0C 00 00 00 00 00 00 // 0080: 0F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // 0090: 00 00 00 00 01 20 00 00 00 06 00 00 00 00 00 00 // . .. 00A0: 00 00 00 00 00 00 00 00 00 00 00 00 01 10 00 00 // 00B0: 04 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // 00C0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // 00D0: 01 20 00 00 08 06 00 00 00 00 00 00 01 80 00 00 // . .. 00E0: 20 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ... 00F0: 00 00 00 00 // ** Signed-off-by: Liav Albani --- tests/data/acpi/q35/FACP| Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes tests/qtest/bios-tables-test-allowed-diff.h | 4 5 files changed, 4 deletions(-) diff --git a/tests/data/acpi/q35/FACP b/tests/data/acpi/q35/FACP index f6a864cc863c7763f6c09d3814ad184a658fa0a0..a8f6a8961109d01059aceef9f1869cde09a2f10c 100644 GIT binary patch delta 23 ecmeyu_=S
[PATCH v3 3/4] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
Signed-off-by: Liav Albani
---
hw/acpi/aml-build.c | 7 ++-
hw/i386/acpi-build.c| 8
hw/i386/acpi-microvm.c | 9 +
include/hw/acpi/acpi-defs.h | 1 +
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..ef5f4cad87 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
build_append_int_noprefix(tbl, 0, 1); /* Reserved */
build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..65dbc1ec36 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,14 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
},
};
+/*
+ * second bit of 16 but IAPC_BOOT_ARCH indicates presence of 8042 or
+ * equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+
+fadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002 :
0x;
+
*data = fadt;
}
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..e5f89164be 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -189,6 +189,15 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
.reset_val = ACPI_GED_RESET_VALUE,
};
+/*
+ * second bit of 16 but IAPC_BOOT_ARCH indicates presence of 8042 or
+ * equivalent micro controller. See table 5-10 of APCI spec version 2.0
+ * (the earliest acpi revision that supports this).
+ */
+
+pmfadt.iapc_boot_arch = isa_check_device_existence("i8042") ? 0x0002
+: 0x;
+
table_offsets = g_array_new(false, true /* clear */,
sizeof(uint32_t));
bios_linker_loader_alloc(tables->linker,
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c97e8633ad..2b42e4192b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
uint16_t plvl2_lat;/* P_LVL2_LAT */
uint16_t plvl3_lat;/* P_LVL3_LAT */
uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
+uint16_t iapc_boot_arch; /* IAPC_BOOT_ARCH */
uint8_t minor_ver; /* FADT Minor Version */
/*
--
2.35.1
[PATCH v3 2/4] tests/acpi: i386: allow FACP acpi table changes
The FACP table is going to be changed for x86/q35 machines. To be sure the following changes are not breaking any QEMU test this change follows step 2 from the bios-tables-test.c guide on changes that affect ACPI tables. Signed-off-by: Liav Albani --- tests/qtest/bios-tables-test-allowed-diff.h | 4 1 file changed, 4 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..7570e39369 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,5 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/q35/FACP", +"tests/data/acpi/q35/FACP.nosmm", +"tests/data/acpi/q35/FACP.slic", +"tests/data/acpi/q35/FACP.xapic", -- 2.35.1
[PATCH v3 1/4] hw/isa: add function to check for existence of device by its type
This function enumerates all attached ISA devices in the machine, and
tries to compare a given device type name to the enumerated devices.
For example, this can help other code to determine if a i8042 controller
exists in the machine.
Signed-off-by: Liav Albani
---
hw/isa/isa-bus.c | 23 +++
include/hw/isa/isa.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 6c31398dda..663aa36d29 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -222,6 +222,29 @@ void isa_build_aml(ISABus *bus, Aml *scope)
}
}
+bool isa_check_device_existence(const char *typename)
+{
+/*
+ * If there's no ISA bus, we know for sure that the checked ISA device type
+ * doesn't exist in the machine.
+ */
+if (isabus == NULL) {
+return false;
+}
+
+BusChild *kid;
+ISADevice *dev;
+
+QTAILQ_FOREACH(kid, >parent_obj.children, sibling) {
+dev = ISA_DEVICE(kid->child);
+const char *object_type = object_get_typename(OBJECT(dev));
+if (object_type && strcmp(object_type, typename) == 0) {
+return true;
+}
+}
+return false;
+}
+
static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
{
ISADevice *d = ISA_DEVICE(dev);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index d4417b34b6..65f0c7e28c 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
MemoryRegion *isa_address_space(ISADevice *dev);
MemoryRegion *isa_address_space_io(ISADevice *dev);
ISADevice *isa_new(const char *name);
+bool isa_check_device_existence(const char *typename);
ISADevice *isa_try_new(const char *name);
bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
ISADevice *isa_create_simple(ISABus *bus, const char *name);
--
2.35.1
[PATCH v3 0/4] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller is present in the system or not, so it doesn't need to do probing of the controller, but just initialize it immediately, before enumerating the ACPI AML namespace. To allow "flexible" indication, I don't hardcode the bit at location 1 as on in the IA-PC boot flags, but try to search for i8042 on the ISA bus to verify it exists in the system. Why this is useful you might ask - this patch allows the guest OS to probe and use the i8042 controller without decoding the ACPI AML blob at all. For example, as a developer of the SerenityOS kernel, I might want to allow people to not try to decode the ACPI AML namespace (for now, we still don't support ACPI AML as it's a work in progress), but still to not probe for the i8042 but just use it after looking in the IA-PC boot flags in the ACPI FADT table. A note about this version of the patch series: I changed the assertion checking if the ISA bus exists to a if statement, because I can see how in the future someone might want to run a x86 machine without an ISA bus so we should not assert if someone calls the ISA check device existence function but return FALSE gracefully. If someone thinks this is wrong, I'm more than happy to discuss and fix the code :) Liav Albani (4): hw/isa: add function to check for existence of device by its type tests/acpi: i386: allow FACP acpi table changes hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table tests/acpi: i386: update FACP table differences hw/acpi/aml-build.c| 7 ++- hw/i386/acpi-build.c | 8 hw/i386/acpi-microvm.c | 9 + hw/isa/isa-bus.c | 23 +++ include/hw/acpi/acpi-defs.h| 1 + include/hw/isa/isa.h | 1 + tests/data/acpi/q35/FACP | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.nosmm | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.slic | Bin 244 -> 244 bytes tests/data/acpi/q35/FACP.xapic | Bin 244 -> 244 bytes 10 files changed, 48 insertions(+), 1 deletion(-) -- 2.35.1
Re: [PATCH v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..5dc625b8d8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK,
NULL)
},
};
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+fadt.iapc_boot_arch = (1 << 1);
+}
+
Looking into this, it seems I messed up with the spaces here. So, I
could fix this and send a v3, but want to see if there are other
comments as well so we can get them fixed and this patch merged (or at
least in a pull request) before the soft feature freeze in 8.3.2022 :)
[PATCH v2 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
Signed-off-by: Liav Albani
---
hw/acpi/aml-build.c | 7 ++-
hw/i386/acpi-build.c| 5 +
hw/i386/acpi-microvm.c | 5 +
include/hw/acpi/acpi-defs.h | 1 +
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..ef5f4cad87 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
build_append_int_noprefix(tbl, 0, 1); /* Reserved */
build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..5dc625b8d8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
},
};
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+fadt.iapc_boot_arch = (1 << 1);
+}
+
*data = fadt;
}
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..756c69b3b0 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
.reset_val = ACPI_GED_RESET_VALUE,
};
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+pmfadt.iapc_boot_arch = (1 << 1);
+}
+
table_offsets = g_array_new(false, true /* clear */,
sizeof(uint32_t));
bios_linker_loader_alloc(tables->linker,
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c97e8633ad..2b42e4192b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
uint16_t plvl2_lat;/* P_LVL2_LAT */
uint16_t plvl3_lat;/* P_LVL3_LAT */
uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
+uint16_t iapc_boot_arch; /* IAPC_BOOT_ARCH */
uint8_t minor_ver; /* FADT Minor Version */
/*
--
2.35.1
[PATCH v2 1/2] hw/isa: add function to check for existence of device by its type
This function enumerates all attached ISA devices in the machine, and
tries to compare a given device type name to the enumerated devices.
For example, this can help other code to determine if a i8042 controller
exists in the machine.
Signed-off-by: Liav Albani
---
hw/isa/isa-bus.c | 23 +++
include/hw/isa/isa.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 6c31398dda..663aa36d29 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -222,6 +222,29 @@ void isa_build_aml(ISABus *bus, Aml *scope)
}
}
+bool isa_check_device_existence(const char *typename)
+{
+/*
+ * If there's no ISA bus, we know for sure that the checked ISA device type
+ * doesn't exist in the machine.
+ */
+if (isabus == NULL) {
+return false;
+}
+
+BusChild *kid;
+ISADevice *dev;
+
+QTAILQ_FOREACH(kid, >parent_obj.children, sibling) {
+dev = ISA_DEVICE(kid->child);
+const char *object_type = object_get_typename(OBJECT(dev));
+if (object_type && strcmp(object_type, typename) == 0) {
+return true;
+}
+}
+return false;
+}
+
static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
{
ISADevice *d = ISA_DEVICE(dev);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index d4417b34b6..65f0c7e28c 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
MemoryRegion *isa_address_space(ISADevice *dev);
MemoryRegion *isa_address_space_io(ISADevice *dev);
ISADevice *isa_new(const char *name);
+bool isa_check_device_existence(const char *typename);
ISADevice *isa_try_new(const char *name);
bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
ISADevice *isa_create_simple(ISABus *bus, const char *name);
--
2.35.1
[PATCH v2 0/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller is present in the system or not, so it doesn't need to do probing of the controller, but just initialize it immediately, before enumerating the ACPI AML namespace. To allow "flexible" indication, I don't hardcode the bit at location 1 as on in the IA-PC boot flags, but try to search for i8042 on the ISA bus to verify it exists in the system. Why this is useful you might ask - this patch allows the guest OS to probe and use the i8042 controller without decoding the ACPI AML blob at all. For example, as a developer of the SerenityOS kernel, I might want to allow people to not try to decode the ACPI AML namespace (for now, we still don't support ACPI AML as it's a work in progress), but still to not probe for the i8042 but just use it after looking in the IA-PC boot flags in the ACPI FADT table. A note about this version of the patch series: I changed the assertion checking if the ISA bus exists to a if statement, because I can see how in the future someone might want to run a x86 machine without an ISA bus so we should not assert if someone calls the ISA check device existence function but return FALSE gracefully. If someone thinks this is wrong, I'm more than happy to discuss and fix the code :) Liav Albani (2): hw/isa: add function to check for existence of device by its type hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table hw/acpi/aml-build.c | 7 ++- hw/i386/acpi-build.c| 5 + hw/i386/acpi-microvm.c | 5 + hw/isa/isa-bus.c| 23 +++ include/hw/acpi/acpi-defs.h | 1 + include/hw/isa/isa.h| 1 + 6 files changed, 41 insertions(+), 1 deletion(-) -- 2.35.1
Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation
On 2/21/22 13:33, Gerd Hoffmann wrote:
Hi,
ICH6 and ICH7 IDE controllers are quite the same as far as I know. I
could change it, but then one could argue that the name ich6-ide seems
like "ich9-ide", so not sure if we can really go on this path.
I think we don't actually have ich9-ide, we only have piix3, piix4 and ahci,
the latter is used by ich9. I just said that calling this new device
ich7-ide instead of ich6-ide would make it more clear it has nothing to do
with ich9.
Well, there actually is ich9-ide in physical hardware. And it's quite
simliar for all ich6 -> ich9 (and possibly more) physical hardware.
I know, but I based it on Intel documentation and the ICH7 machine I
have from 2009. Also, according to libata in Linux, the enum of
piix_controller_ids include ich5, ich6 and ich8 (and much more, these
are more relevant in the list for this reason), and by looking into what
ich6_sata is being used for, I can see it matches the IDE controller PCI
ID on the ICH7 machine I use, so that's another reason to choose this name.
The hardware implements both ide and sata. Typically the bios setup
offers to pick ide or sata mode for the storage controller, and on boot
the chipset is configured accordingly by the firmware.
Yes, and then the BIOS configures the MAP register to indicate the setup
that it was decided by the user in the BIOS configuration.
However, setting the MAP register to zero is a valid value - it
indicates you have only SATA connectors in use (at least that's what
Linux thinks), according to the Intel ICH5 Serial ATA Controller RPM.
qemu never bothered to implement ide mode for q35/ich9. When a guest OS
is so old that it doesn't come with a sata driver there is the option to
just use the 'pc' machine type. And usually that's the better choice
anyway because these old guests tend to have problems with other q35
components too.
That's true if you care about giving emulation only for the benefits of
the guest (so you only care about supporting what the guest OS can
expect from standard IDE controller, not edge cases), but my approach is
looking at a very different goal.
So I'm wondering why you implement ich{6,7,9}-ide in the first place?
What is the use case / benefit?
I talked about it in the last patch about this topic I've sent (v1 to be
precise), but let me describe it again :)
I'm a SerenityOS developer, as you might remember or not, I've talked to
you (Gerd) in the past about SeaBIOS topics related to the OS off-list.
As I said before in this mail, I tend to test the SerenityOS kernel on
the ICH7 machine I have from 2009. That machine has 4 SATA ports and you
can connect 2 PATA devices (as one parallel cable can be used to connect
two devices at once, to one connector on the mainboard).
I've seen that the kernel struggled to use the IDE controller - the main
problem we have is long timeouts because of some problematic pattern in
our code. However, on regular QEMU PC and Q35 machines everything boots
fine. When I wrote this emulation component, I saw the same problem I
had on the bare metal machine, so it is a convenient feature for me to
debug this problem without having to use the bare metal machine - it
helps saving lots of time for me by avoiding the need to compile a
kernel, put it on the SATA harddrive and try to boot it in the rapid
compile-boot-test cycle I have here.
I thought it might be beneficial for other OS developers and hobbyists
like me to have such component. For now, it's an IDE
ICH5/6/7/9-compatible controller, supporting only PCI IDE native mode -
which means you can relocate the resources to anything you want on the
IO space, so it's a legacy-free device in the sense of PCI bus resource
management, but still a legacy device that to use it on bare metal you
need a machine from late 2000s.
Also, I do see a point in expanding this controller with more features.
For example, some ICH6 IDE controllers had AHCI mode within them, so you
could actually enable the AHCI mode and disable IDE mode if you know
what you're doing - you will probably need to assign the IDE PCI BARs
correctly first if you want IDE mode in such controller, or ignore it
and go with AHCI mode instead. Also, this emulation component is only
about PCI IDE native mode currently, but we can easily put it that you
can switch channels between compatibility mode and native mode if wanted
to. My ICH7 test machine has such controller - it allows you to switch
between the two modes, so the OS can decide what to do with the IDE
controller according to its needs.
take care,
Gerd
Best regards,
Liav
[PATCH 2/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller
is present in the system or not, so it doesn't need to do probing of the
controller, but just initialize it immediately, before enumerating the
ACPI AML namespace.
Signed-off-by: Liav Albani
---
hw/acpi/aml-build.c | 7 ++-
hw/i386/acpi-build.c| 5 +
hw/i386/acpi-microvm.c | 5 +
include/hw/acpi/acpi-defs.h | 1 +
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8966e16320..ef5f4cad87 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2152,7 +2152,12 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+/* IAPC_BOOT_ARCH */
+if (f->rev == 1) {
+build_append_int_noprefix(tbl, 0, 2);
+} else {
+build_append_int_noprefix(tbl, f->iapc_boot_arch, 2);
+}
build_append_int_noprefix(tbl, 0, 1); /* Reserved */
build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ebd47aa26f..5dc625b8d8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,6 +192,11 @@ static void init_common_fadt_data(MachineState *ms, Object
*o,
.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
},
};
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+fadt.iapc_boot_arch = (1 << 1);
+}
+
*data = fadt;
}
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 68ca7e7fc2..756c69b3b0 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -189,6 +189,11 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
.reset_val = ACPI_GED_RESET_VALUE,
};
+if (isa_check_device_existence("i8042")) {
+/* Indicates if i8042 is present or not */
+pmfadt.iapc_boot_arch = (1 << 1);
+}
+
table_offsets = g_array_new(false, true /* clear */,
sizeof(uint32_t));
bios_linker_loader_alloc(tables->linker,
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c97e8633ad..2b42e4192b 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -77,6 +77,7 @@ typedef struct AcpiFadtData {
uint16_t plvl2_lat;/* P_LVL2_LAT */
uint16_t plvl3_lat;/* P_LVL3_LAT */
uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
+uint16_t iapc_boot_arch; /* IAPC_BOOT_ARCH */
uint8_t minor_ver; /* FADT Minor Version */
/*
--
2.35.1
[PATCH 1/2] hw/isa: add function to check for existence of device by its type
Signed-off-by: Liav Albani
---
hw/isa/isa-bus.c | 17 +
include/hw/isa/isa.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 6c31398dda..39d1768797 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -222,6 +222,23 @@ void isa_build_aml(ISABus *bus, Aml *scope)
}
}
+bool isa_check_device_existence(const char *typename)
+{
+assert(isabus != NULL);
+
+BusChild *kid;
+ISADevice *dev;
+
+QTAILQ_FOREACH(kid, >parent_obj.children, sibling) {
+dev = ISA_DEVICE(kid->child);
+const char *object_type = object_get_typename(OBJECT(dev));
+if (object_type && strcmp(object_type, typename) == 0) {
+return true;
+}
+}
+return false;
+}
+
static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
{
ISADevice *d = ISA_DEVICE(dev);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index d4417b34b6..65f0c7e28c 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -99,6 +99,7 @@ IsaDma *isa_get_dma(ISABus *bus, int nchan);
MemoryRegion *isa_address_space(ISADevice *dev);
MemoryRegion *isa_address_space_io(ISADevice *dev);
ISADevice *isa_new(const char *name);
+bool isa_check_device_existence(const char *typename);
ISADevice *isa_try_new(const char *name);
bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
ISADevice *isa_create_simple(ISABus *bus, const char *name);
--
2.35.1
[PATCH 0/2] hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table
This can allow the guest OS to determine more easily if i8042 controller is present in the system or not, so it doesn't need to do probing of the controller, but just initialize it immediately, before enumerating the ACPI AML namespace. To allow "flexible" indication, I don't hardcode the bit at location 1 as on in the IA-PC boot flags, but try to search for i8042 on the ISA bus to verify it exists in the system. Why this is useful you might ask - this patch allows the guest OS to probe and use the i8042 controller without decoding the ACPI AML blob at all. For example, as a developer of the SerenityOS kernel, I might want to allow people to not try to decode the ACPI AML namespace (for now, we still don't support ACPI AML as it's a work in progress), but still to not probe for the i8042 but just use it after looking in the IA-PC boot flags in the ACPI FADT table. Liav Albani (2): hw/isa: add function to check for existence of device by its type hw/acpi: add indication for i8042 in IA-PC boot flags of the FADT table hw/acpi/aml-build.c | 7 ++- hw/i386/acpi-build.c| 5 + hw/i386/acpi-microvm.c | 5 + hw/isa/isa-bus.c| 17 + include/hw/acpi/acpi-defs.h | 1 + include/hw/isa/isa.h| 1 + 6 files changed, 35 insertions(+), 1 deletion(-) -- 2.35.1
Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c
On 2/19/22 17:57, BALATON Zoltan wrote:
On Sat, 19 Feb 2022, BALATON Zoltan wrote:
Maybe even the first if that's already contained in the default
function could be avoided with some reorganisation like
if (size == 1) {
remaining switch cases to set val
} else {
val = bmdma_default_read();
}
On second thought this misses the cases where size == 1 and addr is
those handeled in bmdma_default_read so one would still need the
default case in the if part and then it's not much better than
duplicating the if. Maybe calling the default first, then handling the
remaining cases, like
val = bmdma_default_read();
if (size == 1) {
remaining switch cases to set val
}
return val;
is the simplest and avoids the duplicated if. (Now we only have two
trace messages instead of one but that's probably not a problem as
it's only a debugging aid.
Hmm, is it OK though to have two trace messages? I'm not against it, but
if someone tries to use the debug messages to see what's going on, it's
better to not have two of the same message as it will confuse people. We
definitely don't want that to happen.
So, let's keep it simple (and remove code duplications) but also let's
do that correctly, to ensure that in the view of the developer that uses
the debug messages, it all seem clear and neat :)
but I wasn't sure that won't change anything so may need a bit more
thought.
Signed-off-by: Liav Albani
---
hw/ide/pci.c | 47
hw/ide/piix.c | 50 ++-
hw/ide/via.c | 51
++--
include/hw/ide/pci.h | 4
4 files changed, 55 insertions(+), 97 deletions(-)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 84ba733548..c8b867659a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
.reset = bmdma_reset,
};
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size)
Indentation off? Also everywhere else, usually we indent not with
the parenthesis but with the list within. (Auto indent in most
editors does that probably.)
I guess you mean that it should be:
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size)
Like this?
No, like the code you've moved had it. The split line should start
after the ( not on the same column. So:
uint64_t bmdma_default_read(void *opaque, hwaddr addr,
unsigned size)
but this line does not need to be split at all as it fits within 80
chars so better to keep it one line and only split where needed.
I'm using Visual Studio Code, so I might not have the correct
settings for this editor with QEMU.
The checkpatch script doesn't complain on style issues, so what can
I do to make this correct?
If checkpatch is happy then probably not a problem but just look at
how code is indented on other places and follow the same. The coding
style doc may have some words on it too. I don't know what setting
Visual Studio might need.
OK. I'll align it to the character after the start of the parenthesis.
I'll take a look into other code snippets in QEMU, but at least in the
IDE code, there are lots of code style issues (the checkpatch script
says so) so I'll probably look into other parts of QEMU to see how it goes.
I'll take this a bit slow, as I wanted to send v2 today. This is
probably not a good idea as we should let people to see this and maybe
to let the maintainer (John) to look into this and put his comments on
this too. I'll give this a couple of days - no rush here, although I'd
be very happy to see things going forward as soon as possible, so we
merge this patch and then going back to the ich6-ide patch, and then
hopefully more patches to the IDE code to add more features and fixes in
this part of QEMU codebase.
Thanks again for the suggestions, these are awesome and I really
appreciate the effort you put into these!
Best regards,
Liav
Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c
On 2/19/22 13:19, BALATON Zoltan wrote:
On Sat, 19 Feb 2022, Liav Albani wrote:
Instead of letting each implementation to duplicate this code, we can
share these functions between IDE PIIX3/4 and VIA implementations.
OK but there's a way to take this even further as cmd646 also uses
similar functions just with more cases so you could remove the cases
handled by these functions and only leave the difference and call the
default function from the default case. E.g. (untested, just to show
the idea):
hw/ide/cmd646.c:
static uint64_t bmdma_read(void *opaque, hwaddr addr,
unsigned size)
{
BMDMAState *bm = opaque;
PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
uint32_t val;
if (size != 1) {
return ((uint64_t)1 << (size * 8)) - 1;
}
switch(addr & 3) {
case 1:
val = pci_dev->config[MRDMODE];
break;
case 3:
if (bm == >pci_dev->bmdma[0]) {
val = pci_dev->config[UDIDETCR0];
} else {
val = pci_dev->config[UDIDETCR1];
}
break;
default:
val = bmdma_default_read(opaque, addr, size);
break;
}
trace_bmdma_read_cmd646(addr, val);
return val;
}
Yeah, I see how I can do that for both bmdma write and read of cmd646.
I'll send a v2 right away with a fix.
Signed-off-by: Liav Albani
---
hw/ide/pci.c | 47
hw/ide/piix.c | 50 ++-
hw/ide/via.c | 51 ++--
include/hw/ide/pci.h | 4
4 files changed, 55 insertions(+), 97 deletions(-)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 84ba733548..c8b867659a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
.reset = bmdma_reset,
};
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size)
Indentation off? Also everywhere else, usually we indent not with the
parenthesis but with the list within. (Auto indent in most editors
does that probably.)
I guess you mean that it should be:
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size)
Like this?
I'm using Visual Studio Code, so I might not have the correct settings
for this editor with QEMU.
The checkpatch script doesn't complain on style issues, so what can I do
to make this correct?
Best regards,
Liav
Regards,
BALATON Zoltan
[PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c
Instead of letting each implementation to duplicate this code, we can
share these functions between IDE PIIX3/4 and VIA implementations.
Signed-off-by: Liav Albani
---
hw/ide/pci.c | 47
hw/ide/piix.c| 50 ++-
hw/ide/via.c | 51 ++--
include/hw/ide/pci.h | 4
4 files changed, 55 insertions(+), 97 deletions(-)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 84ba733548..c8b867659a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
.reset = bmdma_reset,
};
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+BMDMAState *bm = opaque;
+uint32_t val;
+
+if (size != 1) {
+return ((uint64_t)1 << (size * 8)) - 1;
+}
+
+switch (addr & 3) {
+case 0:
+val = bm->cmd;
+break;
+case 2:
+val = bm->status;
+break;
+default:
+val = 0xff;
+break;
+}
+
+trace_bmdma_read_via(addr, val);
+return val;
+}
+
+void bmdma_default_write(void *opaque, hwaddr addr,
+uint64_t val, unsigned size)
+{
+BMDMAState *bm = opaque;
+
+if (size != 1) {
+return;
+}
+
+trace_bmdma_write_via(addr, val);
+switch (addr & 3) {
+case 0:
+bmdma_cmd_writeb(bm, val);
+break;
+case 2:
+bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val &
0x06);
+break;
+default:;
+}
+}
+
void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
{
if (bus->dma == >dma) {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..fdf3a04cb1 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -35,55 +35,9 @@
#include "hw/ide/pci.h"
#include "trace.h"
-static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
-{
-BMDMAState *bm = opaque;
-uint32_t val;
-
-if (size != 1) {
-return ((uint64_t)1 << (size * 8)) - 1;
-}
-
-switch(addr & 3) {
-case 0:
-val = bm->cmd;
-break;
-case 2:
-val = bm->status;
-break;
-default:
-val = 0xff;
-break;
-}
-
-trace_bmdma_read(addr, val);
-return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
-uint64_t val, unsigned size)
-{
-BMDMAState *bm = opaque;
-
-if (size != 1) {
-return;
-}
-
-trace_bmdma_write(addr, val);
-
-switch(addr & 3) {
-case 0:
-bmdma_cmd_writeb(bm, val);
-break;
-case 2:
-bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val &
0x06);
-break;
-}
-}
-
static const MemoryRegionOps piix_bmdma_ops = {
-.read = bmdma_read,
-.write = bmdma_write,
+.read = bmdma_default_read,
+.write = bmdma_default_write,
};
static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 82def819c4..13f27c9514 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -33,56 +33,9 @@
#include "hw/ide/pci.h"
#include "trace.h"
-static uint64_t bmdma_read(void *opaque, hwaddr addr,
- unsigned size)
-{
-BMDMAState *bm = opaque;
-uint32_t val;
-
-if (size != 1) {
-return ((uint64_t)1 << (size * 8)) - 1;
-}
-
-switch (addr & 3) {
-case 0:
-val = bm->cmd;
-break;
-case 2:
-val = bm->status;
-break;
-default:
-val = 0xff;
-break;
-}
-
-trace_bmdma_read_via(addr, val);
-return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
-uint64_t val, unsigned size)
-{
-BMDMAState *bm = opaque;
-
-if (size != 1) {
-return;
-}
-
-trace_bmdma_write_via(addr, val);
-switch (addr & 3) {
-case 0:
-bmdma_cmd_writeb(bm, val);
-break;
-case 2:
-bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val &
0x06);
-break;
-default:;
-}
-}
-
static const MemoryRegionOps via_bmdma_ops = {
-.read = bmdma_read,
-.write = bmdma_write,
+.read = bmdma_default_read,
+.write = bmdma_default_write,
};
static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..159136f055 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,10 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
}
void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size);
+void bmdma_default_write(void *opaque, hwaddr addr,
[PATCH 0/1] hw/ide: share bmdma read and write functions
This is a preparation before I send v3 of ich6-ide controller emulation patch. I figured that it's more trivial to split the changes this way, by extracting the bmdma functions from via.c and piix.c and sharing them together. Then, I could easily put these into use when I send v3 of the ich6-ide patch by just using the already separated functions. This was suggested by BALATON Zoltan when he submitted a code review on my ich6-ide controller emulation patch. Liav Albani (1): hw/ide: share bmdma read and write functions between piix.c and via.c hw/ide/pci.c | 47 hw/ide/piix.c| 50 ++- hw/ide/via.c | 51 ++-- include/hw/ide/pci.h | 4 4 files changed, 55 insertions(+), 97 deletions(-) -- 2.35.1
Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation
On 2/19/22 02:50, BALATON Zoltan wrote:
+/*
+ * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.
This is a small thing, but if these two are the same maybe keeping
this comment but using the ich7 name everywhere else would make it
less likely to get it confused with ich9. I mean ich6 and ich9 is
easily confused, while ich7 is clearly distinct. But maybe it's just
me, calling it ich6 is also correct and can be preferred by someone else.
ICH6 and ICH7 IDE controllers are quite the same as far as I know. I
could change it, but then one could argue that the name ich6-ide seems
like "ich9-ide", so not sure if we can really go on this path.
+static void ich6_pci_config_write(PCIDevice *d, uint32_t addr,
uint32_t val,
+ int l)
+{
+ uint32_t i;
+
+ pci_default_write_config(d, addr, val, l);
+
+ for (i = addr; i < addr + l; i++) {
+ switch (i) {
+ case 0x40:
+ pci_default_write_config(d, i, 0x8000, 2);
+ continue;
+ case 0x42:
+ pci_default_write_config(d, i, 0x8000, 2);
+ continue;
+ }
+ }
I'm not sure what this tries to do but this for cycle looks suspicious
here. It's also only calls pci_default_write_config() so why didn't
the default worked and why was this override needed?
It's just a loop to ensure that if the guest OS tries to disable an IDE
channel from the PCI config space, it seems "stuck" on enabled. I should
probably put a comment on this to explain this, but I definitely don't
want the guest OS to be able to disable any IDE channel for now (on real
hardware, it does that, but I think we can either skip this feature or
implement in a future patch, as Linux deals with the controller just fine).
+}
+
+static void ich6_ide_reset(DeviceState *dev)
+{
+ PCIIDEState *d = PCI_IDE(dev);
+ PCIDevice *pd = PCI_DEVICE(d);
+ uint8_t *pci_conf = pd->config;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ ide_bus_reset(>bus[i]);
+ }
+
+ /* TODO: this is the default. do not override. */
+ pci_conf[PCI_COMMAND] = 0x00;
+ /* TODO: this is the default. do not override. */
+ pci_conf[PCI_COMMAND + 1] = 0x00;
+ /* TODO: use pci_set_word */
+ pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
+ pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
+ pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
+}
+
+static int pci_ich6_init_ports(PCIIDEState *d)
+{
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ ide_bus_init(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
+ ide_init2(>bus[i], d->native_irq);
+
+ bmdma_init(>bus[i], >bmdma[i], d);
+ d->bmdma[i].bus = >bus[i];
+ ide_register_restart_cb(>bus[i]);
+ }
+
+ return 0;
+}
+
+static void pci_ich6_ide_realize(PCIDevice *dev, Error **errp)
+{
+ PCIIDEState *d = PCI_IDE(dev);
+ uint8_t *pci_conf = dev->config;
+ int rc;
All in all this device looks very similar to piix ide devices with
only some differentces so I wonder if ir could be implemented as
another device in piix.c. We already have 3 variants there: piix3-ide,
piix3-ide-xen, and piix4-ide so maybe putting this device in piix.c
could avoid some code duplication. In that case moving out
bmdma_{read,write} were not needed either although maybe that's a good
idea anyway to share it with other devices.
As for putting the ich6-ide code in piix.c - I'm not against it.
Although, in the long run, if I send more patches to QEMU, I rather
split the files a bit more in the /hw/ide directory as there's a lot of
code duplication to solve.
So, what we could do instead, is to share more code between the devices
and still keep them in separate files.
As for moving out the bmdma_{write,read} functions - I'm still in the
previous mind that we need to move them out as via.c shares the same
code but currently has code duplications for it, and that I want to
solve as part of making the IDE code more clean and to add more features.
However, if it seems necessary to do this cleanup before implementing
this ich6-ide controller, I'm more than happy to wait on this, send a
patch to solve and clean up some issues in the IDE code, and then
re-send this as v3.
I might even consider doing that now if nobody rejects this idea :)
+
+ pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin A */
+
+ /* PCI native mode-only controller, supports bus mastering */
+ pci_conf[PCI_CLASS_PROG] = 0x85;
+
+ bmdma_setup_bar(d);
+ pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
+
+ d->native_irq = pci_allocate_irq(>parent_obj);
Is this irq and the new field in PCIIDEState really needed? If this
device is using PCI interrupts could it do the same as CMD646 ide does
instead?
I looked into how cmd646.c does that, but it creates that with the
qdev_init_gpio_in function. The AHCI controller seems to use
pci_allocate_irq function (in ich.c), as well as other hardware devices
in QEMU,
Re: [PATCH v2 1/2] hw/ide: split bmdma read and write functions from piix.c
On 2/19/22 02:12, BALATON Zoltan wrote: On Fri, 18 Feb 2022, Liav Albani wrote: This is a preparation before implementing another PCI IDE controller that relies on these functions, so these can be shared between both implementations. Signed-off-by: Liav Albani --- hw/ide/bmdma.c | 84 ++ hw/ide/meson.build | 2 +- hw/ide/piix.c | 51 ++--- include/hw/ide/bmdma.h | 34 + 4 files changed, 122 insertions(+), 49 deletions(-) create mode 100644 hw/ide/bmdma.c create mode 100644 include/hw/ide/bmdma.h diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c new file mode 100644 index 00..d12ed730dd --- /dev/null +++ b/hw/ide/bmdma.c @@ -0,0 +1,84 @@ +/* + * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 and ICH6/7. + * + * Copyright (c) 2003 Fabrice Bellard + * Copyright (c) 2006 Openedhand Ltd. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "qemu/osdep.h" +#include "hw/pci/pci.h" +#include "migration/vmstate.h" +#include "qapi/error.h" +#include "qemu/module.h" +#include "sysemu/block-backend.h" +#include "sysemu/blockdev.h" +#include "sysemu/dma.h" + +#include "hw/ide/bmdma.h" +#include "hw/ide/pci.h" +#include "trace.h" Are you sure you need all these includes? I haven't checked but I think there may be some unneeded ones here. On the other hand I'm not sure putting these in a new file is worth it. There are already some bmdma_* functions in hw/ide/pci.c which are declared in include/hw/ide/pci.h so you could just move these there too then no new file, new header nor changes to meson.build is needed in this patch.. Good question, probably not. I'll try to build without them, adding only what seems necessary to complete the build. Also, I think adding those functions to hw/ide/pci.c is a very good idea as it will make the patch smaller and it also makes total sense to me - Bus Master capabilities only appear on PCI IDE controllers and not on the ISA IDE controller (AFAIK). It will happen in v3 :) + +uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size) As I said before these aren't intel specific as the same functions also appear in other ide controllers such as via.c so maybe a better name would be bmdma_default_read and bmdma_default_write or somehting similar so these can be also reused by other non-intel devices too. Right, I see now that via.c uses the exact same functions... I'll change it in v3. The names bmdma_default_read and bmdma_default_write seem like a good choice to me. Regards, BALATON Zoltan Thanks for the review!
[PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation
This type of IDE controller has support for relocating the IO ports and
doesn't use IRQ 14 and 15 but one allocated PCI IRQ for the controller.
There's no x86 chipset in QEMU that will try to attach this device by
default. It is considered a legacy-free device in the aspect of PCI bus
resource management as the guest OS can relocate the IO ports as it sees
fit to its needs. However, this is still a legacy device that belongs to
chipsets from late 2000s.
Signed-off-by: Liav Albani
---
hw/i386/Kconfig | 2 +
hw/ide/Kconfig | 5 +
hw/ide/ich6.c| 204 +++
hw/ide/meson.build | 1 +
include/hw/ide/pci.h | 1 +
include/hw/pci/pci_ids.h | 1 +
6 files changed, 214 insertions(+)
create mode 100644 hw/ide/ich6.c
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d22ac4a4b9..a18de2d962 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -75,6 +75,7 @@ config I440FX
select PCI_I440FX
select PIIX3
select IDE_PIIX
+select IDE_ICH6
select DIMM
select SMBIOS
select FW_CFG_DMA
@@ -101,6 +102,7 @@ config Q35
select PCI_EXPRESS_Q35
select LPC_ICH9
select AHCI_ICH9
+select IDE_ICH6
select DIMM
select SMBIOS
select FW_CFG_DMA
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index dd85fa3619..63304325a5 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -38,6 +38,11 @@ config IDE_VIA
select IDE_PCI
select IDE_QDEV
+config IDE_ICH6
+bool
+select IDE_PCI
+select IDE_QDEV
+
config MICRODRIVE
bool
select IDE_QDEV
diff --git a/hw/ide/ich6.c b/hw/ide/ich6.c
new file mode 100644
index 00..8f46d3fce2
--- /dev/null
+++ b/hw/ide/ich6.c
@@ -0,0 +1,204 @@
+/*
+ * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.
+ *
+ * Copyright (c) 2022 Liav Albani
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/dma.h"
+
+#include "hw/ide/pci.h"
+#include "hw/ide/bmdma.h"
+#include "trace.h"
+
+static const MemoryRegionOps ich6_bmdma_ops = {
+.read = intel_ide_bmdma_read,
+.write = intel_ide_bmdma_write,
+};
+
+static void bmdma_setup_bar(PCIIDEState *d)
+{
+int i;
+
+memory_region_init(>bmdma_bar, OBJECT(d), "ich6-bmdma-container", 16);
+for (i = 0; i < 2; i++) {
+BMDMAState *bm = >bmdma[i];
+
+memory_region_init_io(>extra_io, OBJECT(d), _bmdma_ops, bm,
+ "ich6-bmdma", 4);
+memory_region_add_subregion(>bmdma_bar, i * 8, >extra_io);
+memory_region_init_io(>addr_ioport, OBJECT(d),
+ _addr_ioport_ops, bm, "bmdma", 4);
+memory_region_add_subregion(>bmdma_bar, i * 8 + 4,
>addr_ioport);
+}
+}
+
+static void ich6_pci_config_write(PCIDevice *d, uint32_t addr, uint32_t val,
+int l)
+{
+uint32_t i;
+
+pci_default_write_config(d, addr, val, l);
+
+for (i = addr; i < addr + l; i++) {
+switch (i) {
+case 0x40:
+pci_default_write_config(d, i, 0x8000, 2);
+continue;
+case 0x42:
+pci_default_write_config(d, i, 0x8000, 2);
+continue;
+}
+}
+}
+
+static void ich6_ide_reset(DeviceState *dev)
+{
+PCIIDEState *d = PCI_IDE(dev);
+PCIDevice *pd = PCI_DEVICE(d);
+uint8_t *pci_conf = pd->config;
+int i;
+
+for (i = 0; i < 2; i++) {
+ide_bus_reset(>bus[i]);
+}
+
+/* TODO: this is the default. do not override. */
+pci_conf[PCI_COMMAND] = 0x00;
+/*
[PATCH v2 1/2] hw/ide: split bmdma read and write functions from piix.c
This is a preparation before implementing another PCI IDE controller
that relies on these functions, so these can be shared between both
implementations.
Signed-off-by: Liav Albani
---
hw/ide/bmdma.c | 84 ++
hw/ide/meson.build | 2 +-
hw/ide/piix.c | 51 ++---
include/hw/ide/bmdma.h | 34 +
4 files changed, 122 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/bmdma.c
create mode 100644 include/hw/ide/bmdma.h
diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c
new file mode 100644
index 00..d12ed730dd
--- /dev/null
+++ b/hw/ide/bmdma.c
@@ -0,0 +1,84 @@
+/*
+ * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 and ICH6/7.
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2006 Openedhand Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/dma.h"
+
+#include "hw/ide/bmdma.h"
+#include "hw/ide/pci.h"
+#include "trace.h"
+
+uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size)
+{
+BMDMAState *bm = opaque;
+uint32_t val;
+
+if (size != 1) {
+return ((uint64_t)1 << (size * 8)) - 1;
+}
+
+switch (addr & 3) {
+case 0:
+val = bm->cmd;
+break;
+case 2:
+val = bm->status;
+break;
+default:
+val = 0xff;
+break;
+}
+
+trace_bmdma_read(addr, val);
+return val;
+}
+
+void intel_ide_bmdma_write(void *opaque, hwaddr addr,
+uint64_t val, unsigned size)
+{
+BMDMAState *bm = opaque;
+
+if (size != 1) {
+return;
+}
+
+trace_bmdma_write(addr, val);
+
+switch (addr & 3) {
+case 0:
+bmdma_cmd_writeb(bm, val);
+break;
+case 2:
+uint8_t cur_val = bm->status;
+bm->status = (val & 0x60) | (cur_val & 1) | (cur_val & ~val & 0x06);
+break;
+}
+}
diff --git a/hw/ide/meson.build b/hw/ide/meson.build
index ddcb3b28d2..38f9ae7178 100644
--- a/hw/ide/meson.build
+++ b/hw/ide/meson.build
@@ -7,7 +7,7 @@ softmmu_ss.add(when: 'CONFIG_IDE_ISA', if_true: files('isa.c',
'ioport.c'))
softmmu_ss.add(when: 'CONFIG_IDE_MACIO', if_true: files('macio.c'))
softmmu_ss.add(when: 'CONFIG_IDE_MMIO', if_true: files('mmio.c'))
softmmu_ss.add(when: 'CONFIG_IDE_PCI', if_true: files('pci.c'))
-softmmu_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c'))
+softmmu_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c',
'bmdma.c'))
softmmu_ss.add(when: 'CONFIG_IDE_QDEV', if_true: files('qdev.c'))
softmmu_ss.add(when: 'CONFIG_IDE_SII3112', if_true: files('sii3112.c'))
softmmu_ss.add(when: 'CONFIG_IDE_VIA', if_true: files('via.c'))
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..8f94809eee 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -33,57 +33,12 @@
#include "sysemu/dma.h"
#include "hw/ide/pci.h"
+#include "hw/ide/bmdma.h"
#include "trace.h"
-static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
-{
-BMDMAState *bm = opaque;
-uint32_t val;
-
-if (size != 1) {
-return ((uint64_t)1 << (size * 8)) - 1;
-}
-
-switch(addr & 3) {
-case 0:
-val = bm->cmd;
-break;
-case 2:
-val = bm->status;
-break;
-default:
-val = 0xff;
-break;
-}
-
-trace_bmdma_read(addr, val);
-return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
-uint64_t val, unsig
[PATCH v2 0/2] hw/ide: implement ich6 ide controller support
This is version 2 of this patch, this time a patch series, after following the suggestions from BALATON Zoltan. I implemented this device because I have an old machine from 2009 which has the ICH7 south bridge in it, so when I tried to run Linux on it, it booted just fine (as you might expect), but when I tried to boot with the SerenityOS kernel, it struggled to initialize the IDE controller. Therefore, upstreaming these changes might be beneficial to other OS developers and hobbyists out there, and I will use this to fix the issues within the SerenityOS kernel, without the need of preparing a bare metal setup each time I need to test the code of the kernel. Please keep in mind that while this is usable right now with the Q35 chipset, when trying to boot with an i440FX machine, SeaBIOS doesn't handle this device very well, so it tries no matter what type of IDE controller it sees to assign the IO ports to legacy values. I have a patch I wrote locally which I gladly will send to SeaBIOS, that fixes this problem by ensuring that when attaching a storage device to this controller, SeaBIOS will relocate the IO ports to other values so there's no collision with the default PIIX3/4 IDE controller. Even if SeaBIOS didn't configure this device correctly, Linux will relocate the IO ports and the user can still use the attached storage devices, given that the user managed to boot from a storage device that is not attached to the ICH6 IDE controller but to other storage controller in the system. Liav Albani (2): hw/ide: split bmdma read and write functions from piix.c hw/ide: add ich6 ide controller device emulation hw/i386/Kconfig | 2 + hw/ide/Kconfig | 5 + hw/ide/bmdma.c | 84 hw/ide/ich6.c| 204 +++ hw/ide/meson.build | 3 +- hw/ide/piix.c| 51 +- include/hw/ide/bmdma.h | 34 +++ include/hw/ide/pci.h | 1 + include/hw/pci/pci_ids.h | 1 + 9 files changed, 336 insertions(+), 49 deletions(-) create mode 100644 hw/ide/bmdma.c create mode 100644 hw/ide/ich6.c create mode 100644 include/hw/ide/bmdma.h -- 2.35.1
Re: [PATCH] hw/ide: implement ich6 ide controller support
Hello BALATON, Thank you for helping keeping this patch noticeable to everyone :) I tried to reach out to John via a private email last Saturday (two days ago) so I don't "spam" the mailing list for no good reason. It might be that I should actually refrain from doing so and talk to the maintainer directly on the mailing list once the patch has been submitted to the mailing list. I've not yet seen any response from John so I assume it's a matter of days before he can take care of this. Best regards, Liav On 2/14/22 14:26, BALATON Zoltan wrote: On Sat, 5 Feb 2022, BALATON Zoltan wrote: Hello, Ping? John, do you agree with my comments? Should Liav proceed to send a v2? Thanks, BALATON Zoltan On Sat, 5 Feb 2022, Liav Albani wrote: On 2/5/22 17:48, BALATON Zoltan wrote: On Sat, 5 Feb 2022, Liav Albani wrote: This type of IDE controller has support for relocating the IO ports and doesn't use IRQ 14 and 15 but one allocated PCI IRQ for the controller. I haven't looked at in detail so only a few comments I've got while reading it. What machine needs this? In QEMU I think we only have piix and ich9 emulated for pc and q35 machines but maybe ich6 is also used by some machine I don't know about. Otherwise it looks odd to have ide part of ich6 but not the other parts of this chip. Hi BALATON, This is my first patch to QEMU and the first time I send patches over the mail. I sent my github tree to John Snow (the maintainer of the IDE code in QEMU) for advice if I should send them here and I was encouraged to do that. Welcome and thanks a lot for taking time to contribute and share your results. In case you're not yet aware, these docs should explain how patches are handled on the list: https://www.qemu.org/docs/master/devel/submitting-a-patch.html For the next time patch I'll put a note on writing a descriptive cover letter as it could have put more valuable details on why I sent this patch. There's no such machine type emulating the ICH6 chipset in QEMU. However, I wrote this emulation component as a test for the SerenityOS kernel because I have a machine from 2009 which has an ICH7 southbridge, so, I wanted to emulate such device with QEMU to ease development on it. I found out that Linux with libata was using the controller without any noticeable problems, but the SerenityOS kernel struggled to use this device, so I decided that I should send this patch to get it merged and then I can use it locally and maybe other people will benefit from it. In regard to other components of the ICH6 chipset - I don't think it's worth anybody's time to actually implement them as the ICH9 chipset is quite close to what the ICH6 chipset offers as far as I can tell. The idea of implementing ich6-ide controller was to enable the option of people like me and other OS developers to ensure their kernels operate correctly on such type of device, which is legacy-free device in the aspect of PCI bus resource management but still is a legacy device which belongs to chipsets of late 2000s. That's OK, maybe a short mention (just one sentence) in the commit message explaining this would help to understand why this device model was added. Signed-off-by: Liav Albani --- hw/i386/Kconfig | 2 + hw/ide/Kconfig | 5 + hw/ide/bmdma.c | 83 +++ hw/ide/ich6.c | 211 +++ hw/ide/meson.build | 3 +- hw/ide/piix.c | 50 +- include/hw/ide/pci.h | 5 + include/hw/pci/pci_ids.h | 1 + 8 files changed, 311 insertions(+), 49 deletions(-) create mode 100644 hw/ide/bmdma.c create mode 100644 hw/ide/ich6.c diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig index d22ac4a4b9..a18de2d962 100644 --- a/hw/i386/Kconfig +++ b/hw/i386/Kconfig @@ -75,6 +75,7 @@ config I440FX select PCI_I440FX select PIIX3 select IDE_PIIX + select IDE_ICH6 select DIMM select SMBIOS select FW_CFG_DMA @@ -101,6 +102,7 @@ config Q35 select PCI_EXPRESS_Q35 select LPC_ICH9 select AHCI_ICH9 + select IDE_ICH6 select DIMM select SMBIOS select FW_CFG_DMA diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig index dd85fa3619..63304325a5 100644 --- a/hw/ide/Kconfig +++ b/hw/ide/Kconfig @@ -38,6 +38,11 @@ config IDE_VIA select IDE_PCI select IDE_QDEV +config IDE_ICH6 + bool + select IDE_PCI + select IDE_QDEV + config MICRODRIVE bool select IDE_QDEV diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c new file mode 100644 index 00..979f5974fd --- /dev/null +++ b/hw/ide/bmdma.c @@ -0,0 +1,83 @@ +/* + * QEMU IDE Emulation: PCI PIIX3/4 support. + * + * Copyright (c) 2003 Fabrice Bellard + * Copyright (c) 2006 Openedhand Ltd. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restrictio
Re: [PATCH] hw/ide: implement ich6 ide controller support
On 2/5/22 17:48, BALATON Zoltan wrote: On Sat, 5 Feb 2022, Liav Albani wrote: This type of IDE controller has support for relocating the IO ports and doesn't use IRQ 14 and 15 but one allocated PCI IRQ for the controller. I haven't looked at in detail so only a few comments I've got while reading it. What machine needs this? In QEMU I think we only have piix and ich9 emulated for pc and q35 machines but maybe ich6 is also used by some machine I don't know about. Otherwise it looks odd to have ide part of ich6 but not the other parts of this chip. Hi BALATON, This is my first patch to QEMU and the first time I send patches over the mail. I sent my github tree to John Snow (the maintainer of the IDE code in QEMU) for advice if I should send them here and I was encouraged to do that. For the next time patch I'll put a note on writing a descriptive cover letter as it could have put more valuable details on why I sent this patch. There's no such machine type emulating the ICH6 chipset in QEMU. However, I wrote this emulation component as a test for the SerenityOS kernel because I have a machine from 2009 which has an ICH7 southbridge, so, I wanted to emulate such device with QEMU to ease development on it. I found out that Linux with libata was using the controller without any noticeable problems, but the SerenityOS kernel struggled to use this device, so I decided that I should send this patch to get it merged and then I can use it locally and maybe other people will benefit from it. In regard to other components of the ICH6 chipset - I don't think it's worth anybody's time to actually implement them as the ICH9 chipset is quite close to what the ICH6 chipset offers as far as I can tell. The idea of implementing ich6-ide controller was to enable the option of people like me and other OS developers to ensure their kernels operate correctly on such type of device, which is legacy-free device in the aspect of PCI bus resource management but still is a legacy device which belongs to chipsets of late 2000s. Signed-off-by: Liav Albani --- hw/i386/Kconfig | 2 + hw/ide/Kconfig | 5 + hw/ide/bmdma.c | 83 +++ hw/ide/ich6.c | 211 +++ hw/ide/meson.build | 3 +- hw/ide/piix.c | 50 +- include/hw/ide/pci.h | 5 + include/hw/pci/pci_ids.h | 1 + 8 files changed, 311 insertions(+), 49 deletions(-) create mode 100644 hw/ide/bmdma.c create mode 100644 hw/ide/ich6.c diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig index d22ac4a4b9..a18de2d962 100644 --- a/hw/i386/Kconfig +++ b/hw/i386/Kconfig @@ -75,6 +75,7 @@ config I440FX select PCI_I440FX select PIIX3 select IDE_PIIX + select IDE_ICH6 select DIMM select SMBIOS select FW_CFG_DMA @@ -101,6 +102,7 @@ config Q35 select PCI_EXPRESS_Q35 select LPC_ICH9 select AHCI_ICH9 + select IDE_ICH6 select DIMM select SMBIOS select FW_CFG_DMA diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig index dd85fa3619..63304325a5 100644 --- a/hw/ide/Kconfig +++ b/hw/ide/Kconfig @@ -38,6 +38,11 @@ config IDE_VIA select IDE_PCI select IDE_QDEV +config IDE_ICH6 + bool + select IDE_PCI + select IDE_QDEV + config MICRODRIVE bool select IDE_QDEV diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c new file mode 100644 index 00..979f5974fd --- /dev/null +++ b/hw/ide/bmdma.c @@ -0,0 +1,83 @@ +/* + * QEMU IDE Emulation: PCI PIIX3/4 support. + * + * Copyright (c) 2003 Fabrice Bellard + * Copyright (c) 2006 Openedhand Ltd. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "qemu/osdep.h" +#include "hw/pci/pci.h" +#include "migration/vmstate.h" +#include "qapi/error.h" +#include "qemu/module.h" +#include "sysemu/block-backend.h" +#include "syse
[PATCH] hw/ide: implement ich6 ide controller support
This type of IDE controller has support for relocating the IO ports and
doesn't use IRQ 14 and 15 but one allocated PCI IRQ for the controller.
Signed-off-by: Liav Albani
---
hw/i386/Kconfig | 2 +
hw/ide/Kconfig | 5 +
hw/ide/bmdma.c | 83 +++
hw/ide/ich6.c| 211 +++
hw/ide/meson.build | 3 +-
hw/ide/piix.c| 50 +-
include/hw/ide/pci.h | 5 +
include/hw/pci/pci_ids.h | 1 +
8 files changed, 311 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/bmdma.c
create mode 100644 hw/ide/ich6.c
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d22ac4a4b9..a18de2d962 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -75,6 +75,7 @@ config I440FX
select PCI_I440FX
select PIIX3
select IDE_PIIX
+select IDE_ICH6
select DIMM
select SMBIOS
select FW_CFG_DMA
@@ -101,6 +102,7 @@ config Q35
select PCI_EXPRESS_Q35
select LPC_ICH9
select AHCI_ICH9
+select IDE_ICH6
select DIMM
select SMBIOS
select FW_CFG_DMA
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index dd85fa3619..63304325a5 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -38,6 +38,11 @@ config IDE_VIA
select IDE_PCI
select IDE_QDEV
+config IDE_ICH6
+bool
+select IDE_PCI
+select IDE_QDEV
+
config MICRODRIVE
bool
select IDE_QDEV
diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c
new file mode 100644
index 00..979f5974fd
--- /dev/null
+++ b/hw/ide/bmdma.c
@@ -0,0 +1,83 @@
+/*
+ * QEMU IDE Emulation: PCI PIIX3/4 support.
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2006 Openedhand Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/dma.h"
+
+#include "hw/ide/pci.h"
+#include "trace.h"
+
+uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)
+{
+BMDMAState *bm = opaque;
+uint32_t val;
+
+if (size != 1) {
+return ((uint64_t)1 << (size * 8)) - 1;
+}
+
+switch (addr & 3) {
+case 0:
+val = bm->cmd;
+break;
+case 2:
+val = bm->status;
+break;
+default:
+val = 0xff;
+break;
+}
+
+trace_bmdma_read(addr, val);
+return val;
+}
+
+void piix_bmdma_write(void *opaque, hwaddr addr,
+uint64_t val, unsigned size)
+{
+BMDMAState *bm = opaque;
+
+if (size != 1) {
+return;
+}
+
+trace_bmdma_write(addr, val);
+
+switch (addr & 3) {
+case 0:
+bmdma_cmd_writeb(bm, val);
+break;
+case 2:
+uint8_t cur_val = bm->status;
+bm->status = (val & 0x60) | (cur_val & 1) | (cur_val & ~val & 0x06);
+break;
+}
+}
diff --git a/hw/ide/ich6.c b/hw/ide/ich6.c
new file mode 100644
index 00..21dbc4906b
--- /dev/null
+++ b/hw/ide/ich6.c
@@ -0,0 +1,211 @@
+/*
+ * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.
+ *
+ * Copyright (c) 2022 Liav Albani
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or subst
