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

Re: [Xen-devel] [PATCH v5 09/12] arm/mem_access: Add software guest-page-table walk





On 04/07/17 21:25, Sergej Proskurin wrote:
Hi Julien,

On 07/04/2017 06:58 PM, Julien Grall wrote:
Hi Sergej,

On 06/27/2017 12:52 PM, Sergej Proskurin wrote:
The function p2m_mem_access_check_and_get_page in mem_access.c
translates a gva to an ipa by means of the hardware functionality of the
ARM architecture. This is implemented in the function gva_to_ipa. If
mem_access is active, hardware-based gva to ipa translation might fail,
as gva_to_ipa uses the guest's translation tables, access to which might
be restricted by the active VTTBR. To address this issue, in this commit
we add a software-based guest-page-table walk, which will be used by the
function p2m_mem_access_check_and_get_page perform the gva to ipa
translation in software in one of the following commits.

Note: The introduced function guest_walk_tables assumes that the domain,
the gva of which is to be translated, is running on the currently active
vCPU. To walk the guest's page tables on a different vCPU, the following
registers would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and
SCTLR_EL1.

Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
Acked-by: Julien Grall <julien.grall@xxxxxxx>
---
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
---
v2: Rename p2m_gva_to_ipa to p2m_walk_gpt and move it to p2m.c.

     Move the functionality responsible for walking long-descriptor based
     translation tables out of the function p2m_walk_gpt. Also move out
     the long-descriptor based translation out of this commit.

     Change function parameters in order to return access access rights
     to a requested gva.

     Cosmetic fixes.

v3: Rename the introduced functions to guest_walk_(tables|sd|ld) and
     move the implementation to guest_copy.(c|h).

     Set permissions in guest_walk_tables also if the MMU is disabled.

     Change the function parameter of type "struct p2m_domain *" to
     "struct vcpu *" in the function guest_walk_tables.

v4: Change the function parameter of type "struct p2m_domain *" to
     "struct vcpu *" in the functions guest_walk_(sd|ld) as well.

v5: Merge two if-statements in guest_walk_tables to ease readability.

     Set perms to GV2M_READ as to avoid undefined permissions.

I would appreciate if you mention that you kept my tag even with the
change made and asked whether I am happy with it...


Sorry for not mentioning that. Since it was your suggestion right after
you have acked this patch, I thought it would be sufficient to just
state the changes. I will consider that in the future.

I suggested to perms, I don't remember suggesting to use GVM_READ.


In this case, you need a bit more rationale to explain why setting to
GV2M_READ by default is fine... If it is just a random value, then say it.


I will add a comment in the change log stating that we set perms to
GV2M_READ (that equals to 0) by default to avoid having potentially
random values returned on errors.

Beware that you are re-using GV2M_READ for different purpose. In the current code it is used to check whether a guest VA is readable.

Here, you are using both GV2M_* to return the permissions of a mapping. What matters is whether it is fine to consider a page will always have the read permission.

At the moment, you consider you will always translate the VA -> IPA with EL1 permission (see D4-29 in ARM DDI 0487B.a). So the resulting permission in stage-1 will always contain read.

I am ok if you make this assumption, but this needs to be explained in a comment in the code. So we are not surprised in the future why you always set read permission.


Shall I remove your ack for this patch in the next version?

Yes please.

Cheers,

--
Julien Grall

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

 


Rackspace

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