Re: [XEN PATCH v1 1/1] x86/domctl: add gva_to_gfn command

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2023 17:32, Ковалёв Сергей wrote:
> gva_to_gfn command used for fast address translation in LibVMI project.
> With such a command it is possible to perform address translation in
> single call instead of series of queries to get every page table.
> 
> Thanks to Dmitry Isaykin for involvement.
> 
> Signed-off-by: Sergey Kovalev <valor@xxxxxxx>

Along with other comment which were given already, a few more largely style
related nits. First of all when sending patches you want to make sure they
make it through intact. You have some line wrapping and white space
corruption in what actually ended up on the list.

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1364,6 +1364,23 @@ long arch_do_domctl(
>               copyback = true;
>           break;
> 
> +    case XEN_DOMCTL_gva_to_gfn:
> +    {
> +        uint64_t ga = domctl->u.gva_to_gfn.addr;
> +        uint64_t cr3 = domctl->u.gva_to_gfn.cr3;

Please avoid fixed width types where they're not actually needed. In the
two examples above I guess the 1st wants to be paddr_t while the 2nd
wants to be unsigned long (and pfec below unsigned int).

> +        struct vcpu* v = d->vcpu[0];

* and blank want to switch places.

> +        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;

Blank line between declaration(s) and statement(s) please (and not in
the middle of declarations).

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -948,6 +948,17 @@ struct xen_domctl_paging_mempool {
>       uint64_aligned_t size; /* Size in bytes. */
>   };
> 
> +/*
> + * XEN_DOMCTL_gva_to_gfn.
> + *
> + * Get the guest virtual to guest physicall address translated.
> + */
> +struct xen_domctl_gva_to_gfn {
> +    uint64_aligned_t addr;
> +    uint64_aligned_t cr3;

This is x86-specific. If it's meant to be an x86-only interface, then
it needs to be marked as such by suitable #ifdef around it (you'll
find other examples higher up in the file). Otherwise, if it's meant
to be generic in principle (which I think would be better), but
implemented only on x86 for the time being, the name wants to change
(e.g. "root_pt").

If it remained "cr3", you'd also need to clarify what the non-address
bits of the field mean: They hardly will have their CR3 meaning.

> +    uint64_aligned_t page_order;

This certainly doesn't need to be 64 bits wide. For the foreseeable
future 8 bits will suffice, with explicit padding added as necessary
(though Andrew's request for further inputs may consume some of that
padding).

Jan