Re: [Xen-devel] [RFC 16/16] xen/arm: Track page accessed between batch of Set/Way operations

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 5 Nov 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/5/18 9:35 PM, Stefano Stabellini wrote:
> > On Mon, 8 Oct 2018, Julien Grall wrote:
> > > At the moment, the implementation of Set/Way operations will go through
> > > all the entries of the guest P2M and flush them. However, this is very
> > > expensive and may render unusable a guest OS using them.
> > > 
> > > For instance, Linux 32-bit will use Set/Way operations during secondary
> > > CPU bring-up. As the implementation is really expensive, it may be
> > > possible
> > > to hit the CPU bring-up timeout.
> > > 
> > > To limit the Set/Way impact, we track what pages has been of the guest
> > > has been accessed between batch of Set/Way operations. This is done
> > > using bit[0] (aka valid bit) of the P2M entry.
> > 
> > This is going to improve performance of ill-mannered guests at the cost
> > of hurting performance of well-mannered guests. Is it really a good
> > trade-off? Should this behavior at least be configurable with a Xen
> > command line?
> 
> Well, we have the choice between not been able to boot Linux 32-bit anymore or
> have a slight impact at the boot time for all guests.

Wait -- I thought that with the set/way emulation introduced by patch
#15 we would be able to boot Linux 32-bit already. This patch is a
performance improvement. Or is it actually needed to boot Linux 32-bit?


> As you may have noticed the command line is been suggested below. I didn't yet
> implemented as we agreed at Connect it would be good to start getting feedback
> on it.

Sure. I was thinking about this -- does it make sense to have different
defaults for 32bit and 64bit guests?

32bit -> default p2m_invalidate_root
64bit -> default not


> > 
> > > This patch adds a new per-arch helper is introduced to perform actions
> > > just
> > > before the guest is first unpaused. This will be used to invalidate the
> > > P2M to track access from the start of the guest.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > > 
> > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > Cc: Julien Grall <julien.grall@xxxxxxx>
> > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > Cc: Tim Deegan <tim@xxxxxxx>
> > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > ---
> > >   xen/arch/arm/domain.c       | 14 ++++++++++++++
> > >   xen/arch/arm/domain_build.c |  7 +++++++
> > >   xen/arch/arm/p2m.c          | 32 +++++++++++++++++++++++++++++++-
> > >   xen/arch/x86/domain.c       |  4 ++++
> > >   xen/common/domain.c         |  5 ++++-
> > >   xen/include/asm-arm/p2m.h   |  2 ++
> > >   xen/include/xen/domain.h    |  2 ++
> > >   7 files changed, 64 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > index feebbf5a92..f439f4657a 100644
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -738,6 +738,20 @@ int arch_domain_soft_reset(struct domain *d)
> > >       return -ENOSYS;
> > >   }
> > >   +void arch_domain_creation_finished(struct domain *d)
> > > +{
> > > +    /*
> > > +     * To avoid flushing the whole guest RAM on the first Set/Way, we
> > > +     * invalidate the P2M to track what has been accessed.
> > > +     *
> > > +     * This is only turned when IOMMU is not used or the page-table are
> > > +     * not shared because bit[0] (e.g valid bit) unset will result
> > > +     * IOMMU fault that could be not fixed-up.
> > > +     */
> > > +    if ( !iommu_use_hap_pt(d) )
> > > +        p2m_invalidate_root(p2m_get_hostp2m(d));
> > > +}
> > > +
> > >   static int is_guest_pv32_psr(uint32_t psr)
> > >   {
> > >       switch (psr & PSR_MODE_MASK)
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index f552154e93..de96516faa 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -2249,6 +2249,13 @@ int __init construct_dom0(struct domain *d)
> > >       v->is_initialised = 1;
> > >       clear_bit(_VPF_down, &v->pause_flags);
> > >   +    /*
> > > +     * XXX: We probably want a command line option to invalidate or not
> > > +     * the P2M. This is because invalidating the P2M will not work with
> > > +     * IOMMU, however if this is not done there will be an impact.
> > 
> > Why can't we check on iommu_use_hap_pt(d) like in
> > arch_domain_creation_finished?
> > 
> > In any case, I agree it is a good idea to introduce a command line
> > parameter to toggle the p2m_invalidate_root call at domain creation
> > on/off. There are cases where it should be off even if an IOMMU is
> > present.
> 
> I actually forgot to remove that code as Dom0 should be covered by the change
> below.

Makes sense now


> I am not entirely to understand your last sentence, this feature is turned off
> when an IOMMU is present. So what is your use case here?
 
My sentence was badly written, sorry. I meant to say that even when an
IOMMU is NOT present, there are cases where we might not want to call
p2m_invalidate_root.

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