Re: [PATCH v3 1/5] x86/pmstat: Check size of PMSTAT_get_pxstat buffers

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Thu, Jun 5, 2025 at 11:10 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 27.05.2025 17:26, Ross Lagerwall wrote:
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -215,11 +215,22 @@ typedef struct pm_px_val pm_px_val_t;
> >  DEFINE_XEN_GUEST_HANDLE(pm_px_val_t);
> >
> >  struct pm_px_stat {
> > -    uint8_t total;        /* total Px states */
> > +    /*
> > +     * IN: Number of elements in pt, number of rows/columns in trans_pt
> > +     *     (PMSTAT_get_pxstat)
> > +     * OUT: total Px states (PMSTAT_get_max_px, PMSTAT_get_pxstat)
> > +     */
> > +    uint8_t total;
> >      uint8_t usable;       /* usable Px states */
> >      uint8_t last;         /* last Px state */
> >      uint8_t cur;          /* current Px state */
> > -    XEN_GUEST_HANDLE_64(uint64) trans_pt;   /* Px transition table */
> > +    /*
> > +     * OUT: Px transition table. This should have total * total elements.
> > +     *      As it is a 2-D array, this will not be copied if it is smaller 
> > than
> > +     *      the hypervisor's Px transition table. (PMSTAT_get_pxstat)
> > +     */
> > +    XEN_GUEST_HANDLE_64(uint64) trans_pt;
> > +    /* OUT: This should have total elements (PMSTAT_get_pxstat) */
> >      XEN_GUEST_HANDLE_64(pm_px_val_t) pt;
> >  };
>
> Commentary here is still confusing imo: Since "total" now has two meanings,
> saying "This should have .." in OUT: descriptions is ambiguous. Imo for
> trans_pt you want to say something like "will not be copied if input total is
> less than output total", and for pt "the number of elements copied is the
> smaller of input and output total".
>
> If that's okay with you, I can edit things along these lines while committing,
> at which point
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>

Sure, that's fine with me.

Thanks,
Ross