Re: [Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi all,

On 08/16/2017 12:28 AM, Andrew Cooper wrote:
> On 15/08/2017 23:25, Stefano Stabellini wrote:
>> On Tue, 15 Aug 2017, Julien Grall wrote:
>>> On 14/08/17 22:03, Sergej Proskurin wrote:
 Hi Julien,

 On 08/14/2017 07:37 PM, Julien Grall wrote:
> Hi Sergej,
>
> On 09/08/17 09:20, Sergej Proskurin wrote:
>> +/*
>> + * According to to ARM DDI 0487B.a J1-5927, we return an error if
>> the found
> Please drop one of the 'to'. The rest looks good to me.
>
 Great, thanks. I will remove the second "to" in v9. Would that be an
 Acked-by or shall I tag this patch with a Reviewed-by you?
>>> Acked-by. FIY, you still missing an acked from "The REST" for patch #7, the
>>> rest looks fully acked.
>> I acked patch #7, but patch #8 breaks the build on ARM:
>>
>>
>> In file included from 
>> /local/repos/xen-upstream/xen/include/xen/guest_access.h:10:0,
>>  from device_tree.c:15:
>> /local/repos/xen-upstream/xen/include/asm/guest_access.h:14:32: error: 
>> 'struct domain' declared inside parameter list [-Werror]
>> uint32_t size, bool_t is_write);
>> ^
>> /local/repos/xen-upstream/xen/include/asm/guest_access.h:14:32: error: its 
>> scope is only this definition or declaration, which is probably not what you 
>> want [-Werror]
>> cc1: all warnings being treated as errors
>> make[4]: *** [device_tree.o] Error 1
>>
>>
>> Am I missing anything?
> Possibly a result of Wei's recent patch
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=de62402a9c2e403b049aa238b4fa4e2d618e8870
> which is newer than the posting of this series.
>

Thank you for bringing that up. Since Wei has removed a forward
declaration to struct domain in , my patch series failed to
build right after rebasing to staging. By following Wei's approach,
adding a forward declaration to struct domain in 
fixes the upper issue. I will address this issue separately in patch 08/13.

Thanks,
~Sergej


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Andrew Cooper]

On 15/08/2017 23:25, Stefano Stabellini wrote:
> On Tue, 15 Aug 2017, Julien Grall wrote:
>> On 14/08/17 22:03, Sergej Proskurin wrote:
>>> Hi Julien,
>>>
>>> On 08/14/2017 07:37 PM, Julien Grall wrote:
 Hi Sergej,

 On 09/08/17 09:20, Sergej Proskurin wrote:
> +/*
> + * According to to ARM DDI 0487B.a J1-5927, we return an error if
> the found
 Please drop one of the 'to'. The rest looks good to me.

>>> Great, thanks. I will remove the second "to" in v9. Would that be an
>>> Acked-by or shall I tag this patch with a Reviewed-by you?
>> Acked-by. FIY, you still missing an acked from "The REST" for patch #7, the
>> rest looks fully acked.
> I acked patch #7, but patch #8 breaks the build on ARM:
>
>
> In file included from 
> /local/repos/xen-upstream/xen/include/xen/guest_access.h:10:0,
>  from device_tree.c:15:
> /local/repos/xen-upstream/xen/include/asm/guest_access.h:14:32: error: 
> 'struct domain' declared inside parameter list [-Werror]
> uint32_t size, bool_t is_write);
> ^
> /local/repos/xen-upstream/xen/include/asm/guest_access.h:14:32: error: its 
> scope is only this definition or declaration, which is probably not what you 
> want [-Werror]
> cc1: all warnings being treated as errors
> make[4]: *** [device_tree.o] Error 1
>
>
> Am I missing anything?

Possibly a result of Wei's recent patch
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=de62402a9c2e403b049aa238b4fa4e2d618e8870
which is newer than the posting of this series.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Stefano Stabellini]

On Tue, 15 Aug 2017, Julien Grall wrote:
> On 14/08/17 22:03, Sergej Proskurin wrote:
> > Hi Julien,
> > 
> > On 08/14/2017 07:37 PM, Julien Grall wrote:
> > > Hi Sergej,
> > > 
> > > On 09/08/17 09:20, Sergej Proskurin wrote:
> > > > +/*
> > > > + * According to to ARM DDI 0487B.a J1-5927, we return an error if
> > > > the found
> > > 
> > > Please drop one of the 'to'. The rest looks good to me.
> > > 
> > 
> > Great, thanks. I will remove the second "to" in v9. Would that be an
> > Acked-by or shall I tag this patch with a Reviewed-by you?
> 
> Acked-by. FIY, you still missing an acked from "The REST" for patch #7, the
> rest looks fully acked.

I acked patch #7, but patch #8 breaks the build on ARM:


In file included from 
/local/repos/xen-upstream/xen/include/xen/guest_access.h:10:0,
 from device_tree.c:15:
/local/repos/xen-upstream/xen/include/asm/guest_access.h:14:32: error: 'struct 
domain' declared inside parameter list [-Werror]
uint32_t size, bool_t is_write);
^
/local/repos/xen-upstream/xen/include/asm/guest_access.h:14:32: error: its 
scope is only this definition or declaration, which is probably not what you 
want [-Werror]
cc1: all warnings being treated as errors
make[4]: *** [device_tree.o] Error 1


Am I missing anything?

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,

On 08/15/2017 12:13 PM, Julien Grall wrote:
> 
> 
> On 14/08/17 22:03, Sergej Proskurin wrote:
>> Hi Julien,
>>
>> On 08/14/2017 07:37 PM, Julien Grall wrote:
>>> Hi Sergej,
>>>
>>> On 09/08/17 09:20, Sergej Proskurin wrote:
 +/*
 + * According to to ARM DDI 0487B.a J1-5927, we return an error if
 the found
>>>
>>> Please drop one of the 'to'. The rest looks good to me.
>>>
>>
>> Great, thanks. I will remove the second "to" in v9. Would that be an
>> Acked-by or shall I tag this patch with a Reviewed-by you?
> 
> Acked-by. FIY, you still missing an acked from "The REST" for patch #7,
> the rest looks fully acked.
> 

Yea, I know. I will ping The REST maintainers again.

Thanks,
~Sergej

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

On 14/08/17 22:03, Sergej Proskurin wrote:

Hi Julien,

On 08/14/2017 07:37 PM, Julien Grall wrote:

Hi Sergej,

On 09/08/17 09:20, Sergej Proskurin wrote:

+/*
+ * According to to ARM DDI 0487B.a J1-5927, we return an error if
the found


Please drop one of the 'to'. The rest looks good to me.



Great, thanks. I will remove the second "to" in v9. Would that be an
Acked-by or shall I tag this patch with a Reviewed-by you?


Acked-by. FIY, you still missing an acked from "The REST" for patch #7, 
the rest looks fully acked.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

Hi Julien,

On 08/14/2017 07:37 PM, Julien Grall wrote:
> Hi Sergej,
> 
> On 09/08/17 09:20, Sergej Proskurin wrote:
>> +/*
>> + * According to to ARM DDI 0487B.a J1-5927, we return an error if
>> the found
> 
> Please drop one of the 'to'. The rest looks good to me.
> 

Great, thanks. I will remove the second "to" in v9. Would that be an
Acked-by or shall I tag this patch with a Reviewed-by you?

Thanks,
~Sergej

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Julien Grall]

Hi Sergej,

On 09/08/17 09:20, Sergej Proskurin wrote:

+/*
+ * According to to ARM DDI 0487B.a J1-5927, we return an error if the found


Please drop one of the 'to'. The rest looks good to me.


+ * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or if the PTE
+ * maps a memory block at level 3 (PTE<1:0> == 01).
+ */
+if ( !lpae_is_page(pte, level) && !lpae_is_superpage(pte, level) )
+return -EFAULT;
+
+/* Make sure that the lower bits of the PTE's base address are zero. */
+mask = GENMASK_ULL(47, grainsizes[gran]);
+*ipa = (pfn_to_paddr(pte.walk.base) & mask) | (gva & masks[gran][level]);
+
+/*
+ * Set permissions so that the caller can check the flags by herself. Note
+ * that stage 1 translations also inherit attributes from the tables
+ * (ARM DDI 0487B.a J1-5928).
+ */
+if ( !pte.pt.ro && !ro_table )
+*perms |= GV2M_WRITE;
+if ( !pte.pt.xn && !xn_table )
+*perms |= GV2M_EXEC;
+
+return 0;
 }


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v8 11/13] arm/mem_access: Add long-descriptor based gpt

[Sergej Proskurin]

This commit adds functionality to walk the guest's page tables using the
long-descriptor translation table format for both ARMv7 and ARMv8.
Similar to the hardware architecture, the implementation supports
different page granularities (4K, 16K, and 64K). The implementation is
based on ARM DDI 0487B.a J1-5922, J1-5999, and ARM DDI 0406C.b B3-1510.

Note that the current implementation lacks support for Large VA/PA on
ARMv8.2 architectures (LVA/LPA, 52-bit virtual and physical address
sizes). The associated location in the code is marked appropriately.

Signed-off-by: Sergej Proskurin 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
---
v2: Use TCR_SZ_MASK instead of TTBCR_SZ_MASK for ARM 32-bit guests using
the long-descriptor translation table format.

Cosmetic fixes.

v3: Move the implementation to ./xen/arch/arm/guest_copy.c.

Remove the array strides and declare the array grainsizes as static
const instead of just const to reduce the function stack overhead.

Move parts of the funtion guest_walk_ld into the static functions
get_ttbr_and_gran_64bit and get_top_bit to reduce complexity.

Use the macro BIT(x) instead of (1UL << x).

Add more comments && Cosmetic fixes.

v4: Move functionality responsible for determining the configured IPA
output-size into a separate function get_ipa_output_size. In this
function, we remove the previously used switch statement, which was
responsible for distinguishing between different IPA output-sizes.
Instead, we retrieve the information from the introduced ipa_sizes
array.

