Re: [PATCHv6 2/3] xen: unify foreign GFN map/unmap for auto-xlated physmap guests

[David Vrabel <david.vrabel@citrix.com>]

On 06/03/15 17:51, Stefano Stabellini wrote:
> On Fri, 6 Mar 2015, David Vrabel wrote:
>> Auto-translated physmap guests (arm, arm64 and x86 PVHVM/PVH) map and
>> unmap foreign GFNs using the same method (updating the physmap).
>> Unify the two arm and x86 implementations into one commont one.
>>
>> Note that on arm and arm64, the correct error code will be returned
>> (instead of always -EFAULT) and map/unmap failure warnings are no
>> longer printed.  These changes are required if the foreign domain is
>> paging (-ENOENT failures are expected and must be propagated up to the
>> caller).
>>
[...]
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -253,4 +253,10 @@ config XEN_EFI
>>  	def_bool y
>>  	depends on X86_64 && EFI
>>  
>> +config XEN_AUTO_XLATE
>> +	def_bool y
>> +	depends on ARM || ARM64 || XEN_PVHVM
>> +	help
>> +	  Support for auto-translated physmap guests.
>> +
>>  endmenu
> 
> I think the dependency chain is inverted: in order to enable XEN on ARM
> and ARM64 or to enable XEN_PVHVM, one needs XEN_AUTO_XLATE.
> I think that config XEN should select XEN_AUTO_XLATE on ARM and ARM64
> and config XEN_PVHVM should select XEN_AUTO_XLATE on x86.

This is a non-visible option that is automatically enabled if required,
so I'm not sure what you mean here.

>> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>> +			      int nr, struct page **pages)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < nr; i++) {
>> +		struct xen_remove_from_physmap xrp;
>> +		unsigned long pfn;
>> +
>> +		pfn = page_to_pfn(pages[i]);
>> +
>> +		xrp.domid = DOMID_SELF;
>> +		xrp.gpfn = pfn;
>> +		(void)HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> 
> Why drop the warning we had before?

Because map failures (and hence) unmap failures are expected and would
result in log spam (particularly with an x86 HVM guest).

>> --- a/include/xen/xen-ops.h
>> +++ b/include/xen/xen-ops.h
>> @@ -34,6 +34,16 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>>  			       struct page **pages);
>>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
>>  			       int numpgs, struct page **pages);
>> +#ifdef CONFIG_XEN_AUTO_XLATE
>> +int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
>> +			      unsigned long addr,
>> +			      xen_pfn_t gfn, int nr,
>> +			      pgprot_t prot,
>> +			      unsigned domid, 
>> +			      struct page **pages);
>> +int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
>> +			      int nr, struct page **pages);
>> +#endif
> 
> I don't think we actually need the #ifdef in the header file?

It serves as useful documentation if nothing else.

David