| Message ID | b93383e7883d0a3382ba39136072b7f22ae86143.1351157627.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 10/25/12 11:47, Peter Crosthwaite wrote: > Just put RAM regions in the unimplemented spaces in the MMIO region. These > regions have undefined behaviour, but this at least stops QEMU from segfaulting > when the guest bangs on these registers (and sucessfully fakes reading and > writing the registers with no side effects). Should not be needed, memory api should deal with that properly. Something is fishy somewhere. Maybe the dmacontext thing Peter Maydell noted for patch 5. cheers, Gerd
On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: > On 10/25/12 11:47, Peter Crosthwaite wrote: >> Just put RAM regions in the unimplemented spaces in the MMIO region. These >> regions have undefined behaviour, but this at least stops QEMU from segfaulting >> when the guest bangs on these registers (and sucessfully fakes reading and >> writing the registers with no side effects). > > Should not be needed, memory api should deal with that properly. CC Avi, Whats going on here is there is a container of size 0x1000 created with memory_region_init() and a handful of small subregions are populated. the container is then mapped to a 0x1000 size region of the system memory. What is supposed to happen when the guest access a region in the container for which no subregion has been added? For me it was a segfault, so i needed this patch for guest to proceed past accesses these undefined regions. > Something is fishy somewhere. Maybe the dmacontext thing Peter Maydell > noted for patch 5. > I think thats a separate issue. This is about the guest accessing the EHCI MMIO region not DMA. My implementation of P5 is functionality equivalent to Peters proposal. Just Peters idea will save me two lines of code and a memory leak :) Regards, Peter > cheers, > Gerd > >
On 25 October 2012 14:03, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: >> On 10/25/12 11:47, Peter Crosthwaite wrote: >>> Just put RAM regions in the unimplemented spaces in the MMIO region. These >>> regions have undefined behaviour, but this at least stops QEMU from segfaulting >>> when the guest bangs on these registers (and sucessfully fakes reading and >>> writing the registers with no side effects). >> >> Should not be needed, memory api should deal with that properly. > > CC Avi, > > Whats going on here is there is a container of size 0x1000 created > with memory_region_init() and a handful of small subregions are > populated. the container is then mapped to a 0x1000 size region of the > system memory. What is supposed to happen when the guest access a > region in the container for which no subregion has been added? For me > it was a segfault, so i needed this patch for guest to proceed past > accesses these undefined regions. (1) what does the hardware do? If the hardware device has (the equivalent of) an N bit address bus and treats accesses to unused addresses within the memory range defined by that bus width as RAZ/WI then that's what we should be modelling. If the hardware has real registers in this range we should also be modelling them (possibly as RAZ/WI with a LOG_UNIMP diagnostic). (2) what should the memory system do for accesses where there is no memory region? This is really system specific as it depends what the bus fabric does. For ARM the usual thing would be to generate a decode error response which will result in the guest CPU taking a data abort or prefetch abort. I don't think our memory system currently has any way of saying "for this access generate an exception"... (3) I don't think "qemu segfaults" is a good response to bad guest behaviour so that needs fixing somehow even if we can't model what the h/w does :-) -- PMM
On 10/25/2012 03:03 PM, Peter Crosthwaite wrote: > On Thu, Oct 25, 2012 at 10:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: >> On 10/25/12 11:47, Peter Crosthwaite wrote: >>> Just put RAM regions in the unimplemented spaces in the MMIO region. These >>> regions have undefined behaviour, but this at least stops QEMU from segfaulting >>> when the guest bangs on these registers (and sucessfully fakes reading and >>> writing the registers with no side effects). >> >> Should not be needed, memory api should deal with that properly. > > CC Avi, > > Whats going on here is there is a container of size 0x1000 created > with memory_region_init() and a handful of small subregions are > populated. the container is then mapped to a 0x1000 size region of the > system memory. What is supposed to happen when the guest access a > region in the container for which no subregion has been added? It falls back to the parent container. If there isn't one, something system-specific happens. You can override that by initializing your container with memory_region_init_io(); the callbacks will then receive any accesses which are not caught by any subregion.
On 10/25/2012 03:12 PM, Peter Maydell wrote: > (2) what should the memory system do for accesses where there is > no memory region? This is really system specific as it depends > what the bus fabric does. For ARM the usual thing would be to > generate a decode error response which will result in the guest > CPU taking a data abort or prefetch abort. I don't think our > memory system currently has any way of saying "for this access > generate an exception"... > You could easily have the top-level container have ->ops that generate an exception. > (3) I don't think "qemu segfaults" is a good response to bad > guest behaviour so that needs fixing somehow even if we can't > model what the h/w does :-) Right, a stack trace (or a patch) would be appreciated.
On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote: > On 10/25/2012 03:12 PM, Peter Maydell wrote: >> (2) what should the memory system do for accesses where there is >> no memory region? This is really system specific as it depends >> what the bus fabric does. For ARM the usual thing would be to >> generate a decode error response which will result in the guest >> CPU taking a data abort or prefetch abort. I don't think our >> memory system currently has any way of saying "for this access >> generate an exception"... >> > > You could easily have the top-level container have ->ops that generate > an exception. Ah, yes, there's an 'accepts' callback. (That's kind of awkward as an API because it means your decode logic gets spread between read, write and accept: there are some devices where it would be nice to have the 'default:' case of the address switch say "unknown offset, raise decode error". If the read callback took a uint64_t* rather than returning the read data, we could make both read and write return a success/decode-error type of status result.) -- PMM
On 10/25/2012 03:28 PM, Peter Maydell wrote: > On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote: >> On 10/25/2012 03:12 PM, Peter Maydell wrote: >>> (2) what should the memory system do for accesses where there is >>> no memory region? This is really system specific as it depends >>> what the bus fabric does. For ARM the usual thing would be to >>> generate a decode error response which will result in the guest >>> CPU taking a data abort or prefetch abort. I don't think our >>> memory system currently has any way of saying "for this access >>> generate an exception"... >>> >> >> You could easily have the top-level container have ->ops that generate >> an exception. > > Ah, yes, there's an 'accepts' callback. (That's kind of awkward > as an API because it means your decode logic gets spread between > read, write and accept: there are some devices where it would be > nice to have the 'default:' case of the address switch say "unknown > offset, raise decode error". If the read callback took a uint64_t* > rather than returning the read data, we could make both read and > write return a success/decode-error type of status result.) I actually forgot about ->accepts(). But it isn't needed for this use case, just have the lowest priority region (the container) implement ->read/write that generate the exception. If some other region decodes, the container region will be ignored, if nothing decodes, you get your exception. wrt decode duplication, I've been thinking of a single ->service() callback that accepts a Transaction argument, including all the details (offset, data, and direction). We may also do something like MemoryRegionPortio, but not hacky, that lets you have a MemoryRegion for each register. That means that instead of writing two large functions that duplicates the decode, you write one function per register, and switch on transfer direction.
On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote: > On 10/25/2012 03:28 PM, Peter Maydell wrote: >> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote: >>> You could easily have the top-level container have ->ops that generate >>> an exception. >> >> Ah, yes, there's an 'accepts' callback. (That's kind of awkward >> as an API because it means your decode logic gets spread between >> read, write and accept: there are some devices where it would be >> nice to have the 'default:' case of the address switch say "unknown >> offset, raise decode error". If the read callback took a uint64_t* >> rather than returning the read data, we could make both read and >> write return a success/decode-error type of status result.) > > I actually forgot about ->accepts(). But it isn't needed for this use > case, just have the lowest priority region (the container) implement > ->read/write that generate the exception I don't understand this -- read/write don't have any way of saying "please generate an exception". The only thing I can see in the API that does that is returning false from accepts(). > wrt decode duplication, I've been thinking of a single ->service() > callback that accepts a Transaction argument, including all the details > (offset, data, and direction). If we do this we should make sure that the Transaction allows us to include CPU-architecture dependent info -- for ARM we would want to model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv', 'instruction/data', etc. You also want to include in the transaction attributes who the master end of this transaction is (so a slave can distinguish accesses from a particular CPU core in a cluster, for instance). This would allow us to remove some of the current nasty hacks where devices reach into the CPUArchState to retrieve info that should ideally be modelled as part of the bus transaction. -- PMM
On 10/25/2012 03:50 PM, Peter Maydell wrote: > On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote: >> On 10/25/2012 03:28 PM, Peter Maydell wrote: >>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote: >>>> You could easily have the top-level container have ->ops that generate >>>> an exception. >>> >>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward >>> as an API because it means your decode logic gets spread between >>> read, write and accept: there are some devices where it would be >>> nice to have the 'default:' case of the address switch say "unknown >>> offset, raise decode error". If the read callback took a uint64_t* >>> rather than returning the read data, we could make both read and >>> write return a success/decode-error type of status result.) >> >> I actually forgot about ->accepts(). But it isn't needed for this use >> case, just have the lowest priority region (the container) implement >> ->read/write that generate the exception > > I don't understand this -- read/write don't have any way of saying > "please generate an exception". The only thing I can see in the > API that does that is returning false from accepts(). read/write can call anything. So if the SoC code installs the lowest region, it has access to whatever mechanisms generate the exceptions. (it may not have access to CPUState though). > >> wrt decode duplication, I've been thinking of a single ->service() >> callback that accepts a Transaction argument, including all the details >> (offset, data, and direction). > > If we do this we should make sure that the Transaction allows us to > include CPU-architecture dependent info -- for ARM we would want to > model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv', > 'instruction/data', etc. You also want to include in the transaction > attributes who the master end of this transaction is (so a slave > can distinguish accesses from a particular CPU core in a cluster, > for instance). This would allow us to remove some of the current > nasty hacks where devices reach into the CPUArchState to retrieve > info that should ideally be modelled as part of the bus transaction. Sounds like good arguments for another sweep.
On Thu, Oct 25, 2012 at 11:50 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote: >> On 10/25/2012 03:28 PM, Peter Maydell wrote: >>> On 25 October 2012 14:21, Avi Kivity <avi@redhat.com> wrote: >>>> You could easily have the top-level container have ->ops that generate >>>> an exception. >>> >>> Ah, yes, there's an 'accepts' callback. (That's kind of awkward >>> as an API because it means your decode logic gets spread between >>> read, write and accept: there are some devices where it would be >>> nice to have the 'default:' case of the address switch say "unknown >>> offset, raise decode error". If the read callback took a uint64_t* >>> rather than returning the read data, we could make both read and >>> write return a success/decode-error type of status result.) >> >> I actually forgot about ->accepts(). But it isn't needed for this use >> case, just have the lowest priority region (the container) implement >> ->read/write that generate the exception > > I don't understand this -- read/write don't have any way of saying > "please generate an exception". The only thing I can see in the > API that does that is returning false from accepts(). > >> wrt decode duplication, I've been thinking of a single ->service() >> callback that accepts a Transaction argument, including all the details >> (offset, data, and direction). > > If we do this we should make sure that the Transaction allows us to > include CPU-architecture dependent info -- for ARM we would want to > model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv', > 'instruction/data', etc. If we want these features shouldnt we just bite the bullet and impelement AXI with its own set of QOM definitions? Axi slaves then inherit from TYPE_AXI_SLAVE. If we want ARM specific features then we should replace SYSBUS with an ARM bus that does these things rather than push them up to the memory API. Add to that ARM bus feature list exclusive/non-exclusive as well. You also want to include in the transaction > attributes who the master end of this transaction is (so a slave > can distinguish accesses from a particular CPU core in a cluster, > for instance). This would allow us to remove some of the current > nasty hacks where devices reach into the CPUArchState to retrieve > info that should ideally be modelled as part of the bus transaction. > > -- PMM >
On 25 October 2012 14:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Thu, Oct 25, 2012 at 11:50 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 25 October 2012 14:41, Avi Kivity <avi@redhat.com> wrote: >>> wrt decode duplication, I've been thinking of a single ->service() >>> callback that accepts a Transaction argument, including all the details >>> (offset, data, and direction). >> >> If we do this we should make sure that the Transaction allows us to >> include CPU-architecture dependent info -- for ARM we would want to >> model transaction attributes like 'secure/nonsecure', 'privileged/nonpriv', >> 'instruction/data', etc. > > If we want these features shouldnt we just bite the bullet and > impelement AXI with its own set of QOM definitions? Axi slaves then > inherit from TYPE_AXI_SLAVE. If we want ARM specific features then we > should replace SYSBUS with an ARM bus that does these things rather > than push them up to the memory API. The memory API should provide the basic feature set that you can use to implement system specific buses and transaction signals with. We don't want to have reimplement basic support for reading and writing all over again. Also, we don't necesasrily want to implement AXI specifically; we need to implement the programmer-visible parts of it, but many of those carry over to AXI, APB, AHB... > Add to that ARM bus feature list exclusive/non-exclusive as well. Ah yes. We don't really model ARM exclusive transactions at all well anywhere in QEMU ; I think it all works more by luck than anything else at the moment... -- PMM
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index 78f9dfd..b6418bc 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -396,6 +396,8 @@ struct EHCIState { MemoryRegion mem_caps; MemoryRegion mem_opreg; MemoryRegion mem_ports; + MemoryRegion mem_other_low; + MemoryRegion mem_other_high; int companion_count; /* properties */ @@ -2773,17 +2775,27 @@ static void usb_ehci_initfn(EHCIState *s, DeviceState *dev) qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s); memory_region_init(&s->mem, "ehci", MMIO_SIZE); + if (s->capabase) { + memory_region_init_ram(&s->mem_other_low, "other-low", s->capabase); + } memory_region_init_io(&s->mem_caps, &ehci_mmio_caps_ops, s, "capabilities", s->opregbase); memory_region_init_io(&s->mem_opreg, &ehci_mmio_opreg_ops, s, "operational", PORTSC_BEGIN); memory_region_init_io(&s->mem_ports, &ehci_mmio_port_ops, s, "ports", PORTSC_END - PORTSC_BEGIN); + memory_region_init_ram(&s->mem_other_high, "other-high", MMIO_SIZE - + s->opregbase - (PORTSC_END - PORTSC_BEGIN)); + if (s->capabase) { + memory_region_add_subregion(&s->mem, 0, &s->mem_other_low); + } memory_region_add_subregion(&s->mem, s->capabase, &s->mem_caps); memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg); memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN, &s->mem_ports); + memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_END, + &s->mem_other_high); } static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
Just put RAM regions in the unimplemented spaces in the MMIO region. These regions have undefined behaviour, but this at least stops QEMU from segfaulting when the guest bangs on these registers (and sucessfully fakes reading and writing the registers with no side effects). Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- hw/usb/hcd-ehci.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)