Re: [Part2 PATCH v5 12/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Brijesh Singh]

On 10/6/17 1:49 PM, Borislav Petkov wrote:
...
>> +
>> +static unsigned int sev_poll;
>> +module_param(sev_poll, uint, 0444);
>> +MODULE_PARM_DESC(sev_poll, "Poll for sev command completion - any non-zero value");
> What is that used for? Some debugging leftover probably? If not, add a
> comment for why it is useful.
>

Yes, it was used for debug and can be removed. I will it in next rev.

.....

> Ok, this patch is humongous. Please split it in at least 3 or 4 separate
> logical patches as it is very hard to review as one huge chunk right
> now. Like, for example, the header file additions could be one patch,
> then part of psp-dev.c and so on.
>
> You can then send me those split-up ones as a reply here so that I can
> take a look again.

I will see what I can do, Maybe I will add one command at a time like we
do in svm.c.

> Also, here are a bunch of fixes ontop. Please add
>
> "Improvements by Borislav Petkov <bp@xxxxxxx>"
>
> to the commit message when you decide to use them.

Sure, I will add your Improved-by-tag.

> Btw, the psp_entry thing I've moved to sp-pci.c, you probably should do
> that in the patch which adds it, not here.
>
> Also, I've fixed:
>
> +       /* Clear the interrupt status by writing 1. */
> +       iowrite32(1, psp->io_regs + PSP_P2CMSG_INTSTS);
>
> to really write 1 and not reuse the old status value.

Actually this one is interesting. When we read the status register, one
of the bit in status register will be 1 (i.e it may not always be bit 0)
and we need to write 1 on same bit position to clear it -- basically
write the same value you read. I will improve the comment.Â

> And so on, they should be obvious from the diff.
>
> Thx.
>
> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 1b87a699bd3f..13633eaa7889 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -32,15 +32,11 @@
>  
>  static unsigned int sev_poll;
>  module_param(sev_poll, uint, 0444);
> -MODULE_PARM_DESC(sev_poll, "Poll for sev command completion - any non-zero value");
> +MODULE_PARM_DESC(sev_poll, "Poll for SEV command completion - any non-zero value");
>  
> -DEFINE_MUTEX(sev_cmd_mutex);
> +static DEFINE_MUTEX(sev_cmd_mutex);
>  static bool sev_fops_registered;
>  
> -const struct psp_vdata psp_entry = {
> -	.offset = 0x10500,
> -};
> -
>  static struct psp_device *psp_alloc_struct(struct sp_device *sp)
>  {
>  	struct device *dev = sp->dev;
> @@ -62,34 +58,37 @@ static irqreturn_t psp_irq_handler(int irq, void *data)
>  {
>  	struct psp_device *psp = data;
>  	unsigned int status;
> +	int reg;
>  
> -	/* read the interrupt status */
> +	/* Read the interrupt status: */
>  	status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS);
>  
> -	/* check if its command completion */
> -	if (status & (1 << PSP_CMD_COMPLETE_REG)) {
> -		int reg;
> +	/* Check if it is command completion: */
> +	if (!(status & BIT(PSP_CMD_COMPLETE_REG)))
> +		goto done;
>  
> -		/* check if its 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);
> -		}
> +	/* 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);
>  	}
>  
> -	/* clear the interrupt status by writing 1 */
> -	iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS);
> +done:
> +	/* Clear the interrupt status by writing 1. */
> +	iowrite32(1, psp->io_regs + PSP_P2CMSG_INTSTS);
>  
>  	return IRQ_HANDLED;
>  }
>  
> -static int sev_wait_cmd_poll(struct psp_device *psp, unsigned int timeout,
> +static int sev_wait_cmd_poll(struct psp_device *psp, unsigned int timeouts,
>  			     unsigned int *reg)
>  {
> -	int wait = timeout * 10;	/* 100ms sleep => timeout * 10 */
> +	/* 10*100ms max timeout */
> +	if (timeouts > 10)
> +		timeouts = 10;
>  
> -	while (--wait) {
> +	while (--timeouts) {
>  		msleep(100);
>  
>  		*reg = ioread32(psp->io_regs + PSP_CMDRESP);
> @@ -97,8 +96,8 @@ static int sev_wait_cmd_poll(struct psp_device *psp, unsigned int timeout,
>  			break;
>  	}
>  
> -	if (!wait) {
> -		dev_err(psp->dev, "sev command timed out\n");
> +	if (!timeouts) {
> +		dev_err(psp->dev, "SEV command timed out\n");
>  		return -ETIMEDOUT;
>  	}
>  
> @@ -157,11 +156,16 @@ static int sev_cmd_buffer_len(int cmd)
>  
>  static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
>  {
> -	struct sp_device *sp = sp_get_psp_master_device();
>  	unsigned int phys_lsb, phys_msb;
> -	struct psp_device *psp = sp->psp_data;
> +	struct psp_device *psp;
>  	unsigned int reg, ret;
> +	struct sp_device *sp;
> +
> +	sp = sp_get_psp_master_device();
> +	if (!sp)
> +		return -ENODEV;
>  
> +	psp = sp->psp_data;
>  	if (!psp)
>  		return -ENODEV;
>  
> @@ -170,9 +174,10 @@ static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
>  	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);
> +		cmd, phys_msb, phys_lsb);
> +
>  	print_hex_dump_debug("(in):  ", DUMP_PREFIX_OFFSET, 16, 2, data,
> -			sev_cmd_buffer_len(cmd), false);
> +			     sev_cmd_buffer_len(cmd), false);
>  
>  	/* Only one command at a time... */
>  	mutex_lock(&sev_cmd_mutex);
> @@ -201,7 +206,7 @@ static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
>  unlock:
>  	mutex_unlock(&sev_cmd_mutex);
>  	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> -			sev_cmd_buffer_len(cmd), false);
> +			     sev_cmd_buffer_len(cmd), false);
>  	return ret;
>  }
>  
> diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
> index 51d3cd966eed..6e8f83b41521 100644
> --- a/drivers/crypto/ccp/psp-dev.h
> +++ b/drivers/crypto/ccp/psp-dev.h
> @@ -73,6 +73,4 @@ struct psp_device {
>  	struct miscdevice sev_misc;
>  };
>  
> -extern const struct psp_vdata psp_entry;
> -
>  #endif /* __PSP_DEV_H */
> diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
> index 20a0f3543cf4..f5f43c50698a 100644
> --- a/drivers/crypto/ccp/sp-pci.c
> +++ b/drivers/crypto/ccp/sp-pci.c
> @@ -268,6 +268,12 @@ static int sp_pci_resume(struct pci_dev *pdev)
>  }
>  #endif
>  
> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> +static const struct psp_vdata psp_entry = {
> +	.offset = 0x10500,
> +};
> +#endif
> +
>  static const struct sp_dev_vdata dev_vdata[] = {
>  	{
>  		.bar = 2,
>