[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.
On 06/25/2015 03:45 PM, Lengyel, Tamas wrote: > On Thu, Jun 25, 2015 at 4:46 PM, Ed White <edmund.h.white@xxxxxxxxx> wrote: > >> On 06/25/2015 11:23 AM, Lengyel, Tamas wrote: >>> On Thu, Jun 25, 2015 at 12:48 PM, Ed White <edmund.h.white@xxxxxxxxx> >> 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.white@xxxxxxxxx >>>>>> <mailto:edmund.h.white@xxxxxxxxx>> 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? > The existing altp2m HVM op can't operate on the host p2m, so I don't need a check, which I think reinforces the point I'm trying to make: the code in hvm.c will get spaghetti-like if we go down this route. >> >>> >>>> 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. > See above. Ed _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |