Re: [RFC PATCH 04/25] hw/cxl/device: Introduce a CXL device (8.2.8)

[Jonathan Cameron]

On Mon, 16 Nov 2020 13:11:16 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 20-11-16 13:07:56, Jonathan Cameron wrote:
> > On Tue, 10 Nov 2020 21:47:03 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >   
> > > A CXL device is a type of CXL component. Conceptually, a CXL device
> > > would be a leaf node in a CXL topology. From an emulation perspective,
> > > CXL devices are the most complex and so the actual implementation is
> > > reserved for discrete commits.
> > > 
> > > This new device type is specifically catered towards the eventually
> > > implementation of a Type3 CXL.mem device, 8.2.8.5 in the CXL 2.0
> > > specification.
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> > 
> > As an RFC, would be good to have questions relavant to individual
> > patches if possible.  Makes it easier to know what you want feedback on.
> > 
> > The REG32 being used for 64 bit registers seems awkward. I'd suggest
> > we either break them up into DW and deal with the edge parts manually.
> > 
> > I'm not sure a REG64 definition would work due to lack of explicit alignment
> > guarantees.  Might be fine though.  
> 
> Agreed, although I think the current frequency with which I've had to do this,
> and the XXX comments are decent, it's definitely a bit ugly. I found at least
> two registers (I don't recall one, but the very important command register was
> the other that you noticed below) which have a field that straddles the 32b
> boundary. I think having to do an upper and lower field for that would kind of
> stink.
> 
> Given that the codebase has gone on long enough without REG64, I didn't want 
> to
> poke that bear, although I had wired it up at some point.
> 
> So for now, I'd like to just leave these as they are.
> 
> > 
> > One buglet inline and a few other comments.
> > 
> > Jonathan  
> 
> Thanks. Anything not responded to is acknowledged and will hopefully make its
> way into v2.
> 

...

> 
> >   
> > > +
> > > +/* 8.2.8.4.5.1 Command Return Codes */
> > > +enum {
> > > +    RET_SUCCESS                 = 0x0,
> > > +    RET_BG_STARTED              = 0x1, /* Background Command Started */
> > > +    RET_EINVAL                  = 0x2, /* Invalid Input */
> > > +    RET_ENOTSUP                 = 0x3, /* Unsupported */
> > > +    RET_ENODEV                  = 0x4, /* Internal Error */  
> > 
> > Mapping that to NODEV seems less than obvious.  
> 
> I tried to be cute and map as many things to errno as possible. Suggestions?

Don't bother being cute? :)
More seriously, I'd carry them as matching the spec out until you actually
have to return a standard error.   Fine to have a conversion function
that does a best possible mapping though so as to keep things consistent
across multiple locations.  Mind you perhaps qemu has a standard idiom for this?

cxl_cmd_ret_to_errno()


> 
> >   
> > > +    RET_ERESTART                = 0x5, /* Retry Required */
> > > +    RET_EBUSY                   = 0x6, /* Busy */
> > > +    RET_MEDIA_DISABLED          = 0x7, /* Media Disabled */
> > > +    RET_FW_EBUSY                = 0x8, /* FW Transfer in Progress */
> > > +    RET_FW_OOO                  = 0x9, /* FW Transfer Out of Order */
> > > +    RET_FW_AUTH                 = 0xa, /* FW Authentication Failed */
> > > +    RET_FW_EBADSLT              = 0xb, /* Invalid Slot */
> > > +    RET_FW_ROLLBACK             = 0xc, /* Activation Failed, FW Rolled 
> > > Back */
> > > +    RET_FW_REBOOT               = 0xd, /* Activation Failed, Cold Reset 
> > > Required */
> > > +    RET_ENOENT                  = 0xe, /* Invalid Handle */
> > > +    RET_EFAULT                  = 0xf, /* Invalid Physical Address */
> > > +    RET_POISON_E2BIG            = 0x10, /* Inject Poison Limit Reached */
> > > +    RET_EIO                     = 0x11, /* Permanent Media Failure */
> > > +    RET_ECANCELED               = 0x12, /* Aborted */
> > > +    RET_EACCESS                 = 0x13, /* Invalid Security State */
> > > +    RET_EPERM                   = 0x14, /* Incorrect Passphrase */
> > > +    RET_EPROTONOSUPPORT         = 0x15, /* Unsupported Mailbox */
> > > +    RET_EMSGSIZE                = 0x16, /* Invalid Payload Length */
> > > +    RET_MAX                     = 0x17
> > > +};
> > > +
> > > +/* XXX: actually a 64b register */
> > > +REG32(CXL_DEV_MAILBOX_STS, 0x10)
> > > +    FIELD(CXL_DEV_MAILBOX_STS, BG_OP, 0, 1)
> > > +    FIELD(CXL_DEV_MAILBOX_STS, ERRNO, 32, 16)
> > > +    FIELD(CXL_DEV_MAILBOX_STS, VENDOR_ERRNO, 48, 16)
> > > +
> > > +/* XXX: actually a 64b register */
> > > +REG32(CXL_DEV_BG_CMD_STS, 0x18)
> > > +    FIELD(CXL_DEV_BG_CMD_STS, BG, 0, 16)
> > > +    FIELD(CXL_DEV_BG_CMD_STS, DONE, 16, 7)
> > > +    FIELD(CXL_DEV_BG_CMD_STS, ERRNO, 32, 16)
> > > +    FIELD(CXL_DEV_BG_CMD_STS, VENDOR_ERRNO, 48, 16)
> > > +
> > > +REG32(CXL_DEV_CMD_PAYLOAD, 0x20)
> > > +
> > > +#endif  
> >