Re: [Xen-devel] [PATCH v4 2/6] xen: add a p2mt parameter to map_mmio_regions

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 07/08/2019 01:23, Stefano Stabellini wrote:
> Add a p2mt parameter to map_mmio_regions, pass p2m_mmio_direct_dev on
> ARM and p2m_mmio_direct on x86 -- no changes in behavior.
>
> On x86, introduce a macro to strip away the last parameter and rename
> the existing implementation of map_mmio_regions to map_mmio_region.
> Use map_mmio_region in vpci as it is x86-only today.
This feels quite wrong. You have a "plural" function calling a "singular" 
function. This is usually the other way around. This is also quite difficult for 
a user to understand why the 's' is been dropped/added (depending how you view 
it) because in both case you only deal with a single region.
The confusion is added because there are no unmap_mmio_region so the code looks 
strange to read:
> +        rc = map->map ? map_mmio_region(map->d, _gfn(s), size, _mfn(s))
>                         : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));

Anyway, I realized that Jan suggested it and it is x86 code. So if he is happy 
with it so it be.
[...]

> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index b48e408583..2674caa005 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -919,6 +919,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>           unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
>           unsigned long mfn_end = mfn + nr_mfns - 1;
>           int add = op->u.memory_mapping.add_mapping;
> +        p2m_type_t p2mt;
>   
>           ret = -EINVAL;
>           if ( mfn_end < mfn || /* wrap? */
> @@ -931,6 +932,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>           /* Must break hypercall up as this could take a while. */
>           if ( nr_mfns > 64 )
>               break;
> +
> +        p2mt = p2m_mmio_direct_dev;
> +#else
> +        p2mt = p2m_mmio_direct;
I think it is a pretty bad idea to have arch specific code in common code. This 
is only to make more difficult to add new arch (such as RISCv). Instead we 
should provide helper in arch code.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel