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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 5 Sep 2023 18:55:23 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> 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

I'll push this and the enc() version down into the cxl-component-utils.c
as a precursor patch.

> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> 
Thanks, but the changes to do make these non inline, and include
the larger decode and encode values are big enough I won't pick up
the RB - too much changing (that I might mess up ;)


Jonathan