[PATCH 0/3] today's XSAs assorted 4.12 candidate follow-up

[PATCH 0/3] today's XSAs assorted 4.12 candidate follow-up

[Jan Beulich]

At least patch 1 is a clear candidate for 4.12; the others are less clear,
but I wanted to put them on the table nevertheless. The patches are
grouped together just because of the XSA relationship; they don't
depend on one another.

1: x86/mm: fix #GP(0) in switch_cr3_cr4()
2: IOMMU/x86: make page type checks consistent when mapping pages
3: memory: restrict XENMEM_remove_from_physmap to translated guests

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] x86/mm: fix #GP(0) in switch_cr3_cr4()

[Jan Beulich]

With "pcid=no-xpti" and opposite XPTI settings in two 64-bit PV domains
(achievable with one of "xpti=no-dom0" or "xpti=no-domu"), switching
from a PCID-disabled to a PCID-enabled 64-bit PV domain fails to set
CR4.PCIDE in time, as CR4.PGE would not be set in either (see
pv_guest_cr4_to_real_cr4(), in particular as used by write_ptbase()),
and hence the early CR4 write would be skipped.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -122,6 +122,7 @@ void switch_cr3_cr4(unsigned long cr3, u
         write_cr4(old_cr4);
     }
     else if ( use_invpcid )
+    {
         /*
          * Flushing the TLB via INVPCID is necessary only in case PCIDs are
          * in use, which is true only with INVPCID being available.
@@ -132,6 +133,19 @@ void switch_cr3_cr4(unsigned long cr3, u
          */
         invpcid_flush_all_nonglobals();
 
+        /*
+         * CR4.PCIDE needs to be set before the CR3 write below. Otherwise
+         * - the CR3 write will fault when CR3.NOFLUSH is set (which is the
+         *   case normally),
+         * - the subsequent CR4 write will fault if CR3.PCID != 0.
+         */
+        if ( (old_cr4 & X86_CR4_PCIDE) < (cr4 & X86_CR4_PCIDE) )
+        {
+            write_cr4(cr4);
+            old_cr4 = cr4;
+        }
+    }
+
     /*
      * If we don't change PCIDs, the CR3 write below needs to flush this very
      * PCID, even when a full flush was performed above, as we are currently





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich]

There are currently three more or less different checks:
- _get_page_type() adjusts the IOMMU mappings according to the new type
  alone,
- arch_iommu_populate_page_table() wants just the type to be
  PGT_writable_page,
- iommu_hwdom_init() additionally permits all other types with a type
  refcount of zero.
The canonical one is in _get_page_type(), and as of XSA-288
arch_iommu_populate_page_table() also has no need anymore to deal with
PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still
necessary to consider, since in that case pages don't get handed to
guest_physmap_add_entry(). Furthermore, the function so far also
established r/o mappings, which is not in line with the rules set forth
by the XSA-288 change.

For arch_iommu_populate_page_table() to not encounter PGT_none pages
anymore even in cases where the IOMMU gets enabled for a domain only
when it is already running, replace the IOMMU dependency in
guest_physmap_add_entry()'s handling of PV guests to check just the
system wide state instead of the domain property.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -837,11 +837,11 @@ guest_physmap_add_entry(struct domain *d
          *
          * Retain this property by grabbing a writable type ref and then
          * dropping it immediately.  The result will be pages that have a
-         * writable type (and an IOMMU entry), but a count of 0 (such that
-         * any guest-requested type changes succeed and remove the IOMMU
-         * entry).
+         * writable type (and an IOMMU entry if necessary), but a count of 0
+         * (such that any guest-requested type changes succeed and remove the
+         * IOMMU entry).
          */
-        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
+        if ( !iommu_enabled || t != p2m_ram_rw )
             return 0;
 
         for ( i = 0; i < (1UL << page_order); ++i, ++page )
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
 
         page_list_for_each ( page, &d->page_list )
         {
-            unsigned long mfn = mfn_x(page_to_mfn(page));
-            unsigned long dfn = mfn_to_gmfn(d, mfn);
-            unsigned int mapping = IOMMUF_readable;
-            int ret;
+            if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none )
+            {
+                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
+                if ( get_page_and_type(page, d, PGT_writable_page) )
+                    put_page_and_type(page);
+                else if ( !rc )
+                    rc = -EBUSY;
+            }
 
-            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
-                 ((page->u.inuse.type_info & PGT_type_mask)
-                  == PGT_writable_page) )
-                mapping |= IOMMUF_writable;
+            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
+                  PGT_writable_page) )
+            {
+                unsigned long mfn = mfn_x(page_to_mfn(page));
+                unsigned long dfn = mfn_to_gmfn(d, mfn);
+                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
+                                    IOMMUF_readable | IOMMUF_writable,
+                                    &flush_flags);
 
-            ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
-                            &flush_flags);
-
-            if ( !rc )
-                rc = ret;
+                if ( !rc )
+                    rc = ret;
+            }
 
             if ( !(i++ & 0xfffff) )
                 process_pending_softirqs();





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] x86/mm: fix #GP(0) in switch_cr3_cr4()

[Juergen Gross]

