[PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

This is what both callers of guest_physmap_add_page() in memory.c want
(for the !paging_mode_translate() case), and it is also what both
callers in gnttab_transfer() need (but have been lacking). The other
(x86-specific) callers are all HVM-only, and hence unaffected by this
change.

Sadly this isn't enough yet to drop Arm's dummy macro, as there's one
more use in page_alloc.c.

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

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
          * any guest-requested type changes succeed and remove the IOMMU
          * entry).
          */
-        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
+        if ( t != p2m_ram_rw )
             return 0;
 
         for ( i = 0; i < (1UL << page_order); ++i, ++page )
         {
-            if ( get_page_and_type(page, d, PGT_writable_page) )
+            if ( !need_iommu_pt_sync(d) )
+                /* nothing */;
+            else if ( get_page_and_type(page, d, PGT_writable_page) )
                 put_page_and_type(page);
             else
                 return -EINVAL;
+
+            set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn) + i);
         }
 
         return 0;
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -270,16 +270,10 @@ static void populate_physmap(struct memo
 
             guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order);
 
-            if ( !paging_mode_translate(d) )
-            {
-                for ( j = 0; j < (1U << a->extent_order); j++ )
-                    set_gpfn_from_mfn(mfn_x(mfn_add(mfn, j)), gpfn + j);
-
-                /* Inform the domain of the new page's machine address. */ 
-                if ( unlikely(__copy_mfn_to_guest_offset(a->extent_list, i,
-                                                         mfn)) )
-                    goto out;
-            }
+            if ( !paging_mode_translate(d) &&
+                 /* Inform the domain of the new page's machine address. */
+                 unlikely(__copy_mfn_to_guest_offset(a->extent_list, i, mfn)) )
+                goto out;
         }
     }
 
@@ -755,15 +749,11 @@ static long memory_exchange(XEN_GUEST_HA
             guest_physmap_add_page(d, _gfn(gpfn), mfn,
                                    exch.out.extent_order);
 
-            if ( !paging_mode_translate(d) )
-            {
-                for ( k = 0; k < (1UL << exch.out.extent_order); k++ )
-                    set_gpfn_from_mfn(mfn_x(mfn_add(mfn, k)), gpfn + k);
-                if ( __copy_mfn_to_guest_offset(exch.out.extent_start,
-                                                (i << out_chunk_order) + j,
-                                                mfn) )
-                    rc = -EFAULT;
-            }
+            if ( !paging_mode_translate(d) &&
+                 __copy_mfn_to_guest_offset(exch.out.extent_start,
+                                            (i << out_chunk_order) + j,
+                                            mfn) )
+                rc = -EFAULT;
         }
         BUG_ON( !(d->is_dying) && (j != (1UL << out_chunk_order)) );
 





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

[Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

This is what both callers of guest_physmap_add_page() in memory.c want
(for the !paging_mode_translate() case), and it is also what both
callers in gnttab_transfer() need (but have been lacking). The other
(x86-specific) callers are all HVM-only, and hence unaffected by this
change.

Sadly this isn't enough yet to drop Arm's dummy macro, as there's one
more use in page_alloc.c.

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

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
          * any guest-requested type changes succeed and remove the IOMMU
          * entry).
          */
-        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
+        if ( t != p2m_ram_rw )
             return 0;
 
         for ( i = 0; i < (1UL << page_order); ++i, ++page )
         {
-            if ( get_page_and_type(page, d, PGT_writable_page) )
+            if ( !need_iommu_pt_sync(d) )
+                /* nothing */;
+            else if ( get_page_and_type(page, d, PGT_writable_page) )
                 put_page_and_type(page);
             else
                 return -EINVAL;
+
+            set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn) + i);
         }
 
         return 0;
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -270,16 +270,10 @@ static void populate_physmap(struct memo
 
             guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order);
 
