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

[Dionna Amalie Glaze]

> >   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)