Re: [Xen-devel] [PATCH] x86/altp2m: clean up p2m_{get/set}_suppress_ve()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/23/2018 06:04 PM, Razvan Cojocaru wrote:
> Move p2m_{get/set}_suppress_ve() to p2m.c, replace incorrect
> ASSERT() in p2m-pt.c (since a guest can run in shadow mode even on
> a system with virt exceptions, which would trigger the ASSERT()),
> and move the VMX-isms (cpu_has_vmx_virt_exceptions checks) to
> p2m_ept_{get/set}_entry().
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>

Thanks for the clean up.  Two realtively minor comments...

> @@ -931,6 +942,16 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
>      mfn_t mfn = INVALID_MFN;
>      struct ept_data *ept = &p2m->ept;
>  
> +    if ( sve )
> +    {
> +        if ( !cpu_has_vmx_virt_exceptions )
> +            return INVALID_MFN;
> +
> +        /* #VE should be enabled for this vcpu. */
> +        if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> +            return INVALID_MFN;
> +    }

Is there a good reason to return error her rather than just putting '1'
in the sve location, like the p2m_pt.c version of this function does?


> +int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
> +                        unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    mfn_t mfn;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +
> +    /* #VE should be enabled for this vcpu. */
> +    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> +        return -ENXIO;

What's the purpose of checking for this here, if we don't check for this
in p2m_set_suppress_ve()?

Everything else looks good, thanks!

 -George

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