On 05/02/2018 01:13 PM, Jan Kiszka wrote:
> On 2018-05-02 12:40, Ralf Ramsauer wrote:
>> Hi Jan,
>>
>> On 05/01/2018 07:01 PM, Jan Kiszka wrote:
>>> On 2018-05-01 10:53, Ralf Ramsauer wrote:
>>>>>> +
>>>>>> +#define CON2_TYPE(flags) ((flags) & JAILHOUSE_CON2_TYPE_MASK)
>>>>>> +
>>>>>> +struct jailhouse_debug_console {
>>>>>> + __u64 address;
>>>>>> + __u32 size;
>>>>>> + __u32 flags;
>>>>>> + __u32 divider;
>>>>>> + __u32 gate_nr;
>>>>>> + __u64 clock_reg;
>>>>>> +} __attribute__((packed));
>>>>>> +
>>>>>> +#endif /* !_JAILHOUSE_CONSOLE_H */
>>>>>>
>>>>>
>>>>> It's actually no longer just a "debug" console when giving it to
>>>>> inmates, I would say.
>>>>
>>>> True. So let's rename the structure to struct jailhouse_console while
>>>> moving its location?
>>>
>>> Yep. We still call it jailhouse_system::debug_console, so that remains
>>> unambitious.
>>>
>>>>
>>>>>
>>>>> Moreover, we are creating a way more complex ABI towards our inmates
>>>>> when exposing that, compared to the simple platform info we currently
>>>>> have on x86.
>>>>>
>>>>> And we use the same API and ABI for the cell config now. That concerns
>>>>> me as well. For the cell config, we have at least some revision number
>>>>> now. The Comm Region may need that as well now. But even with that,
>>>>> there are now 3 places that can break on config updates: config
>>>>> definitions for the hypervisor and driver, python modules, and now also
>>>>> inmates...
>>>> We could also define an own (identical) anonymous structure for the
>>>> communication region and copy over members. Then compilation should fail
>>>> if we introduce any changes. Maybe together with some warning comments.
>>>
>>> At least we need to double-check if everything we defined for
>>> jailhouse_debug_console makes sense for an inmate. Dual-channel likely
>>> makes none at all.
>>
>> There's one member that makes no sense for the inmates: .size . This
>> member is only used by the hypervisor. All other fields are of relevance.
>
> Why should size make no sense? Also inmates need to map MMIO pages, and
> that could - at least theoretically - be larger than the standard size
> PAGE_SIZE.
Ok, but we don't do that at the moment.
>
>>
>> Nevertheless, I would still use that definition of the structure for
>> inmates, as it would have to be padded without the size member.
>
> OK, but now look into how fields are encoded, in particular flags. But
The only thing that inmates don't support (concerning flags) is the VGA
console on x86. JAILHOUSE_CON2_TYPE_ROOTPAGE will use the dbcon-hypercall.
> also gate_nr and clock_reg are not fully mature by now IIRC, even for
> the hypervisor case.
Well, the interface itself is mature I would say. Concerning inmates,
I'm passing gate_nr, clock_reg et. al. simply to the driver:
@@ -94,11 +83,11 @@ static void console_init(void)
return;
chip->base = (void *)(unsigned long)
- cmdline_parse_int("con-base", CON_BASE);
- chip->divider = cmdline_parse_int("con-divider", CON_DIVIDER);
- chip->gate_nr = cmdline_parse_int("con-gate-nr", CON_GATE_NR);
+ cmdline_parse_int("con-base", console->address);
+ chip->divider = cmdline_parse_int("con-divider", console->divider);
+ chip->gate_nr = cmdline_parse_int("con-gate-nr", console->gate_nr);
chip->clock_reg = (void *)(unsigned long)
- cmdline_parse_int("con-clock-reg", CON_CLOCK_REG);
+ cmdline_parse_int("con-clock-reg", console->clock_reg);
chip->init(chip);
Whatever is passed by struct jailhouse_console will be passed to the
driver. Maybe drivers are not fully mature and ignore those bits, but
that's another issue.
>
>>
>>>
>>>>
>>>> The communication region did not have any magic/revision so far, I can
>>>> introduce that if you want. But if those magics are at the beginning of
>>>> the structure (where they should be), then we will definitely break old
>>>> existing x86 inmates that are relying on the ABI.
>>>
>>> I don't think we have any out-of-tree x86 user of the region so far. So
>>> it /should/ be fine at this point to break it - for the last time
>>> without a chance to detect that.
>>
>> Ok. Turns out this is a bit tricky: While providing a magic/revision is
>> no problem at all, checking those fields is...
>>
>> I would like to check those field before jumping to inmate_main, because
>> then we can make the assumption that everything is fine and in place.
>> But if we want to do so, we _must_ ensure that all of our inmates have
>> access to the communication region, as it then becomes mandatory for all
>> inmates (which is okay, as it already is mandatory on x86, and will
>> become mandatory for arm after my series).
>
> Will we be using comm region fields on all arch prior to calling
> inmate_main?
>
>>
>> But what shall we do if the validation fails?
>
> That is indeed hard as we then have no communication channel either,
> neither via console nor even via the comm region itself. Except we
> define the new comm region in a way that provides stability for error
> signaling, even if the revision increases and other fields change in an
> incompatible way.
Good point.
>
>>
>> We need something like a panic() that crashes the cell _without_ writing
>> to the console (as it isn't ready at that point, and it will never be).
>> Emitting an invalid hypercall and encoding error information in
>> registers? Spin forever?
>
> See above, before adding another hypercall. It's basically about nailing
> cell_state down at a fixed location, along with a revision field, and
> encoding a state JAILHOUSE_CELL_FAILED_COMM_REV or so.
Got that.
Thanks
Ralf
>
> 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 [email protected].
For more options, visit https://groups.google.com/d/optout.
