[Qemu-devel] [PATCH V2] e1000: Handle IO Port.

[Qemu-devel] [PATCH V2] e1000: Handle IO Port.

[Anthony PERARD]

This patch introduces the two IOPorts on e1000, IOADDR and IODATA. The
IOADDR is used to specify which register we want to access when we read
or write on IODATA.

This patch fixes some weird behavior that I see when I use e1000 with
QEMU/Xen, the guest memory can be corrupted by this NIC because it will
write on memory that it doesn't own anymore after a reset. It's because
the kernel Linux use the IOPort to reset the network card instead of the
MMIO.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

New:
  - remove ioport_reg[2], and use only ioport_addr.
  - append ioport_addr to VMState structure.
  - Reuse e1000_mmio_{readl,writel} in e1000_ioport_{readl,writel} to avoid
    duplication of code.


 hw/e1000.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 96d84f9..332a034 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -58,6 +58,9 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
 #define PNPMMIO_SIZE      0x20000
 #define MIN_BUF_SIZE      60 /* Min. octets in an ethernet frame sans FCS */
 
+#define REG_IOADDR 0x0
+#define REG_IODATA 0x4
+
 /*
  * HW models:
  *  E1000_DEV_ID_82540EM works with Windows and Linux
@@ -83,6 +86,8 @@ typedef struct E1000State_st {
     NICState *nic;
     NICConf conf;
     int mmio_index;
+    int ioport_base;
+    uint32_t ioport_addr;
 
     uint32_t mac_reg[0x8000];
     uint16_t phy_reg[0x20];
@@ -150,13 +155,7 @@ static const char phy_regcap[0x20] = {
     [PHY_ID2] = PHY_R,		[M88E1000_PHY_SPEC_STATUS] = PHY_R
 };
 
-static void
-ioport_map(PCIDevice *pci_dev, int region_num, pcibus_t addr,
-           pcibus_t size, int type)
-{
-    DBGOUT(IO, "e1000_ioport_map addr=0x%04"FMT_PCIBUS
-           " size=0x%08"FMT_PCIBUS"\n", addr, size);
-}
+static void e1000_reset(void *opaque);
 
 static void
 set_interrupt_cause(E1000State *s, int index, uint32_t val)
@@ -202,6 +201,11 @@ rxbufsize(uint32_t v)
 static void
 set_ctrl(E1000State *s, int index, uint32_t val)
 {
+    DBGOUT(IO, "set ctrl = %08x\n", val);
+    if (val & E1000_CTRL_RST) {
+        s->mac_reg[CTRL] = val;
+        e1000_reset(s);
+    }
     /* RST is self clearing */
     s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
 }
@@ -964,6 +968,38 @@ e1000_mmio_readw(void *opaque, target_phys_addr_t addr)
             (8 * (addr & 3))) & 0xffff;
 }
 