-            if ( !paging_mode_translate(d) )
-            {
-                for ( j = 0; j < (1U << a->extent_order); j++ )
-                    set_gpfn_from_mfn(mfn_x(mfn_add(mfn, j)), gpfn + j);
-
-                /* Inform the domain of the new page's machine address. */ 
-                if ( unlikely(__copy_mfn_to_guest_offset(a->extent_list, i,
-                                                         mfn)) )
-                    goto out;
-            }
+            if ( !paging_mode_translate(d) &&
+                 /* Inform the domain of the new page's machine address. */
+                 unlikely(__copy_mfn_to_guest_offset(a->extent_list, i, mfn)) )
+                goto out;
         }
     }
 
@@ -755,15 +749,11 @@ static long memory_exchange(XEN_GUEST_HA
             guest_physmap_add_page(d, _gfn(gpfn), mfn,
                                    exch.out.extent_order);
 
-            if ( !paging_mode_translate(d) )
-            {
-                for ( k = 0; k < (1UL << exch.out.extent_order); k++ )
-                    set_gpfn_from_mfn(mfn_x(mfn_add(mfn, k)), gpfn + k);
-                if ( __copy_mfn_to_guest_offset(exch.out.extent_start,
-                                                (i << out_chunk_order) + j,
-                                                mfn) )
-                    rc = -EFAULT;
-            }
+            if ( !paging_mode_translate(d) &&
+                 __copy_mfn_to_guest_offset(exch.out.extent_start,
+                                            (i << out_chunk_order) + j,
+                                            mfn) )
+                rc = -EFAULT;
         }
         BUG_ON( !(d->is_dying) && (j != (1UL << out_chunk_order)) );
 





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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Julien Grall]

Hi Jan,

On 5/2/19 7:58 AM, Jan Beulich wrote:
> This is what both callers of guest_physmap_add_page() in memory.c want
> (for the !paging_mode_translate() case), and it is also what both
> callers in gnttab_transfer() need (but have been lacking). The other
> (x86-specific) callers are all HVM-only, and hence unaffected by this
> change.
> 
> Sadly this isn't enough yet to drop Arm's dummy macro, as there's one
> more use in page_alloc.c.

I think we can live with that. I have now sent a patch to move the dummy 
implementation in common code (see 
<20190507151458.29350-15-julien.grall@arm.com>).

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

The common code looks fine to me. I will leave Andrew commenting for the 
x86 parts:

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

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Julien Grall]

Hi Jan,

On 5/2/19 7:58 AM, Jan Beulich wrote:
> This is what both callers of guest_physmap_add_page() in memory.c want
> (for the !paging_mode_translate() case), and it is also what both
> callers in gnttab_transfer() need (but have been lacking). The other
> (x86-specific) callers are all HVM-only, and hence unaffected by this
> change.
> 
> Sadly this isn't enough yet to drop Arm's dummy macro, as there's one
> more use in page_alloc.c.

I think we can live with that. I have now sent a patch to move the dummy 
implementation in common code (see 
<20190507151458.29350-15-julien.grall@arm.com>).

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

The common code looks fine to me. I will leave Andrew commenting for the 
x86 parts:

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

Cheers,

-- 
Julien Grall

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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/2/19 7:58 AM, Jan Beulich wrote:
> This is what both callers of guest_physmap_add_page() in memory.c want
> (for the !paging_mode_translate() case), and it is also what both
> callers in gnttab_transfer() need (but have been lacking). The other
> (x86-specific) callers are all HVM-only, and hence unaffected by this
> change.

Sorry, what's going on here?

 -George

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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/2/19 7:58 AM, Jan Beulich wrote:
> This is what both callers of guest_physmap_add_page() in memory.c want
> (for the !paging_mode_translate() case), and it is also what both
> callers in gnttab_transfer() need (but have been lacking). The other
> (x86-specific) callers are all HVM-only, and hence unaffected by this
> change.

Sorry, what's going on here?

 -George

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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote:
> On 5/2/19 7:58 AM, Jan Beulich wrote:
>> This is what both callers of guest_physmap_add_page() in memory.c want
>> (for the !paging_mode_translate() case), and it is also what both
>> callers in gnttab_transfer() need (but have been lacking). The other
>> (x86-specific) callers are all HVM-only, and hence unaffected by this
>> change.
> 
> Sorry, what's going on here?

