Skip to contentSkip to conversation

Qemu: implement readonly memory

PATCH In review Xiao Guangrong 4 participants 17 messages Oct 25, 2012 – Oct 31, 2012

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v2 cover Qemu: implement readonly memory

This patch set make the readonly memory in qemu really readonly by using readonly memory slots feature in kvm to make qemu-kvm safer. Memory regions with readonly property would be plug into kvm as readonly memory slots.

Below module can test this feature:

static int rom_tester_probe(struct pci_dev *dev, const struct pci_device_id *id)

{
	struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
	char buf[6];
	size_t rom_size;
	char * __iomem map;
	int i;

	if (res->flags & (IORESOURCE_ROM_SHADOW | IORESOURCE_ROM_COPY |
	      IORESOURCE_ROM_BIOS_COPY)) {
		dev_printk(KERN_INFO, &dev->dev, "skip ROM COPY.\n");
		return 0;
	}

	if (res->flags &
			(IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY))
	dev_printk(KERN_INFO, &dev->dev, "rom tester\n");

	if (pci_enable_rom(dev)) {
		dev_printk(KERN_INFO, &dev->dev, "do not found Rom\n");
		goto exit;
	}

	map = pci_map_rom(dev, &rom_size);
	if (!map) {
		dev_printk(KERN_INFO, &dev->dev, "map rom fail.\n");
		goto disable_exit;
	}

	dev_printk(KERN_INFO, &dev->dev, "Rom map: %p [size: %lx], phsyc:%llx.\n",
		   map, rom_size, pci_resource_start(dev, PCI_ROM_RESOURCE));

	if (rom_size < 6) {
		printk("map size < 6.\n");
		goto unmap_exit;
	}

	printk("The first 6 bytes:\n");
	for (i = 0; i < 6; i++) {
		buf[i] = map[i];
		printk("%x ", buf[i]);
	}
	printk("\n\n");

	memcpy(map, "KVMKVM", 6);
	if (!memcmp(map, "KVMKVM", 6)) {
		printk("Rom Test: fail.\n");
		goto unmap_exit;
	}

	for (i = 0; i < 6; i++)
		if (buf[i] != ((char *)map)[i]) {
			printk("The %d byte is changed: %x -> %x.\n",
			       i, buf[i], map[i]);
			printk("Rom Test: fail.\n");
			goto unmap_exit;
		}

	printk("Rom Test: Okay.\n");

unmap_exit:

	pci_unmap_rom(dev, map);
disable_exit:
	pci_disable_rom(dev);
exit:
	return 0;
}
static DEFINE_PCI_DEVICE_TABLE(rom_tester_tbl) = {
	{ PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID)},

	{0,}						/* 0 terminated list. */
};

MODULE_DEVICE_TABLE(pci, rom_tester_tbl);

static struct pci_driver rom_tester = {
	.name		= "pci-rom-tester",
	.id_table	= rom_tester_tbl,
	.probe		= rom_tester_probe,
};

static int __init pci_rom_test_init(void)

{
	int rc;

	rc = pci_register_driver(&rom_tester);
	if (rc)
		return rc;

	return 0;
}

static void __exit pci_rom_test_exit(void)

{
	pci_unregister_driver(&rom_tester);
}

module_init(pci_rom_test_init);
module_exit(pci_rom_test_exit);

MODULE_LICENSE("GPL");
MODULE_AUTHOR("Xiao Guangrong <[email protected]>");

we test it with the rom of Intel 82540EM Gigabit Ethernet Controller.
0. start qemu:

    qemu-system-x86_64 -enable-kvm -m 1G -smp 2 -hda fedora16.qcow2 \
        -nic nic,model=e1000,vlan=1,macadrr=52:00:12:34:56 \
        -net user,vlan=1
1. unbind the device:
    echo "0000:00:03.0" > /sys/bus/pci/devices/0000\:00\:03.0/driver/unbind

2. install the test kernel module, here we name it write_rom:
modprobe write_rom
3. print dmesg to verify the result is ok or fail:
dmesg
4. remove the test kernel module.
rmmod write_rom
5. rebind the device to its driver, test if the nic still works:
echo "0000:00:03.0" > /sys/bus/pci/drivers/e1000/bind open firefox and try some web page.

when I use kvm without readonly memory slot, in step 2 it reports:
Rom Test: fail. this means we can write to the memory region of a rom, which is quite not safe for the guest and host.

when I use kvm with readonly memory slot, in step 2 it reports:
Rom Test: Okay.

this means write operation is not successful, and return back to qemu from kvm. thus we can make the rom real rom.

#1 lore raw

PATCH v2 1/5 KVM: define KVM_CAP_READONLY_MEM unconditionally

let it not depend on __KVM_HAVE_READONLY_MEM to make it friendly to userspace

---
 arch/x86/kvm/x86.c  |    2 ++
 include/linux/kvm.h |    2 --
 virt/kvm/kvm_main.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/kvm/x86.c +2 -0
