[Qemu-devel] [PATCH] Add rtl8139b

[Qemu-devel] [PATCH] Add rtl8139b

[takasi-y]

This adds support for RTL8139 below Rev.C.
Name is "rtl8139b".
"rtl8139" is still RTL8139C and above as it was.

Signed-off-by: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
---

Now hw/rtl8139.c can switch RTL8139C+ and others(call them B- here)
 by CPP definition. Actually, a difference is PCI id 1 byte.

But compile-time switch is hard to utilize because one binary support
multiple architecture.
This patch adds a capability to switch them runtime(on invoking qemu).

Now, RTL8139B- is referred by name "rtl8139b".
So, you can enable it with option "-net nic,model=rtl8139b"

The name "rtl8139" refers RTL8139C+ same as it was.
For users, this change should be seen as adding RTL8139B- but any other change.

---
 hw/pci.c     |    4 +++-
 hw/pci.h     |    1 +
 hw/rtl8139.c |   22 +++++++++++++++++-----
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 0689529..5415400 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -650,13 +650,15 @@ void pci_nic_init(PCIBus *bus, NICInfo *nd, int devfn)
         pci_i82559er_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "rtl8139") == 0) {
         pci_rtl8139_init(bus, nd, devfn);
+    } else if (strcmp(nd->model, "rtl8139b") == 0) {
+        pci_rtl8139b_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "e1000") == 0) {
         pci_e1000_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "pcnet") == 0) {
         pci_pcnet_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "?") == 0) {
         fprintf(stderr, "qemu: Supported PCI NICs: i82551 i82557b i82559er"
-                        " ne2k_pci pcnet rtl8139 e1000\n");
+                        " ne2k_pci pcnet rtl8139 rtl8139b e1000\n");
         exit (1);
     } else {
         fprintf(stderr, "qemu: Unsupported NIC: %s\n", nd->model);
diff --git a/hw/pci.h b/hw/pci.h
index 669703e..8ecec22 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -137,6 +137,7 @@ void pci_ne2000_init(PCIBus *bus, NICInfo *nd, int devfn);
 /* rtl8139.c */
 
 void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn);
+void pci_rtl8139b_init(PCIBus *bus, NICInfo *nd, int devfn);
 
 /* e1000.c */
 void pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn);
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index c3ab854..d717a7b 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -349,8 +349,6 @@ enum chip_flags {
 #define RTL8139_PCI_REVID_8139      0x10
 #define RTL8139_PCI_REVID_8139CPLUS 0x20
 
-#define RTL8139_PCI_REVID           RTL8139_PCI_REVID_8139CPLUS
-
 /* Size is 64 * 16bit words */
 #define EEPROM_9346_ADDR_BITS 6
 #define EEPROM_9346_SIZE  (1 << EEPROM_9346_ADDR_BITS)
@@ -2856,7 +2854,7 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
             break;
 
         case PCIRevisionID:
-            ret = RTL8139_PCI_REVID;
+            ret = s->pci_dev->config[0x08];
             DEBUG_PRINT(("RTL8139: PCI Revision ID read 0x%x\n", ret));
             break;
 
@@ -3400,7 +3398,7 @@ static void rtl8139_timer(void *opaque)
 }
 #endif /* RTL8139_ONBOARD_TIMER */
 
-void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
+RTL8139State *pci_rtl8139_common_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     PCIRTL8139State *d;
     RTL8139State *s;
@@ -3416,7 +3414,6 @@ void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
     pci_conf[0x02] = 0x39;
     pci_conf[0x03] = 0x81;
     pci_conf[0x04] = 0x05; /* command = I/O space, Bus Master */
-    pci_conf[0x08] = RTL8139_PCI_REVID; /* PCI revision ID; >=0x20 is for 8139C+ */
     pci_conf[0x0a] = 0x00; /* ethernet network controller */
     pci_conf[0x0b] = 0x02;
     pci_conf[0x0e] = 0x00; /* header_type */
@@ -3462,4 +3459,19 @@ void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
     qemu_mod_timer(s->timer,
         rtl8139_get_next_tctr_time(s,qemu_get_clock(vm_clock)));
 #endif /* RTL8139_ONBOARD_TIMER */
