Re: [Qemu-devel] [PATCH 3/3] support readonly memory feature in qemu

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-09-07 10:26, Liu Sheng wrote:
> 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.
> 
> this can be tested by a kernel module from the author of kvm readonly
> memory slot:
> 
> 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 <xiaoguangrong@linux.vnet.ibm.com>");
> 
> 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.
> and the error message prints out in host console:
>     Guest is writing readonly memory, phys_addr: febc0000, len:4, data[0]:K,
>     skip it
>     Guest is writing readonly memory, phys_addr: febc0000, len:2, data[0]:V,
>     skip it
> this is what we write "KVMKVM".
> this means write operation is not successful, and return back to qemu from kvm.
> thus we can make the rom real rom.
> 
> Signed-off-by: Liu Sheng <liusheng@linux.vnet.ibm.com>
> ---
>  kvm-all.c |   47 ++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index badf1d8..1b66183 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -97,6 +97,9 @@ struct KVMState
>      QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
>      bool direct_msi;
>  #endif
> +#ifdef KVM_CAP_READONLY_MEM
> +    int readonly_mem;
> +#endif

See comment on patch 2: Make sure that KVM_CAP_READONLY_MEM is always
defined and remove all these #ifdefs.

>  };
>  
>  KVMState *kvm_state;
> @@ -261,9 +264,18 @@ 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;
> +    bool log_dirty = memory_region_is_logging(mr);
> +    bool readonly = mr->readonly;

I suppose you want to add some memory_region_is_readonly(). And both
bools can be inlined into the checks below.

> +    flags = log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0;
> +#ifdef KVM_CAP_READONLY_MEM
> +    if (s->readonly_mem) {
> +        flags |= readonly ? KVM_MEM_READONLY : 0;
> +    }
> +#endif
> +    return flags;
>  }
>  
>  static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_dirty)
> @@ -274,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 */
> @@ -622,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) {
> @@ -643,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) {
> @@ -667,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) {
> @@ -689,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) {
> @@ -1393,6 +1405,10 @@ int kvm_init(void)
>      s->direct_msi = (kvm_check_extension(s, KVM_CAP_SIGNAL_MSI) > 0);
>  #endif
>  
> +#ifdef KVM_CAP_READONLY_MEM
> +    s->readonly_mem = kvm_check_extension(s, KVM_CAP_READONLY_MEM);
> +#endif
> +
>      s->intx_set_mask = kvm_check_extension(s, KVM_CAP_PCI_2_3);
>  
>      ret = kvm_arch_init(s);
> @@ -1607,10 +1623,19 @@ int kvm_cpu_exec(CPUArchState *env)
>              break;
>          case KVM_EXIT_MMIO:
>              DPRINTF("handle_mmio\n");
> -            cpu_physical_memory_rw(run->mmio.phys_addr,
> -                                   run->mmio.data,
> -                                   run->mmio.len,
> -                                   run->mmio.is_write);
> +            if (run->mmio.write_readonly_mem) {
> +                fprintf(stderr, "Guest is writing readonly memory, "
> +                    "phys_addr: %llx, len:%x, data[0]:%c, skip it.\n",
> +                    run->mmio.phys_addr,
> +                    run->mmio.len,
> +                    run->mmio.data[0]);

This should be DPRINTF.

> +            } else {
> +                cpu_physical_memory_rw(run->mmio.phys_addr,
> +                                       run->mmio.data,
> +                                       run->mmio.len,
> +                                       run->mmio.is_write);
> +            }
> +
>              ret = 0;
>              break;
>          case KVM_EXIT_IRQ_WINDOW_OPEN:
> 

Great to see this feature for KVM finally! I'm just afraid that this
will finally break good old isapc - due to broken Seabios. KVM used to
"unbreak" it as it didn't respect write protections. ;)

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux