Re: [PATCH 1/3] hw/cxl: Storing physical ports info during enumeration

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

> >> +#define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
> >> +#define LINK_STATE_FLAG_PRSNT            (1 << 2)
> >> +#define LINK_STATE_FLAG_POWER_OFF        (1 << 3)
> >> +
> >> +/* physical port control info - CXL r3.2 table 7-19 */
> >> +typedef enum {
> >> +    PORT_DISABLED = 0,
> >> +    BIND_IN_PROGRESS = 1,
> >> +    UNBIND_IN_PROGRESS = 2,
> >> +    DSP = 3,
> >> +    USP = 4,
> >> +    FABRIC_PORT = 5,
> >> +    INVALID_PORT_ID = 15
> >> +} current_port_config_state;  
> >
> >These aren't used as types really but more as defines.
> >Hence I'd be tempted to make them defines. Also provide
> >defines for the masks.
> >
> >Namespace the defines so it is obvious which field
> >they are in.
> >  
> Will the below changes be the right way to proceed?
> #define CXL_PORT_CONFIG_STATE_DISABLED           0x0
> #define CXL_PORT_CONFIG_STATE_BIND_IN_PROGRESS   0x1 etc.

Yes


...

> >For similar things we do have firmware update in there which
> >is a mixture of device side stuff and CCI specific handling.
> >That might ideally be split up.  Obviously not something for
> >this patch, but maybe we can avoid making CCI more bloated?
> >
> >To me it seems reasonable to store this in the port and set
> >it up whether or not the cci is connected.
> >  
> got it, will try the implementation accordingly. It will be helpful if could share some
> reference on handling the same.

+    /*physical ports information */
+    struct phy_port pports;
+

Move this stuff to the CXLUpstreamPort.
Query the data to fill it in will need to be late enough that
everything is there to query. cxl_usp_reset() might work similar to how
we fill in link registers etc there. 

Then when you handle any of the CCI calls, use CXL_USP(cci->d) to
get to that USP structure.  Check that you have the right type if necessary
first though. 

Jonathan




> >> +
> >>      /* background command handling (times in ms) */
> >>      struct {
> >>          uint16_t opcode;  
> >  
>