Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
Hi, On 11/12/18 4:58 PM, Andrii Anisov wrote: What's wrong with including clean-up patch in a series adding a feature? I do not mean it is wrong. Just assuming that introducing a new feature and cleaning up a code might be different processes with a different review period. We don't have different process nor different review period between clean-up and new feature. I tend to do clean-up when writing new features... See my cacheflush series as well. If the clean-up is small then I will append/prepend to the feature series. This patch modify a function that was called by this patch and therefore depends on the rest of the series. This was written with this series so it makes sense to me to do it together as it dictates the order I would like the patches applied and simplify tracking (I have many series in flight). Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
> What's wrong with including clean-up patch in a series adding a feature? I do not mean it is wrong. Just assuming that introducing a new feature and cleaning up a code might be different processes with a different review period. Sincerely, Andrii Anisov. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
On 11/12/18 4:49 PM, Andrii Anisov wrote: Hello Julien, Hi, I'm just wondering if this patch really belongs to xentrace series. It rather looks like a separate cleanup patch. What's wrong with including clean-up patch in a series adding a feature? Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
Hello Julien,
I'm just wondering if this patch really belongs to xentrace series.
It rather looks like a separate cleanup patch.
Sincerely,
Andrii Anisov.
вт, 6 лист. 2018 о 21:16 Julien Grall пише:
> No functional change intended.
>
> Only reasonable clean-ups are done in this patch. The rest will use _gfn
> for the time being.
>
> Signed-off-by: Julien Grall
> ---
> xen/arch/arm/guestcopy.c | 2 +-
> xen/arch/arm/mm.c| 2 +-
> xen/arch/x86/cpu/vpmu.c | 2 +-
> xen/arch/x86/domain.c| 12 ++--
> xen/arch/x86/domctl.c| 6 +++---
> xen/arch/x86/hvm/dm.c| 2 +-
> xen/arch/x86/hvm/domain.c| 2 +-
> xen/arch/x86/hvm/hvm.c | 9 +
> xen/arch/x86/hvm/svm/svm.c | 8
> xen/arch/x86/hvm/viridian/viridian.c | 24
> xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
> xen/arch/x86/hvm/vmx/vvmx.c | 12 ++--
> xen/arch/x86/mm.c| 24 ++--
> xen/arch/x86/mm/p2m.c| 2 +-
> xen/arch/x86/mm/shadow/hvm.c | 6 +++---
> xen/arch/x86/physdev.c | 3 ++-
> xen/arch/x86/pv/descriptor-tables.c | 5 ++---
> xen/arch/x86/pv/emul-priv-op.c | 6 +++---
> xen/arch/x86/pv/mm.c | 2 +-
> xen/arch/x86/traps.c | 11 ++-
> xen/common/domain.c | 2 +-
> xen/common/event_fifo.c | 12 ++--
> xen/common/memory.c | 4 ++--
> xen/common/tmem_xen.c| 2 +-
> xen/include/asm-arm/p2m.h| 6 +++---
> xen/include/asm-x86/p2m.h| 11 +++
> 26 files changed, 95 insertions(+), 86 deletions(-)
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7a0f3e9d5f..55892062bb 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -37,7 +37,7 @@ static struct page_info *translate_get_page(copy_info_t
> info, uint64_t addr,
> return get_page_from_gva(info.gva.v, addr,
> write ? GV2M_WRITE : GV2M_READ);
>
> -page = get_page_from_gfn(info.gpa.d, paddr_to_pfn(addr), &p2mt,
> P2M_ALLOC);
> +page = get_page_from_gfn(info.gpa.d, gaddr_to_gfn(addr), &p2mt,
> P2M_ALLOC);
>
> if ( !page )
> return NULL;
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 72d0285768..88711096ef 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1268,7 +1268,7 @@ int xenmem_add_to_physmap_one(
>
> /* Take reference to the foreign domain page.
> * Reference will be released in XENMEM_remove_from_physmap */
> -page = get_page_from_gfn(od, idx, &p2mt, P2M_ALLOC);
> +page = get_page_from_gfn(od, _gfn(idx), &p2mt, P2M_ALLOC);
> if ( !page )
> {
> put_pg_owner(od);
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index 8a4f753eae..4d8f153031 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -607,7 +607,7 @@ static int pvpmu_init(struct domain *d,
> xen_pmu_params_t *params)
> struct vcpu *v;
> struct vpmu_struct *vpmu;
> struct page_info *page;
> -uint64_t gfn = params->val;
> +gfn_t gfn = _gfn(params->val);
>
> if ( (params->vcpu >= d->max_vcpus) || (d->vcpu[params->vcpu] ==
> NULL) )
> return -EINVAL;
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f6fe954313..c5cce4b38d 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -797,7 +797,7 @@ int arch_set_info_guest(
> unsigned long flags;
> bool compat;
> #ifdef CONFIG_PV
> -unsigned long cr3_gfn;
> +gfn_t cr3_gfn;
> struct page_info *cr3_page;
> unsigned long cr4;
> int rc = 0;
> @@ -1061,9 +1061,9 @@ int arch_set_info_guest(
> set_bit(_VPF_in_reset, &v->pause_flags);
>
> if ( !compat )
> -cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
> +cr3_gfn = _gfn(xen_cr3_to_pfn(c.nat->ctrlreg[3]));
> else
> -cr3_gfn = compat_cr3_to_pfn(c.cmp->ctrlreg[3]);
> +cr3_gfn = _gfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3]));
> cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
>
> if ( !cr3_page )
> @@ -1092,7 +1092,7 @@ int arch_set_info_guest(
> case 0:
> if ( !compat && !VM_ASSIST(d, m2p_strict) &&
> !paging_mode_refcounts(d) )
> -fill_ro_mpt(_mfn(cr3_gfn));
> +fill_ro_mpt(_mfn(gfn_x(cr3_gfn)));
> break;
> default:
> if ( cr3_page == current->arch.old_guest_table )
> @@ -1107,7 +1107,7 @@ int arch_set_info_guest(
> v->arch.guest_table = pagetable_from_page(cr3_page);
> if ( c.nat->ctrlreg[1] )
> {
> -cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[1]);
> +cr3_gfn = _gfn(xen_cr3
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
On 07/11/18 08:57, Paul Durrant wrote:
>>
>>> static inline struct page_info *get_page_from_gfn(
>>> -struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>> +struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
>>> {
>>> struct page_info *page;
>>> +mfn_t mfn;
>>>
>>> if ( paging_mode_translate(d) )
>>> -return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t,
>> NULL, q);
>>> +return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL,
>> q);
>>> /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
>>> if ( t )
>>> *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
>>> -page = mfn_to_page(_mfn(gfn));
>>> -return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
>>> +
>>> +mfn = _mfn(gfn_x(gfn));
>>> +page = mfn_to_page(mfn);
>>> +return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
>> This looks like it would be cleaner by not splitting mfn out into a
>> separate variable.
>>
>> page = mfn_to_page(_mfn(gfn_x(gfn)));
>>
>> return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
> ^^ er... how's that mfn_valid() going to work? You'd need
> mfn_valid(page_to_mfn(pahge)), or somesuch.
Oops - I'm blind. Sorry for the noise.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
Hi Andrew, On 06/11/2018 20:11, Andrew Cooper wrote: This looks like it would be cleaner by not splitting mfn out into a separate variable. page = mfn_to_page(_mfn(gfn_x(gfn))); return mfn_valid(mfn) && get_page(page, d) ? page : NULL; The only reason this looks odd is because of the mfn => gfn equality, but we are just beside a comment explaining that we are non-translated. I introduced the mfn variable to avoid duplicating _mfn(gfn_x(gfn)) in the two places where gfn was used. I am happy to duplicated if you prefer. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
Hi Paul,
On 07/11/2018 09:24, Paul Durrant wrote:
-Original Message-
From: Julien Grall [mailto:julien.gr...@arm.com]
Sent: 06 November 2018 19:15
To: sstabell...@kernel.org; xen-devel@lists.xenproject.org
Cc: Julien Grall ; Andrew Cooper
; George Dunlap ; Ian
Jackson ; Jan Beulich ; Konrad
Rzeszutek Wilk ; Tim (Xen.org) ; Wei
Liu ; Boris Ostrovsky ;
Suravee Suthikulpanit ; Brian Woods
; Paul Durrant ; Jun
Nakajima ; Kevin Tian ;
Julien Grall
Subject: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use
typesafe gfn
No functional change intended.
Only reasonable clean-ups are done in this patch. The rest will use _gfn
for the time being.
Signed-off-by: Julien Grall
---
xen/arch/arm/guestcopy.c | 2 +-
xen/arch/arm/mm.c| 2 +-
xen/arch/x86/cpu/vpmu.c | 2 +-
xen/arch/x86/domain.c| 12 ++--
xen/arch/x86/domctl.c| 6 +++---
xen/arch/x86/hvm/dm.c| 2 +-
xen/arch/x86/hvm/domain.c| 2 +-
xen/arch/x86/hvm/hvm.c | 9 +
xen/arch/x86/hvm/svm/svm.c | 8
xen/arch/x86/hvm/viridian/viridian.c | 24
xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
xen/arch/x86/hvm/vmx/vvmx.c | 12 ++--
xen/arch/x86/mm.c| 24 ++--
xen/arch/x86/mm/p2m.c| 2 +-
xen/arch/x86/mm/shadow/hvm.c | 6 +++---
xen/arch/x86/physdev.c | 3 ++-
xen/arch/x86/pv/descriptor-tables.c | 5 ++---
xen/arch/x86/pv/emul-priv-op.c | 6 +++---
xen/arch/x86/pv/mm.c | 2 +-
xen/arch/x86/traps.c | 11 ++-
xen/common/domain.c | 2 +-
xen/common/event_fifo.c | 12 ++--
xen/common/memory.c | 4 ++--
xen/common/tmem_xen.c| 2 +-
xen/include/asm-arm/p2m.h| 6 +++---
xen/include/asm-x86/p2m.h| 11 +++
26 files changed, 95 insertions(+), 86 deletions(-)
[snip]
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 5d00256aaa..a7419bd444 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -317,7 +317,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct
hvm_hw_cpu *c)
{
if ( c->cr0 & X86_CR0_PG )
{
-page = get_page_from_gfn(v->domain, c->cr3 >> PAGE_SHIFT,
+page = get_page_from_gfn(v->domain, gaddr_to_gfn(c->cr3),
NULL, P2M_ALLOC);
if ( !page )
{
@@ -2412,9 +2412,9 @@ nsvm_get_nvmcb_page(struct vcpu *v, uint64_t
vmcbaddr)
return NULL;
/* Need to translate L1-GPA to MPA */
-page = get_page_from_gfn(v->domain,
-nv->nv_vvmcxaddr >> PAGE_SHIFT,
-&p2mt, P2M_ALLOC | P2M_UNSHARE);
+page = get_page_from_gfn(v->domain,
+ gaddr_to_gfn(nv->nv_vvmcxaddr >>
PAGE_SHIFT),
Don't you need to lose the '>> PAGE_SHIFT' now?
Yes. Brian reported on IRC and now it is fixed.
Thank you for the review.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
> -Original Message-
> From: Julien Grall [mailto:julien.gr...@arm.com]
> Sent: 06 November 2018 19:15
> To: sstabell...@kernel.org; xen-devel@lists.xenproject.org
> Cc: Julien Grall ; Andrew Cooper
> ; George Dunlap ; Ian
> Jackson ; Jan Beulich ; Konrad
> Rzeszutek Wilk ; Tim (Xen.org) ; Wei
> Liu ; Boris Ostrovsky ;
> Suravee Suthikulpanit ; Brian Woods
> ; Paul Durrant ; Jun
> Nakajima ; Kevin Tian ;
> Julien Grall
> Subject: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use
> typesafe gfn
>
> No functional change intended.
>
> Only reasonable clean-ups are done in this patch. The rest will use _gfn
> for the time being.
>
> Signed-off-by: Julien Grall
> ---
> xen/arch/arm/guestcopy.c | 2 +-
> xen/arch/arm/mm.c| 2 +-
> xen/arch/x86/cpu/vpmu.c | 2 +-
> xen/arch/x86/domain.c| 12 ++--
> xen/arch/x86/domctl.c| 6 +++---
> xen/arch/x86/hvm/dm.c| 2 +-
> xen/arch/x86/hvm/domain.c| 2 +-
> xen/arch/x86/hvm/hvm.c | 9 +
> xen/arch/x86/hvm/svm/svm.c | 8
> xen/arch/x86/hvm/viridian/viridian.c | 24
> xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
> xen/arch/x86/hvm/vmx/vvmx.c | 12 ++--
> xen/arch/x86/mm.c| 24 ++--
> xen/arch/x86/mm/p2m.c| 2 +-
> xen/arch/x86/mm/shadow/hvm.c | 6 +++---
> xen/arch/x86/physdev.c | 3 ++-
> xen/arch/x86/pv/descriptor-tables.c | 5 ++---
> xen/arch/x86/pv/emul-priv-op.c | 6 +++---
> xen/arch/x86/pv/mm.c | 2 +-
> xen/arch/x86/traps.c | 11 ++-
> xen/common/domain.c | 2 +-
> xen/common/event_fifo.c | 12 ++--
> xen/common/memory.c | 4 ++--
> xen/common/tmem_xen.c| 2 +-
> xen/include/asm-arm/p2m.h| 6 +++---
> xen/include/asm-x86/p2m.h| 11 +++
> 26 files changed, 95 insertions(+), 86 deletions(-)
>
[snip]
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 5d00256aaa..a7419bd444 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -317,7 +317,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct
> hvm_hw_cpu *c)
> {
> if ( c->cr0 & X86_CR0_PG )
> {
> -page = get_page_from_gfn(v->domain, c->cr3 >> PAGE_SHIFT,
> +page = get_page_from_gfn(v->domain, gaddr_to_gfn(c->cr3),
> NULL, P2M_ALLOC);
> if ( !page )
> {
> @@ -2412,9 +2412,9 @@ nsvm_get_nvmcb_page(struct vcpu *v, uint64_t
> vmcbaddr)
> return NULL;
>
> /* Need to translate L1-GPA to MPA */
> -page = get_page_from_gfn(v->domain,
> -nv->nv_vvmcxaddr >> PAGE_SHIFT,
> -&p2mt, P2M_ALLOC | P2M_UNSHARE);
> +page = get_page_from_gfn(v->domain,
> + gaddr_to_gfn(nv->nv_vvmcxaddr >>
> PAGE_SHIFT),
Don't you need to lose the '>> PAGE_SHIFT' now?
Paul
> + &p2mt, P2M_ALLOC | P2M_UNSHARE);
> if ( !page )
> return NULL;
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
> -Original Message-
> From: Andrew Cooper
> Sent: 06 November 2018 20:12
> To: Julien Grall ; sstabell...@kernel.org; xen-
> de...@lists.xenproject.org
> Cc: George Dunlap ; Ian Jackson
> ; Jan Beulich ; Konrad
> Rzeszutek Wilk ; Tim (Xen.org) ; Wei
> Liu ; Boris Ostrovsky ;
> Suravee Suthikulpanit ; Brian Woods
> ; Paul Durrant ; Jun
> Nakajima ; Kevin Tian ;
> Julien Grall
> Subject: Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use
> typesafe gfn
>
> Hi - just some cosmetic suggestions.
>
> Subject s/Swich/Switch/
>
> > diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-
> op.c
> > index f73ea4a163..a529ebcc3f 100644
> > --- a/xen/arch/x86/pv/emul-priv-op.c
> > +++ b/xen/arch/x86/pv/emul-priv-op.c
> > @@ -760,12 +760,12 @@ static int write_cr(unsigned int reg, unsigned
> long val,
> > case 3: /* Write CR3 */
> > {
> > struct domain *currd = curr->domain;
> > -unsigned long gfn;
> > +gfn_t gfn;
> > struct page_info *page;
> > int rc;
> >
> > -gfn = !is_pv_32bit_domain(currd)
> > - ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val);
> > +gfn = _gfn(!is_pv_32bit_domain(currd)
> > + ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val));
>
> Please re-indent.
>
> > page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
> > if ( !page )
> > break;
> > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > index 9471d89022..d967e49432 100644
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -817,13 +817,14 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx,
> uint64_t val)
> >
> > if ( p2m_is_paging(t) )
> > {
> > -p2m_mem_paging_populate(d, gmfn);
> > +p2m_mem_paging_populate(d, gfn_x(gfn));
> > return X86EMUL_RETRY;
> > }
> >
> > gdprintk(XENLOG_WARNING,
> > - "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
> > - gmfn, mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN), base);
> > + "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn") to MSR
> %08x\n",
>
> GMFN => GFN.
>
> > + gfn_x(gfn), mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN),
> > + base);
> > return X86EMUL_EXCEPTION;
> > }
> >
> > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> > index d08c595887..db1ec37610 100644
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -489,18 +489,21 @@ struct page_info *p2m_get_page_from_gfn(struct
> p2m_domain *p2m, gfn_t gfn,
> > p2m_query_t q);
> >
> > static inline struct page_info *get_page_from_gfn(
> > -struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> > +struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
> > {
> > struct page_info *page;
> > +mfn_t mfn;
> >
> > if ( paging_mode_translate(d) )
> > -return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t,
> NULL, q);
> > +return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL,
> q);
> >
> > /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
> > if ( t )
> > *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
> > -page = mfn_to_page(_mfn(gfn));
> > -return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
> > +
> > +mfn = _mfn(gfn_x(gfn));
> > +page = mfn_to_page(mfn);
> > +return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
>
> This looks like it would be cleaner by not splitting mfn out into a
> separate variable.
>
> page = mfn_to_page(_mfn(gfn_x(gfn)));
>
> return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
^^ er... how's that mfn_valid() going to work? You'd need
mfn_valid(page_to_mfn(pahge)), or somesuch.
Paul
>
> The only reason this looks odd is because of the mfn => gfn equality,
> but we are just beside a comment explaining that we are non-translated.
>
> ~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn
Hi - just some cosmetic suggestions.
Subject s/Swich/Switch/
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index f73ea4a163..a529ebcc3f 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -760,12 +760,12 @@ static int write_cr(unsigned int reg, unsigned long val,
> case 3: /* Write CR3 */
> {
> struct domain *currd = curr->domain;
> -unsigned long gfn;
> +gfn_t gfn;
> struct page_info *page;
> int rc;
>
> -gfn = !is_pv_32bit_domain(currd)
> - ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val);
> +gfn = _gfn(!is_pv_32bit_domain(currd)
> + ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val));
Please re-indent.
> page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
> if ( !page )
> break;
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 9471d89022..d967e49432 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -817,13 +817,14 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx,
> uint64_t val)
>
> if ( p2m_is_paging(t) )
> {
> -p2m_mem_paging_populate(d, gmfn);
> +p2m_mem_paging_populate(d, gfn_x(gfn));
> return X86EMUL_RETRY;
> }
>
> gdprintk(XENLOG_WARNING,
> - "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
> - gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN),
> base);
> + "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn") to MSR %08x\n",
GMFN => GFN.
> + gfn_x(gfn), mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN),
> + base);
> return X86EMUL_EXCEPTION;
> }
>
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index d08c595887..db1ec37610 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -489,18 +489,21 @@ struct page_info *p2m_get_page_from_gfn(struct
> p2m_domain *p2m, gfn_t gfn,
> p2m_query_t q);
>
> static inline struct page_info *get_page_from_gfn(
> -struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> +struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
> {
> struct page_info *page;
> +mfn_t mfn;
>
> if ( paging_mode_translate(d) )
> -return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t, NULL,
> q);
> +return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL, q);
>
> /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
> if ( t )
> *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
> -page = mfn_to_page(_mfn(gfn));
> -return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
> +
> +mfn = _mfn(gfn_x(gfn));
> +page = mfn_to_page(mfn);
> +return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
This looks like it would be cleaner by not splitting mfn out into a
separate variable.
page = mfn_to_page(_mfn(gfn_x(gfn)));
return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
The only reason this looks odd is because of the mfn => gfn equality,
but we are just beside a comment explaining that we are non-translated.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
