Re: [Xen-devel] [PATCH] x86/xen: support priv-mapping in an HVM tools domain

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 10/20/2017 04:35 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
>> Boris Ostrovsky
>> Sent: 19 October 2017 18:45
>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; x86@xxxxxxxxxx; xen-
>> devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> Cc: Juergen Gross <jgross@xxxxxxxx>; Thomas Gleixner
>> <tglx@xxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>; H. Peter Anvin
>> <hpa@xxxxxxxxx>
>> 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 <paul.durrant@xxxxxxxxxx>
>>> ---
>>> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>>> Cc: Juergen Gross <jgross@xxxxxxxx>
>>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>>> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>>> ---
>>>  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
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel


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