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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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?

~Andrew

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