index ec07cd3..0fd220b 100644
@@ -2189,7 +2189,9 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_GET_TSC_KHZ:
 	case KVM_CAP_PCI_2_3:
 	case KVM_CAP_KVMCLOCK_CTRL:
+#ifdef __KVM_HAVE_READONLY_MEM
 	case KVM_CAP_READONLY_MEM:
+#endif
 	case KVM_CAP_IRQFD_RESAMPLE:
 		r = 1;
 		break;
include/linux/kvm.h +0 -2
index 494a84c..a9001c1 100644
@@ -629,9 +629,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_PPC_GET_SMMU_INFO 78
 #define KVM_CAP_S390_COW 79
 #define KVM_CAP_PPC_ALLOC_HTAB 80
-#ifdef __KVM_HAVE_READONLY_MEM
 #define KVM_CAP_READONLY_MEM 81
-#endif
 #define KVM_CAP_IRQFD_RESAMPLE 82
 #define KVM_CAP_PPC_BOOKE_WATCHDOG 83
 
virt/kvm/kvm_main.c +1 -1
index e26a55f..55f12a1 100644
@@ -684,7 +684,7 @@ static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
 {
 	u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;
 
-#ifdef KVM_CAP_READONLY_MEM
+#ifdef __KVM_HAVE_READONLY_MEM
 	valid_flags |= KVM_MEM_READONLY;
 #endif
 
Signature · 1 line
1.7.7.6
#2 lore raw

Re: [PATCH v2 1/5] KVM: define KVM_CAP_READONLY_MEM unconditionally

Quoted 43 lines · Xiao Guangrong

let it not depend on __KVM_HAVE_READONLY_MEM to make it friendly to userspace

---
 arch/x86/kvm/x86.c  |    2 ++
 include/linux/kvm.h |    2 --
 virt/kvm/kvm_main.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/kvm/x86.c +2 -0
index ec07cd3..0fd220b 100644
@@ -2189,7 +2189,9 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_GET_TSC_KHZ:
 	case KVM_CAP_PCI_2_3:
 	case KVM_CAP_KVMCLOCK_CTRL:
+#ifdef __KVM_HAVE_READONLY_MEM
 	case KVM_CAP_READONLY_MEM:
+#endif
 	case KVM_CAP_IRQFD_RESAMPLE:
 		r = 1;
 		break;
include/linux/kvm.h +0 -2
index 494a84c..a9001c1 100644
@@ -629,9 +629,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_PPC_GET_SMMU_INFO 78
 #define KVM_CAP_S390_COW 79
 #define KVM_CAP_PPC_ALLOC_HTAB 80
-#ifdef __KVM_HAVE_READONLY_MEM
 #define KVM_CAP_READONLY_MEM 81
-#endif
 #define KVM_CAP_IRQFD_RESAMPLE 82
 #define KVM_CAP_PPC_BOOKE_WATCHDOG 83
 
virt/kvm/kvm_main.c +0 -0
index e26a55f..55f12a1 100644

virt/kvm/kvm_main.c static int check_memory_region_flags(struct kvm_userspace_memory_region *mem) · line 690

Quoted 8 lines
virt/kvm/kvm_main.c +1 -1
@@ -684,7 +684,7 @@ static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
 {
 	u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES;
 
-#ifdef KVM_CAP_READONLY_MEM
+#ifdef __KVM_HAVE_READONLY_MEM
 	valid_flags |= KVM_MEM_READONLY;
 #endif
 

Much appreciated.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#3 lore raw

PATCH v2 2/5 Qemu: update header files

In reply to Xiao Guangrong

Sync the header files by using scripts/update-linux-headers.sh

---
 linux-headers/asm-powerpc/kvm.h      |   59 ++++++++++++++++++++++++++++++++++
 linux-headers/asm-powerpc/kvm_para.h |    7 ++--
 linux-headers/asm-x86/kvm.h          |   17 ++++++++++
 linux-headers/linux/kvm.h            |   41 ++++++++++++++++++-----
 4 files changed, 112 insertions(+), 12 deletions(-)
linux-headers/asm-powerpc/kvm.h +59 -0
index 1bea4d8..b89ae4d 100644
@@ -221,6 +221,12 @@ struct kvm_sregs {
 
 			__u32 dbsr;	/* KVM_SREGS_E_UPDATE_DBSR */
 			__u32 dbcr[3];
+			/*
+			 * iac/dac registers are 64bit wide, while this API
+			 * interface provides only lower 32 bits on 64 bit
+			 * processors. ONE_REG interface is added for 64bit
+			 * iac/dac registers.
+			 */
 			__u32 iac[4];
 			__u32 dac[2];
 			__u32 dvc[2];
@@ -326,5 +332,58 @@ struct kvm_book3e_206_tlb_params {
 };
 
 #define KVM_REG_PPC_HIOR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x1)
+#define KVM_REG_PPC_IAC1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x2)
+#define KVM_REG_PPC_IAC2	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x3)
+#define KVM_REG_PPC_IAC3	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x4)
+#define KVM_REG_PPC_IAC4	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x5)
+#define KVM_REG_PPC_DAC1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x6)
+#define KVM_REG_PPC_DAC2	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x7)
+#define KVM_REG_PPC_DABR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x8)
+#define KVM_REG_PPC_DSCR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x9)
+#define KVM_REG_PPC_PURR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa)
+#define KVM_REG_PPC_SPURR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb)
+#define KVM_REG_PPC_DAR		(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xc)
+#define KVM_REG_PPC_DSISR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xd)
+#define KVM_REG_PPC_AMR		(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xe)
+#define KVM_REG_PPC_UAMOR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xf)
+
+#define KVM_REG_PPC_MMCR0	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x10)
+#define KVM_REG_PPC_MMCR1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x11)
+#define KVM_REG_PPC_MMCRA	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x12)
+
+#define KVM_REG_PPC_PMC1	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x18)
+#define KVM_REG_PPC_PMC2	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x19)
+#define KVM_REG_PPC_PMC3	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1a)
+#define KVM_REG_PPC_PMC4	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1b)
+#define KVM_REG_PPC_PMC5	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1c)
+#define KVM_REG_PPC_PMC6	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1d)
+#define KVM_REG_PPC_PMC7	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1e)
+#define KVM_REG_PPC_PMC8	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x1f)
+
+/* 32 floating-point registers */
+#define KVM_REG_PPC_FPR0	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x20)
+#define KVM_REG_PPC_FPR(n)	(KVM_REG_PPC_FPR0 + (n))
+#define KVM_REG_PPC_FPR31	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x3f)
+
+/* 32 VMX/Altivec vector registers */
+#define KVM_REG_PPC_VR0		(KVM_REG_PPC | KVM_REG_SIZE_U128 | 0x40)
+#define KVM_REG_PPC_VR(n)	(KVM_REG_PPC_VR0 + (n))
+#define KVM_REG_PPC_VR31	(KVM_REG_PPC | KVM_REG_SIZE_U128 | 0x5f)
+
+/* 32 double-width FP registers for VSX */
+/* High-order halves overlap with FP regs */
+#define KVM_REG_PPC_VSR0	(KVM_REG_PPC | KVM_REG_SIZE_U128 | 0x60)
+#define KVM_REG_PPC_VSR(n)	(KVM_REG_PPC_VSR0 + (n))
+#define KVM_REG_PPC_VSR31	(KVM_REG_PPC | KVM_REG_SIZE_U128 | 0x7f)
+
+/* FP and vector status/control registers */
+#define KVM_REG_PPC_FPSCR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x80)
+#define KVM_REG_PPC_VSCR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x81)
+
+/* Virtual processor areas */
+/* For SLB & DTL, address in high (first) half, length in low half */
+#define KVM_REG_PPC_VPA_ADDR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x82)
+#define KVM_REG_PPC_VPA_SLB	(KVM_REG_PPC | KVM_REG_SIZE_U128 | 0x83)
+#define KVM_REG_PPC_VPA_DTL	(KVM_REG_PPC | KVM_REG_SIZE_U128 | 0x84)
 
 #endif /* __LINUX_KVM_POWERPC_H */
linux-headers/asm-powerpc/kvm_para.h +4 -3
index c047a84..7e64f57 100644
@@ -75,9 +75,10 @@ struct kvm_vcpu_arch_shared {
 };
 
 #define KVM_SC_MAGIC_R0		0x4b564d21 /* "KVM!" */
-#define HC_VENDOR_KVM		(42 << 16)
-#define HC_EV_SUCCESS		0
-#define HC_EV_UNIMPLEMENTED	12
+
+#define KVM_HCALL_TOKEN(num)     _EV_HCALL_TOKEN(EV_KVM_VENDOR_ID, num)
+
+#include <asm/epapr_hcalls.h>
 
 #define KVM_FEATURE_MAGIC_PAGE	1
 
linux-headers/asm-x86/kvm.h +17 -0
index 246617e..a65ec29 100644
@@ -9,6 +9,22 @@
 #include <linux/types.h>
 #include <linux/ioctl.h>
 
+#define DE_VECTOR 0
+#define DB_VECTOR 1
+#define BP_VECTOR 3
+#define OF_VECTOR 4
+#define BR_VECTOR 5
+#define UD_VECTOR 6
+#define NM_VECTOR 7
+#define DF_VECTOR 8
+#define TS_VECTOR 10
+#define NP_VECTOR 11
+#define SS_VECTOR 12
+#define GP_VECTOR 13
+#define PF_VECTOR 14
+#define MF_VECTOR 16
+#define MC_VECTOR 18
+
 /* Select x86 specific features in <linux/kvm.h> */
 #define __KVM_HAVE_PIT
 #define __KVM_HAVE_IOAPIC
@@ -25,6 +41,7 @@
 #define __KVM_HAVE_DEBUGREGS
 #define __KVM_HAVE_XSAVE
 #define __KVM_HAVE_XCRS
+#define __KVM_HAVE_READONLY_MEM
 
 /* Architectural interrupt line count. */
 #define KVM_NR_INTERRUPTS 256
linux-headers/linux/kvm.h +32 -9

90 lines not on this page · raw message

Signature · 1 line
1.7.7.6
#4 lore raw

Re: [Qemu-devel] [PATCH v2 2/5] Qemu: update header files

Quoted 1 line · Xiao Guangrong

Sync the header files by using scripts/update-linux-headers.sh

I think it would be good if this kind of patch included in the commit message the kernel tree/branch and commit hash that the sync was with.

thanks
-- PMM

#5 lore raw

PATCH v2 3/5 Qemu: do not mark bios readonly

In reply to Xiao Guangrong

In isapc, no i440x device exists in guest that means seabios can not make 0xc0000 to 0x1000000 writable

It works fine in current code since the guest can happily write readonly memory. In order to support readonly slot in Qemu, we do not make the bios readonly anymore

---
 hw/pc_sysfw.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
hw/pc_sysfw.c +0 -2
index b45f0ac..2d56fc7 100644
@@ -156,7 +156,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
     bios = g_malloc(sizeof(*bios));
     memory_region_init_ram(bios, "pc.bios", bios_size);
     vmstate_register_ram_global(bios);
-    memory_region_set_readonly(bios, true);
     ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
     if (ret != 0) {
     bios_error:
@@ -179,7 +178,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-    memory_region_set_readonly(isa_bios, true);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
Signature · 1 line
1.7.7.6
#6 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 26 lines · Xiao Guangrong

In isapc, no i440x device exists in guest that means seabios can not make 0xc0000 to 0x1000000 writable

It works fine in current code since the guest can happily write readonly memory. In order to support readonly slot in Qemu, we do not make the bios readonly anymore

---
 hw/pc_sysfw.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
hw/pc_sysfw.c +0 -1
index b45f0ac..2d56fc7 100644
@@ -156,7 +156,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
     bios = g_malloc(sizeof(*bios));
     memory_region_init_ram(bios, "pc.bios", bios_size);
     vmstate_register_ram_global(bios);
-    memory_region_set_readonly(bios, true);
     ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
     if (ret != 0) {
     bios_error:

hw/pc_sysfw.c static void old_pc_system_rom_init(MemoryRegion *rom_memory) · line 183

Quoted 7 lines
hw/pc_sysfw.c +0 -1
@@ -179,7 +178,6 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-    memory_region_set_readonly(isa_bios, true);
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,

This has two problems: We know it breaks at least Win 95 that overwrites its F-segment during boot. And it applies changes to the shadowed area (below 1 MB) also to the ROM area - I don't think that is the original behaviour on real hardware.

What we need is paravirtual shadow write control for the ISA PC. It's on my todo list, maybe I will be able to look into this during the next week.

BTW, your patch series should allow to drop the KVM special case from pc_system_firmware_init. That version, btw, treats high and low BIOS areas separately - but only reloads the upper area. Hmm...

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#7 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Jan,

Quoted 4 lines · Jan Kiszka

This has two problems: We know it breaks at least Win 95 that overwrites its F-segment during boot. And it applies changes to the shadowed area (below 1 MB) also to the ROM area - I don't think that is the original behaviour on real hardware.

So what is the problem? It can break Win95's running?

I tried to install win95 guest but it failed to boot regardless my patchset was applied or not. I found the information that win 95 is not supported at
http://www.linux-kvm.org/page/Guest_Support_Status

Note: before my patchset, Win 95 still can happily something into ROM area because readonly memory is actually writable on KVM. And win95 can not run on isapc with --no-kvm since it is no way to enable shadow ROM.

Quoted 4 lines

What we need is paravirtual shadow write control for the ISA PC. It's on my todo list, maybe I will be able to look into this during the next week.

You idea is that modify the code of seabios and use a special way (PV) to notify Qemu to make the bios writable?

Actually, I am confused why the guest (including bios) persistently uses shadow ROM even if it is not supported (on ISA PC), i think the right way is move itself to RAM under this case, no?

Quoted 4 lines

BTW, your patch series should allow to drop the KVM special case from pc_system_firmware_init. That version, btw, treats high and low BIOS areas separately - but only reloads the upper area. Hmm...

You mean that also allow Qemu to use pflash to load bios if kvm is enabled? We can not do that for pflash is a RD device which can not be directly written, kvm can not emulate the instruction which implicitly write the memory. (e.g:
using this area as stack).

Thanks!

#8 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 11 lines · Xiao Guangrong

Jan,

Quoted 4 lines · Jan Kiszka

This has two problems: We know it breaks at least Win 95 that overwrites its F-segment during boot. And it applies changes to the shadowed area (below 1 MB) also to the ROM area - I don't think that is the original behaviour on real hardware.

So what is the problem? It can break Win95's running?

Quoted 7 lines

I tried to install win95 guest but it failed to boot regardless my patchset was applied or not. I found the information that win 95 is not supported at
http://www.linux-kvm.org/page/Guest_Support_Status

Note: before my patchset, Win 95 still can happily something into ROM area because readonly memory is actually writable on KVM. And win95 can not run on isapc with --no-kvm since it is no way to enable shadow ROM.

Your patches causes regressions on TCG mode as that is perfectly fine with booting Win95 so far.

Quoted 8 lines
Quoted 5 lines

What we need is paravirtual shadow write control for the ISA PC. It's on my todo list, maybe I will be able to look into this during the next week.

You idea is that modify the code of seabios and use a special way (PV) to notify Qemu to make the bios writable?

Yes.

Quoted 4 lines

Actually, I am confused why the guest (including bios) persistently uses shadow ROM even if it is not supported (on ISA PC), i think the right way is move itself to RAM under this case, no?

I've been told that Seabios has been built around that assumption and the PV shadow control would be simpler to realize.

Quoted 7 lines
Quoted 5 lines

BTW, your patch series should allow to drop the KVM special case from pc_system_firmware_init. That version, btw, treats high and low BIOS areas separately - but only reloads the upper area. Hmm...

You mean that also allow Qemu to use pflash to load bios if kvm is enabled?

Yes.

Quoted 3 lines

We can not do that for pflash is a RD device which can not be directly written, kvm can not emulate the instruction which implicitly write the memory. (e.g:
using this area as stack).

Isn't enabling ROMD support for KVM that whole point of your patches? I do not see yet what prevents this still, but it should be fixed first.

Jan

#9 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 16 lines · Jan Kiszka
Quoted 15 lines · Xiao Guangrong

Jan,

Quoted 4 lines · Jan Kiszka

This has two problems: We know it breaks at least Win 95 that overwrites its F-segment during boot. And it applies changes to the shadowed area (below 1 MB) also to the ROM area - I don't think that is the original behaviour on real hardware.

So what is the problem? It can break Win95's running?

I tried to install win95 guest but it failed to boot regardless my patchset was applied or not. I found the information that win 95 is not supported at
http://www.linux-kvm.org/page/Guest_Support_Status

Quoted 6 lines
Quoted 3 lines

Note: before my patchset, Win 95 still can happily something into ROM area because readonly memory is actually writable on KVM. And win95 can not run on isapc with --no-kvm since it is no way to enable shadow ROM.

Your patches causes regressions on TCG mode as that is perfectly fine with booting Win95 so far.

Aha, i tried accel=tcg, before my patchset, it works for -machine pc but failed for -machine isapc (known issue for seabios). After my patchset, it works fine for both -machine pc and isapc. :)

Quoted 13 lines
Quoted 9 lines
Quoted 6 lines

What we need is paravirtual shadow write control for the ISA PC. It's on my todo list, maybe I will be able to look into this during the next week.

You idea is that modify the code of seabios and use a special way (PV) to notify Qemu to make the bios writable?

Yes.

Quoted 1 line
Quoted 6 lines
Quoted 3 lines

Actually, I am confused why the guest (including bios) persistently uses shadow ROM even if it is not supported (on ISA PC), i think the right way is move itself to RAM under this case, no?

I've been told that Seabios has been built around that assumption and the PV shadow control would be simpler to realize.

Sounds the PV is complexer that directly making the bios area writable (if it works).

Quoted 9 lines
Quoted 8 lines
Quoted 6 lines

BTW, your patch series should allow to drop the KVM special case from pc_system_firmware_init. That version, btw, treats high and low BIOS areas separately - but only reloads the upper area. Hmm...

You mean that also allow Qemu to use pflash to load bios if kvm is enabled?

Quoted 7 lines

Yes.

Quoted 3 lines

We can not do that for pflash is a RD device which can not be directly written, kvm can not emulate the instruction which implicitly write the memory. (e.g:
using this area as stack).

Isn't enabling ROMD support for KVM that whole point of your patches? I

It can generate MMIO exit if ROMD be written, that means the instruction needs kvm's help to be finished if it explicitly/implicitly write the memory.

Quoted 1 line

do not see yet what prevents this still, but it should be fixed first.

For the explicitly write memory access, it is easy to be fixed - we just need to fetch the instruction from EIP and emulate it. But for the implicitly memory access, fixing its emulation is really hard work. Really worth doing it?

#10 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 21 lines · Xiao Guangrong
Quoted 20 lines · Jan Kiszka
Quoted 18 lines · Xiao Guangrong

Jan,

Quoted 4 lines · Jan Kiszka

This has two problems: We know it breaks at least Win 95 that overwrites its F-segment during boot. And it applies changes to the shadowed area (below 1 MB) also to the ROM area - I don't think that is the original behaviour on real hardware.

So what is the problem? It can break Win95's running?

I tried to install win95 guest but it failed to boot regardless my patchset was applied or not. I found the information that win 95 is not supported at
http://www.linux-kvm.org/page/Guest_Support_Status

Note: before my patchset, Win 95 still can happily something into ROM area because readonly memory is actually writable on KVM. And win95 can not run on isapc with --no-kvm since it is no way to enable shadow ROM.

Quoted 6 lines
Quoted 2 lines

Your patches causes regressions on TCG mode as that is perfectly fine with booting Win95 so far.

Aha, i tried accel=tcg, before my patchset, it works for -machine pc but failed for -machine isapc (known issue for seabios). After my patchset, it works fine for both -machine pc and isapc. :)

Indeed, looks like I'm on the wrong track regarding what breaks Win95 in KVM mode.

However: This patch inappropriately allows the guest to change the BIOS content during runtime. And that not only in the lower ISA range, not only for our stepchild isapc but for the high ROM range as well, even with PCI chipset enabled. So this is nothing more than a hack.

Quoted 18 lines
Quoted 18 lines
Quoted 10 lines
Quoted 7 lines

What we need is paravirtual shadow write control for the ISA PC. It's on my todo list, maybe I will be able to look into this during the next week.

You idea is that modify the code of seabios and use a special way (PV) to notify Qemu to make the bios writable?

Yes.

Quoted 4 lines

Actually, I am confused why the guest (including bios) persistently uses shadow ROM even if it is not supported (on ISA PC), i think the right way is move itself to RAM under this case, no?

Quoted 5 lines
Quoted 2 lines

I've been told that Seabios has been built around that assumption and the PV shadow control would be simpler to realize.

Sounds the PV is complexer that directly making the bios area writable (if it works).

But it is the only correct solution. In fact, shadowing means mapping RAM above the ROM, not enabling writability, and then copying necessary bits from the high ROM part to that RAM. Seabios does this when PAM is available, we just need to pull in those bits for PV shadow control.

Quoted 12 lines
Quoted 12 lines
Quoted 9 lines
Quoted 7 lines

BTW, your patch series should allow to drop the KVM special case from pc_system_firmware_init. That version, btw, treats high and low BIOS areas separately - but only reloads the upper area. Hmm...

You mean that also allow Qemu to use pflash to load bios if kvm is enabled?

Yes.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

We can not do that for pflash is a RD device which can not be directly written, kvm can not emulate the instruction which implicitly write the memory. (e.g:
using this area as stack).

Isn't enabling ROMD support for KVM that whole point of your patches? I

It can generate MMIO exit if ROMD be written, that means the instruction needs kvm's help to be finished if it explicitly/implicitly write the memory.

I was assuming that this is what you already do. If you trap write accesses, why not allowing user space to handle them?

Quoted 6 lines
Quoted 2 lines

do not see yet what prevents this still, but it should be fixed first.

For the explicitly write memory access, it is easy to be fixed - we just need to fetch the instruction from EIP and emulate it. But for the implicitly memory access, fixing its emulation is really hard work. Really worth doing it?

Aren't the read-only regions also marked read-only on the host side to avoid that the guest writes to it? Or how is this implemented?

Support for flash emulation in KVM mode is increasingly important, for embedded platform virtualization but also for classic x86 server-like targets. The pflash-backed system firmware device was added for a reason...

Jan

#11 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 29 lines · Jan Kiszka
Quoted 21 lines · Xiao Guangrong
Quoted 20 lines · Jan Kiszka
Quoted 18 lines · Xiao Guangrong

Jan,

Quoted 4 lines · Jan Kiszka

This has two problems: We know it breaks at least Win 95 that overwrites its F-segment during boot. And it applies changes to the shadowed area (below 1 MB) also to the ROM area - I don't think that is the original behaviour on real hardware.

So what is the problem? It can break Win95's running?

I tried to install win95 guest but it failed to boot regardless my patchset was applied or not. I found the information that win 95 is not supported at
http://www.linux-kvm.org/page/Guest_Support_Status

Note: before my patchset, Win 95 still can happily something into ROM area because readonly memory is actually writable on KVM. And win95 can not run on isapc with --no-kvm since it is no way to enable shadow ROM.

Quoted 6 lines
Quoted 2 lines

Your patches causes regressions on TCG mode as that is perfectly fine with booting Win95 so far.

Aha, i tried accel=tcg, before my patchset, it works for -machine pc but failed for -machine isapc (known issue for seabios). After my patchset, it works fine for both -machine pc and isapc. :)

Quoted 7 lines

Indeed, looks like I'm on the wrong track regarding what breaks Win95 in KVM mode.

However: This patch inappropriately allows the guest to change the BIOS content during runtime. And that not only in the lower ISA range, not only for our stepchild isapc but for the high ROM range as well, even with PCI chipset enabled. So this is nothing more than a hack.

Okay.

Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
Quoted 11 lines
Quoted 8 lines

What we need is paravirtual shadow write control for the ISA PC. It's on my todo list, maybe I will be able to look into this during the next week.

You idea is that modify the code of seabios and use a special way (PV) to notify Qemu to make the bios writable?

Yes.

Quoted 4 lines

Actually, I am confused why the guest (including bios) persistently uses shadow ROM even if it is not supported (on ISA PC), i think the right way is move itself to RAM under this case, no?

I've been told that Seabios has been built around that assumption and the PV shadow control would be simpler to realize.

Quoted 7 lines
Quoted 2 lines

Sounds the PV is complexer that directly making the bios area writable (if it works).

But it is the only correct solution. In fact, shadowing means mapping RAM above the ROM, not enabling writability, and then copying necessary bits from the high ROM part to that RAM. Seabios does this when PAM is available, we just need to pull in those bits for PV shadow control.

Okay. I will continue my work after your PV works. :)

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 10 lines
Quoted 8 lines

BTW, your patch series should allow to drop the KVM special case from pc_system_firmware_init. That version, btw, treats high and low BIOS areas separately - but only reloads the upper area. Hmm...

You mean that also allow Qemu to use pflash to load bios if kvm is enabled?

Yes.

Quoted 3 lines

We can not do that for pflash is a RD device which can not be directly written, kvm can not emulate the instruction which implicitly write the memory. (e.g:
using this area as stack).

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Isn't enabling ROMD support for KVM that whole point of your patches? I

It can generate MMIO exit if ROMD be written, that means the instruction needs kvm's help to be finished if it explicitly/implicitly write the memory.

I was assuming that this is what you already do. If you trap write accesses, why not allowing user space to handle them?

Already have done that, guest write ROMD -> vmexit -> return to userspace with MMIO Exit.

Quoted 8 lines
Quoted 7 lines
Quoted 3 lines

do not see yet what prevents this still, but it should be fixed first.

For the explicitly write memory access, it is easy to be fixed - we just need to fetch the instruction from EIP and emulate it. But for the implicitly memory access, fixing its emulation is really hard work. Really worth doing it?

Quoted 6 lines

Aren't the read-only regions also marked read-only on the host side to avoid that the guest writes to it? Or how is this implemented?

Support for flash emulation in KVM mode is increasingly important, for embedded platform virtualization but also for classic x86 server-like targets. The pflash-backed system firmware device was added for a reason...

Please allow me to clarify it more clearly.

The flash is ROMD device means guest can not write it, any kinds of guest write access on this device can cause vmexit to kvm and return to userspace.

We should pay more attention on it if we execute the code in ROMD since we can not use ROMD as stack/page table/IDT table and all other implicitly write access. Of course, if you do not use ROM as those purposes, it is okay. :)

