| Message ID | 20210408170123.8788-13-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new DMA mapping operation for P2PDMA | expand |
On 4/8/21 10:01 AM, Logan Gunthorpe wrote: > Introduce a supports_pci_p2pdma() operation in nvme_ctrl_ops to > replace the fixed NVME_F_PCI_P2PDMA flag such that the dma_map_ops > flags can be checked for PCI P2PDMA support. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > drivers/nvme/host/core.c | 3 ++- > drivers/nvme/host/nvme.h | 2 +- > drivers/nvme/host/pci.c | 11 +++++++++-- > 3 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 0896e21642be..223419454516 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -3907,7 +3907,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid, > blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue); > > blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue); > - if (ctrl->ops->flags & NVME_F_PCI_P2PDMA) > + if (ctrl->ops->supports_pci_p2pdma && > + ctrl->ops->supports_pci_p2pdma(ctrl)) This is a little excessive, as I suspected. How about providing a default .supports_pci_p2pdma routine that returns false, so that the op is always available (non-null)? By "default", maybe that means either requiring an init_the_ops_struct() routine to be used, and/or checking all the users of struct nvme_ctrl_ops. Another idea: maybe you don't really need a bool .supports_pci_p2pdma() routine at all, because the existing .flags really is about right. You just need the flags to be filled in dynamically. So, do that during nvme_pci setup/init time: that's when this module would call dma_pci_p2pdma_supported(). Actually, I think that second idea simplifies things quite a bit, but only if it's possible. I haven't worked through the startup order of calls in nvme_pci. thanks,
On 2021-05-02 7:29 p.m., John Hubbard wrote: > On 4/8/21 10:01 AM, Logan Gunthorpe wrote: >> Introduce a supports_pci_p2pdma() operation in nvme_ctrl_ops to >> replace the fixed NVME_F_PCI_P2PDMA flag such that the dma_map_ops >> flags can be checked for PCI P2PDMA support. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> drivers/nvme/host/core.c | 3 ++- >> drivers/nvme/host/nvme.h | 2 +- >> drivers/nvme/host/pci.c | 11 +++++++++-- >> 3 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index 0896e21642be..223419454516 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -3907,7 +3907,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid, >> blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue); >> >> blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue); >> - if (ctrl->ops->flags & NVME_F_PCI_P2PDMA) >> + if (ctrl->ops->supports_pci_p2pdma && >> + ctrl->ops->supports_pci_p2pdma(ctrl)) > > This is a little excessive, as I suspected. How about providing a > default .supports_pci_p2pdma routine that returns false, so that > the op is always available (non-null)? By "default", maybe that > means either requiring an init_the_ops_struct() routine to be > used, and/or checking all the users of struct nvme_ctrl_ops. Honestly that sounds much more messy to me than simply checking if it's NULL before using it (which is a common, accepted pattern for ops). > Another idea: maybe you don't really need a bool .supports_pci_p2pdma() > routine at all, because the existing .flags really is about right. > You just need the flags to be filled in dynamically. So, do that > during nvme_pci setup/init time: that's when this module would call > dma_pci_p2pdma_supported(). If the flag is filled in dynamically, then the ops struct would have to be non-constant. Ops structs should be constant for security reasons. Logan
On 5/3/21 10:17 AM, Logan Gunthorpe wrote: ... >>> blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue); >>> - if (ctrl->ops->flags & NVME_F_PCI_P2PDMA) >>> + if (ctrl->ops->supports_pci_p2pdma && >>> + ctrl->ops->supports_pci_p2pdma(ctrl)) >> >> This is a little excessive, as I suspected. How about providing a >> default .supports_pci_p2pdma routine that returns false, so that >> the op is always available (non-null)? By "default", maybe that >> means either requiring an init_the_ops_struct() routine to be >> used, and/or checking all the users of struct nvme_ctrl_ops. > > Honestly that sounds much more messy to me than simply checking if it's > NULL before using it (which is a common, accepted pattern for ops). OK, it's a minor suggestion, so feel free to ignore if you prefer it the other way, sure. > >> Another idea: maybe you don't really need a bool .supports_pci_p2pdma() >> routine at all, because the existing .flags really is about right. >> You just need the flags to be filled in dynamically. So, do that >> during nvme_pci setup/init time: that's when this module would call >> dma_pci_p2pdma_supported(). > > If the flag is filled in dynamically, then the ops struct would have to > be non-constant. Ops structs should be constant for security reasons. > Hadn't thought about keeping ops structs constant. OK. thanks,
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 0896e21642be..223419454516 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3907,7 +3907,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid, blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue); blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue); - if (ctrl->ops->flags & NVME_F_PCI_P2PDMA) + if (ctrl->ops->supports_pci_p2pdma && + ctrl->ops->supports_pci_p2pdma(ctrl)) blk_queue_flag_set(QUEUE_FLAG_PCI_P2PDMA, ns->queue); ns->queue->queuedata = ns; diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 07b34175c6ce..9c04df982d2c 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -473,7 +473,6 @@ struct nvme_ctrl_ops { unsigned int flags; #define NVME_F_FABRICS (1 << 0) #define NVME_F_METADATA_SUPPORTED (1 << 1) -#define NVME_F_PCI_P2PDMA (1 << 2) int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val); int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val); int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val); @@ -481,6 +480,7 @@ struct nvme_ctrl_ops { void (*submit_async_event)(struct nvme_ctrl *ctrl); void (*delete_ctrl)(struct nvme_ctrl *ctrl); int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size); + bool (*supports_pci_p2pdma)(struct nvme_ctrl *ctrl); }; #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 7249ae74f71f..14f092973792 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2759,17 +2759,24 @@ static int nvme_pci_get_address(struct nvme_ctrl *ctrl, char *buf, int size) return snprintf(buf, size, "%s\n", dev_name(&pdev->dev)); } +static bool nvme_pci_supports_pci_p2pdma(struct nvme_ctrl *ctrl) +{ + struct nvme_dev *dev = to_nvme_dev(ctrl); + + return dma_pci_p2pdma_supported(dev->dev); +} + static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = { .name = "pcie", .module = THIS_MODULE, - .flags = NVME_F_METADATA_SUPPORTED | - NVME_F_PCI_P2PDMA, + .flags = NVME_F_METADATA_SUPPORTED, .reg_read32 = nvme_pci_reg_read32, .reg_write32 = nvme_pci_reg_write32, .reg_read64 = nvme_pci_reg_read64, .free_ctrl = nvme_pci_free_ctrl, .submit_async_event = nvme_pci_submit_async_event, .get_address = nvme_pci_get_address, + .supports_pci_p2pdma = nvme_pci_supports_pci_p2pdma, }; static int nvme_dev_map(struct nvme_dev *dev)
Introduce a supports_pci_p2pdma() operation in nvme_ctrl_ops to replace the fixed NVME_F_PCI_P2PDMA flag such that the dma_map_ops flags can be checked for PCI P2PDMA support. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/nvme/host/core.c | 3 ++- drivers/nvme/host/nvme.h | 2 +- drivers/nvme/host/pci.c | 11 +++++++++-- 3 files changed, 12 insertions(+), 4 deletions(-)