Re: [Qemu-devel] [Qemu-ppc] [RFC ppc-next PATCH 3/6] memory: add memory_region_to_address()
On 03/23/2013 06:24:49 AM, Peter Maydell wrote: On 22 March 2013 22:05, Scott Wood wrote: > On 03/22/2013 08:08:57 AM, Peter Maydell wrote: >> The memory API flattens the tree of memory regions down into a flat >> view of the address space. These callbacks get called for the >> final flattened view (so you'll never see a pure container in the >> callback, only leaves). The callbacks happen for every region which >> appears in the address space, in linear order. When an update happens >> memory.c identifies the changes between the old flat view and the >> new one and calls callbacks appropriately. > > OK, so .add and .del will be sufficient to capture any manipulation that > would affect whether and where the region we care about is mapped? Yes. Note that if the board (brokenly) maps the region so it is 'hidden' by another region, this manifests as a .del [since it is no longer accessible]. Also I think if the board maps something small on top and in the middle of the region you get an add for each of the partially visible fragments. Personally I'm happy to not worry about either of these cases Yeah, if we do check for those cases it should just be to print an error. on the basis that they would be board model bugs. In some cases it could be guest code doing something screwy, but if you need to support that then turn off the in-kernel pic. >> > The MPIC's memory listener isn't that much code... I'm not sure >> > there's a great need for a central KVM registry. >> >> Well, nor is the ARM memory listener, but why have two bits of >> code doing the same thing when you could have one? > > They're not doing quite the same thing, though, and the effort required to > unify them is non-zero. The two main issues are the way that the address is > communicated to KVM, and the ability to change the mapping after the guest > starts. Ah, guest-programmable mappings are a real use case and not a hypothetical? It's real in terms of how the hardware works. QEMU doesn't yet implement it, but it should, especially with Alex occasionally saying he'd like to see QEMU capable of running U-Boot. Do we run into synchronisation issues with making sure that QEMU and the kernel both agree simultaneously about where the mapping is? I don't think so -- the guest CPU that is doing the moving is stopped for QEMU MMIO, and if the other CPUs try to access it in the meantime, it would be undefined even on real hardware whether it happens before or after the move takes effect. Can the mapping be different between different CPU cores? [let's hope not :-)] Not in a way that's relevant here. There's some per-cpu magic internal to the region, but that's handled within the kernel. Is the mapping controlled by a register within the mapping itself, or is there some separate non-moving register which defines the location of the mappable registers? There's a separate moving register. :-P MPIC is combined with a bunch of other devices into a large region called CCSR, and that region can be moved as a whole by writing to a register at the beginning of CCSR. -Scott
Re: [Qemu-devel] [Qemu-ppc] [RFC ppc-next PATCH 3/6] memory: add memory_region_to_address()
On 22 March 2013 22:05, Scott Wood wrote: > On 03/22/2013 08:08:57 AM, Peter Maydell wrote: >> The memory API flattens the tree of memory regions down into a flat >> view of the address space. These callbacks get called for the >> final flattened view (so you'll never see a pure container in the >> callback, only leaves). The callbacks happen for every region which >> appears in the address space, in linear order. When an update happens >> memory.c identifies the changes between the old flat view and the >> new one and calls callbacks appropriately. > > OK, so .add and .del will be sufficient to capture any manipulation that > would affect whether and where the region we care about is mapped? Yes. Note that if the board (brokenly) maps the region so it is 'hidden' by another region, this manifests as a .del [since it is no longer accessible]. Also I think if the board maps something small on top and in the middle of the region you get an add for each of the partially visible fragments. Personally I'm happy to not worry about either of these cases on the basis that they would be board model bugs. >> This code isn't the >> first use of the memory API listeners, so it's all well-tested code. > > > Sure, I'm not suggesting the code doesn't work -- just trying to understand > how, so I know I'm using it properly. The implementation is a bit opaque > (to me at least), and the listener callbacks aren't documented the way the > normal API functions are. Yeah, it would I guess be good to add doc comments for all the fields in struct MemoryListener describing the semantics of the callbacks. >> > The MPIC's memory listener isn't that much code... I'm not sure >> > there's a great need for a central KVM registry. >> >> Well, nor is the ARM memory listener, but why have two bits of >> code doing the same thing when you could have one? > > They're not doing quite the same thing, though, and the effort required to > unify them is non-zero. The two main issues are the way that the address is > communicated to KVM, and the ability to change the mapping after the guest > starts. Ah, guest-programmable mappings are a real use case and not a hypothetical? Do we run into synchronisation issues with making sure that QEMU and the kernel both agree simultaneously about where the mapping is? Can the mapping be different between different CPU cores? [let's hope not :-)] Is the mapping controlled by a register within the mapping itself, or is there some separate non-moving register which defines the location of the mappable registers? thanks -- PMM
Re: [Qemu-devel] [Qemu-ppc] [RFC ppc-next PATCH 3/6] memory: add memory_region_to_address()
On 03/22/2013 08:08:57 AM, Peter Maydell wrote: On 21 March 2013 22:43, Scott Wood wrote: > What if the update is to a parent memory region, not to the one directly > associated with the device? > > Or does add() get called for all child regions (recursively) in such cases? The memory API flattens the tree of memory regions down into a flat view of the address space. These callbacks get called for the final flattened view (so you'll never see a pure container in the callback, only leaves). The callbacks happen for every region which appears in the address space, in linear order. When an update happens memory.c identifies the changes between the old flat view and the new one and calls callbacks appropriately. OK, so .add and .del will be sufficient to capture any manipulation that would affect whether and where the region we care about is mapped? This code isn't the first use of the memory API listeners, so it's all well-tested code. Sure, I'm not suggesting the code doesn't work -- just trying to understand how, so I know I'm using it properly. The implementation is a bit opaque (to me at least), and the listener callbacks aren't documented the way the normal API functions are. >> However, maybe with a bit of brainstorming we could come up with a >> reasonably generic scheme. > In the kernel API? Or do you mean a generic scheme within QEMU that encodes > any reasonably expected mechanism for setting the device adress (e.g. assume > that it is either a 64-bit attribute, or uses the legacy ARM API), or > perhaps a callback into device code? > > The MPIC's memory listener isn't that much code... I'm not sure > there's a great need for a central KVM registry. Well, nor is the ARM memory listener, but why have two bits of code doing the same thing when you could have one? They're not doing quite the same thing, though, and the effort required to unify them is non-zero. The two main issues are the way that the address is communicated to KVM, and the ability to change the mapping after the guest starts. -Scott
Re: [Qemu-devel] [Qemu-ppc] [RFC ppc-next PATCH 3/6] memory: add memory_region_to_address()
On 21 March 2013 22:43, Scott Wood wrote: > What if the update is to a parent memory region, not to the one directly > associated with the device? > > Or does add() get called for all child regions (recursively) in such cases? The memory API flattens the tree of memory regions down into a flat view of the address space. These callbacks get called for the final flattened view (so you'll never see a pure container in the callback, only leaves). The callbacks happen for every region which appears in the address space, in linear order. When an update happens memory.c identifies the changes between the old flat view and the new one and calls callbacks appropriately. This code isn't the first use of the memory API listeners, so it's all well-tested code. >> However, maybe with a bit of brainstorming we could come up with a >> reasonably generic scheme. > In the kernel API? Or do you mean a generic scheme within QEMU that encodes > any reasonably expected mechanism for setting the device adress (e.g. assume > that it is either a 64-bit attribute, or uses the legacy ARM API), or > perhaps a callback into device code? > > The MPIC's memory listener isn't that much code... I'm not sure > there's a great need for a central KVM registry. Well, nor is the ARM memory listener, but why have two bits of code doing the same thing when you could have one? -- PMM
Re: [Qemu-devel] [Qemu-ppc] [RFC ppc-next PATCH 3/6] memory: add memory_region_to_address()
On 03/21/2013 06:51:57 AM, Alexander Graf wrote:
On 21.03.2013, at 12:49, Alexander Graf wrote:
>
> On 21.03.2013, at 12:44, Peter Maydell wrote:
>
>> On 21 March 2013 11:38, Alexander Graf wrote:
>>>
>>> On 21.03.2013, at 12:32, Peter Maydell wrote:
>>>
On 21 March 2013 11:29, Alexander Graf wrote:
> On 21.03.2013, at 12:22, Peter Maydell wrote:
>> We already nest the VGIC inside another memory region (the
a15mpcore
>> container), and it works fine. This function is just iterating
through
>> "everything any device asked me to tell the kernel about".
>
> So kda is the real physical offset? I'm having a hard time
reading that code :). According to this function:
>
> static void kvm_arm_devlistener_add(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> KVMDevice *kd;
>
> QSLIST_FOREACH(kd, &kvm_devices_head, entries) {
> if (section->mr == kd->mr) {
> kd->kda.addr = section->offset_within_address_space;
> }
> }
> }
What if the update is to a parent memory region, not to the one
directly associated with the device?
Or does add() get called for all child regions (recursively) in such
cases?
>>> The distinction on whether a region is handled by KVM really needs
>>> to be done by the device model.
>>
>> It is -- the device model is what calls kvm_arm_register_device().
>> It's just the mechanics of "how do we tell the kernel the right
>> address for this region at the point when we know it" that are
>> handled in kvm.c.
>
> I think I'm slowly grasping what you're aiming at :). Ok, that
works. You do actually do the listener in the device model, just that
you pass code responsibility over to kvm.c.
>
> That's perfectly valid and sounds like a good model that Scott
probably wants to follow as well :).
s/follow/evaluate/ :).
The currently proposed device api doesn't have a generic notion of
device regions. Regions are a per-device property, because a single
device can have multiple regions.
However, maybe with a bit of brainstorming we could come up with a
reasonably generic scheme.
In the kernel API? Or do you mean a generic scheme within QEMU that
encodes any reasonably expected mechanism for setting the device adress
(e.g. assume that it is either a 64-bit attribute, or uses the legacy
ARM API), or perhaps a callback into device code?
The MPIC's memory listener isn't that much code... I'm not sure there's
a great need for a central KVM registry.
-Scott
Re: [Qemu-devel] [Qemu-ppc] [RFC ppc-next PATCH 3/6] memory: add memory_region_to_address()
On 21.03.2013, at 12:49, Alexander Graf wrote:
>
> On 21.03.2013, at 12:44, Peter Maydell wrote:
>
>> On 21 March 2013 11:38, Alexander Graf wrote:
>>>
>>> On 21.03.2013, at 12:32, Peter Maydell wrote:
>>>
On 21 March 2013 11:29, Alexander Graf wrote:
> On 21.03.2013, at 12:22, Peter Maydell wrote:
>> We already nest the VGIC inside another memory region (the a15mpcore
>> container), and it works fine. This function is just iterating through
>> "everything any device asked me to tell the kernel about".
>
> So kda is the real physical offset? I'm having a hard time reading that
> code :). According to this function:
>
> static void kvm_arm_devlistener_add(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> KVMDevice *kd;
>
> QSLIST_FOREACH(kd, &kvm_devices_head, entries) {
> if (section->mr == kd->mr) {
> kd->kda.addr = section->offset_within_address_space;
> }
> }
> }
>
> it's only the offset within its parent region, which would mean it's
> broken, no?
Address spaces are not the same thing as memory regions :-)
The only address space involved here is the system address space.
(As I say, we currently assume we only get mapped into one address
space, but that could be fixed if necessary.)
>>>
>>> Interesting. Oh well, I'll leave that one to Scott to figure out ;).
>>>
>>> So what if I want to write an in-kernel IDE PIO accelerator?
>>
>> Have the QEMU end of that device call (your equivalent of)
>> kvm_arm_register_device(), and provide a 'reserved' mmio region to
>> its users; the kernel end implements the standard 'tell me where I live'
>> ioctl; that's it.
>>
>>> Or even better yet: An AHCI accelerator that has one MMIO BAR and
>>> another PIO BAR that can be remapped by the guest at any time?
>>
>> Guest remappable KVM regions would require enhancements, yes (it's
>> not like we have an existing mechanism for doing that on any
>> architecture at the moment). The principle of implementing the
>> mechanics of this in common code still holds, probably even more
>> so for the increased complexity.
>>
>>> The distinction on whether a region is handled by KVM really needs
>>> to be done by the device model.
>>
>> It is -- the device model is what calls kvm_arm_register_device().
>> It's just the mechanics of "how do we tell the kernel the right
>> address for this region at the point when we know it" that are
>> handled in kvm.c.
>
> I think I'm slowly grasping what you're aiming at :). Ok, that works. You do
> actually do the listener in the device model, just that you pass code
> responsibility over to kvm.c.
>
> That's perfectly valid and sounds like a good model that Scott probably wants
> to follow as well :).
s/follow/evaluate/ :).
The currently proposed device api doesn't have a generic notion of device
regions. Regions are a per-device property, because a single device can have
multiple regions.
However, maybe with a bit of brainstorming we could come up with a reasonably
generic scheme.
Alex
