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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

On 20.09.17 23:02, Julien Grall wrote:
> 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?
I believe you meant arch-arm.h.

Just to be clear, you are proposing to introduce one
#define XEN_DEFINE_UUID in a public header, and another one in a private 
header?
Public one will be strictly ANSI-compatible, while private one will be 
only gcc-compatible? That is doable, but it will require #ifdef magic 
across different headers.
If other maintainers are okay with that, I can do it in this way.

> > > > > 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.
Aha, looks like  I'm beginning to see what you are mean. Okay I'll make 
those helpers to return bool.
> > 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.
Good point

> > 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...".
Okay, I think I got your idea. And, anyways, you are maintainer :)
Will do as you say.


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