Re: [PATCH v3 04/18] nitro_enclaves: Init PCI device driver
On 01/06/2020 05:55, Benjamin Herrenschmidt wrote: On Tue, 2020-05-26 at 21:35 +0300, Paraschiv, Andra-Irina wrote: This was needed to have an identifier for the overall NE logic - PCI dev, ioctl and misc dev. The ioctl and misc dev logic has pr_* logs, but I can update them to dev_* with misc dev, then remove this prefix. Or #define pr_fmt, but dev_ is better. Yep, the codebase now includes dev_* usage overall. Thanks, Andra Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
Re: [PATCH v3 04/18] nitro_enclaves: Init PCI device driver
On Tue, 2020-05-26 at 21:35 +0300, Paraschiv, Andra-Irina wrote: > This was needed to have an identifier for the overall NE logic - PCI > dev, ioctl and misc dev. > > The ioctl and misc dev logic has pr_* logs, but I can update them to > dev_* with misc dev, then remove this prefix. Or #define pr_fmt, but dev_ is better. Cheers, Ben.
Re: [PATCH v3 04/18] nitro_enclaves: Init PCI device driver
On 27/05/2020 01:19, Greg KH wrote: On Tue, May 26, 2020 at 09:35:33PM +0300, Paraschiv, Andra-Irina wrote: On 26/05/2020 09:48, Greg KH wrote: On Tue, May 26, 2020 at 01:13:20AM +0300, Andra Paraschiv wrote: The Nitro Enclaves PCI device is used by the kernel driver as a means of communication with the hypervisor on the host where the primary VM and the enclaves run. It handles requests with regard to enclave lifetime. Setup the PCI device driver and add support for MSI-X interrupts. Signed-off-by: Alexandru-Catalin Vasile Signed-off-by: Alexandru Ciobotaru Signed-off-by: Andra Paraschiv --- Changelog v2 -> v3 * Remove the GPL additional wording as SPDX-License-Identifier is already in place. * Remove the WARN_ON calls. * Remove linux/bug include that is not needed. * Update static calls sanity checks. * Remove "ratelimited" from the logs that are not in the ioctl call paths. * Update kzfree() calls to kfree(). v1 -> v2 * Add log pattern for NE. * Update PCI device setup functions to receive PCI device data structure and then get private data from it inside the functions logic. * Remove the BUG_ON calls. * Add teardown function for MSI-X setup. * Update goto labels to match their purpose. * Implement TODO for NE PCI device disable state check. * Update function name for NE PCI device probe / remove. --- drivers/virt/nitro_enclaves/ne_pci_dev.c | 252 +++ 1 file changed, 252 insertions(+) create mode 100644 drivers/virt/nitro_enclaves/ne_pci_dev.c diff --git a/drivers/virt/nitro_enclaves/ne_pci_dev.c b/drivers/virt/nitro_enclaves/ne_pci_dev.c new file mode 100644 index ..0b66166787b6 --- /dev/null +++ b/drivers/virt/nitro_enclaves/ne_pci_dev.c @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + */ + +/* Nitro Enclaves (NE) PCI device driver. */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "ne_misc_dev.h" +#include "ne_pci_dev.h" + +#define DEFAULT_TIMEOUT_MSECS (12) /* 120 sec */ + +#define NE "nitro_enclaves: " Why is this needed? The dev_* functions should give you all the information that you need to properly describe the driver and device in question. No extra "prefixes" should be needed at all. This was needed to have an identifier for the overall NE logic - PCI dev, ioctl and misc dev. Why? They are all different "devices", and refer to different interfaces. Stick to what the dev_* gives you for them. You probably want to stick with the pci dev for almost all of those anyway. The ioctl and misc dev logic has pr_* logs, but I can update them to dev_* with misc dev, then remove this prefix. That would be good, thanks. That's already in v4, the pr_* logs are now replaced with dev_*. Thanks, Andra Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
Re: [PATCH v3 04/18] nitro_enclaves: Init PCI device driver
On Tue, May 26, 2020 at 09:35:33PM +0300, Paraschiv, Andra-Irina wrote: > > > On 26/05/2020 09:48, Greg KH wrote: > > On Tue, May 26, 2020 at 01:13:20AM +0300, Andra Paraschiv wrote: > > > The Nitro Enclaves PCI device is used by the kernel driver as a means of > > > communication with the hypervisor on the host where the primary VM and > > > the enclaves run. It handles requests with regard to enclave lifetime. > > > > > > Setup the PCI device driver and add support for MSI-X interrupts. > > > > > > Signed-off-by: Alexandru-Catalin Vasile > > > Signed-off-by: Alexandru Ciobotaru > > > Signed-off-by: Andra Paraschiv > > > --- > > > Changelog > > > > > > v2 -> v3 > > > > > > * Remove the GPL additional wording as SPDX-License-Identifier is already > > > in > > > place. > > > * Remove the WARN_ON calls. > > > * Remove linux/bug include that is not needed. > > > * Update static calls sanity checks. > > > * Remove "ratelimited" from the logs that are not in the ioctl call paths. > > > * Update kzfree() calls to kfree(). > > > > > > v1 -> v2 > > > > > > * Add log pattern for NE. > > > * Update PCI device setup functions to receive PCI device data structure > > > and > > > then get private data from it inside the functions logic. > > > * Remove the BUG_ON calls. > > > * Add teardown function for MSI-X setup. > > > * Update goto labels to match their purpose. > > > * Implement TODO for NE PCI device disable state check. > > > * Update function name for NE PCI device probe / remove. > > > --- > > > drivers/virt/nitro_enclaves/ne_pci_dev.c | 252 +++ > > > 1 file changed, 252 insertions(+) > > > create mode 100644 drivers/virt/nitro_enclaves/ne_pci_dev.c > > > > > > diff --git a/drivers/virt/nitro_enclaves/ne_pci_dev.c > > > b/drivers/virt/nitro_enclaves/ne_pci_dev.c > > > new file mode 100644 > > > index ..0b66166787b6 > > > --- /dev/null > > > +++ b/drivers/virt/nitro_enclaves/ne_pci_dev.c > > > @@ -0,0 +1,252 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights > > > Reserved. > > > + */ > > > + > > > +/* Nitro Enclaves (NE) PCI device driver. */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#include "ne_misc_dev.h" > > > +#include "ne_pci_dev.h" > > > + > > > +#define DEFAULT_TIMEOUT_MSECS (12) /* 120 sec */ > > > + > > > +#define NE "nitro_enclaves: " > > Why is this needed? The dev_* functions should give you all the > > information that you need to properly describe the driver and device in > > question. No extra "prefixes" should be needed at all. > > This was needed to have an identifier for the overall NE logic - PCI dev, > ioctl and misc dev. Why? They are all different "devices", and refer to different interfaces. Stick to what the dev_* gives you for them. You probably want to stick with the pci dev for almost all of those anyway. > The ioctl and misc dev logic has pr_* logs, but I can update them to dev_* > with misc dev, then remove this prefix. That would be good, thanks. greg k-h
Re: [PATCH v3 04/18] nitro_enclaves: Init PCI device driver
On 26/05/2020 09:48, Greg KH wrote: On Tue, May 26, 2020 at 01:13:20AM +0300, Andra Paraschiv wrote: The Nitro Enclaves PCI device is used by the kernel driver as a means of communication with the hypervisor on the host where the primary VM and the enclaves run. It handles requests with regard to enclave lifetime. Setup the PCI device driver and add support for MSI-X interrupts. Signed-off-by: Alexandru-Catalin Vasile Signed-off-by: Alexandru Ciobotaru Signed-off-by: Andra Paraschiv --- Changelog v2 -> v3 * Remove the GPL additional wording as SPDX-License-Identifier is already in place. * Remove the WARN_ON calls. * Remove linux/bug include that is not needed. * Update static calls sanity checks. * Remove "ratelimited" from the logs that are not in the ioctl call paths. * Update kzfree() calls to kfree(). v1 -> v2 * Add log pattern for NE. * Update PCI device setup functions to receive PCI device data structure and then get private data from it inside the functions logic. * Remove the BUG_ON calls. * Add teardown function for MSI-X setup. * Update goto labels to match their purpose. * Implement TODO for NE PCI device disable state check. * Update function name for NE PCI device probe / remove. --- drivers/virt/nitro_enclaves/ne_pci_dev.c | 252 +++ 1 file changed, 252 insertions(+) create mode 100644 drivers/virt/nitro_enclaves/ne_pci_dev.c diff --git a/drivers/virt/nitro_enclaves/ne_pci_dev.c b/drivers/virt/nitro_enclaves/ne_pci_dev.c new file mode 100644 index ..0b66166787b6 --- /dev/null +++ b/drivers/virt/nitro_enclaves/ne_pci_dev.c @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + */ + +/* Nitro Enclaves (NE) PCI device driver. */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "ne_misc_dev.h" +#include "ne_pci_dev.h" + +#define DEFAULT_TIMEOUT_MSECS (12) /* 120 sec */ + +#define NE "nitro_enclaves: " Why is this needed? The dev_* functions should give you all the information that you need to properly describe the driver and device in question. No extra "prefixes" should be needed at all. This was needed to have an identifier for the overall NE logic - PCI dev, ioctl and misc dev. The ioctl and misc dev logic has pr_* logs, but I can update them to dev_* with misc dev, then remove this prefix. Thank you. Andra Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
Re: [PATCH v3 04/18] nitro_enclaves: Init PCI device driver
On Tue, May 26, 2020 at 01:13:20AM +0300, Andra Paraschiv wrote: > The Nitro Enclaves PCI device is used by the kernel driver as a means of > communication with the hypervisor on the host where the primary VM and > the enclaves run. It handles requests with regard to enclave lifetime. > > Setup the PCI device driver and add support for MSI-X interrupts. > > Signed-off-by: Alexandru-Catalin Vasile > Signed-off-by: Alexandru Ciobotaru > Signed-off-by: Andra Paraschiv > --- > Changelog > > v2 -> v3 > > * Remove the GPL additional wording as SPDX-License-Identifier is already in > place. > * Remove the WARN_ON calls. > * Remove linux/bug include that is not needed. > * Update static calls sanity checks. > * Remove "ratelimited" from the logs that are not in the ioctl call paths. > * Update kzfree() calls to kfree(). > > v1 -> v2 > > * Add log pattern for NE. > * Update PCI device setup functions to receive PCI device data structure and > then get private data from it inside the functions logic. > * Remove the BUG_ON calls. > * Add teardown function for MSI-X setup. > * Update goto labels to match their purpose. > * Implement TODO for NE PCI device disable state check. > * Update function name for NE PCI device probe / remove. > --- > drivers/virt/nitro_enclaves/ne_pci_dev.c | 252 +++ > 1 file changed, 252 insertions(+) > create mode 100644 drivers/virt/nitro_enclaves/ne_pci_dev.c > > diff --git a/drivers/virt/nitro_enclaves/ne_pci_dev.c > b/drivers/virt/nitro_enclaves/ne_pci_dev.c > new file mode 100644 > index ..0b66166787b6 > --- /dev/null > +++ b/drivers/virt/nitro_enclaves/ne_pci_dev.c > @@ -0,0 +1,252 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. > + */ > + > +/* Nitro Enclaves (NE) PCI device driver. */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "ne_misc_dev.h" > +#include "ne_pci_dev.h" > + > +#define DEFAULT_TIMEOUT_MSECS (12) /* 120 sec */ > + > +#define NE "nitro_enclaves: " Why is this needed? The dev_* functions should give you all the information that you need to properly describe the driver and device in question. No extra "prefixes" should be needed at all. thanks, greg k-h
[PATCH v3 04/18] nitro_enclaves: Init PCI device driver
The Nitro Enclaves PCI device is used by the kernel driver as a means of
communication with the hypervisor on the host where the primary VM and
the enclaves run. It handles requests with regard to enclave lifetime.
Setup the PCI device driver and add support for MSI-X interrupts.
Signed-off-by: Alexandru-Catalin Vasile
Signed-off-by: Alexandru Ciobotaru
Signed-off-by: Andra Paraschiv
---
Changelog
v2 -> v3
* Remove the GPL additional wording as SPDX-License-Identifier is already in
place.
* Remove the WARN_ON calls.
* Remove linux/bug include that is not needed.
* Update static calls sanity checks.
* Remove "ratelimited" from the logs that are not in the ioctl call paths.
* Update kzfree() calls to kfree().
v1 -> v2
* Add log pattern for NE.
* Update PCI device setup functions to receive PCI device data structure and
then get private data from it inside the functions logic.
* Remove the BUG_ON calls.
* Add teardown function for MSI-X setup.
* Update goto labels to match their purpose.
* Implement TODO for NE PCI device disable state check.
* Update function name for NE PCI device probe / remove.
---
drivers/virt/nitro_enclaves/ne_pci_dev.c | 252 +++
1 file changed, 252 insertions(+)
create mode 100644 drivers/virt/nitro_enclaves/ne_pci_dev.c
diff --git a/drivers/virt/nitro_enclaves/ne_pci_dev.c
b/drivers/virt/nitro_enclaves/ne_pci_dev.c
new file mode 100644
index ..0b66166787b6
--- /dev/null
+++ b/drivers/virt/nitro_enclaves/ne_pci_dev.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ */
+
+/* Nitro Enclaves (NE) PCI device driver. */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "ne_misc_dev.h"
+#include "ne_pci_dev.h"
+
+#define DEFAULT_TIMEOUT_MSECS (12) /* 120 sec */
+
+#define NE "nitro_enclaves: "
+
+static const struct pci_device_id ne_pci_ids[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_AMAZON, PCI_DEVICE_ID_NE) },
+ { 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, ne_pci_ids);
+
+/**
+ * ne_setup_msix - Setup MSI-X vectors for the PCI device.
+ *
+ * @pdev: PCI device to setup the MSI-X for.
+ *
+ * @returns: 0 on success, negative return value on failure.
+ */
+static int ne_setup_msix(struct pci_dev *pdev)
+{
+ struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
+ int nr_vecs = 0;
+ int rc = -EINVAL;
+
+ nr_vecs = pci_msix_vec_count(pdev);
+ if (nr_vecs < 0) {
+ rc = nr_vecs;
+
+ dev_err(&pdev->dev, NE "Error in getting vec count [rc=%d]\n",
+ rc);
+
+ return rc;
+ }
+
+ rc = pci_alloc_irq_vectors(pdev, nr_vecs, nr_vecs, PCI_IRQ_MSIX);
+ if (rc < 0) {
+ dev_err(&pdev->dev, NE "Error in alloc MSI-X vecs [rc=%d]\n",
+ rc);
+
+ return rc;
+ }
+
+ return 0;
+}
+
+/**
+ * ne_teardown_msix - Teardown MSI-X vectors for the PCI device.
+ *
+ * @pdev: PCI device to teardown the MSI-X for.
+ */
+static void ne_teardown_msix(struct pci_dev *pdev)
+{
+ struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
+
+ pci_free_irq_vectors(pdev);
+}
+
+/**
+ * ne_pci_dev_enable - Select PCI device version and enable it.
+ *
+ * @pdev: PCI device to select version for and then enable.
+ *
+ * @returns: 0 on success, negative return value on failure.
+ */
+static int ne_pci_dev_enable(struct pci_dev *pdev)
+{
+ u8 dev_enable_reply = 0;
+ u16 dev_version_reply = 0;
+ struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
+
+ iowrite16(NE_VERSION_MAX, ne_pci_dev->iomem_base + NE_VERSION);
+
+ dev_version_reply = ioread16(ne_pci_dev->iomem_base + NE_VERSION);
+ if (dev_version_reply != NE_VERSION_MAX) {
+ dev_err(&pdev->dev, NE "Error in pci dev version cmd\n");
+
+ return -EIO;
+ }
+
+ iowrite8(NE_ENABLE_ON, ne_pci_dev->iomem_base + NE_ENABLE);
+
+ dev_enable_reply = ioread8(ne_pci_dev->iomem_base + NE_ENABLE);
+ if (dev_enable_reply != NE_ENABLE_ON) {
+ dev_err(&pdev->dev, NE "Error in pci dev enable cmd\n");
+
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/**
+ * ne_pci_dev_disable - Disable PCI device.
+ *
+ * @pdev: PCI device to disable.
+ */
+static void ne_pci_dev_disable(struct pci_dev *pdev)
+{
+ u8 dev_disable_reply = 0;
+ struct ne_pci_dev *ne_pci_dev = pci_get_drvdata(pdev);
+ const unsigned int sleep_time = 10; /* 10 ms */
+ unsigned int sleep_time_count = 0;
+
+ iowrite8(NE_ENABLE_OFF, ne_pci_dev->iomem_base + NE_ENABLE);
+
+ /*
+* Check for NE_ENABLE_OFF in a loop, to handle cases when the device
+* state is not immediately set to disabled and going through a
+* transitory state of disabling.
+*/
+ while (sleep_time_count < DEFAULT_TI
