On Sat, Dec 09, 2023 at 04:36:56PM +0100, Borislav Petkov wrote:
> > +static int __snp_cmd_buf_copy(int cmd, void *cmd_buf, bool to_fw, int fw_err)
> > +{
> > + int (*func)(u64 *paddr, u32 len, bool guest, struct snp_host_map *map);
> > + struct sev_device *sev = psp_master->sev_data;
> > + bool from_fw = !to_fw;
> > +
> > + /*
> > + * After the command is completed, change the command buffer memory to
> > + * hypervisor state.
> > + *
> > + * The immutable bit is automatically cleared by the firmware, so
> > + * no not need to reclaim the page.
> > + */
> > + if (from_fw && sev_legacy_cmd_buf_writable(cmd)) {
> > + if (snp_reclaim_pages(__pa(cmd_buf), 1, true))
> > + return -EFAULT;
> > +
> > + /* No need to go further if firmware failed to execute command. */
> > + if (fw_err)
> > + return 0;
> > + }
> > +
> > + if (to_fw)
> > + func = map_firmware_writeable;
> > + else
> > + func = unmap_firmware_writeable;
>
> Eww, ugly and with the macro above even worse. And completely
> unnecessary.
>
> Define prep_buffer() as a normal function which selects which @func to
> call and then does it. Not like this.
I've rewritten this using a descriptor array to handle buffers for
various command parameters, and switched to allocating bounce buffers
on-demand to avoid some of the init/cleanup coordination. I dont think
any of these are really performance critical and its only for legacy
support, but would be straightforward to add a cache of pre-allocated
buffers later if needed.
I've tried to document/name the helpers so the flow is a bit clearer.
-Mike
>
> ...
>
> > +static inline bool need_firmware_copy(int cmd)
> > +{
> > + struct sev_device *sev = psp_master->sev_data;
> > +
> > + /* After SNP is INIT'ed, the behavior of legacy SEV command is changed. */
>
> "initialized"
>
> > + return ((cmd < SEV_CMD_SNP_INIT) && sev->snp_initialized) ? true : false;
>
> redundant ternary conditional:
>
> return cmd < SEV_CMD_SNP_INIT && sev->snp_initialized;
>
> > +}
> > +
> > +static int snp_aware_copy_to_firmware(int cmd, void *data)
>
> What does "SNP aware" even mean?
>
> > +{
> > + return __snp_cmd_buf_copy(cmd, data, true, 0);
> > +}
> > +
> > +static int snp_aware_copy_from_firmware(int cmd, void *data, int fw_err)
> > +{
> > + return __snp_cmd_buf_copy(cmd, data, false, fw_err);
> > +}
> > +
> > static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> > {
> > struct psp_device *psp = psp_master;
> > struct sev_device *sev;
> > unsigned int phys_lsb, phys_msb;
> > unsigned int reg, ret = 0;
> > + void *cmd_buf;
> > int buf_len;
> >
> > if (!psp || !psp->sev_data)
> > @@ -487,12 +770,28 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> > * work for some memory, e.g. vmalloc'd addresses, and @data may not be
> > * physically contiguous.
> > */
> > - if (data)
> > - memcpy(sev->cmd_buf, data, buf_len);
> > + if (data) {
> > + if (sev->cmd_buf_active > 2)
>
> What is that silly counter supposed to mean?
>
> Nested SNP commands?
>
> > + return -EBUSY;
> > +
> > + cmd_buf = sev->cmd_buf_active ? sev->cmd_buf_backup : sev->cmd_buf;
> > +
> > + memcpy(cmd_buf, data, buf_len);
> > + sev->cmd_buf_active++;
> > +
> > + /*
> > + * The behavior of the SEV-legacy commands is altered when the
> > + * SNP firmware is in the INIT state.
> > + */
> > + if (need_firmware_copy(cmd) && snp_aware_copy_to_firmware(cmd, cmd_buf))
>
> Move that need_firmware_copy() check inside snp_aware_copy_to_firmware()
> and the other one.
>
> > + return -EFAULT;
> > + } else {
> > + cmd_buf = sev->cmd_buf;
> > + }
> >
> > /* Get the physical address of the command buffer */
> > - phys_lsb = data ? lower_32_bits(__psp_pa(sev->cmd_buf)) : 0;
> > - phys_msb = data ? upper_32_bits(__psp_pa(sev->cmd_buf)) : 0;
> > + phys_lsb = data ? lower_32_bits(__psp_pa(cmd_buf)) : 0;
> > + phys_msb = data ? upper_32_bits(__psp_pa(cmd_buf)) : 0;
> >
> > dev_dbg(sev->dev, "sev command id %#x buffer 0x%08x%08x timeout %us\n",
> > cmd, phys_msb, phys_lsb, psp_timeout);
>
> ...
>
> > @@ -639,6 +947,14 @@ static int ___sev_platform_init_locked(int *error, bool probe)
> > if (probe && !psp_init_on_probe)
> > return 0;
> >
> > + /*
> > + * Allocate the intermediate buffers used for the legacy command handling.
> > + */
> > + if (rc != -ENODEV && alloc_snp_host_map(sev)) {
>
> Why isn't this
>
> if (!rc && ...)
>
> > + dev_notice(sev->dev, "Failed to alloc host map (disabling legacy SEV)\n");
> > + goto skip_legacy;
>
> No need for that skip_legacy silly label. Just "return 0" here.
>
> ...
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
>
