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.
Cheers,
--
Julien Grall
