[Qemu-devel] MMIO address changes

[Qemu-devel] MMIO address changes

[Paul Brook]

I've just committed a patch that changes the MMIO callback interface for 
devices.  Instead of being passed an absolute address these are now passed an 
offset from the start[1] of the memory region that was registered.

By itself this change has fairly neutral benefit, it just moves logic about. 
However it makes subsequent dynamic board configuration bits nicer, and is a 
step towards a proper bus level API.

Most of the groundwork for this is already there, from my earlier changes to 
separate ram and MMIO addresses TLB handling.

The main notable change it that the PhysPageDesc structure is not bigger. This 
isn't ideal, however l2_phys_map needs to go away anyway, so I'm not really 
worried about this.  

Some devices register their memory regions in multiple segments. To facilitate 
this I have added cpu_register_physical_memory_offset.

Most of the remaining changes are fairly mechanical tweaks to fix devices that 
explicitly compensated for the absolute address. Many devices are untouched 
because they ignore the high bits of the address.

I've tried to be fairly thorough with the changes, and tested what I can. 
However it's possible I missed or broke something, so please test your 
favourite targets.

Paul

[1] It's actually the offset from the start of the first page of that region. 
In practice this difference doesn't matter, and makes the implementation a 
lot simpler.

Re: [Qemu-devel] MMIO address changes

[takasi-y]

Hi,

> I've just committed a patch that changes the MMIO callback interface for 
> devices.  Instead of being passed an absolute address these are now passed an 
> offset from the start[1] of the memory region that was registered.
Good! It is just what I wanted.

> I've tried to be fairly thorough with the changes, and tested what I can. 
> However it's possible I missed or broke something, so please test your 
> favourite targets.
Mmmmm, sh/r2d is broken. I'll post a fix later.
But, before that, could you please tell me if this modification below
 is good and acceptable ?

