Re: Fwd: [PATCH v5] xen/arm : emulation of arm's PSCI v0.2 standard

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-07-25 at 12:40 +0530, Parth Dixit wrote:

> >
> > I notice that sometimes you have an else case setting
> > PSCI_INVALID_PARAMETERS and other times you don't. Is that just an
> > oversight or is there a reason why they should differ?

> It was done because rest of the functions were using default preloaded
> value with return type int32 , functions with else case are functions
> that have return type other than int32(uint32 or register_t)  but as
> you have commented further if PSCI* result codes do not require
> casting, this will go away.
> To summarize i will remove the casting of result codes this will also
> do away the need of else statements in patchset v6.

Sounds good.

> >> +
> >> +    if ( is_32bit_domain(d) )
> >> +    {
> >> +        regs->pc32 = entry_point;
> >> +        regs->r0 = context_id;
> >> +    }
> >> +#ifdef CONFIG_ARM_64
> >> +    else
> >> +    {
> >> +        regs->pc = entry_point;
> >> +        regs->x0 = context_id;
> >> +    }
> >> +#endif
> >> +    vcpu_block_unless_event_pending(v);
> >> +    return PSCI_SUCCESS;
> >
> > This only seems to be handling the case where the requested power state
> > is power down and not the standby state.
> >
> > By my reading when bit 16 if the power_state argument is 0 (standby)
> > then entry_point and context_id are ignored and the PSCI call is
> > expected to return success to the place it was called from, not jump to
> > entry_point.
> >
> > If bit 16 is one you are supposed to return to entry_point with x0/r0
> > set to context_id, but because you return PSCI_SUCCESS that will get
> > written to x0/r0 by the generic handler clobbering your write of x0/r0
> > above. You should probably return the desired value (with an explanatory
> > comment)

> valid point, i am just wondering why kvm implementation ignored these values,

It's possible that I'm being overly picky in reading the spec.

My concern is that there isn't currently much in the way of reference
implementations of this thing to compare against for real world
behaviour. When such things turn up and behave differently to Xen (or
KVM for that matter), perhaps because they interpret the ambiguities in
the spec differently, and the spec is clarified we may end up having to
change. Which then runs the risk of regressions for existing guests etc.

> i will implement it in next patchset

Thanks.

> > The upper bits of power_state also describe an affinity level to put to
> > sleep, which you also don't handle AFAICT. Perhaps since we don't
> > actually have affinities > 0 what you've done is OK, what do you think?

> I think it is better to put a comment explicitly stating that we have
> ignored the value because affinities > 0 are not valid for xen or we
> can check for affinity and return if it is greater than zero.
> I think i'll go with check, will take care in next patchset

I'm not 100% what the spec allows/requires us to do here. I've asked
ARM. I suspect shutting down only the local core would be permissible.

Ian.