On Wed, 2020-07-29 at 15:48 +0200, Klaus Jensen wrote: > 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. Yep, this is exactly what I was thinking.
Best regards, Maxim Levitsky > > > > @@ -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 :) >