Re: [PATCH v4 1/2] nvme: indicate CMB support through controller capabilities register

[Maxim Levitsky <mlevitsk@redhat.com>]

On Wed, 2020-07-01 at 14:48 -0700, Andrzej Jakowski wrote:
> This patch sets CMBS bit in controller capabilities register when user
> configures NVMe driver with CMB support, so capabilites are correctly
> reported to guest OS.
> 
> Signed-off-by: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c      | 2 +-
>  include/block/nvme.h | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 1aee042d4c..9f11f3e9da 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1582,6 +1582,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>      NVME_CAP_SET_TO(n->bar.cap, 0xf);
>      NVME_CAP_SET_CSS(n->bar.cap, 1);
>      NVME_CAP_SET_MPSMAX(n->bar.cap, 4);
> +    NVME_CAP_SET_CMBS(n->bar.cap, n->params.cmb_size_mb ? 1 : 0);
>  
>      n->bar.vs = 0x00010200;
>      n->bar.intmc = n->bar.intms = 0;
> @@ -1591,7 +1592,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      NvmeCtrl *n = NVME(pci_dev);
>      Error *local_err = NULL;
> -
>      int i;
>  
>      nvme_check_constraints(n, &local_err);
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 1720ee1d51..14cf398dfa 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -35,6 +35,7 @@ enum NvmeCapShift {
>      CAP_MPSMIN_SHIFT   = 48,
>      CAP_MPSMAX_SHIFT   = 52,
>      CAP_PMR_SHIFT      = 56,
> +    CAP_CMB_SHIFT      = 57,
>  };
>  
>  enum NvmeCapMask {
> @@ -48,6 +49,7 @@ enum NvmeCapMask {
>      CAP_MPSMIN_MASK    = 0xf,
>      CAP_MPSMAX_MASK    = 0xf,
>      CAP_PMR_MASK       = 0x1,
> +    CAP_CMB_MASK       = 0x1,
>  };
>  
>  #define NVME_CAP_MQES(cap)  (((cap) >> CAP_MQES_SHIFT)   & CAP_MQES_MASK)
> @@ -78,8 +80,10 @@ enum NvmeCapMask {
>                                                             << CAP_MPSMIN_SHIFT)
>  #define NVME_CAP_SET_MPSMAX(cap, val) (cap |= (uint64_t)(val & CAP_MPSMAX_MASK)\
>                                                              << CAP_MPSMAX_SHIFT)
> -#define NVME_CAP_SET_PMRS(cap, val) (cap |= (uint64_t)(val & CAP_PMR_MASK)\
> +#define NVME_CAP_SET_PMRS(cap, val)   (cap |= (uint64_t)(val & CAP_PMR_MASK)   \
>                                                              << CAP_PMR_SHIFT)
> +#define NVME_CAP_SET_CMBS(cap, val)   (cap |= (uint64_t)(val & CAP_CMB_MASK)   \
> +                                                           << CAP_CMB_SHIFT)
>  
>  enum NvmeCcShift {
>      CC_EN_SHIFT     = 0,


I wonder how this could have beeing forgotten. Hmm.
I see that Linux kernel uses CMBSZ != for that.
I guess this explains it.

Reviewed-by: Maxim Levitsky <mlevitsky@gmail.com>

Best regards,
	Maxim Levitsky