[Qemu-devel] [PATCH] rtl8139: Fix invalid IO access alignment

[Qemu-devel] [PATCH] rtl8139: Fix invalid IO access alignment

[Julian Pidancet]

This patch makes iPXE work with the rtl8139 emulation. The rtl8139
driver in iPXE issues a 16bit access on the ChipCmd register
(offset 0x37) to check the status of the rx buffer. The offset of the
ioport access was getting fixed up to 0x36 in qemu, causing the value
read in iPXE to be invalid.

This fixes an issue with iPXE reporting timeouts during TFTP transfers.

Signed-off-by: Julian Pidancet <julian.pidancet@gmail.com>
---
 hw/rtl8139.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 4c37993..0ff06da 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -1971,7 +1971,7 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
     cplus_tx_ring_desc += 16 * descriptor;
 
     DPRINTF("+++ C+ mode reading TX descriptor %d from host memory at "
-        "%08x0x%08x = 0x"DMA_ADDR_FMT"\n", descriptor, s->TxAddr[1],
+        "%08x %08x = 0x"DMA_ADDR_FMT"\n", descriptor, s->TxAddr[1],
         s->TxAddr[0], cplus_tx_ring_desc);
 
     uint32_t val, txdw0,txdw1,txbufLO,txbufHI;
@@ -2800,7 +2800,7 @@ static void rtl8139_io_writew(void *opaque, uint8_t addr, uint32_t val)
 {
     RTL8139State *s = opaque;
 
-    addr &= 0xfe;
+    addr &= 0xff;
 
     switch (addr)
     {
@@ -2900,7 +2900,7 @@ static void rtl8139_io_writel(void *opaque, uint8_t addr, uint32_t val)
 {
     RTL8139State *s = opaque;
 
-    addr &= 0xfc;
+    addr &= 0xff;
 
     switch (addr)
     {
@@ -3043,7 +3043,7 @@ static uint32_t rtl8139_io_readw(void *opaque, uint8_t addr)
     RTL8139State *s = opaque;
     uint32_t ret;
 
-    addr &= 0xfe; /* mask lower bit */
+    addr &= 0xff;
 
     switch (addr)
     {
@@ -3120,7 +3120,7 @@ static uint32_t rtl8139_io_readl(void *opaque, uint8_t addr)
     RTL8139State *s = opaque;
     uint32_t ret;
 
-    addr &= 0xfc; /* also mask low 2 bits */
+    addr &= 0xff;
 
     switch (addr)
     {
-- 
Julian Pidancet

Re: [Qemu-devel] [PATCH] rtl8139: Fix invalid IO access alignment

[Stefan Hajnoczi]

On Sun, Nov 13, 2011 at 6:13 PM, Julian Pidancet
<julian.pidancet@gmail.com> wrote:
> This patch makes iPXE work with the rtl8139 emulation. The rtl8139
> driver in iPXE issues a 16bit access on the ChipCmd register
> (offset 0x37) to check the status of the rx buffer. The offset of the
> ioport access was getting fixed up to 0x36 in qemu, causing the value
> read in iPXE to be invalid.
>
> This fixes an issue with iPXE reporting timeouts during TFTP transfers.
>
> Signed-off-by: Julian Pidancet <julian.pidancet@gmail.com>
> ---
>  hw/rtl8139.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)

I checked the datasheet and the register byte at 0x38, after ChipCmd,
is undocumented.  iPXE is being weird, I don't see the reason for the
inw().

> @@ -2800,7 +2800,7 @@ static void rtl8139_io_writew(void *opaque, uint8_t addr, uint32_t val)
>  {
>     RTL8139State *s = opaque;
>
> -    addr &= 0xfe;
> +    addr &= 0xff;

You can simply drop the masking since
rtl8139_ioport_readw()/rtl8139_mmio_readw() already do addr & 0xFF.
Same applies for the other hunks in this patch.

Stefan

Re: [Qemu-devel] [PATCH] rtl8139: Fix invalid IO access alignment

[Julian Pidancet]

On Mon, Nov 14, 2011 at 2:38 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sun, Nov 13, 2011 at 6:13 PM, Julian Pidancet
> <julian.pidancet@gmail.com> wrote:
>> This patch makes iPXE work with the rtl8139 emulation. The rtl8139
>> driver in iPXE issues a 16bit access on the ChipCmd register
>> (offset 0x37) to check the status of the rx buffer. The offset of the
>> ioport access was getting fixed up to 0x36 in qemu, causing the value
>> read in iPXE to be invalid.
>>
>> This fixes an issue with iPXE reporting timeouts during TFTP transfers.
>>
>> Signed-off-by: Julian Pidancet <julian.pidancet@gmail.com>
>> ---
>>  hw/rtl8139.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> I checked the datasheet and the register byte at 0x38, after ChipCmd,
> is undocumented.  iPXE is being weird, I don't see the reason for the
> inw().
>

I agree, changing this inw() to an inb() in iPXE also fixes the issue.
But this patch makes more sense because it reflects how real hardware
would behave.

>> @@ -2800,7 +2800,7 @@ static void rtl8139_io_writew(void *opaque, uint8_t addr, uint32_t val)
>>  {
>>     RTL8139State *s = opaque;
>>
>> -    addr &= 0xfe;
>> +    addr &= 0xff;
>
> You can simply drop the masking since
> rtl8139_ioport_readw()/rtl8139_mmio_readw() already do addr & 0xFF.
> Same applies for the other hunks in this patch.
>

Right, I will send an v2 shortly.

-- 
Julian

Re: [Qemu-devel] [PATCH] rtl8139: Fix invalid IO access alignment

[Stefan Hajnoczi]

On Mon, Nov 14, 2011 at 2:46 PM, Julian Pidancet
<julian.pidancet@gmail.com> wrote:
> On Mon, Nov 14, 2011 at 2:38 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Sun, Nov 13, 2011 at 6:13 PM, Julian Pidancet
>> <julian.pidancet@gmail.com> wrote:
>>> This patch makes iPXE work with the rtl8139 emulation. The rtl8139
>>> driver in iPXE issues a 16bit access on the ChipCmd register
>>> (offset 0x37) to check the status of the rx buffer. The offset of the
>>> ioport access was getting fixed up to 0x36 in qemu, causing the value
>>> read in iPXE to be invalid.
>>>
>>> This fixes an issue with iPXE reporting timeouts during TFTP transfers.
>>>
>>> Signed-off-by: Julian Pidancet <julian.pidancet@gmail.com>
>>> ---
>>>  hw/rtl8139.c |   10 +++++-----
>>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> I checked the datasheet and the register byte at 0x38, after ChipCmd,
>> is undocumented.  iPXE is being weird, I don't see the reason for the
>> inw().
>>
>
> I agree, changing this inw() to an inb() in iPXE also fixes the issue.
> But this patch makes more sense because it reflects how real hardware
> would behave.

Yep, we need to support real-world (weird) software.  Sending a patch
to iPXE is good, but QEMU should work with existing iPXE builds.

Stefan