Re: [PATCH v4 02/10] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support

[fan]

On Mon, Mar 04, 2024 at 12:40:18PM +0000, Jørgen Hansen wrote:
> On 2/21/24 19:15, nifan.cxl@gmail.com wrote:
> > CAUTION: This email originated from outside of Western Digital. Do not 
> > click on links or open attachments unless you recognize the sender and know 
> > that the content is safe.
> > 
> > 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > Per cxl spec r3.1, add dynamic capacity region representative based on
> > Table 8-165 and extend the cxl type3 device definition to include dc region
> > information. Also, based on info in 8.2.9.9.9.1, add 'Get Dynamic Capacity
> > Configuration' mailbox support.
> > 
> > Note: decode_len of a dc region is aligned to 256*MiB, divided by
> > 256 * MiB before returned to the host for "Get Dynamic Capacity 
> > Configuration"
> > mailbox command.
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >   hw/cxl/cxl-mailbox-utils.c  | 110 ++++++++++++++++++++++++++++++++++++
> >   include/hw/cxl/cxl_device.h |  16 ++++++
> >   2 files changed, 126 insertions(+)
> > 
> 
> [snip]
> 
> > 
> >       /* No separation for PCI MB as protocol handled in PCI device */
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 3cf3077afa..6df7fecdf1 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -422,6 +422,17 @@ typedef struct CXLPoison {
> >   typedef QLIST_HEAD(, CXLPoison) CXLPoisonList;
> >   #define CXL_POISON_LIST_LIMIT 256
> > 
> > +#define DCD_MAX_REGION_NUM 8
> > +
> > +typedef struct CXLDCDRegion {
> > +    uint64_t base;
> > +    uint64_t decode_len; /* aligned to 256*MiB */
> > +    uint64_t len;
> > +    uint64_t block_size;
> > +    uint32_t dsmadhandle;
> > +    uint8_t flags;
> > +} CXLDCDRegion;
> > +
> 
> To be consistent with the naming of extents, and functions defined later 
> on, shouldn't this be CXLDCRegion (DC instead of DCD)?
> 

I was also thinking about it and a bit lazy to make the change until
someone points out. Now I have a good reason to make the change :-).
Thanks.

Fan

> >   struct CXLType3Dev {
> >       /* Private */
> >       PCIDevice parent_obj;
> > @@ -454,6 +465,11 @@ struct CXLType3Dev {
> >       unsigned int poison_list_cnt;
> >       bool poison_list_overflowed;
> >       uint64_t poison_list_overflow_ts;
> > +
> > +    struct dynamic_capacity {
> > +        uint8_t num_regions; /* 0-8 regions */
> > +        CXLDCDRegion regions[DCD_MAX_REGION_NUM];
> > +    } dc;
> >   };
> > 
> >   #define TYPE_CXL_TYPE3 "cxl-type3"
> > --
> > 2.43.0
> > 
> > 
> 
> Thanks,
> Jorgen