Re: [Xen-devel] [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi Julien,

On 31.01.18 00:06, Julien Grall wrote:
> On 30 January 2018 at 19:35, Volodymyr Babchuk
> <volodymyr_babchuk@xxxxxxxx> wrote:
> > On 30.01.18 20:44, Julien Grall wrote:
> > > On 30/01/18 18:28, Volodymyr Babchuk wrote:
> > > > Hi Julien,
> > > >
> > > > On 30.01.18 20:01, Julien Grall wrote:
> > > > > On 26/01/18 18:27, Volodymyr Babchuk wrote:
> > > > > > Hi,
> > > > >
> > > > > Hi Volodymyr,
> > > > >
> > > > > > On 26.01.18 20:15, Julien Grall wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 26/01/18 18:09, Volodymyr Babchuk wrote:
> > > > > > > > On 24.01.18 20:34, Julien Grall wrote:
> > > > > > > > > -    case PSCI_0_2_FN32(AFFINITY_INFO):
> > > > > > > > > -    case PSCI_0_2_FN64(AFFINITY_INFO):
> > > > > > > > > +    switch ( fid )
> > > > > > > > >        {
> > > > > > > > > -        register_t taff = PSCI_ARG(regs, 1);
> > > > > > > > > -        uint32_t laff = PSCI_ARG32(regs, 2);
> > > > > > > > > -
> > > > > > > > > -        perfc_incr(vpsci_cpu_affinity_info);
> > > > > > > > > -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff,
> > > > > > > > > laff));
> > > > > > > > > -        return true;
> > > > > > > > > -    }
> > > > > > > > > -
> > > > > > > > >        case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
> > > > > > > > >            return fill_function_call_count(regs,
> > > > > > > > > SSSC_SMCCC_FUNCTION_COUNT);
> > > > > > > > Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
> > > > > > > > Maybe it is time to introduce function get_psci_0_2_fn_count() and
> > > > > > > > use it there, what do you think?
> > > > > > >
> > > > > > > Definitely not a function. It is a static number. But I can think of
> > > > > > > separate the call count.
> > > > > > Yep, separate call count for vPSCI and for SSSC itself would be a good
> > > > > > thing.
> > > > >
> > > > > Looking a bit more into it, this will not make a real improvement. This
> > > > > will be equally difficult to remember to update the call count.
> > > > Nevertheless, I think that it is right thing to hold call count in the
> > > > same file, where calls are implemented. This increases chances that call
> > > > count will be held in sync.
> > >
> > > So you are suggesting to implement a function? If so, that's a no-go from
> > > my side.
> > I'm not insist on function if you can propose alternative.
> > But why you are against getter function in the first place?
> Because a function returning a const value is pretty dumb.
I can't agree with you there. In my opinion - if it is needed for 
clarity - then it is fine.
Nevertheless, you have my opinion. It is up to you what to do with it.

> > I wanted to propose another approach: define this call count in
> > vpsci.h, but there is no vpsci.h, instead you use psci.h, which is confusing
> > in itself.
> I thought about vpsci.h, but basically you will have only 2 functions in it and
> the number of PSCI calls. That's it.
Is  this really a problem? It is quite natural to find declarations for 
something.c in something.h. By moving declaration into different file, 
you are hiding it from anyone who does not carry sacred knowledge (or 
use grep/cscope, yes).
And then, when people decide to extend something.c they continue to put 
declarations into inappropriate.h. Just look at processor.h as a good 
example. All functions it define are implemented either in traps.c or 
domain.c. But functions from processor.c are defined in procinfo.h.
I can tell for sure, that this confuses newbies.


> So it is not going to help to update because the header will unlikely need to
> change when adding new PSCI call.
Yes. But at least we can put comment above switch(fid):

/* Please don't forget to update call count in (v)psci.h */


> [...]
>
> > > I looked at some implementation of SMCCC and those calls are either not
> > > handled or the number are not correct.
> > I agree that *some* implementations can not conform to SMCCC.But, I thought
> > you want Xen to follow standards as close as possible...
> It is not about cannot conform but only implements partially SMCCC. I could name
> ATF for that (yes).
>
> So it makes me more confident the call count is not something people seem to
> care.
So, you propose to fix this only when something will trip over this?

--
Volodymyr Babchuk

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel