On Thu, Jun 25, 2015 at 4:46 PM, Ed White <edmund.h.wh...@intel.com> wrote:
> On 06/25/2015 11:23 AM, Lengyel, Tamas wrote: > > 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. > > Whether or not it's possible to merge the two isn't in dispute. The > question is which path results in the easiest to understand and > maintain outcome for users of the hypercalls and maintainers of > the implementation. If it turns out to be that merging the two is too big of a hassle, I would agree with Razvan, some code-deduplication would be fine instead of a complete merger. I still think it would be cleaner. > Having said that, I don't think your check > catches an attempt to place an intra-domain restriction on the > host p2m with altp2m active. > The check above I copied from the existing code you do your hvm op. Do you explicitly check for that conditions somewhere else? Why not append it you need to restrict that condition? > > > > >> 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. > > In order to keep the p2m's coherent and respect the primacy of the host > p2m, changes that occur in the host p2m can cause changes in altp2m's to > be lost. At the moment there is not even any notification that has > occurred, although that's something I'm working on. The minimum > *guaranteed* granularity of that type of altp2m invalidation is > an entire altp2m. The more pages you change in an altp2m, the more > chance there is of a collision causing an invalidation, so for this > version of altp2m we encourage as few changes as possible by requiring > a separate hypercall for each page modification. > > Ed > OK, but we could check for the condition where npages>1 and an altp2m is specified to return -EOPNOTSUPP. It could be documented in the exposed part of the API that with altp2m this is a restriction. Tamas
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
