Re: [Xen-devel] [PATCH v6 5/9] x86/mem_sharing: use default_access in add_to_physmap

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Wed, Jan 29, 2020 at 7:56 AM Tamas K Lengyel
<tamas.k.lengyel@xxxxxxxxx> wrote:
>
> On Wed, Jan 29, 2020 at 7:44 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >
> > On 29.01.2020 15:05, Tamas K Lengyel wrote:
> > > On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >>
> > >> On 28.01.2020 18:02, Tamas K Lengyel wrote:
> > >>> On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >>>>
> > >>>> On 27.01.2020 19:06, Tamas K Lengyel wrote:
> > >>>>> When plugging a hole in the target physmap don't use the access 
> > >>>>> permission
> > >>>>> returned by __get_gfn_type_access as it can be non-sensical, leading 
> > >>>>> to
> > >>>>> spurious vm_events being sent out for access violations at unexpected
> > >>>>> locations. Make use of p2m->default_access instead.
> > >>>>
> > >>>> As before, to me "can be non-sensical" is insufficient as a reason
> > >>>> here. If it can also be a "good" value, it still remains unclear
> > >>>> why in that case p2m->default_access is nevertheless the right
> > >>>> value to use.
> > >>>
> > >>> I have already explained in the previous version of the patch why I
> > >>> said "can be". Forgot to change the commit message from "can be" to
> > >>> "is".
> > >>
> > >> Changing just the commit message would be easy while committing.
> > >> But even with the change I would ask why this is. Looking at
> > >> ept_get_entry() (and assuming p2m_pt_get_entry() will work
> > >> similarly, minus the p2m_access_t which can't come out of the
> > >> PTE just yet), I see
> > >>
> > >>     if ( is_epte_valid(ept_entry) )
> > >>     {
> > >>         *t = p2m_recalc_type(recalc || ept_entry->recalc,
> > >>                              ept_entry->sa_p2mt, p2m, gfn);
> > >>         *a = ept_entry->access;
> > >>
> > >> near its end. Which means even a hole can have its access field
> > >> set. So it's still not clear to me from the description why
> > >> p2m->default_access is uniformly the value to use. Wouldn't you
> > >> rather want to override the original value only if it's
> > >> p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not
> > >> paged-out pages)?
> > >
> > > At this point I would just rather state that add_to_physmap only works
> > > on actual holes, not with paged-out pages. In fact, I would like to
> > > see mem_paging being dropped from the codebase entirely since it's
> > > been abandoned for years and noone expressing any interest in keeping
> > > it. In the interim I would rather not spend unnecessary cycles on
> > > speculating about potential corner-cases of mem_paging when noone
> > > actually uses it.
> > >
> > >> Of course then the question is whether there
> > >> wouldn't be an ambiguity with p2m_access_n having got set
> > >> explicitly on the page. But maybe this is impossible for
> > >> p2m_invalid / p2m_mmio_dm?
> > >
> > > As far as mem_access permissions go, I don't know of any usecase that
> > > would set mem_access permission on a hole even if by looks of it it is
> > > technically possible. At this point I would rather just put this
> > > corner-case's description in a comment.
> >
> > I think I would ack a revised patch having this properly explained.
>
> That's fine, I'll add some comments to this effect and reword the
> commit message.
>

Actually, looking at the implementation of p2m_set_mem_access it's not
clear to me whether we can even have a hole with a mem_access
permission set. Can you have a "hole" type with a valid gfn? If not,
this is a non-issue since p2m_set_mem_access only sets mem_access
permissions that pass !gfn_eq(gfn, INVALID_GFN).

Tamas

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