I guess that's a Jan-wrote-an-unparsable-description-once-again
question?

1) The two callers in common/memory.c currently call set_gpfn_from_mfn()
themselves, so moving the call into guest_physmap_add_page() helps
tidy their code.

2) The two callers in common/grant_table.c fail to make that call alongside
the one to guest_physmap_add_page(), so will actually get fixed by the
change.

3) Other callers are HVM only and are hence unaffected by a change to
the function's !paging_mode_translate() part.

Jan



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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote:
> On 5/2/19 7:58 AM, Jan Beulich wrote:
>> This is what both callers of guest_physmap_add_page() in memory.c want
>> (for the !paging_mode_translate() case), and it is also what both
>> callers in gnttab_transfer() need (but have been lacking). The other
>> (x86-specific) callers are all HVM-only, and hence unaffected by this
>> change.
> 
> Sorry, what's going on here?

I guess that's a Jan-wrote-an-unparsable-description-once-again
question?

1) The two callers in common/memory.c currently call set_gpfn_from_mfn()
themselves, so moving the call into guest_physmap_add_page() helps
tidy their code.

2) The two callers in common/grant_table.c fail to make that call alongside
the one to guest_physmap_add_page(), so will actually get fixed by the
change.

3) Other callers are HVM only and are hence unaffected by a change to
the function's !paging_mode_translate() part.

Jan



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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/8/19 2:57 PM, Jan Beulich wrote:
>>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote:
>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>> This is what both callers of guest_physmap_add_page() in memory.c want
>>> (for the !paging_mode_translate() case), and it is also what both
>>> callers in gnttab_transfer() need (but have been lacking). The other
>>> (x86-specific) callers are all HVM-only, and hence unaffected by this
>>> change.
>>
>> Sorry, what's going on here?
> 
> I guess that's a Jan-wrote-an-unparsable-description-once-again
> question?

Sort of, yes. :-)  It's not unparsable; it's just missing some information.

> 1) The two callers in common/memory.c currently call set_gpfn_from_mfn()
> themselves, so moving the call into guest_physmap_add_page() helps
> tidy their code.
> 
> 2) The two callers in common/grant_table.c fail to make that call alongside
> the one to guest_physmap_add_page(), so will actually get fixed by the
> change.
> 
> 3) Other callers are HVM only and are hence unaffected by a change to
> the function's !paging_mode_translate() part.

Right.  I think I would have written something like this:

8<---

When giving ownership of a page to a PV guest, a number of things need
to be done, including adding the page into the IOMMU if appropriate, and
updating the m2p.  At the moment this is done by calling
guest_physmap_add_entry() and set_gpfn_from_mfn() separately.

Unfortunately, this duplication leads to mistakes: gnttab_transfer()
calls guest_physmap_add_entry() but fails to call set_gpfn_from_mfn().

Since guest_physmap_add_entry() is already special-casing PV guests,
move set_gpfn_from_mfn() into that function.

--->8

I have a technical question about the patch; I'll reply inline to the
patch itself.

 -George

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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/8/19 2:57 PM, Jan Beulich wrote:
>>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote:
>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>> This is what both callers of guest_physmap_add_page() in memory.c want
>>> (for the !paging_mode_translate() case), and it is also what both
>>> callers in gnttab_transfer() need (but have been lacking). The other
>>> (x86-specific) callers are all HVM-only, and hence unaffected by this
>>> change.
>>
>> Sorry, what's going on here?
> 
> I guess that's a Jan-wrote-an-unparsable-description-once-again
> question?

Sort of, yes. :-)  It's not unparsable; it's just missing some information.

> 1) The two callers in common/memory.c currently call set_gpfn_from_mfn()
> themselves, so moving the call into guest_physmap_add_page() helps
> tidy their code.
> 
> 2) The two callers in common/grant_table.c fail to make that call alongside
> the one to guest_physmap_add_page(), so will actually get fixed by the
> change.
> 
> 3) Other callers are HVM only and are hence unaffected by a change to
> the function's !paging_mode_translate() part.

Right.  I think I would have written something like this:

