Re: [PATCH v3 2/2] nvme: allow cmb and pmr to be enabled on same device

[Klaus Jensen]

On Jun 25 15:57, Andrzej Jakowski wrote:
> On 6/25/20 4:13 AM, Klaus Jensen wrote:
> > 
> > Come to think of it, the above might not even be sufficient since if just 
> > one
> > of the nvme_addr_is_cmb checks fails, we end up issuing an invalid
> > pci_dma_read. But I think that it will error out gracefully on that. But
> > this needs to be checked.
> > 
> > I suggest that you just drop the size check from this patch since it's not
> > needed and might need more work to be safe in the context of SGLs anyway.
> > 
> 
> How about just MMIO access to CMB region? It can be done to any address.
> What guarantees that this will not go outside of CMB region?
> 

This was addressed in commit 87ad860c622c ("nvme: fix out-of-bounds
access to the CMB").

> >> @@ -1453,33 +1457,62 @@ static void nvme_init_namespace(NvmeCtrl *n, 
> >> NvmeNamespace *ns, Error **errp)
> >>      id_ns->nuse = id_ns->ncap;
> >>  }
> >>  
> >> -static void nvme_init_cmb(NvmeCtrl *n, PCIDevice *pci_dev)
> >> +static void nvme_bar4_init(PCIDevice *pci_dev, Error **errp)
> >>  {
> >> -    NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_CMB_BIR);
> >> -    NVME_CMBLOC_SET_OFST(n->bar.cmbloc, 0);
> >> -
> >> -    NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
> >> -    NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
> >> -    NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
> >> -    NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
> >> -    NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
> >> -    NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2); /* MBs */
> >> -    NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb);
> >> -
> >> -    n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
> >> -    memory_region_init_io(&n->ctrl_mem, OBJECT(n), &nvme_cmb_ops, n,
> >> -                          "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
> >> -    pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc),
> >> +    NvmeCtrl *n = NVME(pci_dev);
> >> +    int status;
> >> +    uint64_t bar_size;
> >> +    uint32_t msix_vectors;
> >> +    uint32_t nvme_pba_offset;
> >> +    uint32_t cmb_size_units;
> >> +
> >> +    msix_vectors = n->params.max_ioqpairs + 1;
> >> +    nvme_pba_offset = PCI_MSIX_ENTRY_SIZE * msix_vectors;
> >> +    bar_size = nvme_pba_offset + QEMU_ALIGN_UP(msix_vectors, 64) / 8;
> >> +
> >> +    if (n->params.cmb_size_mb) {
> >> +        NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
> >> +        NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
> >> +        NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
> >> +        NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
> >> +        NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
> >> +        NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2); /* MBs */
> >> +        NVME_CMBSZ_SET_SZ(n->bar.cmbsz, n->params.cmb_size_mb);
> >> +
> >> +        cmb_size_units = NVME_CMBSZ_GETSIZEUNITS(n->bar.cmbsz);
> >> +        n->cmb_offset = QEMU_ALIGN_UP(bar_size, cmb_size_units);
> >> +
> >> +        NVME_CMBLOC_SET_BIR(n->bar.cmbloc, NVME_MSIX_BIR);
> >> +        NVME_CMBLOC_SET_OFST(n->bar.cmbloc, n->cmb_offset / 
> >> cmb_size_units);
> >> +
> >> +        n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
> >> +
> >> +        bar_size += n->cmb_offset;
> >> +        bar_size += NVME_CMBSZ_GETSIZE(n->bar.cmbsz);
> >> +    }
> >> +
> >> +    bar_size = pow2ceil(bar_size);
> >> +
> >> +    memory_region_init_io(&n->bar4, OBJECT(n), &nvme_cmb_ops, n,
> >> +                          "nvme-bar4", bar_size);
> >> +
> >> +    status = msix_init(pci_dev, n->params.max_ioqpairs + 1,
> >> +                       &n->bar4, NVME_MSIX_BIR, 0,
> >> +                       &n->bar4, NVME_MSIX_BIR, nvme_pba_offset,
> >> +                       0, errp);
> > 
> > This needs to use n->params.msix_qsize instead of
> > n->params.max_ioqpairs.
> 
> Makese sense.

Another thing here. You are initializing a single memory region for bar4
and use nvme_cmb_ops as callbacks for that.

msix_init then overlays two memory regions ontop of this (for the table
and the pba). I'm not sure this is... correct? Paolo, can you shed any
light on this?

It looks to me like we need to do something more like what
msix_init_exclusive_bar does:

  1. do a memory_region_init for n->bar4
  2. msix_init adds io regions for the msix table and pba.
  3. we should then add an io region for the cmb like msix_init does
     (with a memory_region_init_io and a memory_region_add_subregion
     pair) and keep n->ctrl_mem around.
  4. pci_register_bar on n->bar4

Thus, no "general" mmio_ops are registered for bar4, but only for the
ctrl_mem and the msix_{table,pba}_mmio regions.