#12 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 6 lines · Xiao Guangrong
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

We can not do that for pflash is a RD device which can not be directly written, kvm can not emulate the instruction which implicitly write the memory. (e.g:
using this area as stack).

Isn't enabling ROMD support for KVM that whole point of your patches? I

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

It can generate MMIO exit if ROMD be written, that means the instruction needs kvm's help to be finished if it explicitly/implicitly write the memory.

I was assuming that this is what you already do. If you trap write accesses, why not allowing user space to handle them?

Already have done that, guest write ROMD -> vmexit -> return to userspace with MMIO Exit.

Great.

Quoted 16 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines

do not see yet what prevents this still, but it should be fixed first.

For the explicitly write memory access, it is easy to be fixed - we just need to fetch the instruction from EIP and emulate it. But for the implicitly memory access, fixing its emulation is really hard work. Really worth doing it?

Quoted 6 lines

Aren't the read-only regions also marked read-only on the host side to avoid that the guest writes to it? Or how is this implemented?

Support for flash emulation in KVM mode is increasingly important, for embedded platform virtualization but also for classic x86 server-like targets. The pflash-backed system firmware device was added for a reason...

Quoted 8 lines

Please allow me to clarify it more clearly.

The flash is ROMD device means guest can not write it, any kinds of guest write access on this device can cause vmexit to kvm and return to userspace.

