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

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,

[...]

> 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?

>
>
> 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?

Cheers,
~Sergej


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