On Wed, Sep 26, 2007 at 06:02:21PM +0200, Laurent Vivier wrote:
> Daniel P. Berrange wrote:
> > On Wed, Sep 26, 2007 at 05:47:20PM +0200, Laurent Vivier wrote:
> >> Hi,
> >>
> >> I think there is a bug in qemu RTL8139.
> >>
> >> RTL8139 uses:
> >>
> >> cpu_register_physical_memory(addr + 0, 0x100, s->rtl8139_mmio_io_addr);
> >>
> >> But in the comment of cpu_register_physical_memory() we have:
> >>
> >> "'size' must be a multiple of the target page size."
> >>
> >> And I think 0x100 is not a multiple of target page size.... :-P
> >
> > Latest upstream QEMU has fixed its memory handling so that MMIO regions
> > do not need to be a multiple of page size. Changing RTL8139 to use a
> > block of size 0x1000 is a reasonable short term hack around the problem,
> > but syncing with latest QEMU is the real solution, since there are other
> > places in the code which will have similar issues.
> >
>
> So this explains why rtl8139.c from QEMU CVS always uses 0x100.
>
> Thank you for the comment.
>
> Avi, you know what you have to do ;-)
I did start on back porting the QEMU subpage handling fixes to KVM for
Fedora 7, but in the end went for the simpler s/0x100/0x1000/ quick hack.
I'm attaching the patch which I started against kvm-24 in case it is useful,
though note that the only testing I did with it was to see if a F7 guest
booted and saw distinct MAC addrs. It should apply with minor fuzz/offsets
to at least kvm-35.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
diff -rup kvm-24/qemu/cpu-all.h kvm-24.new/qemu/cpu-all.h
--- kvm-24/qemu/cpu-all.h 2007-05-08 04:44:27.000000000 -0400
+++ kvm-24.new/qemu/cpu-all.h 2007-07-16 14:39:16.000000000 -0400
@@ -841,6 +841,7 @@ extern uint8_t *bios_mem;
exception, the write memory callback gets the ram offset instead of
the physical address */
#define IO_MEM_ROMD (1)
+#define IO_MEM_SUBPAGE (2)
typedef void CPUWriteMemoryFunc(void *opaque, target_phys_addr_t addr,
uint32_t value);
typedef uint32_t CPUReadMemoryFunc(void *opaque, target_phys_addr_t addr);
diff -rup kvm-24/qemu/exec.c kvm-24.new/qemu/exec.c
--- kvm-24/qemu/exec.c 2007-05-08 04:44:27.000000000 -0400
+++ kvm-24.new/qemu/exec.c 2007-07-16 14:33:40.000000000 -0400
@@ -144,6 +144,14 @@ static int tlb_flush_count;
static int tb_flush_count;
static int tb_phys_invalidate_count;
+#define SUBPAGE_IDX(addr) ((addr) & ~TARGET_PAGE_MASK)
+typedef struct subpage_t {
+ target_phys_addr_t base;
+ CPUReadMemoryFunc **mem_read[TARGET_PAGE_SIZE];
+ CPUWriteMemoryFunc **mem_write[TARGET_PAGE_SIZE];
+ void *opaque[TARGET_PAGE_SIZE];
+} subpage_t;
+
static void page_init(void)
{
/* NOTE: we can always suppose that qemu_host_page_size >=
@@ -1808,6 +1816,30 @@ static inline void tlb_set_dirty(CPUStat
}
#endif /* defined(CONFIG_USER_ONLY) */
+static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
+ int memory);
+static void *subpage_init (target_phys_addr_t base, uint32_t *phys,
+ int orig_memory);
+#define CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr, end_addr2, \
+ need_subpage) \
+ do { \
+ if (addr > start_addr) \
+ start_addr2 = 0; \
+ else { \
+ start_addr2 = start_addr & ~TARGET_PAGE_MASK; \
+ if (start_addr2 > 0) \
+ need_subpage = 1; \
+ } \
+ \
+ if ((start_addr + orig_size) - addr >= TARGET_PAGE_SIZE) \
+ end_addr2 = TARGET_PAGE_SIZE - 1; \
+ else { \
+ end_addr2 = (start_addr + orig_size - 1) & ~TARGET_PAGE_MASK; \
+ if (end_addr2 < TARGET_PAGE_SIZE - 1) \
+ need_subpage = 1; \
+ } \
+ } while (0)
+
/* register physical memory. 'size' must be a multiple of the target
page size. If (phys_offset & ~TARGET_PAGE_MASK) != 0, then it is an
io memory page */
@@ -1818,15 +1850,56 @@ void cpu_register_physical_memory(target
target_phys_addr_t addr, end_addr;
PhysPageDesc *p;
CPUState *env;
+ unsigned long orig_size = size;
+ void *subpage;
size = (size + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
- end_addr = start_addr + size;
+ end_addr = start_addr + (target_phys_addr_t)size;
for(addr = start_addr; addr != end_addr; addr += TARGET_PAGE_SIZE) {
- p = phys_page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
- p->phys_offset = phys_offset;
- if ((phys_offset & ~TARGET_PAGE_MASK) <= IO_MEM_ROM ||
- (phys_offset & IO_MEM_ROMD))
- phys_offset += TARGET_PAGE_SIZE;
+ p = phys_page_find(addr >> TARGET_PAGE_BITS);
+ if (p && p->phys_offset != IO_MEM_UNASSIGNED) {
+ unsigned long orig_memory = p->phys_offset;
+ target_phys_addr_t start_addr2, end_addr2;
+ int need_subpage = 0;
+
+ CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr, end_addr2,
+ need_subpage);
+ if (need_subpage) {
+ if (!(orig_memory & IO_MEM_SUBPAGE)) {
+ subpage = subpage_init((addr & TARGET_PAGE_MASK),
+ &p->phys_offset, orig_memory);
+ } else {
+ subpage = io_mem_opaque[(orig_memory & ~TARGET_PAGE_MASK)
+ >> IO_MEM_SHIFT];
+ }
+ subpage_register(subpage, start_addr2, end_addr2, phys_offset);
+ } else {
+ p->phys_offset = phys_offset;
+ if ((phys_offset & ~TARGET_PAGE_MASK) <= IO_MEM_ROM ||
+ (phys_offset & IO_MEM_ROMD))
+ phys_offset += TARGET_PAGE_SIZE;
+ }
+ } else {
+ p = phys_page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
+ p->phys_offset = phys_offset;
+ if ((phys_offset & ~TARGET_PAGE_MASK) <= IO_MEM_ROM ||
+ (phys_offset & IO_MEM_ROMD))
+ phys_offset += TARGET_PAGE_SIZE;
+ else {
+ target_phys_addr_t start_addr2, end_addr2;
+ int need_subpage = 0;
+
+ CHECK_SUBPAGE(addr, start_addr, start_addr2, end_addr,
+ end_addr2, need_subpage);
+
+ if (need_subpage) {
+ subpage = subpage_init((addr & TARGET_PAGE_MASK),
+ &p->phys_offset, IO_MEM_UNASSIGNED);
+ subpage_register(subpage, start_addr2, end_addr2,
+ phys_offset);
+ }
+ }
+ }
}
/* since each CPU stores ram addresses in its TLB cache, we must
@@ -1965,6 +2038,149 @@ static CPUWriteMemoryFunc *notdirty_mem_
notdirty_mem_writel,
};
+static inline uint32_t subpage_readlen (subpage_t *mmio, target_phys_addr_t
addr,
+ unsigned int len)
+{
+ CPUReadMemoryFunc **mem_read;
+ uint32_t ret;
+ unsigned int idx;
+
+ idx = SUBPAGE_IDX(addr - mmio->base);
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d\n", __func__,
+ mmio, len, addr, idx);
+#endif
+ mem_read = mmio->mem_read[idx];
+ ret = (*mem_read[len])(mmio->opaque[idx], addr);
+
+ return ret;
+}
+
+static inline void subpage_writelen (subpage_t *mmio, target_phys_addr_t addr,
+ uint32_t value, unsigned int len)
+{
+ CPUWriteMemoryFunc **mem_write;
+ unsigned int idx;
+
+ idx = SUBPAGE_IDX(addr - mmio->base);
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: subpage %p len %d addr " TARGET_FMT_plx " idx %d value
%08x\n", __func__,
+ mmio, len, addr, idx, value);
+#endif
+ mem_write = mmio->mem_write[idx];
+ (*mem_write[len])(mmio->opaque[idx], addr, value);
+}
+
+static uint32_t subpage_readb (void *opaque, target_phys_addr_t addr)
+{
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
+#endif
+
+ return subpage_readlen(opaque, addr, 0);
+}
+
+static void subpage_writeb (void *opaque, target_phys_addr_t addr,
+ uint32_t value)
+{
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value);
+#endif
+ subpage_writelen(opaque, addr, value, 0);
+}
+
+static uint32_t subpage_readw (void *opaque, target_phys_addr_t addr)
+{
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
+#endif
+
+ return subpage_readlen(opaque, addr, 1);
+}
+
+static void subpage_writew (void *opaque, target_phys_addr_t addr,
+ uint32_t value)
+{
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value);
+#endif
+ subpage_writelen(opaque, addr, value, 1);
+}
+
+static uint32_t subpage_readl (void *opaque, target_phys_addr_t addr)
+{
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: addr " TARGET_FMT_plx "\n", __func__, addr);
+#endif
+
+ return subpage_readlen(opaque, addr, 2);
+}
+
+static void subpage_writel (void *opaque,
+ target_phys_addr_t addr, uint32_t value)
+{
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: addr " TARGET_FMT_plx " val %08x\n", __func__, addr, value);
+#endif
+ subpage_writelen(opaque, addr, value, 2);
+}
+
+static CPUReadMemoryFunc *subpage_read[] = {
+ &subpage_readb,
+ &subpage_readw,
+ &subpage_readl,
+};
+
+static CPUWriteMemoryFunc *subpage_write[] = {
+ &subpage_writeb,
+ &subpage_writew,
+ &subpage_writel,
+};
+
+static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
+ int memory)
+{
+ int idx, eidx;
+
+ if (start >= TARGET_PAGE_SIZE || end >= TARGET_PAGE_SIZE)
+ return -1;
+ idx = SUBPAGE_IDX(start);
+ eidx = SUBPAGE_IDX(end);
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: %p start %08x end %08x idx %08x eidx %08x mem %d\n", __func__,
+ mmio, start, end, idx, eidx, memory);
+#endif
+ memory >>= IO_MEM_SHIFT;
+ for (; idx <= eidx; idx++) {
+ mmio->mem_read[idx] = io_mem_read[memory];
+ mmio->mem_write[idx] = io_mem_write[memory];
+ mmio->opaque[idx] = io_mem_opaque[memory];
+ }
+
+ return 0;
+}
+
+static void *subpage_init (target_phys_addr_t base, uint32_t *phys,
+ int orig_memory)
+{
+ subpage_t *mmio;
+ int subpage_memory;
+
+ mmio = qemu_mallocz(sizeof(subpage_t));
+ if (mmio != NULL) {
+ mmio->base = base;
+ subpage_memory = cpu_register_io_memory(0, subpage_read,
subpage_write, mmio);
+#if defined(DEBUG_SUBPAGE)
+ printf("%s: %p base " TARGET_FMT_plx " len %08x %d\n", __func__,
+ mmio, base, TARGET_PAGE_SIZE, subpage_memory);
+#endif
+ *phys = subpage_memory | IO_MEM_SUBPAGE;
+ subpage_register(mmio, 0, TARGET_PAGE_SIZE - 1, orig_memory);
+ }
+
+ return mmio;
+}
+
static void io_mem_init(void)
{
cpu_register_io_memory(IO_MEM_ROM >> IO_MEM_SHIFT, error_mem_read,
unassigned_mem_write, NULL);
