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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> 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.

>
>> +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.

~Andrew

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