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

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

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

As I see it, the implementation in hvm.c would become a lot less clean, and 
every direct user of the existing hypercall would have to change for no good 
reason.

Razvan's suggestion to merge the functions that implement the p2m changes I'm 
more ambivalent about. Personally, I prefer not to have code that contains lots 
of conditional logic, which would be the result, but I don't feel that strongly 
about it.

Ed

Ravi> This also has implications for the XSM hooks used for these hypercalls - 
altp2m default policy is to allow for intra-domain , which is not the case for 
XENMEM_access_op - 
Any thoughts on how to manage this difference if we merge them?

Ravi


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel