Re: [PATCH v5 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[George Dunlap <dunlapg@umich.edu>]

Sorry, looks like this may not have gone through.
 -G
On Thu, Sep 20, 2018 at 5:08 PM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On Thu, Sep 20, 2018 at 4:53 PM Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
> >
> > On 9/20/18 2:34 PM, George Dunlap wrote:
> > >> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> > >> +                        unsigned int altp2m_idx)
> > > This should clearly be in p2m.c, and...
> > >
> > >> +{
> > >> +    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;
> > >> +    int rc;
> > >> +
> > >> +    if ( !cpu_has_vmx_virt_exceptions )
> > >> +        return -EOPNOTSUPP;
> > > We should avoid Intel-specific checks in common code.
> > >
> > > In fact, this is wrong, because you can choose to run a guest in shadow
> > > mode even on a system with virt exceptions -- in which case you'd
> > > trigger the ASSERT() in p2m-pt.c:p2m_pt_set_entry().
> > >
> > > Probably what should happen is that we should move the vmx check into
> > > p2m-ept.c:p2m_ept_set_entry(), and replace the ASSERT(sve = 0) in
> > > p2m-pt.c:p2m_pt_set_entry() with "if ( sve != 0 ) return -ENOTSUPP;".
> > >
> > > Although what should probably *really* happen is that
> > > `HVMOP_altp2m_vcpu_enable_notify` should fail with -EOPNOTSUPP instead
> > > of silently succeeding.
> >
> > Do you mean HVMOP_altp2m_set_suppress_ve here, or am I misunderstanding
> > your comment? I'm happy to do the exact modifications you've requested
> > above but I'm afraid I don't follow the HVMOP_altp2m_vcpu_enable_notify
> > comment.
>
> At the moment, it appears that you can call
> HVMOP_altp2m_vcpu_enable_notify to set a vcpu's veinfo_gfn even if the
> hardware or the p2m mode doesn't support #VE.  It would be better if
> callers got the -EOPNOTSUPP error at that point, rather than having
> vcpu_enable_notify succeed and then having set_suppress_ve fail.
>
> To be clear, I'm not requiring that as a clean-up, I'm just saying
> that such a change would be an improvement (which you may consider
> doing if you wish).
>
>  -George

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