On 05/03/2019 14:25, Jan Beulich wrote:
> With "pcid=no-xpti" and opposite XPTI settings in two 64-bit PV domains
> (achievable with one of "xpti=no-dom0" or "xpti=no-domu"), switching
> from a PCID-disabled to a PCID-enabled 64-bit PV domain fails to set
> CR4.PCIDE in time, as CR4.PGE would not be set in either (see
> pv_guest_cr4_to_real_cr4(), in particular as used by write_ptbase()),
> and hence the early CR4 write would be skipped.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
originally introduced it (d818f3cb7c ["hvm: Use main memory for video
memory"]) and the one then purging it again (78c3097e4f ["Remove unused
XENMEM_remove_from_physmap"]) make clear that this operation is intended
for use on HVM (i.e. translated) guests only. Restrict it at least as
much, because for PV guests documentation (in the public header) does
not even match the implementation: It talks about GPFN as input, but
get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
back the value passed in).

Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
directly into top level hypercall handling, and clarify things in the
public header accordingly.

Take the liberty and also replace a pointless use of "current" with a
more efficient use of an existing local variable (or function parameter
to be precise).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: It could be further restricted, disallowing its use by a HVM guest
     on itself.
TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
     pointlessly populating a PoD slot just to unpopulate it again right
     away, with the page then free floating, i.e. no longer available
     for use to replace another PoD slot, and (afaict) no longer
     accessible by the guest in any way.
TBD: Is using guest_physmap_remove_page() here really appropriate? It
     means that e.g. MMIO pages wouldn't be removed. Going through
     guest_remove_page() (while skipping the de-allocation step) would
     seem more appropriate to me, which would address the P2M_ALLOC
     aspect above as well.
     
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
     mfn_t mfn = INVALID_MFN;
     p2m_type_t p2mt;
 
-    if ( !paging_mode_translate(d) )
-        return -EACCES;
-
     switch ( space )
     {
         case XENMAPSPACE_shared_info:
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
     long rc = 0;
     union xen_add_to_physmap_batch_extra extra;
 
+    ASSERT(paging_mode_translate(d));
+
     if ( xatp->space != XENMAPSPACE_gmfn_foreign )
         extra.res0 = 0;
     else
@@ -997,12 +999,15 @@ static int get_reserved_device_memory(xe
 
 static long xatp_permission_check(struct domain *d, unsigned int space)
 {
+    if ( !paging_mode_translate(d) )
+        return -EACCES;
+
     /*
      * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
      * to map this kind of space to itself.
      */
     if ( (space == XENMAPSPACE_dev_mmio) &&
-         (!is_hardware_domain(current->domain) || (d != current->domain)) )
+         (!is_hardware_domain(d) || (d != current->domain)) )
         return -EACCES;
 
     return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
@@ -1386,7 +1391,9 @@ long do_memory_op(unsigned long cmd, XEN
         if ( d == NULL )
             return -ESRCH;
 
-        rc = xsm_remove_from_physmap(XSM_TARGET, curr_d, d);
+        rc = paging_mode_translate(d)
+             ? xsm_remove_from_physmap(XSM_TARGET, curr_d, d)
+             : -EACCES;
         if ( rc )
         {
             rcu_unlock_domain(d);
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -231,7 +231,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_map
 
 /*
  * Sets the GPFN at which a particular page appears in the specified guest's
- * pseudophysical address space.
+ * physical address space (translated guests only).
  * arg == addr of xen_add_to_physmap_t.
  */
 #define XENMEM_add_to_physmap      7
@@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
 
 /*
  * Unmaps the page appearing at a particular GPFN from the specified guest's
- * pseudophysical address space.
+ * physical address space (translated guests only).
  * arg == addr of xen_remove_from_physmap_t.
  */
 #define XENMEM_remove_from_physmap      15




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[Juergen Gross]

On 05/03/2019 14:26, Jan Beulich wrote:
> There are currently three more or less different checks:
> - _get_page_type() adjusts the IOMMU mappings according to the new type
>   alone,
> - arch_iommu_populate_page_table() wants just the type to be
>   PGT_writable_page,
> - iommu_hwdom_init() additionally permits all other types with a type
>   refcount of zero.
> The canonical one is in _get_page_type(), and as of XSA-288
> arch_iommu_populate_page_table() also has no need anymore to deal with
> PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still
> necessary to consider, since in that case pages don't get handed to
> guest_physmap_add_entry(). Furthermore, the function so far also
> established r/o mappings, which is not in line with the rules set forth
> by the XSA-288 change.
> 
> For arch_iommu_populate_page_table() to not encounter PGT_none pages
> anymore even in cases where the IOMMU gets enabled for a domain only
> when it is already running, replace the IOMMU dependency in
> guest_physmap_add_entry()'s handling of PV guests to check just the
> system wide state instead of the domain property.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm unable to decide whether this patch should make it into 4.12 or not
with the given information. What happens without this patch (worst
case)?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Juergen Gross]

On 05/03/2019 14:28, Jan Beulich wrote:
> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
> XENMEM_remove_from_physmap"]) make clear that this operation is intended
> for use on HVM (i.e. translated) guests only. Restrict it at least as
> much, because for PV guests documentation (in the public header) does
> not even match the implementation: It talks about GPFN as input, but
> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
> back the value passed in).
> 
> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
> directly into top level hypercall handling, and clarify things in the
> public header accordingly.
> 
> Take the liberty and also replace a pointless use of "current" with a
> more efficient use of an existing local variable (or function parameter
> to be precise).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Do I read this correctly: nothing bad for the host can happen without
this patch? If this is correct I'd like to defer this patch until 4.13.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] x86/mm: fix #GP(0) in switch_cr3_cr4()

[Andrew Cooper]

On 05/03/2019 13:25, Jan Beulich wrote:
> With "pcid=no-xpti" and opposite XPTI settings in two 64-bit PV domains
> (achievable with one of "xpti=no-dom0" or "xpti=no-domu"), switching
> from a PCID-disabled to a PCID-enabled 64-bit PV domain fails to set
> CR4.PCIDE in time, as CR4.PGE would not be set in either (see
> pv_guest_cr4_to_real_cr4(), in particular as used by write_ptbase()),

This is stale with XSA-293 in place, and is now pv_fixup_guest_cr4()

> and hence the early CR4 write would be skipped.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Also +1 for fixing in 4.12

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich]

>>> On 05.03.19 at 14:50, <jgross@suse.com> wrote:
> On 05/03/2019 14:26, Jan Beulich wrote:
>> There are currently three more or less different checks:
>> - _get_page_type() adjusts the IOMMU mappings according to the new type
>>   alone,
>> - arch_iommu_populate_page_table() wants just the type to be
>>   PGT_writable_page,
>> - iommu_hwdom_init() additionally permits all other types with a type
>>   refcount of zero.
>> The canonical one is in _get_page_type(), and as of XSA-288
>> arch_iommu_populate_page_table() also has no need anymore to deal with
>> PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still
>> necessary to consider, since in that case pages don't get handed to
>> guest_physmap_add_entry(). Furthermore, the function so far also
>> established r/o mappings, which is not in line with the rules set forth
>> by the XSA-288 change.
>> 
>> For arch_iommu_populate_page_table() to not encounter PGT_none pages
>> anymore even in cases where the IOMMU gets enabled for a domain only
>> when it is already running, replace the IOMMU dependency in
>> guest_physmap_add_entry()'s handling of PV guests to check just the
>> system wide state instead of the domain property.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm unable to decide whether this patch should make it into 4.12 or not
> with the given information. What happens without this patch (worst
> case)?

I don't think anything really bad can happen, or else this would have
been part of one of the XSAs. The patch is bringing things in line with
what XSA-288 did, without it being obvious what bad could result
from not doing so. The larger part of change here is for hwdom only
anyway.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 05.03.19 at 14:53, <jgross@suse.com> wrote:
> On 05/03/2019 14:28, Jan Beulich wrote:
>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>> much, because for PV guests documentation (in the public header) does
>> not even match the implementation: It talks about GPFN as input, but
>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>> back the value passed in).
>> 
>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>> directly into top level hypercall handling, and clarify things in the
>> public header accordingly.
>> 
>> Take the liberty and also replace a pointless use of "current" with a
>> more efficient use of an existing local variable (or function parameter
>> to be precise).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Do I read this correctly: nothing bad for the host can happen without
> this patch?

If you add "It is believed that ...", then yes.

> If this is correct I'd like to defer this patch until 4.13.

Okay.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi Jan,

Sorry I was meant to answer to this earlier on.

On 05/03/2019 13:28, Jan Beulich wrote:
> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
> XENMEM_remove_from_physmap"]) make clear that this operation is intended
> for use on HVM (i.e. translated) guests only. Restrict it at least as
> much, because for PV guests documentation (in the public header) does
> not even match the implementation: It talks about GPFN as input, but
> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
> back the value passed in).
> 
> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
> directly into top level hypercall handling, and clarify things in the
> public header accordingly.
> 
> Take the liberty and also replace a pointless use of "current" with a
> more efficient use of an existing local variable (or function parameter
> to be precise).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: It could be further restricted, disallowing its use by a HVM guest
>       on itself.

By HVM guest, do you refer to any auto-translated guest?

The interface XENME_remove_from_physmap is used by Arm to remove foreign 
mappings from its p2m. There are potentially other space with similar case (e.g 
grant-table...).

> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>       pointlessly populating a PoD slot just to unpopulate it again right
>       away, with the page then free floating, i.e. no longer available
>       for use to replace another PoD slot, and (afaict) no longer
>       accessible by the guest in any way.
> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>       means that e.g. MMIO pages wouldn't be removed. Going through
>       guest_remove_page() (while skipping the de-allocation step) would
>       seem more appropriate to me, which would address the P2M_ALLOC
>       aspect above as well.

How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO pages?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote:
> On 05/03/2019 13:28, Jan Beulich wrote:
>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>> much, because for PV guests documentation (in the public header) does
>> not even match the implementation: It talks about GPFN as input, but
>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>> back the value passed in).
>> 
>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>> directly into top level hypercall handling, and clarify things in the
>> public header accordingly.
>> 
>> Take the liberty and also replace a pointless use of "current" with a
>> more efficient use of an existing local variable (or function parameter
>> to be precise).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: It could be further restricted, disallowing its use by a HVM guest
>>       on itself.
> 
> By HVM guest, do you refer to any auto-translated guest?

Yes - sorry for using an x86 term.

> The interface XENME_remove_from_physmap is used by Arm to remove foreign 
> mappings from its p2m. There are potentially other space with similar case 
> (e.g grant-table...).

Oh, I see - this option goes away then.

>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>       pointlessly populating a PoD slot just to unpopulate it again right
>>       away, with the page then free floating, i.e. no longer available
>>       for use to replace another PoD slot, and (afaict) no longer
>>       accessible by the guest in any way.
>> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>>       means that e.g. MMIO pages wouldn't be removed. Going through
>>       guest_remove_page() (while skipping the de-allocation step) would
>>       seem more appropriate to me, which would address the P2M_ALLOC
>>       aspect above as well.
> 
> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO 
> pages?

Well, there's XENMAPSPACE_dev_mmio which xatp handles. But
perhaps the MMIO example is more confusing than helpful. The
question really just is whether guest_remove_page() shouldn't
be used here instead of guest_physmap_remove_page().

But of course - first of all I'd like to get acks (or feedback what to
change) on the actual patch here. The further points would all, if
anything, result in independent patches.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi Jan,

On 4/2/19 5:10 PM, Jan Beulich wrote:
>>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote:
>> On 05/03/2019 13:28, Jan Beulich wrote:
>>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>>> much, because for PV guests documentation (in the public header) does
>>> not even match the implementation: It talks about GPFN as input, but
>>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>>> back the value passed in).
>>>
>>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>>> directly into top level hypercall handling, and clarify things in the
>>> public header accordingly.
>>>
>>> Take the liberty and also replace a pointless use of "current" with a
>>> more efficient use of an existing local variable (or function parameter
>>> to be precise).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> TBD: It could be further restricted, disallowing its use by a HVM guest
>>>        on itself.
>>
>> By HVM guest, do you refer to any auto-translated guest?
> 
> Yes - sorry for using an x86 term.
> 
>> The interface XENME_remove_from_physmap is used by Arm to remove foreign
>> mappings from its p2m. There are potentially other space with similar case
>> (e.g grant-table...).
> 
> Oh, I see - this option goes away then.
> 
>>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>>        pointlessly populating a PoD slot just to unpopulate it again right
>>>        away, with the page then free floating, i.e. no longer available
>>>        for use to replace another PoD slot, and (afaict) no longer
>>>        accessible by the guest in any way.
>>> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>>>        means that e.g. MMIO pages wouldn't be removed. Going through
>>>        guest_remove_page() (while skipping the de-allocation step) would
>>>        seem more appropriate to me, which would address the P2M_ALLOC
>>>        aspect above as well.
>>
>> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO
>> pages?
> 
> Well, there's XENMAPSPACE_dev_mmio which xatp handles. But
> perhaps the MMIO example is more confusing than helpful. The
> question really just is whether guest_remove_page() shouldn't
> be used here instead of guest_physmap_remove_page()
de-allocation step aside, I am not really convinced you can reuse 
guest_remove_page() here. On x86, the function will not work on certain 
p2m types. Is it what we really want?

> 
> But of course - first of all I'd like to get acks (or feedback what to
> change) on the actual patch here. The further points would all, if
> anything, result in independent patches.

Make sense. I will have a look at the patch.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi Jan,

On 4/2/19 5:10 PM, Jan Beulich wrote:
>>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote:
>> On 05/03/2019 13:28, Jan Beulich wrote:
>>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>>> much, because for PV guests documentation (in the public header) does
>>> not even match the implementation: It talks about GPFN as input, but
>>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>>> back the value passed in).
>>>
>>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>>> directly into top level hypercall handling, and clarify things in the
>>> public header accordingly.
>>>
>>> Take the liberty and also replace a pointless use of "current" with a
>>> more efficient use of an existing local variable (or function parameter
>>> to be precise).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> TBD: It could be further restricted, disallowing its use by a HVM guest
>>>        on itself.
>>
>> By HVM guest, do you refer to any auto-translated guest?
> 
> Yes - sorry for using an x86 term.
> 
>> The interface XENME_remove_from_physmap is used by Arm to remove foreign
>> mappings from its p2m. There are potentially other space with similar case
>> (e.g grant-table...).
> 
> Oh, I see - this option goes away then.
> 
>>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>>        pointlessly populating a PoD slot just to unpopulate it again right
>>>        away, with the page then free floating, i.e. no longer available
>>>        for use to replace another PoD slot, and (afaict) no longer
>>>        accessible by the guest in any way.
>>> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>>>        means that e.g. MMIO pages wouldn't be removed. Going through
>>>        guest_remove_page() (while skipping the de-allocation step) would
>>>        seem more appropriate to me, which would address the P2M_ALLOC
>>>        aspect above as well.
>>
>> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO
>> pages?
> 
> Well, there's XENMAPSPACE_dev_mmio which xatp handles. But
> perhaps the MMIO example is more confusing than helpful. The
> question really just is whether guest_remove_page() shouldn't
> be used here instead of guest_physmap_remove_page()
de-allocation step aside, I am not really convinced you can reuse 
guest_remove_page() here. On x86, the function will not work on certain 
p2m types. Is it what we really want?

> 
> But of course - first of all I'd like to get acks (or feedback what to
> change) on the actual patch here. The further points would all, if
> anything, result in independent patches.

Make sense. I will have a look at the patch.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi Jan,