We should pay more attention on it if we execute the code in ROMD since we can not use ROMD as stack/page table/IDT table and all other implicitly write access. Of course, if you do not use ROM as those purposes, it is okay. :)

So the problem is that there is KVM code that still blindly writes to guest memory and does not take the memory regions' protection flag into account? And we cannot mark those regions read only in the host's page table?

Jan

#13 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 5 lines · Jan Kiszka
Quoted 5 lines

Please allow me to clarify it more clearly.

The flash is ROMD device means guest can not write it, any kinds of guest write access on this device can cause vmexit to kvm and return to userspace.

Quoted 8 lines
Quoted 3 lines

We should pay more attention on it if we execute the code in ROMD since we can not use ROMD as stack/page table/IDT table and all other implicitly write access. Of course, if you do not use ROM as those purposes, it is okay. :)

So the problem is that there is KVM code that still blindly writes to guest memory and does not take the memory regions' protection flag into account? And we cannot mark those regions read only in the host's page table?

KVM has the ability to catch this kind of write access on ROMD, it is just hard to emulate the implicitly memory access.

#14 lore raw

Re: [PATCH v2 3/5] Qemu: do not mark bios readonly

Quoted 11 lines · Xiao Guangrong
Quoted 9 lines · Jan Kiszka
Quoted 8 lines

