[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [V1 PATCH 3/3] pvh: disallow PHYSDEVOP_pirq_eoi_gmfn_v2/v1
On 02/28/2014 08:09 AM, Jan Beulich wrote: On 28.02.14 at 04:06, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |