Re: [PATCH v1 2/2] Implemented Amd Secure Processor device driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.04.2024 17:36, Andrei Semenov wrote:
> Signed-off-by: Andrei Semenov <andrei.semenov@xxxxxxxx>

Again a few nits on top of Andrew's remarks:

> --- /dev/null
> +++ b/xen/arch/x86/include/asm/psp-sev.h
> @@ -0,0 +1,655 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * AMD Secure Encrypted Virtualization (SEV) driver interface
> + *
> + * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
> + *
> + * Author: Brijesh Singh <brijesh.singh@xxxxxxx>
> + *
> + * SEV API spec is available at https://developer.amd.com/sev
> + */
> +
> +#ifndef __PSP_SEV_H__
> +#define __PSP_SEV_H__
> +
> +#include <xen/types.h>
> +
> +/**
> + * SEV platform and guest management commands
> + */
> +enum sev_cmd {
> +     /* platform commands */
> +     SEV_CMD_INIT                    = 0x001,

>From the looks of it (style) this file may be coming from Linux. Any file
take from somewhere wants to have its origin specified precisely, to aid
in future updating. (Once again, an empty description is not acceptable
here anyway.)

> --- /dev/null
> +++ b/xen/drivers/crypto/Kconfig
> @@ -0,0 +1,10 @@
> +config AMD_SP
> +        bool "AMD Secure Processor" if EXPERT
> +        depends on X86
> +        default n

No need for this line.

> --- /dev/null
> +++ b/xen/drivers/crypto/asp.c
> @@ -0,0 +1,808 @@
> +#include <xen/init.h>
> +#include <xen/pci.h>
> +#include <xen/list.h>
> +#include <xen/tasklet.h>
> +#include <xen/pci_ids.h>
> +#include <xen/delay.h>
> +#include <xen/timer.h>
> +#include <xen/wait.h>
> +#include <xen/smp.h>
> +#include <asm/msi.h>
> +#include <asm/system.h>
> +#include <asm/psp-sev.h>
> +
> +/*
> +TODO:
> +-  GLOBAL:
> +     - add command line params for tunables
> + - INTERRUPT MODE:
> +    - CET shadow stack: adapt #CP handler???
> +    - Serialization: must be done by the client? adapt spinlock?
> + */
> +
> +#define PSP_CAPABILITY_SEV                      (1 << 0)
> +#define PSP_CAPABILITY_TEE                      (1 << 1)
> +#define PSP_CAPABILITY_PSP_SECURITY_REPORTING   (1 << 7)
> +#define PSP_CAPABILITY_PSP_SECURITY_OFFSET      8
> +
> +#define PSP_INTSTS_CMD_COMPLETE       (1 << 1)
> +
> +#define SEV_CMDRESP_CMD_MASK          0x7ff0000
> +#define SEV_CMDRESP_CMD_SHIFT         16
> +#define SEV_CMDRESP_CMD(cmd)          ((cmd) << SEV_CMDRESP_CMD_SHIFT)
> +#define SEV_CMDRESP_STS_MASK          0xffff
> +#define SEV_CMDRESP_STS(x)            ((x) & SEV_CMDRESP_STS_MASK)
> +#define SEV_CMDRESP_RESP              (1 << 31)

(1u << 31)

> +#define SEV_CMDRESP_IOC               (1 << 0)
> +
> +#define ASP_CMD_BUFF_SIZE    0x1000
> +#define SEV_FW_BLOB_MAX_SIZE 0x4000
> +
> +/*
> + * SEV platform state
> + */
> +enum sev_state {
> +        SEV_STATE_UNINIT                = 0x0,
> +        SEV_STATE_INIT                  = 0x1,
> +        SEV_STATE_WORKING               = 0x2,
> +        SEV_STATE_MAX

Too deep indentation here.

> +};
> +
> +struct sev_vdata {
> +    const unsigned int cmdresp_reg;
> +    const unsigned int cmdbuff_addr_lo_reg;
> +    const unsigned int cmdbuff_addr_hi_reg;
> +};
> +
> +struct psp_vdata {
> +    const unsigned short   base_offset;
> +    const struct sev_vdata *sev;
> +    const unsigned int feature_reg;
> +    const unsigned int inten_reg;
> +    const unsigned int intsts_reg;
> +    const char* name;
> +};
> +
> +static struct sev_vdata sevv1 = {
> +    .cmdresp_reg         = 0x10580,     /* C2PMSG_32 */
> +    .cmdbuff_addr_lo_reg = 0x105e0,     /* C2PMSG_56 */
> +    .cmdbuff_addr_hi_reg = 0x105e4,     /* C2PMSG_57 */
> +};
> +
> +static struct sev_vdata sevv2 = {
> +    .cmdresp_reg         = 0x10980,     /* C2PMSG_32 */
> +    .cmdbuff_addr_lo_reg = 0x109e0,     /* C2PMSG_56 */
> +    .cmdbuff_addr_hi_reg = 0x109e4,     /* C2PMSG_57 */
> +};
> +
> +static struct psp_vdata pspv1 = {
> +    .base_offset = PCI_BASE_ADDRESS_2,
> +    .sev         = &sevv1,
> +    .feature_reg = 0x105fc,     /* C2PMSG_63 */
> +    .inten_reg   = 0x10610,     /* P2CMSG_INTEN */
> +    .intsts_reg  = 0x10614,     /* P2CMSG_INTSTS */
> +    .name = "pspv1",
> +};
> +
> +static struct psp_vdata pspv2 = {
> +    .base_offset = PCI_BASE_ADDRESS_2,
> +    .sev         = &sevv2,
> +    .feature_reg = 0x109fc,     /* C2PMSG_63 */
> +    .inten_reg   = 0x10690,     /* P2CMSG_INTEN */
> +    .intsts_reg  = 0x10694,     /* P2CMSG_INTSTS */
> +    .name = "pspv2",
> +};
> +
> +static struct psp_vdata pspv4 = {
> +    .base_offset = PCI_BASE_ADDRESS_2,
> +    .sev         = &sevv2,
> +    .feature_reg = 0x109fc,     /* C2PMSG_63 */
> +    .inten_reg   = 0x10690,     /* P2CMSG_INTEN */
> +    .intsts_reg  = 0x10694,     /* P2CMSG_INTSTS */
> +    .name = "pspv4",
> +};
> +
> +static struct psp_vdata pspv6 = {
> +    .base_offset =  PCI_BASE_ADDRESS_2,
> +    .sev         = &sevv2,
> +    .feature_reg = 0x109fc,     /* C2PMSG_63 */
> +    .inten_reg   = 0x10510,     /* P2CMSG_INTEN */
> +    .intsts_reg  = 0x10514,     /* P2CMSG_INTSTS */
> +    .name = "pspv6",
> +};
> +
> +struct amd_sp_dev
> +{
> +    struct list_head list;
> +    struct pci_dev   *pdev;
> +    struct  psp_vdata *vdata;
> +    void    *io_base;
> +    paddr_t io_pbase;
> +    size_t  io_size;
> +    int     irq;
> +    int     state;
> +    void* cmd_buff;
> +    uint32_t cbuff_pa_low;
> +    uint32_t cbuff_pa_high;
> +    unsigned int capability;
> +    uint8_t api_major;
> +    uint8_t api_minor;
> +    uint8_t build;
> +    int     intr_rcvd;
> +    int     cmd_timeout;
> +    struct timer cmd_timer;
> +    struct waitqueue_head cmd_in_progress;
> +};
> +
> +LIST_HEAD(amd_sp_units);
> +#define for_each_sp_unit(sp) \
> +    list_for_each_entry(sp, &amd_sp_units, list)
> +
> +static spinlock_t _sp_cmd_lock = SPIN_LOCK_UNLOCKED;
> +
> +static struct amd_sp_dev *amd_sp_master;
> +
> +static void do_sp_irq(void *data);
> +static DECLARE_SOFTIRQ_TASKLET(sp_irq_tasklet, do_sp_irq, NULL);
> +
> +static bool force_sync = false;
> +static unsigned int asp_timeout_val = 30000;
> +static unsigned long long asp_sync_delay = 100ULL;
> +static int asp_sync_tries = 10;
> +
> +static void sp_cmd_lock(void)
> +{
> +    spin_lock(&_sp_cmd_lock);
> +}
> +
> +static void sp_cmd_unlock(void)
> +{
> +    spin_unlock(&_sp_cmd_lock);
> +}
> +
> +static int sev_cmd_buffer_len(int cmd)
> +{
> +    switch (cmd) {
> +        case SEV_CMD_INIT:                      return sizeof(struct 
> sev_data_init);

No mix of styles please: Either you retain Linux style (assuming the
file again comes from there), or you fully switch to Xen style.
There are other style issues also further down; please go through
and check everything again.

> +        case SEV_CMD_INIT_EX:                   return sizeof(struct 
> sev_data_init_ex);
> +        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);
> +        case SEV_CMD_DOWNLOAD_FIRMWARE:         return sizeof(struct 
> sev_data_download_firmware);
> +        case SEV_CMD_GET_ID:                    return sizeof(struct 
> sev_data_get_id);
> +        case SEV_CMD_ATTESTATION_REPORT:        return sizeof(struct 
> sev_data_attestation_report);
> +        case SEV_CMD_SEND_CANCEL:               return sizeof(struct 
> sev_data_send_cancel);
> +        default:                                return 0;
> +    }
> +
> +    return 0;
> +}
> +
> +static void invalidate_cache(void *unused)
> +{
> +    wbinvd();
> +}
> +
> +int _sev_do_cmd(struct amd_sp_dev *sp, int cmd, void *data, int *psp_ret)
> +{
> +    unsigned int cbuff_pa_low, cbuff_pa_high, cmd_val;
> +    int buf_len, cmdresp, rc;
> +
> +
> +    buf_len = sev_cmd_buffer_len(cmd);
> +
> +
> +    if ( data )

No double blank lines please, anywhere.

> +static void do_sp_cmd_timer(void *data)
> +{
> +    struct amd_sp_dev *sp = (struct amd_sp_dev*)data;

Please avoid casts whenever possible.

> +static int __init amd_sp_probe(void)
> +{
> +    int bus = 0, devfn = 0, rc;
> +    struct  amd_sp_dev *sp;
> +
> +     if ( !boot_cpu_has(X86_FEATURE_SEV) )
> +     {
> +      dprintk(XENLOG_INFO, "AMD SEV isn't supported on the platform\n");
> +      return 0;
> +     }
> +
> +     if ( boot_cpu_has(X86_FEATURE_XEN_SHSTK) )
> +     {
> +      force_sync = true;
> +
> +      dprintk(XENLOG_INFO,"AMD SEV: CET-SS detected - sync mode forced\n");
> +     }
> +
> +    for ( bus = 0; bus < 256; ++bus )
> +        for ( devfn = 0; devfn < 256; ++devfn )
> +        {
> +            struct pci_dev *pdev;

const?

Jan