Re: [PATCH v3] hw/cxl: Fix missing write mask for HDM decoder target list registers

[Ben Widawsky]

On 22-06-07 17:37:02, Jonathan Cameron wrote:
> On Tue, 7 Jun 2022 09:19:28 -0700
> Ben Widawsky <bwidawsk@kernel.org> wrote:
> 
> > On 22-06-07 17:07:47, Jonathan Cameron wrote:
> > > Without being able to write these registers, no interleaving is possible.
> > > More refined checks of HDM register state on commit to follow.
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > > v3: Actually pass the parameter to the call...
> > > v2: (Ben Widawsky)
> > > - Correctly set a tighter write mask for the endpoint devices where this
> > >   register has a different use.
> > >   
> > >  hw/cxl/cxl-component-utils.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > index 7985c9bfca..2208284ee6 100644
> > > --- a/hw/cxl/cxl-component-utils.c
> > > +++ b/hw/cxl/cxl-component-utils.c
> > > @@ -154,7 +154,8 @@ static void ras_init_common(uint32_t *reg_state, 
> > > uint32_t *write_msk)
> > >      reg_state[R_CXL_RAS_ERR_CAP_CTRL] = 0x00;
> > >  }
> > >  
> > > -static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk)
> > > +static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
> > > +                            enum reg_type type)
> > >  {
> > >      int decoder_count = 1;
> > >      int i;
> > > @@ -174,6 +175,12 @@ static void hdm_init_common(uint32_t *reg_state, 
> > > uint32_t *write_msk)
> > >          write_msk[R_CXL_HDM_DECODER0_SIZE_LO + i * 0x20] = 0xf0000000;
> > >          write_msk[R_CXL_HDM_DECODER0_SIZE_HI + i * 0x20] = 0xffffffff;
> > >          write_msk[R_CXL_HDM_DECODER0_CTRL + i * 0x20] = 0x13ff;
> > > +        if (type == CXL2_DEVICE) {
> > > +            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > > 0xf0000000;
> > > +        } else {
> > > +            write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_LO + i * 0x20] = 
> > > 0xffffffff;
> > > +        }
> > > +        write_msk[R_CXL_HDM_DECODER0_TARGET_LIST_HI + i * 0x20] = 
> > > 0xffffffff;  
> > 
> > Should it be (type == CXL2_DEVICE || type == CXL2_TYPE3_DEVICE) ?
> 
> Good point, but also for consistency I think we need 
> type == CXL2_LOGICAL_DEVICE as well.

I was looking at this and I am not sure, but I defer to you.

> 
> We will only exercise the match to CXL2_TYPE3_DEVICE currently
> as we don't have any emulation for MLDs (and hence LD) or type 1/2 devices
> (CXL2_DEVICE).
> 
> I'll send a v4 out tomorrow.
> 

Sounds good, feel free to keep the r-b tag.

> > 
> > Otherwise,
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > >      }
> > >  }
> > >  
> > > @@ -239,7 +246,7 @@ void cxl_component_register_init_common(uint32_t 
> > > *reg_state, uint32_t *write_msk
> > >      }
> > >  
> > >      init_cap_reg(HDM, 5, 1);
> > > -    hdm_init_common(reg_state, write_msk);
> > > +    hdm_init_common(reg_state, write_msk, type);
> > >  
> > >      if (caps < 5) {
> > >          return;
> > > -- 
> > > 2.32.0
> > >   
>