On 09/07/2017 05:19 PM, Brijesh Singh wrote:
Hi Boris,
On 09/07/2017 09:27 AM, Borislav Petkov wrote:
...
The commit message above reads better to me as the help text than what
you have here.
Also, in order to make it easier for the user, I think we'll need a
CONFIG_AMD_MEM_ENCRYPT_SEV or so and make that depend on CONFIG_KVM_AMD,
this above and all the other pieces that are needed. Just so that when
the user builds such a kernel, all is enabled and not her having to go
look for what else is needed.
And then put the sev code behind that config option. Depending on how
ugly it gets...
I will add more detail in the help text. I will look into adding some
depends.
...
+
+void psp_add_device(struct psp_device *psp)
That function is needlessly global and should be static, AFAICT.
Better yet, it is called only once and its body is trivial so you can
completely get rid of it and meld it into the callsite.
Agreed, will do.
.....
+
+static struct psp_device *psp_alloc_struct(struct sp_device *sp)
"psp_alloc()" is enough I guess.
I was trying to adhere to the existing ccp-dev.c function naming
conversion.
I would prefer that we not shorten this. The prior incarnation,
ccp_alloc_struct(), has/had been around for a while. And there are a
number of
similarly named allocation functions in the driver that we like to keep
sorted.
If anything, it should be more explanatory, IMO.
....
static.
Please audit all your functions in the psp pile and make them static if
not needed outside of their compilation unit.
Will do.
+{
+ unsigned int status;
+ irqreturn_t ret = IRQ_HANDLED;
+ struct psp_device *psp = data;
Please sort function local variables declaration in a reverse christmas
tree order:
<type> longest_variable_name;
<type> shorter_var_name;
<type> even_shorter;
<type> i;
Got it, will do
+
+ /* read the interrupt status */
+ status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS);
+
+ /* invoke subdevice interrupt handlers */
+ if (status) {
+ if (psp->sev_irq_handler)
+ ret = psp->sev_irq_handler(irq, psp->sev_irq_data);
+ if (psp->tee_irq_handler)
+ ret = psp->tee_irq_handler(irq, psp->tee_irq_data);
+ }
+
+ /* clear the interrupt status */
+ iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS);
We're clearing the status by writing the same value back?!? Shouldn't
that be:
iowrite32(0, psp->io_regs + PSP_P2CMSG_INTSTS);
Actually the SW should write "1" to clear the bit. To make it clear, I
can use value 1 and add comment.
Below I see
iowrite32(0xffffffff, psp->io_regs + PSP_P2CMSG_INTSTS);
which is supposed to clear IRQs. Btw, you can write that:
iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTSTS);
Sure, I will do that
...
...
+
+ sp_set_psp_master(sp);
So this function is called only once and declared somewhere else. You
could simply do here:
if (sp->set_psp_master_device)
sp->set_psp_master_device(sp);
and get rid of one more global function.
Sure I can do that.
....
+ /* Enable interrupt */
+ dev_dbg(dev, "Enabling interrupts ...\n");
+ iowrite32(7, psp->io_regs + PSP_P2CMSG_INTEN);
Uh, a magic "7"! Exciting!
I wonder what that means and whether it could be a define with an
explanatory name instead. Ditto for the other values...
I will try to define some macro instead of hard coded values.
....
+
+int psp_dev_resume(struct sp_device *sp)
+{
+ return 0;
+}
+
+int psp_dev_suspend(struct sp_device *sp, pm_message_t state)
+{
+ return 0;
+}
Those last two are completely useless. Delete them pls.
We don't have any PM support, I agree will delete it.
...
+int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler,
+ void *data)
+{
+ psp->sev_irq_data = data;
+ psp->sev_irq_handler = handler;
+
+ return 0;
+}
+
+int psp_free_sev_irq(struct psp_device *psp, void *data)
+{
+ if (psp->sev_irq_handler) {
+ psp->sev_irq_data = NULL;
+ psp->sev_irq_handler = NULL;
+ }
+
+ return 0;
+}
Both void. Please do not return values from functions which are simply
void functions by design.
thanks, will fix it.
...
+int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler,
+ void *data);
+int psp_free_sev_irq(struct psp_device *psp, void *data);
+
+int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler,
+ void *data);
Let them stick out.
okay
...
+int psp_free_tee_irq(struct psp_device *psp, void *data);
+
+struct psp_device *psp_get_master_device(void);
+
+extern const struct psp_vdata psp_entry;
+
+#endif /* __PSP_DEV_H */
diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
So this file is called sp-dev and the other psp-dev. Confusing.
And in general, why isn't the whole thing a single psp-dev and you can
save yourself all the registering blabla and have a single driver for
the whole PSP functionality?
Distros will have to enable everything anyway and the whole CCP/PSP code
is only a couple of KBs so you can just as well put it all into a single
driver. Hm.
PSP provides the interface for communicating with SEV and TEE FWs. I choose
to add generic PSP interface first then plug the SEV FW support. The TEE
commands may be totally different from SEV FW commands hence I tried to put
all the SEV specific changes into one place and adhere to current ccp file
naming convention.
At high level, AMD-SP (AMD Secure Processor) (i.e CCP driver) will
provide the
support for CCP, SEV and TEE FW commands.
+--- CCP
|
AMD-SP --|
| +--- SEV
| |
+---- PSP ---*
|
+---- TEE
-Brijesh