+    return s;
+}
+
+void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
+{
+    RTL8139State *s;
+    s = pci_rtl8139_common_init(bus, nd, devfn);
+    s->pci_dev->config[0x08] = RTL8139_PCI_REVID_8139CPLUS;
+}
+
+void pci_rtl8139b_init(PCIBus *bus, NICInfo *nd, int devfn)
+{
+    RTL8139State *s;
+    s = pci_rtl8139_common_init(bus, nd, devfn);
+    s->pci_dev->config[0x08] = RTL8139_PCI_REVID_8139;
 }
-- 
1.5.6.3

/yoshii

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Paul Brook]

On Sunday 14 December 2008, takasi-y@ops.dti.ne.jp wrote:
> This adds support for RTL8139 below Rev.C.
> Name is "rtl8139b".

You should also make this act like a revb device.

Paul

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Igor Kovalenko]

On Mon, Dec 15, 2008 at 3:47 AM, Paul Brook <paul@codesourcery.com> wrote:
> On Sunday 14 December 2008, takasi-y@ops.dti.ne.jp wrote:
>> This adds support for RTL8139 below Rev.C.
>> Name is "rtl8139b".
>
> You should also make this act like a revb device.
>

It should work as is, implemented registers map is the same as in
8139C. Driver will detect C+ capable device using PCI ID as a hint.

8139C+ is really a "8139C in extended mode" so well "8139B-" is a bit confusing.

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Jamie Lokier]

takasi-y@ops.dti.ne.jp wrote:
> +                        " ne2k_pci pcnet rtl8139 rtl8139b e1000\n");

On seeing this in qemu output, and with no documentation, it suggests
"rtl8139b" is a later device revision than "rtl8139", and preferable
to use for new VMs.

Imho it would be better to write " rtl8139b rtl8139 " and of course
document that rtl8139b is an older device than rtl8139.

-- Jamie

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Igor Kovalenko]

On Tue, Dec 16, 2008 at 10:19 PM, Jamie Lokier <jamie@shareable.org> wrote:
> takasi-y@ops.dti.ne.jp wrote:
>> +                        " ne2k_pci pcnet rtl8139 rtl8139b e1000\n");
>
> On seeing this in qemu output, and with no documentation, it suggests
> "rtl8139b" is a later device revision than "rtl8139", and preferable
> to use for new VMs.
>
> Imho it would be better to write " rtl8139b rtl8139 " and of course
> document that rtl8139b is an older device than rtl8139.
>

-net nic,model=rtl8139[,quirks=8139cplus|8139b] default to 8139cplus

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Alexey Eremenko]

What is the reason behind implementing two versions of the same NIC in Qemu ?

-- 
-Alexey Eromenko "Technologov"

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Igor Kovalenko]

On Tue, Dec 16, 2008 at 10:53 PM, Alexey Eremenko <al4321@gmail.com> wrote:
> What is the reason behind implementing two versions of the same NIC in Qemu ?

No idea :) But the device is capable to work in ether of two modes,
older and cplus, the availability of latter is hinted by appropriate
PCI rev id.

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Jamie Lokier]

Igor Kovalenko wrote:
> On Tue, Dec 16, 2008 at 10:19 PM, Jamie Lokier <jamie@shareable.org> wrote:
> > takasi-y@ops.dti.ne.jp wrote:
> >> +                        " ne2k_pci pcnet rtl8139 rtl8139b e1000\n");
> >
> > On seeing this in qemu output, and with no documentation, it suggests
> > "rtl8139b" is a later device revision than "rtl8139", and preferable
> > to use for new VMs.
> >
> > Imho it would be better to write " rtl8139b rtl8139 " and of course
> > document that rtl8139b is an older device than rtl8139.
> >
> 
> -net nic,model=rtl8139[,quirks=8139cplus|8139b] default to 8139cplus

Nice!  I'd change to [,revision=8139cplus|8139b], as it's not a quirk.

-- Jamie

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Andreas Färber]

Am 16.12.2008 um 21:42 schrieb Jamie Lokier:

> Igor Kovalenko wrote:
>> On Tue, Dec 16, 2008 at 10:19 PM, Jamie Lokier  
>> <jamie@shareable.org> wrote:
>>> takasi-y@ops.dti.ne.jp wrote:
>>>> +                        " ne2k_pci pcnet rtl8139 rtl8139b  
>>>> e1000\n");
>>>
>>> On seeing this in qemu output, and with no documentation, it  
>>> suggests
>>> "rtl8139b" is a later device revision than "rtl8139", and preferable
>>> to use for new VMs.
>>>
>>> Imho it would be better to write " rtl8139b rtl8139 " and of course
>>> document that rtl8139b is an older device than rtl8139.
>>>
>>
>> -net nic,model=rtl8139[,quirks=8139cplus|8139b] default to 8139cplus
>
> Nice!  I'd change to [,revision=8139cplus|8139b], as it's not a quirk.

In that case [,revision=cplus|b] would be even simpler. :)

Andreas

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Igor Kovalenko]

On Wed, Dec 17, 2008 at 12:36 AM, Andreas Färber <andreas.faerber@web.de> wrote:
>
> Am 16.12.2008 um 21:42 schrieb Jamie Lokier:
>
>> Igor Kovalenko wrote:
>>>
>>> On Tue, Dec 16, 2008 at 10:19 PM, Jamie Lokier <jamie@shareable.org>
>>> wrote:
>>>>
>>>> takasi-y@ops.dti.ne.jp wrote:
>>>>>
>>>>> +                        " ne2k_pci pcnet rtl8139 rtl8139b e1000\n");
>>>>
>>>> On seeing this in qemu output, and with no documentation, it suggests
>>>> "rtl8139b" is a later device revision than "rtl8139", and preferable
>>>> to use for new VMs.
>>>>
>>>> Imho it would be better to write " rtl8139b rtl8139 " and of course
>>>> document that rtl8139b is an older device than rtl8139.
>>>>
>>>
>>> -net nic,model=rtl8139[,quirks=8139cplus|8139b] default to 8139cplus
>>
>> Nice!  I'd change to [,revision=8139cplus|8139b], as it's not a quirk.
>
> In that case [,revision=cplus|b] would be even simpler. :)
>

Well, no, C+ is a mode of operation supported on 8139 rev C chip, and
probably some later ones. Not "all older chips starting with rev b do
not have it". Extended mode can cause problems with some buggy C+ mode
drivers so it is sometimes handy to disable C+ mode and revert to the
other "compatible" but somewhat slower driver.

Maybe original poster's rationale is required to continue :)

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH] Add rtl8139b

[Igor Kovalenko]

On Wed, Dec 17, 2008 at 1:12 AM, Igor Kovalenko
<igor.v.kovalenko@gmail.com> wrote:
> On Wed, Dec 17, 2008 at 12:36 AM, Andreas Färber <andreas.faerber@web.de> wrote:
>>
>> Am 16.12.2008 um 21:42 schrieb Jamie Lokier:
>>
>>> Igor Kovalenko wrote:
>>>>
>>>> On Tue, Dec 16, 2008 at 10:19 PM, Jamie Lokier <jamie@shareable.org>
>>>> wrote:
>>>>>
>>>>> takasi-y@ops.dti.ne.jp wrote:
>>>>>>
>>>>>> +                        " ne2k_pci pcnet rtl8139 rtl8139b e1000\n");
>>>>>
>>>>> On seeing this in qemu output, and with no documentation, it suggests
>>>>> "rtl8139b" is a later device revision than "rtl8139", and preferable
>>>>> to use for new VMs.
>>>>>
>>>>> Imho it would be better to write " rtl8139b rtl8139 " and of course
>>>>> document that rtl8139b is an older device than rtl8139.
>>>>>
>>>>
>>>> -net nic,model=rtl8139[,quirks=8139cplus|8139b] default to 8139cplus
>>>
>>> Nice!  I'd change to [,revision=8139cplus|8139b], as it's not a quirk.
>>
>> In that case [,revision=cplus|b] would be even simpler. :)
>>
>
> Well, no, C+ is a mode of operation supported on 8139 rev C chip, and
> probably some later ones. Not "all older chips starting with rev b do
> not have it". Extended mode can cause problems with some buggy C+ mode
> drivers so it is sometimes handy to disable C+ mode and revert to the
> other "compatible" but somewhat slower driver.
>
> Maybe original poster's rationale is required to continue :)
>

FWIW RTL8139C board had different revs. Some were exposing PCI ids of
less than 0x20 which now means "no c+ mode supported".

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH] Add rtl8139b

[takasi-y]

