Re: [PATCH v2 2/4] hw/cxl/device: read from register values in mdev_reg_read()

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>, Fan Ni <fan.ni@samsung.com>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	<linux-cxl@vger.kernel.org>, <qemu-devel@nongnu.org>
Subject: Re: [PATCH v2 2/4] hw/cxl/device: read from register values in mdev_reg_read()
Date: Thu, 11 Jan 2024 12:32:50 +0000	[thread overview]
Message-ID: <20240111123250.00007748@Huawei.com> (raw)
In-Reply-To: <20240109174550.00000f6c@Huawei.com>

On Tue, 9 Jan 2024 17:45:50 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 22 Dec 2023 18:00:49 +0900
> Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
> 
> > In the current mdev_reg_read() implementation, it consistently returns
> > that the Media Status is Ready (01b). This was fine until commit
> > 25a52959f99d ("hw/cxl: Add support for device sanitation") because the
> > media was presumed to be ready.
> > 
> > However, as per the CXL 3.0 spec "8.2.9.8.5.1 Sanitize (Opcode 4400h)",
> > during sanitation, the Media State should be set to Disabled (11b). The
> > mentioned commit correctly sets it to Disabled, but mdev_reg_read()
> > still returns Media Status as Ready.
> > 
> > To address this, update mdev_reg_read() to read register values instead
> > of returning dummy values.
> > 
> > Fixes: commit 25a52959f99d ("hw/cxl: Add support for device sanitation")
> > Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>  
> 
> I've applied this one to my tree.  (I'll push that out in a day or two after
> tidying up some other outstanding stuff). 
> 
> Sometime in next week or so I'll send out a set bundling together various
> fixes and cleanup with the intent for getting it applied.

I've changed how this works because what this is doing as presented is
overwriting the mailbox capability register.  mbox_reg_state64 is as the
name should have made obvious to reviewers such as me, the mailbox registers!

Anyhow, I'll put an alternative fix in place.

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  hw/cxl/cxl-device-utils.c   | 17 +++++++++++------
> >  include/hw/cxl/cxl_device.h |  4 +++-
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> > index 29de298117..ba3f80e6e7 100644
> > --- a/hw/cxl/cxl-device-utils.c
> > +++ b/hw/cxl/cxl-device-utils.c
> > @@ -229,12 +229,9 @@ static void mailbox_reg_write(void *opaque, hwaddr offset, uint64_t value,
> >  
> >  static uint64_t mdev_reg_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> > -    uint64_t retval = 0;
> > -
> > -    retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MEDIA_STATUS, 1);
> > -    retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MBOX_READY, 1);
> > +    CXLDeviceState *cxl_dstate = opaque;
> >  
> > -    return retval;
> > +    return cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS];
> >  }
> >  
> >  static void ro_reg_write(void *opaque, hwaddr offset, uint64_t value,
> > @@ -371,7 +368,15 @@ static void mailbox_reg_init_common(CXLDeviceState *cxl_dstate)
> >      cxl_dstate->mbox_msi_n = msi_n;
> >  }
> >  
> > -static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { }
> > +static void memdev_reg_init_common(CXLDeviceState *cxl_dstate)
> > +{
> > +    uint64_t memdev_status_reg;
> > +
> > +    memdev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, 1);
> > +    memdev_status_reg = FIELD_DP64(memdev_status_reg, CXL_MEM_DEV_STS,
> > +                                   MBOX_READY, 1);
> > +    cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = memdev_status_reg;
> > +}
> >  
> >  void cxl_device_register_init_t3(CXLType3Dev *ct3d)
> >  {
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index b2cb280e16..b318d94b36 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -408,7 +408,9 @@ static inline void __toggle_media(CXLDeviceState *cxl_dstate, int val)
> >  {
> >      uint64_t dev_status_reg;
> >  
> > -    dev_status_reg = FIELD_DP64(0, CXL_MEM_DEV_STS, MEDIA_STATUS, val);
> > +    dev_status_reg = cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS];
> > +    dev_status_reg = FIELD_DP64(dev_status_reg, CXL_MEM_DEV_STS, MEDIA_STATUS,
> > +                                val);
> >      cxl_dstate->mbox_reg_state64[R_CXL_MEM_DEV_STS] = dev_status_reg;
> >  }
> >  #define cxl_dev_disable_media(cxlds)                    \  
> 
> 