[PATCH v5 01/12] KVM: ARM: Introduce KVM_SET_DEVICE_ADDRESS ioctl

[Christoffer Dall]

On Wed, Jan 9, 2013 at 10:11 AM, Peter Maydell <peter.maydell at linaro.org> wrote:
> On 9 January 2013 14:58, Alexander Graf <agraf at suse.de> wrote:
>>
>> On 09.01.2013, at 15:48, Peter Maydell wrote:
>>
>>> On 9 January 2013 10:02, Alexander Graf <agraf at suse.de> wrote:
>>>> I think we should make thus at least potentially generic. In fact, I wouldn't even
>>>> mind calling it DEV_REG with the same semantics as ONE_REG, just that it also
>>>> gets a unique dev id that gets created during in-kernel device creation and that
>>>> it's a vm ioctl.
>>>
>>> Well, we might want a DEV_REG, but you might as well just make ONE_REG
>>> OK as a VM ioctl, since there's no reason not to have not-per-cpu but not
>>> device registers. ONE_REG already supports dividing up the ID space so
>>> you can say which device or whatever is being used, because we had
>>> things like GIC registers in mind when we put that API together.
>>
>> ONE_REG's address space today doesn't include a field for the target device,
>
> There's 16 bits of "register group type".
>
>> because that's already predetermined by the fd you run it on. Hence my
>> suggestion to add it as separate field, because then we keep the id space
>> mask-free.
>
> Er, it already has masks for the different parts of ID space. That's a feature,
> not a bug.
>
>>> However, this shouldn't be DEV_REG, because this isn't actually setting state
>>> in the irqchip device, it's configuring the kernel's part of the system model
>>> [compare wiring up irq routing, which also has a custom ioctl rather than a
>>> generic one]. As such it definitely needs to happen only before the VM is
>>> actually started and not during VM execution, unlike a DEV_REG which would
>>> presumably be generally valid.
>>
>> The same can be true for ONE_REG/SET_SREGS. On PPC we support setting
>> the PVR (CPU type) via SREGS because ONE_REG only came later. Setting
>> the CPU type only makes sense before you first start using a vcpu. After that
>> point it should be forbidden.
>>
>> So in a way, the irqchip memory address location is a device register, as it has
>> all the properties that a register on that device would have. It is
>>
>>   * per device
>>   * one id for one purpose
>>   * may or may not be writable during certain times
>
> There's a distinction between "things you need to do in machine setup"
> and "things you need to propagate for migration". It should be possible
> to do a migration by doing a complete copy of all ONE_REG (and DEV_REG
> if we have it) state from source to destination, so it needs to be always
> possible to write them.
>
>> We would also keep the gates open to get a new register id one day for a second
>> address. Imagine an interrupt controller that splits its address mappings into
>> "PIC base registers" and "MSI target registers", though the MSI registers logically
>> belong to the PIC. Then the ioctl as designed here gets stuck too.
>
> This is exactly what ARM's GIC has already. We have several memory regions
> (one for the distributor, one for the cpu interface), and map them with several
> calls to SET_DEVICE_ADDRESS.
>
>>> (b) we didn't need to tangle up and delay the KVM ARM work with a vague
>>> and unspecified desire for general configurability
>>
>> Yeah, that was the basic idea. Considering that the patch set hasn't been going
>> in for another 2 months after that discussion indicates that this isn't too much of
>> an issue though :).
>
well, now it's actually going in, and it seems like this is the last
issue. Let's hold our breaths here for a second and let me write up a
new device attribute API and send that out, and if you guys can
possibly live with it, then please consider accepting it.

-Christoffer