On 09.01.2013, at 16:11, Peter Maydell wrote: > On 9 January 2013 14:58, Alexander Graf <agraf@xxxxxxx> wrote: >> >> On 09.01.2013, at 15:48, Peter Maydell wrote: >> >>> On 9 January 2013 10:02, Alexander Graf <agraf@xxxxxxx> 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". That's "register group", not "device id". It's constant per id. If you have an ARM register, it will always stay an ARM register. It's not going to change. But if you reuse those bits for the device id, they are going to change, because a device id is similar to a handle that should be returned by the device create ioctl. > >> 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. All those masks are constant masks and only used to make reading the IDs easier. They never change within a single type of a register. > >>> 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. You need to somewhere encode a list of registers you want to synchronize for migration anyways. Don't include this register in that list then. > >> 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. I see. > >>> (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 :). > > We might get there faster if people didn't keep nitpicking the APIs at the > last minute :-) I guess we just had too much bad experience with designing horrible APIs in the past on the PPC side ;). Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
