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

[Maxim Levitsky]

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