[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
|