Hi,
"Alexey Eremenko" <al4321@gmail.com> wrote:
> What is the reason behind implementing two versions of the same NIC in Qemu ?
Simple answer is because it is on "r2d".
(r2d(hw/r2d.c in qemu) is a board which has sh4 CPU and rtl8139 and others on it)

More precisely, because..
1. rtl8139 is supported by two different drivers in Linux kernel.
  Devices RevID<0x20 are supported by "8139too", others are by "8139cp".
2. Not like PC in qemu which has very flexible model, r2d is more
 specifically modeled. That is not only because HW is not flexible very much,
but also because software is built statically, as is common for embed devices.
Usually, linux kernel for r2d statically includes 8139too, but 8139cp.

"Igor Kovalenko" <igor.v.kovalenko@gmail.com> wrote:
> Maybe original poster's rationale is required to continue :)

As far as my understanding, C+ is "B + new functions".
B's device drivers can handle it in same way, though RevID differs.
That's why my patch changes only RevID. Actually, "change only RevID" was
the starting point. I did it because I thought I can support B with small
modifications, and with only little side effect.

> -net nic,model=rtl8139[,quirks=8139cplus|8139b] default to 8139cplus
I think this change is a bit heavy.
Now, qemu code requires different name for different NIC, because differences
 of NICs are handled by NICInfo.model string.

Updated patch attached. Still no documentation (only enumerating a name)
 because I can't find suitable place.
How about using somewhat more self descriptive name like "rtl8139old" ?
/yoshii

This adds support for RTL8139 below Rev.C.
Name is "rtl8139b".
"rtl8139" is still RTL8139C and above as it was.

Signed-off-by: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
---
 hw/pci.c      |    4 +++-
 hw/pci.h      |    1 +
 hw/r2d.c      |    2 +-
 hw/rtl8139.c  |   33 +++++++++++++++++++++------------
 qemu-doc.texi |    2 +-
 5 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 8252d21..9950d29 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -663,6 +663,8 @@ void pci_nic_init(PCIBus *bus, NICInfo *nd, int devfn)
         pci_i82557b_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "i82559er") == 0) {
         pci_i82559er_init(bus, nd, devfn);
+    } else if (strcmp(nd->model, "rtl8139b") == 0) {
+        pci_rtl8139b_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "rtl8139") == 0) {
         pci_rtl8139_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "e1000") == 0) {
@@ -673,7 +675,7 @@ void pci_nic_init(PCIBus *bus, NICInfo *nd, int devfn)
         virtio_net_init(bus, nd, devfn);
     } else if (strcmp(nd->model, "?") == 0) {
         fprintf(stderr, "qemu: Supported PCI NICs: i82551 i82557b i82559er"
-                        " ne2k_pci pcnet rtl8139 e1000 virtio\n");
+                        " ne2k_pci pcnet rtl8139b rtl8139 e1000 virtio\n");
         exit (1);
     } else {
         fprintf(stderr, "qemu: Unsupported NIC: %s\n", nd->model);
diff --git a/hw/pci.h b/hw/pci.h
index 3b1caf5..daca180 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -157,6 +157,7 @@ void pci_ne2000_init(PCIBus *bus, NICInfo *nd, int devfn);
 /* rtl8139.c */
 
 void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn);
+void pci_rtl8139b_init(PCIBus *bus, NICInfo *nd, int devfn);
 
 /* e1000.c */
 void pci_e1000_init(PCIBus *bus, NICInfo *nd, int devfn);
diff --git a/hw/r2d.c b/hw/r2d.c
index 5d5eb1e..e2496fe 100644
--- a/hw/r2d.c
+++ b/hw/r2d.c
@@ -230,7 +230,7 @@ static void r2d_init(ram_addr_t ram_size, int vga_ram_size,
         drives_table[drive_get_index(IF_IDE, 0, 0)].bdrv, NULL);
 
     /* NIC: rtl8139 on-board, and 2 slots. */
