Re: [Qemu-devel] Memory API: handling unassigned physical memory
On Tue, May 1, 2012 at 6:48 PM, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote: On 01/05/12 07:57, Blue Swirl wrote: Therefore I can't change it to my (modified) sbus_mmio_map() function because it would break other non-SPARC platforms, and AIUI there is nothing in the memory API that allows me to move a subregion to a different MemoryRegion parent, even if I can get a reference to it with sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I misunderstood something? Sysbus is used as a generic class for motherboard devices, there is an assumption that there is no higher level bus. What we need here is a full blown bus. The translations and mappigs between bus addresses and motherboard addresses should be done in a Sysbus to SBus bridge device, just like PCI host bridges do. Since SBus is mapped directly to physical addresses, is this mapping not just 1:1? Or does it make sense to re-work all the offsets of the various peripherals to be from the base address of the first slot? The mapping is not direct, from device point of view there's IOMMU in between and without help from IOMMU (or using the direct mode) the device can't access the full 36 bit address space. The offset should be from the start of the current slot, not first slot. ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On Wed, May 2, 2012 at 3:15 PM, Bob Breuer breu...@mc.net wrote: On 5/1/2012 1:48 PM, Mark Cave-Ayland wrote: On 01/05/12 07:57, Blue Swirl wrote: Therefore I can't change it to my (modified) sbus_mmio_map() function because it would break other non-SPARC platforms, and AIUI there is nothing in the memory API that allows me to move a subregion to a different MemoryRegion parent, even if I can get a reference to it with sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I misunderstood something? Sysbus is used as a generic class for motherboard devices, there is an assumption that there is no higher level bus. What we need here is a full blown bus. The translations and mappigs between bus addresses and motherboard addresses should be done in a Sysbus to SBus bridge device, just like PCI host bridges do. Since SBus is mapped directly to physical addresses, is this mapping not just 1:1? Or does it make sense to re-work all the offsets of the various peripherals to be from the base address of the first slot? It would be nice to have the device offsets be relative to the slot. User-pluggable sbus devices should be possible. Yes, currently all devices are fixed but for example display adapter should be pluggable and only on-board devices should be fixed. I've just pushed an update of a dbri sbus device model to github ( https://github.com/breuerr/qemu/commits/dbri-pre2 ). This device was built-in to at least the SS-20, but also available as an sbus add-in card (SunLink ISDN). There's an fcode rom so it can be probed by OBP, and if we could get something like -device SUNW,,DBRIe,slot=1 to work, then a user could add it to most of the sun4m machine models. Nice work. Bob
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 5/1/2012 1:48 PM, Mark Cave-Ayland wrote: On 01/05/12 07:57, Blue Swirl wrote: Therefore I can't change it to my (modified) sbus_mmio_map() function because it would break other non-SPARC platforms, and AIUI there is nothing in the memory API that allows me to move a subregion to a different MemoryRegion parent, even if I can get a reference to it with sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I misunderstood something? Sysbus is used as a generic class for motherboard devices, there is an assumption that there is no higher level bus. What we need here is a full blown bus. The translations and mappigs between bus addresses and motherboard addresses should be done in a Sysbus to SBus bridge device, just like PCI host bridges do. Since SBus is mapped directly to physical addresses, is this mapping not just 1:1? Or does it make sense to re-work all the offsets of the various peripherals to be from the base address of the first slot? It would be nice to have the device offsets be relative to the slot. User-pluggable sbus devices should be possible. I've just pushed an update of a dbri sbus device model to github ( https://github.com/breuerr/qemu/commits/dbri-pre2 ). This device was built-in to at least the SS-20, but also available as an sbus add-in card (SunLink ISDN). There's an fcode rom so it can be probed by OBP, and if we could get something like -device SUNW,,DBRIe,slot=1 to work, then a user could add it to most of the sun4m machine models. Bob
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On Mon, Apr 30, 2012 at 13:52, Mark Cave-Ayland
mark.cave-ayl...@ilande.co.uk wrote:
On 30/04/12 14:27, Peter Maydell wrote:
Hi Peter,
IMO the best fix is to unsysbus the device and qomify it instead. This
way we're 100% flexible in how we can attach it.
You don't need to wait for QOM to grow enough features to
replace sysbus. If you don't like what sysbus_mmio_map() does, you
Oh - so does this mean that QOM is not feature-complete?
At least 'Pin' class to replace GPIOs and IRQs is not in HEAD (but
Anthony already has a tree somewhere) and I can't remember what was
the plan for buses.
can always use sysbus_mmio_get_region() to get the MemoryRegion* and
then deal with it however you need to. This is the standard way
to deal with I have a sysbus device which I want to map into my
custom container object.
I already have code to do this for the sun4m-only hardware modelled on
sysbus_mmio_map() like this:
static void sbus_mmio_map(void *sbus, SysBusDevice *s, int n,
target_phys_addr_t addr)
{
MemoryRegion *sbus_mem, *mem;
target_phys_addr_t sbus_base;
SBusState *sbus_state = FROM_SYSBUS(SBusState, (SysBusDevice *)sbus);
sbus_mem = sysbus_mmio_get_region((SysBusDevice *)sbus, 0);
mem = sysbus_mmio_get_region(s, n);
/* SBus addresses are physical addresses, so subtract start of region */
sbus_base = sbus_state-base;
memory_region_add_subregion(sbus_mem, addr - sbus_base, mem);
}
The key problem is that this doesn't worked with shared peripherals, such as
the ESP device which is also used on various PPC Mac models as well as
SPARC. That's because its init function looks like this:
void esp_init(target_phys_addr_t espaddr, int it_shift,
ESPDMAMemoryReadWriteFunc dma_memory_read,
ESPDMAMemoryReadWriteFunc dma_memory_write,
void *dma_opaque, qemu_irq irq, qemu_irq *reset,
qemu_irq *dma_enable)
{
...
...
sysbus_mmio_map(s, 0, espaddr);
...
}
Therefore I can't change it to my (modified) sbus_mmio_map() function
because it would break other non-SPARC platforms, and AIUI there is nothing
in the memory API that allows me to move a subregion to a different
MemoryRegion parent, even if I can get a reference to it with
sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I
misunderstood something?
Sysbus is used as a generic class for motherboard devices, there is an
assumption that there is no higher level bus. What we need here is a
full blown bus. The translations and mappigs between bus addresses and
motherboard addresses should be done in a Sysbus to SBus bridge
device, just like PCI host bridges do.
ATB,
Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On Mon, Apr 30, 2012 at 14:33, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote: On 30/04/12 15:03, Peter Maydell wrote: Therefore I can't change it to my (modified) sbus_mmio_map() function because it would break other non-SPARC platforms, and AIUI there is nothing in the memory API that allows me to move a subregion to a different MemoryRegion parent, even if I can get a reference to it with sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I misunderstood something? Init functions like esp_init should be purely convenience functions which create, set properties on, and map the sysbus/qdev device. If they make use of private knowledge about the internals of the device then this is wrong and should be fixed. For esp_init() it looks like the handling of dma_memory_read, dma_memory_write, dma_opaque, it_shift and dma_enabled are wrong. If you fix that then you can just ignore the convenience function, and create, configure and map the device as appropriate for you. Right I think I'm starting to understand this now - in which case it becomes a matter of just copying a handful of lines within sun4m which is more bearable. In your view, would a suitable fix be to change dma_memory_read, dma_memory_write, dma_opaque, it_shift and dma_enabled to be qdev properties and modify esp_init() to return the qdev reference so they can be set by the caller? There's an ongoing work to introduce IOMMUs by changing how DMA work and this could simplify the DMA part. There's no clean way to use function pointers in qdev. For it_shift, a qdev or QOM property should be OK. The signal dma_enabled should be eventually replaced by a Pin. ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/30/2012 04:40 PM, Peter Maydell wrote: On 30 April 2012 14:36, Avi Kivity a...@redhat.com wrote: On 04/30/2012 04:27 PM, Peter Maydell wrote: On 30 April 2012 14:23, Avi Kivity a...@redhat.com wrote: IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. You don't need to wait for QOM to grow enough features to replace sysbus. If you don't like what sysbus_mmio_map() does, you can always use sysbus_mmio_get_region() to get the MemoryRegion* and then deal with it however you need to. This is the standard way to deal with I have a sysbus device which I want to map into my custom container object. I believe that API voids you warrantee. I wrote it for essentially the purpose described above :-) If you're the owner of the sysbus device in question then it's entirely fine as you are the one deciding whether to use the traditional map function or not. It's as good as we're going to get until QOM actually lets you export memory regions and pins, at which point we can just convert all the sysbus devices. Sure. But expect breakage if sysbus changes, for example dropping use of get_system_memory(). -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 13:39, Avi Kivity a...@redhat.com wrote: On 04/30/2012 04:40 PM, Peter Maydell wrote: I wrote it for essentially the purpose described above :-) If you're the owner of the sysbus device in question then it's entirely fine as you are the one deciding whether to use the traditional map function or not. It's as good as we're going to get until QOM actually lets you export memory regions and pins, at which point we can just convert all the sysbus devices. Sure. But expect breakage if sysbus changes, for example dropping use of get_system_memory(). That's OK, I'm happy to nak sysbus patches which break this use case :-) -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/30/2012 04:43 PM, Anthony Liguori wrote: On 04/30/2012 08:36 AM, Avi Kivity wrote: On 04/30/2012 04:27 PM, Peter Maydell wrote: On 30 April 2012 14:23, Avi Kivitya...@redhat.com wrote: IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. You don't need to wait for QOM to grow enough features to replace sysbus. If you don't like what sysbus_mmio_map() does, you can always use sysbus_mmio_get_region() to get the MemoryRegion* and then deal with it however you need to. This is the standard way to deal with I have a sysbus device which I want to map into my custom container object. I believe that API voids you warrantee. All that a QOM conversion would do is eliminate the use of sysbus and derive the object directly from DeviceState. Then, you would map the MemoryRegion exported by the device directly. So sysbus_mmio_get_region() seems like the right API to use. I think you're right. The real difference is that there is no longer an implied container region (which is just get_system_memory() in current sysbus). -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 03:41 PM, Peter Maydell wrote: On 1 May 2012 13:39, Avi Kivity a...@redhat.com wrote: On 04/30/2012 04:40 PM, Peter Maydell wrote: I wrote it for essentially the purpose described above :-) If you're the owner of the sysbus device in question then it's entirely fine as you are the one deciding whether to use the traditional map function or not. It's as good as we're going to get until QOM actually lets you export memory regions and pins, at which point we can just convert all the sysbus devices. Sure. But expect breakage if sysbus changes, for example dropping use of get_system_memory(). That's OK, I'm happy to nak sysbus patches which break this use case :-) sysbus should just die. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 13:42, Avi Kivity a...@redhat.com wrote: sysbus should just die. Totally agreed. It's not going to go quietly though... -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/30/2012 04:40 PM, Mark Cave-Ayland wrote: On 30/04/12 14:23, Avi Kivity wrote: Hi Avi, My understanding based upon this is that it would be impossible to register a different parent MemoryRegion without duplicating the init function for all shared devices which seems undesirable :( What are the requirements? You need a different catch-all mmio handler for each slot? Or would one catch-all mmio handler for all sysbus devices suffice? A single catch-all for all sysbus devices would suffice, however I'm thinking it makes sense to have one MemoryRegion per slot so that the devices can register onto the bus using their slot relative address to allow for potentially moving hardware between slots. Ok, so you have a hierarchy: bus - slot - devices in slot? That hierarchy is present in the hardware too, or that's how I interpret slot relative addresses. The only solution I can think of is to make sysbus_mmio_map() more intelligent so that instead of blindly adding the device to the root MemoryRegion, it traverses down the MemoryRegion hierarchy starting from the root to the furtherest leaf node it can find based upon the specified address and then adds the new subregion there. Hence if I add my SBus memory regions first before call the various peripheral init functions, everything should end up in the correct part of the memory tree. This solution attempts to reconstruct the memory hierarchy from the address, instead of maintaining it in the device tree. So I guess that is bad... Well, it's a lot of work - bad. I believe this should preserve compatibility for existing sysbus API users with just a single root MemoryRegion, however due to lack of any documentation related to sysbus I'm not sure if this would break other platforms or maybe even violate one of its key design features? IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. That's interesting - I didn't realise that sysbus is a legacy interface with respect to QOM. Maybe it's just wishful thinking on my part. But I always saw sysbus as a wrong interface - it corresponds to no real bus and doesn't allow hierarchy, unlike our pci or even isa implementations. Is there any documentation related to this? Then again, converting all of the devices over to QOM and testing that it doesn't break all platforms/busses suddenly becomes a huge job... You can just follow Peter's suggestion, although qomification would be preferable IMO. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 03:43 PM, Peter Maydell wrote: On 1 May 2012 13:42, Avi Kivity a...@redhat.com wrote: sysbus should just die. Totally agreed. It's not going to go quietly though... Not if you keep suggesting workarounds when I tell unsuspecting developers to qomify their devices. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 13:48, Avi Kivity a...@redhat.com wrote: On 05/01/2012 03:43 PM, Peter Maydell wrote: On 1 May 2012 13:42, Avi Kivity a...@redhat.com wrote: sysbus should just die. Totally agreed. It's not going to go quietly though... Not if you keep suggesting workarounds when I tell unsuspecting developers to qomify their devices. When QOM supports (1) exporting gpio signals and (2) exporting memory regions, then it will be providing the main things that sysbus provides. (Sysbus today is essentially I need a device that exports some memory regions and some I/O pins.) At that point it will be sensible to say convert your sysbus devices to QOM. Until QOM is actually able to provide the functionality, it's not a workable replacement. -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 03:49 PM, Peter Maydell wrote: On 1 May 2012 13:48, Avi Kivity a...@redhat.com wrote: On 05/01/2012 03:43 PM, Peter Maydell wrote: On 1 May 2012 13:42, Avi Kivity a...@redhat.com wrote: sysbus should just die. Totally agreed. It's not going to go quietly though... Not if you keep suggesting workarounds when I tell unsuspecting developers to qomify their devices. When QOM supports (1) exporting gpio signals and (2) exporting memory regions, then it will be providing the main things that sysbus provides. (Sysbus today is essentially I need a device that exports some memory regions and some I/O pins.) At that point it will be sensible to say convert your sysbus devices to QOM. Until QOM is actually able to provide the functionality, it's not a workable replacement. Doesn't propertyMemoryRegion (or however it's phrased) provide the functionality for memory? But I agree. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 08:01 AM, Avi Kivity wrote: On 05/01/2012 03:49 PM, Peter Maydell wrote: On 1 May 2012 13:48, Avi Kivitya...@redhat.com wrote: On 05/01/2012 03:43 PM, Peter Maydell wrote: On 1 May 2012 13:42, Avi Kivitya...@redhat.com wrote: sysbus should just die. Totally agreed. It's not going to go quietly though... Not if you keep suggesting workarounds when I tell unsuspecting developers to qomify their devices. When QOM supports (1) exporting gpio signals and This is trivial. It'll come in as soon as 1.2 opens up. If folks want to start working on a branch with it: https://github.com/aliguori/qemu/tree/qom-pin.4 (2) exporting memory regions, then it will be providing the main things that sysbus provides. This is a little tricky. Here's the problems I've encountered so far: a) A lot of devices need the equivalent of it_shift. it_shift affects how addresses are decoded and the size of the memory region. it_shift usually needs to be a device property. Since we need to know the size of the memory region to initialize it, we need to know the value of it_shift before we can initialize it which means we have to delay the initialization of the mmemory region until realize. I think a nice fix would be to make it_shift as memory region mutator and allow it to be set after initialization. b) There's some duplication in MemoryRegions with respect to QOM. Memory regions want to have a name but with QOM they'll be addressable via a path. I go back and forth about how aggressively we want to refactor MemoryRegions. If someone wants to spend some time converting MemoryRegion to QOM, that would certainly be helpful. So far, I've been avoiding it but we'll eventually need to do it. Regards, Anthony Liguori
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 01:57 AM, Blue Swirl wrote: On Mon, Apr 30, 2012 at 13:52, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote: On 30/04/12 14:27, Peter Maydell wrote: Hi Peter, IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. You don't need to wait for QOM to grow enough features to replace sysbus. If you don't like what sysbus_mmio_map() does, you Oh - so does this mean that QOM is not feature-complete? At least 'Pin' class to replace GPIOs and IRQs is not in HEAD (but Anthony already has a tree somewhere) and I can't remember what was the plan for buses. I'm planning on pushing bus support in before I freeze 1.1. Regards, Anthony Liguori
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 14:50, Anthony Liguori anth...@codemonkey.ws wrote: On 05/01/2012 03:49 PM, Peter Maydell wrote: When QOM supports (1) exporting gpio signals and This is trivial. It'll come in as soon as 1.2 opens up. You need to get it through code review first... (just as a random example, this commit: https://github.com/aliguori/qemu/commit/46483049813fca2c23fb7cd04eeab89905500c4c is going down the wrong path because it's incorrectly introducing knowledge of device internals to the utility i8259_init() function.) -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 09:00 AM, Peter Maydell wrote:
On 1 May 2012 14:50, Anthony Liguorianth...@codemonkey.ws wrote:
On 05/01/2012 03:49 PM, Peter Maydell wrote:
When QOM supports (1) exporting gpio signals and
This is trivial. It'll come in as soon as 1.2 opens up.
You need to get it through code review first...
Obviously.
(just as a random example, this commit:
https://github.com/aliguori/qemu/commit/46483049813fca2c23fb7cd04eeab89905500c4c
is going down the wrong path because it's incorrectly introducing
knowledge of device internals to the utility i8259_init()
function.)
Eh?
Do you mean:
-qdev_connect_gpio_out(dev-qdev, 0, irq_set[2]);
+pin_connect_qemu_irq(s-int_out[0], irq_set[2]);
There are three ways to do this:
1) pin_connect_qemu_irq(i8259_get_int_out(s), irq_set[2]);
2) pin_connect_qemu_irq(s-int_out[0], irq_set[2]);
3) pin_connect_qemu_irq(PIN(object_get_child(s, int_out[0])), irq_set[2]);
Having converted a lot of devices by hand, I prefer (2). I'd like to move to a
model of:
typedef struct PICDevice
{
DeviceState parent;
Pin int_out[0];
MemoryRegion io;
/* private */
PICState *priv;
} PICDevice;
This let's us use (2), while still embedding devices and keeping the gory state
details private to the implementation.
Regards,
Anthony Liguori
-- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 04:50 PM, Anthony Liguori wrote: On 05/01/2012 08:01 AM, Avi Kivity wrote: On 05/01/2012 03:49 PM, Peter Maydell wrote: On 1 May 2012 13:48, Avi Kivitya...@redhat.com wrote: On 05/01/2012 03:43 PM, Peter Maydell wrote: On 1 May 2012 13:42, Avi Kivitya...@redhat.com wrote: sysbus should just die. Totally agreed. It's not going to go quietly though... Not if you keep suggesting workarounds when I tell unsuspecting developers to qomify their devices. When QOM supports (1) exporting gpio signals and This is trivial. It'll come in as soon as 1.2 opens up. If folks want to start working on a branch with it: https://github.com/aliguori/qemu/tree/qom-pin.4 (2) exporting memory regions, then it will be providing the main things that sysbus provides. This is a little tricky. Here's the problems I've encountered so far: a) A lot of devices need the equivalent of it_shift. it_shift affects how addresses are decoded and the size of the memory region. it_shift usually needs to be a device property. Since we need to know the size of the memory region to initialize it, we need to know the value of it_shift before we can initialize it which means we have to delay the initialization of the mmemory region until realize. I think a nice fix would be to make it_shift as memory region mutator and allow it to be set after initialization. Indeed I wanted to make it_shift as part of the memory core. But a mutator? It doesn't change in real hardware, so this looks artificial. So far all mutators really change at runtime. What is the problem with delaying region initialization until realize? b) There's some duplication in MemoryRegions with respect to QOM. Memory regions want to have a name but with QOM they'll be addressable via a path. I go back and forth about how aggressively we want to refactor MemoryRegions. These days region names are purely for debugging. The ABI bit was moved to a separate function. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 09:09 AM, Avi Kivity wrote: On 05/01/2012 04:50 PM, Anthony Liguori wrote: On 05/01/2012 08:01 AM, Avi Kivity wrote: On 05/01/2012 03:49 PM, Peter Maydell wrote: On 1 May 2012 13:48, Avi Kivitya...@redhat.com wrote: On 05/01/2012 03:43 PM, Peter Maydell wrote: On 1 May 2012 13:42, Avi Kivitya...@redhat.com wrote: sysbus should just die. Totally agreed. It's not going to go quietly though... Not if you keep suggesting workarounds when I tell unsuspecting developers to qomify their devices. When QOM supports (1) exporting gpio signals and This is trivial. It'll come in as soon as 1.2 opens up. If folks want to start working on a branch with it: https://github.com/aliguori/qemu/tree/qom-pin.4 (2) exporting memory regions, then it will be providing the main things that sysbus provides. This is a little tricky. Here's the problems I've encountered so far: a) A lot of devices need the equivalent of it_shift. it_shift affects how addresses are decoded and the size of the memory region. it_shift usually needs to be a device property. Since we need to know the size of the memory region to initialize it, we need to know the value of it_shift before we can initialize it which means we have to delay the initialization of the mmemory region until realize. I think a nice fix would be to make it_shift as memory region mutator and allow it to be set after initialization. Indeed I wanted to make it_shift as part of the memory core. But a mutator? It doesn't change in real hardware, so this looks artificial. So far all mutators really change at runtime. QOM has a concept of initialization and realize. You can change properties after initialization but not before realize. So as long as it_shift can be set after initialization but before realize (which I think is roughly memory_region_add_subregion) it doesn't need to be a mutator. What is the problem with delaying region initialization until realize? We need to initialize the MemoryRegion in order to expose it as a property. We want to do that during initialize. Here's an example: qom-create isa-i8259 foo qom-set /peripheral/foo/io it_shift 1 qom-set /peripheral/foo/realize true For this to work, it_shift needs to be a QOM property of the io MemoryRegion. The MemoryRegion needs to be created in instance_init. b) There's some duplication in MemoryRegions with respect to QOM. Memory regions want to have a name but with QOM they'll be addressable via a path. I go back and forth about how aggressively we want to refactor MemoryRegions. These days region names are purely for debugging. The ABI bit was moved to a separate function. Fair enough. BTW, in the branch I've posted, I've got a number of memory API conversions or removal of legacy interfaces. Regards, Anthony Liguori
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 15:06, Anthony Liguori anth...@codemonkey.ws wrote: Do you mean: - qdev_connect_gpio_out(dev-qdev, 0, irq_set[2]); + pin_connect_qemu_irq(s-int_out[0], irq_set[2]); There are three ways to do this: 1) pin_connect_qemu_irq(i8259_get_int_out(s), irq_set[2]); No good unless you're autogenerating all those accessor functions. 2) pin_connect_qemu_irq(s-int_out[0], irq_set[2]); Exposes the whole of the struct (including all the private memmbers) to anything that wants to use it. As you say later on in the email, this requires splitting everything up into two structs, one for publicly accessible and one for private implementation...which your series doesn't seem to do at the moment. 3) pin_connect_qemu_irq(PIN(object_get_child(s, int_out[0])), irq_set[2]); This is a bit verbose. Something more like pin_connect(s, int_out[0], self, int_set[2]); would be a bit less longwinded. Or even connect(TYPE_PIN, s, int_out[0], self, int_set[2]); (No, this doesn't do compile time type checking. I don't think that's a problem particularly, or at least not enough of one to justify not doing it this way. The object model we have is dynamic and does things at runtime...) -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 05:15 PM, Anthony Liguori wrote: I think a nice fix would be to make it_shift as memory region mutator and allow it to be set after initialization. Indeed I wanted to make it_shift as part of the memory core. But a mutator? It doesn't change in real hardware, so this looks artificial. So far all mutators really change at runtime. QOM has a concept of initialization and realize. You can change properties after initialization but not before realize. So as long as it_shift can be set after initialization but before realize (which I think is roughly memory_region_add_subregion) it doesn't need to be a mutator. Ok, good. What is the problem with delaying region initialization until realize? We need to initialize the MemoryRegion in order to expose it as a property. We want to do that during initialize. Here's an example: qom-create isa-i8259 foo qom-set /peripheral/foo/io it_shift 1 qom-set /peripheral/foo/realize true For this to work, it_shift needs to be a QOM property of the io MemoryRegion. The MemoryRegion needs to be created in instance_init. So it looks like we need two phase initialization for memory regions as well? Not so pretty. b) There's some duplication in MemoryRegions with respect to QOM. Memory regions want to have a name but with QOM they'll be addressable via a path. I go back and forth about how aggressively we want to refactor MemoryRegions. These days region names are purely for debugging. The ABI bit was moved to a separate function. Fair enough. BTW, in the branch I've posted, I've got a number of memory API conversions or removal of legacy interfaces. Nice. But you use get_system_io(), which is bad. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 09:20 AM, Peter Maydell wrote:
On 1 May 2012 15:06, Anthony Liguorianth...@codemonkey.ws wrote:
Do you mean:
-qdev_connect_gpio_out(dev-qdev, 0, irq_set[2]);
+pin_connect_qemu_irq(s-int_out[0], irq_set[2]);
There are three ways to do this:
1) pin_connect_qemu_irq(i8259_get_int_out(s), irq_set[2]);
No good unless you're autogenerating all those accessor functions.
2) pin_connect_qemu_irq(s-int_out[0], irq_set[2]);
Exposes the whole of the struct (including all the private
memmbers) to anything that wants to use it. As you say
later on in the email, this requires splitting everything up
into two structs, one for publicly accessible and one for
private implementation...which your series doesn't seem
to do at the moment.
3) pin_connect_qemu_irq(PIN(object_get_child(s, int_out[0])), irq_set[2]);
This is a bit verbose. Something more like
pin_connect(s, int_out[0], self, int_set[2]);
This is incorrect, it would have to be:
Error *err = NULL;
if (pin_connect(s, in_out[0], self, int_set[2], err)) {
error_propagate(errp, err);
return;
}
would be a bit less longwinded. Or even
connect(TYPE_PIN, s, int_out[0], self, int_set[2]);
Not checking for failure is not an option.
(No, this doesn't do compile time type checking. I don't
think that's a problem particularly, or at least not enough
of one to justify not doing it this way. The object model we
have is dynamic and does things at runtime...)
Correctness is more important to me than brevity.
And really, we should focus on killing things like i8259_init(). These types of
functions should go away in favor of proper modeling of SoCs/SuperIO chips.
Regards,
Anthony Liguori
-- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 09:26 AM, Avi Kivity wrote: On 05/01/2012 05:15 PM, Anthony Liguori wrote: I think a nice fix would be to make it_shift as memory region mutator and allow it to be set after initialization. Indeed I wanted to make it_shift as part of the memory core. But a mutator? It doesn't change in real hardware, so this looks artificial. So far all mutators really change at runtime. QOM has a concept of initialization and realize. You can change properties after initialization but not before realize. So as long as it_shift can be set after initialization but before realize (which I think is roughly memory_region_add_subregion) it doesn't need to be a mutator. Ok, good. What is the problem with delaying region initialization until realize? We need to initialize the MemoryRegion in order to expose it as a property. We want to do that during initialize. Here's an example: qom-create isa-i8259 foo qom-set /peripheral/foo/io it_shift 1 qom-set /peripheral/foo/realize true For this to work, it_shift needs to be a QOM property of the io MemoryRegion. The MemoryRegion needs to be created in instance_init. So it looks like we need two phase initialization for memory regions as well? Yes, exactly. Converting MemoryRegion to QOM will give it a two phase initialization FWIW. Not so pretty. It's unavoidable because we're dealing with a graph. The connections between objects may form loops which effectively means that we have dependency loops. That means we need to be able to create all objects first and then establish the links between them. This is why we need two stage initialization. b) There's some duplication in MemoryRegions with respect to QOM. Memory regions want to have a name but with QOM they'll be addressable via a path. I go back and forth about how aggressively we want to refactor MemoryRegions. These days region names are purely for debugging. The ABI bit was moved to a separate function. Fair enough. BTW, in the branch I've posted, I've got a number of memory API conversions or removal of legacy interfaces. Nice. But you use get_system_io(), which is bad. Only in the device setup code. That will all eventually get moved into a SuperIO chip. The PC code needs massive amounts of refactoring. Regards, Anthony Liguori
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 16:09, Anthony Liguori anth...@codemonkey.ws wrote:
On 05/01/2012 09:20 AM, Peter Maydell wrote:
This is a bit verbose. Something more like
pin_connect(s, int_out[0], self, int_set[2]);
This is incorrect, it would have to be:
Error *err = NULL;
if (pin_connect(s, in_out[0], self, int_set[2], err)) {
error_propagate(errp, err);
return;
}
would be a bit less longwinded. Or even
connect(TYPE_PIN, s, int_out[0], self, int_set[2]);
Not checking for failure is not an option.
The assumption is that failure to connect is a fatal error,
and we can happily just assert()/hw_error()/etc.
(Obviously we would provide APIs for the occasional use case
that really did want to do connect if possible and do something
else in case of failure, but that's definitely not the
common case.)
(No, this doesn't do compile time type checking. I don't
think that's a problem particularly, or at least not enough
of one to justify not doing it this way. The object model we
have is dynamic and does things at runtime...)
Correctness is more important to me than brevity.
And really, we should focus on killing things like i8259_init().
Functions like i8259_init() exist precisely because
QOM/qdev don't provide brevity and people trying to
use these devices do in fact value brevity. That's why
I want the standard native connect this thing to this
other thing function to be short and simple.
-- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 05/01/2012 10:20 AM, Peter Maydell wrote:
On 1 May 2012 16:09, Anthony Liguorianth...@codemonkey.ws wrote:
On 05/01/2012 09:20 AM, Peter Maydell wrote:
This is a bit verbose. Something more like
pin_connect(s, int_out[0], self, int_set[2]);
This is incorrect, it would have to be:
Error *err = NULL;
if (pin_connect(s, in_out[0], self, int_set[2],err)) {
error_propagate(errp, err);
return;
}
would be a bit less longwinded. Or even
connect(TYPE_PIN, s, int_out[0], self, int_set[2]);
Not checking for failure is not an option.
The assumption is that failure to connect is a fatal error,
and we can happily just assert()/hw_error()/etc.
So that means that we have a bug from someone miss-typing, instead of your
hotplug attempt failing with an error, your entire guest is destroyed. That
doesn't sound very nice to me.
Device initialization should never exit() (unless you really hit a fatal error
like OOM).
(No, this doesn't do compile time type checking. I don't
think that's a problem particularly, or at least not enough
of one to justify not doing it this way. The object model we
have is dynamic and does things at runtime...)
Correctness is more important to me than brevity.
And really, we should focus on killing things like i8259_init().
Functions like i8259_init() exist precisely because
QOM/qdev don't provide brevity and people trying to
use these devices do in fact value brevity.
No, they exist because we aren't modeling correctly.
i8259_init() is doing a few different things at once. Once you split things
between init and realize, you no longer have long chunks of redundant code.
That's why
I want the standard native connect this thing to this
other thing function to be short and simple.
With my previous proposal, it's just:
s-irq_in = b-int_out[0];
This is why I like exposing public members in structures. It's brief and safe.
Regards,
Anthony Liguori
-- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 16:26, Anthony Liguori anth...@codemonkey.ws wrote: On 05/01/2012 10:20 AM, Peter Maydell wrote: The assumption is that failure to connect is a fatal error, and we can happily just assert()/hw_error()/etc. So that means that we have a bug from someone miss-typing, instead of your hotplug attempt failing with an error, your entire guest is destroyed. That doesn't sound very nice to me. If you're trying to hotplug a buggily implemented device then all bets are off, really. Device initialization should never exit() (unless you really hit a fatal error like OOM). (No, this doesn't do compile time type checking. I don't think that's a problem particularly, or at least not enough of one to justify not doing it this way. The object model we have is dynamic and does things at runtime...) Correctness is more important to me than brevity. And really, we should focus on killing things like i8259_init(). Functions like i8259_init() exist precisely because QOM/qdev don't provide brevity and people trying to use these devices do in fact value brevity. No, they exist because we aren't modeling correctly. Most of them are simply convenience functions that just do create, configure, wire up, init. (The ones that do more than that need fixing anyway.) i8259_init() is doing a few different things at once. Once you split things between init and realize, you no longer have long chunks of redundant code. ...you have redundant code scattered in multiple places instead? That's why I want the standard native connect this thing to this other thing function to be short and simple. With my previous proposal, it's just: s-irq_in = b-int_out[0]; This is why I like exposing public members in structures. It's brief and safe. Obvious question: why isn't property setting done this way, then? Surely it's equally brief and safe to say cpu-level = def_level; rather than object_property_set_int(OBJECT(cpu), def-level, level, error); I don't particularly object to this sort of public struct vs private struct object model, it's just not what you've actually implemented. -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On May 1, 2012 10:37 AM, Peter Maydell peter.mayd...@linaro.org wrote: On 1 May 2012 16:26, Anthony Liguori anth...@codemonkey.ws wrote: On 05/01/2012 10:20 AM, Peter Maydell wrote: The assumption is that failure to connect is a fatal error, and we can happily just assert()/hw_error()/etc. So that means that we have a bug from someone miss-typing, instead of your hotplug attempt failing with an error, your entire guest is destroyed. That doesn't sound very nice to me. If you're trying to hotplug a buggily implemented device then all bets are off, really. Its never okay to kill a guest. We should fail gracefully when possible or simply avoid failing in the first place. Device initialization should never exit() (unless you really hit a fatal error like OOM). (No, this doesn't do compile time type checking. I don't think that's a problem particularly, or at least not enough of one to justify not doing it this way. The object model we have is dynamic and does things at runtime...) Correctness is more important to me than brevity. And really, we should focus on killing things like i8259_init(). Functions like i8259_init() exist precisely because QOM/qdev don't provide brevity and people trying to use these devices do in fact value brevity. No, they exist because we aren't modeling correctly. Most of them are simply convenience functions that just do create, configure, wire up, init. (The ones that do more than that need fixing anyway. i8259_init() is doing a few different things at once. Once you split things between init and realize, you no longer have long chunks of redundant code. ...you have redundant code scattered in multiple places instead? Redundant is the wrong word. It seems like the code ought to be reduced to a single function call but it really cannot. That's why I want the standard native connect this thing to this other thing function to be short and simple. With my previous proposal, it's just: s-irq_in = b-int_out[0]; This is why I like exposing public members in structures. It's brief and safe. Obvious question: why isn't property setting done this way, then? Surely it's equally brief and safe to say cpu-level = def_level; rather than object_property_set_int(OBJECT(cpu), def-level, level, error); A primary design consideration for QOM was to *not* require using the generic property interface to set properties. That doesnt mean that you can/should access all properties this way. It depends on the specific property. I don't particularly object to this sort of public struct vs private struct object model, it's just not what you've actually implemented. One step at a time. Regards, Anthony Liguori -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 01/05/12 07:57, Blue Swirl wrote: Therefore I can't change it to my (modified) sbus_mmio_map() function because it would break other non-SPARC platforms, and AIUI there is nothing in the memory API that allows me to move a subregion to a different MemoryRegion parent, even if I can get a reference to it with sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I misunderstood something? Sysbus is used as a generic class for motherboard devices, there is an assumption that there is no higher level bus. What we need here is a full blown bus. The translations and mappigs between bus addresses and motherboard addresses should be done in a Sysbus to SBus bridge device, just like PCI host bridges do. Since SBus is mapped directly to physical addresses, is this mapping not just 1:1? Or does it make sense to re-work all the offsets of the various peripherals to be from the base address of the first slot? ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 01/05/12 16:20, Peter Maydell wrote: Correctness is more important to me than brevity. And really, we should focus on killing things like i8259_init(). Functions like i8259_init() exist precisely because QOM/qdev don't provide brevity and people trying to use these devices do in fact value brevity. That's why I want the standard native connect this thing to this other thing function to be short and simple. My understanding was that the *_init() functions were legacy and shouldn't be used any more - at least I've started removing them and replacing them with the slighty more long-winded QOM versions in my working tree. Or should I just leave them as they are copy the bits I need to a separate initialisation function? ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 1 May 2012 19:57, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote: On 01/05/12 16:20, Peter Maydell wrote: Correctness is more important to me than brevity. And really, we should focus on killing things like i8259_init(). Functions like i8259_init() exist precisely because QOM/qdev don't provide brevity and people trying to use these devices do in fact value brevity. That's why I want the standard native connect this thing to this other thing function to be short and simple. My understanding was that the *_init() functions were legacy and shouldn't be used any more - at least I've started removing them and replacing them with the slighty more long-winded QOM versions in my working tree. Yes, that is my opinion and is why I'm arguing with Anthony here... -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 01/05/12 08:10, Blue Swirl wrote: In your view, would a suitable fix be to change dma_memory_read, dma_memory_write, dma_opaque, it_shift and dma_enabled to be qdev properties and modify esp_init() to return the qdev reference so they can be set by the caller? There's an ongoing work to introduce IOMMUs by changing how DMA work and this could simplify the DMA part. There's no clean way to use function pointers in qdev. In my current working tree, I have actually managed to get this working with a customised qdev type macro and the standard qdev pointer type - so it may not be particularly great, but it works. For it_shift, a qdev or QOM property should be OK. The signal dma_enabled should be eventually replaced by a Pin. And this is a sysbus concept, yes? ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
Am 01.05.2012 20:50, schrieb Mark Cave-Ayland: On 01/05/12 08:10, Blue Swirl wrote: The signal dma_enabled should be eventually replaced by a Pin. And this is a sysbus concept, yes? No, it'll be a general DeviceState concept. SysBus is supposed to die out gradually. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/29/2012 10:35 PM, Anthony Liguori wrote: On 04/29/2012 12:25 PM, Mark Cave-Ayland wrote: Hi all, I've been having a look at handling SBUS probes within qemu-system-sparc when I came across a very simple crash bug with git master trying to access unassigned physical addresses: (qemu) info mtree memory -7ffe (prio 0, RW): system -07ff (prio 0, RW): sun4m.ram 0800-0fff (prio 0, RW): empty-slot 1000-10003fff (prio 0, RW): iommu 10004000-1fffefff (prio 0, RW): empty-slot 5020-502f (prio 0, RW): tcx.dac ... ... AFAICT the devices are mapped to physical addresses (uandsing sysbus_mmio_map) so the xp monitor command should be able to at least read these regions: (qemu) xp 0x5020 5020: 0x (qemu) xp 0x5010 Segmentation fault There is definitely a QEMU bug here in that an incorrect physical memory access shouldn't segfault QEMU. However some off-list discussions with Blue suggested that it may be possible to defer this behaviour to the memory API (rather than in exec.c) by allowing boards to register a simple device to handle unassigned memory accesses, e.g. like the empty_slot device used to trap certain memory accesses within SPARC. Yes. I think it's even possible to do this now, you can create an mmio region for the bus and add subregions to it. All subregions automatically overlap the container region. Simply replace memory_region_init(bus-address_space, ...) memory_region_add_subregion(bus-address_space, addr, dev-mmio) with memory_region_init_io(bus-address_space, bus_nodev_ops, bus, ...) memory_region_add_subregion(bus-address_space, addr, dev-mmio) // unchanged This was never used so it hasn't been tested, but the code was written with it in mind. I didn't want to document this so we could back out of it, but if it's useful, let's use it. What does real hardware do? Does hardware assert a line if an invalid MMIO request is generated? Note that real hardware varies within the board. Different buses can take different actions (and possibly even be reconfigured at runtime). This would enable us to easily solve the problem for SPARC since we could create a parent memory region for the entire physical address space that would simply update the status register and raise the required IRQ. Is this the best way to solve the problem or is there something else that I've missed? I think you want to look at memory_region_add_subregion_overlap(). Unneeded, see above. And remember, every time you need something from the memory API, look at a window system. In this case, we're replacing a transparent container window with an opaque one. Note that this may have side effects if the container region overlaps another region with higher priority. It's unlikely, but 'info mtree' is recommended before use. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30/04/12 09:41, Avi Kivity wrote:
Yes. I think it's even possible to do this now, you can create an mmio
region for the bus and add subregions to it. All subregions
automatically overlap the container region.
Simply replace
memory_region_init(bus-address_space, ...)
memory_region_add_subregion(bus-address_space, addr,dev-mmio)
with
memory_region_init_io(bus-address_space,bus_nodev_ops, bus, ...)
memory_region_add_subregion(bus-address_space, addr,dev-mmio) //
unchanged
This was never used so it hasn't been tested, but the code was written
with it in mind. I didn't want to document this so we could back out of
it, but if it's useful, let's use it.
Hi Avi,
Well I've attempted to code something like this, but I've hit a problem
with devices that aren't just used by SPARC, such as the disk controller
- it seems that the sysbus API can only MMIO map devices into the root
MemoryRegion :(
The SBUS I am trying to model is actually attached to the physical
address bus, but effectively the top 4 bits of the address control the
multiplexing of the on-board RAM and the individual per-slot address
lines. Effectively what I'm trying to model based on a 128MB default
memory setting is something like this:
Memory
0x0 - 0x007ff - Normal RAM
SBus
0x2000 - 0x02fff - SBus Slot 0
0x3000 - 0x03fff - SBus Slot 1
0x4000 - 0x04fff - SBus Slot 2
0x5000 - 0x05fff - SBus Slot 3
0x5020 - 0x502f - TCX DAC
0x5030 - 0x5030081b - TCH THC8
0x50301000 - 0x50301fff - TCX THC24
0x5070 - 0x50700fff - TCX TEC
0x6 - 0x06fff - SBus Slot 4
0x7 - 0x07fff - SBus Slot 5
Using your example above, I believe I can create a simple memory
hierarchy to represent this and catch unknown operations in a
bus_nodev_ops section no problem. The issue is that several shared
peripherals such as ESP have the following in their init function:
void esp_init(target_phys_addr_t espaddr, int it_shift,
ESPDMAMemoryReadWriteFunc dma_memory_read,
ESPDMAMemoryReadWriteFunc dma_memory_write,
void *dma_opaque, qemu_irq irq, qemu_irq *reset,
qemu_irq *dma_enable)
{
...
...
sysbus_mmio_map(s, 0, espaddr);
...
}
and sysbus_mmio_map() looks like this:
void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr)
{
...
...
memory_region_add_subregion(get_system_memory(),
addr,
dev-mmio[n].memory);
}
My understanding based upon this is that it would be impossible to
register a different parent MemoryRegion without duplicating the init
function for all shared devices which seems undesirable :(
The only solution I can think of is to make sysbus_mmio_map() more
intelligent so that instead of blindly adding the device to the root
MemoryRegion, it traverses down the MemoryRegion hierarchy starting from
the root to the furtherest leaf node it can find based upon the
specified address and then adds the new subregion there. Hence if I add
my SBus memory regions first before call the various peripheral init
functions, everything should end up in the correct part of the memory tree.
I believe this should preserve compatibility for existing sysbus API
users with just a single root MemoryRegion, however due to lack of any
documentation related to sysbus I'm not sure if this would break other
platforms or maybe even violate one of its key design features?
ATB,
Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/30/2012 03:17 PM, Mark Cave-Ayland wrote:
On 30/04/12 09:41, Avi Kivity wrote:
Yes. I think it's even possible to do this now, you can create an mmio
region for the bus and add subregions to it. All subregions
automatically overlap the container region.
Simply replace
memory_region_init(bus-address_space, ...)
memory_region_add_subregion(bus-address_space, addr,dev-mmio)
with
memory_region_init_io(bus-address_space,bus_nodev_ops, bus, ...)
memory_region_add_subregion(bus-address_space, addr,dev-mmio) //
unchanged
This was never used so it hasn't been tested, but the code was written
with it in mind. I didn't want to document this so we could back out of
it, but if it's useful, let's use it.
Hi Avi,
Well I've attempted to code something like this, but I've hit a
problem with devices that aren't just used by SPARC, such as the disk
controller - it seems that the sysbus API can only MMIO map devices
into the root MemoryRegion :(
The SBUS I am trying to model is actually attached to the physical
address bus, but effectively the top 4 bits of the address control the
multiplexing of the on-board RAM and the individual per-slot address
lines. Effectively what I'm trying to model based on a 128MB default
memory setting is something like this:
Memory
0x0 - 0x007ff - Normal RAM
SBus
0x2000 - 0x02fff - SBus Slot 0
0x3000 - 0x03fff - SBus Slot 1
0x4000 - 0x04fff - SBus Slot 2
0x5000 - 0x05fff - SBus Slot 3
0x5020 - 0x502f - TCX DAC
0x5030 - 0x5030081b - TCH THC8
0x50301000 - 0x50301fff - TCX THC24
0x5070 - 0x50700fff - TCX TEC
0x6 - 0x06fff - SBus Slot 4
0x7 - 0x07fff - SBus Slot 5
Using your example above, I believe I can create a simple memory
hierarchy to represent this and catch unknown operations in a
bus_nodev_ops section no problem. The issue is that several shared
peripherals such as ESP have the following in their init function:
void esp_init(target_phys_addr_t espaddr, int it_shift,
ESPDMAMemoryReadWriteFunc dma_memory_read,
ESPDMAMemoryReadWriteFunc dma_memory_write,
void *dma_opaque, qemu_irq irq, qemu_irq *reset,
qemu_irq *dma_enable)
{
...
...
sysbus_mmio_map(s, 0, espaddr);
...
}
and sysbus_mmio_map() looks like this:
void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr)
{
...
...
memory_region_add_subregion(get_system_memory(),
addr,
dev-mmio[n].memory);
}
My understanding based upon this is that it would be impossible to
register a different parent MemoryRegion without duplicating the init
function for all shared devices which seems undesirable :(
What are the requirements? You need a different catch-all mmio handler
for each slot? Or would one catch-all mmio handler for all sysbus
devices suffice?
The only solution I can think of is to make sysbus_mmio_map() more
intelligent so that instead of blindly adding the device to the root
MemoryRegion, it traverses down the MemoryRegion hierarchy starting
from the root to the furtherest leaf node it can find based upon the
specified address and then adds the new subregion there. Hence if I
add my SBus memory regions first before call the various peripheral
init functions, everything should end up in the correct part of the
memory tree.
This solution attempts to reconstruct the memory hierarchy from the
address, instead of maintaining it in the device tree.
I believe this should preserve compatibility for existing sysbus API
users with just a single root MemoryRegion, however due to lack of
any documentation related to sysbus I'm not sure if this would break
other platforms or maybe even violate one of its key design features?
IMO the best fix is to unsysbus the device and qomify it instead. This
way we're 100% flexible in how we can attach it.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30 April 2012 14:23, Avi Kivity a...@redhat.com wrote: IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. You don't need to wait for QOM to grow enough features to replace sysbus. If you don't like what sysbus_mmio_map() does, you can always use sysbus_mmio_get_region() to get the MemoryRegion* and then deal with it however you need to. This is the standard way to deal with I have a sysbus device which I want to map into my custom container object. -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/30/2012 04:27 PM, Peter Maydell wrote: On 30 April 2012 14:23, Avi Kivity a...@redhat.com wrote: IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. You don't need to wait for QOM to grow enough features to replace sysbus. If you don't like what sysbus_mmio_map() does, you can always use sysbus_mmio_get_region() to get the MemoryRegion* and then deal with it however you need to. This is the standard way to deal with I have a sysbus device which I want to map into my custom container object. I believe that API voids you warrantee. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30/04/12 14:23, Avi Kivity wrote: Hi Avi, My understanding based upon this is that it would be impossible to register a different parent MemoryRegion without duplicating the init function for all shared devices which seems undesirable :( What are the requirements? You need a different catch-all mmio handler for each slot? Or would one catch-all mmio handler for all sysbus devices suffice? A single catch-all for all sysbus devices would suffice, however I'm thinking it makes sense to have one MemoryRegion per slot so that the devices can register onto the bus using their slot relative address to allow for potentially moving hardware between slots. The only solution I can think of is to make sysbus_mmio_map() more intelligent so that instead of blindly adding the device to the root MemoryRegion, it traverses down the MemoryRegion hierarchy starting from the root to the furtherest leaf node it can find based upon the specified address and then adds the new subregion there. Hence if I add my SBus memory regions first before call the various peripheral init functions, everything should end up in the correct part of the memory tree. This solution attempts to reconstruct the memory hierarchy from the address, instead of maintaining it in the device tree. So I guess that is bad... I believe this should preserve compatibility for existing sysbus API users with just a single root MemoryRegion, however due to lack of any documentation related to sysbus I'm not sure if this would break other platforms or maybe even violate one of its key design features? IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. That's interesting - I didn't realise that sysbus is a legacy interface with respect to QOM. Is there any documentation related to this? Then again, converting all of the devices over to QOM and testing that it doesn't break all platforms/busses suddenly becomes a huge job... ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30 April 2012 14:36, Avi Kivity a...@redhat.com wrote: On 04/30/2012 04:27 PM, Peter Maydell wrote: On 30 April 2012 14:23, Avi Kivity a...@redhat.com wrote: IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. You don't need to wait for QOM to grow enough features to replace sysbus. If you don't like what sysbus_mmio_map() does, you can always use sysbus_mmio_get_region() to get the MemoryRegion* and then deal with it however you need to. This is the standard way to deal with I have a sysbus device which I want to map into my custom container object. I believe that API voids you warrantee. I wrote it for essentially the purpose described above :-) If you're the owner of the sysbus device in question then it's entirely fine as you are the one deciding whether to use the traditional map function or not. It's as good as we're going to get until QOM actually lets you export memory regions and pins, at which point we can just convert all the sysbus devices. -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/30/2012 08:36 AM, Avi Kivity wrote: On 04/30/2012 04:27 PM, Peter Maydell wrote: On 30 April 2012 14:23, Avi Kivitya...@redhat.com wrote: IMO the best fix is to unsysbus the device and qomify it instead. This way we're 100% flexible in how we can attach it. You don't need to wait for QOM to grow enough features to replace sysbus. If you don't like what sysbus_mmio_map() does, you can always use sysbus_mmio_get_region() to get the MemoryRegion* and then deal with it however you need to. This is the standard way to deal with I have a sysbus device which I want to map into my custom container object. I believe that API voids you warrantee. All that a QOM conversion would do is eliminate the use of sysbus and derive the object directly from DeviceState. Then, you would map the MemoryRegion exported by the device directly. So sysbus_mmio_get_region() seems like the right API to use. Regards, Anthony Liguori
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30/04/12 14:27, Peter Maydell wrote:
Hi Peter,
IMO the best fix is to unsysbus the device and qomify it instead. This
way we're 100% flexible in how we can attach it.
You don't need to wait for QOM to grow enough features to
replace sysbus. If you don't like what sysbus_mmio_map() does, you
Oh - so does this mean that QOM is not feature-complete?
can always use sysbus_mmio_get_region() to get the MemoryRegion* and
then deal with it however you need to. This is the standard way
to deal with I have a sysbus device which I want to map into my
custom container object.
I already have code to do this for the sun4m-only hardware modelled on
sysbus_mmio_map() like this:
static void sbus_mmio_map(void *sbus, SysBusDevice *s, int n,
target_phys_addr_t addr)
{
MemoryRegion *sbus_mem, *mem;
target_phys_addr_t sbus_base;
SBusState *sbus_state = FROM_SYSBUS(SBusState, (SysBusDevice *)sbus);
sbus_mem = sysbus_mmio_get_region((SysBusDevice *)sbus, 0);
mem = sysbus_mmio_get_region(s, n);
/* SBus addresses are physical addresses, so subtract start of
region */
sbus_base = sbus_state-base;
memory_region_add_subregion(sbus_mem, addr - sbus_base, mem);
}
The key problem is that this doesn't worked with shared peripherals,
such as the ESP device which is also used on various PPC Mac models as
well as SPARC. That's because its init function looks like this:
void esp_init(target_phys_addr_t espaddr, int it_shift,
ESPDMAMemoryReadWriteFunc dma_memory_read,
ESPDMAMemoryReadWriteFunc dma_memory_write,
void *dma_opaque, qemu_irq irq, qemu_irq *reset,
qemu_irq *dma_enable)
{
...
...
sysbus_mmio_map(s, 0, espaddr);
...
}
Therefore I can't change it to my (modified) sbus_mmio_map() function
because it would break other non-SPARC platforms, and AIUI there is
nothing in the memory API that allows me to move a subregion to a
different MemoryRegion parent, even if I can get a reference to it with
sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I
misunderstood something?
ATB,
Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30 April 2012 14:52, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote:
The key problem is that this doesn't worked with shared peripherals, such as
the ESP device which is also used on various PPC Mac models as well as
SPARC. That's because its init function looks like this:
void esp_init(target_phys_addr_t espaddr, int it_shift,
ESPDMAMemoryReadWriteFunc dma_memory_read,
ESPDMAMemoryReadWriteFunc dma_memory_write,
void *dma_opaque, qemu_irq irq, qemu_irq *reset,
qemu_irq *dma_enable)
{
...
...
sysbus_mmio_map(s, 0, espaddr);
...
}
Therefore I can't change it to my (modified) sbus_mmio_map() function
because it would break other non-SPARC platforms, and AIUI there is nothing
in the memory API that allows me to move a subregion to a different
MemoryRegion parent, even if I can get a reference to it with
sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I
misunderstood something?
Init functions like esp_init should be purely convenience functions
which create, set properties on, and map the sysbus/qdev device. If
they make use of private knowledge about the internals of the device
then this is wrong and should be fixed. For esp_init() it looks like
the handling of dma_memory_read, dma_memory_write, dma_opaque, it_shift
and dma_enabled are wrong.
If you fix that then you can just ignore the convenience function,
and create, configure and map the device as appropriate for you.
-- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30/04/12 15:03, Peter Maydell wrote: Therefore I can't change it to my (modified) sbus_mmio_map() function because it would break other non-SPARC platforms, and AIUI there is nothing in the memory API that allows me to move a subregion to a different MemoryRegion parent, even if I can get a reference to it with sysbus_mmio_get_region() after the sysbus_mmio_map() call - or have I misunderstood something? Init functions like esp_init should be purely convenience functions which create, set properties on, and map the sysbus/qdev device. If they make use of private knowledge about the internals of the device then this is wrong and should be fixed. For esp_init() it looks like the handling of dma_memory_read, dma_memory_write, dma_opaque, it_shift and dma_enabled are wrong. If you fix that then you can just ignore the convenience function, and create, configure and map the device as appropriate for you. Right I think I'm starting to understand this now - in which case it becomes a matter of just copying a handful of lines within sun4m which is more bearable. In your view, would a suitable fix be to change dma_memory_read, dma_memory_write, dma_opaque, it_shift and dma_enabled to be qdev properties and modify esp_init() to return the qdev reference so they can be set by the caller? ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30 April 2012 15:33, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote: On 30/04/12 15:03, Peter Maydell wrote: Right I think I'm starting to understand this now - in which case it becomes a matter of just copying a handful of lines within sun4m which is more bearable. In your view, would a suitable fix be to change dma_memory_read, dma_memory_write, dma_opaque, it_shift and dma_enabled to be qdev properties and modify esp_init() to return the qdev reference so they can be set by the caller? They should be some kind of qdev property, probably. Since you can't change a property after the qdev_init there's not much point changing esp_init to return the DeviceState*, though. (Disclaimer: I'm not sure what the best QOM/qdev practice is for here is a set of function pointers. In my ideal world this would be done by setting a single strongly typed qdev property which encapsulates the idea of here is a connection into which you can plug something that satisfies this dma read/write API.) -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30/04/12 15:39, Peter Maydell wrote: Right I think I'm starting to understand this now - in which case it becomes a matter of just copying a handful of lines within sun4m which is more bearable. In your view, would a suitable fix be to change dma_memory_read, dma_memory_write, dma_opaque, it_shift and dma_enabled to be qdev properties and modify esp_init() to return the qdev reference so they can be set by the caller? They should be some kind of qdev property, probably. Since you can't change a property after the qdev_init there's not much point changing esp_init to return the DeviceState*, though. In the meantime I've found a bit of reference documentation related to QOM and properties here: http://wiki.qemu.org/Features/QOM. (Disclaimer: I'm not sure what the best QOM/qdev practice is for here is a set of function pointers. In my ideal world this would be done by setting a single strongly typed qdev property which encapsulates the idea of here is a connection into which you can plug something that satisfies this dma read/write API.) Note that if properties can't be set by the caller after construction, then I'm effectively going to have to copy the entire esp_init() function; so until the QOM magic happens to allow plugging things into arbitrary buses, I might as well just keep a copy in sun4m.c with my local modifications as sun4_esp_init() and be done with it. Slightly frustrating, but I think that's going to be the easiest solution for the moment. ATB, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30 April 2012 15:55, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote: Note that if properties can't be set by the caller after construction, then I'm effectively going to have to copy the entire esp_init() function Yes. As I say, these init functions are purely convenience functions for the simple case. (My opinion is that if it seems like a pain to have to open code them it's an indication that we ought to be trying to make it easier to create-configure-init QOM objects. But that's not entirely trivial in C.) -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 30 April 2012 15:55, Mark Cave-Ayland mark.cave-ayl...@ilande.co.uk wrote: I might as well just keep a copy in sun4m.c with my local modifications as sun4_esp_init() and be done with it. Slightly frustrating, but I think that's going to be the easiest solution for the moment. You will need to address the properties issue anyway, though, because sun4m.c doesn't have access to the layout of struct ESPState. -- PMM
Re: [Qemu-devel] Memory API: handling unassigned physical memory
Am 30.04.2012 16:55, schrieb Mark Cave-Ayland: Note that if properties can't be set by the caller after construction, then I'm effectively going to have to copy the entire esp_init() function; The idea would be to use the QOM way of creating the object, using QOM and/or custom ways to set properties/fields and then realize it. The infrastructure for the latter is still missing, so for now you would split the esp_init function into one part that goes to the call sites and an esp_realize function that gets called for any remaining part after the appropriate properties and MMIO mappings have been set up. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/29/2012 12:25 PM, Mark Cave-Ayland wrote: Hi all, I've been having a look at handling SBUS probes within qemu-system-sparc when I came across a very simple crash bug with git master trying to access unassigned physical addresses: (qemu) info mtree memory -7ffe (prio 0, RW): system -07ff (prio 0, RW): sun4m.ram 0800-0fff (prio 0, RW): empty-slot 1000-10003fff (prio 0, RW): iommu 10004000-1fffefff (prio 0, RW): empty-slot 5020-502f (prio 0, RW): tcx.dac ... ... AFAICT the devices are mapped to physical addresses (uandsing sysbus_mmio_map) so the xp monitor command should be able to at least read these regions: (qemu) xp 0x5020 5020: 0x (qemu) xp 0x5010 Segmentation fault There is definitely a QEMU bug here in that an incorrect physical memory access shouldn't segfault QEMU. However some off-list discussions with Blue suggested that it may be possible to defer this behaviour to the memory API (rather than in exec.c) by allowing boards to register a simple device to handle unassigned memory accesses, e.g. like the empty_slot device used to trap certain memory accesses within SPARC. What does real hardware do? Does hardware assert a line if an invalid MMIO request is generated? This would enable us to easily solve the problem for SPARC since we could create a parent memory region for the entire physical address space that would simply update the status register and raise the required IRQ. Is this the best way to solve the problem or is there something else that I've missed? I think you want to look at memory_region_add_subregion_overlap(). Regards, Anthony Liguori Many thanks, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On Sun, Apr 29, 2012 at 19:35, Anthony Liguori anth...@codemonkey.ws wrote: On 04/29/2012 12:25 PM, Mark Cave-Ayland wrote: Hi all, I've been having a look at handling SBUS probes within qemu-system-sparc when I came across a very simple crash bug with git master trying to access unassigned physical addresses: (qemu) info mtree memory -7ffe (prio 0, RW): system -07ff (prio 0, RW): sun4m.ram 0800-0fff (prio 0, RW): empty-slot 1000-10003fff (prio 0, RW): iommu 10004000-1fffefff (prio 0, RW): empty-slot 5020-502f (prio 0, RW): tcx.dac ... ... AFAICT the devices are mapped to physical addresses (uandsing sysbus_mmio_map) so the xp monitor command should be able to at least read these regions: (qemu) xp 0x5020 5020: 0x (qemu) xp 0x5010 Segmentation fault There is definitely a QEMU bug here in that an incorrect physical memory access shouldn't segfault QEMU. However some off-list discussions with Blue suggested that it may be possible to defer this behaviour to the memory API (rather than in exec.c) by allowing boards to register a simple device to handle unassigned memory accesses, e.g. like the empty_slot device used to trap certain memory accesses within SPARC. What does real hardware do? Does hardware assert a line if an invalid MMIO request is generated? I think the SBus controller (in IOMMU) detects a timeout if no device responds to MMIO and raises an IRQ, same as with other bus errors. This would enable us to easily solve the problem for SPARC since we could create a parent memory region for the entire physical address space that would simply update the status register and raise the required IRQ. Is this the best way to solve the problem or is there something else that I've missed? I think you want to look at memory_region_add_subregion_overlap(). Regards, Anthony Liguori Many thanks, Mark.
Re: [Qemu-devel] Memory API: handling unassigned physical memory
On 04/29/2012 03:09 PM, Blue Swirl wrote: On Sun, Apr 29, 2012 at 19:35, Anthony Liguorianth...@codemonkey.ws wrote: On 04/29/2012 12:25 PM, Mark Cave-Ayland wrote: Hi all, I've been having a look at handling SBUS probes within qemu-system-sparc when I came across a very simple crash bug with git master trying to access unassigned physical addresses: (qemu) info mtree memory -7ffe (prio 0, RW): system -07ff (prio 0, RW): sun4m.ram 0800-0fff (prio 0, RW): empty-slot 1000-10003fff (prio 0, RW): iommu 10004000-1fffefff (prio 0, RW): empty-slot 5020-502f (prio 0, RW): tcx.dac ... ... AFAICT the devices are mapped to physical addresses (uandsing sysbus_mmio_map) so the xp monitor command should be able to at least read these regions: (qemu) xp 0x5020 5020: 0x (qemu) xp 0x5010 Segmentation fault There is definitely a QEMU bug here in that an incorrect physical memory access shouldn't segfault QEMU. However some off-list discussions with Blue suggested that it may be possible to defer this behaviour to the memory API (rather than in exec.c) by allowing boards to register a simple device to handle unassigned memory accesses, e.g. like the empty_slot device used to trap certain memory accesses within SPARC. What does real hardware do? Does hardware assert a line if an invalid MMIO request is generated? I think the SBus controller (in IOMMU) detects a timeout if no device responds to MMIO and raises an IRQ, same as with other bus errors. I see, so it's similar to ISA. The memory API assumes positive decoding and I think the best way we have to cope with this is to register overlapped regions with the default region having a lower priority than anything that sits on top of it. Would be nice to have a way to mark a MemoryRegion as allowing overlap even if you add a subregion without using memory_region_add_subregion_overlap(). Regards, Anthony Liguori This would enable us to easily solve the problem for SPARC since we could create a parent memory region for the entire physical address space that would simply update the status register and raise the required IRQ. Is this the best way to solve the problem or is there something else that I've missed? I think you want to look at memory_region_add_subregion_overlap(). Regards, Anthony Liguori Many thanks, Mark.
