On Tue, Nov 04, 2014 at 08:17:07PM +0000, Marc Zyngier wrote: > 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). > I don't think it's that bad. It would be just like pud_free() and friends which ignore the struct mm * parameter. But anyhow, if you feeel strongly about one way or the other, then go with it. I've said my piece. -Christoffer _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
