[Qemu-devel] Re: [kvm-devel] [ kvm-Bugs-1802223 ] nics have same hw address (rtl8139)

[Qemu-devel] Re: [kvm-devel] [ kvm-Bugs-1802223 ] nics have same hw address (rtl8139)

[Laurent Vivier]

[-- Attachment #1.1: Type: text/plain, Size: 3312 bytes --]

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

The attached patch corrects the problem.

Laurent

SourceForge.net wrote:
> Bugs item #1802223, was opened at 2007-09-25 22:59
> Message generated for change (Tracker Item Submitted) made by Item Submitter
> You can respond by visiting: 
> https://sourceforge.net/tracker/?func=detail&atid=893831&aid=1802223&group_id=180599
> 
> Please note that this message will contain a full copy of the comment thread,
> including the initial issue submission, for this request,
> not just the latest update.
> Category: None
> Group: None
> Status: Open
> Resolution: None
> Priority: 5
> Private: No
> Submitted By: xeb (xebd)
> Assigned to: Nobody/Anonymous (nobody)
> Summary: nics have same hw address (rtl8139)
> 
> Initial Comment:
> Hello!
> 
> Host:Linux 2.6.22-gentoo-r2 #2 SMP Fri Aug 3 07:01:46 MSD 2007 x86_64 AMD Athlon(tm) 64 X2 Dual Core Processor 5600+ AuthenticAMD GNU/Linux,Gentoo, kvm-44
> 
> Guest:Linux 2.6.22-hardened-r4
> 
> command line:
> qemu-system-x86_64 -hda server_base_x86.img -hdc server_swap.img -localtime -m 128 \
>     -net nic,vlan=0,macaddr=52:54:00:12:34:56,model=rtl8139 -net tap,vlan=0,ifname=tap3,script=no \
>     -net nic,vlan=1,macaddr=52:54:00:12:34:57,model=rtl8139 -net tap,vlan=1,ifname=tap4,script=no \
>     -net nic,vlan=2,macaddr=52:54:00:12:34:58,model=rtl8139 -net tap,vlan=2,ifname=tap5,script=no \
>     -nographic
> 
> ifconfig on guest:
> eth0      Link encap:Ethernet  HWaddr 52:54:00:12:34:58
>           UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
>           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:1000
>           RX bytes:0 (0.0 b)  TX bytes:0 (0.0 b)
>           Interrupt:11 Base address:0x4000
> 
> eth1      Link encap:Ethernet  HWaddr 52:54:00:12:34:58
>           inet addr:192.168.11.1  Bcast:192.168.11.255  Mask:255.255.255.0
>           UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
>           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:1000
>           RX bytes:0 (0.0 b)  TX bytes:0 (0.0 b)
>           Interrupt:9 Base address:0x6100
> 
> eth2      Link encap:Ethernet  HWaddr 52:54:00:12:34:58
>           inet addr:192.168.13.1  Bcast:192.168.13.255  Mask:255.255.255.0
>           UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
>           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:1000
>           RX bytes:0 (0.0 b)  TX bytes:0 (0.0 b)
>           Interrupt:11 Base address:0x8200
> 
> As can you see they have same hwaddr.
> With model=ne2k_pci nics have correct hw addresses 

Laurent
-- 
------------- Laurent.Vivier@bull.net  --------------
          "Software is hard" - Donald Knuth

[-- Attachment #1.2: rtl8139.patch --]
[-- Type: application/mbox, Size: 1402 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[Qemu-devel] Re: [kvm-devel] [ kvm-Bugs-1802223 ] nics have same hw address (rtl8139)

[Daniel P. Berrange]

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.

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  -=| 

[Qemu-devel] Re: [kvm-devel] [ kvm-Bugs-1802223 ] nics have same hw address (rtl8139)

[Laurent Vivier]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

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 ;-)

Laurent
-- 
------------- Laurent.Vivier@bull.net  --------------
          "Software is hard" - Donald Knuth


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[Qemu-devel] Re: [kvm-devel] [ kvm-Bugs-1802223 ] nics have same hw address (rtl8139)

[Daniel P. Berrange]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

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  -=| 

[-- Attachment #2: kvm-subpages.patch --]
[-- Type: text/plain, Size: 10340 bytes --]

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);

[Qemu-devel] Re: [kvm-devel] [ kvm-Bugs-1802223 ] nics have same hw address (rtl8139)

[Avi Kivity]

Daniel P. Berrange wrote:
> 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.
>   

Everything points to a qemu merge being sorely needed.  Does anyone have
any experience with recent versions?


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.