Re: [PATCH] kvm: device-assignment: Add PCI option ROM support

["Yang, Sheng" <sheng.yang@intel.com>]

On Tuesday 23 June 2009 00:09:28 Alex Williamson wrote:
> On Mon, 2009-06-22 at 13:32 +0800, Yang, Sheng wrote:
> > On Friday 19 June 2009 21:44:40 Alex Williamson wrote:
> > > On Fri, 2009-06-19 at 15:27 +0800, Yang, Sheng wrote:
> > > > On Friday 19 June 2009 00:28:41 Alex Williamson wrote:
> > > > > The one oddity I noticed is that even when the enable bit is clear,
> > > > > the guest can read the ROM.  I don't know that this is actually
> > > > > illegal, vs returning zeros or ones though.  It seems like maybe
> > > > > the generic PCI code isn't tracking the enable bit.  I think that's
> > > > > an independent problem from this patch though.  Thanks,
> > > >
> > > > That should be fine. I've taken a look at code, seems Linux kernel
> > > > set enable_bit when someone begin to read rom, and copy rom to
> > > > buffer, then unmap the rom. So the rom can be read when enable bit
> > > > clear.
> > >
> > > For this testing, I used an mmap of the ROM address though, so the
> > > kernel caching shouldn't have been involved.  It looks to me like the
> > > problem is that the map function provided via pci_register_io_region()
> > > only knows how to create mappings, not tear them down.  I think maybe
> > > pci_update_mappings() should still call the map_func when new_addr is
> > > -1 to let the io space drive shutdown the mapping.  As it is, once we
> > > setup the mapping, it lives until something else happens to overlap it,
> > > regardless of the state of the PCI BAR.  Thanks,
> >
> > I think it may not necessary to tear them down, for the bar mapping won't
> > change IIUR.
>
> We can't guarantee that, the OS can move them if it understands the
> resources available to the PCI bus.  It typically doesn't move them
> though.
>
> > And you are accessing the sysfs file, right? In the Linux kernel, IIRC,
> > pci_create_sysfs_dev_files() create sysfs file, and hook the read to
> > pci_read_rom(), which called pci_map_rom(), which would call
> > pci_enable_rom(), and write the enable_rom bit to the rom_base_reg. So
> > that the rom can be read regardless of enable_rom bit state - and .
> >
> > But I also found something interested. The write hook of file,
> > pci_write_rom() seems won't cause NMI(and seems you need write a char
> > rather than 0 to enable the accessing?). So why NMI happen in host?...
>
> As I mentioned, I'm not using the /sys files to write to the ROM
> precisely because the rom write function is only to enable/disable the
> ROM BAR.  I'm using setpci to manually enable the ROM, then I use the
> test program below to mmap the ROM address from /dev/mem, read part of
> it, try to write the first few bytes, then read it back.  You'll need to
> change the hard coded address if you want to test yourself.  Obviously
> don't do it on a system in use by others since it will likely take it
> down.  Thanks,

Oh, yes. Sorry for completely miss the method... Yeah, by this method, the ROM 
shouldn't present to guest. And you are right, the PCI mapping is in only one 
direction. I think we can fix it in QEmu upstream.

-- 
regards
Yang, Sheng

>
> Alex
>
> #include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> #include <sys/mman.h>
> #include <sys/types.h>
> #include <sys/stat.h>
>
> #define DEV_MEM		"/dev/mem"
> #define ROM_ADDR	0xe6300000
>
> int main(void)
> {
> 	unsigned char *map;
> 	int i, fd = open(DEV_MEM, O_RDWR);
>
> 	if (fd == -1) {
> 		printf("Failed to open /dev/mem: %s\n", strerror(errno));
> 		return -1;
> 	}
>
> 	map = mmap(NULL, getpagesize(), PROT_READ|PROT_WRITE,
> 		   MAP_SHARED, fd, ROM_ADDR);
>
> 	if (map == MAP_FAILED) {
> 		printf("Failed to mmap /dev/mem: %s\n", strerror(errno));
> 		close(fd);
> 		return -1;
> 	}
>
> 	for (i = 0; i < 64;) {
> 		printf("%02x", map[i++]);
> 		if (i % 16 == 0)
> 			printf("\n");
> 		else if (i % 4 == 0)
> 			printf("  ");
> 		else
> 			printf(" ");
> 	}
>
> 	printf("Writing...");
> 	map[0] = 0xba;
> 	map[1] = 0xdb;
> 	map[2] = 0xad;
> 	map[3] = 0xc0;
> 	map[4] = 0xff;
> 	map[5] = 0xee;
> 	printf("done\n");
>
> 	for (i = 0; i < 64;) {
> 		printf("%02x", map[i++]);
> 		if (i % 16 == 0)
> 			printf("\n");
> 		else if (i % 4 == 0)
> 			printf("  ");
> 		else
> 			printf(" ");
> 	}
>
> 	munmap(map, getpagesize());
> 	close(fd);
> 	return 0;
> }