Re: [Xen-devel] [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 06 March 2020 13:19
> To: Paul Durrant <xadimgnik@xxxxxxxxx>
> Cc: pdurrant@xxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Andrew Cooper' 
> <andrew.cooper3@xxxxxxxxxx>;
> 'George Dunlap' <george.dunlap@xxxxxxxxxx>; 'Wei Liu' <wl@xxxxxxx>; 'Roger 
> Pau Monné'
> <roger.pau@xxxxxxxxxx>
> Subject: Re: [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in 
> p2m_alloc_table
> 
> On 06.03.2020 14:11, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 06 March 2020 13:07
> >> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> >> Cc: pdurrant@xxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper 
> >> <andrew.cooper3@xxxxxxxxxx>;
> >> George Dunlap <george.dunlap@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau 
> >> Monné
> <roger.pau@xxxxxxxxxx>
> >> Subject: RE: [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in 
> >> p2m_alloc_table
> >>
> >> CAUTION: This email originated from outside of the organization. Do not 
> >> click links or open
> >> attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 06.03.2020 13:50, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeulich@xxxxxxxx>
> >>>> Sent: 06 March 2020 12:47
> >>>> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> >>>> Cc: pdurrant@xxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper 
> >>>> <andrew.cooper3@xxxxxxxxxx>;
> >>>> George Dunlap <george.dunlap@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger 
> >>>> Pau Monné
> >> <roger.pau@xxxxxxxxxx>
> >>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in 
> >>>> p2m_alloc_table
> >>>>
> >>>> On 06.03.2020 13:07, Durrant, Paul wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
> >>>>>> Sent: 06 March 2020 11:46
> >>>>>> To: pdurrant@xxxxxxxx
> >>>>>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul 
> >>>>>> <pdurrant@xxxxxxxxxxxx>; Andrew Cooper
> >>>>>> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; 
> >>>>>> Wei Liu <wl@xxxxxxx>;
> >> Roger
> >>>> Pau
> >>>>>> Monné <roger.pau@xxxxxxxxxx>
> >>>>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in 
> >>>>>> p2m_alloc_table
> >>>>>>
> >>>>>> On 05.03.2020 13:45, pdurrant@xxxxxxxx wrote:
> >>>>>>> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> >>>>>>>
> >>>>>>> There does not seem to be any justification for refusing to create the
> >>>>>>> domain's p2m table simply because it may have assigned pages.
> >>>>>>
> >>>>>> I think there is: If any such allocation had happened before, how
> >>>>>> would it be represented in the domain's p2m?
> >>>>>
> >>>>> Insertion into the p2m is a separate action from page allocation. Why 
> >>>>> should they be linked?
> >>>>
> >>>> They are, because of how XENMEM_populate_physmap works. Yes,
> >>>> they _could_ be separate steps, but that's only a theoretical
> >>>> consideration.
> >>>
> >>> Then surely the check should be in the XENMEM_populate_physmap code?
> >>
> >> How that? populate-physmap can be called any number of times. We
> >> can't refuse a 2nd call there just because a 1st one had happened
> >> already. Or did you mean the inverse check (i.e. that there
> >> already is a p2m)?
> >
> > Yes, I mean check the p2m has been initialized there.
> >
> >> This surely wouldn't be a bad idea, as
> >> otherwise both ept_get_entry() and p2m_pt_get_entry() would
> >> blindly map MFN 0. But adding such a check wouldn't eliminate
> >> the reason to also have the check that you're proposing to drop.
> >>
> >
> > Why not? Anywhere assuming the existence of a p2m ought to check
> > for it;
> 
> As said - I agree this wouldn't be a bad thing to do. It would
> be a requirement if paging_enable() wasn't called from
> hvm_domain_initialise(), but via a distinct domctl. But since
> it is, there's no way to invoke populate-physmap on a domain
> without its p2m root table already allocated.
> 
> > I still can't see why initialising the p2m after having allocated
> > pages (PGC_extra or otherwise) is inherently wrong.
> 
> "inherently" as in "from an abstract pov" - yes. But within the
> constraints of the hypercalls available - no. Yet what gets
> checked has to be of practical use, not just of theoretical one.
> I.e. I'd be fine to see the check go away when a viable
> alternative mechanism to allocate and _then_ populate p2m gets
> introduced.
> 

OK... it still seems like the wrong place to me, but I'll leave the check and 
simply exclude PGG_extra pages.

  Paul


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