Hi,

On 04/11/2018 11:17 AM, f...@ozog.com wrote:
> On 11.04.2018 10:27, Jan Kiszka wrote:
>> On 2018-04-11 09:55, f...@ozog.com wrote:
>>> In trying to get the tiny-demo send logs to jailhouse debug console I
>>> found that:
>>> - inmate lib handling of cmdline is broken
>>> - inmate lib detection whether to use VMCALL (Intel) or VMMCALL (AMD)
>>> instructions to do hypercalls is broken
>>>
>>> I found fixes but I'd like to discuss how to properly address the issues
>>>
>>> I -  cmdline
>>>
>>> I.1 - problem
>>>
>>> according to inmates/lib/x86/inmate.lds
>>> cmdline is located at offset 0x100 in the running inmate
>>> it contains one byte (a zero).
>>> inmate code starts at 0x101
>>
>> Well, that's the location of the text section if there is nothing in the
>> inmate that otherwise uses the cmdline section. But if your inmate is
>> using the command line (lib/cmdline.c), it will usually reserve another
>> 256 bytes (CMDLINE_BUFFER_SIZE). If something else provides a
>> CMDLINE_BUFFER, that will will and may change that value.
>>
>>>
>>> according to Documentation/debug-output.md
>>> command line parameters are specified using -s <STRING>
>>>
>>> lastly, cell_shutdown_load() in tools/jailhouse.c considers "-s
>>> <STRING>" as a second image to be loaded
>>>
>>> so the command
>>>
>>> jailhouse cell load --name tiny-demo inmates/demos/x86/tiny-demo.bin -s
>>> "con-type=JAILHOUSE"
>>
>> ...is wrong. It's missing the "-a 0x100" on x86 (would be 0x1000 on ARM
>> architectures). The documentation above is listing this in the examples
>> section.
>>
>> Please suggest better wording of that document that could help to avoid
>> such misunderstanding.
>>
>>>
>>> has the following effect:
>>>
>>> load_image() @ driver/cell.c uses the first available memory_region and
>>> copies inmates/demos/x86/tiny-demo.bin
>>> to that location
>>> then load_image() actually uses the same memory_region (same physical
>>> start but ioremapped at a different location)
>>> and overwrites the first bytes of inmates/demos/x86/tiny-demo.bin.
>>>
>>> Unexpectedly, the string "con-type=JAILHOUSE" happen to correspond to
>>> executable code bytes and it does "something" on my system.
>>>
>>> I.2 - possible solution
>>>
>>> the simplest way to solve this is:
>>> - reserve say the rest of the first page to boot code and command line:
>>> a inmate.lds patch can do the job
>>> - change jailhouse.c to load the fake image at offset 0x100 (updating
>>> target_address) so that driver.c does not override the .boot section
>>>
>>> If this is acceptable for now, I can publish patches.
>>
>> This is a documentation issue, at most. See above.
> 
> I now understand.
> 
> My reading of
>    cell load { ID | [--name] NAME } { IMAGE | { -s | --string } "STRING" }
>              [-a | --address ADDRESS] ...
> 
> was either wrong or have to be improved
> 
> adding {} seems clearer to me:
>    cell load { ID | [--name] NAME } { { IMAGE | { -s | --string }
> "STRING" }
>              [-a | --address ADDRESS] } ...
> 
> The following example is misleading:
> 
>     jailhouse cell load foocell inmate.bin -s "con-type=JAILHOUSE" -a 0x100
> 
> it looks like -s and -a are parameters to inmate.bin
> 
> The following better shows that we are loading two images,
> the second one patching the first one at offset 0x100
> 
>     jailhouse cell load foocell
>         inmate.bin -a 0 \
>         -s "con-type=JAILHOUSE" -a 0x100
> 
> which can be abbreviated to :
> 
>     jailhouse cell load foocell
>         inmate.bin \
>         -s "con-type=JAILHOUSE" -a 0x100
> 
> The following sample illustrate the overall concepts of multiple images
> loading
> 
>     jailhouse cell load foocell \
>         inmate.bin \
>         -s "con-type=JAILHOUSE" -a 0x100 \
>         sharedobject.so -a 0x1000000 \
>         ramfs.bin -a 0x2000000
> 
> If this is correct, I'll propose a set of Documentation patches
This is correct. You could also explicitly mention that the address is
defaulted to 0x0.

And that images or strings are loaded in the same order as specified, if
regions overlap, the latter will overwrite the content.

  Ralf
> 
>>
>>>
>>> II - VMCALL
>>>
>>> II.1 - problem
>>> hypercall_init() @ checks for X86_FEATURE_VMX to use either VMCALL or
>>> VMMCALL.
>>> but vcpu_handle_cpuid() @ hypervisor/arch/x86/vcpu.c always clears this
>>> bit for non root cell.
>>> Thus, hypercalls in non AMD system do not work at all in non-root cells
>>> based on inmates demo code!
>>
>> Yeah, that's a regression of my commit 39dfc93aa472...
>>
>>>
>>> II.2 - possible solution
>>>
>>> Linux as a guest uses a "synthetic" bit, X86_FEATURE_VMMCALL,
>>> see
>>> https://elixir.bootlin.com/linux/v4.16.1/source/arch/x86/include/asm/cpufeatures.h#L225
>>>
>>>
>>> to detect proper alternative
>>> #define KVM_HYPERCALL \
>>>         ALTERNATIVE(".byte 0x0f,0x01,0xc1", ".byte 0x0f,0x01,0xd9",
>>> X86_FEATURE_VMMCALL)
>>>
>>> I would suggest the same, adding a bit in cpuid leaf
>>> (JAILHOUSE_CPUID_FEATURES).
>>>
>>
>> That X86_FEATURE_VMMCALL is set by Linux itself. We should port that
>> logic over: wwitch to VMMCALL if and only if an AMD CPU is detected
>> (linux/arch/x86/kernel/cpu/amd.c), otherwise stick with Intel's VMCALL.
>> Patch welcome!
>>
>> Jan
> 

-- 
You received this message because you are subscribed to the Google Groups 
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to jailhouse-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to