8<---

When giving ownership of a page to a PV guest, a number of things need
to be done, including adding the page into the IOMMU if appropriate, and
updating the m2p.  At the moment this is done by calling
guest_physmap_add_entry() and set_gpfn_from_mfn() separately.

Unfortunately, this duplication leads to mistakes: gnttab_transfer()
calls guest_physmap_add_entry() but fails to call set_gpfn_from_mfn().

Since guest_physmap_add_entry() is already special-casing PV guests,
move set_gpfn_from_mfn() into that function.

--->8

I have a technical question about the patch; I'll reply inline to the
patch itself.

 -George

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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/2/19 7:58 AM, Jan Beulich wrote:
> This is what both callers of guest_physmap_add_page() in memory.c want
> (for the !paging_mode_translate() case), and it is also what both
> callers in gnttab_transfer() need (but have been lacking). The other
> (x86-specific) callers are all HVM-only, and hence unaffected by this
> change.
> 
> Sadly this isn't enough yet to drop Arm's dummy macro, as there's one
> more use in page_alloc.c.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>           * any guest-requested type changes succeed and remove the IOMMU
>           * entry).
>           */
> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
> +        if ( t != p2m_ram_rw )
>              return 0;

So, you seem to be claiming that the only way to get here is via
guest_physmap_add_page(), which will always call this function with
p2m_ram_rw.  So then what's the point of this conditional at all
anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?

And if we ever *do* get here with t == p2m_ram_rw, do we really not want
to call set_gpfn_from_mfn()?

 -George

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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/2/19 7:58 AM, Jan Beulich wrote:
> This is what both callers of guest_physmap_add_page() in memory.c want
> (for the !paging_mode_translate() case), and it is also what both
> callers in gnttab_transfer() need (but have been lacking). The other
> (x86-specific) callers are all HVM-only, and hence unaffected by this
> change.
> 
> Sadly this isn't enough yet to drop Arm's dummy macro, as there's one
> more use in page_alloc.c.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>           * any guest-requested type changes succeed and remove the IOMMU
>           * entry).
>           */
> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
> +        if ( t != p2m_ram_rw )
>              return 0;

So, you seem to be claiming that the only way to get here is via
guest_physmap_add_page(), which will always call this function with
p2m_ram_rw.  So then what's the point of this conditional at all
anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?

And if we ever *do* get here with t == p2m_ram_rw, do we really not want
to call set_gpfn_from_mfn()?

 -George

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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 08.05.19 at 16:59, <george.dunlap@citrix.com> wrote:
> On 5/8/19 2:57 PM, Jan Beulich wrote:
>>>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote:
>>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>>> This is what both callers of guest_physmap_add_page() in memory.c want
>>>> (for the !paging_mode_translate() case), and it is also what both
>>>> callers in gnttab_transfer() need (but have been lacking). The other
>>>> (x86-specific) callers are all HVM-only, and hence unaffected by this
>>>> change.
>>>
>>> Sorry, what's going on here?
>> 
>> I guess that's a Jan-wrote-an-unparsable-description-once-again
>> question?
> 
> Sort of, yes. :-)  It's not unparsable; it's just missing some information.
> 
>> 1) The two callers in common/memory.c currently call set_gpfn_from_mfn()
>> themselves, so moving the call into guest_physmap_add_page() helps
>> tidy their code.
>> 
>> 2) The two callers in common/grant_table.c fail to make that call alongside
>> the one to guest_physmap_add_page(), so will actually get fixed by the
>> change.
>> 
>> 3) Other callers are HVM only and are hence unaffected by a change to
>> the function's !paging_mode_translate() part.
> 
> Right.  I think I would have written something like this:

Well, I can certainly use your suggested text, but for now I
was meaning to perhaps use the text from my earlier reply as
replacement, if that's deemed better than the original. Just
let me know.

Jan