On 3/5/19 1:28 PM, Jan Beulich wrote:
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>   
>   /*
>    * Unmaps the page appearing at a particular GPFN from the specified guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).

While you modify the comment, can you replace GPFN with GFN?

Other than that:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheer,s

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi Jan,

On 3/5/19 1:28 PM, Jan Beulich wrote:
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>   
>   /*
>    * Unmaps the page appearing at a particular GPFN from the specified guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).

While you modify the comment, can you replace GPFN with GFN?

Other than that:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheer,s

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
> On 3/5/19 1:28 PM, Jan Beulich wrote:
>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>   
>>   /*
>>    * Unmaps the page appearing at a particular GPFN from the specified guest's
>> - * pseudophysical address space.
>> + * physical address space (translated guests only).
> 
> While you modify the comment, can you replace GPFN with GFN?

I did consider this when writing the patch, but then this would
bring it out of sync with the structure field and its associated
comment. Plus the "add" side comment would then want
changing too. And public/memory.h has quite a few more uses
of GPFN / gpfn.

To be honest I'd prefer to leave this as is right here, for it to get
cleaned up in one go.

> Other than that:
> 
> Reviewed-by: Julien Grall <julien.grall@arm.com>

Thanks, but as per above I'm not sure whether to actually apply
it.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
> On 3/5/19 1:28 PM, Jan Beulich wrote:
>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>   
>>   /*
>>    * Unmaps the page appearing at a particular GPFN from the specified guest's
>> - * pseudophysical address space.
>> + * physical address space (translated guests only).
> 
> While you modify the comment, can you replace GPFN with GFN?

I did consider this when writing the patch, but then this would
bring it out of sync with the structure field and its associated
comment. Plus the "add" side comment would then want
changing too. And public/memory.h has quite a few more uses
of GPFN / gpfn.

To be honest I'd prefer to leave this as is right here, for it to get
cleaned up in one go.

> Other than that:
> 
> Reviewed-by: Julien Grall <julien.grall@arm.com>

Thanks, but as per above I'm not sure whether to actually apply
it.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
> On 4/2/19 5:10 PM, Jan Beulich wrote:
>>>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote:
>>> On 05/03/2019 13:28, Jan Beulich wrote:
>>>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>>>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>>>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>>>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>>>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>>>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>>>> much, because for PV guests documentation (in the public header) does
>>>> not even match the implementation: It talks about GPFN as input, but
>>>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>>>> back the value passed in).
>>>>
>>>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>>>> directly into top level hypercall handling, and clarify things in the
>>>> public header accordingly.
>>>>
>>>> Take the liberty and also replace a pointless use of "current" with a
>>>> more efficient use of an existing local variable (or function parameter
>>>> to be precise).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> TBD: It could be further restricted, disallowing its use by a HVM guest
>>>>        on itself.
>>>
>>> By HVM guest, do you refer to any auto-translated guest?
>> 
>> Yes - sorry for using an x86 term.
>> 
>>> The interface XENME_remove_from_physmap is used by Arm to remove foreign
>>> mappings from its p2m. There are potentially other space with similar case
>>> (e.g grant-table...).
>> 
>> Oh, I see - this option goes away then.
>> 
>>>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>>>        pointlessly populating a PoD slot just to unpopulate it again right
>>>>        away, with the page then free floating, i.e. no longer available
>>>>        for use to replace another PoD slot, and (afaict) no longer
>>>>        accessible by the guest in any way.
>>>> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>>>>        means that e.g. MMIO pages wouldn't be removed. Going through
>>>>        guest_remove_page() (while skipping the de-allocation step) would
>>>>        seem more appropriate to me, which would address the P2M_ALLOC
>>>>        aspect above as well.
>>>
>>> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO
>>> pages?
>> 
>> Well, there's XENMAPSPACE_dev_mmio which xatp handles. But
>> perhaps the MMIO example is more confusing than helpful. The
>> question really just is whether guest_remove_page() shouldn't
>> be used here instead of guest_physmap_remove_page()
> de-allocation step aside, I am not really convinced you can reuse 
> guest_remove_page() here. On x86, the function will not work on certain 
> p2m types. Is it what we really want?

Hmm, I'm confused. Afaics the only two types it refuses a request
for are p2m_invalid and p2m_mmio_dm. These represent cases
where there's no p2m entry anyway, i.e. nothing to remove. Am
I perhaps overlooking something you see?

Or are you referring to the mfn_invalid() check (which isn't x86-
specific)? This ought to be covered by the p2m_is_paging() and
p2m_mmio_direct special cases a few lines up from there. Other
cases with invalid MFNs would indeed represent an error condition
imo.

In the end it's actually quite the opposite that I'm thinking: For
the caller it shouldn't, for example, matter whether the
requested page was paged out. We wouldn't even call
guest_physmap_remove_page() in that case, while
guest_remove_page() would take care of it.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
> On 4/2/19 5:10 PM, Jan Beulich wrote:
>>>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote:
>>> On 05/03/2019 13:28, Jan Beulich wrote:
>>>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>>>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>>>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>>>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>>>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>>>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>>>> much, because for PV guests documentation (in the public header) does
>>>> not even match the implementation: It talks about GPFN as input, but
>>>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>>>> back the value passed in).
>>>>
>>>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>>>> directly into top level hypercall handling, and clarify things in the
>>>> public header accordingly.
>>>>
>>>> Take the liberty and also replace a pointless use of "current" with a
>>>> more efficient use of an existing local variable (or function parameter
>>>> to be precise).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> TBD: It could be further restricted, disallowing its use by a HVM guest
>>>>        on itself.
>>>
>>> By HVM guest, do you refer to any auto-translated guest?
>> 
>> Yes - sorry for using an x86 term.
>> 
>>> The interface XENME_remove_from_physmap is used by Arm to remove foreign
>>> mappings from its p2m. There are potentially other space with similar case
>>> (e.g grant-table...).
>> 
>> Oh, I see - this option goes away then.
>> 
>>>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>>>        pointlessly populating a PoD slot just to unpopulate it again right
>>>>        away, with the page then free floating, i.e. no longer available
>>>>        for use to replace another PoD slot, and (afaict) no longer
>>>>        accessible by the guest in any way.
>>>> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>>>>        means that e.g. MMIO pages wouldn't be removed. Going through
>>>>        guest_remove_page() (while skipping the de-allocation step) would
>>>>        seem more appropriate to me, which would address the P2M_ALLOC
>>>>        aspect above as well.
>>>
>>> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO
>>> pages?
>> 
>> Well, there's XENMAPSPACE_dev_mmio which xatp handles. But
>> perhaps the MMIO example is more confusing than helpful. The
>> question really just is whether guest_remove_page() shouldn't
>> be used here instead of guest_physmap_remove_page()
> de-allocation step aside, I am not really convinced you can reuse 
> guest_remove_page() here. On x86, the function will not work on certain 
> p2m types. Is it what we really want?

Hmm, I'm confused. Afaics the only two types it refuses a request
for are p2m_invalid and p2m_mmio_dm. These represent cases
where there's no p2m entry anyway, i.e. nothing to remove. Am
I perhaps overlooking something you see?

Or are you referring to the mfn_invalid() check (which isn't x86-
specific)? This ought to be covered by the p2m_is_paging() and
p2m_mmio_direct special cases a few lines up from there. Other
cases with invalid MFNs would indeed represent an error condition
imo.

In the end it's actually quite the opposite that I'm thinking: For
the caller it shouldn't, for example, matter whether the
requested page was paged out. We wouldn't even call
guest_physmap_remove_page() in that case, while
guest_remove_page() would take care of it.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi Jan,

On 4/8/19 3:02 PM, Jan Beulich wrote:
>>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
>> On 3/5/19 1:28 PM, Jan Beulich wrote:
>>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>>    
>>>    /*
>>>     * Unmaps the page appearing at a particular GPFN from the specified guest's
>>> - * pseudophysical address space.
>>> + * physical address space (translated guests only).
>>
>> While you modify the comment, can you replace GPFN with GFN?
> 
> I did consider this when writing the patch, but then this would
> bring it out of sync with the structure field and its associated
> comment. Plus the "add" side comment would then want
> changing too. And public/memory.h has quite a few more uses
> of GPFN / gpfn.
> 
> To be honest I'd prefer to leave this as is right here, for it to get
> cleaned up in one go.

I am fine with that.

> 
>> Other than that:
>>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> Thanks, but as per above I'm not sure whether to actually apply > it.

Yes it applies, for the common code (and indirectly Arm).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi Jan,

On 4/8/19 3:02 PM, Jan Beulich wrote:
>>>> On 08.04.19 at 13:58, <julien.grall@arm.com> wrote:
>> On 3/5/19 1:28 PM, Jan Beulich wrote:
>>> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>>>    
>>>    /*
>>>     * Unmaps the page appearing at a particular GPFN from the specified guest's
>>> - * pseudophysical address space.
>>> + * physical address space (translated guests only).
>>
>> While you modify the comment, can you replace GPFN with GFN?
> 
> I did consider this when writing the patch, but then this would
> bring it out of sync with the structure field and its associated
> comment. Plus the "add" side comment would then want
> changing too. And public/memory.h has quite a few more uses
> of GPFN / gpfn.
> 
> To be honest I'd prefer to leave this as is right here, for it to get
> cleaned up in one go.

I am fine with that.

> 
>> Other than that:
>>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> Thanks, but as per above I'm not sure whether to actually apply > it.

Yes it applies, for the common code (and indirectly Arm).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi,

On 08/04/2019 15:29, Jan Beulich wrote:
>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>> On 4/2/19 5:10 PM, Jan Beulich wrote:
>>>>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote:
>>>> On 05/03/2019 13:28, Jan Beulich wrote:
>>>>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>>>>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>>>>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>>>>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>>>>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>>>>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>>>>> much, because for PV guests documentation (in the public header) does
>>>>> not even match the implementation: It talks about GPFN as input, but
>>>>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>>>>> back the value passed in).
>>>>>
>>>>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>>>>> directly into top level hypercall handling, and clarify things in the
>>>>> public header accordingly.
>>>>>
>>>>> Take the liberty and also replace a pointless use of "current" with a
>>>>> more efficient use of an existing local variable (or function parameter
>>>>> to be precise).
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> TBD: It could be further restricted, disallowing its use by a HVM guest
>>>>>         on itself.
>>>>
>>>> By HVM guest, do you refer to any auto-translated guest?
>>>
>>> Yes - sorry for using an x86 term.
>>>
>>>> The interface XENME_remove_from_physmap is used by Arm to remove foreign
>>>> mappings from its p2m. There are potentially other space with similar case
>>>> (e.g grant-table...).
>>>
>>> Oh, I see - this option goes away then.
>>>
>>>>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>>>>         pointlessly populating a PoD slot just to unpopulate it again right
>>>>>         away, with the page then free floating, i.e. no longer available
>>>>>         for use to replace another PoD slot, and (afaict) no longer
>>>>>         accessible by the guest in any way.
>>>>> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>>>>>         means that e.g. MMIO pages wouldn't be removed. Going through
>>>>>         guest_remove_page() (while skipping the de-allocation step) would
>>>>>         seem more appropriate to me, which would address the P2M_ALLOC
>>>>>         aspect above as well.
>>>>
>>>> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO
>>>> pages?
>>>
>>> Well, there's XENMAPSPACE_dev_mmio which xatp handles. But
>>> perhaps the MMIO example is more confusing than helpful. The
>>> question really just is whether guest_remove_page() shouldn't
>>> be used here instead of guest_physmap_remove_page()
>> de-allocation step aside, I am not really convinced you can reuse
>> guest_remove_page() here. On x86, the function will not work on certain
>> p2m types. Is it what we really want?
> 
> Hmm, I'm confused. Afaics the only two types it refuses a request
> for are p2m_invalid and p2m_mmio_dm. These represent cases
> where there's no p2m entry anyway, i.e. nothing to remove. Am
> I perhaps overlooking something you see?
> 
> Or are you referring to the mfn_invalid() check (which isn't x86-
> specific)? This ought to be covered by the p2m_is_paging() and
> p2m_mmio_direct special cases a few lines up from there. Other
> cases with invalid MFNs would indeed represent an error condition
> imo.

I am referring to get_page(...) which would not work for foreign pages.

> 
> In the end it's actually quite the opposite that I'm thinking: For
> the caller it shouldn't, for example, matter whether the
> requested page was paged out. We wouldn't even call
> guest_physmap_remove_page() in that case, while
> guest_remove_page() would take care of it.

But those pages should never be removed via physmap, I am correct?

Anyway, guest_physmap_remove_page() would need some rework to do the correct 
things for physmap. I will wait an see your patch to comment.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi,

On 08/04/2019 15:29, Jan Beulich wrote:
>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>> On 4/2/19 5:10 PM, Jan Beulich wrote:
>>>>>> On 02.04.19 at 12:26, <julien.grall@arm.com> wrote:
>>>> On 05/03/2019 13:28, Jan Beulich wrote:
>>>>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>>>>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>>>>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>>>>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>>>>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>>>>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>>>>> much, because for PV guests documentation (in the public header) does
>>>>> not even match the implementation: It talks about GPFN as input, but
>>>>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>>>>> back the value passed in).
>>>>>
>>>>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>>>>> directly into top level hypercall handling, and clarify things in the
>>>>> public header accordingly.
>>>>>
>>>>> Take the liberty and also replace a pointless use of "current" with a
>>>>> more efficient use of an existing local variable (or function parameter
>>>>> to be precise).
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> TBD: It could be further restricted, disallowing its use by a HVM guest
>>>>>         on itself.
>>>>
>>>> By HVM guest, do you refer to any auto-translated guest?
>>>
>>> Yes - sorry for using an x86 term.
>>>
>>>> The interface XENME_remove_from_physmap is used by Arm to remove foreign
>>>> mappings from its p2m. There are potentially other space with similar case
>>>> (e.g grant-table...).
>>>
>>> Oh, I see - this option goes away then.
>>>
>>>>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>>>>         pointlessly populating a PoD slot just to unpopulate it again right
>>>>>         away, with the page then free floating, i.e. no longer available
>>>>>         for use to replace another PoD slot, and (afaict) no longer
>>>>>         accessible by the guest in any way.
>>>>> TBD: Is using guest_physmap_remove_page() here really appropriate? It
>>>>>         means that e.g. MMIO pages wouldn't be removed. Going through
>>>>>         guest_remove_page() (while skipping the de-allocation step) would
>>>>>         seem more appropriate to me, which would address the P2M_ALLOC
>>>>>         aspect above as well.
>>>>
>>>> How is that an issue? Does XENMEM_add_to_physmap allows you to map MMIO
>>>> pages?
>>>
>>> Well, there's XENMAPSPACE_dev_mmio which xatp handles. But
>>> perhaps the MMIO example is more confusing than helpful. The
>>> question really just is whether guest_remove_page() shouldn't
>>> be used here instead of guest_physmap_remove_page()
>> de-allocation step aside, I am not really convinced you can reuse
>> guest_remove_page() here. On x86, the function will not work on certain
>> p2m types. Is it what we really want?
> 
> Hmm, I'm confused. Afaics the only two types it refuses a request
> for are p2m_invalid and p2m_mmio_dm. These represent cases
> where there's no p2m entry anyway, i.e. nothing to remove. Am
> I perhaps overlooking something you see?
> 
> Or are you referring to the mfn_invalid() check (which isn't x86-
> specific)? This ought to be covered by the p2m_is_paging() and
> p2m_mmio_direct special cases a few lines up from there. Other
> cases with invalid MFNs would indeed represent an error condition
> imo.

I am referring to get_page(...) which would not work for foreign pages.

> 
> In the end it's actually quite the opposite that I'm thinking: For
> the caller it shouldn't, for example, matter whether the
> requested page was paged out. We wouldn't even call
> guest_physmap_remove_page() in that case, while
> guest_remove_page() would take care of it.

But those pages should never be removed via physmap, I am correct?

Anyway, guest_physmap_remove_page() would need some rework to do the correct 
things for physmap. I will wait an see your patch to comment.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>> de-allocation step aside, I am not really convinced you can reuse
>>> guest_remove_page() here. On x86, the function will not work on certain
>>> p2m types. Is it what we really want?
>> 
>> Hmm, I'm confused. Afaics the only two types it refuses a request
>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>> I perhaps overlooking something you see?
>> 
>> Or are you referring to the mfn_invalid() check (which isn't x86-
>> specific)? This ought to be covered by the p2m_is_paging() and
>> p2m_mmio_direct special cases a few lines up from there. Other
>> cases with invalid MFNs would indeed represent an error condition
>> imo.
> 
> I am referring to get_page(...) which would not work for foreign pages.

Ah, that's part of the de-allocation portion of the code, which I've
previously indicated would need to be skipped in the case here.

>> In the end it's actually quite the opposite that I'm thinking: For
>> the caller it shouldn't, for example, matter whether the
>> requested page was paged out. We wouldn't even call
>> guest_physmap_remove_page() in that case, while
>> guest_remove_page() would take care of it.
> 
> But those pages should never be removed via physmap, I am correct?

The guest is unaware of paging, so as long as it's permitted to
use this hypercall, it should make no difference to it whether a
page is actually in memory at the time it issues this hypercall.

> Anyway, guest_physmap_remove_page() would need some rework to do the correct 
> things for physmap.

Of course.

> I will wait an see your patch to comment.

Well, the reason for the TBD item in the patch here was precisely
to figure out whether creating such a patch would make sense,
or whether the current use of guest_physmap_remove_page() is
correct.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>> de-allocation step aside, I am not really convinced you can reuse
>>> guest_remove_page() here. On x86, the function will not work on certain
>>> p2m types. Is it what we really want?
>> 
>> Hmm, I'm confused. Afaics the only two types it refuses a request
>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>> I perhaps overlooking something you see?
>> 
>> Or are you referring to the mfn_invalid() check (which isn't x86-
>> specific)? This ought to be covered by the p2m_is_paging() and
>> p2m_mmio_direct special cases a few lines up from there. Other
>> cases with invalid MFNs would indeed represent an error condition
>> imo.
> 
> I am referring to get_page(...) which would not work for foreign pages.

Ah, that's part of the de-allocation portion of the code, which I've
previously indicated would need to be skipped in the case here.

>> In the end it's actually quite the opposite that I'm thinking: For
>> the caller it shouldn't, for example, matter whether the
>> requested page was paged out. We wouldn't even call
>> guest_physmap_remove_page() in that case, while
>> guest_remove_page() would take care of it.
> 
> But those pages should never be removed via physmap, I am correct?

The guest is unaware of paging, so as long as it's permitted to
use this hypercall, it should make no difference to it whether a
page is actually in memory at the time it issues this hypercall.

> Anyway, guest_physmap_remove_page() would need some rework to do the correct 
> things for physmap.

Of course.

> I will wait an see your patch to comment.

Well, the reason for the TBD item in the patch here was precisely
to figure out whether creating such a patch would make sense,
or whether the current use of guest_physmap_remove_page() is
correct.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi,

On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>> de-allocation step aside, I am not really convinced you can reuse
>>>> guest_remove_page() here. On x86, the function will not work on certain
>>>> p2m types. Is it what we really want?
>>>
>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>> I perhaps overlooking something you see?
>>>
>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>> specific)? This ought to be covered by the p2m_is_paging() and
>>> p2m_mmio_direct special cases a few lines up from there. Other
>>> cases with invalid MFNs would indeed represent an error condition
>>> imo.
>>
>> I am referring to get_page(...) which would not work for foreign pages.
> 
> Ah, that's part of the de-allocation portion of the code, which I've
> previously indicated would need to be skipped in the case here.
> 
>>> In the end it's actually quite the opposite that I'm thinking: For
>>> the caller it shouldn't, for example, matter whether the
>>> requested page was paged out. We wouldn't even call
>>> guest_physmap_remove_page() in that case, while
>>> guest_remove_page() would take care of it.
>>
>> But those pages should never be removed via physmap, I am correct?
> 
> The guest is unaware of paging, so as long as it's permitted to
> use this hypercall, it should make no difference to it whether a
> page is actually in memory at the time it issues this hypercall.

Well, I only agree with that statement if it is possible to map page 
that can be page out with one of the physmap hypercall.

If it is not possible, then I don't think we should allow the guest to 
remove those pages.

One of my main concern is a guest trying to mistakenly use 
XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your 
point above, the former would not do de-allocation. So you would end up 
losing the page forever (there are no way to get the page back for 
auto-translated guest).

I realize the issue is already present (at least on Arm) today. But if 
we were going to modify XENME_remove_from_physmap, then a bit more 
safety check to avoid a guest shooting its own foot would be useful.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Julien Grall]

Hi,

On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>> de-allocation step aside, I am not really convinced you can reuse
>>>> guest_remove_page() here. On x86, the function will not work on certain
>>>> p2m types. Is it what we really want?
>>>
>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>> I perhaps overlooking something you see?
>>>
>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>> specific)? This ought to be covered by the p2m_is_paging() and
>>> p2m_mmio_direct special cases a few lines up from there. Other
>>> cases with invalid MFNs would indeed represent an error condition
>>> imo.
>>
>> I am referring to get_page(...) which would not work for foreign pages.
> 
> Ah, that's part of the de-allocation portion of the code, which I've
> previously indicated would need to be skipped in the case here.
> 
>>> In the end it's actually quite the opposite that I'm thinking: For
>>> the caller it shouldn't, for example, matter whether the
>>> requested page was paged out. We wouldn't even call
>>> guest_physmap_remove_page() in that case, while
>>> guest_remove_page() would take care of it.
>>
>> But those pages should never be removed via physmap, I am correct?
> 
> The guest is unaware of paging, so as long as it's permitted to
> use this hypercall, it should make no difference to it whether a
> page is actually in memory at the time it issues this hypercall.

Well, I only agree with that statement if it is possible to map page 
that can be page out with one of the physmap hypercall.

If it is not possible, then I don't think we should allow the guest to 
remove those pages.

One of my main concern is a guest trying to mistakenly use 
XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your 
point above, the former would not do de-allocation. So you would end up 
losing the page forever (there are no way to get the page back for 
auto-translated guest).

I realize the issue is already present (at least on Arm) today. But if 
we were going to modify XENME_remove_from_physmap, then a bit more 
safety check to avoid a guest shooting its own foot would be useful.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 14.04.19 at 18:33, <julien.grall@arm.com> wrote:
> Hi,
> 
> On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>>> de-allocation step aside, I am not really convinced you can reuse
>>>>> guest_remove_page() here. On x86, the function will not work on certain
>>>>> p2m types. Is it what we really want?
>>>>
>>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>>> I perhaps overlooking something you see?
>>>>
>>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>>> specific)? This ought to be covered by the p2m_is_paging() and
>>>> p2m_mmio_direct special cases a few lines up from there. Other
>>>> cases with invalid MFNs would indeed represent an error condition
>>>> imo.
>>>
>>> I am referring to get_page(...) which would not work for foreign pages.
>> 
>> Ah, that's part of the de-allocation portion of the code, which I've
>> previously indicated would need to be skipped in the case here.
>> 
>>>> In the end it's actually quite the opposite that I'm thinking: For
>>>> the caller it shouldn't, for example, matter whether the
>>>> requested page was paged out. We wouldn't even call
>>>> guest_physmap_remove_page() in that case, while
>>>> guest_remove_page() would take care of it.
>>>
>>> But those pages should never be removed via physmap, I am correct?
>> 
>> The guest is unaware of paging, so as long as it's permitted to
>> use this hypercall, it should make no difference to it whether a
>> page is actually in memory at the time it issues this hypercall.
> 
> Well, I only agree with that statement if it is possible to map page 
> that can be page out with one of the physmap hypercall.
> 
> If it is not possible, then I don't think we should allow the guest to 
> remove those pages.

From the looks of it XENMAPSPACE_gmfn mapping would simply
fail for paged-out pages. Hence on one hand I agree with you
that "add" and "remove" should act similarly. Otoh though I
think we'd widen a problem here, because to me it looks like
passing a GFN of a paged out page to add-to-physmap should
work (and transparently to the guest).

> One of my main concern is a guest trying to mistakenly use 
> XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your 
> point above, the former would not do de-allocation. So you would end up 
> losing the page forever (there are no way to get the page back for 
> auto-translated guest).

Correct - to me that's implied from the sub-function name.
Just like add-to-physmap doesn't allocate anything, remove-
from-physmap doesn't free.

> I realize the issue is already present (at least on Arm) today. But if 
> we were going to modify XENME_remove_from_physmap, then a bit more 
> safety check to avoid a guest shooting its own foot would be useful.

I'm not sure I see ways of properly checking for such
situations - right after any such check the information gathered
might already be stale. And I don't think we go to great lengths
to prevent guests shooting themselves in the foot by other
means.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 14.04.19 at 18:33, <julien.grall@arm.com> wrote:
> Hi,
> 
> On 4/9/19 1:21 PM, Jan Beulich wrote:
>>>>> On 09.04.19 at 11:50, <julien.grall@arm.com> wrote:
>>> On 08/04/2019 15:29, Jan Beulich wrote:
>>>>>>> On 08.04.19 at 13:47, <julien.grall@arm.com> wrote:
>>>>> de-allocation step aside, I am not really convinced you can reuse
>>>>> guest_remove_page() here. On x86, the function will not work on certain
>>>>> p2m types. Is it what we really want?
>>>>
>>>> Hmm, I'm confused. Afaics the only two types it refuses a request
>>>> for are p2m_invalid and p2m_mmio_dm. These represent cases
>>>> where there's no p2m entry anyway, i.e. nothing to remove. Am
>>>> I perhaps overlooking something you see?
>>>>
>>>> Or are you referring to the mfn_invalid() check (which isn't x86-
>>>> specific)? This ought to be covered by the p2m_is_paging() and
>>>> p2m_mmio_direct special cases a few lines up from there. Other
>>>> cases with invalid MFNs would indeed represent an error condition
>>>> imo.
>>>
>>> I am referring to get_page(...) which would not work for foreign pages.
>> 
>> Ah, that's part of the de-allocation portion of the code, which I've
>> previously indicated would need to be skipped in the case here.
>> 
>>>> In the end it's actually quite the opposite that I'm thinking: For
>>>> the caller it shouldn't, for example, matter whether the
>>>> requested page was paged out. We wouldn't even call
>>>> guest_physmap_remove_page() in that case, while
>>>> guest_remove_page() would take care of it.
>>>
>>> But those pages should never be removed via physmap, I am correct?
>> 
>> The guest is unaware of paging, so as long as it's permitted to
>> use this hypercall, it should make no difference to it whether a
>> page is actually in memory at the time it issues this hypercall.
> 
> Well, I only agree with that statement if it is possible to map page 
> that can be page out with one of the physmap hypercall.
> 
> If it is not possible, then I don't think we should allow the guest to 
> remove those pages.

From the looks of it XENMAPSPACE_gmfn mapping would simply
fail for paged-out pages. Hence on one hand I agree with you
that "add" and "remove" should act similarly. Otoh though I
think we'd widen a problem here, because to me it looks like
passing a GFN of a paged out page to add-to-physmap should
work (and transparently to the guest).

> One of my main concern is a guest trying to mistakenly use 
> XENMEM_remove_from_physmap rather XENMEM_decrease_reservation. IIUC your 
> point above, the former would not do de-allocation. So you would end up 
> losing the page forever (there are no way to get the page back for 
> auto-translated guest).

Correct - to me that's implied from the sub-function name.
Just like add-to-physmap doesn't allocate anything, remove-
from-physmap doesn't free.

> I realize the issue is already present (at least on Arm) today. But if 
> we were going to modify XENME_remove_from_physmap, then a bit more 
> safety check to avoid a guest shooting its own foot would be useful.

I'm not sure I see ways of properly checking for such
situations - right after any such check the information gathered
might already be stale. And I don't think we go to great lengths
to prevent guests shooting themselves in the foot by other
means.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 05.03.19 at 14:28,  wrote:
> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
> XENMEM_remove_from_physmap"]) make clear that this operation is intended
> for use on HVM (i.e. translated) guests only. Restrict it at least as
> much, because for PV guests documentation (in the public header) does
> not even match the implementation: It talks about GPFN as input, but
> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
> back the value passed in).
> 
> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
> directly into top level hypercall handling, and clarify things in the
> public header accordingly.
> 
> Take the liberty and also replace a pointless use of "current" with a
> more efficient use of an existing local variable (or function parameter
> to be precise).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Andrew, Goerge - any chance of getting an ack for the pretty simple
x86-specific code adjustment?

Jan

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>      mfn_t mfn = INVALID_MFN;
>      p2m_type_t p2mt;
>  
> -    if ( !paging_mode_translate(d) )
> -        return -EACCES;
> -
>      switch ( space )
>      {
>          case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>      long rc = 0;
>      union xen_add_to_physmap_batch_extra extra;
>  
> +    ASSERT(paging_mode_translate(d));
> +
>      if ( xatp->space != XENMAPSPACE_gmfn_foreign )
>          extra.res0 = 0;
>      else
> @@ -997,12 +999,15 @@ static int get_reserved_device_memory(xe
>  
>  static long xatp_permission_check(struct domain *d, unsigned int space)
>  {
> +    if ( !paging_mode_translate(d) )
> +        return -EACCES;
> +
>      /*
>       * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
>       * to map this kind of space to itself.
>       */
>      if ( (space == XENMAPSPACE_dev_mmio) &&
> -         (!is_hardware_domain(current->domain) || (d != current->domain)) )
> +         (!is_hardware_domain(d) || (d != current->domain)) )
>          return -EACCES;
>  
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> @@ -1386,7 +1391,9 @@ long do_memory_op(unsigned long cmd, XEN
>          if ( d == NULL )
>              return -ESRCH;
>  
> -        rc = xsm_remove_from_physmap(XSM_TARGET, curr_d, d);
> +        rc = paging_mode_translate(d)
> +             ? xsm_remove_from_physmap(XSM_TARGET, curr_d, d)
> +             : -EACCES;
>          if ( rc )
>          {
>              rcu_unlock_domain(d);
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -231,7 +231,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_map
>  
>  /*
>   * Sets the GPFN at which a particular page appears in the specified 
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
>   * arg == addr of xen_add_to_physmap_t.
>   */
>  #define XENMEM_add_to_physmap      7
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>  
>  /*
>   * Unmaps the page appearing at a particular GPFN from the specified 
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
>   * arg == addr of xen_remove_from_physmap_t.
>   */
>  #define XENMEM_remove_from_physmap      15
> 
> 
> 
> 





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] Ping: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 05.03.19 at 14:28,  wrote:
> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
> XENMEM_remove_from_physmap"]) make clear that this operation is intended
> for use on HVM (i.e. translated) guests only. Restrict it at least as
> much, because for PV guests documentation (in the public header) does
> not even match the implementation: It talks about GPFN as input, but
> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
> back the value passed in).
> 
> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
> directly into top level hypercall handling, and clarify things in the
> public header accordingly.
> 
> Take the liberty and also replace a pointless use of "current" with a
> more efficient use of an existing local variable (or function parameter
> to be precise).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Andrew, Goerge - any chance of getting an ack for the pretty simple
x86-specific code adjustment?

Jan

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>      mfn_t mfn = INVALID_MFN;
>      p2m_type_t p2mt;
>  
> -    if ( !paging_mode_translate(d) )
> -        return -EACCES;
> -
>      switch ( space )
>      {
>          case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>      long rc = 0;
>      union xen_add_to_physmap_batch_extra extra;
>  
> +    ASSERT(paging_mode_translate(d));
> +
>      if ( xatp->space != XENMAPSPACE_gmfn_foreign )
>          extra.res0 = 0;
>      else
> @@ -997,12 +999,15 @@ static int get_reserved_device_memory(xe
>  
>  static long xatp_permission_check(struct domain *d, unsigned int space)
>  {
> +    if ( !paging_mode_translate(d) )
> +        return -EACCES;
> +
>      /*
>       * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain
>       * to map this kind of space to itself.
>       */
>      if ( (space == XENMAPSPACE_dev_mmio) &&
> -         (!is_hardware_domain(current->domain) || (d != current->domain)) )
> +         (!is_hardware_domain(d) || (d != current->domain)) )
>          return -EACCES;
>  
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> @@ -1386,7 +1391,9 @@ long do_memory_op(unsigned long cmd, XEN
>          if ( d == NULL )
>              return -ESRCH;
>  
> -        rc = xsm_remove_from_physmap(XSM_TARGET, curr_d, d);
> +        rc = paging_mode_translate(d)
> +             ? xsm_remove_from_physmap(XSM_TARGET, curr_d, d)
> +             : -EACCES;
>          if ( rc )
>          {
>              rcu_unlock_domain(d);
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -231,7 +231,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_map
>  
>  /*
>   * Sets the GPFN at which a particular page appears in the specified 
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
>   * arg == addr of xen_add_to_physmap_t.
>   */
>  #define XENMEM_add_to_physmap      7
> @@ -298,7 +298,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_add_to_physm
>  
>  /*
>   * Unmaps the page appearing at a particular GPFN from the specified 
> guest's
> - * pseudophysical address space.
> + * physical address space (translated guests only).
>   * arg == addr of xen_remove_from_physmap_t.
>   */
>  #define XENMEM_remove_from_physmap      15
> 
> 
> 
> 





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[George Dunlap]

On 3/5/19 1:26 PM, Jan Beulich wrote:
> There are currently three more or less different checks:
> - _get_page_type() adjusts the IOMMU mappings according to the new type
>   alone,
> - arch_iommu_populate_page_table() wants just the type to be
>   PGT_writable_page,
> - iommu_hwdom_init() additionally permits all other types with a type
>   refcount of zero.
> The canonical one is in _get_page_type(), and as of XSA-288
> arch_iommu_populate_page_table() also has no need anymore to deal with
> PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still
> necessary to consider, since in that case pages don't get handed to
> guest_physmap_add_entry(). Furthermore, the function so far also
> established r/o mappings, which is not in line with the rules set forth
> by the XSA-288 change.
> 
> For arch_iommu_populate_page_table() to not encounter PGT_none pages
> anymore even in cases where the IOMMU gets enabled for a domain only
> when it is already running, replace the IOMMU dependency in
> guest_physmap_add_entry()'s handling of PV guests to check just the
> system wide state instead of the domain property.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -837,11 +837,11 @@ guest_physmap_add_entry(struct domain *d
>           *
>           * Retain this property by grabbing a writable type ref and then
>           * dropping it immediately.  The result will be pages that have a
> -         * writable type (and an IOMMU entry), but a count of 0 (such that
> -         * any guest-requested type changes succeed and remove the IOMMU
> -         * entry).
> +         * writable type (and an IOMMU entry if necessary), but a count of 0
> +         * (such that any guest-requested type changes succeed and remove the
> +         * IOMMU entry).
>           */
> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
> +        if ( !iommu_enabled || t != p2m_ram_rw )
>              return 0;

This looks good.  One question about the next one...

>  
>          for ( i = 0; i < (1UL << page_order); ++i, ++page )
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> -            unsigned long mfn = mfn_x(page_to_mfn(page));
> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
> -            unsigned int mapping = IOMMUF_readable;
> -            int ret;
> +            if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none )
> +            {
> +                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
> +                if ( get_page_and_type(page, d, PGT_writable_page) )
> +                    put_page_and_type(page);
> +                else if ( !rc )
> +                    rc = -EBUSY;
> +            }
>  
> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
> -                 ((page->u.inuse.type_info & PGT_type_mask)
> -                  == PGT_writable_page) )
> -                mapping |= IOMMUF_writable;
> +            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
> +                  PGT_writable_page) )
> +            {
> +                unsigned long mfn = mfn_x(page_to_mfn(page));
> +                unsigned long dfn = mfn_to_gmfn(d, mfn);
> +                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
> +                                    IOMMUF_readable | IOMMUF_writable,
> +                                    &flush_flags);

What's the idea behind calling iommu_map() here, rather than relying on
the one in _get_page_type()?  Does need_iommu_pt_sync() not work yet at
this point, or do you expect there to be pages that have been marked
PGT_writable without having gone through _get_page_type()?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[George Dunlap]

On 3/5/19 1:26 PM, Jan Beulich wrote:
> There are currently three more or less different checks:
> - _get_page_type() adjusts the IOMMU mappings according to the new type
>   alone,
> - arch_iommu_populate_page_table() wants just the type to be
>   PGT_writable_page,
> - iommu_hwdom_init() additionally permits all other types with a type
>   refcount of zero.
> The canonical one is in _get_page_type(), and as of XSA-288
> arch_iommu_populate_page_table() also has no need anymore to deal with
> PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still
> necessary to consider, since in that case pages don't get handed to
> guest_physmap_add_entry(). Furthermore, the function so far also
> established r/o mappings, which is not in line with the rules set forth
> by the XSA-288 change.
> 
> For arch_iommu_populate_page_table() to not encounter PGT_none pages
> anymore even in cases where the IOMMU gets enabled for a domain only
> when it is already running, replace the IOMMU dependency in
> guest_physmap_add_entry()'s handling of PV guests to check just the
> system wide state instead of the domain property.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -837,11 +837,11 @@ guest_physmap_add_entry(struct domain *d
>           *
>           * Retain this property by grabbing a writable type ref and then
>           * dropping it immediately.  The result will be pages that have a
> -         * writable type (and an IOMMU entry), but a count of 0 (such that
> -         * any guest-requested type changes succeed and remove the IOMMU
> -         * entry).
> +         * writable type (and an IOMMU entry if necessary), but a count of 0
> +         * (such that any guest-requested type changes succeed and remove the
> +         * IOMMU entry).
>           */
> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
> +        if ( !iommu_enabled || t != p2m_ram_rw )
>              return 0;

This looks good.  One question about the next one...

>  
>          for ( i = 0; i < (1UL << page_order); ++i, ++page )
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> -            unsigned long mfn = mfn_x(page_to_mfn(page));
> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
> -            unsigned int mapping = IOMMUF_readable;
> -            int ret;
> +            if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none )
> +            {
> +                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
> +                if ( get_page_and_type(page, d, PGT_writable_page) )
> +                    put_page_and_type(page);
> +                else if ( !rc )
> +                    rc = -EBUSY;
> +            }
>  
> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
> -                 ((page->u.inuse.type_info & PGT_type_mask)
> -                  == PGT_writable_page) )
> -                mapping |= IOMMUF_writable;
> +            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
> +                  PGT_writable_page) )
> +            {
> +                unsigned long mfn = mfn_x(page_to_mfn(page));
> +                unsigned long dfn = mfn_to_gmfn(d, mfn);
> +                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
> +                                    IOMMUF_readable | IOMMUF_writable,
> +                                    &flush_flags);

