Re: [Xen-devel] [PATCH 2/2] x86/pv: Code improvements to do_update_descriptor()

["Jan Beulich" <JBeulich@xxxxxxxx>]

 >>> On 06.12.18 at 19:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/pv/descriptor-tables.c
> +++ b/xen/arch/x86/pv/descriptor-tables.c
> @@ -206,30 +206,26 @@ int compat_set_gdt(XEN_GUEST_HANDLE_PARAM(uint) 
> frame_list,
>      return ret;
>  }
>  
> -long do_update_descriptor(uint64_t pa, uint64_t desc)
> +long do_update_descriptor(uint64_t gaddr, uint64_t desc)
>  {
>      struct domain *currd = current->domain;
> -    unsigned long gmfn = pa >> PAGE_SHIFT;
> -    unsigned long mfn;
> -    unsigned int  offset;
> -    seg_desc_t *gdt_pent, d;
> +    gfn_t gfn = gaddr_to_gfn(gaddr);
> +    mfn_t mfn;
> +    seg_desc_t *entry, d;
>      struct page_info *page;
>      long ret = -EINVAL;
>  
> -    offset = ((unsigned int)pa & ~PAGE_MASK) / sizeof(seg_desc_t);
> +    d.raw = desc;
>  
> -    *(uint64_t *)&d = desc;

Instead of old or new variant, how about simply making the
second function parameter seg_desc_t?

> +    /* gaddr must be aligned, or it will corrupt adjacent descriptors. */
> +    if ( !IS_ALIGNED(gaddr, sizeof(d)) || !check_descriptor(currd, &d) )
> +        goto out;

I'd really prefer if we didn't see goto-s added where they're really
unneeded. You easily "return -EINVAL" here and ...

> -    page = get_page_from_gfn(currd, gmfn, NULL, P2M_ALLOC);
> -    if ( (((unsigned int)pa % sizeof(seg_desc_t)) != 0) ||
> -         !page ||
> -         !check_descriptor(currd, &d) )
> -    {
> -        if ( page )
> -            put_page(page);
> -        return -EINVAL;
> -    }
> -    mfn = mfn_x(page_to_mfn(page));
> +    page = get_page_from_gfn(currd, gfn_x(gfn), NULL, P2M_ALLOC);
> +    if ( !page )
> +        goto out;

... here, avoiding the if() at the "out" label at the same time.

> @@ -244,19 +240,20 @@ long do_update_descriptor(uint64_t pa, uint64_t desc)
>          break;
>      }
>  
> -    paging_mark_dirty(currd, _mfn(mfn));
> +    paging_mark_dirty(currd, mfn);
>  
>      /* All is good so make the update. */
> -    gdt_pent = map_domain_page(_mfn(mfn));
> -    write_atomic((uint64_t *)&gdt_pent[offset], *(uint64_t *)&d);
> -    unmap_domain_page(gdt_pent);
> +    entry = map_domain_page(mfn) + (gaddr & ~PAGE_MASK);
> +    ACCESS_ONCE(entry->raw) = d.raw;

Why not "ACCESS_ONCE(*entry) = d;"? I'm having trouble to
understand why the macro insists on using scalar types (there's
no comment there explaining the need).

The earlier and this change together would then also eliminate
the need for a union as it seems.

Jan



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