| Message ID | 1311448659-17424-2-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
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);
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~
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~ >
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~
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~ >
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~
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
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~
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);
Signed-off-by: Richard Henderson <rth@twiddle.net> --- cpu-common.h | 7 +++++++ exec.c | 12 ++++++------ 2 files changed, 13 insertions(+), 6 deletions(-)