[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.