On Tue, Nov 04, 2014 at 08:17:07PM +0000, Marc Zyngier wrote:

On 04/11/14 19:18, Christoffer Dall wrote:

quoted

On Tue, Nov 04, 2014 at 06:05:17PM +0000, Marc Zyngier wrote:

quoted

On 04/11/14 17:24, Andre Przywara wrote:

quoted

Hi,

On 04/11/14 15:44, Christoffer Dall wrote:

quoted

On Fri, Oct 31, 2014 at 05:26:50PM +0000, Andre Przywara wrote:

quoted

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@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