Please allow me to clarify it more clearly.

The flash is ROMD device means guest can not write it, any kinds of guest write access on this device can cause vmexit to kvm and return to userspace.

We should pay more attention on it if we execute the code in ROMD since we can not use ROMD as stack/page table/IDT table and all other implicitly write access. Of course, if you do not use ROM as those purposes, it is okay. :)

Quoted 7 lines
Quoted 4 lines

So the problem is that there is KVM code that still blindly writes to guest memory and does not take the memory regions' protection flag into account? And we cannot mark those regions read only in the host's page table?

KVM has the ability to catch this kind of write access on ROMD, it is just hard to emulate the implicitly memory access.

Drop them? It is highly unlikely that they trigger the magic write-enable patterns at the right spot in a ROMD device.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#15 lore raw

PATCH v2 4/5 Qemu: implement readonly memory

In reply to Xiao Guangrong

As readonly memory is support in kvm, this patch supports this feature in qemu, mainly pluging the memory region with KVM_MEM_READONLY flag to kvm

---
 kvm-all.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
kvm-all.c +17 -7
index 92a7137..6b9395a 100644
@@ -101,6 +101,7 @@ struct KVMState
     QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
     bool direct_msi;
 #endif
+    bool readonly_mem;
 };
 
 KVMState *kvm_state;
