Re: [PATCH v4 4/4] virt/coco/sev-guest: interpret VMM errors from guest request

From: Dionna Amalie Glaze
Date: Fri Oct 28 2022 - 12:01:04 EST


> > static u32 vmpck_id;
> > module_param(vmpck_id, uint, 0444);
> > MODULE_PARM_DESC(vmpck_id, "The VMPCK ID to use when communicating with the PSP.");
> >
> > +static int rate_hz = 2;
>
> s/int/unsigned int/ to match uint below.
>

I'll change the uint to int, since the types in ratelimit_struct are int.

> > +module_param(rate_burst, uint, 0444);
> > +MODULE_PARM_DESC(rate_burst, "The rate limit burst amount to limit requests to.");
> > +
> > /* Mutex to serialize the shared buffer access and command handling. */
> > static DEFINE_MUTEX(snp_cmd_mutex);
> >
> > @@ -305,9 +316,12 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
> > u8 type, void *req_buf, size_t req_sz, void *resp_buf,
> > u32 resp_sz, __u64 *exitinfo2)
> > {
> > + unsigned int vmm_err;
> > u64 seqno;
> > int rc;
> >
> > + might_resched();
> > +
>
> Not sure this is needed. This may call cond_resched() right away, so I
> don't think this is doing what you think it is (as I only see it used in
> scheduler code and spinlock code). Did you mean to use might_sleep()?

Rereading the docs about it, I'll remove it.

>
> > /* Get message sequence and verify that its a non-zero */
> > seqno = snp_get_msg_seqno(snp_dev);
> > if (!seqno)
> > @@ -320,9 +334,35 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
> > if (rc)
> > return rc;
> >
> > +retry:
> > + /*
> > + * Rate limit commands internally since the host can also throttle, and
> > + * we don't want to create a tight request spin that could end up
> > + * getting this VM throttled more heavily.
> > + */
> > + if (!__ratelimit(&snp_dev->rs)) {
>
> This doesn't do any sleeping/delaying, it just returns a 0 or 1, so you
> could still cause a tight spin here. I guess that shouldn't be a problem
> is nothing else is ready to run. But maybe adding an msleep()/usleep()
> here based on the rate limit parameters (half the rate limit?) would be
> good so that the CPU isn't pegged while rating for the ratelimit to be
> satisified?
>

It looks like sleep_timeout_interruptible((rate_hz * HZ) / 2) could be
the appropriate solution for this.

> I'll let others chime in on that and see if it is appropriate.
>

Same, I'll wait until later Monday to send out v5.

> Thanks,
> Tom

Thanks for the reviews :)

--
-Dionna Glaze, PhD (she/her)