Re: [PATCH V5] xen/gnttab: Store frame GFN in struct page_info on Arm

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 10/02/2022 16:55, Oleksandr wrote:
> On 10.02.22 11:46, Julien Grall wrote:
> > On 08/02/2022 19:50, Oleksandr Tyshchenko wrote:
> > > On 08.02.22 13:58, Julien Grall wrote:
> > > Below the diff I have locally:
> > >
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index 5646343..90d7563 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -1315,11 +1315,32 @@ static inline int p2m_remove_mapping(struct
> > > domain *d,
> > >                                         mfn_t mfn)
> > >    {
> > >        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > +    unsigned long i;
> > >        int rc;
> > >
> > >        p2m_write_lock(p2m);
> > > +    for ( i = 0; i < nr; )
> > > +    {
> > > +        unsigned int cur_order;
> > > +        bool valid;
> > > +        mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
> > > NULL, NULL,
> > > +                                         &cur_order, &valid); > +
> > > +        if ( valid &&
> > valid is a copy of the LPAE bit valid. This may be 0 if Xen decided to 
> > clear it (i.e when emulating set/way). Yet the mapping itself is 
> > considered valid from Xen PoV.
> > So you want to replace with a different check (see below).
>
> Hmm, I got it, so ...
>
>
> > > +             (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), 
> > > mfn_return)) )
> > > +        {
> > > +            rc = -EILSEQ;
> > > +            goto out;
> > > +        }
> > > +
> > > +        i += (1UL << cur_order) -
> > > +             ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
> > > +    }
> > > +
> > >        rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
> > >                           p2m_invalid, p2m_access_rwx);
> > > +
> > > +out:
> > >        p2m_write_unlock(p2m);
> > >
> > >        return rc;
> > >
> > >
> > > Could you please clarify, is it close to what you had in mind? If yes, I
> > > am wondering, don't we need this check to be only executed for xenheap
> > > pages (and, probably, which P2M's entry type in RAM?) rather than for
> > > all pages?
> > From my understanding, for the purpose of this work, we only strictly 
> > need to check that for xenheap pages.
>
>   ... yes, but ...
>
>
> > But I think it would be a good opportunity to harden the P2M code. At 
> > the moment, on Arm, you can remove any mappings you want (even with 
> > the wrong helpers). This lead us to a few issues when mapping were 
> > overriden silently (in particular when building dom0).
> > So I would say we should enforce it for every RAM mapping. 
>
> ... I think this makes sense, so the proper check in that case, I 
> assume, should contain p2m_is_any_ram() macro:

Correct, p2m_is_any_ram() looks the macro we want to use here.

> > Note that, I would like to see this change in a separate commit. It 
> > will be easier to review.
>
> ok, I will introduce this check by a separate patch.
Thank you!

[...]

> > > It is going to be a non-protected write to GFN portion of type_info.
> > Well no. You are using a Read-Modify-Write operation on type_info. 
> > This is not atomic and will overwrite any change (if any) done on 
> > other part of the type_info.
>
> I am confused a bit, to which my comment your comment above belongs (to 
> the diff for page_set_xenheap_gfn() above or to sentence right after it)?
> The "It is going to be a non-protected write to GFN portion of 
> type_info." sentence is related to the diff for alloc_heap_pages() 
> below. Sorry if I didn't separate the comments properly.
Ok. So it will be a write operation, but I still don't understand why 
you think it is just the GFN portion.
The code is using "...type_info = PGT_TYPE_INFO_INITIALIZER". So the 
full 64-bit (assuming arm64) will be modified.
In general, the GFN takes 60 of the 64-bits. So any time you need to 
modify the GFN (or the count_info), you will end up to modify the entire 
of type_info (and vice-versa). If this is becoming we problem (e.g. the 
count_info is actively used) we will need to use cmpxchg() to modify the 
GFN portion.
Cheers,

--
Julien Grall