On Thu, Jun 25, 2015 at 12:48 PM, Ed White <edmund.h.wh...@intel.com> wrote:
> On 06/25/2015 06:40 AM, Razvan Cojocaru wrote: > > On 06/25/2015 03:44 PM, Lengyel, Tamas wrote: > >> On Wed, Jun 24, 2015 at 2:06 PM, Ed White <edmund.h.wh...@intel.com > >> <mailto:edmund.h.wh...@intel.com>> wrote: > >> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote: > >> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx, > >> >> + unsigned long pfn, > xenmem_access_t > >> >> access) > >> >> +{ > >> >> > >> > > >> > This function IMHO should be merged with p2m_set_mem_access and > should be > >> > triggerable with the same memop (XENMEM_access_op) hypercall > instead of > >> > introducing a new hvmop one. > >> > >> I think we should vote on this. My view is that it makes > >> XENMEM_access_op > >> too complicated to use. > >> > >> The two functions are not very long and share enough code that it would > >> justify merging. The only big change added is the copy from host->alt > >> when the entry doesn't exists in alt, and that itself is pretty self > >> contained. Let's see if we can get a third opinion on it.. > > > > At first sight (I admit I'm rather late in the game and haven't had a > > chance to follow the series closely from the beginning), the two > > functions do seem to be mergeable (or at least the common code factored > > out in static helper functions). > > > > Also, if Ed's concern is that the libxc API would look unnatural if > > xc_set_mem_access() is used for both purposes, as far as I can tell the > > only difference could be a non-zero last altp2m parameter, so I agree > > with you that the less functions doing almost the same thing the better > > (I have been guilty of this in the past too, for example with my > > xc_enable_introspection() function ;) ). > > > > So I'd say, yes, if possible merge them. > > So here are my reasons why I don't think we should merge the hypercalls, > in more detail: > > Although the two hypercalls are similar, they are not identical. For one > thing, the existing hypercall can only be used cross-domain whereas the > altp2m one can be used cross-domain or intra-domain. Fair point, the use of rcu_lock_live_remote_domain_by_id in the memaccess memop handler precludes it working for the intra-domain case. However, now that we have a valid use-case for it working when a domain applies restrictions on itself, it would be fine to change that to rcu_lock_domain_by_any_id. It has been just used as a sanity check. The code you are using in hvm.c could be abstracted as p2m_altp2m_sanity_check: "!is_hvm_domain(d) || !hvm_altp2m_supported() || !d->arch.altp2m_active" and ran when the altp2m field is non-zero to catch buggy tools. > Also, the existing > hypercall can be used to modify a range of pages and the new one can only > modify a single page, and that is intentional. > Please elaborate on this. > > As I see it, the implementation in hvm.c would become a lot less clean, > and every direct user of the existing hypercall would have to change for > no good reason. > For 4.6 I reworked the entire vm_event/mem_access system, so that is already happening irrespective of altp2m. It's fine to add support for the altp2m field before 4.6 freezes. > Razvan's suggestion to merge the functions that implement the p2m changes > I'm more ambivalent about. Personally, I prefer not to have code that > contains lots of conditional logic, which would be the result, but I > don't feel that strongly about it. > > Ed > Thanks, Tamas
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
