Re: [Xen-devel] [PATCH v2 01/20] x86: make hvm_{get/set}_param accessible

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Fri, Dec 20, 2019 at 11:00 AM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 20/12/2019 17:50, Tamas K Lengyel wrote:
> > On Fri, Dec 20, 2019 at 10:47 AM Andrew Cooper
> > <andrew.cooper3@xxxxxxxxxx> wrote:
> >> On 20/12/2019 17:36, Tamas K Lengyel wrote:
> >>> On Fri, Dec 20, 2019 at 10:32 AM Andrew Cooper
> >>> <andrew.cooper3@xxxxxxxxxx> wrote:
> >>>> On 20/12/2019 17:27, Tamas K Lengyel wrote:
> >>>>> On Fri, Dec 20, 2019 at 9:47 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>>> On 18.12.2019 20:40, Tamas K Lengyel wrote:
> >>>>>>> Currently the hvm parameters are only accessible via the HVMOP 
> >>>>>>> hypercalls. By
> >>>>>>> exposing hvm_{get/set}_param it will be possible for VM forking to 
> >>>>>>> copy the
> >>>>>>> parameters directly into the clone domain.
> >>>>>> Having peeked ahead at patch 17, where this gets used, I wonder why
> >>>>>> you want a pair of one-by-one functions, rather than a copy-all one.
> >>>>>> This then wouldn't require exposure of the functions you touch here.
> >>>>> Well, provided there is no such function in existence today it was
> >>>>> just easier to use what's already available. I still wouldn't want to
> >>>>> implement a one-shot function like that because this same code-path is
> >>>>> shared by the save-restore operations on the toolstack side, so at
> >>>>> least I have a reasonable assumption that it won't break on me in the
> >>>>> future.
> >>>> In particular, a number of the set operations are distinctly
> >>>> non-trivial.  (OTOH, those are not long for this world, and should be
> >>>> creation X86_EMU_* constants instead).
> >>>>
> >>> I actually wanted to ask about that. In
> >>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxc/xc_sr_save_x86_hvm.c;h=97a8c49807f192c47209525f51e4d79a50c66cec;hb=HEAD#l61
> >>> the toolstack only selects certain HVM params to be saved (and
> >>> restored later). I originally followed the same logic in the fork
> >>> code-path but after a lot of experiments it looks like it's actually
> >>> OK to grab all params but only call set_param on the ones that have a
> >>> non-zero value. So setting some params with a zero value has certainly
> >>> lead to crashes, but otherwise it seems to "just work" to copy all the
> >>> rest.
> >> I think you're trying to ascribe any form of design/plan to a system
> >> which had none. :)
> >>
> >> The code you quote was like that because that is how legacy migration
> >> worked.  That said, eliding empty records was an effort-saving exercise
> >> (avoid redundant hypercalls on destination side), not because there was
> >> any suggestion that attempting to explicitly set 0 would crash.
> >>
> >> Do you have any idea which param was causing problems?
> > Yes, HVM_PARAM_IDENT_PT was one sure. There may have been others (I
> > don't recall now) but simply checking for non-zero value before
> > calling set_param resolved everything.
>
> IDENT_PT is an Westmere(?) wrinkle.
>
> There was one processor back in those days which supported EPT, but
> didn't support VT-x running in unpaged mode.  Therefore, we had to fake
> up unpaged mode by pointing vCR3 at an identity pagetable inside the
> guests physical address space.

Eh, yikes.

>
> The crash won't be from the IDENT_PT itself, but the paging_update_cr3()
> side effect.  Was it a host crash, or guest crash?
>

Yes, that's what I recall after I looked into it. It was a guest a
crash as I remember.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel