Re: [Qemu-ppc] [RFC ppc-next PATCH 3/6] memory: add memory_region_to_address()

[Alexander Graf]

On 21.03.2013, at 12:22, Peter Maydell wrote:

> On 21 March 2013 11:14, Alexander Graf <address@hidden> wrote:
>> 
>> On 21.03.2013, at 12:09, Peter Maydell wrote:
>> 
>>> On 21 March 2013 11:05, Alexander Graf <address@hidden> wrote:
>>>> 
>>>> On 21.03.2013, at 12:01, Peter Maydell wrote:
>>>> 
>>>>> On 21 March 2013 10:59, Alexander Graf <address@hidden> wrote:
>>>>>> On 21.03.2013, at 11:53, Peter Maydell wrote:
>>>>>>> Check kvm_arm_register_device() in target-arm/kvm.c. Basically
>>>>>>> the VGIC device model calls this function to say "tell the kernel
>>>>>>> where this MemoryRegion is in the system address space, when it
>>>>>>> eventually gets mapped". The code in kvm.c uses the memory system's
>>>>>>> Notifier API to get a callback when the region is mapped into
>>>>>>> an address space, which it uses to track the offset in the
>>>>>>> address space. Finally, we use a machine init notifier so that
>>>>>>> just before everything finally starts we can make the KVM ioctls
>>>>>>> to say "here is where everything lives".
>>>>>> 
>>>>>> Same thing here. The question is how the kvm-vgic code in QEMU
>>>>>> finds out where it got mapped to. Scott adds this patch to do
>>>>>> this, but I'd assume you have some other way :)
>>>>> 
>>>>> Hmm? The kvm-vgic code in QEMU doesn't need to know where it
>>>>> lives. We have to tell the kernel so it can map its bits of
>>>>> registers in at the right place, that's all.
>>>> 
>>>> The kvm-vgic code in QEMU needs to tell the kernel, no? For
>>>> that, it needs to know what to tell the kernel.
>>> 
>>> No. As I explained earlier, all the kvm-vgic code needs to do
>>> is call kvm_arm_register_device(). That code in kvm.c then takes
>>> care of telling the kernel. hw/kvm/arm_gic.c itself never knows or
>>> needs to know where it's mapped. This is the whole point of the
>>> mechanism involving notifiers.
>> 
>> I fully disagree. Code that talks to the in-kernel device should
>> live in hw/kvm/<device>.c, not in some random target-XXX/kvm.c file.
> 
> The code in kvm.c is entirely generic -- it provides a mechanism
> for a device to say "this memory region is kernel ID X and it will
> want to know where it lives". The kvm.c code will work for any device
> with a memory mapped region, whether it's the GIC or something else.
> 
>> Of course the board defines where the device gets mapped to, but
>> the communication with the in-kernel device bits should really be
>> contained to the device itself.
> 
> You're arguing that every device should implement its own set
> of notifier functions so it can get called back when its memory
> regions are finally mapped, just so it can make a non-device-specific
> KVM ioctl? The obvious thing to do is abstract that functionality
> out.

What I'm arguing is that every device should look as if it was a QEMU device. 
Devices that happen to live in KVM, should still make a significant effort to 
expose themselves to the board model as if they were QEMU devices.

So yes, I think the device model should at least register the memory listener, 
because only the device model knows what its memory regions would map to in 
KVM's world. Not all devices have a single flat region. Some have more than one.

Whether we have a helper function in (generic) kvm.c that can call a (generic) 
ioctl to set a device's region X is a different matter. I'd be open to that if 
it makes sense.

> 
>> So this is the function that gets invoked on ARM:
>> 
>> static void kvm_arm_machine_init_done(Notifier *notifier, void *data)
>> {
>>    KVMDevice *kd, *tkd;
>> 
>>    memory_listener_unregister(&devlistener);
>>    QSLIST_FOREACH_SAFE(kd, &kvm_devices_head, entries, tkd) {
>>        if (kd->kda.addr != -1) {
>>            if (kvm_vm_ioctl(kvm_state, KVM_ARM_SET_DEVICE_ADDR,
>>                             &kd->kda) < 0) {
>>                fprintf(stderr, "KVM_ARM_SET_DEVICE_ADDRESS failed: %s\n",
>>                        strerror(errno));
>>                abort();
>>            }
>>        }
>>        g_free(kd);
>>    }
>> }
>> 
>> This only goes one level deep, right? So if you ever have to nest the
>> VGIC inside of another MemoryRegion, this will break, right?
> 
> 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?


Alex