* [RFC PATCH 0/1] CXL: Standalone switch CCI driver
@ 2022-10-24 15:53 Jonathan Cameron
2022-10-24 15:53 ` [RFC PATCH 1/1] cxl/pci: Add support for stand alone CXL Switch mailbox CCI Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2022-10-24 15:53 UTC (permalink / raw)
To: linux-cxl, jonathan.cameron
Cc: Dan Williams, Alison Schofield, Vishal Verma, Ira Weiny,
Ben Widawsky, linuxarm, Viacheslav A . Dubeyko
CXL rev 3.0 introduced the option for a PCI function, intended to sit on an
upstream port of a CXL switch. This function provides a mailbox
interface similar to that seen on CXL type 3 devices. However, the
command set is mostly different and intended for Fabric management.
Note however that as we add support for multi headed devices (MHDs)
a subset of commands will be available on selected MHD type 3 mailboxes.
(tunnelling DCD commands for example)
See: CXL rev 3.0
7.2.9 Switch Mailbox CCI
8.1.13 Switch Malibox CCI Configuration Space Layout
8.2.8.6 Switch Mailbox CCI capability
It is probably relatively unusual that a typical host of CXL devices
will have access to the one of these devices, in many cases they will
be on a port connected to a BMC or similar. There are a few use cases
where the host might be in charge of the configuration.
These are very convenient for testing in conjunction with the QEMU
emulation though so far CXL switch and type 3 emulation is in QEMU
is not complex enough to make these particular interesting.
This initial support provides only a few commands but I'm sending it
out as an RFC to get some input on how we should refactor the CXL core
code to support these devices that use some of the provide functionality.
Test wise, there is emulation support on
http://gitlab.com/jic23/qemu cxl-2022-10-24 branch
Assumption for now is that the switch CCI function sits alongside
a CXL USP. A config blob along the lines of:
-device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
-device cxl-rp,port=0,bus=cxl.1,id=root_port0,chassis=0,slot=2 \
-device cxl-rp,port=1,bus=cxl.1,id=root_port2,chassis=0,slot=3 \
-device cxl-upstream,port=33,bus=root_port0,id=us0,multifunction=on,addr=0.0,sn=12345678 \
-device cxl-switch-mailbox-cci,bus=root_port0,addr=0.1 \
-device cxl-downstream,port=0,bus=us0,id=swport0,chassis=0,slot=4 \
-device cxl-downstream,port=1,bus=us0,id=swport1,chassis=0,slot=5 \
-device cxl-downstream,port=2,bus=us0,id=swport2,chassis=0,slot=6 \
-device cxl-downstream,port=3,bus=us0,id=swport3,chassis=0,slot=7
should create a switch with a suitable function 1 alongside the switch CCI.
Test wise, an example using the user space ioctls is given below.
Longer term this will want to be done with a suitable open source fabric
manager library / program.
#include <linux/types.h>
#include <stdint.h>
#include <sys/ioctl.h>
#include <stdio.h>
#include <fcntl.h>
#include <stdlib.h>
#include "cxl_mem.h"
/* Move to appropriate header later */
struct cxl_cmd_infostat_identify_rsp {
uint16_t pcie_vid;
uint16_t pcie_did;
uint16_t pcie_subsys_vid;
uint16_t pcie_subsys_id;
uint64_t sn;
uint8_t max_message_size;
uint8_t component_type;
};
struct cxl_cmd_infostat_get_bg_cmd_sts_rsp {
uint8_t status;
uint8_t rsvd;
uint16_t opcode;
uint16_t returncode;
uint16_t vendor_ext_status;
};
struct cxl_cmd_identify_switch_device_rsp {
uint8_t ingress_port_id;
uint8_t rsvd;
uint8_t num_physical_ports;
uint8_t num_vcs;
uint8_t active_port_bm[0x20];
uint8_t vcs_bm[0x20];
uint16_t total_num_vPPBs;
uint16_t num_bound_vPPBs;
uint8_t num_hdm_decoders;
} __attribute__((packed));
int main()
{
struct cxl_send_command cmd = {};
struct cxl_cmd_infostat_identify_rsp is_identify;
struct cxl_cmd_identify_switch_device_rsp switch_identify;
struct cxl_cmd_infostat_get_bg_cmd_sts_rsp bg_cmd_status;
int fd;
int rc, i;
fd = open("/dev/cxl/swcci0", O_RDWR);
if (fd < 0) {
printf("could not open file\n");
return 0;
}
cmd.id = CXL_MEM_COMMAND_ID_RAW;
cmd.id = CXL_MEM_COMMAND_ID_INFO_STAT_IDENT;
cmd.out.size = sizeof(is_identify);
cmd.out.payload = (__u64)&is_identify;
rc = ioctl(fd, CXL_MEM_SEND_COMMAND, &cmd);
if (rc) {
printf("rc %d\n", rc);
if (rc < 0)
return rc;
}
printf("Identify on switch:\n");
printf("VID:0x%04x DID:0x%04x\n", is_identify.pcie_vid, is_identify.pcie_did);
printf("Subsys: VID:0x%04x DID:0x%04x\n", is_identify.pcie_subsys_vid, is_identify.pcie_subsys_id);
cmd.id = CXL_MEM_COMMAND_ID_GET_BG_CMD_STATUS;
cmd.out.size = sizeof(bg_cmd_status);
cmd.out.payload = (__u64)&bg_cmd_status;
rc = ioctl(fd, CXL_MEM_SEND_COMMAND, &cmd);
if (rc) {
printf("rc %d\n", rc);
if (rc < 0)
return rc;
}
cmd.id = CXL_MEM_COMMAND_ID_IDENTIFY_SWITCH_DEVICE;
cmd.out.size = sizeof(switch_identify);
cmd.out.payload = (__u64)&switch_identify;
rc = ioctl(fd, CXL_MEM_SEND_COMMAND, &cmd);
if (rc) {
printf("rc %d\n", rc);
if (rc < 0)
return rc;
}
printf("Switch indent ingress=%#x #ports=%d\n",
switch_identify.ingress_port_id,
switch_identify.num_physical_ports);
for (i = 0; i < sizeof(switch_identify.active_port_bm); i++) {
int j;
for (j = 0; j < 8; j++) {
if (switch_identify.active_port_bm[i] & 1 << j) {
printf("Port %x active\n", i * 8 + j);
}
}
}
return 0;
}
Jonathan Cameron (1):
cxl/pci: Add support for stand alone CXL Switch mailbox CCI
drivers/cxl/core/Makefile | 1 +
drivers/cxl/core/core.h | 1 +
drivers/cxl/core/mbox.c | 20 ++++-
drivers/cxl/core/port.c | 4 +
drivers/cxl/core/switch-cci.c | 142 ++++++++++++++++++++++++++++++++++
drivers/cxl/cxlmem.h | 3 +
drivers/cxl/cxlpci.h | 3 +
drivers/cxl/cxlswitch.h | 17 ++++
drivers/cxl/pci.c | 93 +++++++++++++++++++++-
include/uapi/linux/cxl_mem.h | 3 +
10 files changed, 282 insertions(+), 5 deletions(-)
create mode 100644 drivers/cxl/core/switch-cci.c
create mode 100644 drivers/cxl/cxlswitch.h
--
2.37.2
^ permalink raw reply [flat|nested] 4+ messages in thread* [RFC PATCH 1/1] cxl/pci: Add support for stand alone CXL Switch mailbox CCI 2022-10-24 15:53 [RFC PATCH 0/1] CXL: Standalone switch CCI driver Jonathan Cameron @ 2022-10-24 15:53 ` Jonathan Cameron 2022-10-24 19:17 ` [External] " Viacheslav A.Dubeyko 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Cameron @ 2022-10-24 15:53 UTC (permalink / raw) To: linux-cxl, jonathan.cameron Cc: Dan Williams, Alison Schofield, Vishal Verma, Ira Weiny, Ben Widawsky, linuxarm, Viacheslav A . Dubeyko CXL 3.0 defines a mailbox PCI function independent of any other CXL components. The intent is that instances of this mailbox will be found as additional PCI functions of upstream CXL switch ports. RFC: Including this directly in cxl/pci.c as a second pci_driver, is a bit hacky. The alternative is to factor out all the common infrastructure to a library module shared by the Type 3 PCI driver and the Switch CCI driver. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- Options to consider: 1 - Factor out all the shared code and have a separate module for switch CCI driver. Messy! 2 - Is sharing the allow lists between type 3 devices and switch CCI an issue? Not a whole lot of overlap... --- drivers/cxl/core/Makefile | 1 + drivers/cxl/core/core.h | 1 + drivers/cxl/core/mbox.c | 20 ++++- drivers/cxl/core/port.c | 4 + drivers/cxl/core/switch-cci.c | 142 ++++++++++++++++++++++++++++++++++ drivers/cxl/cxlmem.h | 3 + drivers/cxl/cxlpci.h | 3 + drivers/cxl/cxlswitch.h | 17 ++++ drivers/cxl/pci.c | 93 +++++++++++++++++++++- include/uapi/linux/cxl_mem.h | 3 + 10 files changed, 282 insertions(+), 5 deletions(-) diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile index 79c7257f4107..18275e153437 100644 --- a/drivers/cxl/core/Makefile +++ b/drivers/cxl/core/Makefile @@ -10,4 +10,5 @@ cxl_core-y += memdev.o cxl_core-y += mbox.o cxl_core-y += pci.o cxl_core-y += hdm.o +cxl_core-y += switch-cci.o cxl_core-$(CONFIG_CXL_REGION) += region.o diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index 1d8f87be283f..8ac1a0e2c98f 100644 --- a/drivers/cxl/core/core.h +++ b/drivers/cxl/core/core.h @@ -70,5 +70,6 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port, int cxl_memdev_init(void); void cxl_memdev_exit(void); void cxl_mbox_init(void); +int cxl_switch_cci_init(void); #endif /* __CXL_CORE_H__ */ diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 0c90f13870a4..a4e2cb454094 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -5,6 +5,7 @@ #include <linux/debugfs.h> #include <linux/mutex.h> #include <cxlmem.h> +#include <cxlpci.h> #include <cxl.h> #include "core.h" @@ -43,6 +44,8 @@ static bool cxl_raw_allow_all; * 0, and the user passed in 1, it is an error. */ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = { + CXL_CMD(INFO_STAT_IDENTIFY, 0, 18, 0), + CXL_CMD(GET_BG_CMD_STATUS, 0, 8, 0), CXL_CMD(IDENTIFY, 0, 0x43, CXL_CMD_FLAG_FORCE_ENABLE), #ifdef CONFIG_CXL_MEM_RAW_COMMANDS CXL_CMD(RAW, CXL_VARIABLE_PAYLOAD, CXL_VARIABLE_PAYLOAD, 0), @@ -65,6 +68,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = { CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0), CXL_CMD(SCAN_MEDIA, 0x11, 0, 0), CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0), + CXL_CMD(IDENTIFY_SWITCH_DEVICE, 0, 0x49, 0), }; /* @@ -526,10 +530,9 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds, return rc; } -int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) +int __cxl_send_cmd(struct cxl_dev_state *cxlds, struct device *dev, + struct cxl_send_command __user *s) { - struct cxl_dev_state *cxlds = cxlmd->cxlds; - struct device *dev = &cxlmd->dev; struct cxl_send_command send; struct cxl_mbox_cmd mbox_cmd; int rc; @@ -539,7 +542,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) if (copy_from_user(&send, s, sizeof(send))) return -EFAULT; - rc = cxl_validate_cmd_from_user(&mbox_cmd, cxlmd->cxlds, &send); + rc = cxl_validate_cmd_from_user(&mbox_cmd, cxlds, &send); if (rc) return rc; @@ -553,6 +556,15 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) return 0; } +EXPORT_SYMBOL_GPL(__cxl_send_cmd); + +int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) +{ + struct cxl_dev_state *cxlds = cxlmd->cxlds; + struct device *dev = &cxlmd->dev; + + return __cxl_send_cmd(cxlds, dev, s); +} static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 *out) { diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index bffde862de0b..3b2e93bc4684 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1859,6 +1859,10 @@ static __init int cxl_core_init(void) if (rc) return rc; + rc = cxl_switch_cci_init(); + if (rc) + return rc; + cxl_bus_wq = alloc_ordered_workqueue("cxl_port", 0); if (!cxl_bus_wq) { rc = -ENOMEM; diff --git a/drivers/cxl/core/switch-cci.c b/drivers/cxl/core/switch-cci.c new file mode 100644 index 000000000000..1ce51cc07cba --- /dev/null +++ b/drivers/cxl/core/switch-cci.c @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <cxlswitch.h> +#include "cxlmem.h" /* For now to get the cxl_device_state */ +#include "cxlpci.h" +#include "core.h" + + +static int cxl_sw_major; +static DEFINE_IDA(cxl_swdev_ida); +static DECLARE_RWSEM(cxl_swdev_rwsem); + +static inline struct cxl_swdev *to_cxl_swdev(struct device *dev) +{ + return container_of(dev, struct cxl_swdev, dev); +} + +static char *cxl_swdev_devnode(struct device *dev, umode_t *mode, kuid_t *uid, + kgid_t *gid) +{ + return kasprintf(GFP_KERNEL, "cxl/%s", dev_name(dev)); +} + +static void cxl_swdev_release(struct device *dev) +{ + struct cxl_swdev *cxlswd = to_cxl_swdev(dev); + + ida_free(&cxl_swdev_ida, cxlswd->id); + kfree(cxlswd); +} + +static const struct device_type cxl_swdev_type = { + .name = "cxl_swdev", + .release = cxl_swdev_release, + .devnode = cxl_swdev_devnode, +}; + +static long __cxl_swdev_ioctl(struct cxl_swdev *cxlswd, unsigned int cmd, + unsigned long arg) +{ + switch (cmd) { + case CXL_MEM_SEND_COMMAND: + return __cxl_send_cmd(cxlswd->cxlds, &cxlswd->dev, (void __user *)arg); + default: + return -ENOTTY; + } +} + +static long cxl_swdev_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct cxl_swdev *cxlswd = file->private_data; + int rc = -ENXIO; + + down_read(&cxl_swdev_rwsem); + if (cxlswd->cxlds) + rc = __cxl_swdev_ioctl(cxlswd, cmd, arg); + up_read(&cxl_swdev_rwsem); + + return rc; +} + +static int cxl_swdev_open(struct inode *inode, struct file *file) +{ + struct cxl_memdev *cxlswd = + container_of(inode->i_cdev, typeof(*cxlswd), cdev); + + get_device(&cxlswd->dev); + file->private_data = cxlswd; + + return 0; +} + +static int cxl_swdev_release_file(struct inode *inode, struct file *file) +{ + struct cxl_swdev *cxlswd = + container_of(inode->i_cdev, typeof(*cxlswd), cdev); + + put_device(&cxlswd->dev); + + return 0; +} + +static const struct file_operations cxl_swdev_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = cxl_swdev_ioctl, + .open = cxl_swdev_open, + .release = cxl_swdev_release_file, + .compat_ioctl = compat_ptr_ioctl, + .llseek = noop_llseek, +}; + +struct cxl_swdev *cxl_swdev_alloc(struct cxl_dev_state *cxlds) +{ + struct cxl_swdev *cxlswd; + struct device *dev; + struct cdev *cdev; + int rc; + + cxlswd = kzalloc(sizeof(*cxlswd), GFP_KERNEL); + if (!cxlswd) + return ERR_PTR(-ENOMEM); + + rc = ida_alloc_range(&cxl_swdev_ida, 0, 10, GFP_KERNEL); + if (rc < 0) { + kfree(cxlswd); + return ERR_PTR(rc); + } + + cxlswd->id = rc; + cxlswd->cxlds = cxlds; + dev = &cxlswd->dev; + device_initialize(dev); + dev->parent = cxlds->dev; + dev->bus = &cxl_bus_type; + dev->devt = MKDEV(cxl_sw_major, cxlswd->id); + dev->type = &cxl_swdev_type; + device_set_pm_not_required(dev); + cdev = &cxlswd->cdev; + cdev_init(cdev, &cxl_swdev_fops); + rc = dev_set_name(dev, "swcci%d", cxlswd->id); + if (rc) { + put_device(dev); + ida_free(&cxl_swdev_ida, cxlswd->id); + return ERR_PTR(rc); + } + + return cxlswd; +} +EXPORT_SYMBOL_NS_GPL(cxl_swdev_alloc, CXL); + +__init int cxl_switch_cci_init(void) +{ + dev_t devt; + int rc; + + rc = alloc_chrdev_region(&devt, 0, 10, "cxlsw"); + if (rc) + return rc; + cxl_sw_major = MAJOR(devt); + + return 0; +} diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 88e3a8e54b6a..ef9c0c347daf 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -252,6 +252,8 @@ struct cxl_dev_state { enum cxl_opcode { CXL_MBOX_OP_INVALID = 0x0000, + CXL_MBOX_OP_INFO_STAT_IDENTIFY = 0x0001, + CXL_MBOX_OP_GET_BG_CMD_STATUS = 0x0002, CXL_MBOX_OP_RAW = CXL_MBOX_OP_INVALID, CXL_MBOX_OP_GET_FW_INFO = 0x0200, CXL_MBOX_OP_ACTIVATE_FW = 0x0202, @@ -273,6 +275,7 @@ enum cxl_opcode { CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS = 0x4303, CXL_MBOX_OP_SCAN_MEDIA = 0x4304, CXL_MBOX_OP_GET_SCAN_MEDIA = 0x4305, + CXL_MBOX_OP_IDENTIFY_SWITCH_DEVICE = 0x5100, CXL_MBOX_OP_MAX = 0x10000 }; diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h index eec597dbe763..7f53b601ad7c 100644 --- a/drivers/cxl/cxlpci.h +++ b/drivers/cxl/cxlpci.h @@ -75,4 +75,7 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port); struct cxl_dev_state; int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm); void read_cdat_data(struct cxl_port *port); +struct cxl_send_command; +int __cxl_send_cmd(struct cxl_dev_state *cxlds, struct device *dev, + struct cxl_send_command __user *s); #endif /* __CXL_PCI_H__ */ diff --git a/drivers/cxl/cxlswitch.h b/drivers/cxl/cxlswitch.h new file mode 100644 index 000000000000..4d1689db653c --- /dev/null +++ b/drivers/cxl/cxlswitch.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef CXLSWITCH_H +#define CXLSWITCH_H +#include <linux/cdev.h> +#include <linux/device.h> +#include "cxl.h" + +struct cxl_dev_state; +struct cxl_swdev { + struct device dev; + struct cdev cdev; + struct cxl_dev_state *cxlds; + int id; +}; + +struct cxl_swdev *cxl_swdev_alloc(struct cxl_dev_state *cxlds); +#endif diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index faeb5d9d7a7a..a7aec6247e86 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -12,6 +12,7 @@ #include <linux/io.h> #include "cxlmem.h" #include "cxlpci.h" +#include "cxlswitch.h" #include "cxl.h" /** @@ -520,6 +521,96 @@ static struct pci_driver cxl_pci_driver = { }, }; +static int cxl_swmbcci_probe(struct pci_dev *pdev, const struct pci_device_id *id) +{ + struct cxl_dev_state *cxlds; + struct cxl_register_map map; + struct cxl_swdev *cxlswd; + int rc; + + rc = pcim_enable_device(pdev); + if (rc) + return rc; + + cxlds = cxl_dev_state_create(&pdev->dev); + if (IS_ERR(cxlds)) + return PTR_ERR(cxlds); + + rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map); + if (rc) + return rc; + + rc = cxl_map_regs(cxlds, &map); + if (rc) + return rc; + + rc = cxl_pci_setup_mailbox(cxlds); + if (rc) + return rc; + + cxlswd = cxl_swdev_alloc(cxlds); + if (IS_ERR(cxlswd)) + return PTR_ERR(cxlswd); + + pci_set_drvdata(pdev, cxlswd); + + rc = cxl_enumerate_cmds(cxlds); + if (rc) + goto error_put_device; + + rc = cdev_device_add(&cxlswd->cdev, &cxlswd->dev); + if (rc) + goto error_put_device; + + return 0; + +error_put_device: + put_device(&cxlswd->dev); + return rc; +} + +static void cxl_swbmcci_remove(struct pci_dev *pdev) +{ + struct cxl_swdev *cxlswd = pci_get_drvdata(pdev); + struct device *dev = &cxlswd->dev; + + cdev_device_del(&cxlswd->cdev, dev); + put_device(&cxlswd->dev); +} + +static const struct pci_device_id cxl_swmbcci_pci_tbl[] = { + { PCI_DEVICE_CLASS((0x0c0b << 8 | 0), ~0)}, + {} +}; + +static struct pci_driver cxl_swmbcci_driver = { + .name = "cxl_swmbcci", + .id_table = cxl_swmbcci_pci_tbl, + .probe = cxl_swmbcci_probe, + .remove = cxl_swbmcci_remove, +}; + +static int __init cxl_pci_init(void) +{ + int rc; + + rc = pci_register_driver(&cxl_pci_driver); + if (rc) + return rc; + + rc = pci_register_driver(&cxl_swmbcci_driver); + if (rc) { + pci_unregister_driver(&cxl_pci_driver); + return rc; + } + return 0; +} +module_init(cxl_pci_init); +static void __exit cxl_pci_exit(void) +{ + pci_unregister_driver(&cxl_swmbcci_driver); + pci_unregister_driver(&cxl_pci_driver); +} +module_exit(cxl_pci_exit); MODULE_LICENSE("GPL v2"); -module_pci_driver(cxl_pci_driver); MODULE_IMPORT_NS(CXL); diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h index c71021a2a9ed..ea03a289e56f 100644 --- a/include/uapi/linux/cxl_mem.h +++ b/include/uapi/linux/cxl_mem.h @@ -41,6 +41,9 @@ ___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"), \ ___C(SCAN_MEDIA, "Scan Media"), \ ___C(GET_SCAN_MEDIA, "Get Scan Media Results"), \ + ___C(INFO_STAT_IDENTIFY, "Get Information"), \ + ___C(GET_BG_CMD_STATUS, "Background Command Status"), \ + ___C(IDENTIFY_SWITCH_DEVICE, "Identify Switch Device"), \ ___C(MAX, "invalid / last command") #define ___C(a, b) CXL_MEM_COMMAND_ID_##a -- 2.37.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [External] [RFC PATCH 1/1] cxl/pci: Add support for stand alone CXL Switch mailbox CCI 2022-10-24 15:53 ` [RFC PATCH 1/1] cxl/pci: Add support for stand alone CXL Switch mailbox CCI Jonathan Cameron @ 2022-10-24 19:17 ` Viacheslav A.Dubeyko [not found] ` <20221025114251.000031f8@huawei.com> 0 siblings, 1 reply; 4+ messages in thread From: Viacheslav A.Dubeyko @ 2022-10-24 19:17 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-cxl, Dan Williams, Alison Schofield, Vishal Verma, Ira Weiny, Ben Widawsky, linuxarm Hi Jonathan, > On Oct 24, 2022, at 8:53 AM, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > CXL 3.0 defines a mailbox PCI function independent of any other CXL > components. The intent is that instances of this mailbox will be found > as additional PCI functions of upstream CXL switch ports. > > RFC: Including this directly in cxl/pci.c as a second pci_driver, is a > bit hacky. The alternative is to factor out all the common > infrastructure to a library module shared by the Type 3 PCI driver > and the Switch CCI driver. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > Options to consider: > 1 - Factor out all the shared code and have a separate module for > switch CCI driver. Messy! > 2 - Is sharing the allow lists between type 3 devices and switch CCI > an issue? Not a whole lot of overlap... > --- > drivers/cxl/core/Makefile | 1 + > drivers/cxl/core/core.h | 1 + > drivers/cxl/core/mbox.c | 20 ++++- > drivers/cxl/core/port.c | 4 + > drivers/cxl/core/switch-cci.c | 142 ++++++++++++++++++++++++++++++++++ > drivers/cxl/cxlmem.h | 3 + > drivers/cxl/cxlpci.h | 3 + > drivers/cxl/cxlswitch.h | 17 ++++ > drivers/cxl/pci.c | 93 +++++++++++++++++++++- > include/uapi/linux/cxl_mem.h | 3 + > 10 files changed, 282 insertions(+), 5 deletions(-) > > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile > index 79c7257f4107..18275e153437 100644 > --- a/drivers/cxl/core/Makefile > +++ b/drivers/cxl/core/Makefile > @@ -10,4 +10,5 @@ cxl_core-y += memdev.o > cxl_core-y += mbox.o > cxl_core-y += pci.o > cxl_core-y += hdm.o > +cxl_core-y += switch-cci.o > cxl_core-$(CONFIG_CXL_REGION) += region.o > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 1d8f87be283f..8ac1a0e2c98f 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -70,5 +70,6 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port, > int cxl_memdev_init(void); > void cxl_memdev_exit(void); > void cxl_mbox_init(void); > +int cxl_switch_cci_init(void); > > #endif /* __CXL_CORE_H__ */ > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 0c90f13870a4..a4e2cb454094 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -5,6 +5,7 @@ > #include <linux/debugfs.h> > #include <linux/mutex.h> > #include <cxlmem.h> > +#include <cxlpci.h> > #include <cxl.h> > > #include "core.h" > @@ -43,6 +44,8 @@ static bool cxl_raw_allow_all; > * 0, and the user passed in 1, it is an error. > */ > static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = { > + CXL_CMD(INFO_STAT_IDENTIFY, 0, 18, 0), > + CXL_CMD(GET_BG_CMD_STATUS, 0, 8, 0), As far as I can see, it is used hexadecimal constants, mostly. So, it will be better to use 0x12 instead of 18. However, it will be great, from my of view, to use some constant declarations that can explain why 18 exactly here. Otherwise, maybe, some comments could explain these numbers? > CXL_CMD(IDENTIFY, 0, 0x43, CXL_CMD_FLAG_FORCE_ENABLE), > #ifdef CONFIG_CXL_MEM_RAW_COMMANDS > CXL_CMD(RAW, CXL_VARIABLE_PAYLOAD, CXL_VARIABLE_PAYLOAD, 0), > @@ -65,6 +68,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = { > CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0), > CXL_CMD(SCAN_MEDIA, 0x11, 0, 0), > CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0), > + CXL_CMD(IDENTIFY_SWITCH_DEVICE, 0, 0x49, 0), > }; > > /* > @@ -526,10 +530,9 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds, > return rc; > } > > -int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > +int __cxl_send_cmd(struct cxl_dev_state *cxlds, struct device *dev, > + struct cxl_send_command __user *s) > { > - struct cxl_dev_state *cxlds = cxlmd->cxlds; > - struct device *dev = &cxlmd->dev; > struct cxl_send_command send; > struct cxl_mbox_cmd mbox_cmd; > int rc; > @@ -539,7 +542,7 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > if (copy_from_user(&send, s, sizeof(send))) > return -EFAULT; > > - rc = cxl_validate_cmd_from_user(&mbox_cmd, cxlmd->cxlds, &send); > + rc = cxl_validate_cmd_from_user(&mbox_cmd, cxlds, &send); > if (rc) > return rc; > > @@ -553,6 +556,15 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > > return 0; > } > +EXPORT_SYMBOL_GPL(__cxl_send_cmd); > + I am guessing… If cxl_send_cmd is introduces already, then do we really need to keep __cxl_send_cmd as not static? Any special use-cases for using __cxl_send_cmd? > +int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > +{ > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + struct device *dev = &cxlmd->dev; > + > + return __cxl_send_cmd(cxlds, dev, s); > +} > > static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 *out) > { > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index bffde862de0b..3b2e93bc4684 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1859,6 +1859,10 @@ static __init int cxl_core_init(void) > if (rc) > return rc; > > + rc = cxl_switch_cci_init(); > + if (rc) > + return rc; > + > cxl_bus_wq = alloc_ordered_workqueue("cxl_port", 0); > if (!cxl_bus_wq) { > rc = -ENOMEM; > diff --git a/drivers/cxl/core/switch-cci.c b/drivers/cxl/core/switch-cci.c > new file mode 100644 > index 000000000000..1ce51cc07cba > --- /dev/null > +++ b/drivers/cxl/core/switch-cci.c > @@ -0,0 +1,142 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +#include <cxlswitch.h> > +#include "cxlmem.h" /* For now to get the cxl_device_state */ > +#include "cxlpci.h" > +#include "core.h" > + > + > +static int cxl_sw_major; > +static DEFINE_IDA(cxl_swdev_ida); > +static DECLARE_RWSEM(cxl_swdev_rwsem); I am not sure how good and safe could be static semaphore. Currently, I cannot share any potential troubles with it. But lifetime of this semaphore will be the same as module itself. So, it sounds that we will take memory even if functionality of this module never will be used. And I assume that we pollute the global namespace with the name of this semaphore. > + > +static inline struct cxl_swdev *to_cxl_swdev(struct device *dev) > +{ > + return container_of(dev, struct cxl_swdev, dev); > +} > + > +static char *cxl_swdev_devnode(struct device *dev, umode_t *mode, kuid_t *uid, > + kgid_t *gid) > +{ > + return kasprintf(GFP_KERNEL, "cxl/%s", dev_name(dev)); > +} > + > +static void cxl_swdev_release(struct device *dev) > +{ > + struct cxl_swdev *cxlswd = to_cxl_swdev(dev); > + > + ida_free(&cxl_swdev_ida, cxlswd->id); > + kfree(cxlswd); > +} It looks slightly confusing for me to see cxl_swdev_release() without allocate pair around. Maybe, it makes sense to keep allocate/release pair at the same place? Otherwise, it needs to search allocate method to understand the release logic. > + > +static const struct device_type cxl_swdev_type = { > + .name = "cxl_swdev", > + .release = cxl_swdev_release, > + .devnode = cxl_swdev_devnode, > +}; > + > +static long __cxl_swdev_ioctl(struct cxl_swdev *cxlswd, unsigned int cmd, > + unsigned long arg) > +{ > + switch (cmd) { > + case CXL_MEM_SEND_COMMAND: > + return __cxl_send_cmd(cxlswd->cxlds, &cxlswd->dev, (void __user *)arg); > + default: > + return -ENOTTY; > + } > +} > + > +static long cxl_swdev_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct cxl_swdev *cxlswd = file->private_data; > + int rc = -ENXIO; > + > + down_read(&cxl_swdev_rwsem); > + if (cxlswd->cxlds) > + rc = __cxl_swdev_ioctl(cxlswd, cmd, arg); > + up_read(&cxl_swdev_rwsem); > + Can we rearrange the logic? if (cxlswd->cxlds) { down_read(&cxl_swdev_rwsem); rc = __cxl_swdev_ioctl(cxlswd, cmd, arg); up_read(&cxl_swdev_rwsem); } Does cxl_swdev_rwsem protect the cxlswd->cxlds pointer? > + return rc; > +} > + > +static int cxl_swdev_open(struct inode *inode, struct file *file) > +{ > + struct cxl_memdev *cxlswd = > + container_of(inode->i_cdev, typeof(*cxlswd), cdev); > + > + get_device(&cxlswd->dev); > + file->private_data = cxlswd; > + > + return 0; > +} > + > +static int cxl_swdev_release_file(struct inode *inode, struct file *file) > +{ > + struct cxl_swdev *cxlswd = > + container_of(inode->i_cdev, typeof(*cxlswd), cdev); > + > + put_device(&cxlswd->dev); > + > + return 0; > +} > + > +static const struct file_operations cxl_swdev_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = cxl_swdev_ioctl, > + .open = cxl_swdev_open, > + .release = cxl_swdev_release_file, > + .compat_ioctl = compat_ptr_ioctl, > + .llseek = noop_llseek, > +}; > + > +struct cxl_swdev *cxl_swdev_alloc(struct cxl_dev_state *cxlds) > +{ > + struct cxl_swdev *cxlswd; > + struct device *dev; > + struct cdev *cdev; > + int rc; > + > + cxlswd = kzalloc(sizeof(*cxlswd), GFP_KERNEL); > + if (!cxlswd) > + return ERR_PTR(-ENOMEM); > + > + rc = ida_alloc_range(&cxl_swdev_ida, 0, 10, GFP_KERNEL); > + if (rc < 0) { > + kfree(cxlswd); Maybe, it makes sense to combine all free logic for failure case in one place? I believe it could make the logic more clean and method could be easy to modify in the future without introduction of new tricky bugs. > + return ERR_PTR(rc); > + } > + > + cxlswd->id = rc; > + cxlswd->cxlds = cxlds; > + dev = &cxlswd->dev; > + device_initialize(dev); > + dev->parent = cxlds->dev; > + dev->bus = &cxl_bus_type; > + dev->devt = MKDEV(cxl_sw_major, cxlswd->id); > + dev->type = &cxl_swdev_type; > + device_set_pm_not_required(dev); > + cdev = &cxlswd->cdev; > + cdev_init(cdev, &cxl_swdev_fops); > + rc = dev_set_name(dev, "swcci%d", cxlswd->id); > + if (rc) { > + put_device(dev); > + ida_free(&cxl_swdev_ida, cxlswd->id); Ditto. > + return ERR_PTR(rc); > + } > + > + return cxlswd; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_swdev_alloc, CXL); > + > +__init int cxl_switch_cci_init(void) > +{ > + dev_t devt; > + int rc; > + > + rc = alloc_chrdev_region(&devt, 0, 10, "cxlsw"); > + if (rc) > + return rc; > + cxl_sw_major = MAJOR(devt); > + > + return 0; > +} > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 88e3a8e54b6a..ef9c0c347daf 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -252,6 +252,8 @@ struct cxl_dev_state { > > enum cxl_opcode { > CXL_MBOX_OP_INVALID = 0x0000, > + CXL_MBOX_OP_INFO_STAT_IDENTIFY = 0x0001, > + CXL_MBOX_OP_GET_BG_CMD_STATUS = 0x0002, I know that people likes to hardcode constants. :) But it could be hard to follow these constants. Maybe, we need to introduce some comments? > CXL_MBOX_OP_RAW = CXL_MBOX_OP_INVALID, > CXL_MBOX_OP_GET_FW_INFO = 0x0200, > CXL_MBOX_OP_ACTIVATE_FW = 0x0202, > @@ -273,6 +275,7 @@ enum cxl_opcode { > CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS = 0x4303, > CXL_MBOX_OP_SCAN_MEDIA = 0x4304, > CXL_MBOX_OP_GET_SCAN_MEDIA = 0x4305, > + CXL_MBOX_OP_IDENTIFY_SWITCH_DEVICE = 0x5100, > CXL_MBOX_OP_MAX = 0x10000 > }; > > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h > index eec597dbe763..7f53b601ad7c 100644 > --- a/drivers/cxl/cxlpci.h > +++ b/drivers/cxl/cxlpci.h > @@ -75,4 +75,7 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port); > struct cxl_dev_state; > int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm); > void read_cdat_data(struct cxl_port *port); > +struct cxl_send_command; > +int __cxl_send_cmd(struct cxl_dev_state *cxlds, struct device *dev, > + struct cxl_send_command __user *s); > #endif /* __CXL_PCI_H__ */ > diff --git a/drivers/cxl/cxlswitch.h b/drivers/cxl/cxlswitch.h > new file mode 100644 > index 000000000000..4d1689db653c > --- /dev/null > +++ b/drivers/cxl/cxlswitch.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef CXLSWITCH_H > +#define CXLSWITCH_H > +#include <linux/cdev.h> > +#include <linux/device.h> > +#include "cxl.h" > + > +struct cxl_dev_state; > +struct cxl_swdev { > + struct device dev; > + struct cdev cdev; > + struct cxl_dev_state *cxlds; > + int id; > +}; > + > +struct cxl_swdev *cxl_swdev_alloc(struct cxl_dev_state *cxlds); Do we declare alloc but not the free/release? Why so? > +#endif > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index faeb5d9d7a7a..a7aec6247e86 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -12,6 +12,7 @@ > #include <linux/io.h> > #include "cxlmem.h" > #include "cxlpci.h" > +#include "cxlswitch.h" > #include "cxl.h" > > /** > @@ -520,6 +521,96 @@ static struct pci_driver cxl_pci_driver = { > }, > }; > > +static int cxl_swmbcci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > +{ > + struct cxl_dev_state *cxlds; > + struct cxl_register_map map; > + struct cxl_swdev *cxlswd; > + int rc; > + > + rc = pcim_enable_device(pdev); > + if (rc) > + return rc; > + > + cxlds = cxl_dev_state_create(&pdev->dev); > + if (IS_ERR(cxlds)) > + return PTR_ERR(cxlds); > + > + rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map); > + if (rc) > + return rc; > + > + rc = cxl_map_regs(cxlds, &map); > + if (rc) > + return rc; > + > + rc = cxl_pci_setup_mailbox(cxlds); > + if (rc) > + return rc; > + > + cxlswd = cxl_swdev_alloc(cxlds); > + if (IS_ERR(cxlswd)) > + return PTR_ERR(cxlswd); > + > + pci_set_drvdata(pdev, cxlswd); > + > + rc = cxl_enumerate_cmds(cxlds); > + if (rc) > + goto error_put_device; > + > + rc = cdev_device_add(&cxlswd->cdev, &cxlswd->dev); > + if (rc) > + goto error_put_device; > + > + return 0; > + > +error_put_device: If we failed, then no cxlswd release. So, when this memory is released then? Maybe, I don’t follow the code logic here. > + put_device(&cxlswd->dev); > + return rc; > +} > + > +static void cxl_swbmcci_remove(struct pci_dev *pdev) > +{ > + struct cxl_swdev *cxlswd = pci_get_drvdata(pdev); > + struct device *dev = &cxlswd->dev; > + > + cdev_device_del(&cxlswd->cdev, dev); > + put_device(&cxlswd->dev); > +} > + > +static const struct pci_device_id cxl_swmbcci_pci_tbl[] = { > + { PCI_DEVICE_CLASS((0x0c0b << 8 | 0), ~0)}, I believe (0x0c0b << 8 | 0) could be represented by some define, for example. Also, it’s hard to follow what magic 0x0c0b represent. And why 8 was used for shift operation. Maybe, some constant or comments make sense to add? Thanks, Slava. > + {} > +}; > + > +static struct pci_driver cxl_swmbcci_driver = { > + .name = "cxl_swmbcci", > + .id_table = cxl_swmbcci_pci_tbl, > + .probe = cxl_swmbcci_probe, > + .remove = cxl_swbmcci_remove, > +}; > + > +static int __init cxl_pci_init(void) > +{ > + int rc; > + > + rc = pci_register_driver(&cxl_pci_driver); > + if (rc) > + return rc; > + > + rc = pci_register_driver(&cxl_swmbcci_driver); > + if (rc) { > + pci_unregister_driver(&cxl_pci_driver); > + return rc; > + } > + return 0; > +} > +module_init(cxl_pci_init); > +static void __exit cxl_pci_exit(void) > +{ > + pci_unregister_driver(&cxl_swmbcci_driver); > + pci_unregister_driver(&cxl_pci_driver); > +} > +module_exit(cxl_pci_exit); > MODULE_LICENSE("GPL v2"); > -module_pci_driver(cxl_pci_driver); > MODULE_IMPORT_NS(CXL); > diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h > index c71021a2a9ed..ea03a289e56f 100644 > --- a/include/uapi/linux/cxl_mem.h > +++ b/include/uapi/linux/cxl_mem.h > @@ -41,6 +41,9 @@ > ___C(GET_SCAN_MEDIA_CAPS, "Get Scan Media Capabilities"), \ > ___C(SCAN_MEDIA, "Scan Media"), \ > ___C(GET_SCAN_MEDIA, "Get Scan Media Results"), \ > + ___C(INFO_STAT_IDENTIFY, "Get Information"), \ > + ___C(GET_BG_CMD_STATUS, "Background Command Status"), \ > + ___C(IDENTIFY_SWITCH_DEVICE, "Identify Switch Device"), \ > ___C(MAX, "invalid / last command") > > #define ___C(a, b) CXL_MEM_COMMAND_ID_##a > -- > 2.37.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <20221025114251.000031f8@huawei.com>]
* Re: [External] [RFC PATCH 1/1] cxl/pci: Add support for stand alone CXL Switch mailbox CCI [not found] ` <20221025114251.000031f8@huawei.com> @ 2022-10-25 18:45 ` Viacheslav A.Dubeyko 0 siblings, 0 replies; 4+ messages in thread From: Viacheslav A.Dubeyko @ 2022-10-25 18:45 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-cxl, Dan Williams, Alison Schofield, Vishal Verma, Ira Weiny, Ben Widawsky, linuxarm Hi Jonathan, > On Oct 25, 2022, at 3:42 AM, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > >>> <skipped> >>> + >>> +static int cxl_sw_major; >>> +static DEFINE_IDA(cxl_swdev_ida); >>> +static DECLARE_RWSEM(cxl_swdev_rwsem); >> > >> I am not sure how good and safe could be static semaphore. Currently, >> I cannot share any potential troubles with it. But lifetime of this >> semaphore will be the same as module itself. So, it sounds that we >> will take memory even if functionality of this module never will be >> used. And I assume that we pollute the global namespace with the name >> of this semaphore. > > Modeled directly after the equivalent in cxl/core/memdev.c > > I'm not sure I care about the tiny amount of memory the semaphore will > take up. Which namespace are you referring to? From a C point of view > it's static so local to this file. I might be missing another form of > namespace? (maybe debug or something like that?) > I think my worry is more semantical one. Usually, we need to use any synchronization primitive (for example, semaphore) to guarantee consistent access/modification of some variables or structure’s fields. So, if cxl_swdev_rwsem is not part of any structure, then it is not clear semantically what variables or structure’s fields needs to be protected by semaphore. Potentially, it could be source of misunderstanding if anybody else tries to modify the code or fix bugs. But I am OK with current approach and I don’t see any other serious issues here. Thanks, Slava. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-10-25 18:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-24 15:53 [RFC PATCH 0/1] CXL: Standalone switch CCI driver Jonathan Cameron
2022-10-24 15:53 ` [RFC PATCH 1/1] cxl/pci: Add support for stand alone CXL Switch mailbox CCI Jonathan Cameron
2022-10-24 19:17 ` [External] " Viacheslav A.Dubeyko
[not found] ` <20221025114251.000031f8@huawei.com>
2022-10-25 18:45 ` Viacheslav A.Dubeyko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.