| Message ID | 20171020023413.122280-14-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On Thu, Oct 19, 2017 at 09:33:48PM -0500, Brijesh Singh wrote: > +static int __sev_platform_init(struct sev_data_init *data, int *error) > +{ > + int rc = 0; > + > + mutex_lock(&fw_init_mutex); > + > + if (!fw_init_count) { I still don't like global semaphores. Can you get the status and check for PSTATE.INIT state and do the init only if the platform is in PSTATE.UNINIT?
On 10/23/2017 04:20 AM, Borislav Petkov wrote: > On Thu, Oct 19, 2017 at 09:33:48PM -0500, Brijesh Singh wrote: >> +static int __sev_platform_init(struct sev_data_init *data, int *error) >> +{ >> + int rc = 0; >> + >> + mutex_lock(&fw_init_mutex); >> + >> + if (!fw_init_count) { > > I still don't like global semaphores. Can you get the status and check > for PSTATE.INIT state and do the init only if the platform is in > PSTATE.UNINIT? > Calling PLATFORM_GET_STATUS is not required, we can manage the state through a simple ref count variable. Issuing PSP commands will always be much more expensive compare to accessing a protected global variable. I would prefer to avoid invoking PSP command if possible. Additionally, the global semaphore is still needed to serialize the sev_platform_init() and sev_platform_shutdown() from multiple processes. e.g If process "A" calls sev_platform_init() and if it gets preempted due to whatever reason then we don't want another process to issue the shutdown command while process "A" is in middle of sev_platform_init(). -Brijesh
On Mon, Oct 23, 2017 at 02:57:04PM -0500, Brijesh Singh wrote: > Calling PLATFORM_GET_STATUS is not required, we can manage the state through > a simple ref count variable. Issuing PSP commands will always be much more > expensive compare to accessing a protected global variable. What does "protected" mean here? In any case, that variable can be a simple bool as you use it as such. > I would prefer to avoid invoking PSP command if possible. > Additionally, the global semaphore is still needed to serialize > the sev_platform_init() and sev_platform_shutdown() from multiple > processes. e.g If process "A" calls sev_platform_init() and if it gets > preempted due to whatever reason then we don't want another process > to issue the shutdown command while process "A" is in middle of > sev_platform_init(). How? You're holding fw_init_mutex.
On 10/26/2017 08:56 AM, Borislav Petkov wrote: > On Mon, Oct 23, 2017 at 02:57:04PM -0500, Brijesh Singh wrote: >> Calling PLATFORM_GET_STATUS is not required, we can manage the state through >> a simple ref count variable. Issuing PSP commands will always be much more >> expensive compare to accessing a protected global variable. > > What does "protected" mean here? > Access global variable after acquiring the semaphore. > In any case, that variable can be a simple bool as you use it as such. > I am not using the variable (fw_init_count) as boolean. The variable gets incremented in sev_platform_init() and decremented in sev_platform_shutdown(). In very first call to sev_platform_init (i.e when variable is zero) we issue PLATFORM_INIT command, similarly PLATFORM_SHUTDOWN is issued on the last (i.e when variable value is reached to zero). The variable is used as ref counter. >> I would prefer to avoid invoking PSP command if possible. >> Additionally, the global semaphore is still needed to serialize >> the sev_platform_init() and sev_platform_shutdown() from multiple >> processes. e.g If process "A" calls sev_platform_init() and if it gets >> preempted due to whatever reason then we don't want another process >> to issue the shutdown command while process "A" is in middle of >> sev_platform_init(). > > How? You're holding fw_init_mutex. > In your previous reply you comments on global semaphore (fw_init_mutex) and in response I tried to highlight why we need the global semaphore. Did I misunderstood your comment ? -Brijesh
On Thu, Oct 26, 2017 at 11:56:57AM -0500, Brijesh Singh wrote: > The variable is used as ref counter. ... and it can't be converted to a boolean because...? > In your previous reply you comments on global semaphore (fw_init_mutex) and > in response I tried to highlight why we need the global semaphore. Did I > misunderstood your comment ? Yes, what happens if you get preempted while holding the mutex? Will the other process be able to do anything?
On 10/26/2017 12:44 PM, Borislav Petkov wrote: > On Thu, Oct 26, 2017 at 11:56:57AM -0500, Brijesh Singh wrote: >> The variable is used as ref counter. > > ... and it can't be converted to a boolean because...? > SHUTDOWN command unconditionally transitions a platform to uninitialized state. The command does not care how many processes are actively using the PSP. We don't want to shutdown the firmware while other process is still using it. e.g consider three processes (A, B, C) Process A: ---------- sev_platform_init() sev_do_cmd(..) ... ... sev_do_cmd(..) ... sev_platform_shutdown() Process B: ----------- sev_platform_init() sev_do_cmd(...) sev_platform_shutdown() Process C: ---------- sev_platform_init() sev_do_cmd(...) sev_do_cmd(...) sev_do_cmd(...) sev_platform_shutdown() As per the SEV spec section 5.1.2 (platform state machine), several commands require that platform should be initialized before issuing the actual command. As you can see Process B may finish quickly and SHUTDOWN from process B will simply uninitialize the firmware and cause unexpected result to process A and C. >> In your previous reply you comments on global semaphore (fw_init_mutex) and >> in response I tried to highlight why we need the global semaphore. Did I >> misunderstood your comment ? > > Yes, what happens if you get preempted while holding the mutex? Will the other > process be able to do anything? > If other process tries to issue the sev_platform_init/shutdown() then they have to wait. The sev_platform_init() and sev_platform_shutdown() uses the same global mutex. See the original code below. +static int __sev_platform_init(struct sev_data_init *data, int *error) +{ + int rc = 0; + + mutex_lock(&fw_init_mutex); + + if (!fw_init_count) { + rc = sev_do_cmd(SEV_CMD_INIT, data, error); + if (rc) + goto unlock; + } + + fw_init_count++; + +unlock: + mutex_unlock(&fw_init_mutex); + return rc; + +} + +int sev_platform_shutdown(int *error) +{ + int rc = 0; + + mutex_lock(&fw_init_mutex); + + if (!fw_init_count) + goto unlock; + + if (fw_init_count == 1) { + rc = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error); + if (rc) + goto unlock; + } + + fw_init_count--; + +unlock: + mutex_unlock(&fw_init_mutex); + return rc; +}
On Thu, Oct 26, 2017 at 02:26:15PM -0500, Brijesh Singh wrote: > SHUTDOWN command unconditionally transitions a platform to uninitialized > state. The command does not care how many processes are actively using the > PSP. We don't want to shutdown the firmware while other process is still > using it. So why do you have to init and shutdown the PSP each time you execute a command? Why isn't the PSP initialized, *exactly* *once* at driver init and shut down, also exactly once at driver exit? > If other process tries to issue the sev_platform_init/shutdown() then they > have to wait. Exactly, and not what you said earlier: "If process "A" calls sev_platform_init() and if it gets preempted due to whatever reason then we don't want another process to issue the shutdown command while process "A" is in middle of sev_platform_init()." IOW, if your critical regions are protected properly by a mutex, nothing like the above will happen. But what you're trying to explain to me is that the fw_init_count is going to prevent a premature shutdown when it is > 1. But that's not what I meant... Anyway, see my question above.
On 10/26/2017 03:13 PM, Borislav Petkov wrote: > On Thu, Oct 26, 2017 at 02:26:15PM -0500, Brijesh Singh wrote: >> SHUTDOWN command unconditionally transitions a platform to uninitialized >> state. The command does not care how many processes are actively using the >> PSP. We don't want to shutdown the firmware while other process is still >> using it. > > So why do you have to init and shutdown the PSP each time you execute a > command? Why isn't the PSP initialized, *exactly* *once* at driver init > and shut down, also exactly once at driver exit? Wish we could do that but the following reasons makes things complicated: 1) The commands must be issued from the PSP master devices, at PSP initialization time we do not know the PSP 'master' device. Hence we will not able to invoke sev_platform_init() during the PSP initialization time. 2) some commands require the platform to be in UNINIT state -- e.g FACTORY_RESET. So, if we do the INIT at the PSP initialization time then we still need to perform the SHUTDOWN outside the normal code flow to handle these commands. we can workaround #1 by adding some hooks in sp_pci_init() to invoke the PSP initialization routines after pci_register_driver() is done but #2 can get painful because it will require us calling the SHUTDOWN outside the sp_pci_exit() code flow. -Brijesh
On Thu, Oct 26, 2017 at 03:59:32PM -0500, Brijesh Singh wrote: > we can workaround #1 by adding some hooks in sp_pci_init() to invoke the PSP > initialization routines after pci_register_driver() is done but #2 can get > painful because it will require us calling the SHUTDOWN outside the > sp_pci_exit() code flow. Ok, do that and init the PSP master and then put the device in UNINIT state only in the functions which execute those commands which need the device to be in UNINIT state, e.g., wrap the SEV_CMD_FACTORY_RESET glue in a command function which does put the device in the UNINIT state as a first step. Then, when that function is done, put the device in the mode which the other commands would expect it to be in, e.g., INIT state. This way you'll simplify the whole command flow considerably and won't have to "toggle" the device each time and will save yourself a lot of time on command execution. Thx.
On 10/27/17 2:56 AM, Borislav Petkov wrote: > On Thu, Oct 26, 2017 at 03:59:32PM -0500, Brijesh Singh wrote: >> we can workaround #1 by adding some hooks in sp_pci_init() to invoke the PSP >> initialization routines after pci_register_driver() is done but #2 can get >> painful because it will require us calling the SHUTDOWN outside the >> sp_pci_exit() code flow. > Ok, do that and init the PSP master and then put the device in UNINIT > state only in the functions which execute those commands which need the > device to be in UNINIT state, e.g., wrap the SEV_CMD_FACTORY_RESET glue > in a command function which does put the device in the UNINIT state as a > first step. transiting a platform in UINIT state to handle the FACTORY_RESET can have a negative consequence. Consider this scenario: Process A --------- sev_launch_start(...) while (count < 10000) { sev_launch_update(...) } sev_launch_finish() ... ... Process B: --------- .... sev_factory_reset(); .... If in order to handle the FACTORY_RESET we transition a platform in UINIT state then it will results as unexpected failure from the sev_launch_update() because the FACTORY_RESET command remove all the state information created by sev_launch_start() etc. I think our design so far is simple, if command require INIT state then caller executes sev_platform_init(), then command and finish with sev_platform_shutdown(). If command does not require INIT state, then simply issue the command. e.g currently, when caller issues FACTORY_RESET then we pass command directly to PSP and if FW is in INIT state then FACTORY_RESET returns error (INVALID_STATE/EBUSY) and we propagate the error code to userspace. User can retry the command sometime later when nobody else is using the PSP. > > Then, when that function is done, put the device in the mode which the > other commands would expect it to be in, e.g., INIT state. > > This way you'll simplify the whole command flow considerably and won't > have to "toggle" the device each time and will save yourself a lot of > time on command execution. > > Thx. >
On Fri, Oct 27, 2017 at 06:28:38AM -0500, Brijesh Singh wrote: > ... User can retry the command sometime later when nobody else is > using the PSP. That still doesn't prevent you from doing two things: * make that fw_init_count a proper kref instead of your homegrown thing * do not preemptively execute commands on the PSP if you can't possibly know what the next command is going to be - instead, just put it in the required state only when you really have to. I.e., don't do all that unnecessary INIT -> CMD -> SHUTDOWN game for no reason. Thx.
On 10/27/17 3:15 PM, Borislav Petkov wrote: > On Fri, Oct 27, 2017 at 06:28:38AM -0500, Brijesh Singh wrote: >> ... User can retry the command sometime later when nobody else is >> using the PSP. > That still doesn't prevent you from doing two things: > > * make that fw_init_count a proper kref instead of your homegrown thing OK, I can use kref in next patch. > > * do not preemptively execute commands on the PSP if you can't possibly > know what the next command is going to be - instead, just put it in the > required state only when you really have to. I.e., don't do all that > unnecessary INIT -> CMD -> SHUTDOWN game for no reason. Yep, we are doing state transition only when we really need to. At least so far I have tried to avoid making any unnecessary state transitions. > Thx. >
On Fri, Oct 27, 2017 at 03:25:24PM -0500, Brijesh Singh wrote: > Yep, we are doing state transition only when we really need to. At least > so far I have tried to avoid making any unnecessary state transitions. So change all those which do INIT -> CMD -> SHUTDOWN to do only the command as the INIT state is the default state, AFAIU it.
On 10/27/17 3:27 PM, Borislav Petkov wrote: > On Fri, Oct 27, 2017 at 03:25:24PM -0500, Brijesh Singh wrote: >> Yep, we are doing state transition only when we really need to. At least >> so far I have tried to avoid making any unnecessary state transitions. > So change all those which do INIT -> CMD -> SHUTDOWN to do only the > command as the INIT state is the default state, AFAIU it. > I don't that will work. It may be possible that I am not able to follow what exactly you have in mind. Please let me clarify it, on boot the firmware is in UINIT state. Firmware maintain three states: UINIT, INIT and WORKING. Few commands can be executed in UINIT, several command can be executed in INIT, some command can be executed in WORKING and few commands can be executed in any states (see SEV spec for platform state). We do not have ioctls to issue the INIT and SHUTDOWN command. As I have explained in previous messages, the SHUTDOWN unconditionally destory's the FW context hence we have refrained from providing the access for this command to userspace. My approach is, when userspace issues a command we check if command requires INIT state, if so then we do INIT -> CMD -> SHUTDOWN. If command can be executed in any state then we issue the command . If command need to be executed in UINIT state then we *do not* do SHUTDOWN->CMD, instead we issue the cmd and PSP will fail with error code and caller can check the error code to determine why command failed. If I go with your recommendation then I am not able to see who will transition the platform from UINIT -> INIT so that command can run? Lets try with very simple example: # modprobe ccp /* FW is in INIT state */ # userspace runs ioctl(fd, SEV_USER_PEK_GEN, &err) This will fail because PEK_GEN require the platform in INIT state and nobody has done the state transition from INIT -> UINIT. -Brijesh
On Fri, Oct 27, 2017 at 04:28:31PM -0500, Brijesh Singh wrote: > This will fail because PEK_GEN require the platform in INIT state and > nobody has done the state transition from INIT -> UINIT. Huh, FW is in INIT state and PEK_GEN wants it to be in INIT state. Typo? Aaanyway, I don't like this whole notion of prematurely and predictively executing commands on the PSP if it is not needed. So how about executing only those commands which put the FW in the required state and then executing the actual command? I.e., if a command needs to be executed in UINIT state, you put the PSP in that state before executing that command. If the command needs to be in INIT state, you put the PSP in INIT state first and so on... For convenience, you could carry the current PSP state in some struct psp_dev member or whatever and query it before running the respective commands. Hmmm?
On 10/27/17 4:49 PM, Borislav Petkov wrote: > On Fri, Oct 27, 2017 at 04:28:31PM -0500, Brijesh Singh wrote: >> This will fail because PEK_GEN require the platform in INIT state and >> nobody has done the state transition from INIT -> UINIT. > Huh, FW is in INIT state and PEK_GEN wants it to be in INIT state. Typo? Yes it is typo. PEK_GEN wants FW to be in INIT state hence someone need to transition from UNINIT -> INIT. > Aaanyway, I don't like this whole notion of prematurely and predictively > executing commands on the PSP if it is not needed. So how about > executing only those commands which put the FW in the required state and > then executing the actual command? That's what I am doing except FACTORY_RESET. The FACTORY_RESET require the FW to be in UNINIT state, since we don't who else is running in parallel hence its not safe to issue the SHUTDOWN to transition from INIT -> UNINIT. If FW is not in correct state this command will fail with error code (INVALID_STATE) and user can retry (please note that user can always use PLATFORM_STATUS to query the current FW state before issuing a command). I see that we can do a small optimization -- since we already know the FW state hence we can avoid issuing PSP command when we know for sure that command will fail because we are not in correct state. > > I.e., if a command needs to be executed in UINIT state, you put the PSP > in that state before executing that command. If the command needs to be > in INIT state, you put the PSP in INIT state first and so on... If command needs INIT state and FW is not in INIT state then its safe to transition from UNINIT -> INIT. But if command needs UNINIT state and FW is in INIT state then its not safe to transition -- in those case we simply return EBUSY and let the user retry the command.
On Fri, Oct 27, 2017 at 05:59:23PM -0500, Brijesh Singh wrote: > Yes it is typo. PEK_GEN wants FW to be in INIT state hence someone need > to transition from UNINIT -> INIT. Which, once you've done it once on driver init, is there. > That's what I am doing except FACTORY_RESET. Well, not really. Lemme pick a command at random... PEK_CSR. For that, you do INIT -> PEK_CSR -> SHUTDOWN. Doc says, platform needs to be in INIT or WORKING state. But nothing says you should shut it down. Spec says, SHUTDOWN transitions platform to UNINIT state. So when the next command comes in which needs the platform to be in INIT state, you go and INIT it again. For no reason *WHATSOEVER*! I know, you're gonna say, but what if the next command needs a different state than INIT. Well, *then* you transition it, in the command function. When that function executes. But not before that and not in preparation that *maybe* the next command will be it. Now, if you did: INIT once during driver init PEK_CSR (platform remains in INIT state) <--- the next command here can execute directly if it is allowed in INIT state. Instead, the platform has been shutdown and you init it again. Do you see now what I mean? IOW, once you init the PSP master, you should keep it in the INIT state - or the state in which most commands expect it to be and thus save yourself all that unnecessary toggling. If a command needs it to be in a different state, only *then* you transition it. Instead, what you have now is that you call INIT and SHUTDOWN around SEV_PEK_GEN, SEV_PDH_GEN, SEV_PEK_CSR, SEV_PEK_CERT_IMPORT, SEV_PDH_CERT_EXPORT and for all those, the platform must be in INIT (for some in WORKING state) but for all in INIT state and "The platform remains be in the same state after completion." So the whole SHUTDOWN -> INIT wankery in-between is a pure waste of electrons. > I see that we can do a small optimization -- since we already know > the FW state hence we can avoid issuing PSP command when we know for > sure that command will fail because we are not in correct state. As I said before, you should do that regardless by recording the current state of the PSP in variable so that you can save yourself the status querying. > If command needs INIT state and FW is not in INIT state then its safe to > transition from UNINIT -> INIT. But if command needs UNINIT state and FW > is in INIT state then its not safe to transition -- in those case we > simply return EBUSY and let the user retry the command. Whatever - that doesn't contradict what I'm proposing.
On 10/27/17 7:00 PM, Borislav Petkov wrote: > On Fri, Oct 27, 2017 at 05:59:23PM -0500, Brijesh Singh wrote: >> Yes it is typo. PEK_GEN wants FW to be in INIT state hence someone need >> to transition from UNINIT -> INIT. > Which, once you've done it once on driver init, is there. > >> That's what I am doing except FACTORY_RESET. > Well, not really. Lemme pick a command at random... > > PEK_CSR. For that, you do INIT -> PEK_CSR -> SHUTDOWN. > > Doc says, platform needs to be in INIT or WORKING state. But nothing > says you should shut it down. Spec says, SHUTDOWN transitions platform > to UNINIT state. So when the next command comes in which needs the > platform to be in INIT state, you go and INIT it again. For no reason > *WHATSOEVER*! > > I know, you're gonna say, but what if the next command needs a different > state than INIT. Well, *then* you transition it, in the command > function. When that function executes. But not before that and not in > preparation that *maybe* the next command will be it. > > Now, if you did: > > INIT once during driver init > > PEK_CSR > > (platform remains in INIT state) > > <--- the next command here can execute directly if it is allowed in INIT > state. > > Instead, the platform has been shutdown and you init it again. Do you > see now what I mean? Yes, I can see that with your proposal we may able to save some PSP interaction because after command execution we do not restore to the previous state. e.g before executing the PEK_CSR command if FW was in UINIT then we do UNINIT -> INIT and leave it to INIT state. > IOW, once you init the PSP master, you should keep it in the INIT state > - or the state in which most commands expect it to be and thus save > yourself all that unnecessary toggling. If a command needs it to be in a > different state, only *then* you transition it. Let me implement it and send you the patch. I think the command execution function will look like this: static int sev_ioctl_do_pek_csr(...) { .... .... mutex(&fw_init_mutex); /* If FW is not in INIT state then initialize before executing command */ if (psp->sev_state != SEV_STATE_INIT) { rc = sev_platform_init(...); if (rc) { mutex_unlock(&fw_init_mutex); return rc; } } rc = sev_do_cmd(....) mutex_unlock(&fw_init_mutex); return rc; } and factory reset will look like this static int sev_ioctl_do_reset(...) { mutex(&fw_init_mutex); /* If FW is not in UINIT state then shutdown before executing command */ if (psp->sev_state != SEV_STATE_INIT) sev_platform_shutdown(...); rc = sev_do_cmd(....) mutex_unlock(&fw_init_mutex); return rc; } > Instead, what you have now is that you call INIT and SHUTDOWN > around SEV_PEK_GEN, SEV_PDH_GEN, SEV_PEK_CSR, SEV_PEK_CERT_IMPORT, > SEV_PDH_CERT_EXPORT and for all those, the platform must be in INIT > (for some in WORKING state) but for all in INIT state and "The platform > remains be in the same state after completion." So the whole SHUTDOWN -> > INIT wankery in-between is a pure waste of electrons. > >> I see that we can do a small optimization -- since we already know >> the FW state hence we can avoid issuing PSP command when we know for >> sure that command will fail because we are not in correct state. > As I said before, you should do that regardless by recording the current > state of the PSP in variable so that you can save yourself the status > querying. > >> If command needs INIT state and FW is not in INIT state then its safe to >> transition from UNINIT -> INIT. But if command needs UNINIT state and FW >> is in INIT state then its not safe to transition -- in those case we >> simply return EBUSY and let the user retry the command. > Whatever - that doesn't contradict what I'm proposing. >
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c index b5789f878560..e9966d5fc6c4 100644 --- a/drivers/crypto/ccp/psp-dev.c +++ b/drivers/crypto/ccp/psp-dev.c @@ -26,6 +26,14 @@ #include "sp-dev.h" #include "psp-dev.h" +#define DEVICE_NAME "sev" + +static DEFINE_MUTEX(sev_cmd_mutex); +static DEFINE_MUTEX(fw_init_mutex); + +static struct sev_misc_dev *sev_misc_dev; +static int fw_init_count; + static struct psp_device *psp_alloc_struct(struct sp_device *sp) { struct device *dev = sp->dev; @@ -45,9 +53,298 @@ static struct psp_device *psp_alloc_struct(struct sp_device *sp) static irqreturn_t psp_irq_handler(int irq, void *data) { + struct psp_device *psp = data; + unsigned int status; + int reg; + + /* Read the interrupt status: */ + status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS); + + /* Check if it is command completion: */ + if (!(status & BIT(PSP_CMD_COMPLETE_REG))) + goto done; + + /* Check if it is SEV command completion: */ + reg = ioread32(psp->io_regs + PSP_CMDRESP); + if (reg & PSP_CMDRESP_RESP) { + psp->sev_int_rcvd = 1; + wake_up(&psp->sev_int_queue); + } + +done: + /* Clear the interrupt status by writing the same value we read. */ + iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS); + return IRQ_HANDLED; } +static void sev_wait_cmd_ioc(struct psp_device *psp, unsigned int *reg) +{ + psp->sev_int_rcvd = 0; + + wait_event(psp->sev_int_queue, psp->sev_int_rcvd); + *reg = ioread32(psp->io_regs + PSP_CMDRESP); +} + +static int sev_cmd_buffer_len(int cmd) +{ + switch (cmd) { + case SEV_CMD_INIT: return sizeof(struct sev_data_init); + case SEV_CMD_PLATFORM_STATUS: return sizeof(struct sev_user_data_status); + case SEV_CMD_PEK_CSR: return sizeof(struct sev_data_pek_csr); + case SEV_CMD_PEK_CERT_IMPORT: return sizeof(struct sev_data_pek_cert_import); + case SEV_CMD_PDH_CERT_EXPORT: return sizeof(struct sev_data_pdh_cert_export); + case SEV_CMD_LAUNCH_START: return sizeof(struct sev_data_launch_start); + case SEV_CMD_LAUNCH_UPDATE_DATA: return sizeof(struct sev_data_launch_update_data); + case SEV_CMD_LAUNCH_UPDATE_VMSA: return sizeof(struct sev_data_launch_update_vmsa); + case SEV_CMD_LAUNCH_FINISH: return sizeof(struct sev_data_launch_finish); + case SEV_CMD_LAUNCH_MEASURE: return sizeof(struct sev_data_launch_measure); + case SEV_CMD_ACTIVATE: return sizeof(struct sev_data_activate); + case SEV_CMD_DEACTIVATE: return sizeof(struct sev_data_deactivate); + case SEV_CMD_DECOMMISSION: return sizeof(struct sev_data_decommission); + case SEV_CMD_GUEST_STATUS: return sizeof(struct sev_data_guest_status); + case SEV_CMD_DBG_DECRYPT: return sizeof(struct sev_data_dbg); + case SEV_CMD_DBG_ENCRYPT: return sizeof(struct sev_data_dbg); + case SEV_CMD_SEND_START: return sizeof(struct sev_data_send_start); + case SEV_CMD_SEND_UPDATE_DATA: return sizeof(struct sev_data_send_update_data); + case SEV_CMD_SEND_UPDATE_VMSA: return sizeof(struct sev_data_send_update_vmsa); + case SEV_CMD_SEND_FINISH: return sizeof(struct sev_data_send_finish); + case SEV_CMD_RECEIVE_START: return sizeof(struct sev_data_receive_start); + case SEV_CMD_RECEIVE_FINISH: return sizeof(struct sev_data_receive_finish); + case SEV_CMD_RECEIVE_UPDATE_DATA: return sizeof(struct sev_data_receive_update_data); + case SEV_CMD_RECEIVE_UPDATE_VMSA: return sizeof(struct sev_data_receive_update_vmsa); + case SEV_CMD_LAUNCH_UPDATE_SECRET: return sizeof(struct sev_data_launch_secret); + default: return 0; + } + + return 0; +} + +static int sev_do_cmd(int cmd, void *data, int *psp_ret) +{ + unsigned int phys_lsb, phys_msb; + unsigned int reg, ret = 0; + struct psp_device *psp; + struct sp_device *sp; + + sp = sp_get_psp_master_device(); + if (!sp) + return -ENODEV; + + psp = sp->psp_data; + if (!psp) + return -ENODEV; + + /* Get the physical address of the command buffer */ + phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0; + phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0; + + dev_dbg(psp->dev, "sev command id %#x buffer 0x%08x%08x\n", + cmd, phys_msb, phys_lsb); + + print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data, + sev_cmd_buffer_len(cmd), false); + + /* Only one command at a time... */ + mutex_lock(&sev_cmd_mutex); + + iowrite32(phys_lsb, psp->io_regs + PSP_CMDBUFF_ADDR_LO); + iowrite32(phys_msb, psp->io_regs + PSP_CMDBUFF_ADDR_HI); + + reg = cmd; + reg <<= PSP_CMDRESP_CMD_SHIFT; + reg |= PSP_CMDRESP_IOC; + iowrite32(reg, psp->io_regs + PSP_CMDRESP); + + /* wait for command completion */ + sev_wait_cmd_ioc(psp, ®); + + if (psp_ret) + *psp_ret = reg & PSP_CMDRESP_ERR_MASK; + + if (reg & PSP_CMDRESP_ERR_MASK) { + dev_dbg(psp->dev, "sev command %#x failed (%#010x)\n", + cmd, reg & PSP_CMDRESP_ERR_MASK); + ret = -EIO; + } + + mutex_unlock(&sev_cmd_mutex); + print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data, + sev_cmd_buffer_len(cmd), false); + return ret; +} + +static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) +{ + return -ENOTTY; +} + +static const struct file_operations sev_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = sev_ioctl, +}; + +static int __sev_platform_init(struct sev_data_init *data, int *error) +{ + int rc = 0; + + mutex_lock(&fw_init_mutex); + + if (!fw_init_count) { + rc = sev_do_cmd(SEV_CMD_INIT, data, error); + if (rc) + goto unlock; + } + + fw_init_count++; + +unlock: + mutex_unlock(&fw_init_mutex); + return rc; + +} + +int sev_platform_init(struct sev_data_init *data, int *error) +{ + struct sev_data_init *input = NULL; + int rc; + + if (!data) { + input = kzalloc(sizeof(*input), GFP_KERNEL); + if (!input) + return -ENOMEM; + + data = input; + } + + rc = __sev_platform_init(data, error); + + kfree(input); + return rc; +} +EXPORT_SYMBOL_GPL(sev_platform_init); + +int sev_platform_shutdown(int *error) +{ + int rc = 0; + + mutex_lock(&fw_init_mutex); + + if (!fw_init_count) + goto unlock; + + if (fw_init_count == 1) { + rc = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error); + if (rc) + goto unlock; + } + + fw_init_count--; + +unlock: + mutex_unlock(&fw_init_mutex); + return rc; +} +EXPORT_SYMBOL_GPL(sev_platform_shutdown); + +int sev_platform_status(struct sev_user_data_status *data, int *error) +{ + return sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error); +} +EXPORT_SYMBOL_GPL(sev_platform_status); + +int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, + void *data, int *error) +{ + if (!filep || filep->f_op != &sev_fops) + return -EBADF; + + return sev_do_cmd(cmd, data, error); +} +EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user); + +int sev_guest_deactivate(struct sev_data_deactivate *data, int *error) +{ + return sev_do_cmd(SEV_CMD_DEACTIVATE, data, error); +} +EXPORT_SYMBOL_GPL(sev_guest_deactivate); + +int sev_guest_activate(struct sev_data_activate *data, int *error) +{ + return sev_do_cmd(SEV_CMD_ACTIVATE, data, error); +} +EXPORT_SYMBOL_GPL(sev_guest_activate); + +int sev_guest_decommission(struct sev_data_decommission *data, int *error) +{ + return sev_do_cmd(SEV_CMD_DECOMMISSION, data, error); +} +EXPORT_SYMBOL_GPL(sev_guest_decommission); + +int sev_guest_df_flush(int *error) +{ + return sev_do_cmd(SEV_CMD_DF_FLUSH, 0, error); +} +EXPORT_SYMBOL_GPL(sev_guest_df_flush); + +static int sev_ops_init(struct psp_device *psp) +{ + struct device *dev = psp->dev; + int ret; + + /* + * SEV feature support can be detected on multiple devices but the SEV + * FW commands must be issued on the master. During probe, we do not + * know the master hence we create /dev/sev on the first device probe. + * sev_do_cmd() finds the right master device to which to issue the + * command to the firmware. + */ + if (!sev_misc_dev) { + struct miscdevice *misc; + + sev_misc_dev = devm_kzalloc(dev, sizeof(*sev_misc_dev), GFP_KERNEL); + if (!sev_misc_dev) + return -ENOMEM; + + misc = &sev_misc_dev->misc; + misc->minor = MISC_DYNAMIC_MINOR; + misc->name = DEVICE_NAME; + misc->fops = &sev_fops; + + ret = misc_register(misc); + if (ret) + return ret; + + kref_init(&sev_misc_dev->refcount); + } else { + kref_get(&sev_misc_dev->refcount); + } + + init_waitqueue_head(&psp->sev_int_queue); + psp->sev_misc = sev_misc_dev; + dev_info(dev, "registered SEV device\n"); + + return 0; +} + +static int sev_init(struct psp_device *psp) +{ + /* Check if device supports SEV feature */ + if (!(ioread32(psp->io_regs + PSP_FEATURE_REG) & 1)) { + dev_dbg(psp->dev, "device does not support SEV\n"); + return 1; + } + + return sev_ops_init(psp); +} + +static void sev_exit(struct kref *ref) +{ + struct sev_misc_dev *sev_dev = container_of(ref, struct sev_misc_dev, refcount); + + misc_deregister(&sev_dev->misc); +} + int psp_dev_init(struct sp_device *sp) { struct device *dev = sp->dev; @@ -84,11 +381,17 @@ int psp_dev_init(struct sp_device *sp) if (sp->set_psp_master_device) sp->set_psp_master_device(sp); + ret = sev_init(psp); + if (ret) + goto e_irq; + /* Enable interrupt */ iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTEN); return 0; +e_irq: + sp_free_psp_irq(psp->sp, psp); e_err: sp->psp_data = NULL; @@ -101,5 +404,8 @@ void psp_dev_destroy(struct sp_device *sp) { struct psp_device *psp = sp->psp_data; + if (psp->sev_misc) + kref_put(&sev_misc_dev->refcount, sev_exit); + sp_free_psp_irq(sp, psp); } diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h index 55b7808367c3..13467c484f6d 100644 --- a/drivers/crypto/ccp/psp-dev.h +++ b/drivers/crypto/ccp/psp-dev.h @@ -25,9 +25,21 @@ #include <linux/interrupt.h> #include <linux/irqreturn.h> #include <linux/dmaengine.h> +#include <linux/psp-sev.h> +#include <linux/miscdevice.h> #include "sp-dev.h" +#define PSP_C2PMSG(_num) ((_num) << 2) +#define PSP_CMDRESP PSP_C2PMSG(32) +#define PSP_CMDBUFF_ADDR_LO PSP_C2PMSG(56) +#define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57) +#define PSP_FEATURE_REG PSP_C2PMSG(63) + +#define PSP_P2CMSG(_num) ((_num) << 2) +#define PSP_CMD_COMPLETE_REG 1 +#define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG) + #define PSP_P2CMSG_INTEN 0x0110 #define PSP_P2CMSG_INTSTS 0x0114 @@ -44,6 +56,11 @@ #define MAX_PSP_NAME_LEN 16 +struct sev_misc_dev { + struct kref refcount; + struct miscdevice misc; +}; + struct psp_device { struct list_head entry; @@ -54,6 +71,10 @@ struct psp_device { struct sp_device *sp; void __iomem *io_regs; + + unsigned int sev_int_rcvd; + wait_queue_head_t sev_int_queue; + struct sev_misc_dev *sev_misc; }; #endif /* __PSP_DEV_H */ diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 15bda519538e..21511419bfe6 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -491,4 +491,163 @@ struct sev_data_dbg { u32 len; /* In */ } __packed; +#ifdef CONFIG_CRYPTO_DEV_SP_PSP + +/** + * sev_platform_init - perform SEV INIT command + * + * @init: sev_data_init structure to be processed + * @error: SEV command return code + * + * Returns: + * 0 if the SEV successfully processed the command + * -%ENODEV if the SEV device is not available + * -%ENOTSUPP if the SEV does not support SEV + * -%ETIMEDOUT if the SEV command timed out + * -%EIO if the SEV returned a non-zero return code + */ +int sev_platform_init(struct sev_data_init *init, int *error); + +/** + * sev_platform_shutdown - perform SEV SHUTDOWN command + * + * @error: SEV command return code + * + * Returns: + * 0 if the SEV successfully processed the command + * -%ENODEV if the SEV device is not available + * -%ENOTSUPP if the SEV does not support SEV + * -%ETIMEDOUT if the SEV command timed out + * -%EIO if the SEV returned a non-zero return code + */ +int sev_platform_shutdown(int *error); + +/** + * sev_platform_status - perform SEV PLATFORM_STATUS command + * + * @init: sev_data_status structure to be processed + * @error: SEV command return code + * + * Returns: + * 0 if the SEV successfully processed the command + * -%ENODEV if the SEV device is not available + * -%ENOTSUPP if the SEV does not support SEV + * -%ETIMEDOUT if the SEV command timed out + * -%EIO if the SEV returned a non-zero return code + */ +int sev_platform_status(struct sev_user_data_status *status, int *error); + +/** + * sev_issue_cmd_external_user - issue SEV command by other driver with a file + * handle. + * + * This function can be used by other drivers to issue a SEV command on + * behalf of userspace. The caller must pass a valid SEV file descriptor + * so that we know that it has access to SEV device. + * + * @filep - SEV device file pointer + * @cmd - command to issue + * @data - command buffer + * @error: SEV command return code + * + * Returns: + * 0 if the SEV successfully processed the command + * -%ENODEV if the SEV device is not available + * -%ENOTSUPP if the SEV does not support SEV + * -%ETIMEDOUT if the SEV command timed out + * -%EIO if the SEV returned a non-zero return code + * -%EINVAL if the SEV file descriptor is not valid + */ +int sev_issue_cmd_external_user(struct file *filep, unsigned int id, + void *data, int *error); + +/** + * sev_guest_deactivate - perform SEV DEACTIVATE command + * + * @deactivate: sev_data_deactivate structure to be processed + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int sev_guest_deactivate(struct sev_data_deactivate *data, int *error); + +/** + * sev_guest_activate - perform SEV ACTIVATE command + * + * @activate: sev_data_activate structure to be processed + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int sev_guest_activate(struct sev_data_activate *data, int *error); + +/** + * sev_guest_df_flush - perform SEV DF_FLUSH command + * + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int sev_guest_df_flush(int *error); + +/** + * sev_guest_decommission - perform SEV DECOMMISSION command + * + * @decommission: sev_data_decommission structure to be processed + * @sev_ret: sev command return code + * + * Returns: + * 0 if the sev successfully processed the command + * -%ENODEV if the sev device is not available + * -%ENOTSUPP if the sev does not support SEV + * -%ETIMEDOUT if the sev command timed out + * -%EIO if the sev returned a non-zero return code + */ +int sev_guest_decommission(struct sev_data_decommission *data, int *error); + +#else /* !CONFIG_CRYPTO_DEV_SP_PSP */ + +static inline int +sev_platform_status(struct sev_user_data_status *status, int *error) { return -ENODEV; } + +static inline int +sev_platform_init(struct sev_data_init *init, int *error) { return -ENODEV; } + +static inline int sev_platform_shutdown(int *error) { return -ENODEV; } + +static inline int +sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; } + +static inline int +sev_guest_decommission(struct sev_data_decommission *data, int *error) { return -ENODEV; } + +static inline int +sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV; } + +static inline int sev_guest_df_flush(int *error) { return -ENODEV; } + +static inline int +sev_issue_cmd_external_user(struct file *filep, + unsigned int id, void *data, int *error) +{ + return -ENODEV; +} + +#endif /* CONFIG_CRYPTO_DEV_SP_PSP */ + #endif /* __PSP_SEV_H__ */
AMD's new Secure Encrypted Virtualization (SEV) feature allows the memory contents of virtual machines to be transparently encrypted with a key unique to the VM. The programming and management of the encryption keys are handled by the AMD Secure Processor (AMD-SP) which exposes the commands for these tasks. The complete spec is available at: http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf Extend the AMD-SP driver to provide the following support: - an in-kernel API to communicate with the SEV firmware. The API can be used by the hypervisor to create encryption context for a SEV guest. - a userspace IOCTL to manage the platform certificates. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Radim Krčmář" <rkrcmar@redhat.com> Cc: Borislav Petkov <bp@suse.de> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Gary Hook <gary.hook@amd.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: linux-crypto@vger.kernel.org Cc: kvm@vger.kernel.org Cc: linux-kernel@vger.kernel.org Improvements-by: Borislav Petkov <bp@suse.de> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- drivers/crypto/ccp/psp-dev.c | 306 +++++++++++++++++++++++++++++++++++++++++++ drivers/crypto/ccp/psp-dev.h | 21 +++ include/linux/psp-sev.h | 159 ++++++++++++++++++++++ 3 files changed, 486 insertions(+)