[v1] hw/cxl: Support emulating 4 HDM decoders throughout topology

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

Posted by Jonathan Cameron via 1 year, 2 months ago

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;
+}
+
 uint8_t cxl_interleave_ways_enc(int iw, Error **errp);
+int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp);
 uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp);
 
 static inline hwaddr cxl_decode_ig(int ig)
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 378f1082ce..e96398e8af 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -392,6 +392,23 @@ uint8_t cxl_interleave_ways_enc(int iw, Error **errp)
     }
 }
 
+int cxl_interleave_ways_dec(uint8_t iw_enc, Error **errp)
+{
+    switch (iw_enc) {
+    case 0x0: return 1;
+    case 0x1: return 2;
+    case 0x2: return 4;
+    case 0x3: return 8;
+    case 0x4: return 16;
+    case 0x8: return 3;
+    case 0x9: return 6;
+    case 0xa: return 12;
+    default:
+        error_setg(errp, "Encoded interleave ways: %d not supported", iw_enc);
+        return 0;
+    }
+}
+
 uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp)
 {
     switch (gran) {
-- 
2.39.2

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

Posted by Philippe Mathieu-Daudé 1 year, 2 months ago

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?

Alternatively:

   unsigned cxl_decoder_count_dec(unsigned enc_cnt)
   {
       return enc_cnt <= 5 ? 2 * enc_cnt : 0;
   }

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

Posted by Jonathan Cameron via 1 year, 2 months ago

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.


> 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.

>    }
> 
>

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

Posted by Jonathan Cameron via 1 year, 2 months ago

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.


> 
> 
> > 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.
> 
> >    }
> > 
> >   
> 
> 
>

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

Posted by Philippe Mathieu-Daudé 1 year, 2 months ago

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>

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

Posted by Jonathan Cameron via 1 year, 2 months ago

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