Re: [PATCH 4/4] spapr: Improve spapr_reallocate_hpt() error reporting

[Greg Kurz]

On Mon, 26 Oct 2020 14:49:34 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 10/26/20 1:40 PM, Greg Kurz wrote:
> > spapr_reallocate_hpt() has three users, two of which pass &error_fatal
> > and the third one, htab_load(), passes &local_err, uses it to detect
> > failures and simply propagates -EINVAL up to vmstate_load(), which will
> > cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt()
> > doesn't return right away when an error is detected in some cases. Also,
> > the comment suggesting that the caller is welcome to try to carry on
> > seems like a remnant in this respect.
> > 
> > This can be improved:
> > - change spapr_reallocate_hpt() to always report a negative errno on
> >    failure, either as reported by KVM or -ENOSPC if the HPT is smaller
> >    than what was asked,
> > - use that to detect failures in htab_load() which is preferred over
> >    checking &local_err,
> > - propagate this negative errno to vmstate_load() because it is more
> >    accurate than propagating -EINVAL for all possible errors.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> ...
> 
> > -void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift,
> > -                          Error **errp)
> > +int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp)
> >   {
> >       ERRP_GUARD();
> >       long rc;
> > @@ -1496,7 +1495,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, 
> > int shift,
> >   
> >       if (rc == -EOPNOTSUPP) {
> >           error_setg(errp, "HPT not supported in nested guests");
> > -        return;
> > +        return -EOPNOTSUPP;
> >       }
> >   
> >       if (rc < 0) {
> > @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, 
> > int shift,
> >           error_setg_errno(errp, errno, "Failed to allocate KVM HPT of 
> > order %d",
> >                            shift);
> >           error_append_hint(errp, "Try smaller maxmem?\n");
> > -        /* This is almost certainly fatal, but if the caller really
> > -         * wants to carry on with shift == 0, it's welcome to try */
> > +        return -errno;
> 
> Maybe returning here should be in a previous patch.
> Otherwise patch looks good.
> 

It could have been indeed...

> >       } else if (rc > 0) {
> >           /* kernel-side HPT allocated */
> >           if (rc != shift) {
> > @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, 
> > int shift,
> >                          "Requested order %d HPT, but kernel allocated 
> > order %ld",
> >                          shift, rc);
> >               error_append_hint(errp, "Try smaller maxmem?\n");
> > +            return -ENOSPC;

... along with this one.

I didn't go this way because it doesn't really affect the final behavior since
QEMU exits in all cases. It's mostly about propagating an appropriate errno up
to VMState in the case of htab_load(). But if you find it clearer and I need
to post a v2, I can certainly do that.

> >           }
> >   
> >           spapr->htab_shift = shift;
> > @@ -1533,6 +1532,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, 
> > int shift,
> >       /* We're setting up a hash table, so that means we're not radix */
> >       spapr->patb_entry = 0;
> >       spapr_set_all_lpcrs(0, LPCR_HR | LPCR_UPRT);
> > +    return 0;
> >   }
> >   
> >   void spapr_setup_hpt(SpaprMachineState *spapr)
> > @@ -2286,11 +2286,13 @@ static int htab_load(QEMUFile *f, void *opaque, int 
> > version_id)
> >       }
> >   
> >       if (section_hdr) {
> > +        int ret;
> > +
> >           /* First section gives the htab size */
> > -        spapr_reallocate_hpt(spapr, section_hdr, &local_err);
> > -        if (local_err) {
> > +        ret = spapr_reallocate_hpt(spapr, section_hdr, &local_err);
> > +        if (ret < 0) {
> >               error_report_err(local_err);
> > -            return -EINVAL;
> > +            return ret;
> >           }
> >           return 0;
> >       }
> ...
>