From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [V10 PATCH 23/23] PVH xen: introduce vmexit handler for PVH Date: Thu, 15 Aug 2013 17:44:10 +0100 Message-ID: <520D055A.4050700@eu.citrix.com> References: <1374631171-15224-1-git-send-email-mukesh.rathor@oracle.com> <1374631171-15224-24-git-send-email-mukesh.rathor@oracle.com> <20130725162840.GD87903@ocelot.phlegethon.org> <20130725193057.434cd254@mantra.us.oracle.com> <20130726104519.GB99616@ocelot.phlegethon.org> <20130806173711.2cb6219c@mantra.us.oracle.com> <20130807095404.GA54382@ocelot.phlegethon.org> <20130815163722.GD60836@ocelot.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130815163722.GD60836@ocelot.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: "xen-devel@lists.xensource.com" , "keir.xen@gmail.com" List-Id: xen-devel@lists.xenproject.org On 15/08/13 17:37, Tim Deegan wrote: > At 16:51 +0100 on 15 Aug (1376585469), George Dunlap wrote: >> On Wed, Aug 7, 2013 at 10:54 AM, Tim Deegan wrote: >>> At 17:37 -0700 on 06 Aug (1375810631), Mukesh Rathor wrote: >>>> On Fri, 26 Jul 2013 11:45:19 +0100 >>>> Tim Deegan wrote: >>>> >>>>> At 19:30 -0700 on 25 Jul (1374780657), Mukesh Rathor wrote: >>>>>> On Thu, 25 Jul 2013 17:28:40 +0100 >>>>>> Tim Deegan wrote: >>>>>> >>>>>>> At 18:59 -0700 on 23 Jul (1374605971), Mukesh Rathor wrote: >>>>>>>> +/* Just like HVM, PVH should be using "cpuid" from the kernel >>>>>>>> mode. */ +static int vmxit_invalid_op(struct cpu_user_regs >>>>>>>> *regs) +{ >>>>>>>> + if ( guest_kernel_mode(current, regs) >>>>>>>> || !emulate_forced_invalid_op(regs) ) >>>>>>>> + hvm_inject_hw_exception(TRAP_invalid_op, >>>>>>>> HVM_DELIVER_NO_ERROR_CODE); >>>>>>> Was this discussed before? It seems harsh to stop kernel-mode >>>>>>> code from using the pv cpuid operation if it wants to. In >>>>>>> particular, what about loadable kernel modules? >>>>>> Yes, few times on the xen mailing list. The only PVH guest, linux >>>>>> as of now, the pv ops got rewired to use native cpuid, which is >>>>>> how hvm does it. >>>>> Yes, but presumably you want to make it easy for other PV guests to >>>>> port to PVH too? >>>> True, but how would not allowing kernel mode emulation impede that? >>>> I fail to understand why a new kernel would wanna use xen signature >>>> emulation over just plain cpuid instruction? >>> I'm talking about existing PV kernel code that already uses PV CPUID. >>> And in particular what if that kernel code is in a device driver, or >>> even a third-party loadable module? Porting the core kernel from PV to >>> PVH shouldn't break that code if it doesn't have to. >>> >>> But TBH my objection is really more aesthetic than anything else. >>> Restricting the PV CPUID instruction here adds another ragged edge in >>> the ABI that all kernel writers have to think about, and for little or >>> no benfit. And, to be clear, I object to it and this patch does not >>> have my Ack. >> Are you in particular saying that you think PVH guests should be >> allowed to use the PV CPUID as a prerequisite for the patch series to >> go in? >> >> Correct me if I'm wrong, but: >> 1. CPUID is the only forced_emulated op at the moment >> 2. The kernel is the only caller we can reasonably expect to use the >> forced_emulated ops > No - the forced-invalid-op CPUID is used from userspace too, e.g. in > tools/misc/xen-detect.c. The current patch keeps support for userspace > but not for kernel users. I would prefer to keep it for both, on the > grounds that even in the kernel there may be users of it that the person > porting the kernel does not control (e.g. in third-party modules). Ah, right. That makes sense then -- we'd want to have the same toolstack binaries for PV and PVH. But then if we're going to have it, I agree that there's no reason not to just go ahead and make it available in all modes. We can just deprecate it in kernel mode. -George