What's the idea behind calling iommu_map() here, rather than relying on
the one in _get_page_type()?  Does need_iommu_pt_sync() not work yet at
this point, or do you expect there to be pages that have been marked
PGT_writable without having gone through _get_page_type()?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich]

>>> On 13.05.19 at 15:44, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:26 PM, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
>>  
>>          page_list_for_each ( page, &d->page_list )
>>          {
>> -            unsigned long mfn = mfn_x(page_to_mfn(page));
>> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
>> -            unsigned int mapping = IOMMUF_readable;
>> -            int ret;
>> +            if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none )
>> +            {
>> +                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
>> +                if ( get_page_and_type(page, d, PGT_writable_page) )
>> +                    put_page_and_type(page);
>> +                else if ( !rc )
>> +                    rc = -EBUSY;
>> +            }
>>  
>> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
>> -                 ((page->u.inuse.type_info & PGT_type_mask)
>> -                  == PGT_writable_page) )
>> -                mapping |= IOMMUF_writable;
>> +            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
>> +                  PGT_writable_page) )
>> +            {
>> +                unsigned long mfn = mfn_x(page_to_mfn(page));
>> +                unsigned long dfn = mfn_to_gmfn(d, mfn);
>> +                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
>> +                                    IOMMUF_readable | IOMMUF_writable,
>> +                                    &flush_flags);
> 
> What's the idea behind calling iommu_map() here, rather than relying on
> the one in _get_page_type()?  Does need_iommu_pt_sync() not work yet at
> this point, or do you expect there to be pages that have been marked
> PGT_writable without having gone through _get_page_type()?

