From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mukesh Rathor <mukesh.rathor@oracle.com>
Subject: Re: [PATCH 14/18] PVH xen: Checks, asserts,
 and limitations for PVH
Date: Wed, 26 Jun 2013 19:43:58 -0700
Message-ID: <20130626194358.00213218@mantra.us.oracle.com>
References: <1372118507-16864-1-git-send-email-mukesh.rathor@oracle.com>
	<1372118507-16864-15-git-send-email-mukesh.rathor@oracle.com>
	<51C984E702000078000E0478@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <51C984E702000078000E0478@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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