On 20/03/2023 7:35 pm, Ковалёв Сергей wrote:
> 20.03.2023 22:07, Andrew Cooper пишет:
>>
>> More generally, issuing the hypercall under vcpu0 isn't necessarily
>> correct. It is common for all vCPUs to have equivalent paging settings,
>> but e.g. Xen transiently disables CR4.CET and CR0.WP in order to make
>> self-modifying code changes.
>>
>> Furthermore, the setting of CR4.{PAE,PSE} determines reserved bits, so
>> you can't even ignore the access rights and hope that the translation
>> works out correctly.
>
> Thanks! I didn't think about such things earlier. I should to think
> this know carefully.
If you haven't already, read
https://github.com/xen-project/xen/blob/master/xen/arch/x86/mm/guest_walk.c
and
https://github.com/andyhhp/xtf/blob/pagetable-emulation/tests/pagetable-emulation/main.c
These are various notes and tests I made last time I had to rewrite
Xen's pagewalk from scratch.
>
>>
>> Ideally we'd have a pagewalk algorithm which didn't require taking a
>> vcpu, and instead just took a set of paging configuration, but it is all
>> chronically entangled right now.
>>
>
> Do You mean to add new implementation of "paging_ga_to_gfn_cr3"?
Yes, but I didn't mean for this to be taken as a suggestion. It's far
more work than it sounds...
>
>> I think, at a minimum, you need to take a vcpu_id as an input, but I
>> suspect to make this a usable API you want an altp2m view id too.
>>
>
> Why we should consider altp2m while translating guest virtual address to
> guest physical one?
Because altp2m can change the gfn mappings, and therefore the contents
of the pagetables.
A pagewalk from cr3 in one view can end up being totally different to a
walk from the same cr3 in a different view.
>
>> Also, I'm pretty sure this is only safe for a paused vCPU. If the vCPU
>> isn't paused, then there's a TOCTOU race in the pagewalk code when
>> inspecting control registers.
>>
>
> Thanks! Should we pause the domain?
Certainly the vCPU. Chances are that if you're making this hypercall
from a libvmi callback, the vCPU in question is already paused, at which
point taking one extra pause ref on it is very quick.
>
>>> + uint32_t pfec = PFEC_page_present;
>>> + unsigned int page_order;
>>> +
>>> + uint64_t gfn = paging_ga_to_gfn_cr3(v, cr3, ga, &pfec,
>>> &page_order);
>>> + domctl->u.gva_to_gfn.addr = gfn;
>>> + domctl->u.gva_to_gfn.page_order = page_order;
>>
>> page_order is only not stack rubble if gfn is different to INVALID_GFN.
>>
>
> Sorry but I don't understand "is only not stack rubble". Do you mean
> that I should initialize "page_order" while defining it?
page_order is only initialised when gfn returns != INVALID_GFN.
See the function description.
~Andrew
