[PATCH v3 15/19] arm/arm64: KVM: add opaque private pointer to MMIO accessors

[Christoffer Dall]

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 at arm.com>
> >>>>
> >>>> 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