> 8<---
> 
> When giving ownership of a page to a PV guest, a number of things need
> to be done, including adding the page into the IOMMU if appropriate, and
> updating the m2p.  At the moment this is done by calling
> guest_physmap_add_entry() and set_gpfn_from_mfn() separately.
> 
> Unfortunately, this duplication leads to mistakes: gnttab_transfer()
> calls guest_physmap_add_entry() but fails to call set_gpfn_from_mfn().
> 
> Since guest_physmap_add_entry() is already special-casing PV guests,
> move set_gpfn_from_mfn() into that function.
> 
> --->8
> 
> I have a technical question about the patch; I'll reply inline to the
> patch itself.
> 
>  -George





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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 08.05.19 at 16:59, <george.dunlap@citrix.com> wrote:
> On 5/8/19 2:57 PM, Jan Beulich wrote:
>>>>> On 07.05.19 at 18:15, <george.dunlap@citrix.com> wrote:
>>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>>> This is what both callers of guest_physmap_add_page() in memory.c want
>>>> (for the !paging_mode_translate() case), and it is also what both
>>>> callers in gnttab_transfer() need (but have been lacking). The other
>>>> (x86-specific) callers are all HVM-only, and hence unaffected by this
>>>> change.
>>>
>>> Sorry, what's going on here?
>> 
>> I guess that's a Jan-wrote-an-unparsable-description-once-again
>> question?
> 
> Sort of, yes. :-)  It's not unparsable; it's just missing some information.
> 
>> 1) The two callers in common/memory.c currently call set_gpfn_from_mfn()
>> themselves, so moving the call into guest_physmap_add_page() helps
>> tidy their code.
>> 
>> 2) The two callers in common/grant_table.c fail to make that call alongside
>> the one to guest_physmap_add_page(), so will actually get fixed by the
>> change.
>> 
>> 3) Other callers are HVM only and are hence unaffected by a change to
>> the function's !paging_mode_translate() part.
> 
> Right.  I think I would have written something like this:

Well, I can certainly use your suggested text, but for now I
was meaning to perhaps use the text from my earlier reply as
replacement, if that's deemed better than the original. Just
let me know.

Jan

> 8<---
> 
> When giving ownership of a page to a PV guest, a number of things need
> to be done, including adding the page into the IOMMU if appropriate, and
> updating the m2p.  At the moment this is done by calling
> guest_physmap_add_entry() and set_gpfn_from_mfn() separately.
> 
> Unfortunately, this duplication leads to mistakes: gnttab_transfer()
> calls guest_physmap_add_entry() but fails to call set_gpfn_from_mfn().
> 
> Since guest_physmap_add_entry() is already special-casing PV guests,
> move set_gpfn_from_mfn() into that function.
> 
> --->8
> 
> I have a technical question about the patch; I'll reply inline to the
> patch itself.
> 
>  -George





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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote:
> On 5/2/19 7:58 AM, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>>           * any guest-requested type changes succeed and remove the IOMMU
>>           * entry).
>>           */
>> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
>> +        if ( t != p2m_ram_rw )
>>              return 0;
> 
> So, you seem to be claiming that the only way to get here is via
> guest_physmap_add_page(),

Well, I'm not "claiming" anything here, I'm just modifying existing
code (and no more than what fits under this patch's title).

> which will always call this function with
> p2m_ram_rw.  So then what's the point of this conditional at all
> anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?
> 
> And if we ever *do* get here with t == p2m_ram_rw, do we really not want
> to call set_gpfn_from_mfn()?

Thinking about e.g. p2m_grant_map_* I wouldn't want to add the
suggested ASSERT(), and the M2P doesn't want updating in that
case either.

Jan



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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote:
> On 5/2/19 7:58 AM, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>>           * any guest-requested type changes succeed and remove the IOMMU
>>           * entry).
>>           */
>> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
>> +        if ( t != p2m_ram_rw )
>>              return 0;
> 
> So, you seem to be claiming that the only way to get here is via
> guest_physmap_add_page(),