+static void
+e1000_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
+{
+    E1000State *s = opaque;
+
+    if (addr == s->ioport_base + REG_IOADDR) {
+        DBGOUT(IO, "e1000_ioport_writel write base: 0x%04x\n", val);
+        s->ioport_addr = val & 0xfffff;
+    } else if (addr == (s->ioport_base + REG_IODATA)) {
+        DBGOUT(IO, "e1000_ioport_writel %x: 0x%04x\n", s->ioport_addr, val);
+        e1000_mmio_writel(s, s->ioport_addr, val);
+    } else {
+        DBGOUT(UNKNOWN, "IO unknown write addr=0x%08x,val=0x%08x\n",
+               addr, val);
+    }
+}
+
+static uint32_t
+e1000_ioport_readl(void *opaque, uint32_t addr)
+{
+    E1000State *s = opaque;
+
+    if (addr == s->ioport_base + REG_IOADDR) {
+        return s->ioport_addr & 0xfffff;
+    } else if (addr == (s->ioport_base + REG_IODATA)) {
+        return e1000_mmio_readl(s, s->ioport_addr);
+    } else {
+        DBGOUT(UNKNOWN, "IO unknown read addr=0x%08x\n", addr);
+    }
+    return 0;
+}
+
 static bool is_version_1(void *opaque, int version_id)
 {
     return version_id == 1;
@@ -971,7 +1007,7 @@ static bool is_version_1(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_e1000 = {
     .name = "e1000",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField []) {
@@ -1043,6 +1079,7 @@ static const VMStateDescription vmstate_e1000 = {
         VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, RA, 32),
         VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, MTA, 128),
         VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, VFTA, 128),
+        VMSTATE_UINT32_V(ioport_addr, E1000State, 3),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -1083,6 +1120,30 @@ static const uint32_t mac_reg_init[] = {
 
 /* PCI interface */
 
+static void
+e1000_ioport_map(PCIDevice *pci_dev, int region_num, pcibus_t addr,
+                 pcibus_t size, int type)
+{
+    E1000State *d = DO_UPCAST(E1000State, dev, pci_dev);
+
+    DBGOUT(IO, "e1000_ioport_map addr=0x%04" FMT_PCIBUS
+           " size=0x%08" FMT_PCIBUS "\n", addr, size);
+
+    d->ioport_base = addr;
+
+    /* Writes that are less than 32 bits are ignored on IOADDR.
+     * For the Flash access, a write can be less than 32 bits for
+     * IODATA register, but is not handled.
+     */
+
+    register_ioport_read(addr, size, 1, e1000_ioport_readl, d);
+
+    register_ioport_read(addr, size, 2, e1000_ioport_readl, d);
+
+    register_ioport_write(addr, size, 4, e1000_ioport_writel, d);
+    register_ioport_read(addr, size, 4, e1000_ioport_readl, d);
+}
+
 static CPUWriteMemoryFunc * const e1000_mmio_write[] = {
     e1000_mmio_writeb,	e1000_mmio_writew,	e1000_mmio_writel
 };
@@ -1137,6 +1198,7 @@ static void e1000_reset(void *opaque)
 {
     E1000State *d = opaque;
 
+    d->ioport_addr = 0;
     memset(d->phy_reg, 0, sizeof d->phy_reg);
     memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
     memset(d->mac_reg, 0, sizeof d->mac_reg);
@@ -1179,7 +1241,7 @@ static int pci_e1000_init(PCIDevice *pci_dev)
                            PCI_BASE_ADDRESS_SPACE_MEMORY, e1000_mmio_map);
 
     pci_register_bar(&d->dev, 1, IOPORT_SIZE,
-                           PCI_BASE_ADDRESS_SPACE_IO, ioport_map);
+                           PCI_BASE_ADDRESS_SPACE_IO, e1000_ioport_map);
 
     memmove(d->eeprom_data, e1000_eeprom_template,
         sizeof e1000_eeprom_template);
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH V2] e1000: Handle IO Port.

[Peter Maydell]

On 30 June 2011 20:28, Anthony PERARD <anthony.perard@citrix.com> wrote:
> @@ -202,6 +201,11 @@ rxbufsize(uint32_t v)
>  static void
>  set_ctrl(E1000State *s, int index, uint32_t val)
>  {
> +    DBGOUT(IO, "set ctrl = %08x\n", val);
> +    if (val & E1000_CTRL_RST) {
> +        s->mac_reg[CTRL] = val;
> +        e1000_reset(s);
> +    }
>     /* RST is self clearing */
>     s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
>  }

This is still not right -- you want something like:

    if (val & E1000_CTRL_RST) {
        e1000_reset(s);
        return;
    }
    s->mac_reg[CTRL] = val;

-- PMM

Re: [Qemu-devel] [PATCH V2] e1000: Handle IO Port.

[Anthony PERARD]

On Thu, Jun 30, 2011 at 22:09, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 June 2011 20:28, Anthony PERARD <anthony.perard@citrix.com> wrote:
>> @@ -202,6 +201,11 @@ rxbufsize(uint32_t v)
>>  static void
>>  set_ctrl(E1000State *s, int index, uint32_t val)
>>  {
>> +    DBGOUT(IO, "set ctrl = %08x\n", val);
>> +    if (val & E1000_CTRL_RST) {
>> +        s->mac_reg[CTRL] = val;
>> +        e1000_reset(s);
>> +    }
>>     /* RST is self clearing */
>>     s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
>>  }
>
> This is still not right -- you want something like:

:(, sorry, I'll resend the patch with that.

>    if (val & E1000_CTRL_RST) {
>        e1000_reset(s);
>        return;
>    }
>    s->mac_reg[CTRL] = val;

Thanks,

-- 
Anthony PERARD