No, I think I simply didn't realize that this could be deleted altogether
with the added get_page_and_type() invocation.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich]

>>> On 13.05.19 at 15:44, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:26 PM, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
>>  
>>          page_list_for_each ( page, &d->page_list )
>>          {
>> -            unsigned long mfn = mfn_x(page_to_mfn(page));
>> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
>> -            unsigned int mapping = IOMMUF_readable;
>> -            int ret;
>> +            if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none )
>> +            {
>> +                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
>> +                if ( get_page_and_type(page, d, PGT_writable_page) )
>> +                    put_page_and_type(page);
>> +                else if ( !rc )
>> +                    rc = -EBUSY;
>> +            }
>>  
>> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
>> -                 ((page->u.inuse.type_info & PGT_type_mask)
>> -                  == PGT_writable_page) )
>> -                mapping |= IOMMUF_writable;
>> +            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
>> +                  PGT_writable_page) )
>> +            {
>> +                unsigned long mfn = mfn_x(page_to_mfn(page));
>> +                unsigned long dfn = mfn_to_gmfn(d, mfn);
>> +                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
>> +                                    IOMMUF_readable | IOMMUF_writable,
>> +                                    &flush_flags);
> 
> What's the idea behind calling iommu_map() here, rather than relying on
> the one in _get_page_type()?  Does need_iommu_pt_sync() not work yet at
> this point, or do you expect there to be pages that have been marked
> PGT_writable without having gone through _get_page_type()?

No, I think I simply didn't realize that this could be deleted altogether
with the added get_page_and_type() invocation.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[George Dunlap]

On 3/5/19 1:28 PM, Jan Beulich wrote:
> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
> XENMEM_remove_from_physmap"]) make clear that this operation is intended
> for use on HVM (i.e. translated) guests only. Restrict it at least as
> much, because for PV guests documentation (in the public header) does
> not even match the implementation: It talks about GPFN as input, but
> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
> back the value passed in).
> 
> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
> directly into top level hypercall handling, and clarify things in the
> public header accordingly.
> 
> Take the liberty and also replace a pointless use of "current" with a
> more efficient use of an existing local variable (or function parameter
> to be precise).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good, sorry for the delay:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

A couple of comments:

> ---
> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>      pointlessly populating a PoD slot just to unpopulate it again right
>      away, with the page then free floating, i.e. no longer available
>      for use to replace another PoD slot, and (afaict) no longer
>      accessible by the guest in any way.

Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
immediately see any reason to do the allocation -- I think it just must
have been C&P without much thought given as to what was going to happen
next.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>      mfn_t mfn = INVALID_MFN;
>      p2m_type_t p2mt;
>  
> -    if ( !paging_mode_translate(d) )
> -        return -EACCES;
> -
>      switch ( space )
>      {
>          case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>      long rc = 0;
>      union xen_add_to_physmap_batch_extra extra;
>  
> +    ASSERT(paging_mode_translate(d));

So, just trying to think through the principles behind these two.  We
know that this is never going to be called w/o first calling
xatp_permission_check(); if we assume that such a change will be tested
(i.e., that something with paging_mode_translate() will call this
hypercall before a release), then a single ASSERT() should be enough to
make sure that both functions are updated properly?

I guess that's good enough.  (It's hard not to start to get paranoid
when you ask yourself these sorts of questions.)

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[George Dunlap]

On 3/5/19 1:28 PM, Jan Beulich wrote:
> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
> XENMEM_remove_from_physmap"]) make clear that this operation is intended
> for use on HVM (i.e. translated) guests only. Restrict it at least as
> much, because for PV guests documentation (in the public header) does
> not even match the implementation: It talks about GPFN as input, but
> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
> back the value passed in).
> 
> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
> directly into top level hypercall handling, and clarify things in the
> public header accordingly.
> 
> Take the liberty and also replace a pointless use of "current" with a
> more efficient use of an existing local variable (or function parameter
> to be precise).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks good, sorry for the delay:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

