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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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]);

Regardless:

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