From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 14/18] PVH xen: Checks, asserts, and limitations for PVH Date: Thu, 27 Jun 2013 08:25:43 +0100 Message-ID: <51CC051702000078000E1071@nat28.tlf.novell.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> <20130626194358.00213218@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130626194358.00213218@mantra.us.oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor Cc: xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 27.06.13 at 04:43, Mukesh Rathor wrote: > On Tue, 25 Jun 2013 10:54:15 +0100 > "Jan Beulich" wrote: > >> >>> On 25.06.13 at 02:01, Mukesh Rathor >> >>> 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. An error return seems correct, but -ENOSYS doesn't seem the best possible error code to correctly identify the kind of error. -EOPNOTSUPP perhaps? >> > --- 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. Removing the one above would be my preference, but the only requirement I have is that both cases should be consistent with one another. Jan