Re: [Xen-devel] [PATCH v2 3/4] x86/hvm: Break out __hvm_copy()'s translation logic

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.09.17 at 18:05, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3069,6 +3069,83 @@ void hvm_task_switch(
>      hvm_unmap_entry(nptss_desc);
>  }
>  
> +enum hvm_translation_result hvm_translate_get_page(
> +    struct vcpu *v, unsigned long addr, bool linear, uint32_t pfec,
> +    pagefault_info_t *pfinfo, struct page_info **page_p,
> +    gfn_t *gfn_p, p2m_type_t *p2mt_p)
> +{
> +    struct page_info *page;
> +    p2m_type_t p2mt;
> +    gfn_t gfn;
> +
> +    if ( linear )
> +    {
> +        gfn = _gfn(paging_gva_to_gfn(v, addr, &pfec));
> +
> +        if ( gfn_eq(gfn, INVALID_GFN) )
> +        {
> +            if ( pfec & PFEC_page_paged )
> +                return HVMTRANS_gfn_paged_out;
> +
> +            if ( pfec & PFEC_page_shared )
> +                return HVMTRANS_gfn_shared;
> +
> +            if ( pfinfo )
> +            {
> +                pfinfo->linear = addr;
> +                pfinfo->ec = pfec & ~PFEC_implicit;
> +            }
> +
> +            return HVMTRANS_bad_linear_to_gfn;
> +        }
> +    }
> +    else
> +    {
> +        gfn = _gfn(addr >> PAGE_SHIFT);

This wants to become gaddr_to_gfn(), now that we have it, and ...

> +        ASSERT(!pfinfo);
> +    }
> +
> +    /*
> +     * No need to do the P2M lookup for internally handled MMIO, benefiting
> +     * - 32-bit WinXP (& older Windows) on AMD CPUs for LAPIC accesses,
> +     * - newer Windows (like Server 2012) for HPET accesses.
> +     */
> +    if ( v == current
> +         && !nestedhvm_vcpu_in_guestmode(v)
> +         && hvm_mmio_internal(gfn_x(gfn) << PAGE_SHIFT) )

... this one gfn_to_gaddr().

> +        return HVMTRANS_bad_gfn_to_mfn;
> +
> +    page = get_page_from_gfn(v->domain, gfn_x(gfn), &p2mt, P2M_UNSHARE);
> +
> +    if ( !page )
> +        return HVMTRANS_bad_gfn_to_mfn;
> +
> +    if ( p2m_is_paging(p2mt) )
> +    {
> +        put_page(page);
> +        p2m_mem_paging_populate(v->domain, gfn_x(gfn));
> +        return HVMTRANS_gfn_paged_out;
> +    }
> +    if ( p2m_is_shared(p2mt) )
> +    {
> +        put_page(page);
> +        return HVMTRANS_gfn_shared;
> +    }
> +    if ( p2m_is_grant(p2mt) )
> +    {
> +        put_page(page);
> +        return HVMTRANS_unhandleable;
> +    }
> +
> +    *page_p = page;
> +    if ( gfn_p )
> +        *gfn_p = gfn;
> +    if ( p2mt_p )
> +        *p2mt_p = p2mt;

Considering the use in patch 4, is it really useful for p2mt_p to be an
optional output? I.e. do you foresee (in the near future) a case where
p2m_is_discard_write() doesn't need handling by the caller?

> @@ -103,6 +104,17 @@ enum hvm_translation_result hvm_fetch_from_guest_linear(
>      void *buf, unsigned long addr, int size, uint32_t pfec,
>      pagefault_info_t *pfinfo);
>  
> +/*
> + * Get a reference on the page under an HVM physical or linear address.  If
> + * linear, a pagewalk is performed using pfec (fault details optionally in
> + * pfinfo).
> + * On success, returns HVMTRANS_okay with a reference taken on **_page.
> + */
> +enum hvm_translation_result hvm_translate_get_page(
> +    struct vcpu *v, unsigned long addr, bool linear, uint32_t pfec,
> +    pagefault_info_t *pfinfo, struct page_info **page_p,
> +    gfn_t *gfn_p, p2m_type_t *p2mt_p);

The optional nature of whichever outputs we decide to be optional
should probably be called out in the comment, just like it is for pfinfo.

None of the points brought up would really require another version,
they could all easily be dealt with while committing. But of course we
first need to agree on at least this last one.

Jan


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