-    pci_rtl8139_init(pci, &nd_table[0], 2 << 3);
+    pci_rtl8139b_init(pci, &nd_table[0], 2 << 3);
     for (i = 1; i < nb_nics; i++)
         pci_nic_init(pci, &nd_table[i], -1);
 
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index c3ab854..2aa889c 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -349,8 +349,6 @@ enum chip_flags {
 #define RTL8139_PCI_REVID_8139      0x10
 #define RTL8139_PCI_REVID_8139CPLUS 0x20
 
-#define RTL8139_PCI_REVID           RTL8139_PCI_REVID_8139CPLUS
-
 /* Size is 64 * 16bit words */
 #define EEPROM_9346_ADDR_BITS 6
 #define EEPROM_9346_SIZE  (1 << EEPROM_9346_ADDR_BITS)
@@ -1213,13 +1211,13 @@ static void rtl8139_reset(RTL8139State *s)
     /* ACK the reset */
     s->TxConfig = 0;
 
-#if 0
-//    s->TxConfig |= HW_REVID(1, 0, 0, 0, 0, 0, 0); // RTL-8139  HasHltClk
-    s->clock_enabled = 0;
-#else
-    s->TxConfig |= HW_REVID(1, 1, 1, 0, 1, 1, 0); // RTL-8139C+ HasLWake
-    s->clock_enabled = 1;
-#endif
+    if (s->pci_dev->config[0x08] == RTL8139_PCI_REVID_8139) {
+        s->TxConfig |= HW_REVID(1, 0, 0, 0, 0, 0, 0); // RTL-8139  HasHltClk
+        s->clock_enabled = 0;
+    } else {
+        s->TxConfig |= HW_REVID(1, 1, 1, 0, 1, 1, 0); // RTL-8139C+ HasLWake
+        s->clock_enabled = 1;
+    }
 
     s->bChipCmdState = CmdReset; /* RxBufEmpty bit is calculated on read from ChipCmd */;
 
@@ -2856,7 +2854,7 @@ static uint32_t rtl8139_io_readb(void *opaque, uint8_t addr)
             break;
 
         case PCIRevisionID:
-            ret = RTL8139_PCI_REVID;
+            ret = s->pci_dev->config[0x08];
             DEBUG_PRINT(("RTL8139: PCI Revision ID read 0x%x\n", ret));
             break;
 
@@ -3400,7 +3398,8 @@ static void rtl8139_timer(void *opaque)
 }
 #endif /* RTL8139_ONBOARD_TIMER */
 
-void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
+void pci_rtl8139_common_init(PCIBus *bus, NICInfo *nd, int devfn,
+                                      uint8_t revid)
 {
     PCIRTL8139State *d;
     RTL8139State *s;
@@ -3416,7 +3415,7 @@ void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
     pci_conf[0x02] = 0x39;
     pci_conf[0x03] = 0x81;
     pci_conf[0x04] = 0x05; /* command = I/O space, Bus Master */
-    pci_conf[0x08] = RTL8139_PCI_REVID; /* PCI revision ID; >=0x20 is for 8139C+ */
+    pci_conf[0x08] = revid; /* PCI revision ID; >=0x20 is for 8139C+ */
     pci_conf[0x0a] = 0x00; /* ethernet network controller */
     pci_conf[0x0b] = 0x02;
     pci_conf[0x0e] = 0x00; /* header_type */
@@ -3463,3 +3462,13 @@ void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
         rtl8139_get_next_tctr_time(s,qemu_get_clock(vm_clock)));
 #endif /* RTL8139_ONBOARD_TIMER */
 }
+
+void pci_rtl8139b_init(PCIBus *bus, NICInfo *nd, int devfn)
+{
+    pci_rtl8139_common_init(bus, nd, devfn, RTL8139_PCI_REVID_8139);
+}
+
+void pci_rtl8139_init(PCIBus *bus, NICInfo *nd, int devfn)
+{
+    pci_rtl8139_common_init(bus, nd, devfn, RTL8139_PCI_REVID_8139CPLUS);
+}
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 698e0d5..b826e01 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -614,7 +614,7 @@ target. Optionally, the MAC address can be changed. If no
 Qemu can emulate several different models of network card.
 Valid values for @var{type} are
 @code{i82551}, @code{i82557b}, @code{i82559er},
-@code{ne2k_pci}, @code{ne2k_isa}, @code{pcnet}, @code{rtl8139},
+@code{ne2k_pci}, @code{ne2k_isa}, @code{pcnet}, @code{rtl8139b}, @code{rtl8139},
 @code{e1000}, @code{smc91c111}, @code{lance} and @code{mcf_fec}.
 Not all devices are supported on all targets.  Use -net nic,model=?
 for a list of available devices for your target.
-- 
1.5.6.3