Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)

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

> >>
> >> +static const struct CXLLogCapabilities cxl_get_log_cap[MAX_LOG_TYPE] = {
> >> +    [CEL] = {.param_flags.pflags = CXL_LOG_CAP_CLEAR | CXL_LOG_CAP_POPULATE,
> >> +             .uuid = cel_uuid},
> >> +};
> >> +
> >> +static void cxl_init_get_log(CXLCCI *cci)
> >> +{
> >> +    for (int set = CEL; set < MAX_LOG_TYPE; set++) {
> >> +        cci->supported_log_cap[set] = cxl_get_log_cap[set];  
> >
> >As below. Can we just use a static const pointer in cci?  
> 
> static const pointer in cci gives compilation error as it used below
> to assign value:
>      cci->supported_log_cap[set] = cxl_get_log_cap[set];

True. That is what I am suggesting changing.

cci->supported_log_cap = cxl_get_log_cap;

This is currently iterating over a list and copying everything. Why
bother, just set a pointer to the source list.  If there
are choices for that, pick between them but keep all those lists const.



> 

> >> +typedef struct CXLLogCapabilities {
> >> +    CXLParamFlags param_flags;
> >> +    QemuUUID uuid;
> >> +} CXLLogCapabilities;
> >> +
> >>  typedef struct CXLCCI {
> >>      struct cxl_cmd cxl_cmd_set[256][256];
> >>      struct cel_log {
> >> @@ -202,6 +230,9 @@ typedef struct CXLCCI {
> >>      } cel_log[1 << 16];
> >>      size_t cel_size;
> >>
> >> +    /* get log capabilities */
> >> +    CXLLogCapabilities supported_log_cap[MAX_LOG_TYPE];  
> >Perhaps a const pointer is appropriate?  
> 
> const pointer here gives compilation error as it is used below 
> to assign value:
>      cci->supported_log_cap[set] = cxl_get_log_cap[set];
As above.  This set is the thing I'm suggesting changing.

One general review process thing is it is much more productive
to just crop anything you agree with and just have the discussion
focused on questions etc that are outstanding.

Jonathan