A couple of comments:

> ---
> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>      pointlessly populating a PoD slot just to unpopulate it again right
>      away, with the page then free floating, i.e. no longer available
>      for use to replace another PoD slot, and (afaict) no longer
>      accessible by the guest in any way.

Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
immediately see any reason to do the allocation -- I think it just must
have been C&P without much thought given as to what was going to happen
next.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>      mfn_t mfn = INVALID_MFN;
>      p2m_type_t p2mt;
>  
> -    if ( !paging_mode_translate(d) )
> -        return -EACCES;
> -
>      switch ( space )
>      {
>          case XENMAPSPACE_shared_info:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>      long rc = 0;
>      union xen_add_to_physmap_batch_extra extra;
>  
> +    ASSERT(paging_mode_translate(d));

So, just trying to think through the principles behind these two.  We
know that this is never going to be called w/o first calling
xatp_permission_check(); if we assume that such a change will be tested
(i.e., that something with paging_mode_translate() will call this
hypercall before a release), then a single ASSERT() should be enough to
make sure that both functions are updated properly?

I guess that's good enough.  (It's hard not to start to get paranoid
when you ask yourself these sorts of questions.)

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 13.05.19 at 16:35, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:28 PM, Jan Beulich wrote:
>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>> much, because for PV guests documentation (in the public header) does
>> not even match the implementation: It talks about GPFN as input, but
>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>> back the value passed in).
>> 
>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>> directly into top level hypercall handling, and clarify things in the
>> public header accordingly.
>> 
>> Take the liberty and also replace a pointless use of "current" with a
>> more efficient use of an existing local variable (or function parameter
>> to be precise).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Looks good, sorry for the delay:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks.

> A couple of comments:
> 
>> ---
>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>      pointlessly populating a PoD slot just to unpopulate it again right
>>      away, with the page then free floating, i.e. no longer available
>>      for use to replace another PoD slot, and (afaict) no longer
>>      accessible by the guest in any way.
> 
> Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
> immediately see any reason to do the allocation -- I think it just must
> have been C&P without much thought given as to what was going to happen
> next.

The question is: If we remove it, what is the -ENOENT condition
going to be?

>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>>      mfn_t mfn = INVALID_MFN;
>>      p2m_type_t p2mt;
>>  
>> -    if ( !paging_mode_translate(d) )
>> -        return -EACCES;
>> -
>>      switch ( space )
>>      {
>>          case XENMAPSPACE_shared_info:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>>      long rc = 0;
>>      union xen_add_to_physmap_batch_extra extra;
>>  
>> +    ASSERT(paging_mode_translate(d));
> 
> So, just trying to think through the principles behind these two.  We
> know that this is never going to be called w/o first calling
> xatp_permission_check(); if we assume that such a change will be tested
> (i.e., that something with paging_mode_translate() will call this
> hypercall before a release), then a single ASSERT() should be enough to
> make sure that both functions are updated properly?

Yes, that's my expectation.

> I guess that's good enough.  (It's hard not to start to get paranoid
> when you ask yourself these sorts of questions.)

Good. (Indeed.)

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

[Jan Beulich]

>>> On 13.05.19 at 16:35, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:28 PM, Jan Beulich wrote:
>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>> much, because for PV guests documentation (in the public header) does
>> not even match the implementation: It talks about GPFN as input, but
>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>> back the value passed in).
>> 
>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>> directly into top level hypercall handling, and clarify things in the
>> public header accordingly.
>> 
>> Take the liberty and also replace a pointless use of "current" with a
>> more efficient use of an existing local variable (or function parameter
>> to be precise).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Looks good, sorry for the delay:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks.

> A couple of comments:
> 
>> ---
>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>      pointlessly populating a PoD slot just to unpopulate it again right
>>      away, with the page then free floating, i.e. no longer available
>>      for use to replace another PoD slot, and (afaict) no longer
>>      accessible by the guest in any way.
> 
> Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
> immediately see any reason to do the allocation -- I think it just must
> have been C&P without much thought given as to what was going to happen
> next.

The question is: If we remove it, what is the -ENOENT condition
going to be?

>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>>      mfn_t mfn = INVALID_MFN;
>>      p2m_type_t p2mt;
>>  
>> -    if ( !paging_mode_translate(d) )
>> -        return -EACCES;
>> -
>>      switch ( space )
>>      {
>>          case XENMAPSPACE_shared_info:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>>      long rc = 0;
>>      union xen_add_to_physmap_batch_extra extra;
>>  
>> +    ASSERT(paging_mode_translate(d));
> 
> So, just trying to think through the principles behind these two.  We
> know that this is never going to be called w/o first calling
> xatp_permission_check(); if we assume that such a change will be tested
> (i.e., that something with paging_mode_translate() will call this
> hypercall before a release), then a single ASSERT() should be enough to
> make sure that both functions are updated properly?

Yes, that's my expectation.

> I guess that's good enough.  (It's hard not to start to get paranoid
> when you ask yourself these sorts of questions.)

Good. (Indeed.)

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich]

>>> On 13.05.19 at 15:44, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:26 PM, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
>>  
>>          page_list_for_each ( page, &d->page_list )
>>          {
>> -            unsigned long mfn = mfn_x(page_to_mfn(page));
>> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
>> -            unsigned int mapping = IOMMUF_readable;
>> -            int ret;
>> +            if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none )
>> +            {
>> +                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
>> +                if ( get_page_and_type(page, d, PGT_writable_page) )
>> +                    put_page_and_type(page);
>> +                else if ( !rc )
>> +                    rc = -EBUSY;
>> +            }
>>  
>> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
>> -                 ((page->u.inuse.type_info & PGT_type_mask)
>> -                  == PGT_writable_page) )
>> -                mapping |= IOMMUF_writable;
>> +            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
>> +                  PGT_writable_page) )
>> +            {
>> +                unsigned long mfn = mfn_x(page_to_mfn(page));
>> +                unsigned long dfn = mfn_to_gmfn(d, mfn);
>> +                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
>> +                                    IOMMUF_readable | IOMMUF_writable,
>> +                                    &flush_flags);
> 
> What's the idea behind calling iommu_map() here, rather than relying on
> the one in _get_page_type()?  Does need_iommu_pt_sync() not work yet at
> this point, or do you expect there to be pages that have been marked
> PGT_writable without having gone through _get_page_type()?

"iommu=dom0-strict" doesn't work without this. That's because the
PV Dom0 builder sets the pages in the intial allocation to
PGT_writable (or the necessary page table types) long before
calling iommu_hwdom_init(). Probably this could be re-arranged, but
I don't think I'm up to this right now.

But having at least made the attempt still pointed out two issues
with the patch: For PGT_none pages (i.e. get_page_and_type()
actually having the intended effect) IOTLB flushing wasn't done
(not a big problem, since Dom0 hasn't run yet, but inconsistent
with the function calling iommu_iotlb_flush_all() in the first place).
Plus I don't think the {get,put}_page_and_type() dance should
be done at all for PVH Dom0.

So a v2 is coming in any event.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich]

>>> On 13.05.19 at 15:44, <george.dunlap@citrix.com> wrote:
> On 3/5/19 1:26 PM, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -192,21 +192,27 @@ void __hwdom_init iommu_hwdom_init(struc
>>  
>>          page_list_for_each ( page, &d->page_list )
>>          {
>> -            unsigned long mfn = mfn_x(page_to_mfn(page));
>> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
>> -            unsigned int mapping = IOMMUF_readable;
>> -            int ret;
>> +            if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_none )
>> +            {
>> +                ASSERT(!(page->u.inuse.type_info & PGT_count_mask));
>> +                if ( get_page_and_type(page, d, PGT_writable_page) )
>> +                    put_page_and_type(page);
>> +                else if ( !rc )
>> +                    rc = -EBUSY;
>> +            }
>>  
>> -            if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
>> -                 ((page->u.inuse.type_info & PGT_type_mask)
>> -                  == PGT_writable_page) )
>> -                mapping |= IOMMUF_writable;
>> +            if ( ((page->u.inuse.type_info & PGT_type_mask) ==
>> +                  PGT_writable_page) )
>> +            {
>> +                unsigned long mfn = mfn_x(page_to_mfn(page));
>> +                unsigned long dfn = mfn_to_gmfn(d, mfn);
>> +                int ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0,
>> +                                    IOMMUF_readable | IOMMUF_writable,
>> +                                    &flush_flags);
> 
> What's the idea behind calling iommu_map() here, rather than relying on
> the one in _get_page_type()?  Does need_iommu_pt_sync() not work yet at
> this point, or do you expect there to be pages that have been marked
> PGT_writable without having gone through _get_page_type()?

"iommu=dom0-strict" doesn't work without this. That's because the
PV Dom0 builder sets the pages in the intial allocation to
PGT_writable (or the necessary page table types) long before
calling iommu_hwdom_init(). Probably this could be re-arranged, but
I don't think I'm up to this right now.

But having at least made the attempt still pointed out two issues
with the patch: For PGT_none pages (i.e. get_page_and_type()
actually having the intended effect) IOTLB flushing wasn't done
(not a big problem, since Dom0 hasn't run yet, but inconsistent
with the function calling iommu_iotlb_flush_all() in the first place).
Plus I don't think the {get,put}_page_and_type() dance should
be done at all for PVH Dom0.

So a v2 is coming in any event.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel