On Mon, 16 Nov 2020 13:11:16 -0800
Ben Widawsky <ben.widaw...@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.widaw...@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.widaw...@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
> >
