[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 16/17] xen/mm: Convert {s, g}et_gpfn_from_mfn() to use typesafe MFN
- To: Hongyan Xia <hx242@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- From: Julien Grall <julien@xxxxxxx>
- Date: Mon, 23 Mar 2020 12:26:59 +0000
- Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien.grall@xxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Delivery-date: Mon, 23 Mar 2020 12:27:10 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
Hi,
On 23/03/2020 12:11, Hongyan Xia wrote:
On Sun, 2020-03-22 at 16:14 +0000, julien@xxxxxxx wrote:
From: Julien Grall <julien.grall@xxxxxxx>
The first parameter of {s,g}et_gpfn_from_mfn() is an MFN, so it can
be
switched to use the typesafe.
At the same time, replace gpfn with pfn in the helpers as they all
deal
with PFN and also turn the macros to static inline.
Note that the return of the getter and the 2nd parameter of the
setter
have not been converted to use typesafe PFN because it was requiring
more changes than expected.
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
---
This was originally sent as part of "xen/arm: Properly disable
M2P
on Arm" [1].
Changes since the original version:
- mfn_to_gmfn() is still present for now so update it
- Remove stray +
- Avoid churn in set_pfn_from_mfn() by inverting mfn and mfn_
- Remove tags
- Fix build in mem_sharing
[1] <20190603160350.29806-1-julien.grall@xxxxxxx>
---
xen/arch/x86/cpu/mcheck/mcaction.c | 2 +-
xen/arch/x86/mm.c | 14 +++----
xen/arch/x86/mm/mem_sharing.c | 20 ++++-----
xen/arch/x86/mm/p2m-pod.c | 4 +-
xen/arch/x86/mm/p2m-pt.c | 35 ++++++++--------
xen/arch/x86/mm/p2m.c | 66 +++++++++++++++-------------
--
xen/arch/x86/mm/paging.c | 4 +-
xen/arch/x86/pv/dom0_build.c | 6 +--
xen/arch/x86/x86_64/traps.c | 8 ++--
xen/common/page_alloc.c | 2 +-
xen/include/asm-arm/mm.h | 2 +-
xen/include/asm-x86/grant_table.h | 2 +-
xen/include/asm-x86/mm.h | 12 ++++--
xen/include/asm-x86/p2m.h | 2 +-
14 files changed, 93 insertions(+), 86 deletions(-)
[...]
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index abf4cc23e4..11614f9107 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -319,7 +319,7 @@ struct page_info *get_page_from_gva(struct vcpu
*v, vaddr_t va,
#define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY)
/* Xen always owns P2M on ARM */
-#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn);
} while (0)
+static inline void set_pfn_from_mfn(mfn_t mfn, unsigned long pfn) {}
#define mfn_to_gmfn(_d, mfn) (mfn)
I do not have a setup to compile and test code for Arm, but wouldn't
the compiler complain about unused arguments here? The marco version
explicitly silenced compiler complaints.
The macro version does not use (void)(arg) for silencing unused
parameter. It is for evaluating (mfn) but ignore the result. A compiler
would warn without (void) because we build Xen with -Wall which include
-Wunused-value.
Xen is not used with -Wunused-parameter, so there is no concern about
unused parameters. If we ever decided to turn on -Wunused-parameter (or
-Wextra), then we will have quite a bit of code to modify (such as
callbacks not using all the parameters) to make it compile.
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-
x86/grant_table.h
index 5871238f6d..b6a09c4c6c 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -41,7 +41,7 @@ static inline int
replace_grant_host_mapping(uint64_t addr, mfn_t frame,
#define gnttab_get_frame_gfn(gt, st, idx)
({ \
mfn_t mfn_ = (st) ? gnttab_status_mfn(gt,
idx) \
: gnttab_shared_mfn(gt,
idx); \
- unsigned long gpfn_ =
get_gpfn_from_mfn(mfn_x(mfn_)); \
+ unsigned long gpfn_ =
get_pfn_from_mfn(mfn_); \
VALID_M2P(gpfn_) ? _gfn(gpfn_) :
INVALID_GFN; \
})
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 53f2ed7c7d..2a4f42e78f 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -500,9 +500,10 @@ extern paddr_t mem_hotplug;
*/
extern bool machine_to_phys_mapping_valid;
-static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
long pfn)
+static inline void set_pfn_from_mfn(mfn_t mfn_, unsigned long pfn)
{
- const struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));
+ const unsigned long mfn = mfn_x(mfn_);
+ const struct domain *d = page_get_owner(mfn_to_page(mfn_));
unsigned long entry = (d && (d == dom_cow)) ? SHARED_M2P_ENTRY :
pfn;
if ( !machine_to_phys_mapping_valid )
@@ -515,11 +516,14 @@ static inline void set_gpfn_from_mfn(unsigned
long mfn, unsigned long pfn)
extern struct rangeset *mmio_ro_ranges;
-#define get_gpfn_from_mfn(mfn) (machine_to_phys_mapping[(mfn)])
+static inline unsigned long get_pfn_from_mfn(mfn_t mfn)
+{
+ return machine_to_phys_mapping[mfn_x(mfn)];
+}
Any specific reason this (and some other macros) are turned into static
inline? I don't have a problem with them being inline functions but
just wondering if there is a reason to do so.
static inline provides better safety check than macro. So we tend to
switch to static inline whenever the headers inter-dependency madness is
not interplaying.
#define mfn_to_gmfn(_d, mfn) \
( (paging_mode_translate(_d)) \
- ? get_gpfn_from_mfn(mfn) \
+ ? get_pfn_from_mfn(_mfn(mfn)) \
: (mfn) )
#define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) |
((unsigned)(pfn) >> 20))
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index a2c6049834..39dae242b0 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -505,7 +505,7 @@ static inline struct page_info
*get_page_from_gfn(
static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn)
{
if ( paging_mode_translate(d) )
- return _gfn(get_gpfn_from_mfn(mfn_x(mfn)));
+ return _gfn(get_pfn_from_mfn(mfn));
else
return _gfn(mfn_x(mfn));
}
Apart from the two comments above, looks good to me.
Reviewed-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
Thank you!
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
