[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 16/18] x86: introduce helper for recording degree of contiguity in page tables


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 15 Dec 2021 14:57:01 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=/ZHD19mx1v5bAytFP65czEDogI0hoGJz/gYVrGxQjyY=; b=cY3AbsRO8KRlKyXCcguw8nguOgNQj4DGRXX+oeEVue/2w8r+47tFqmuCfr8tLm7knTNa2ZLvi6JLwq/618aCGmIIQRLVumd/h2/cRQC5K1LyP5jhLYnIcTjrMvIDYpwP61p1FbH8xqCsD3vX03VawAVpYrQc7Zjt32kaCuEDFkGv2tLMxNmKFq7225uOxazkvI+whTqmg1If+7OxCkxVQZfSy8afElBG2OxM9d9SLgE9NYWF54DyY5jYR81gESVsyAZ+JVxcjSyeNW4W1m6KBTujm8pptqchoHIvygzbrQ7LDj2qs97XuViYq4s5rqB5XW+Od5HVTa8SHuVf4vgTrg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eKoX8p469MbGid/k9BHgQ0q1bm5OVYwNFdHk1ecVFdZPe99xwpXs7pKMpE4bB9t5UnCN0j/QI84XxooPlmQMp8DtqEIhXmxW4rw1C+BwPFLcB6Cm22G8VUcmAM0TSM/QsHAlAx1XDHMkDUjjfV5QGlQuq5BsTrDfyAAbGr3zrl5yKNqRSDdtEwqwZQm7mthaqaEbdqfTCLRt6UmRPTESEUk5F6CSPiRNH7yA8mfpwz9N+nKA/L50dWSZV02UwW7gOkfPtootNa2HH4O9HkBVW7tJRDdTQvlo5pDY944Mj0YfOt02F9zMHOUIlpGyuqpCGIpu2Zl3JRs9Ys/FQ/8/Rg==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Wed, 15 Dec 2021 13:57:29 +0000
  • Ironport-data: A9a23:Yisarq/ByWLeVoO1PVKJDrUDbXmTJUtcMsCJ2f8bNWPcYEJGY0x3x zMcXjuDO/2NYmT8eIwlbdi/9k0EuZGAnNIxQVQ/qSs8E34SpcT7XtnIdU2Y0wF+jyHgoOCLy +1EN7Es+ehtFie0Si9AttENlFEkvU2ybuOU5NXsZ2YhFWeIdA970Ug6wrRg39Yy6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPhq8 dBSqrCsEzsEI4eLnblHQzp7Kh1xaPguFL/veRBTsOSWxkzCNXDt3+9vHAc9OohwFuRfWD8Us 6ZCcXZUM07F17neLLGTE4GAguw5K8bmJsUHs2xIxjDFF/c2B5vERs0m4PcGhG9q2p4QRp4yY eIQZWNPURjeeydUGV41ApEhh9y2hnnGJmgwRFW9+vNsvjm7IBZK+KDkLd79atGMA8JPkS6wu Wbu72n/RBYAO7S3yzCI73atje/nhj7gVcQZE7jQ3u5nhhify3IeDDUSVECnur+ph0imQdVdJ kcIvC00osAPGFeDF4enGUfi+Tjd40BaC4E4//AGBB+l0ZPR/Bm1GjI4YyNrL9l77t8VZgUW2 Qrc9z/2PgBHvLqQQHOb076bqzKuJCQYRVM/iT84oRgtuIe6/txq5v7bZpM6SfPu0IWpcd3l6 2nS9HBWulkFsSIcO0xXF3jjiinkmJXGRxVdCu7/DjP8tVMRiGJIiuWVBbnnARRocNbxorqp5 iFsdy2iAAYmVMDleMulGrplIV1Rz6zZWAAweHY2d3Xbyxyj+mS4Yadb6yxkKUFiP64sIGGyM RKL510Buc8NZhNGiJObharrVazGKoC6SrzYug38NIISMvCdiifZlM2RWaJg9z+0yxV9+U3OE ZyabdytHR4n5VdPl1KLqxMm+eZznEgWnDqLLbiilkjP+efPPBa9FOZeWHPTP79R0U9xiFiMm zqpH5DRkEs3vSyXSnS/zLP/2nhWdyVmXs6v9JQMHgNBSyI/cFwc5zbq6epJU6RunrhPl/eO+ Xe4W0RCz0H4i2GBIgKPAk2Popu1NXqmhX5kbyEqI3iy3H0vPdSm4KsFLsNldrg77u1zi/VzS qBdKcmHB/1OTBXB+igcMsah/NAzKkzziFLcJTehbRg+Y4VkG17D9Oj7c1a97yIJFCe265cz+ uXyygPBTJMfbA1+F8KKOum3xla8sCFFyuJ/VkfFOPdJf0Do/NQ4IiD9lKZvccoNNQ/C1n2R0 APPWUUUouzEookU9tjVhP/b89f1QrUmRkcDRjvV97e7MyXe71GP+44YXbbaZy3ZWUP15L6mO bdfwcbjPaBVh11NqYd9TepmlPps+9v1qrZG5Q14B3GXPU+zA7ZtL3Taj8lCsqpBmu1QtQesA x/d/9BbPfOCOd//EU5XLw0gN7zR2fYRkzjUzPI0PESlu3MnoOvZCR1fb0uWlShQDLppK4d0k +4utfkf5xG7lhd3YM2NiTpZ9jjUI3ENO0n9Wkr23GM/ZtIX92x/
  • Ironport-hdrordr: A9a23:G/7ot6lKZma5gH4mphjNF64lHuTpDfNXiWdD5ihNYBxZY6Wkfp +V7YgmPE7P+UsssS8b6Kq90fG7MDrhHZ4c2/hhAV7QZnivhILIFvA60WKG+Vfd8kLFh5tgPM tbAtFD4ZjLfC5HZMvBkW+F+rUbsYG6GcKT9JPjJh5WJGkGGsUQiHYDe3nrbHGeBjM2cqbRfK D22iMtnUvRRZ1jVLXIOpBzZZmxmzSkruOfXfaJbyRK1OGc5gnG1JfKVzyjmjsOWTJGxrkvtU LflRbi26mlu/anjjfBym769f1t6YHc4+oGIPbJptkeKz3qhArtTp9mQae+sDc8p/zqwEo2ke PLvwwrM61Imi7slyCO0EfQMjvboWgTAkzZuA6laLzY0JzErQcBepV8bERiA0jkAgQbzYNBOe lwrimkXtJsfFn9dCaX3bb1vkZR93Zc50BSytL61xZkMbf3b9Rq3O8iFEU/KuZjIMr/g7pXdd VGHYXS4u1bfkidaG2ctm5zwMa0VnB2BRueRFMe0/bllwS+sUoJjXfw/vZv20voNahNBaVs9q DBKOBlhbtORsgZYeZ0A/oAW9K+DijITQjXOGyfLFz7HOVfUki97aLf8fEw/qWnaZYIxJw9lN DIV05Zr3c7fwbrBdeV1JNG/xjRSCG2XCjryMtZ+59l04eMA4bDIGmGUhQjgsGgq/IQDonSXO uyIotfB7v5IW7nCe9yrk7DsllpWDkjueEuy5oGsmO104P2w9fRx6Hmmd7oVfXQLQo=
  • Ironport-sdr: i/pK7wI4uPqswoz5W2tg9D+OP2NUXlyr8pioedD9LDYFZWvDv6nsdI+2hRVArD9LwH/XAHnz8n j/icTDZuG+z//OgRBdhPNqGkFOXlPg/9yUZyDtM1y6NvMwR4NTRB82MCvwQbGvnnUyclV1d0i6 7kMcW6uSuY3fS0MQL0vyH4nh1FBLNHr4shSezUGIfHW7emq8gJAId52RQUZdJNC5x7jfPY/VI+ Qp/X4lh3GUOVSJsCrwu180i0OE0P8epdx4Nsb2wFcd7eIoSxYlM3AHj1QSd7uhHrKmNStDJSsU pJkVv6ct2nKZurvPryuTpNDY
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Fri, Sep 24, 2021 at 11:55:30AM +0200, Jan Beulich wrote:
> This is a re-usable helper (kind of a template) which gets introduced
> without users so that the individual subsequent patches introducing such
> users can get committed independently of one another.
> 
> See the comment at the top of the new file. To demonstrate the effect,
> if a page table had just 16 entries, this would be the set of markers
> for a page table with fully contiguous mappings:
> 
> index  0 1 2 3 4 5 6 7 8 9 A B C D E F
> marker 4 0 1 0 2 0 1 0 3 0 1 0 2 0 1 0
> 
> "Contiguous" here means not only present entries with successively
> increasing MFNs, each one suitably aligned for its slot, but also a
> respective number of all non-present entries.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: New.
> 
> --- /dev/null
> +++ b/xen/include/asm-x86/contig-marker.h
> @@ -0,0 +1,105 @@
> +#ifndef __ASM_X86_CONTIG_MARKER_H
> +#define __ASM_X86_CONTIG_MARKER_H
> +
> +/*
> + * Short of having function templates in C, the function defined below is
> + * intended to be used by multiple parties interested in recording the
> + * degree of contiguity in mappings by a single page table.
> + *
> + * Scheme: Every entry records the order of contiguous successive entries,
> + * up to the maximum order covered by that entry (which is the number of
> + * clear low bits in its index, with entry 0 being the exception using
> + * the base-2 logarithm of the number of entries in a single page table).
> + * While a few entries need touching upon update, knowing whether the
> + * table is fully contiguous (and can hence be replaced by a higher level
> + * leaf entry) is then possible by simply looking at entry 0's marker.
> + *
> + * Prereqs:
> + * - CONTIG_MASK needs to be #define-d, to a value having at least 4
> + *   contiguous bits (ignored by hardware), before including this file,
> + * - page tables to be passed here need to be initialized with correct
> + *   markers.

Given this requirement I think it would make sense to place the page
table marker initialization currently placed in iommu_alloc_pgtable as
a helper here also?

> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/lib.h>
> +#include <xen/page-size.h>
> +
> +/* This is the same for all anticipated users, so doesn't need passing in. */
> +#define CONTIG_LEVEL_SHIFT 9
> +#define CONTIG_NR          (1 << CONTIG_LEVEL_SHIFT)
> +
> +#define GET_MARKER(e) MASK_EXTR(e, CONTIG_MASK)
> +#define SET_MARKER(e, m) \
> +    ((void)(e = ((e) & ~CONTIG_MASK) | MASK_INSR(m, CONTIG_MASK)))
> +
> +enum PTE_kind {
> +    PTE_kind_null,
> +    PTE_kind_leaf,
> +    PTE_kind_table,
> +};
> +
> +static bool update_contig_markers(uint64_t *pt, unsigned int idx,

Maybe pt_update_contig_markers, so it's not such a generic name.

> +                                  unsigned int level, enum PTE_kind kind)
> +{
> +    unsigned int b, i = idx;
> +    unsigned int shift = (level - 1) * CONTIG_LEVEL_SHIFT + PAGE_SHIFT;
> +
> +    ASSERT(idx < CONTIG_NR);
> +    ASSERT(!(pt[idx] & CONTIG_MASK));
> +
> +    /* Step 1: Reduce markers in lower numbered entries. */
> +    while ( i )
> +    {
> +        b = find_first_set_bit(i);
> +        i &= ~(1U << b);
> +        if ( GET_MARKER(pt[i]) > b )
> +            SET_MARKER(pt[i], b);
> +    }
> +
> +    /* An intermediate table is never contiguous with anything. */
> +    if ( kind == PTE_kind_table )
> +        return false;
> +
> +    /*
> +     * Present entries need in sync index and address to be a candidate
> +     * for being contiguous: What we're after is whether ultimately the
> +     * intermediate table can be replaced by a superpage.
> +     */
> +    if ( kind != PTE_kind_null &&
> +         idx != ((pt[idx] >> shift) & (CONTIG_NR - 1)) )

Don't you just need to check that the address is aligned to at least
idx, not that it's exactly aligned?

AFAICT this will return early if the address has an alignment that
exceeds the requirements imposed by idx.

> +        return false;
> +
> +    /* Step 2: Check higher numbered entries for contiguity. */
> +    for ( b = 0; b < CONTIG_LEVEL_SHIFT && !(idx & (1U << b)); ++b )
> +    {
> +        i = idx | (1U << b);
> +        if ( (kind == PTE_kind_leaf
> +              ? ((pt[i] ^ pt[idx]) & ~CONTIG_MASK) != (1ULL << (b + shift))

Maybe this could be a macro, CHECK_CONTIG or some such? It's also used
below.

I would also think this would be clearer as:

(pt[idx] & ~CONTIG_MASK) + (1ULL << (shift + b)) == (pt[i] & ~CONTIG_MASK)

> +              : pt[i] & ~CONTIG_MASK) ||

Isn't PTE_kind_null always supposed to be empty? (ie: wouldn't this
check always succeed?)

> +             GET_MARKER(pt[i]) != b )
> +            break;
> +    }
> +
> +    /* Step 3: Update markers in this and lower numbered entries. */
> +    for ( ; SET_MARKER(pt[idx], b), b < CONTIG_LEVEL_SHIFT; ++b )
> +    {
> +        i = idx ^ (1U << b);
> +        if ( (kind == PTE_kind_leaf
> +              ? ((pt[i] ^ pt[idx]) & ~CONTIG_MASK) != (1ULL << (b + shift))
> +              : pt[i] & ~CONTIG_MASK) ||
> +             GET_MARKER(pt[i]) != b )
> +            break;
> +        idx &= ~(1U << b);

There's an iteration where idx will be 0, and then there's no further
point in doing the & anymore?

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.