On Wed, Mar 25, 2020 at 11:16 AM Roger Pau Monné <roger....@citrix.com> wrote:
>
> On Wed, Mar 25, 2020 at 11:00:05AM -0600, Tamas K Lengyel wrote:
> > On Wed, Mar 25, 2020 at 10:52 AM Julien Grall <jul...@xen.org> wrote:
> > >
> > >
> > >
> > > On 25/03/2020 16:47, Tamas K Lengyel wrote:
> > > > On Wed, Mar 25, 2020 at 10:42 AM Julien Grall <jul...@xen.org> wrote:
> > > >>
> > > >> Hi,
> > > >>
> > > >> On 25/03/2020 16:34, Tamas K Lengyel wrote:
> > > >>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > > >>>>> index 9f51370327..1ed7d13084 100644
> > > >>>>> --- a/xen/arch/x86/mm/p2m.c
> > > >>>>> +++ b/xen/arch/x86/mm/p2m.c
> > > >>>>> @@ -509,6 +509,12 @@ mfn_t __get_gfn_type_access(struct p2m_domain
> > > >>>>> *p2m, unsigned long gfn_l,
> > > >>>>>
> > > >>>>> mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
> > > >>>>>
> > > >>>>> + /* Check if we need to fork the page */
> > > >>>>> + if ( (q & P2M_ALLOC) && p2m_is_hole(*t) &&
> > > >>>>> + !mem_sharing_fork_page(p2m->domain, gfn, q & P2M_UNSHARE)
> > > >>>>> )
> > > >>>>> + mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
> > > >>>>> +
> > > >>>>> + /* Check if we need to unshare the page */
> > > >>>>> if ( (q & P2M_UNSHARE) && p2m_is_shared(*t) )
> > > >>>>> {
> > > >>>>> ASSERT(p2m_is_hostp2m(p2m));
> > > >>>>> @@ -588,7 +594,8 @@ struct page_info *p2m_get_page_from_gfn(
> > > >>>>> return page;
> > > >>>>>
> > > >>>>> /* Error path: not a suitable GFN at all */
> > > >>>>> - if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) &&
> > > >>>>> !p2m_is_pod(*t) )
> > > >>>>> + if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) &&
> > > >>>>> !p2m_is_pod(*t) &&
> > > >>>>> + !mem_sharing_is_fork(p2m->domain) )
> > > >>>>> return NULL;
> > > >>>>> }
> > > >>>>>
> > > >>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
> > > >>>>> index b4eb476a9c..62aed53a16 100644
> > > >>>>> --- a/xen/common/domain.c
> > > >>>>> +++ b/xen/common/domain.c
> > > >>>>> @@ -1270,6 +1270,9 @@ int map_vcpu_info(struct vcpu *v, unsigned
> > > >>>>> long gfn, unsigned offset)
> > > >>>>>
> > > >>>>> v->vcpu_info = new_info;
> > > >>>>> v->vcpu_info_mfn = page_to_mfn(page);
> > > >>>>> +#ifdef CONFIG_MEM_SHARING
> > > >>>>> + v->vcpu_info_offset = offset;
> > > >>>>
> > > >>>> There's no need to introduce this field, you can just use
> > > >>>> v->vcpu_info
> > > >>>> & ~PAGE_MASK AFAICT.
> > > >>>
> > > >>> Just doing what you suggest above results in:
> > > >>>
> > > >>> mem_sharing.c:1603:55: error: invalid operands to binary & (have
> > > >>> ‘vcpu_info_t * const’ {aka ‘union <anonymous> * const’} and ‘long
> > > >>> int’)
> > > >>> d_vcpu->vcpu_info &
> > > >>> ~PAGE_MASK);
> > > >>>
> > > >>> I can of course cast the vcpu_info pointer to (long int), it's just a
> > > >>> bit ugly. Thoughts?
> > > >>
> > > >> FWIW, I will also need the offset for liveupdate. I have used (unsigned
> > > >> long)v->vcpu_info & ~PAGE_MASK so far but this is not really pretty.
> > > >>
> > > >> So I am all for either a new field or a macro hiding this uglyness.
> > > >
> > > > A macro sounds like a good way to go, no need for an extra field if we
> > > > can calculate it based on the currently existing one. How about
> > > >
> > > > #define VCPU_INFO_OFFSET(v) (((unsigned long)v->vcpu_info) & ~PAGE_MASK)
> > >
> > > I was more thinking a generic macro to find the offset in a page.
> > >
> > > PAGE_OFFSET(ptr) ((unsigned long)(ptr) & ~PAGE_MASK)
> >
> > LGTM. Should we stuff this into xen/sched.h or asm/page.h?
>
> page.h would be better, albeit you will have to duplicate it for x86
> and Arm. There's xen/pfn.h which is not arch specific, but feels a bit
> weird to place it there.
>
I'll go with page.h, that allows us to use vaddr_t for the ARM macro.
Tamas
