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

[Ed White <edmund.h.white@intel.com>]

On 06/25/2015 03:45 PM, Lengyel, Tamas wrote:
> 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?
> 

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