types: Live Migration #913
Conversation
NateThornton
force-pushed
the
lm_types
branch
3 times, most recently
from
300f72e
to
4a2effd
Compare
src/nvme/types.h
Outdated
| NVME_LM_LOG_USER_DATA_CHANGES = 0, | ||
| NVME_LM_TRACK_MEMORY_CHANGES = 1, | ||
| NVME_LM_TRACK_SEND_SELECT_SHIFT = 0, | ||
| NVME_LM_TRACK_SEND_SELECT_MASK = 16, |
src/nvme/types.h
Outdated
| NVME_LM_CREATE_CDQ = 0, | ||
| NVME_LM_DELETE_CDQ = 1, | ||
| NVME_LM_CDQ_SELECT_SHIFT = 0, | ||
| NVME_LM_CDQ_SELECT_MASK = 16, |
There was a problem hiding this comment.
Also please use the SEL instead the Select. This makes it consistent with the recently added field names (and usually better searchable in the spec). Also is there a reason you don't add the MOS (looking at the 2.1 spec)
src/nvme/types.h
Outdated
| */ | ||
| enum nvme_lm_cdq_fields { | ||
| NVME_LM_CREATE_CDQ = 0, | ||
| NVME_LM_DELETE_CDQ = 1, |
There was a problem hiding this comment.
Use the infix SEL, e.g.NVME_LM_SEL_CREATE_CDQ . I prefer to keep the naming structure in sync with the spec. Having variations and interpretation is making it really hard to find the spot in the spec.
src/nvme/types.h
Outdated
| NVME_LM_CDQ_SELECT_MASK = 16, | ||
| NVME_LM_QUEUE_TYPE_USER_DATA_MIGRATION_QUEUE = 0, | ||
| NVME_LM_QUEUE_TYPE_SHIFT = 16, | ||
| NVME_LM_QUEUE_TYPE_MASK = 0xffff, |
There was a problem hiding this comment.
and here I'd say it should match the spec and be prefixed NVME_LM_QT_...
src/nvme/types.h
Outdated
| NVME_LM_QUEUE_TYPE_SHIFT = 16, | ||
| NVME_LM_QUEUE_TYPE_MASK = 0xffff, | ||
| NVME_LM_CREATE_CDQ_PC = 1, | ||
| NVME_LM_CREATE_CDQ_CNTLID_SHIFT = 16, |
There was a problem hiding this comment.
Should this value not be 0, Figure 166 User Data Migration Queue -- Create Queue Specific
There was a problem hiding this comment.
This is where I wasn't sure - should they be absolute bit position in the CDW, or relative to the parent field - Management Specific Operation (MOS) in this case.
I'll proceed more closely to the spec - if the fields are presented as relative to MOS then the shift and mask values will reflect that as well.
src/nvme/types.h
Outdated
| */ | ||
| enum nvme_lm_track_send_fields { | ||
| NVME_LM_LOG_USER_DATA_CHANGES = 0, | ||
| NVME_LM_TRACK_MEMORY_CHANGES = 1, |
src/nvme/types.h
Outdated
| */ | ||
| enum nvme_lm_migration_send_fields { | ||
| NVME_LM_SUSPEND = 0, | ||
| NVME_LM_RESUME = 1, |
There was a problem hiding this comment.
and 2.1 lists a 2h Set Controller State and MOS
src/nvme/types.h
Outdated
| NVME_LM_DUDMQ = 1, | ||
| NVME_LM_DUDMQ_SHIFT = 31, | ||
| NVME_LM_DUDMQ_MASK = 0x1, | ||
| NVME_LM_STYPE_NOTIFICATION = 0, |
There was a problem hiding this comment.
The 2.1 spec calls this Suspend Notification
| NVME_LM_STYPE_SUSPEND = 1, | ||
| NVME_LM_STYPE_SHIFT = 16, | ||
| NVME_LM_STYPE_MASK = 0xff, | ||
|
|
There was a problem hiding this comment.
For Migration Send, CNTLID is common to all SEL types. See NVME_LM_MIGRATION_SEND_CNTLID_SHIFT above.
I could break them up so they read more closely with the spec, but then I would have 3 CNTLID fields - is that preferred?
| NVME_LM_MIGRATION_SEND_CSUUIDI_SHIFT = 24, | ||
| NVME_LM_MIGRATION_SEND_CSUUIDI_MASK = 0xff, | ||
| NVME_LM_MIGRATION_SEND_CSVI_SHIFT = 16, | ||
| NVME_LM_MIGRATION_SEND_CSVI_MASK = 0xff, |
There was a problem hiding this comment.
CNTLID is also mentioned in 2.1
src/nvme/types.h
Outdated
| * completion dword0 of Migration Receive command | ||
| */ | ||
| enum nvme_lm_migration_recv_fields { | ||
| NVME_LM_GET_CONTROLLER_STATE = 0, |
src/nvme/types.h
Outdated
| NVME_LM_MIGRATION_RECV_SEL_SHIFT = 0, | ||
| NVME_LM_MIGRATION_RECV_SEL_MASK = 0xff, | ||
| NVME_LM_MIGRATION_RECV_CNTLID_SHIFT = 0, | ||
| NVME_LM_MIGRATION_RECV_CNTLID_MASK = 0xffff, |
There was a problem hiding this comment.
shouldn't this be grouped withe the CSUIDXP and CSIUUDI fields?
| NVME_LM_MIGRATION_RECV_CTRL_SUSPENDED_SHIFT = 0, | ||
| NVME_LM_MIGRATION_RECV_CTRL_SUSPENDED_MASK = 0x1, | ||
| }; | ||
|
|
There was a problem hiding this comment.
One general comment here is that it would be way easier to review if the definition order would match the order in the spec. E.g CSVI, CSUIDXP, CSUUIDI, CNTLID.
src/nvme/types.h
Outdated
| NVME_LM_MIGRATION_RECV_CSVI_SHIFT = 16, | ||
| NVME_LM_MIGRATION_RECV_CSVI_MASK = 0xff, | ||
| NVME_LM_MIGRATION_RECV_CTRL_SUSPENDED = 1, | ||
| NVME_LM_MIGRATION_RECV_CTRL_SUSPENDED_SHIFT = 0, |
There was a problem hiding this comment.
Is CTRL_SUSPENDED called CSUP in the spec?
|
Comments reviewed - thanks for detailed scrutiny. I hope to have the changes ready by end of week. Will resolve with #916 too. |
|
Thanks a lot. Yeah, I went down the rabbit hole :) |
Continues efforts from the newly minted TP4159 PCIe Infrastructure for Live Migration specification by adding additional types. Signed-off-by: Nate Thornton <[email protected]>
NateThornton
force-pushed
the
lm_types
branch
3 times, most recently
from
f2da12d
to
0aedaba
Compare
Continues efforts from the newly minted TP4159 PCIe Infrastructure for Live Migration specification by adding additional types.