Hi Jan,
On 26/03/2020 15:39, Jan Beulich wrote:
On 22.03.2020 17:14, jul...@xen.org wrote:
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -952,25 +952,27 @@ int arch_set_info_guest(
}
else
{
- unsigned long pfn = pagetable_get_pfn(v->arch.guest_table);
+ mfn_t mfn = pagetable_get_mfn(v->arch.guest_table);
bool fail;
if ( !compat )
{
- fail = xen_pfn_to_cr3(pfn) != c.nat->ctrlreg[3];
+ fail = mfn_to_cr3(mfn) != c.nat->ctrlreg[3];
The patch, besides a few other comments further down, looks fine
on its own, but I don't think it can be acked without seeing the
effects of the adjustments pending to the patch introducing
mfn_to_cr3() and friends.
@@ -3116,24 +3116,24 @@ int vcpu_destroy_pagetables(struct vcpu *v)
/* Free that page if non-zero */
do {
- if ( mfn )
+ if ( !mfn_eq(mfn, _mfn(0)) )
I admit I'm not fully certain either, but at the first glance
if ( mfn_x(mfn) )
would seem more in line with the original code to me (and then
also elsewhere).
It is doing *exactly* the same things. The whole point of typesafe is to
use typesafe helper not open-coding test everywhere.
It is also easier to spot any use of MFN 0 within the code as you know
could grep "_mfn(0)".
Therefore I will insist to the code as-is.
@@ -3560,19 +3561,18 @@ long do_mmuext_op(
if ( unlikely(rc) )
break;
- old_mfn = pagetable_get_pfn(curr->arch.guest_table_user);
+ old_mfn = pagetable_get_mfn(curr->arch.guest_table_user);
/*
* This is particularly important when getting restarted after the
* previous attempt got preempted in the put-old-MFN phase.
*/
- if ( old_mfn == op.arg1.mfn )
+ if ( mfn_eq(old_mfn, new_mfn) )
break;
- if ( op.arg1.mfn != 0 )
+ if ( !mfn_eq(new_mfn, _mfn(0)) )
At least here I would clearly prefer the old code to be kept.
See above.
@@ -3580,19 +3580,19 @@ long do_mmuext_op(
else if ( rc != -ERESTART )
gdprintk(XENLOG_WARNING,
"Error %d installing new mfn %" PRI_mfn "\n",
- rc, op.arg1.mfn);
+ rc, mfn_x(new_mfn));
Here I'm also not sure I see the point of the conversion.
op.arg1.mfn and mfn are technically not the same type. The former is a
xen_pfn_t, whilst the latter is mfn_t.
In practice they are both unsigned long on x86, so it should be fine to
use PRI_mfn. However, I think this is an abuse and we should aim to use
the proper PRI_* for a type.
@@ -2351,11 +2351,11 @@ int sh_safe_not_to_sync(struct vcpu *v, mfn_t gl1mfn)
ASSERT(mfn_valid(smfn));
#endif
- if ( pagetable_get_pfn(v->arch.shadow_table[0]) == mfn_x(smfn)
+ if ( mfn_eq(pagetable_get_mfn(v->arch.shadow_table[0]), smfn)
#if (SHADOW_PAGING_LEVELS == 3)
- || pagetable_get_pfn(v->arch.shadow_table[1]) == mfn_x(smfn)
- || pagetable_get_pfn(v->arch.shadow_table[2]) == mfn_x(smfn)
- || pagetable_get_pfn(v->arch.shadow_table[3]) == mfn_x(smfn)
+ || mfn_eq(pagetable_get_mfn(v->arch.shadow_table[1]), smfn)
+ || mfn_eq(pagetable_get_mfn(v->arch.shadow_table[2]), smfn)
+ || mfn_eq(pagetable_get_mfn(v->arch.shadow_table[3]), smfn)
#endif
)
While here moving the || to their designated places would make
the code look worse overall, ...
@@ -3707,7 +3707,7 @@ sh_update_linear_entries(struct vcpu *v)
/* Don't try to update the monitor table if it doesn't exist */
if ( shadow_mode_external(d)
- && pagetable_get_pfn(v->arch.monitor_table) == 0 )
+ && pagetable_is_null(v->arch.monitor_table) )
... could I talk you into moving the && here to the end of the
previous line, as you're touching this anyway?
I will do.
Also, seeing there's quite a few conversions to pagetable_is_null()
and also seeing that this patch is quite big - could this
conversion be split out?
I will have a look.
@@ -213,17 +214,17 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa,
unsigned int flags)
#ifndef __ASSEMBLY__
/* Page-table type. */
-typedef struct { u64 pfn; } pagetable_t;
-#define pagetable_get_paddr(x) ((paddr_t)(x).pfn << PAGE_SHIFT)
+typedef struct { mfn_t mfn; } pagetable_t;
+#define PAGETABLE_NULL_MFN _mfn(0)
I'd prefer to get away without this constant.
I would rather keep the constant as it makes easier to understand what
_mfn(0) means in the context of the pagetable.
Cheers,
--
Julien Grall