Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

[Julien Grall <julien.grall@xxxxxxx>]

On 20/09/2017 19:11, Volodymyr Babchuk wrote:
> On 20.09.17 20:21, Julien Grall wrote:
> > On 19/09/17 22:44, Volodymyr Babchuk wrote:
> > > Hi Julien,
> > Hi Volodymyr,
> >
> > > On 13.09.17 14:11, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
> > > > > +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t
> *u)
> > > > Actually why do you pass a pointer for u? This requires every caller
> > > > to introduce temporary variable because the UUID is usually a define.
> > > Hmm, another way probably is to pass a whole structure as a parameter.
> > > Are you suggesting this approach? Something like
> > > fill_uuid(regs, (xen_uuid_t)MY_UUID)?
> > Something list that. But why do you need the cast? MY_UUID is supposed
> > to be a xen_uuid_t. No?
> It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
> you remember that thread, there is a requirement to make public headers
> compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
> Instead it is defined as a plain initializer list.
In that case why don't introduce a version for non-strict ansi? This 
would introduce a bit of safety and avoid cast a bit unexplained. (see 
how __DECL_REG(...,...) is done in include/public/asm-arm.h?
> > > > With your current solution each caller as to do:
> > > >
> > > > xen_uuid_t foo = MY_UUID;
> > > >
> > > > fill_uuid(regs, &foo);
> > > >
> > > > return true;
> > > >
> > > > What I suggested in the previous version is to get fill_uuid return
> > > > true. So you make each caller simpler.
> > > Yes, but it will not be correct semantically. There will arise many
> > > questions:
> > > 1. Why helper function that only writes data returns bool?
> > > 2. If it returns true, can it return false?
> > > 3. Should we check its return value before passing it further?
> >
> > I really don't see how
> >
> > return fill_uuid(regs, MY_UUID);
> >
> > would be semantically incorrect or even raise all those questions. It is
> > perfectly fine to always return true. We have place like that and it
> > helps to streamline the code.This is a somewhat arguable topic.
> Yes, your variant produces less lines of code.
> But it is harder to read, actually. `return fill_uid(regs, MY_UUID)`
> implies that there are some logic in `fill_uid()` and it can return
> different results, depending on its parameters. Which is not true and,
> thus, misleading.
> Just try to look at this from stranger's point of view. Usually you
> don't declare function as a `bool` just to spare a line of code. If you
> see boolean function, you expect that there are some reason, some logic
> behind this.
You didn't get my point. When doing emulation, most of the time case in 
a switch have similar prototype. It gets an input and may (or may not) 
return an error. It is easier to think of each case with the same 
prototype rather than using different one for the sake of avoiding using 
bool because you would always return true. It can also help to switch to 
an array instead.
> Look, I have an idea how to resolve this.
> fill_uid() will check that MY_UUID != {ffffffff, fffff, fffff, ffff,
> ffffffffffff}.
> If UUID is invalid, it will print warning to console and return false.
> If UUID is valid, it will fill registers and return true.
> Now it will be semantically correct to define it as `bool fill_uuid()`
Just no. UUID are coming from the hypervisor and most of the time (if 
not always) static. There are clearly no point to always check the UUID 
is valid when it is likely is.
> This can work for `fill_uuid()`. But you also expressed the same idea
> regarding code that return version. I can create helper function that
> fills registers with version info. But there I don't see any excuse to
> declare that helper as boolean.
The idea is you are going to abstract each case and make easier to 
potentially use an array rather than switch in some cases. So you want 
to return bool for every function.
Anyway, there are other bugs to fix and it seems unhelpful to argue 
here. I will write a follow-up because I don't like the idea of trying 
to adapt prototype just because "it does not make sense to return true...".
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel