Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Oct 20, 2021 at 12:57:11PM +0200, Jan Beulich wrote:
> On 20.10.2021 10:04, Roger Pau Monné wrote:
> > On Fri, Oct 15, 2021 at 11:48:33AM +0200, Jan Beulich wrote:
> >> On 15.10.2021 11:39, Jan Beulich wrote:
> >>> On 22.09.2021 10:21, Roger Pau Monne wrote:
> >>>> --- a/xen/include/public/domctl.h
> >>>> +++ b/xen/include/public/domctl.h
> >>>> @@ -87,14 +87,22 @@ struct xen_domctl_createdomain {
> >>>>      /*
> >>>>       * Various domain limits, which impact the quantity of resources
> >>>>       * (global mapping space, xenheap, etc) a guest may consume.  For
> >>>> -     * max_grant_frames and max_maptrack_frames, < 0 means "use the
> >>>> -     * default maximum value in the hypervisor".
> >>>> +     * max_grant_frames, max_maptrack_frames and max_gnttab_version < 0
> >>>> +     * means "use the default maximum value in the hypervisor".
> >>>>       */
> >>>>      uint32_t max_vcpus;
> >>>>      uint32_t max_evtchn_port;
> >>>>      int32_t max_grant_frames;
> >>>>      int32_t max_maptrack_frames;
> >>>>  
> >>>> +/* Grant version, use low 4 bits. */
> >>>> +#define XEN_DOMCTL_GRANT_version_mask    0xf
> >>>> +#define XEN_DOMCTL_GRANT_version_default 0xf
> >>>> +
> >>>> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
> >>>> +
> >>>> +    uint32_t grant_opts;
> >>>
> >>> As it now seems unlikely that this will make 4.16, please don't forget
> >>> to bump the interface version for 4.17. With that and preferably with
> >>> the nit above addressed, hypervisor parts:
> >>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> Actually no, I'm afraid there is an issue with migration: If the tool
> >> stack remembers the "use default" setting and hands this to the new
> >> host, that host's default may be different from the source host's. It
> >> is the effective max-version that needs passing on in this case, which
> >> in turn requires a means to obtain the value.
> > 
> > Hm, my thinking was that the admin (or a higer level orchestration
> > tool) would already have performed the necessary adjustments to assert
> > the environments are compatible.
> 
> I don't think we can rely on this in the hypervisor. We may take this
> as a prereq for proper working, but I think we ought to detect
> violations and report errors in such a case.
> 
> > This problem already exist today where you can migrate a VM from a
> > host having the max default grant version to one having
> > gnttab=max-ver:1 without complains.
> 
> Are you sure about "without complaints"?

Without hypervisor complaining AFAICT, as the max grant version is
not migrated or checked in any way ATM.

> What would a guest do if it
> expects to be able to use v2, since it was able to on the prior host?

IMO any well behaved guest should be capable of handling both v1 and
v2, and lacking v2 a guest should fallback to v1 on resume, as the
grant table needs to be re-initialized anyway. I think it would be
wrong (ie: a bug) for guests to assume v2 to be present on resume
based on the fact it was present previously, as it would be wrong for
a block frontend to assume the same features to be present on resume
for example.

If a guest only supports grant v2 then I think it's up to the
administrator to set the max version explicitly in the config file and
that would be acknowledged on the destination end and migration
aborted if not supported. I think the behavior after this patch
is more user friendly that the current one, as no checks are
performed currently.

Thanks, Roger.