From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
xen-devel@lists.xenproject.org, bertrand.marquis@arm.com,
Julien Grall <julien.grall@arm.com>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()
Date: Mon, 30 Nov 2020 14:05:25 -0800 (PST) [thread overview]
Message-ID: <alpine.DEB.2.21.2011301359290.1100@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <d02e29cb-a4f1-4ebe-a04f-67b4a159a193@xen.org>
On Sat, 28 Nov 2020, Julien Grall wrote:
> Hi Stefano,
>
> On 24/11/2020 00:25, Stefano Stabellini wrote:
> > On Mon, 23 Nov 2020, Julien Grall wrote:
> > > Hi Stefano,
> > >
> > > On 23/11/2020 22:27, Stefano Stabellini wrote:
> > > > On Fri, 20 Nov 2020, Julien Grall wrote:
> > > > > > > /*
> > > > > > > * For arm32, page-tables are different on each CPUs. Yet,
> > > > > > > they
> > > > > > > share
> > > > > > > @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long
> > > > > > > virt,
> > > > > > > spin_lock(&xen_pt_lock);
> > > > > > > - for ( ; addr < addr_end; addr += PAGE_SIZE )
> > > > > > > + while ( left )
> > > > > > > {
> > > > > > > - rc = xen_pt_update_entry(root, addr, mfn, flags);
> > > > > > > + unsigned int order;
> > > > > > > + unsigned long mask;
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * Don't take into account the MFN when removing mapping
> > > > > > > (i.e
> > > > > > > + * MFN_INVALID) to calculate the correct target order.
> > > > > > > + *
> > > > > > > + * XXX: Support superpage mappings if nr is not aligned
> > > > > > > to a
> > > > > > > + * superpage size.
> > > > > >
> > > > > > It would be good to add another sentence to explain that the checks
> > > > > > below are simply based on masks and rely on the mfn, vfn, and also
> > > > > > nr_mfn to be superpage aligned. (It took me some time to figure it
> > > > > > out.)
> > > > >
> > > > > I am not sure to understand what you wrote here. Could you suggest a
> > > > > sentence?
> > > >
> > > > Something like the following:
> > > >
> > > > /*
> > > > * Don't take into account the MFN when removing mapping (i.e
> > > > * MFN_INVALID) to calculate the correct target order.
> > > > *
> > > > * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
> > > > * aligned, and it uses `mask' to check for that.
> > >
> > > Unfortunately, I am still not sure to understand this comment.
> > > The loop can deal with any (super)page size (4KB, 2MB, 1GB). There are no
> > > assumption on any alignment for mfn, vfn and nr_mfn.
> > >
> > > By OR-ing the 3 components together, we can use it to find out the maximum
> > > size that can be used for the mapping.
> > >
> > > So can you clarify what you mean?
> >
> > In pseudo-code:
> >
> > mask = mfn | vfn | nr_mfns;
> > if (mask & ((1<<FIRST_ORDER) - 1))
> > if (mask & ((1<<SECOND_ORDER) - 1))
> > if (mask & ((1<<THIRD_ORDER) - 1))
> > ...
> >
> > As you wrote the mask is used to find the max size that can be used for
> > the mapping.
> >
> > But let's take nr_mfns out of the equation for a moment for clarity:
> >
> > mask = mfn | vfn;
> > if (mask & ((1<<FIRST_ORDER) - 1))
> > if (mask & ((1<<SECOND_ORDER) - 1))
> > if (mask & ((1<<THIRD_ORDER) - 1))
> > ...
> >
> > How would you describe this check? I'd call this an alignment check,
> > is it not?
> If you take the ``if`` alone, yes they are alignment check. But if you take
> the overall code, then it will just compute which mapping size can be used.
>
> However, what I am disputing here is "rely" because there are no assumption
> made on the alignment in the loop (we are able to cater any size). In fact,
> the fact mfn and vfn should be aligned to the mapping size is a requirement
> from the hardware and not the implementation.
OK, maybe the "rely" gives a bad impression. What about:
This loop relies on mfn, vfn, and nr_mfn, to be all superpage aligned
(mfn and vfn have to be architecturally), and it uses `mask' to check
for that.
Feel free to reword it differently if you have a better idea.
next prev parent reply other threads:[~2020-11-30 22:05 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-19 19:07 [PATCH RFC 0/6] xen/arm: mm: Add limited support for superpages Julien Grall
2020-11-19 19:07 ` [PATCH RFC 1/6] xen/arm: mm: Remove special case for CPU0 in dump_hyp_walk() Julien Grall
2020-11-24 17:10 ` Bertrand Marquis
2020-11-28 11:14 ` Julien Grall
2020-11-30 21:58 ` Stefano Stabellini
2020-11-19 19:07 ` [PATCH RFC 2/6] xen/arm: mm: Remove ; at the end of mm_printk() Julien Grall
2020-11-20 0:41 ` Stefano Stabellini
2020-11-24 12:19 ` Bertrand Marquis
2020-11-19 19:07 ` [PATCH RFC 3/6] xen/arm: setup: Call unregister_init_virtual_region() after the last init function Julien Grall
2020-11-24 13:25 ` Bertrand Marquis
2020-11-19 19:07 ` [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry() Julien Grall
2020-11-20 1:46 ` Stefano Stabellini
2020-11-20 16:09 ` Julien Grall
2020-11-23 22:27 ` Stefano Stabellini
2020-11-23 23:23 ` Julien Grall
2020-11-24 0:25 ` Stefano Stabellini
2020-11-28 11:53 ` Julien Grall
2020-11-30 22:05 ` Stefano Stabellini [this message]
2021-04-25 15:11 ` Julien Grall
2020-11-24 18:13 ` Bertrand Marquis
2020-11-25 18:03 ` Julien Grall
2020-11-19 19:07 ` [PATCH RFC 5/6] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings Julien Grall
2020-11-20 1:47 ` Stefano Stabellini
2020-11-19 19:07 ` [PATCH RFC 6/6] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen() Julien Grall
2020-11-20 1:54 ` Stefano Stabellini
2021-01-23 11:44 ` [PATCH RFC 0/6] xen/arm: mm: Add limited support for superpages Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.21.2011301359290.1100@sstabellini-ThinkPad-T480s \
--to=sstabellini@kernel.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=bertrand.marquis@arm.com \
--cc=julien.grall@arm.com \
--cc=julien@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).