Re: [Xen-devel] [PATCH 1/2] x86/mm: factor out the code for shattering an l3 PTE

["Xia, Hongyan" <hongyxia@xxxxxxxxxx>]

On Mon, 2019-12-09 at 14:22 +0100, Jan Beulich wrote:
> On 07.12.2019 19:20, Xia, Hongyan wrote:
> > On Fri, 2019-12-06 at 17:50 +0000, Andrew Cooper wrote:
> > > On 06/12/2019 15:53, Hongyan Xia wrote:
> > > > +/* Shatter an l3 entry and populate l2. If virt is passed in,
> > > > also
> > > > do flush. */
> > > > +static void shatter_l3e(l3_pgentry_t *pl3e, l2_pgentry_t *l2t,
> > > > +        unsigned long virt, bool locking)
> > > > +{
> > > > +    unsigned int i;
> > > > +    l3_pgentry_t ol3e = *pl3e;
> > > > +
> > > > +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> > > > +        l2e_write(l2t + i,
> > > > +                  l2e_from_pfn(l3e_get_pfn(ol3e) +
> > > > +                               (i << PAGETABLE_ORDER),
> > > > +                               l3e_get_flags(ol3e)));
> > > 
> > > The PTE macros are especially poor for generated asm, and in
> > > cases
> > > like
> > > this, I'd like to improve things.
> > > 
> > > In particular, I believe the following code has identical
> > > behaviour:
> > > 
> > > l2_pgentry_t nl2e = l2e_from_intpte(l3e_get_intpte(ol3e));
> > > 
> > > for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, nl2e.l2 +=
> > > PAGETABLE_ORDER )
> > >     l2e_write(l2t + i, nl2e);
> > > 
> > > (although someone please double check my logic) and rather better
> > > asm
> > > generation.  (I also expect there to be some discussion on
> > > whether
> > > using
> > > n2le.l2 directly is something we'd want to start doing.)
> > > 
> > 
> > I believe it should be nl2e.l2 += 1<<(PAGETABLE_ORDER+PAGE_SHIFT) ?
> 
> Indeed.
> 
> > Although the code rarely touches the field (.l2) directly, so maybe
> > use
> > the macros (get_intpte -> add -> from_intpte) for consistency? They
> > should produce the same code if the compiler is not too stupid.
> 
> I think the to/from intpte transformations should be used sparingly
> too (as being dangerous). How about we make PTEs proper unions, with
> a full-field intpte_t as we have it now combined with a set of bit
> fields? This would at least eliminate the need for using PAGE_SHIFT
> in constructs like the above.

I can see this makes the code look much nicer. One concern I have is
that Andrew's suggestion was to improve the generated assembly code,
and using packed bit fields may generate even more masking and bit
shifting, which in the end might give us more assembly code than before
the refactoring.

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