Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields

[Klaus Jensen]

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 :)