On Jul 29 16:17, Maxim Levitsky wrote: > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote: > > From: Klaus Jensen <k.jen...@samsung.com> > > > > Since the device does not have any persistent state storage, no > > features are "saveable" and setting the Save (SV) field in any Set > > Features command will result in a Feature Identifier Not Saveable status > > code. > > > > Similarly, if the Select (SEL) field is set to request saved values, the > > devices will (as it should) return the default values instead. > > > > Since this also introduces "Supported Capabilities", the nsid field is > > now also checked for validity wrt. the feature being get/set'ed. > > > > Signed-off-by: Klaus Jensen <k.jen...@samsung.com> > > --- > > hw/block/nvme.c | 103 +++++++++++++++++++++++++++++++++++++----- > > hw/block/trace-events | 4 +- > > include/block/nvme.h | 27 ++++++++++- > > 3 files changed, 119 insertions(+), 15 deletions(-) > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index 2d85e853403f..df8b786e4875 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -1083,20 +1091,47 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, > > NvmeCmd *cmd, NvmeRequest *req) > > { > > uint32_t dw10 = le32_to_cpu(cmd->cdw10); > > uint32_t dw11 = le32_to_cpu(cmd->cdw11); > > + uint32_t nsid = le32_to_cpu(cmd->nsid); > > uint32_t result; > > uint8_t fid = NVME_GETSETFEAT_FID(dw10); > > + NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10); > > uint16_t iv; > > > > static const uint32_t nvme_feature_default[NVME_FID_MAX] = { > > [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT, > > }; > > > > - trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11); > > + trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11); > > > > if (!nvme_feature_support[fid]) { > > return NVME_INVALID_FIELD | NVME_DNR; > > } > > > > + if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) { > > + if (!nsid || nsid > n->num_namespaces) { > > + /* > > + * The Reservation Notification Mask and Reservation > > Persistence > > + * features require a status code of Invalid Field in Command > > when > > + * NSID is 0xFFFFFFFF. Since the device does not support those > > + * features we can always return Invalid Namespace or Format > > as we > > + * should do for all other features. > > + */ > > + return NVME_INVALID_NSID | NVME_DNR; > > + } > > + } > > + > > + switch (sel) { > > + case NVME_GETFEAT_SELECT_CURRENT: > > + break; > > + case NVME_GETFEAT_SELECT_SAVED: > > + /* no features are saveable by the controller; fallthrough */ > > + case NVME_GETFEAT_SELECT_DEFAULT: > > + goto defaults; > > I hate to say it, but while I have nothing against using 'goto' (unlike some > types I met), > In this particular case it feels like it would be better to have a separate > function for > defaults, or have even have a a separate function per feature and have it > return current/default/saved/whatever > value. The later would allow to have each feature self contained in its own > function. > > But on the other hand I see that you fail back to defaults for unchangeble > features, which does make > sense. In other words, I don't have strong opinion against using goto here > after all. > > When feature code will be getting more features in the future (pun intended) > you probably will have to split it,\ > like I suggest to keep code complexity low. >
Argh... I know you are right. Since you are "accepting" the current state with your R-b and it already carries one from Dmitry I think I'll let this stay for now, but I will fix this in a follow up patch for sure. > > @@ -926,6 +949,8 @@ typedef struct NvmeLBAF { > > uint8_t rp; > > } NvmeLBAF; > > > > +#define NVME_NSID_BROADCAST 0xffffffff > > Cool, you probably want eventually to go over code and > change all places that use the number to the define. > (No need to do this now) > True. Noted :)