[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: |
Maxim Levitsky |
Subject: |
Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields |
Date: |
Wed, 29 Jul 2020 21:47:16 +0300 |
User-agent: |
Evolution 3.36.3 (3.36.3-1.fc32) |
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.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.
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 :)
>