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

Re: [PATCH 3/6] x86/mem-paging: use guest handle for XENMEM_paging_op_prep

Hi Jan,

On 20/04/2020 08:26, Jan Beulich wrote:
On 17.04.2020 19:13, Julien Grall wrote:
On 17/04/2020 10:44, Jan Beulich wrote:
On 17.04.2020 10:37, Julien Grall wrote:
On 16/04/2020 16:46, Jan Beulich wrote:
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1779,7 +1779,8 @@ void p2m_mem_paging_populate(struct doma
     * mfn if populate was called for  gfn which was nominated but not evicted. 
     * this case only the p2mt needs to be forwarded.
-int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l, uint64_t buffer)
+int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l,
+                        XEN_GUEST_HANDLE_PARAM(const_uint8) buffer)

Shouldn't this technically be XEN_GUEST_HANDLE_64() to match the field?

I think an argument can be made for going either way - as a function
parameter it should have the type chosen. Do you see any (possibly
just latent) breakage from using _PARAM() rather than _64()?
I know they are the same on x86, but from an abstract PoV they are 
fundamentally different.

XEN_GUEST_HANDLE_PARAM() represents a guest pointer, when pased as an
hypercall argument.

XEN_GUEST_HANDLE() represents a guest pointer, when passed as a field
in a struct in memory.

In this case, the guest pointer was part of a structure. So I think
you want to use XEN_GUEST_HANDLE().

Hmm, looks like I was confused about what the two mean. So far I was
under the impression that _PARAM() was to be used for function
parameters in general, not just hypercall ones. While the text near
the macro definitions is quite clear in this regard, I'm afraid
Stefano's original series (first and foremost commit abf06ea91d12's
playing with e.g. handle_iomem_range()) was rather confusing than
helpful - it looks to me as if quite a bit of the "casting" could
actually be dropped (I'll see about doing some cleanup there). Plus
I'm afraid other mixing of plain vs param has been introduced on
x86, at least for dm.c:track_dirty_vram()'s calls to
{hap,shadow}_track_dirty_vram(); this is just the first instance
I've found - there may be more.

I agree the commit you mention above is confusing. If we follow the definition, then the conversion between the two internally should never have been done.

Maybe Stefano can clarify the intention?

FWIW, the different matters on Arm. Although, it looks like the
compiler will not warn you if you are using the wrong handler :(.

I find this highly suspicious, but can't check myself until back
in the office - these are distinct compound types after all, so
this shouldn't just be a warning, but an error. Or did you merely
mean there's no warning on x86?

I mean on Arm 32-bit. I have changed one of the function to use XEN_GUEST_HANDLE_PARAM() rather than XEN_GUEST_HANDLE() but not changing the caller.

It is probably because they are both defined using an union. Interestly, the type will also not be checked, so the code a function will happily accept a XEN_GUEST_HANDLE_PARAM(uint8) even if the prototype requested XEN_GUEST_HANDLE_PARAM(uint64).

This looks rather messy, maybe we should use a structure (and some alignment) to add more safety.


Julien Grall



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