On Thu, Jun 25, 2015 at 4:46 PM, Ed White <edmund.h.white@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.white@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.white@intel.com
> >>>> <mailto:edmund.h.white@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