Re: [PATCH v11 42/45] virt: Add SEV-SNP guest driver

[Dave Hansen]

> +++ b/drivers/virt/coco/sevguest/Kconfig
> @@ -0,0 +1,12 @@
> +config SEV_GUEST
> +	tristate "AMD SEV Guest driver"
> +	default m
> +	depends on AMD_MEM_ENCRYPT && CRYPTO_AEAD2
> +	help
> +	  SEV-SNP firmware provides the guest a mechanism to communicate with
> +	  the PSP without risk from a malicious hypervisor who wishes to read,
> +	  alter, drop or replay the messages sent. The driver provides
> +	  userspace interface to communicate with the PSP to request the
> +	  attestation report and more.
> +
> +	  If you choose 'M' here, this module will be called sevguest.
...
> +/* Return a non-zero on success */
> +static u64 snp_get_msg_seqno(struct snp_guest_dev *snp_dev)
> +{
> +	u64 count = __snp_get_msg_seqno(snp_dev);
> +
> +	/*
> +	 * The message sequence counter for the SNP guest request is a  64-bit
> +	 * value but the version 2 of GHCB specification defines a 32-bit storage
> +	 * for it. If the counter exceeds the 32-bit value then return zero.
> +	 * The caller should check the return value, but if the caller happens to
> +	 * not check the value and use it, then the firmware treats zero as an
> +	 * invalid number and will fail the  message request.
> +	 */
> +	if (count >= UINT_MAX) {
> +		pr_err_ratelimited("request message sequence counter overflow\n");
> +		return 0;
> +	}
> +
> +	return count;
> +}

I didn't see a pr_fmt defined anywhere.  But, for a "driver", should
this be a dev_err()?

...
> +static void free_shared_pages(void *buf, size_t sz)
> +{
> +	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +
> +	if (!buf)
> +		return;
> +
> +	if (WARN_ONCE(set_memory_encrypted((unsigned long)buf, npages),
> +		      "failed to restore encryption mask (leak it)\n"))
> +		return;
> +
> +	__free_pages(virt_to_page(buf), get_order(sz));
> +}

Nit: It's a bad practice to do important things inside a WARN_ON() _or_
and if().  This should be:

	int ret;

	...

	ret = set_memory_encrypted((unsigned long)buf, npages));

	if (ret) {
		WARN_ONCE(...);
		return;
	}
	
BTW, this look like a generic allocator thingy.  But it's only ever used
to allocate a 'struct snp_guest_msg'.  Why all the trouble to allocate
and free one fixed-size structure?  The changelog and comments don't
shed any light.