From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [V1 PATCH 3/3] pvh: disallow PHYSDEVOP_pirq_eoi_gmfn_v2/v1 Date: Mon, 3 Mar 2014 12:30:38 +0000 Message-ID: <531475EE.8070309@eu.citrix.com> References: <1393290237-28427-1-git-send-email-mukesh.rathor@oracle.com> <1393290237-28427-2-git-send-email-mukesh.rathor@oracle.com> <1393290237-28427-3-git-send-email-mukesh.rathor@oracle.com> <1393290237-28427-4-git-send-email-mukesh.rathor@oracle.com> <530C7362020000780011F0F0@nat28.tlf.novell.com> <530C7663020000780011F104@nat28.tlf.novell.com> <20140225152514.GA4322@konrad-lan.dumpdata.com> <530CD069020000780011F3D1@nat28.tlf.novell.com> <20140225123314.334b7fec@mantra.us.oracle.com> <530DC4A6020000780011F6C0@nat28.tlf.novell.com> <20140227190649.47d4eb7a@mantra.us.oracle.com> <5310524802000078001201B8@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5310524802000078001201B8@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Mukesh Rathor Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 02/28/2014 08:09 AM, Jan Beulich wrote: >>>> On 28.02.14 at 04:06, Mukesh Rathor wrote: >> So, sigh, in conclusion, what would you prefer: >> >> a. add this new check to hvm_physdev_op. >> b. leave the new check here. >> c. add this check to hvm_physdev_op, and move the existing pvh checks >> from do_physdev_op to hvm_physdev_op also. > > From a formal pov, all PVH special casing > should go away from hvm_physdev_op _and_ do_physdev_op, > with the possibly exception of things that are really unsuitable > for PVH (in which case it would be more clean to have a distinct > pvh_physdev_op). Temporary workarounds like the one here > should obviously(!?) go where other temporary workarounds > are - with what we have, that's hvm_physdev_op(). As I think > Tim said elsewhere - sprinkling around arbitrary PVH checks is > just not acceptable. I begin to regret that I gave my okay for > all these fixme-s and stuff to go in (with it yet to be seen when > their elimination would start), because I'm getting the feeling > that you take this as an excuse to add further such stuff. > > So to answer your question above: The way to go depends on > the long term intended behavior: If there is anything that will > need special casing no matter what, option d) is likely going to > be the cleanest one - introduce pvh_physdev_op(). But each > exception there either needs a proper rationale, or a proper > fixme annotation. And such special casing should be done with > future code changes in mind, i.e. considering the cost of > maintaining all of {do,hvm,pvh}_physdev_op() when new > (sub-)ops get added. [Moving back to the main list] So is the main reason that the checks in do_physdev_op() exist just that some of the commands have not yet been implemented for PVH? If so, I'd much rather have the checks in do_physdev_op(). I think long-term, I'd prefer to have them in do_physdev_op() (as Mukesh has done here) even if they're going to stay, rather than having an extra function in another file with a whitelist / blacklist. Yes, having the checks in do_physdev_op() is a bit ugly, but it should make it clear in one place which paths are valid for which kinds of guests. -George