Re: [PATCH v3 02/18] nitro_enclaves: Define the PCI device interface
On 01/06/2020 05:59, Benjamin Herrenschmidt wrote: On Wed, 2020-05-27 at 00:21 +0200, Greg KH wrote: There are a couple of data structures with more than one member and multiple field sizes. And for the ones that are not, gathered as feedback from previous rounds of review that should consider adding a "flags" field in there for further extensibility. Please do not do that in ioctls. Just create new calls instead of trying to "extend" existing ones. It's always much easier. I can modify to have "__packed" instead of the attribute callout. Make sure you even need that, as I don't think you do for structures like the above one, right? Hrm, my impression (granted I only just started to look at this code) is that these are protocol messages with the PCI devices, not strictly just ioctl arguments (though they do get conveyed via such ioctls). Andra-Irina, did I get that right ? :-) Correct, these data structures having "__packed" attribute map the messages (requests / replies) for the communication with the NE PCI device. The data structures from the ioctl commands are not directly used as part of the communication with the NE PCI device, but several fields of them e.g. enclave start flags. Some of the fields from the NE PCI device data structures e.g. the physical address of a memory region (gpa) are set by the internal kernel logic. That said, I still think that by carefully ordering the fields and using explicit padding, we can avoid the need of the packed attributed. Regarding your question in the previous mail from this thread and the mention above on the same topic, that should be possible. IIRC, there were 2 data structures remaining with "__packed" attribute. Thank you, Ben. 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 02/18] nitro_enclaves: Define the PCI device interface
On Wed, 2020-05-27 at 00:21 +0200, Greg KH wrote: > > There are a couple of data structures with more than one member and multiple > > field sizes. And for the ones that are not, gathered as feedback from > > previous rounds of review that should consider adding a "flags" field in > > there for further extensibility. > > Please do not do that in ioctls. Just create new calls instead of > trying to "extend" existing ones. It's always much easier. > > > I can modify to have "__packed" instead of the attribute callout. > > Make sure you even need that, as I don't think you do for structures > like the above one, right? Hrm, my impression (granted I only just started to look at this code) is that these are protocol messages with the PCI devices, not strictly just ioctl arguments (though they do get conveyed via such ioctls). Andra-Irina, did I get that right ? :-) That said, I still think that by carefully ordering the fields and using explicit padding, we can avoid the need of the packed attributed. Cheers, Ben.
Re: [PATCH v3 02/18] nitro_enclaves: Define the PCI device interface
On Tue, 2020-05-26 at 20:01 +0300, Paraschiv, Andra-Irina wrote:
>
> On 26/05/2020 09:44, Greg KH wrote:
> > On Tue, May 26, 2020 at 01:13:18AM +0300, Andra Paraschiv wrote:
> > > +struct enclave_get_slot_req {
> > > + /* Context ID (CID) for the enclave vsock device. */
> > > + u64 enclave_cid;
> > > +} __attribute__ ((__packed__));
> >
> > Can you really "pack" a single member structure?
> >
> > Anyway, we have better ways to specify this instead of the "raw"
> > __attribute__ option. But first see if you really need any of
> > these, at
> > first glance, I do not think you do at all, and they can all be
> > removed.
>
> There are a couple of data structures with more than one member and
> multiple field sizes. And for the ones that are not, gathered as
> feedback from previous rounds of review that should consider adding
> a
> "flags" field in there for further extensibility.
>
> I can modify to have "__packed" instead of the attribute callout.
I tend to prefer designing the protocol so that all the fields are
naturally aligned, which should avoid the need for the attribute. Is
it possible in this case ?
Cheers,
Ben.
Re: [PATCH v3 02/18] nitro_enclaves: Define the PCI device interface
On 27/05/2020 01:21, Greg KH wrote:
On Tue, May 26, 2020 at 08:01:36PM +0300, Paraschiv, Andra-Irina wrote:
On 26/05/2020 09:44, Greg KH wrote:
On Tue, May 26, 2020 at 01:13:18AM +0300, Andra Paraschiv wrote:
+struct enclave_get_slot_req {
+ /* Context ID (CID) for the enclave vsock device. */
+ u64 enclave_cid;
+} __attribute__ ((__packed__));
Can you really "pack" a single member structure?
Anyway, we have better ways to specify this instead of the "raw"
__attribute__ option. But first see if you really need any of these, at
first glance, I do not think you do at all, and they can all be removed.
There are a couple of data structures with more than one member and multiple
field sizes. And for the ones that are not, gathered as feedback from
previous rounds of review that should consider adding a "flags" field in
there for further extensibility.
Please do not do that in ioctls. Just create new calls instead of
trying to "extend" existing ones. It's always much easier.
I can modify to have "__packed" instead of the attribute callout.
Make sure you even need that, as I don't think you do for structures
like the above one, right?
For the ones like the above, not, I just customized the usage of "__packed".
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 02/18] nitro_enclaves: Define the PCI device interface
On Tue, May 26, 2020 at 08:01:36PM +0300, Paraschiv, Andra-Irina wrote:
>
>
> On 26/05/2020 09:44, Greg KH wrote:
> > On Tue, May 26, 2020 at 01:13:18AM +0300, Andra Paraschiv wrote:
> > > +struct enclave_get_slot_req {
> > > + /* Context ID (CID) for the enclave vsock device. */
> > > + u64 enclave_cid;
> > > +} __attribute__ ((__packed__));
> > Can you really "pack" a single member structure?
> >
> > Anyway, we have better ways to specify this instead of the "raw"
> > __attribute__ option. But first see if you really need any of these, at
> > first glance, I do not think you do at all, and they can all be removed.
>
> There are a couple of data structures with more than one member and multiple
> field sizes. And for the ones that are not, gathered as feedback from
> previous rounds of review that should consider adding a "flags" field in
> there for further extensibility.
Please do not do that in ioctls. Just create new calls instead of
trying to "extend" existing ones. It's always much easier.
> I can modify to have "__packed" instead of the attribute callout.
Make sure you even need that, as I don't think you do for structures
like the above one, right?
thanks,
greg k-h
Re: [PATCH v3 02/18] nitro_enclaves: Define the PCI device interface
On 26/05/2020 09:44, Greg KH wrote:
On Tue, May 26, 2020 at 01:13:18AM +0300, Andra Paraschiv wrote:
+struct enclave_get_slot_req {
+ /* Context ID (CID) for the enclave vsock device. */
+ u64 enclave_cid;
+} __attribute__ ((__packed__));
Can you really "pack" a single member structure?
Anyway, we have better ways to specify this instead of the "raw"
__attribute__ option. But first see if you really need any of these, at
first glance, I do not think you do at all, and they can all be removed.
There are a couple of data structures with more than one member and
multiple field sizes. And for the ones that are not, gathered as
feedback from previous rounds of review that should consider adding a
"flags" field in there for further extensibility.
I can modify to have "__packed" instead of the attribute callout.
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 02/18] nitro_enclaves: Define the PCI device interface
On Tue, May 26, 2020 at 01:13:18AM +0300, Andra Paraschiv wrote:
> +struct enclave_get_slot_req {
> + /* Context ID (CID) for the enclave vsock device. */
> + u64 enclave_cid;
> +} __attribute__ ((__packed__));
Can you really "pack" a single member structure?
Anyway, we have better ways to specify this instead of the "raw"
__attribute__ option. But first see if you really need any of these, at
first glance, I do not think you do at all, and they can all be removed.
thanks,
greg k-h
[PATCH v3 02/18] nitro_enclaves: Define the PCI device interface
The Nitro Enclaves (NE) driver communicates with a new PCI device, that
is exposed to a virtual machine (VM) and handles commands meant for
handling enclaves lifetime e.g. creation, termination, setting memory
regions. The communication with the PCI device is handled using a MMIO
space and MSI-X interrupts.
This device communicates with the hypervisor on the host, where the VM
that spawned the enclave itself run, e.g. to launch a VM that is used
for the enclave.
Define the MMIO space of the PCI device, the commands that are
provided by this device. Add an internal data structure used as private
data for the PCI device driver and the functions for the PCI device init
/ uninit and command requests handling.
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.
v1 -> v2
* Update path naming to drivers/virt/nitro_enclaves.
* Update NE_ENABLE_OFF / NE_ENABLE_ON defines.
---
drivers/virt/nitro_enclaves/ne_pci_dev.h | 254 +++
1 file changed, 254 insertions(+)
create mode 100644 drivers/virt/nitro_enclaves/ne_pci_dev.h
diff --git a/drivers/virt/nitro_enclaves/ne_pci_dev.h
b/drivers/virt/nitro_enclaves/ne_pci_dev.h
new file mode 100644
index ..eafef6524d97
--- /dev/null
+++ b/drivers/virt/nitro_enclaves/ne_pci_dev.h
@@ -0,0 +1,254 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ */
+
+#ifndef _NE_PCI_DEV_H_
+#define _NE_PCI_DEV_H_
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* Nitro Enclaves (NE) PCI device identifier */
+
+#define PCI_DEVICE_ID_NE (0xe4c1)
+#define PCI_BAR_NE (0x03)
+
+/* Device registers */
+
+/**
+ * (1 byte) Register to notify the device that the driver is using it
+ * (Read/Write).
+ */
+#define NE_ENABLE (0x)
+#define NE_ENABLE_OFF (0x00)
+#define NE_ENABLE_ON (0x01)
+
+/* (2 bytes) Register to select the device run-time version (Read/Write). */
+#define NE_VERSION (0x0002)
+#define NE_VERSION_MAX (0x0001)
+
+/**
+ * (4 bytes) Register to notify the device what command was requested
+ * (Write-Only).
+ */
+#define NE_COMMAND (0x0004)
+
+/**
+ * (4 bytes) Register to notify the driver that a reply or a device event
+ * is available (Read-Only):
+ * - Lower half - command reply counter
+ * - Higher half - out-of-band device event counter
+ */
+#define NE_EVTCNT (0x000c)
+#define NE_EVTCNT_REPLY_SHIFT (0)
+#define NE_EVTCNT_REPLY_MASK (0x)
+#define NE_EVTCNT_REPLY(cnt) (((cnt) & NE_EVTCNT_REPLY_MASK) >> \
+ NE_EVTCNT_REPLY_SHIFT)
+#define NE_EVTCNT_EVENT_SHIFT (16)
+#define NE_EVTCNT_EVENT_MASK (0x)
+#define NE_EVTCNT_EVENT(cnt) (((cnt) & NE_EVTCNT_EVENT_MASK) >> \
+ NE_EVTCNT_EVENT_SHIFT)
+
+/* (240 bytes) Buffer for sending the command request payload (Read/Write). */
+#define NE_SEND_DATA (0x0010)
+
+/* (240 bytes) Buffer for receiving the command reply payload (Read-Only). */
+#define NE_RECV_DATA (0x0100)
+
+/* Device MMIO buffer sizes */
+
+/* 240 bytes for send / recv buffer. */
+#define NE_SEND_DATA_SIZE (240)
+#define NE_RECV_DATA_SIZE (240)
+
+/* MSI-X interrupt vectors */
+
+/* MSI-X vector used for command reply notification. */
+#define NE_VEC_REPLY (0)
+
+/* MSI-X vector used for out-of-band events e.g. enclave crash. */
+#define NE_VEC_EVENT (1)
+
+/* Device command types. */
+enum ne_pci_dev_cmd_type {
+ INVALID_CMD = 0,
+ ENCLAVE_START = 1,
+ ENCLAVE_GET_SLOT = 2,
+ ENCLAVE_STOP = 3,
+ SLOT_ALLOC = 4,
+ SLOT_FREE = 5,
+ SLOT_ADD_MEM = 6,
+ SLOT_ADD_VCPU = 7,
+ SLOT_COUNT = 8,
+ NEXT_SLOT = 9,
+ SLOT_INFO = 10,
+ SLOT_ADD_BULK_VCPUS = 11,
+ MAX_CMD,
+};
+
+/* Device commands - payload structure for requests and replies. */
+
+struct enclave_start_req {
+ /* Slot unique id mapped to the enclave to start. */
+ u64 slot_uid;
+
+ /**
+* Context ID (CID) for the enclave vsock device.
+* If 0, CID is autogenerated.
+*/
+ u64 enclave_cid;
+
+ /* Flags for the enclave to start with (e.g. debug mode). */
+ u64 flags;
+} __attribute__ ((__packed__));
+
+struct enclave_get_slot_req {
+ /* Context ID (CID) for the enclave vsock device. */
+ u64 enclave_cid;
+} __attribute__ ((__packed__));
+
+struct enclave_stop_req {
+ /* Slot unique id mapped to the enclave to stop. */
+ u64 slot_uid;
+} __attribute__ ((__packed__));
+
+struct slot_alloc_req {
+ /* In order to avoid weird sizeof edge cases. */
+ u8 unused;
+} __attribute__ ((__packed__));
+
+struct slot_free_req {
+ /* Slot unique id mapped to the slot to free. */
+ u64 slot_uid;
+} __attribute__ ((__packed__));
+
+struct slot_add_mem_req {
+ /* Slot unique id mapped to
