Re: [Xen-devel] [PATCH] x86/mm: Consolidate all Xen L4 slot writing into init_xen_l4_slots()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.17 at 12:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/09/17 11:09, Jan Beulich wrote:
>>>>> On 01.09.17 at 19:07, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> Having all of this logic together makes it easier to follow Xen's virtual
>>> setup across the whole system.
>>>
>>> No practical changes to the resulting L4, although the logic has been
>>> rearanged to avoid rewriting some slots.  This changes the zap_ro_mpt
>>> parameter to simply ro_mpt.  Another side effect is that highmem-start= is
>>> applied consistently to all L4 tables, not just PV ones.
>> Is this side effect really a good idea to have?
> 
> Yes.  Otherwise the value of highmem-start= as a debugging mechanism is
> rather stunted.

Why? When it was introduced it was supposed to allow debugging
issues with the partial PV directmap vs the always-full HVM/idle one.

>> I.e. are you certain
>> HVM-only code (i.e. such known to only ever run in HVM or idle
>> vCPU context) is all prepared to not have everything direct-
>> mapped, and is using map_domain_page() et al everywhere?
> 
> I'm not aware of any places which would violate this.  Then again, as
> highmem-start= is too bitrotten to function, I can't test.

I'll see to look into what's broken.

>>> +void init_xen_l4_slots(l4_pgentry_t *l4t, mfn_t l4mfn,
>>> +                       const struct domain *d, mfn_t sl4mfn, bool ro_mpt)
>>> +{
>>> +    /* Slot 256: RO M2P (if applicable). */
>>> +    l4t[l4_table_offset(RO_MPT_VIRT_START)] =
>>> +        ro_mpt ? idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)]
>>> +               : l4e_empty();
>> While the patch in general looks correct, I'm also not convinced
>> having the slot numbers here as well as doing the operation in an
>> open-coded slot-by-slot fashion is a good idea: The comments
>> and the intended ordering here can easily go stale with future
>> adjustments to the virtual address layout.
> 
> The point of having all of this in one place is that there is only one
> place to change if the virtual address layout changes, so the chances of
> it getting stale are reduced.
> 
> The slot-by-slot fashion is how the old function was implemented after
> optimisation, except this version is shorter because we don't rewrite
> several slots.

Well, I certainly did assume the compiler would make this slot-by-
slot either way, so my concern is solely with how the source is
looking.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel