On 10/20/2017 04:35 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:[email protected]] On Behalf Of
>> Boris Ostrovsky
>> Sent: 19 October 2017 18:45
>> To: Paul Durrant <[email protected]>; [email protected]; xen-
>> [email protected]; [email protected]
>> Cc: Juergen Gross <[email protected]>; Thomas Gleixner
>> <[email protected]>; Ingo Molnar <[email protected]>; H. Peter Anvin
>> <[email protected]>
>> Subject: Re: [Xen-devel] [PATCH] x86/xen: support priv-mapping in an HVM
>> tools domain
>>
>> On 10/19/2017 11:26 AM, Paul Durrant wrote:
>>> If the domain has XENFEAT_auto_translated_physmap then use of the PV-
>>> specific HYPERVISOR_mmu_update hypercall is clearly incorrect.
>>>
>>> This patch adds checks in xen_remap_domain_gfn_array() and
>>> xen_unmap_domain_gfn_array() which call through to the approprate
>>> xlate_mmu function if the feature is present. A check is also added
>>> to xen_remap_domain_gfn_range() to fail with -EOPNOTSUPP since this
>>> should not be used in an HVM tools domain.
>>>
>>> Signed-off-by: Paul Durrant <[email protected]>
>>> ---
>>> Cc: Boris Ostrovsky <[email protected]>
>>> Cc: Juergen Gross <[email protected]>
>>> Cc: Thomas Gleixner <[email protected]>
>>> Cc: Ingo Molnar <[email protected]>
>>> Cc: "H. Peter Anvin" <[email protected]>
>>> ---
>>>  arch/x86/xen/mmu.c | 14 ++++++++++++--
>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
>>> index 3e15345abfe7..d33e7dbe3129 100644
>>> --- a/arch/x86/xen/mmu.c
>>> +++ b/arch/x86/xen/mmu.c
>>> @@ -172,6 +172,9 @@ int xen_remap_domain_gfn_range(struct
>> vm_area_struct *vma,
>>>                            pgprot_t prot, unsigned domid,
>>>                            struct page **pages)
>>>  {
>>> +   if (xen_feature(XENFEAT_auto_translated_physmap))
>>> +           return -EOPNOTSUPP;
>>> +
>> This is never called on XENFEAT_auto_translated_physmap domains, there
>> is a check in privcmd_ioctl_mmap() for that.
> Yes, that's true but it seems like the wrong place for such a check. I could 
> remove that one it you'd prefer.

I actually think that perhaps we could wrap privcmd_ioctl_mmap() with
"#ifdef CONFIG_XEN_PV" (#else return -ENOSYS) and move
xen_remap_domain_gfn_range() to mmu_pv.c. We can then remove it from ARM
code too.

>
>>>     return do_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid,
>> pages);
>>>  }
>>>  EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
>>> @@ -182,6 +185,10 @@ int xen_remap_domain_gfn_array(struct
>> vm_area_struct *vma,
>>>                            int *err_ptr, pgprot_t prot,
>>>                            unsigned domid, struct page **pages)
>>>  {
>>> +   if (xen_feature(XENFEAT_auto_translated_physmap))
>>> +           return xen_xlate_remap_gfn_array(vma, addr, gfn, nr,
>> err_ptr,
>>> +                                            prot, domid, pages);
>>> +
>> So how did this work before? In fact, I don't see any callers of
>> xen_xlate_{re|un}map_gfn_range().
> I assume mean 'array' for the map since there is no 
> xen_xlate_remap_gfn_range() function. I'm not quite sure what you're asking? 
> Without this patch the mmu code in an x86 domain simply assumes the domain is 
> PV... the xlate code is currently only used via the arm mmu code (where it 
> clearly knows it's not PV). AFAICS this Is just a straightforward buggy 
> assumption in the x86 code.

Looks like this was originally intended for dom0 PVH and was removed by
063334f. So it should indeed be restored.


-boris

>
>   Paul
>
>> -boris
>>
>>
>>>     /* We BUG_ON because it's a programmer error to pass a NULL
>> err_ptr,
>>>      * and the consequences later is quite hard to detect what the actual
>>>      * cause of "wrong memory was mapped in".
>>> @@ -193,9 +200,12 @@
>> EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_array);
>>>  /* Returns: 0 success */
>>>  int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
>>> -                          int numpgs, struct page **pages)
>>> +                          int nr, struct page **pages)
>>>  {
>>> -   if (!pages || !xen_feature(XENFEAT_auto_translated_physmap))
>>> +   if (xen_feature(XENFEAT_auto_translated_physmap))
>>> +           return xen_xlate_unmap_gfn_range(vma, nr, pages);
>>> +
>>> +   if (!pages)
>>>             return 0;
>>>
>>>     return -EINVAL;
>>
>> _______________________________________________
>> Xen-devel mailing list
>> [email protected]
>> https://lists.xen.org/xen-devel

Reply via email to