Re: [Xen-devel] [PATCH v3 1/2] x86/mm: factor out the code for shattering an l3 PTE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.12.2019 11:58, Hongyan Xia wrote:
> map_pages_to_xen and modify_xen_mappings are performing almost exactly
> the same operations when shattering an l3 PTE, the only difference
> being whether we want to flush.
> 
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> 
> ---
> Changes in v3:
> - style and indentation changes.
> - return -ENOMEM instead of -1.
> 
> Changes in v2:
> - improve asm.
> - re-read pl3e from memory when taking the lock.
> - move the allocation of l2t inside the shatter function.
> ---
>  xen/arch/x86/mm.c | 98 +++++++++++++++++++++++------------------------
>  1 file changed, 49 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7d4dd80a85..97f11b6016 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5151,6 +5151,52 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
>                           flush_area_local((const void *)v, f) : \
>                           flush_area_all((const void *)v, f))
>  
> +/* Shatter an l3 entry and populate l2. If virt is passed in, also do flush. 
> */
> +static int shatter_l3e(l3_pgentry_t *pl3e, unsigned long virt, bool locking)
> +{
> +    unsigned int i;
> +    l3_pgentry_t ol3e = *pl3e;
> +    l2_pgentry_t l2e = l2e_from_intpte(l3e_get_intpte(ol3e));
> +    l2_pgentry_t *l2t = alloc_xen_pagetable();
> +
> +    if ( !l2t )
> +        return -ENOMEM;
> +
> +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> +    {
> +        l2e_write(l2t + i, l2e);
> +        l2e = l2e_from_intpte(
> +                  l2e_get_intpte(l2e) + (PAGE_SIZE << PAGETABLE_ORDER));

Andrew - iirc you had suggested this (in some different form, but
to the same effect) to improve code generation. If you're convinced
that the downside of the loss of type safety is worth the win in
generated code, I'm not going to stand in the way here, but it'll
then need to be you to ack these two patches in their eventually
final shape.

> +    }
> +
> +    if ( locking )
> +        spin_lock(&map_pgdir_lock);
> +    if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
> +         (l3e_get_flags(*pl3e) & _PAGE_PSE) )
> +    {
> +        l3e_write_atomic(pl3e,
> +            l3e_from_paddr((paddr_t)virt_to_maddr(l2t), __PAGE_HYPERVISOR));

Why the cast? (I'm sorry if this was there on v3 already and I
didn't spot it. And if this remains the only thing to adjust,
then I guess this could be taken care of while committing.)

Jan

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