@@ -265,9 +266,16 @@ err:
  * dirty pages logging control
  */
 
-static int kvm_mem_flags(KVMState *s, bool log_dirty)
+static int kvm_mem_flags(KVMState *s, MemoryRegion *mr)
 {
-    return log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0;
+    int flags;
+
+    flags = memory_region_is_logging(mr) ? KVM_MEM_LOG_DIRTY_PAGES : 0;
+
+    if (s->readonly_mem && mr->readonly) {
+        flags |= KVM_MEM_READONLY;
+    }
+    return flags;
 }
 
 static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_dirty)
@@ -278,7 +286,7 @@ static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_dirty)
 
     old_flags = mem->flags;
 
-    flags = (mem->flags & ~mask) | kvm_mem_flags(s, log_dirty);
+    flags = (mem->flags & ~mask) | (log_dirty ? mask : 0);
     mem->flags = flags;
 
     /* If nothing changed effectively, no need to issue ioctl */
@@ -626,7 +634,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             mem->memory_size = old.memory_size;
             mem->start_addr = old.start_addr;
             mem->ram = old.ram;
-            mem->flags = kvm_mem_flags(s, log_dirty);
+            mem->flags = kvm_mem_flags(s, mr);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -647,7 +655,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             mem->memory_size = start_addr - old.start_addr;
             mem->start_addr = old.start_addr;
             mem->ram = old.ram;
-            mem->flags =  kvm_mem_flags(s, log_dirty);
+            mem->flags =  kvm_mem_flags(s, mr);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -671,7 +679,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             size_delta = mem->start_addr - old.start_addr;
             mem->memory_size = old.memory_size - size_delta;
             mem->ram = old.ram + size_delta;
-            mem->flags = kvm_mem_flags(s, log_dirty);
+            mem->flags = kvm_mem_flags(s, mr);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -693,7 +701,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
     mem->memory_size = size;
     mem->start_addr = start_addr;
     mem->ram = ram;
-    mem->flags = kvm_mem_flags(s, log_dirty);
+    mem->flags = kvm_mem_flags(s, mr);
 
     err = kvm_set_user_memory_region(s, mem);
     if (err) {
@@ -1390,6 +1398,8 @@ int kvm_init(void)
         s->irq_set_ioctl = KVM_IRQ_LINE_STATUS;
     }
 
+    s->readonly_mem = kvm_check_extension(s, KVM_CAP_READONLY_MEM);
+
     ret = kvm_arch_init(s);
     if (ret < 0) {
         goto err;
Signature · 1 line
1.7.7.6
#16 lore raw

PATCH v2 5/5 Qemu: mark pci rom readonly

In reply to Xiao Guangrong

Since it is ROM which is really readonly in the system

---
 hw/pci.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
hw/pci.c +1 -0
index 2ca6ff6..2a287bb 100644
@@ -1803,6 +1803,7 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
     }
     pdev->has_rom = true;
     memory_region_init_ram(&pdev->rom, name, size);
+    memory_region_set_readonly(&pdev->rom, true);
     vmstate_register_ram(&pdev->rom, &pdev->qdev);
     ptr = memory_region_get_ram_ptr(&pdev->rom);
     load_image(path, ptr);
Signature · 1 line
1.7.7.6