On Mon, Jan 11, 2016 at 04:09:29PM +0100, Christoffer Dall wrote: > On Mon, Jan 11, 2016 at 03:07:17PM +0100, Andrew Jones wrote: > > On Sat, Jan 09, 2016 at 03:03:39PM +0000, Marc Zyngier wrote: > > > On Sat, 9 Jan 2016 13:29:56 +0100 > > > Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote: > > > > > > > On Thu, Jan 07, 2016 at 09:36:47PM +0100, Andrew Jones wrote: > > > > > On Thu, Jan 07, 2016 at 02:56:15PM +0000, Peter Maydell wrote: > > > > > > On 7 January 2016 at 14:49, Shannon Zhao <zhaoshenglong@xxxxxxxxxx> wrote: > > > > > > >>> + > > > > > > >>> +Groups: > > > > > > >>> + KVM_DEV_ARM_PMU_GRP_IRQ > > > > > > >>> + Attributes: > > > > > > >>> + The attr field of kvm_device_attr encodes one value: > > > > > > >>> + bits: | 63 .... 32 | 31 .... 0 | > > > > > > >>> + values: | reserved | vcpu_index | > > > > > > >>> + A value describing the PMU overflow interrupt number for the specified > > > > > > >>> + vcpu_index vcpu. This interrupt could be a PPI or SPI, but for one VM the > > > > > > >>> + interrupt type must be same for each vcpu. As a PPI, the interrupt number is > > > > > > >>> + same for all vcpus, while as a SPI it must be different for each vcpu. > > > > > > >> > > > > > > >> I see we're using vcpu_index rather than MPIDR affinity value > > > > > > >> for specifying which CPU we're configuring. Is this in line with > > > > > > >> our planned API for GICv3 configuration? > > > > > > >> > > > > > > > Here vcpu_index is used to indexing the vCPU, no special use. > > > > > > > > > > > > Yes, but you can identify the CPU by index, or by its MPIDR. > > > > > > We had a discussion about which was the best way for doing > > > > > > the VGIC API, and I can't remember which way round we ended up > > > > > > going for. Whichever we chose, we should do the same thing here. > > > > > > > > > > I think we should start up a new discussion on this. My understanding, > > > > > after a chat with Igor, who was involved in the untangling of vcpu-id and > > > > > apic-id for x86, is that using vcpu-id is preferred, unless of course > > > > > the device expects an apic-id/mpidr, in which case there's no reason to > > > > > translate it on both sides. > > > > > > > > > > > > > I'm fairly strongly convinced that we should use the full 32-bit > > > > compressed MPIDR for everything ARM related going forward, as this will > > > > cover any case required and leverages and architecturally defined way of > > > > uniquely identifying a (v)CPU. > > > > > > +1. > > > > > > vcpu_ids, indexes or any other constructs are just a bunch > > > of KVM-specific definitions that do not describe the VM from an > > > architecture PoV. In contrast, the MPIDR is guaranteed to be unique > > > stable, and identifies a given (v)CPU. > > > > > > > cpu-cpu and cpu-device interfaces should certainly use MPIDR, if they do > > in real hardware, to allow us to match emulation code to specs and keep > > sanity. But I assume those are the only places of "everything" you guys > > are referring to, as everywhere else we should stick to using the concept > > of vcpu-ids/indices. Since vcpu-indices are just counters they keep us > > from needing all the data structures to be large, complex, sparse things. > > Identifiers separate from MPIDR also allow hotunplug/plug to more easily > > reuse resources, i.e. remap indices to other vcpus as necessary. > > Are vcpu ids already exposed to userspace (beyond the stupid > KVM_IRQ_LINE) ioctl and as such we're bound to whatever upper limit and > format they have? The only other place I found is KVM_CREATE_VCPU. I suppose we could move to MPIDR for that, and it would be a nice way to handle the "userspace determines MPIDR" work that I plan to do. Both KVM and its userspaces would still use some counter-based vcpu identifiers internally, to avoid large, sparse structures, but I guess the advantage is that they don't have to agree on how they do that. The 'vcpu id' used by KVM_CREATE_VCPU is already 32-bits, and is supposed to be an arbitrary identifier. That all looks good for converting to MPIDR. > > If not, I think decoupling an internal ID and uniquely identifying a CPU > is a good idea. > > > > > In the PMU case above it seems better to use a vcpu-index. KVM and KVM's > > userspace both have unique vcpu-indices and unique MPIDRs per vcpu. The > > use here isn't based on a hardware spec, so there's nowhere to look for > > how MPIDR should/shouldn't be used. This is just a KVM spec. Here we might > > as well use the easiest to use unique identifier. > > I think the only things that should matter here are (in no particular > order): > - Userspace convenience > - Clarity > - Avoiding ambiguity Agreed. I believe the counter-based vcpu-id brings the most convenience for any non-MPIDR based APIs, i.e. no spec saying MPIDR should be used. However, picking which counter-based vcpu-id (if more than one exist) costs both convenience and clarity. I guess that's another argument for switching to MPIDR... > > > > > That said, I like the vcpu ioctl method much better. With that we avoid > > the need for vcpu identifiers all together. I'm even having third > > thoughts about the gic per vcpu registers. If we go with extending > > GET/SET_DEVICE_ATTR here, then I think we should do the same there as > > well. That would then leave only KVM_IRQ_LINE using a vcpu-index, which, > > with its 8-bit vcpu-index, we've outgrown for gicv3 machine types already > > anyway. > > > For the GIC, I think we've discussed this in the past. It really > depends whether you think about the GIC as one device (the distributor) > separate from the CPUs, with a bunch of separate devices attached to > each CPU and wired together somehow, or if you think of this as one big > coherent thing where parts of the device are specific to each CPU. I keep flip-flopping my view, which is why I keep flip-flopping my opinion on how to deal with its register API :-) > > I tend to interpret the GIC as the latter and I think the kernel and > userspace implementations are also done that way, suggesting we should > stick with the device API for all GIC-related state (as was also the > suggestion for the GICv3 save/restore API). I think the save/restore case is where I always flip to seeing it as a bunch of separate per cpu devices. It would feel better to me to save/restore the cpu-gic registers the same way we do all other cpu registers. I think we can get the best of both worlds by extending the SET/GET_DEVICE_ATTR to vcpus, and then using both the device ioctl and the vcpu ioctls. > > Similarly, because the GIC architecture refers to CPUs using the MPIDR, > we should do the same in this interface. Otherwise, I think you have to > define stricly how the exposed VCPU ID maps to an MPIDR somehow. Well, we're creating our own interface to a gic model, so there's nothing in the spec that says MPIDR is needed here, but if there's no good reason not to change the KVM_CREATE_VCPU vcpu-id to MPIDR, then we certainly wouldn't want to invent a new vcpu-id for this use. My current feelings are: 1) avoid needing vcpu identifiers (use vcpu ioctl whenever possible) 2) if we need them, they should match the same one used by KVM_CREATE_VCPU (whenever possible) 3) any device api that doesn't provide a 32-bit identifier field should just be special-cased, and have plenty of documentation explaining how to handle it - explaining what other limitations it puts on the guest, etc. None of those feelings exclude using MPIDR as _the_ vcpu-id (I don't think). It appears the main benefit of switching to MPIDR would be to decouple KVM from its userspaces wrt to choosing the non-MPIDR vcpu identification used for managing data structures and looping over vcpus internally. We also gain a clean way for userspace to choose the MPIDR for each vcpu, which we need to do anyway. Thanks, drew -- 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