Well, I'm not "claiming" anything here, I'm just modifying existing
code (and no more than what fits under this patch's title).

> which will always call this function with
> p2m_ram_rw.  So then what's the point of this conditional at all
> anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?
> 
> And if we ever *do* get here with t == p2m_ram_rw, do we really not want
> to call set_gpfn_from_mfn()?

Thinking about e.g. p2m_grant_map_* I wouldn't want to add the
suggested ASSERT(), and the M2P doesn't want updating in that
case either.

Jan



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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/8/19 4:16 PM, Jan Beulich wrote:
>>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote:
>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>>>           * any guest-requested type changes succeed and remove the IOMMU
>>>           * entry).
>>>           */
>>> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
>>> +        if ( t != p2m_ram_rw )
>>>              return 0;
>>
>> So, you seem to be claiming that the only way to get here is via
>> guest_physmap_add_page(),
> 
> Well, I'm not "claiming" anything here, I'm just modifying existing
> code (and no more than what fits under this patch's title).

Not here, but in the other email.

But looking at it -- it looks like on x86, guest_physmap_add_entry() is
actually called from *exactly* two locations:
hvm/grant_table.c:create_grant_p2m_mapping(), and
p2m.h:guest_physmap_add_page().

Which sort of makes me wonder if it might not be better to add the PV
conditional to guest_physmap_add_page() instead, and leave
guest_physmap_add_entry() as entirely HVM.

> 
>> which will always call this function with
>> p2m_ram_rw.  So then what's the point of this conditional at all
>> anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?
>>
>> And if we ever *do* get here with t == p2m_ram_rw, do we really not want
>> to call set_gpfn_from_mfn()?
> 
> Thinking about e.g. p2m_grant_map_* I wouldn't want to add the
> suggested ASSERT(), and the M2P doesn't want updating in that
> case either.

Sorry, do you think p2m_grant_map_* is more likely somehow than
p2m_ram_ro?  It looks very much like neither one should ever happen. The
purpose of having an ASSERT() there is to alert developers making such a
fundamental change to the fact that they need to think carefully about
what should happen in that case.

(For safety sake, because ASSERT is only a debugging aid, we certainly
need to keep some sort of conditional here to make sure non-writable
pages don't end up with writable mappings in the IOMMU.)

If we don't have an ASSERT(), then we need to think carefully about
whether we should call set_gp2n_from_mfn() if t is, for instance,
p2m_ram_ro.

 -George

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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap]

On 5/8/19 4:16 PM, Jan Beulich wrote:
>>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote:
>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>>>           * any guest-requested type changes succeed and remove the IOMMU
>>>           * entry).
>>>           */
>>> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
>>> +        if ( t != p2m_ram_rw )
>>>              return 0;
>>
>> So, you seem to be claiming that the only way to get here is via
>> guest_physmap_add_page(),
> 
> Well, I'm not "claiming" anything here, I'm just modifying existing
> code (and no more than what fits under this patch's title).

Not here, but in the other email.

But looking at it -- it looks like on x86, guest_physmap_add_entry() is
actually called from *exactly* two locations:
hvm/grant_table.c:create_grant_p2m_mapping(), and
p2m.h:guest_physmap_add_page().

Which sort of makes me wonder if it might not be better to add the PV
conditional to guest_physmap_add_page() instead, and leave
guest_physmap_add_entry() as entirely HVM.

> 
>> which will always call this function with
>> p2m_ram_rw.  So then what's the point of this conditional at all
>> anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?
>>
>> And if we ever *do* get here with t == p2m_ram_rw, do we really not want
>> to call set_gpfn_from_mfn()?
> 
> Thinking about e.g. p2m_grant_map_* I wouldn't want to add the
> suggested ASSERT(), and the M2P doesn't want updating in that
> case either.

Sorry, do you think p2m_grant_map_* is more likely somehow than
p2m_ram_ro?  It looks very much like neither one should ever happen. The
purpose of having an ASSERT() there is to alert developers making such a
fundamental change to the fact that they need to think carefully about
what should happen in that case.

(For safety sake, because ASSERT is only a debugging aid, we certainly
need to keep some sort of conditional here to make sure non-writable
pages don't end up with writable mappings in the IOMMU.)

If we don't have an ASSERT(), then we need to think carefully about
whether we should call set_gp2n_from_mfn() if t is, for instance,
p2m_ram_ro.

 -George

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

Re: [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 08.05.19 at 17:45, <george.dunlap@citrix.com> wrote:
> On 5/8/19 4:16 PM, Jan Beulich wrote:
>>>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote:
>>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>>>>           * any guest-requested type changes succeed and remove the IOMMU
>>>>           * entry).
>>>>           */
>>>> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
>>>> +        if ( t != p2m_ram_rw )
>>>>              return 0;
>>>
>>> So, you seem to be claiming that the only way to get here is via
>>> guest_physmap_add_page(),
>> 
>> Well, I'm not "claiming" anything here, I'm just modifying existing
>> code (and no more than what fits under this patch's title).
> 
> Not here, but in the other email.
> 
> But looking at it -- it looks like on x86, guest_physmap_add_entry() is
> actually called from *exactly* two locations:
> hvm/grant_table.c:create_grant_p2m_mapping(), and
> p2m.h:guest_physmap_add_page().
> 
> Which sort of makes me wonder if it might not be better to add the PV
> conditional to guest_physmap_add_page() instead, and leave
> guest_physmap_add_entry() as entirely HVM.

Yes, I think I'll do this.

>>> which will always call this function with
>>> p2m_ram_rw.  So then what's the point of this conditional at all
>>> anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?
>>>
>>> And if we ever *do* get here with t == p2m_ram_rw, do we really not want
>>> to call set_gpfn_from_mfn()?
>> 
>> Thinking about e.g. p2m_grant_map_* I wouldn't want to add the
>> suggested ASSERT(), and the M2P doesn't want updating in that
>> case either.
> 
> Sorry, do you think p2m_grant_map_* is more likely somehow than
> p2m_ram_ro?  It looks very much like neither one should ever happen. The
> purpose of having an ASSERT() there is to alert developers making such a
> fundamental change to the fact that they need to think carefully about
> what should happen in that case.

Let's face it - p2m types are a HVM concept only anyway. But this
discussion becomes moot (afaict) with the change above anyway.

Jan



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

Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[Jan Beulich]

>>> On 08.05.19 at 17:45, <george.dunlap@citrix.com> wrote:
> On 5/8/19 4:16 PM, Jan Beulich wrote:
>>>>> On 08.05.19 at 17:08, <george.dunlap@citrix.com> wrote:
>>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>>>>           * any guest-requested type changes succeed and remove the IOMMU
>>>>           * entry).
>>>>           */
>>>> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
>>>> +        if ( t != p2m_ram_rw )
>>>>              return 0;
>>>
>>> So, you seem to be claiming that the only way to get here is via
>>> guest_physmap_add_page(),
>> 
>> Well, I'm not "claiming" anything here, I'm just modifying existing
>> code (and no more than what fits under this patch's title).
> 
> Not here, but in the other email.
> 
> But looking at it -- it looks like on x86, guest_physmap_add_entry() is
> actually called from *exactly* two locations:
> hvm/grant_table.c:create_grant_p2m_mapping(), and
> p2m.h:guest_physmap_add_page().
> 
> Which sort of makes me wonder if it might not be better to add the PV
> conditional to guest_physmap_add_page() instead, and leave
> guest_physmap_add_entry() as entirely HVM.

Yes, I think I'll do this.

>>> which will always call this function with
>>> p2m_ram_rw.  So then what's the point of this conditional at all
>>> anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?
>>>
>>> And if we ever *do* get here with t == p2m_ram_rw, do we really not want
>>> to call set_gpfn_from_mfn()?
>> 
>> Thinking about e.g. p2m_grant_map_* I wouldn't want to add the
>> suggested ASSERT(), and the M2P doesn't want updating in that
>> case either.
> 
> Sorry, do you think p2m_grant_map_* is more likely somehow than
> p2m_ram_ro?  It looks very much like neither one should ever happen. The
> purpose of having an ASSERT() there is to alert developers making such a
> fundamental change to the fact that they need to think carefully about
> what should happen in that case.

Let's face it - p2m types are a HVM concept only anyway. But this
discussion becomes moot (afaict) with the change above anyway.

Jan



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