Re: [RFC PATCH v2 5/8] arm/mem_access: Add software guest-page-table walk

[Julien Grall <julien.grall@arm.com>]

On 08/06/2017 13:43, Sergej Proskurin wrote:
> Hi Julien,

Hi Sergej,

>
> [...]
>
>> I know I suggested to move in p2m.c. Looking at the diff stat, this
>> will increase quite a lot p2m.c which is already big.
>>
>> How about introducing a file guest_walk.c which contain the new
>> functions?
>>
>
> No problem at all. I will gladly move the functionality into a separate
> file.
>
>>
>>> +                             vaddr_t gva, paddr_t *ipa,
>>> +                             unsigned int *perm_ro)
>>
>> I am a bit confused with perm_ro. Will you only return 0/1? If so it
>> should be a bool.
>>
>> But we likely want to know more permission such as the execution bit...
>>
>
> Yes, I agree that we should return more permissions back to the caller.
> I suggest that we agree on required permissions at this point, as do not
> only have the execution bit (!XN) but also we distinguish between the
> Privileged XN (PXN) bit in the long-descriptor format, as well as the
> access permissions bits (AP[2:x]), which inform which EL/PL is allowed
> to access (RWX) the particular memory region.
>
> Or do you think returning an additional execute bit (!XN) would suffice
> for now, as we don't really care about execution permissions at
> different EL's/PL's at this point?

I think Read/Write/eXecute should be enough for now. We can add 
additional one later on.

My main point here is not about the permission returned but the 
interface. At the moment, you return a boolean-like value and it would 
be tedious to update the callers. What I would like to see is a set of 
flags that we can easily extend. Something like:

#define PERM_READ  (1 << 0)
#define PERM_WRITE (1 << 1)
#define PERM_EXEC  (1 << 2)

We probably want to make them a bit less generic or re-use something 
already existing (I haven't checked what we currently have).

>>
>>
>> The name gpt is not very used in Xen and would prefer a clearer name
>> such as the x86 one "guest_walk_tables".
>
> Sounds good to me. I have changed the names to guest_walk_(tables|sd|ld).
>
>>
>>> +                 paddr_t *ipa, unsigned int *perm_ro)
>>> +{
>>> +    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
>>> +    register_t tcr = READ_SYSREG(TCR_EL1);
>>> +#ifdef CONFIG_ARM_64
>>> +    struct domain *d = p2m->domain;
>>> +#endif
>>
>> The only place use *d is in the is_32bit_domain, so no need to add
>> some #ifdef and define the variable.
>>
>
> [...]
>
>>> +
>>> +#ifdef CONFIG_ARM_64
>>> +    if ( is_32bit_domain(d) )
>>> +#endif
>>
>> is_32bit_domain exists for 32-bit Xen. So not need to have this #ifdef.
>>
>
> True. The reason for this #ifdef is that since is_32bit_domain(d)
> resolves to a (1) on ARMv7, the compiler complained about the fact that
> the variable struct domain *d was not used. To resolve this, I can
> simply use p2m->domain at this point and remove the local variable
> completely. Alternatively, I could use struct domain *d as a function
> parameter instead of struct p2m_domain *p2m. I believe it would be
> cleaner to provide the domain instead of the p2m as parameter, as we
> don't really need the p2m. What would you prefer?

Technically this should be a vCPU as the guest page-table may be 
different on each vCPU. So I would prefer to use a vCPU here.

Also, it would allow us to make sure the function has been called with 
vCPU == current (i.e an ASSERT).

Cheers,

-- 
Julien Grall

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