Xen project Mailing List

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?

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.