Patchwork [1/7] Export the unassigned_mem read/write functions.

login
register
mail settings
Submitter Richard Henderson
Date July 23, 2011, 7:17 p.m.
Message ID <1311448659-17424-2-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/106475/
State New
Headers show

Comments

Richard Henderson - July 23, 2011, 7:17 p.m.
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 cpu-common.h |    7 +++++++
 exec.c       |   12 ++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)
Anthony Liguori - July 24, 2011, 1:28 p.m.
On 07/23/2011 02:17 PM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson<rth@twiddle.net>

Why?

Regards,

Anthony Liguori

> ---
>   cpu-common.h |    7 +++++++
>   exec.c       |   12 ++++++------
>   2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index 44b04b3..78e1bad 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -56,6 +56,13 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
>       cpu_register_physical_memory_offset(start_addr, size, phys_offset, 0);
>   }
>
> +extern CPUReadMemoryFunc unassigned_mem_readb;
> +extern CPUReadMemoryFunc unassigned_mem_readw;
> +extern CPUReadMemoryFunc unassigned_mem_readl;
> +extern CPUWriteMemoryFunc unassigned_mem_writeb;
> +extern CPUWriteMemoryFunc unassigned_mem_writew;
> +extern CPUWriteMemoryFunc unassigned_mem_writel;
> +
>   ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
>   ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
>                           ram_addr_t size, void *host);
> diff --git a/exec.c b/exec.c
> index 2160ded..c00badd 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3232,7 +3232,7 @@ ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>       return ram_addr;
>   }
>
> -static uint32_t unassigned_mem_readb(void *opaque, target_phys_addr_t addr)
> +uint32_t unassigned_mem_readb(void *opaque, target_phys_addr_t addr)
>   {
>   #ifdef DEBUG_UNASSIGNED
>       printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
> @@ -3243,7 +3243,7 @@ static uint32_t unassigned_mem_readb(void *opaque, target_phys_addr_t addr)
>       return 0;
>   }
>
> -static uint32_t unassigned_mem_readw(void *opaque, target_phys_addr_t addr)
> +uint32_t unassigned_mem_readw(void *opaque, target_phys_addr_t addr)
>   {
>   #ifdef DEBUG_UNASSIGNED
>       printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
> @@ -3254,7 +3254,7 @@ static uint32_t unassigned_mem_readw(void *opaque, target_phys_addr_t addr)
>       return 0;
>   }
>
> -static uint32_t unassigned_mem_readl(void *opaque, target_phys_addr_t addr)
> +uint32_t unassigned_mem_readl(void *opaque, target_phys_addr_t addr)
>   {
>   #ifdef DEBUG_UNASSIGNED
>       printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
> @@ -3265,7 +3265,7 @@ static uint32_t unassigned_mem_readl(void *opaque, target_phys_addr_t addr)
>       return 0;
>   }
>
> -static void unassigned_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +void unassigned_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
>   {
>   #ifdef DEBUG_UNASSIGNED
>       printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
> @@ -3275,7 +3275,7 @@ static void unassigned_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_
>   #endif
>   }
>
> -static void unassigned_mem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +void unassigned_mem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
>   {
>   #ifdef DEBUG_UNASSIGNED
>       printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
> @@ -3285,7 +3285,7 @@ static void unassigned_mem_writew(void *opaque, target_phys_addr_t addr, uint32_
>   #endif
>   }
>
> -static void unassigned_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +void unassigned_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>   {
>   #ifdef DEBUG_UNASSIGNED
>       printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
Richard Henderson - July 24, 2011, 4:42 p.m.
On 07/24/2011 06:28 AM, Anthony Liguori wrote:
> On 07/23/2011 02:17 PM, Richard Henderson wrote:
>> Signed-off-by: Richard Henderson<rth@twiddle.net>
> 
> Why?

So that I can write i/o functions like this:

  switch (addr) {
  case 0: ...
  case 64: ...
  case 128: ...
  ...
  default:
    unassigned_mem_readl(...)
  }

Perhaps Avi's rewrite makes this unnecessary; I browsed through
his patch set but didn't immediately see if there's a way for
the i/o function to return "failure".

What I certainly don't want to do is write this with 100 tiny
functions registering 8 bytes each, registered some tiny 
distance away from each other.


r~
Anthony Liguori - July 24, 2011, 6:56 p.m.
On 07/24/2011 11:42 AM, Richard Henderson wrote:
> On 07/24/2011 06:28 AM, Anthony Liguori wrote:
>> On 07/23/2011 02:17 PM, Richard Henderson wrote:
>>> Signed-off-by: Richard Henderson<rth@twiddle.net>
>>
>> Why?
>
> So that I can write i/o functions like this:
>
>    switch (addr) {
>    case 0: ...
>    case 64: ...
>    case 128: ...
>    ...
>    default:
>      unassigned_mem_readl(...)
>    }
>
> Perhaps Avi's rewrite makes this unnecessary; I browsed through
> his patch set but didn't immediately see if there's a way for
> the i/o function to return "failure".

What is returned by totally unregistered MMIO is defined by the chipset. 
  What's returned by an empty space in the MMIO space of a device is 
device specific.

What does your device return if there's an access at 32?

Regards,

Anthony Liguori

>
> What I certainly don't want to do is write this with 100 tiny
> functions registering 8 bytes each, registered some tiny
> distance away from each other.
>
>
> r~
>
Richard Henderson - July 24, 2011, 7 p.m.
On 07/24/2011 11:56 AM, Anthony Liguori wrote:
> What is returned by totally unregistered MMIO is defined by the
> chipset. What's returned by an empty space in the MMIO space of a
> device is device specific.

It's one and the same here, it's the chipset I'm implementing.

> What does your device return if there's an access at 32?

A machine check.


r~
Anthony Liguori - July 25, 2011, 1:47 a.m.
On 07/24/2011 02:00 PM, Richard Henderson wrote:
> On 07/24/2011 11:56 AM, Anthony Liguori wrote:
>> What is returned by totally unregistered MMIO is defined by the
>> chipset. What's returned by an empty space in the MMIO space of a
>> device is device specific.
>
> It's one and the same here, it's the chipset I'm implementing.

Sorry if its obvious, which patch in the series actually uses this?

Regards,

Anthony Liguori

>> What does your device return if there's an access at 32?
>
> A machine check.
>
>
> r~
>
Richard Henderson - July 25, 2011, 2:14 a.m.
On 07/24/2011 06:47 PM, Anthony Liguori wrote:
> On 07/24/2011 02:00 PM, Richard Henderson wrote:
>> On 07/24/2011 11:56 AM, Anthony Liguori wrote:
>>> What is returned by totally unregistered MMIO is defined by the
>>> chipset. What's returned by an empty space in the MMIO space of a
>>> device is device specific.
>>
>> It's one and the same here, it's the chipset I'm implementing.
> 
> Sorry if its obvious, which patch in the series actually uses this?

4/7, see hw/alpha_typhoon.c.


r~
Edgar Iglesias - Aug. 4, 2011, 11:58 p.m.
On Sun, Jul 24, 2011 at 01:56:00PM -0500, Anthony Liguori wrote:
> On 07/24/2011 11:42 AM, Richard Henderson wrote:
> >On 07/24/2011 06:28 AM, Anthony Liguori wrote:
> >>On 07/23/2011 02:17 PM, Richard Henderson wrote:
> >>>Signed-off-by: Richard Henderson<rth@twiddle.net>
> >>
> >>Why?
> >
> >So that I can write i/o functions like this:
> >
> >   switch (addr) {
> >   case 0: ...
> >   case 64: ...
> >   case 128: ...
> >   ...
> >   default:
> >     unassigned_mem_readl(...)
> >   }
> >
> >Perhaps Avi's rewrite makes this unnecessary; I browsed through
> >his patch set but didn't immediately see if there's a way for
> >the i/o function to return "failure".
> 
> What is returned by totally unregistered MMIO is defined by the
> chipset.  What's returned by an empty space in the MMIO space of a
> device is device specific.

Not really, IMO.

Im not going to say that HW does it this way or another because
different HW may do it different ways, but from my experience this
how most HW generally works:

Accesses to unmapped addresses are decoded, routed and finally handled
by the first node that realizes that the addr is unmapped. The action
might be to ignore or to signal some kind of error. AFAIK, most systems
will signal the error by sending an error back to the CPU via dedicated
control signals or by raising interrupts (the latter is very uncommon).

The common case is to signal an error via ctrl signals on the bus that
go back to the CPU and the final decision on what to do is made by the
CPU. QEMU currently doesnt model ctrl lines for bus accesses so entering
a per CPU xxx_unassigned_acceess function is pretty much in line with
most HW.

QEMU only models the data lanes so I think Richards patch is quite OK
because it brings the decission back to the CPU model.

I would prefer though, if the new Memory API would let devices pass
ctrl data back to the CPU in addition to the data lanes. That would
probably involve quite a bit of work though. CC:ing Avi, you might
have more input on this.

Cheers
Richard Henderson - Aug. 5, 2011, 3:39 p.m.
On 08/04/2011 04:58 PM, Edgar E. Iglesias wrote:
> QEMU only models the data lanes so I think Richards patch is quite OK
> because it brings the decission back to the CPU model.
> 
> I would prefer though, if the new Memory API would let devices pass
> ctrl data back to the CPU in addition to the data lanes. That would
> probably involve quite a bit of work though. CC:ing Avi, you might
> have more input on this.

Amusingly, after having updated my system to Avi's tree, I find
I don't need the unassigned_mem_read[bwl] helpers anymore, but
only the cpu_unassigned_access function, which is already exported.

It would be nice if the new api allowed signaling of errors, but
I can't think of a really nice way of doing that.


r~

Patch

diff --git a/cpu-common.h b/cpu-common.h
index 44b04b3..78e1bad 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -56,6 +56,13 @@  static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
     cpu_register_physical_memory_offset(start_addr, size, phys_offset, 0);
 }
 
+extern CPUReadMemoryFunc unassigned_mem_readb;
+extern CPUReadMemoryFunc unassigned_mem_readw;
+extern CPUReadMemoryFunc unassigned_mem_readl;
+extern CPUWriteMemoryFunc unassigned_mem_writeb;
+extern CPUWriteMemoryFunc unassigned_mem_writew;
+extern CPUWriteMemoryFunc unassigned_mem_writel;
+
 ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
 ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
                         ram_addr_t size, void *host);
