On 07.05.19 at 17:14, <julien.grall@xxxxxxx> wrote:
mfn_to_gfn and mfn_to_gmfn are doing exactly the same except the former
is using mfn_t.
... and gfn_t (return type) as of patch 3.
Furthermore, the naming of the former is more consistent with the
current naming scheme (GFN/MFN). So use replace mfn_to_gmfn with
mfn_to_gfn in x86 code.
Nit: Either "use" or "replace with", but not both.
@@ -713,19 +713,20 @@ int arch_domain_soft_reset(struct domain *d)
ASSERT( owner == d );
mfn = page_to_mfn(page);
- gfn = mfn_to_gmfn(d, mfn_x(mfn));
+ gfn = mfn_to_gfn(d, mfn);
/*
* gfn == INVALID_GFN indicates that the shared_info page was never mapped
* to the domain's address space and there is nothing to replace.
*/
- if ( gfn == gfn_x(INVALID_GFN) )
+ if ( gfn_eq(gfn, INVALID_GFN) )
goto exit_put_page;
- if ( !mfn_eq(get_gfn_query(d, gfn, &p2mt), mfn) )
+ if ( !mfn_eq(get_gfn_query(d, gfn_x(gfn), &p2mt), mfn) )
{
- printk(XENLOG_G_ERR "Failed to get Dom%d's shared_info GFN (%lx)\n",
- d->domain_id, gfn);
+ printk(XENLOG_G_ERR
+ "Failed to get %pd's shared_info GFN (%"PRI_gfn")\n",
I'd recommend to drop the parentheses from the format string at the
same time.
@@ -733,31 +734,34 @@ int arch_domain_soft_reset(struct domain *d)
new_page = alloc_domheap_page(d, 0);
if ( !new_page )
{
- printk(XENLOG_G_ERR "Failed to alloc a page to replace"
- " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
+ printk(XENLOG_G_ERR
+ "Failed to alloc a page to replace %pd's shared_info frame
%"PRI_gfn"\n",
s/frame/GFN/ to better match the earlier one? Same in the further log
messages here then.
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2632,19 +2632,20 @@ int free_page_type(struct page_info *page, unsigned
long type,
{
#ifdef CONFIG_PV
struct domain *owner = page_get_owner(page);
- unsigned long gmfn;
int rc;
if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
{
+ gfn_t gfn;
+
/* A page table is dirtied when its type count becomes zero. */
paging_mark_dirty(owner, page_to_mfn(page));
ASSERT(!shadow_mode_refcounts(owner));
- gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
- if ( VALID_M2P(gmfn) )
- shadow_remove_all_shadows(owner, _mfn(gmfn));
+ gfn = mfn_to_gfn(owner, page_to_mfn(page));
+ if ( VALID_M2P(gfn_x(gfn)) )
+ shadow_remove_all_shadows(owner, _mfn(gfn_x(gfn)));
}
This is a highly suspicious change imo (albeit the code was bogus
already before): It certainly isn't GFN here even if we were to assume
translated mode could be in use. One other caller of
the function, sh_page_fault() passes a variable named gmfn as well,
but typed mfn_t (and this gmfn gets set from get_gfn(), i.e. is _not_
a GFN). The 3rd one, shadow_prepare_page_type_change(), clearly
passes an MFN.
I think the best course of action here is to split out the change,
just to explain why removing the mfn_to_gmfn() here altogether
is appropriate nowadays: PV guests can't be in translated mode
anymore, and hence mfn_to_gmfn() doesn't do any translation. At
that point the VALID_M2P() check can go away as well, so you'll be
able to simply do
shadow_remove_all_shadows(owner, page_to_mfn(page));
perhaps with another !shadow_mode_translate() assertion added
next to the one that's already there. Tim, thoughts?
With this split out and irrespective of whether you decide to follow
the format string suggestions further up