Remove the defines GRANULE_SIZE_INDEX_* and TTBR0_VALID from
guest_walk.h. Instead, introduce the enums granule_size_index
active_ttbr directly inside of guest_walk.c so that the associated
fields don't get exported.

Adapt the function to the new parameter of type "struct vcpu *".

Remove support for 52bit IPA output-sizes entirely from this commit.

Use lpae_* helpers instead of p2m_* helpers.

Cosmetic fixes & Additional comments.

v5: Make use of the function vgic_access_guest_memory to read page table
entries in guest memory.

Invert the indeces of the arrays "offsets" and "masks" and simplify
readability by using an appropriate macro for the entries.

Remove remaining CONFIG_ARM_64 #ifdefs.

Remove the use of the macros BITS_PER_WORD and BITS_PER_DOUBLE_WORD.

Use GENMASK_ULL instead of manually creating complex masks to ease
readability.

Also, create a macro CHECK_BASE_SIZE which simply reduces the code
size and simplifies readability.

Make use of the newly introduced lpae_page macro in the if-statement
to test for invalid/reserved mappings in the L3 PTE.

Cosmetic fixes and additional comments.

v6: Convert the macro CHECK_BASE_SIZE into a helper function
check_base_size. The use of the old CHECK_BASE_SIZE was confusing as
it affected the control-flow through a return as part of the macro.

Return the value -EFAULT instead of -EINVAL if access to the guest's
memory fails.

Simplify the check in the end of the table walk that ensures that
the found PTE is a page or a superpage. The new implementation
checks if the pte maps a valid page or a superpage and returns an
-EFAULT only if both conditions are not true.

Adjust the type of the array offsets to paddr_t instead of vaddr_t
to allow working with the changed *_table_offset_* helpers, which
return offsets of type paddr_t.

Make use of renamed function access_guest_memory_by_ipa instead of
vgic_access_guest_memory.

v7: Change the return type of check_base_size to bool as it returns only
two possible values and the caller is interested only whether the call
has succeeded or not.

Use a mask for the computation of the IPA, as the lower values of
the PTE's base address do not need to be zeroed out.

Cosmetic fixes in comments.

v8: By calling access_guest_memory_by_ipa in guest_walk_(ld|sd), we rely
on the p2m->lock (rw_lock) to be recursive. To avoid bugs in the
future implementation, we add a comment in struct p2m_domain to
address this case. Thus, we make the future implementation aware of
the nested use of the lock.
---
 xen/arch/arm/guest_walk.c | 398 +-
 xen/include/asm-arm/p2m.h |   8 +-
 2 files changed, 403 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 78badc2949..c6441ab2f8 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -15,7 +15,10 @@
  * this program; If not, see .
  */
 
+#include 
 #include 
+#include 
+#include 
 
 /*
  * The function guest_walk_sd translates a given GVA into an IPA using the
@@ -33,6 +36,174 @@ static int guest_walk_sd(const struct vcpu *v,
 }
 
 /*
+ * Get the IPA