diff --git a/exec.c b/exec.c
--- a/exec.c
+++ b/exec.c
@@ -2295,6 +2295,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_
t start_addr,
                 p->region_offset = 0;
             } else {
                 p->phys_offset = phys_offset;
+                p->region_offset = region_offset;
                 if ((phys_offset & ~TARGET_PAGE_MASK) <= IO_MEM_ROM ||
                     (phys_offset & IO_MEM_ROMD))
                     phys_offset += TARGET_PAGE_SIZE;

You can see the difference when you register a region overriding
 other(perhaps bigger) region.

hw/sh7750.c hits this by registering
 0xfc000000 .. 0xffffffff for CPU control registers,
then
 0xffe00000 .. 0xffe00028
 0xffe80000 .. 0xffe80028
 0xffd00000 .. 0xffd01000
 0xffd80000 .. 0xffd81000
 ...
for peripheral modules.

I can divide the first region to some pieces to avoid overlapping.
That is one solution, and is not bad to do (even without this issue), though.
Ah, in addition,
I'd like to know if registering overlapping region is valid or no.

> [1] It's actually the offset from the start of the first page of that region. 
> In practice this difference doesn't matter, and makes the implementation a 
> lot simpler.
Simpler is better.
But, I'd like to have a guide to write a code when it does matter.

Now, at least hw/sm501.c has been already broken by this case.
It registers palette register array at
 base + MMIO_BASE_OFFSET + SM501_DC + SM501_DC_PANEL_PALETTE,
 = 0x10000000 + 0x3e00000 + 0x080000 + 0x400
so, palette index offsets 0x400.

Of cource I can fix it by
 addr -= (MMIO_BASE_OFFSET + SM501_DC + SM501_DC_PANEL_PALETTE)&~TARGET_PAGE_MASK;
before accessing the register. Looks not so smart...
Are there any idea?

Cheers,
/yoshii

Re: [Qemu-devel] MMIO address changes

[Paul Brook]

> diff --git a/exec.c b/exec.c
> --- a/exec.c
> +++ b/exec.c
> @@ -2295,6 +2295,7 @@ void
> cpu_register_physical_memory_offset(target_phys_addr_ t start_addr,
>                  p->region_offset = 0;
>              } else {
>                  p->phys_offset = phys_offset;
> +                p->region_offset = region_offset;
>                  if ((phys_offset & ~TARGET_PAGE_MASK) <= IO_MEM_ROM ||
>                      (phys_offset & IO_MEM_ROMD))
>                      phys_offset += TARGET_PAGE_SIZE;

I guess this makes sense, however..

> You can see the difference when you register a region overriding
>  other(perhaps bigger) region.
>
> hw/sh7750.c hits this by registering
>  0xfc000000 .. 0xffffffff for CPU control registers,
> then
>  0xffe00000 .. 0xffe00028
>  0xffe80000 .. 0xffe80028
>  0xffd00000 .. 0xffd01000
>  0xffd80000 .. 0xffd81000
>  ...

You should not rely on overlapping regions doing anything sensible.

Paul

Re: [Qemu-devel] MMIO address changes

[Paul Brook]

> Now, at least hw/sm501.c has been already broken by this case.
> It registers palette register array at
>  base + MMIO_BASE_OFFSET + SM501_DC + SM501_DC_PANEL_PALETTE,
>  = 0x10000000 + 0x3e00000 + 0x080000 + 0x400
> so, palette index offsets 0x400.
>
> Of cource I can fix it by
>  addr -= (MMIO_BASE_OFFSET + SM501_DC +
> SM501_DC_PANEL_PALETTE)&~TARGET_PAGE_MASK; before accessing the register.
> Looks not so smart...
> Are there any idea?

Proably simpler to just use a single region for both.

Paul

[Qemu-devel] [PATCH] sh4: Followup to commit #5849 "Change MMIO callbacks..."

[takasi-y]

Thank you, Paul.

> You should not rely on overlapping regions doing anything sensible.
OK. fixed.

> Proably simpler to just use a single region for both.
Ah, that's right. Now the code is very simple (than I have expected).

Here is the patch. Please apply it.

Fixes to be needed for commit #5849 "Change MMIO callbacks..."
  hw/sh7750.c:
   - Divide region of CPU control registers to avoid overlapping
     to peripheral modules.
   - Delete unused var "icr", which had moved to hw/sh_intc.c.
  hw/sm501.c:
   - Merge non page aligned palette registers into the region of
     control registers.

Signed-off-by: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
---
 hw/sh7750.c |   14 ++++------
 hw/sm501.c  |   82 +++++++++++++++++++++++++---------------------------------
 2 files changed, 41 insertions(+), 55 deletions(-)

diff --git a/hw/sh7750.c b/hw/sh7750.c
index 1d18010..f44e522 100644
--- a/hw/sh7750.c
+++ b/hw/sh7750.c
@@ -58,7 +58,6 @@ typedef struct SH7750State {
     uint16_t periph_portdirb;	/* Direction seen from the peripherals */
     sh7750_io_device *devices[NB_DEVICES];	/* External peripherals */
 
-    uint16_t icr;
     /* Cache */
     uint32_t ccr;
 
@@ -218,8 +217,6 @@ static uint32_t sh7750_mem_readw(void *opaque, target_phys_addr_t addr)
 	return porta_lines(s);
     case SH7750_PDTRB_A7:
 	return portb_lines(s);
-    case 0x1fd00000:
-        return s->icr;
     default:
 	error_access("word read", addr);
 	assert(0);
@@ -313,9 +310,6 @@ static void sh7750_mem_writew(void *opaque, target_phys_addr_t addr,
 	    assert(0);
 	}
 	return;
-    case 0x1fd00000:
-        s->icr = mem_value;
-	return;
     default:
 	error_access("word write", addr);
 	assert(0);
@@ -643,8 +637,12 @@ SH7750State *sh7750_init(CPUSH4State * cpu)
     sh7750_io_memory = cpu_register_io_memory(0,
 					      sh7750_mem_read,
 					      sh7750_mem_write, s);
-    cpu_register_physical_memory_offset(0x1c000000, 0x04000000,
-                                        sh7750_io_memory, 0x1c000000);
+    cpu_register_physical_memory_offset(0x1f000000, 0x1000,
+                                        sh7750_io_memory, 0x1f000000);
+    cpu_register_physical_memory_offset(0x1f800000, 0x1000,
+                                        sh7750_io_memory, 0x1f800000);
+    cpu_register_physical_memory_offset(0x1fc00000, 0x1000,
+                                        sh7750_io_memory, 0x1fc00000);
 
     sh7750_mm_cache_and_tlb = cpu_register_io_memory(0,
 						     sh7750_mmct_read,
diff --git a/hw/sm501.c b/hw/sm501.c
index de61075..d496614 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -638,6 +638,32 @@ static CPUWriteMemoryFunc *sm501_system_config_writefn[] = {
     &sm501_system_config_write,
 };
 
+static uint32_t sm501_palette_read(void *opaque, target_phys_addr_t addr)
+{
+    SM501State * s = (SM501State *)opaque;
+    SM501_DPRINTF("sm501 palette read addr=%x\n", (int)addr);
+
+    /* TODO : consider BYTE/WORD access */
+    /* TODO : consider endian */
+
+    assert(0 <= addr && addr < 0x400 * 3);
+    return *(uint32_t*)&s->dc_palette[addr];
+}
+
+static void sm501_palette_write(void *opaque,
+				target_phys_addr_t addr, uint32_t value)
+{
+    SM501State * s = (SM501State *)opaque;
+    SM501_DPRINTF("sm501 palette write addr=%x, val=%x\n",
+		  (int)addr, value);
+
+    /* TODO : consider BYTE/WORD access */
+    /* TODO : consider endian */
+
+    assert(0 <= addr && addr < 0x400 * 3);
+    *(uint32_t*)&s->dc_palette[addr] = value;
+}
+
 static uint32_t sm501_disp_ctrl_read(void *opaque, target_phys_addr_t addr)
 {
     SM501State * s = (SM501State *)opaque;
@@ -719,6 +745,10 @@ static uint32_t sm501_disp_ctrl_read(void *opaque, target_phys_addr_t addr)
 	ret = s->dc_crt_hwc_addr;
 	break;
 
+    case SM501_DC_PANEL_PALETTE ... SM501_DC_PANEL_PALETTE + 0x400*3 - 4:
+        ret = sm501_palette_read(opaque, addr - SM501_DC_PANEL_PALETTE);
+	break;
+
     default:
 	printf("sm501 disp ctrl : not implemented register read."
 	       " addr=%x\n", (int)addr);
@@ -823,6 +853,10 @@ static void sm501_disp_ctrl_write(void *opaque,
 	s->dc_crt_hwc_addr = value & 0x0000FFFF;
 	break;
 
+    case SM501_DC_PANEL_PALETTE ... SM501_DC_PANEL_PALETTE + 0x400*3 - 4:
+        sm501_palette_write(opaque, addr - SM501_DC_PANEL_PALETTE, value);
+	break;
+
     default:
 	printf("sm501 disp ctrl : not implemented register write."
 	       " addr=%x, val=%x\n", (int)addr, value);
@@ -842,45 +876,6 @@ static CPUWriteMemoryFunc *sm501_disp_ctrl_writefn[] = {
     &sm501_disp_ctrl_write,
 };
 
-static uint32_t sm501_palette_read(void *opaque, target_phys_addr_t addr)
-{
-    SM501State * s = (SM501State *)opaque;
-    SM501_DPRINTF("sm501 palette read addr=%x\n", (int)addr);
-
-    /* TODO : consider BYTE/WORD access */
-    /* TODO : consider endian */
-
-    assert(0 <= addr && addr < 0x400 * 3);
-    return *(uint32_t*)&s->dc_palette[addr];
-}
-
-static void sm501_palette_write(void *opaque,
-				target_phys_addr_t addr, uint32_t value)
-{
-    SM501State * s = (SM501State *)opaque;
-    SM501_DPRINTF("sm501 palette write addr=%x, val=%x\n",
-		  (int)addr, value);
-
-    /* TODO : consider BYTE/WORD access */
-    /* TODO : consider endian */
-
-    assert(0 <= addr && addr < 0x400 * 3);
-    *(uint32_t*)&s->dc_palette[addr] = value;
-}
-
-static CPUReadMemoryFunc *sm501_palette_readfn[] = {
-    &sm501_palette_read,
-    &sm501_palette_read,
-    &sm501_palette_read,
-};
-
-static CPUWriteMemoryFunc *sm501_palette_writefn[] = {
-    &sm501_palette_write,
-    &sm501_palette_write,
-    &sm501_palette_write,
-};
-
-
 /* draw line functions for all console modes */
 
 #include "pixel_ops.h"
@@ -1070,7 +1065,6 @@ void sm501_init(DisplayState *ds, uint32_t base, unsigned long local_mem_base,
     SM501State * s;
     int sm501_system_config_index;
     int sm501_disp_ctrl_index;
-    int sm501_palette_index;
 
     /* allocate management data region */
     s = (SM501State *)qemu_mallocz(sizeof(SM501State));
@@ -1098,13 +1092,7 @@ void sm501_init(DisplayState *ds, uint32_t base, unsigned long local_mem_base,
     sm501_disp_ctrl_index = cpu_register_io_memory(0, sm501_disp_ctrl_readfn,
 						   sm501_disp_ctrl_writefn, s);
     cpu_register_physical_memory(base + MMIO_BASE_OFFSET + SM501_DC,
-				 0x400, sm501_disp_ctrl_index);
-
-    sm501_palette_index = cpu_register_io_memory(0, sm501_palette_readfn,
-						   sm501_palette_writefn, s);
-    cpu_register_physical_memory(base + MMIO_BASE_OFFSET
-				 + SM501_DC + SM501_DC_PANEL_PALETTE,
-				 0x400 * 3, sm501_palette_index);
+				 0x1000, sm501_disp_ctrl_index);
 
     /* bridge to serial emulation module */
     if (chr)
-- 
1.5.6.3

[Qemu-devel] Re: [PATCH] sh4: Followup to commit #5849 "Change MMIO callbacks..."

[Vladimir Prus]

takasi-y@ops.dti.ne.jp wrote:

> Thank you, Paul.
> 
>> You should not rely on overlapping regions doing anything sensible.
> OK. fixed.
> 
>> Proably simpler to just use a single region for both.
> Ah, that's right. Now the code is very simple (than I have expected).
> 
> Here is the patch. Please apply it.
> 
> Fixes to be needed for commit #5849 "Change MMIO callbacks..."
>   hw/sh7750.c:
>    - Divide region of CPU control registers to avoid overlapping
>      to peripheral modules.
>    - Delete unused var "icr", which had moved to hw/sh_intc.c.

Does sh_intc.c actually handle icr right now? Although I have a local
patch that makes it handle icr0 as well as LVLMODE, I don't see this
patch in SVN HEAD of qemu.

- Volodya

Re: [Qemu-devel] Re: [PATCH] sh4: Followup to commit #5849 "Change MMIO callbacks..."

[takasi-y]

> Does sh_intc.c actually handle icr right now? Although I have a local
> patch that makes it handle icr0 as well as LVLMODE, I don't see this
> patch in SVN HEAD of qemu.
Oops, I should have sent one more.
(I found your patch includes equivalent one, later).
Sorry for my careless behavior.
/yoshii

Re: [Qemu-devel] MMIO address changes

[Edgar E. Iglesias]

On Mon, Dec 01, 2008 at 06:59:35PM +0000, Paul Brook wrote:
> I've just committed a patch that changes the MMIO callback interface for 
> devices.  Instead of being passed an absolute address these are now passed an 
> offset from the start[1] of the memory region that was registered.
> 
> By itself this change has fairly neutral benefit, it just moves logic about. 
> However it makes subsequent dynamic board configuration bits nicer, and is a 
> step towards a proper bus level API.
> 
> Most of the groundwork for this is already there, from my earlier changes to 
> separate ram and MMIO addresses TLB handling.
> 
> The main notable change it that the PhysPageDesc structure is not bigger. This 
> isn't ideal, however l2_phys_map needs to go away anyway, so I'm not really 
> worried about this.  
> 
> Some devices register their memory regions in multiple segments. To facilitate 
> this I have added cpu_register_physical_memory_offset.
> 
> Most of the remaining changes are fairly mechanical tweaks to fix devices that 
> explicitly compensated for the absolute address. Many devices are untouched 
> because they ignore the high bits of the address.
> 
> I've tried to be fairly thorough with the changes, and tested what I can. 
> However it's possible I missed or broke something, so please test your 
> favourite targets.

FWIW the etrax-fs machine works OK although I am seeing a noticeable slow-down
after the patch.

Cheers


> 
> Paul
> 
> [1] It's actually the offset from the start of the first page of that region. 
> In practice this difference doesn't matter, and makes the implementation a 
> lot simpler.
> 

Re: [Qemu-devel] MMIO address changes

[Paul Brook]

> > I've tried to be fairly thorough with the changes, and tested what I can.
> > However it's possible I missed or broke something, so please test your
> > favourite targets.
>
> FWIW the etrax-fs machine works OK although I am seeing a noticeable
> slow-down after the patch.

Strange. I wouldn't expect significant slowdown.
AFAIK the hot-path code should be pretty much unchanged (if anything faster 
because the IO handlers don't need to adjust the address). Even the TLB fill 
handler overhead (tlb_set_page_exec) should be negligible.

Paul

Re: [Qemu-devel] MMIO address changes

[Edgar E. Iglesias]

On Wed, Dec 03, 2008 at 02:03:53PM +0000, Paul Brook wrote:
> > > I've tried to be fairly thorough with the changes, and tested what I can.
> > > However it's possible I missed or broke something, so please test your
> > > favourite targets.
> >
> > FWIW the etrax-fs machine works OK although I am seeing a noticeable
> > slow-down after the patch.
> 
> Strange. I wouldn't expect significant slowdown.
> AFAIK the hot-path code should be pretty much unchanged (if anything faster 
> because the IO handlers don't need to adjust the address). Even the TLB fill 
> handler overhead (tlb_set_page_exec) should be negligible.

Hello Paul,

I went through your patch and AFAICT it looks fine. The only thing I saw
that may cause slowdowns was the size increase of PageDesc and subpage.

In case anybody want's to reproduce this, just start the etrax image I've
posted before, login trough telnet and do a ls -alR /, you'll see a very
noticeable difference.

Best regards
Edgar

Re: [Qemu-devel] MMIO address changes

[Edgar E. Iglesias]

On Wed, Dec 03, 2008 at 01:17:45PM +0100, Edgar E. Iglesias wrote:
> On Mon, Dec 01, 2008 at 06:59:35PM +0000, Paul Brook wrote:
> > I've just committed a patch that changes the MMIO callback interface for 
> > devices.  Instead of being passed an absolute address these are now passed an 
> > offset from the start[1] of the memory region that was registered.
> > 
> > By itself this change has fairly neutral benefit, it just moves logic about. 
> > However it makes subsequent dynamic board configuration bits nicer, and is a 
> > step towards a proper bus level API.
> > 
> > Most of the groundwork for this is already there, from my earlier changes to 
> > separate ram and MMIO addresses TLB handling.
> > 
> > The main notable change it that the PhysPageDesc structure is not bigger. This 
> > isn't ideal, however l2_phys_map needs to go away anyway, so I'm not really 
> > worried about this.  
> > 
> > Some devices register their memory regions in multiple segments. To facilitate 
> > this I have added cpu_register_physical_memory_offset.
> > 
> > Most of the remaining changes are fairly mechanical tweaks to fix devices that 
> > explicitly compensated for the absolute address. Many devices are untouched 
> > because they ignore the high bits of the address.
> > 
> > I've tried to be fairly thorough with the changes, and tested what I can. 
> > However it's possible I missed or broke something, so please test your 
> > favourite targets.
> 
> FWIW the etrax-fs machine works OK although I am seeing a noticeable slow-down
> after the patch.

I tracked this down and the MMIO patch was for some reason triggering a bug
in the ETRAX models causing the slowdown. ETRAX ethernet/dma works fine
again.

Cheers

[Qemu-devel] [PATCH] usb-ohci: Add address masking.

[takasi-y]

Hi,
About this
> [1] It's actually the offset from the start of the first page of that region. 
> In practice this difference doesn't matter, and makes the implementation a 
> lot simpler.
again.

The patch below adds address mask(0xff) to mmio read/write funcs in usb-ohci.
I know now we are deleting this kind of address mask/subtract things.
But because of the restriction [1] above, we have to add address mask here.

The mmio change potentially breaks PCI device emulations which has mmio region
smaller than page size, like usb-ohci and rtl8139 (both have 256byte), because
PCI memory regions are configured to be aligned to it size, but to page.

usb-ohci is really broken now.
rtl8139 is OK because it already has address mask.
Others are not checked.

I think the most important thing is eliminating dependency to base address,
and small numbers of masks should be acceptable.

Regards,
/yoshii

This patch adds address mask to mmio read/write function.
Because its memory region is not always aligned to page.

Signed-off-by: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
---
 hw/usb-ohci.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 585f3a4..bfa7d77 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -1366,6 +1366,7 @@ static uint32_t ohci_mem_read(void *ptr, target_phys_addr_t addr)
         fprintf(stderr, "usb-ohci: Mis-aligned read\n");
         return 0xffffffff;
     }
+    addr &= 0xff;
 
     if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) {
         /* HcRhPortStatus */
@@ -1462,6 +1463,7 @@ static void ohci_mem_write(void *ptr, target_phys_addr_t addr, uint32_t val)
         fprintf(stderr, "usb-ohci: Mis-aligned write\n");
         return;
     }
+    addr &= 0xff;
 
     if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) {
         /* HcRhPortStatus */
-- 
1.5.6.3

Re: [Qemu-devel] [PATCH] usb-ohci: Add address masking.

[andrzej zaborowski]

2008/12/5  <takasi-y@ops.dti.ne.jp>:
> Hi,
> About this
>> [1] It's actually the offset from the start of the first page of that region.
>> In practice this difference doesn't matter, and makes the implementation a
>> lot simpler.
> again.
>
> The patch below adds address mask(0xff) to mmio read/write funcs in usb-ohci.
> I know now we are deleting this kind of address mask/subtract things.
> But because of the restriction [1] above, we have to add address mask here.
>
> The mmio change potentially breaks PCI device emulations which has mmio region
> smaller than page size, like usb-ohci and rtl8139 (both have 256byte), because
> PCI memory regions are configured to be aligned to it size, but to page.

I think it's more correct to substract an offset from the address
because masking causes the address to wrap and effectively the
registers become visible at multiple addresses.

On the other hand I don't see why we need the restriction [1].
Looking at exec.c, at least for io regions, it should be acceptable
for ->region_offset to be unaligned.

>
> usb-ohci is really broken now.
> rtl8139 is OK because it already has address mask.
> Others are not checked.

Cheers

Re: [Qemu-devel] [PATCH] usb-ohci: Add address masking.

[Paul Brook]

> On the other hand I don't see why we need the restriction [1].
> Looking at exec.c, at least for io regions, it should be acceptable
> for ->region_offset to be unaligned.

The low bits of env->iotlb are used for other purposes.

Paul

Re: [Qemu-devel] [PATCH] usb-ohci: Add address masking.

[Thomas Bandelier]

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

Hi,

Why are you writing "usb-ohci is *really* broken now." ??
Is it supposed to be working or not ?
On which status are you basing your affirmation?

Is it a good thing to completely break USB ohci in the development trunk?

Regards

Thomas




On Fri, Dec 5, 2008 at 2:52 PM, <takasi-y@ops.dti.ne.jp> wrote:

> Hi,
> About this
> > [1] It's actually the offset from the start of the first page of that
> region.
> > In practice this difference doesn't matter, and makes the implementation
> a
> > lot simpler.
> again.
>
> The patch below adds address mask(0xff) to mmio read/write funcs in
> usb-ohci.
> I know now we are deleting this kind of address mask/subtract things.
> But because of the restriction [1] above, we have to add address mask here.
>
> The mmio change potentially breaks PCI device emulations which has mmio
> region
> smaller than page size, like usb-ohci and rtl8139 (both have 256byte),
> because
> PCI memory regions are configured to be aligned to it size, but to page.
>
> usb-ohci is really broken now.
> rtl8139 is OK because it already has address mask.
> Others are not checked.
>
> I think the most important thing is eliminating dependency to base address,
> and small numbers of masks should be acceptable.
>
> Regards,
> /yoshii
>
> This patch adds address mask to mmio read/write function.
> Because its memory region is not always aligned to page.
>
> Signed-off-by: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
> ---
>  hw/usb-ohci.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
> index 585f3a4..bfa7d77 100644
> --- a/hw/usb-ohci.c
> +++ b/hw/usb-ohci.c
> @@ -1366,6 +1366,7 @@ static uint32_t ohci_mem_read(void *ptr,
> target_phys_addr_t addr)
>         fprintf(stderr, "usb-ohci: Mis-aligned read\n");
>         return 0xffffffff;
>     }
> +    addr &= 0xff;
>
>     if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) {
>         /* HcRhPortStatus */
> @@ -1462,6 +1463,7 @@ static void ohci_mem_write(void *ptr,
> target_phys_addr_t addr, uint32_t val)
>         fprintf(stderr, "usb-ohci: Mis-aligned write\n");
>         return;
>     }
> +    addr &= 0xff;
>
>     if (addr >= 0x54 && addr < 0x54 + ohci->num_ports * 4) {
>         /* HcRhPortStatus */
> --
> 1.5.6.3
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 3130 bytes --]

Re: [Qemu-devel] [PATCH] usb-ohci: Add address masking.

[takasi-y]

Ah, sorry, this question might be because of my bad choice of word....
# Which word was better, may be "actually" ?

> Why are you writing "usb-ohci is *really* broken now." ??
> Is it supposed to be working or not ?
It doesn't work when the memory region is not aligned to page. Otherwise works.

What I wanted to say by "really broken" was not that it was "broken beyond fix", 
but was
- Logically, the mmio change has a possibility to make usb-ohci not work.
- You can produce this condition on current code.
 (this part became the word "really")

r2d was the case(, though it was off-tree at that time).
hw/ppc* s are the same, because they initialize NICs first then usb-ohci.
 qemu -nic model,rtl8139 -usb ...
can produce the case.

I wrote it "potentially breaks" because
- r2d + linux was obviously in trouble, but it was off-tree code at that time.
- I didn't have evaluation environment ready for ppc. I checked them with
  only qemu-ppc without software on it.
- Whether it results in a problem or not depends on the software run on it,
  because PCI configuration is software task.

> Is it a good thing to completely break USB ohci in the development trunk?
No. This issue can be fixed by my patch (if masking is acceptable).
/yoshii

[Qemu-devel] Re: MMIO address changes

[Vladimir Prus]

Paul Brook wrote:

> I've just committed a patch that changes the MMIO callback interface for
> devices.  Instead of being passed an absolute address these are now passed an
> offset from the start[1] of the memory region that was registered.
...
> [1] It's actually the offset from the start of the first page of that region.
> In practice this difference doesn't matter, and makes the implementation a
> lot simpler.

This breaks something for me. I have:

         smc91c111_init(&nd_table[0], (0xb4000000 + 0x01800000 + 0x300) & 0x1fffffff,
                   get_irq(s, 7));

and smc91c111_init presently does:

        cpu_register_physical_memory(base, 16, iomemtype);

and smc emulation promptly dies because an access with address of 0x30e is made,
with 0x300 apparently being the offset from the page start, with 0xe being the desired
offset. What is the cleanest way to address this? Reverting your change to 
sms91c111.c works, fwiw.

- Volodya

Re: [Qemu-devel] Re: MMIO address changes

[andrzej zaborowski]

2008/12/11 Vladimir Prus <vladimir@codesourcery.com>:
> Paul Brook wrote:
>
>> I've just committed a patch that changes the MMIO callback interface for
>> devices.  Instead of being passed an absolute address these are now passed an
>> offset from the start[1] of the memory region that was registered.
> ...
>> [1] It's actually the offset from the start of the first page of that region.
>> In practice this difference doesn't matter, and makes the implementation a
>> lot simpler.
>
> This breaks something for me. I have:
>
>         smc91c111_init(&nd_table[0], (0xb4000000 + 0x01800000 + 0x300) & 0x1fffffff,
>                   get_irq(s, 7));
>
> and smc91c111_init presently does:
>
>        cpu_register_physical_memory(base, 16, iomemtype);
>
> and smc emulation promptly dies because an access with address of 0x30e is made,
> with 0x300 apparently being the offset from the page start, with 0xe being the desired
> offset. What is the cleanest way to address this? Reverting your change to
> sms91c111.c works, fwiw.

Masking the address with 15 should be acceptable for smc91c111.c I
think.  The other possibility is reverting the change to keep
subtracting the base address, but which would be masked with
TARGET_PAGE_MASK.

Cheers