diff --git a/exec.c b/exec.c
index 2160ded..c00badd 100644
--- a/exec.c
+++ b/exec.c
@@ -3232,7 +3232,7 @@  ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
     return ram_addr;
 }
 
-static uint32_t unassigned_mem_readb(void *opaque, target_phys_addr_t addr)
+uint32_t unassigned_mem_readb(void *opaque, target_phys_addr_t addr)
 {
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
@@ -3243,7 +3243,7 @@  static uint32_t unassigned_mem_readb(void *opaque, target_phys_addr_t addr)
     return 0;
 }
 
-static uint32_t unassigned_mem_readw(void *opaque, target_phys_addr_t addr)
+uint32_t unassigned_mem_readw(void *opaque, target_phys_addr_t addr)
 {
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
@@ -3254,7 +3254,7 @@  static uint32_t unassigned_mem_readw(void *opaque, target_phys_addr_t addr)
     return 0;
 }
 
-static uint32_t unassigned_mem_readl(void *opaque, target_phys_addr_t addr)
+uint32_t unassigned_mem_readl(void *opaque, target_phys_addr_t addr)
 {
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
@@ -3265,7 +3265,7 @@  static uint32_t unassigned_mem_readl(void *opaque, target_phys_addr_t addr)
     return 0;
 }
 
-static void unassigned_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
+void unassigned_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
@@ -3275,7 +3275,7 @@  static void unassigned_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_
 #endif
 }
 
-static void unassigned_mem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
+void unassigned_mem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);
@@ -3285,7 +3285,7 @@  static void unassigned_mem_writew(void *opaque, target_phys_addr_t addr, uint32_
 #endif
 }
 
-static void unassigned_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
+void unassigned_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem write " TARGET_FMT_plx " = 0x%x\n", addr, val);