Re: [PATCH 1/2] hw/cxl: Add utility functions decoder interleave ways and target count.

[Philippe Mathieu-Daudé]

On 5/9/23 17:06, Jonathan Cameron wrote:
> On Tue, 5 Sep 2023 15:56:39 +0100
> Jonathan Cameron via <qemu-devel@nongnu.org> wrote:
>
> > On Mon, 4 Sep 2023 20:26:59 +0200
> > Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > > On 4/9/23 18:47, Jonathan Cameron wrote:
> > > > As an encoded version of these key configuration parameters is
> > > > a register, provide functions to extract it again so as to avoid
> > > > the need for duplicating the storage.
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > >    include/hw/cxl/cxl_component.h | 14 ++++++++++++++
> > > >    hw/cxl/cxl-component-utils.c   | 17 +++++++++++++++++
> > > >    2 files changed, 31 insertions(+)
> > > >
> > > > diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> > > > index 42c7e581a7..f0ad9cf7de 100644
> > > > --- a/include/hw/cxl/cxl_component.h
> > > > +++ b/include/hw/cxl/cxl_component.h
> > > > @@ -238,7 +238,21 @@ static inline int cxl_decoder_count_enc(int count)
> > > >        return 0;
> > > >    }
> > > >    
> > > > +static inline int cxl_decoder_count_dec(int enc_cnt)
> > > > +{
> > > > +    switch (enc_cnt) {
> > > > +    case 0: return 1;
> > > > +    case 1: return 2;
> > > > +    case 2: return 4;
> > > > +    case 3: return 6;
> > > > +    case 4: return 8;
> > > > +    case 5: return 10;
> > > > +    }
> > > > +    return 0;
> > > > +}
> > >
> > > Why inline?
> > >    
> >
> > Bad habit.
> Nope. I'm being slow.  This is in a header so if I don't
> mark it inline I get a bunch of defined but not used warnings.
>
> Obviously I could move the implementation of this and the matching
> encoding routines out of the header. I haven't done so for now.

Inlined function in hw/ are hardly justifiable. They make the headers
and debugging sessions harder to read in my experience. Compilers are
becoming clever and clever, and we have LTO, so I rather privilege
code maintainability. My 2 cents :)

> > > Alternatively:
> > >
> > >     unsigned cxl_decoder_count_dec(unsigned enc_cnt)
> > >     {
> > >         return enc_cnt <= 5 ? 2 * enc_cnt : 0;
> >
> > It gets a little more fiddly than the code I'm proposing implies.
> > For Switches and Host Bridges larger values are defined
> > (we just don't emulate them yet and may never do so) and those
> > don't have a sensible mapping.
> >
> > I guess there is no harm in adding the full decode however
> > which will make it more obvious why it was a switch statement.

Right, no problem.

Preferably having this tiny function not inlined:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>