Re: [PATCH v3 3/4] hw/cxl: Fix and use same calculation for HDM decoder block size everywhere

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

On Wed, 13 Sep 2023 08:53:55 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 11/9/23 13:43, Jonathan Cameron wrote:
> > In order to avoid having the size of the per HDM decoder register block
> > repeated in lots of places, create the register definitions for HDM
> > decoder 1 and use the offset between the first registers in HDM decoder 0 and
> > HDM decoder 1 to establish the offset.
> > 
> > Calculate in each function as this is more obvious and leads to shorter
> > line lengths than a single #define which would need a long name
> > to be specific enough.
> > 
> > Note that the code currently only supports one decoder, so the bugs this
> > fixes don't actually affect anything. Previously the offset didn't
> > take into account that the write_msk etc are 4 byte fields.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > --
> > v3:
> > New patch to separate this out from the addition of HDM decoders.
> > ---
> >   include/hw/cxl/cxl_component.h |  2 ++
> >   hw/cxl/cxl-component-utils.c   | 19 +++++++++++--------
> >   hw/cxl/cxl-host.c              |  4 +++-
> >   hw/mem/cxl_type3.c             | 24 +++++++++++++++---------
> >   4 files changed, 31 insertions(+), 18 deletions(-)  
> 
> 
> > @@ -761,26 +763,30 @@ static void ct3_exit(PCIDevice *pci_dev)
> >   /* TODO: Support multiple HDM decoders and DPA skip */
> >   static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
> >   {
> > +    int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO;
> >       uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
> >       uint64_t decoder_base, decoder_size, hpa_offset;
> >       uint32_t hdm0_ctrl;
> >       int ig, iw;
> > +    int i = 0;
> >   
> > -    decoder_base = (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI] << 32) |
> > -                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO]);
> > +    decoder_base =
> > +        (((uint64_t)cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * hdm_inc] << 32) |
> > +                    cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * hdm_inc]);  
> 
> Alternatively easier to review as (matter of taste ?):
> 
> decoder_base = deposit64(cache_mem[R_CXL_HDM_DECODER0_BASE_LO + i * 
> hdm_inc], 32, 32,
>                           cache_mem[R_CXL_HDM_DECODER0_BASE_HI + i * 
> hdm_inc]);

I'll leave if for now for consistency in the CXL code.  Might make
sense to consider this as a cross subsystem cleanup at some point though!
Thanks for the suggestion.

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

Jonathan

> 
>