On 04/11/14 19:18, Christoffer Dall wrote: > On Tue, Nov 04, 2014 at 06:05:17PM +0000, Marc Zyngier wrote: >> On 04/11/14 17:24, Andre Przywara wrote: >>> Hi, >>> >>> On 04/11/14 15:44, Christoffer Dall wrote: >>>> On Fri, Oct 31, 2014 at 05:26:50PM +0000, Andre Przywara wrote: >>>>> For a GICv2 there is always only one (v)CPU involved: the one that >>>>> does the access. On a GICv3 the access to a CPU redistributor is >>>>> memory-mapped, but not banked, so the (v)CPU affected is determined by >>>>> looking at the MMIO address region being accessed. >>>>> To allow passing the affected CPU into the accessors, extend them to >>>>> take an opaque private pointer parameter. >>>>> For the current GICv2 emulation we ignore it and simply pass NULL >>>>> on the call. >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> >>>> >>>> Why does it have to be an opaque private pointer? Would it not always >>>> be a struct vcpu * or a vcpu_id then? >>> >>> IIRC Marc suggested this once be more future proof. Also a pointer makes >>> it easier to pass NULL in the GICv2 parts of the code, which makes it >>> more obvious that this value is not used in this case. >>> >>> Marc, did I miss some more rationale? >>> Does that still hold? >> >> The main idea was to have a general purpose pointer that you can >> associate with the decoded region. Some form of private context, just >> like we have for a lot of other kernel structures. >> >> Now, I think having that as a explicit pointer looks truly awful. Can't >> that be folded into struct kvm_exit_mmio that is already passed around? >> It would make some sense that the private context is associated with the >> actual access... I haven't seen how that interacts with the GICv3 code >> though. >> > Well, the idea with a (void *private) is to have something, which is > *generic* be reusable and extendable, no argument there. > > So my question is, are we implementing some generic feature, where > having that extendability makes things better and clearer, or are we > just wrapping an int in a (void *) so we don't have to add another > parameter if sometime in the unknown future we need another additional > piece of information. > > There are plenty of examples where you just pass NULL to a typed pointer > or 0 to an int parameter as well. > > I'm not trying to fight the idea of a private pointer, I just want to > make sure we do what we can to keep this code somewhat sane, so if we > have a set of functions where we in 75% of the cases pass a vcpu * and > in the other cases don't, then I really think we want a vcpu * > parameter. For the time being, I don't see any other use than a vcpu pointer for the GICv3 case. Now, none of the MMIO decoding framework is GICv3 specific, and it feels a bit weird to hardcode the idea of a vcpu pointer being passed around for code that doesn't really care about it (GICv2). Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
