Re: [Xen-devel] [PATCH v4 1/2] xen: Switch parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall <julien.grall@xxxxxxx>]

Hi Paul,

Thank you for the review,

On 20/08/2019 09:15, Paul Durrant wrote:
> > -----Original Message-----
> > From: Julien Grall <julien.grall@xxxxxxx>
> > Sent: 19 August 2019 15:27
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Julien Grall <julien.grall@xxxxxxx>; Stefano Stabellini 
> > <sstabellini@xxxxxxxxxx>; Volodymyr
> > Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper 
> > <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> > <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Jan Beulich 
> > <jbeulich@xxxxxxxx>;
> > Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Wei Liu 
> > <wl@xxxxxxx>;
> > Roger Pau Monne <roger.pau@xxxxxxxxxx>; Boris Ostrovsky 
> > <boris.ostrovsky@xxxxxxxxxx>; Suravee
> > Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Brian Woods 
> > <brian.woods@xxxxxxx>; Paul Durrant
> > <Paul.Durrant@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian 
> > <kevin.tian@xxxxxxxxx>
> > Subject: [PATCH v4 1/2] xen: Switch 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.
> >
> > The code in get_page_from_gfn is slightly reworked to also handle a bad
> > behavior because it is not safe to use mfn_to_page(...) because
> > mfn_valid(...) succeeds.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>
> ... with a few suggestions below ...
>
> [snip]
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 029eea3b85..236bd6ed38 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2168,7 +2168,7 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
> >   {
> >       struct vcpu *v = current;
> >       struct domain *d = v->domain;
> > -    unsigned long gfn, old_value = v->arch.hvm.guest_cr[0];
> > +    unsigned long old_value = v->arch.hvm.guest_cr[0];
> >       struct page_info *page;
> >
> >       HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR0 value = %lx", value);
> > @@ -2223,7 +2223,8 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
> >           if ( !paging_mode_hap(d) )
> >           {
> >               /* The guest CR3 must be pointing to the guest physical. */
> > -            gfn = v->arch.hvm.guest_cr[3] >> PAGE_SHIFT;
> > +            gfn_t gfn = gaddr_to_gfn(v->arch.hvm.guest_cr[3]);
> > +
> >               page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> >               if ( !page )
> >               {
> > @@ -2315,7 +2316,7 @@ int hvm_set_cr3(unsigned long value, bool may_defer)
> >       {
> >           /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
> >           HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
> > -        page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
> > +        page = get_page_from_gfn(v->domain, gaddr_to_gfn(value),
> >                                    NULL, P2M_ALLOC);
> I think you can reduce the scope of 'page' above in the same way you did with 
> 'gfn' above
For this one and ...

> >           if ( !page )
> >               goto bad_cr3;
> [snip]
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 0060310d74..38bdef0862 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -674,7 +674,7 @@ static int vmx_restore_cr0_cr3(
> >       {
> >           if ( cr0 & X86_CR0_PG )
> >           {
> > -            page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT,
> > +            page = get_page_from_gfn(v->domain, gaddr_to_gfn(cr3),
> >                                        NULL, P2M_ALLOC);
> Here also you can reduce the scope of 'page' (although only into the scope just 
> outside the 'if')
... this one, we don't change the type of the variable so I don't feel such 
clean-ups belong to this patch.
> >               if ( !page )
> >               {
> [snip]
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 7531406543..f8e2b6f921 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2083,7 +2083,7 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t 
> > nl1e,
> >               p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
> >                               P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
> >
> > -            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
> > +            page = get_page_from_gfn(pg_dom, _gfn(l1e_get_pfn(nl1e)), &p2mt, 
> > q);
> 'l1e_get_pfn(nl1e)' is repeated 3 times within this scope AFAICT so probably 
> worth a local variable while you're in the neighbourhood, to reduce verbosity.
I can only find 2 use of l1e_get_pfn within mod_l1_entry. But then, this sort of 
clean-up should be in there own patch.
But I will leave that to the x86 folks as I don't want to be roped in endless 
clean-up. I know there will be more ;).
> >               if ( p2m_is_paged(p2mt) )
> >               {
> [snip]
> > diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> > index 3a3c15890b..4f3f438614 100644
> > --- a/xen/arch/x86/physdev.c
> > +++ b/xen/arch/x86/physdev.c
> > @@ -229,7 +229,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> > arg)
> >               break;
> >
> >           ret = -EINVAL;
> > -        page = get_page_from_gfn(current->domain, info.gmfn, NULL, P2M_ALLOC);
> > +        page = get_page_from_gfn(current->domain, _gfn(info.gmfn),
> > +                                 NULL, P2M_ALLOC);
> 'currd' has actually been defined at the top of the function so if you lose the 
> 'current->domain' you can re-flow this back onto one line I think.
Sounds reasonable, but there are 3 more characters than the normal, so it will 
still have to live on 2 lines :(.
Cheers,

--
Julien Grall

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