Re: [PATCH 14/18] PVH xen: Checks, asserts, and limitations for PVH

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Tue, 25 Jun 2013 10:54:15 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -578,6 +578,9 @@ int32_t hvm_set_mem_pinned_cacheattr(
> >  {
> >      struct hvm_mem_pinned_cacheattr_range *range;
> >  
> > +    /* A PVH guest writes to MSR_IA32_CR_PAT natively. */
> > +    ASSERT(!is_pvh_domain(d));
> 
> This can't be an assert, or did I overlook you preventing the
> function to be called for PVH guests.
> 
> The comment would then be wrong too, as there is a path
> leading here from a domctl (i.e. unaffected by how the guest
> itself would access the MSR).

Well, there are no callers right now, and I wanted to catch any during
my test runs. But, now I think the ASSERT should be replaced with
returning  -ENOSYS. Let me know if you disagree.


> 
> > --- a/xen/arch/x86/x86_64/traps.c
> > +++ b/xen/arch/x86/x86_64/traps.c
> > @@ -440,6 +440,8 @@ static long register_guest_callback(struct
> > callback_register *reg) long ret = 0;
> >      struct vcpu *v = current;
> >  
> > +    ASSERT(!is_pvh_vcpu(v));
> > +
> 
> For one, I don't think there has been anything so far making
> clear that this is unreachable for PVH.

hvm_do_hypercall() returns -ENOSYS for both callers of register_guest_callback
so this is unreachable for PVH. I can even remove the ASSERT if you'd
like.

> And then it is inconsistent to do this here, but not also in
> unregister_guest_callback().

I can add one here too, or remove the one from register.

thanks
Mukesh