[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select
From: |
Klaus Jensen |
Subject: |
Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields |
Date: |
Wed, 29 Jul 2020 15:48:56 +0200 |
On Jul 29 16:17, Maxim Levitsky wrote:
> On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@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.jensen@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 :)
- [PATCH v3 10/18] hw/block/nvme: flush write cache when disabled, (continued)
- [PATCH v3 10/18] hw/block/nvme: flush write cache when disabled, Klaus Jensen, 2020/07/06
- [PATCH v3 11/18] hw/block/nvme: add remaining mandatory controller parameters, Klaus Jensen, 2020/07/06
- [PATCH v3 13/18] hw/block/nvme: make sure ncqr and nsqr is valid, Klaus Jensen, 2020/07/06
- [PATCH v3 14/18] hw/block/nvme: support identify namespace descriptor list, Klaus Jensen, 2020/07/06
- [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields, Klaus Jensen, 2020/07/06
- [PATCH v3 18/18] hw/block/nvme: bump supported version to v1.3, Klaus Jensen, 2020/07/06
- [PATCH v3 17/18] hw/block/nvme: provide the mandatory subnqn field, Klaus Jensen, 2020/07/06
- [PATCH v3 16/18] hw/block/nvme: enforce valid queue creation sequence, Klaus Jensen, 2020/07/06
- [PATCH v3 15/18] hw/block/nvme: reject invalid nsid values in active namespace id list, Klaus Jensen, 2020/07/06