Re: [Xen-devel] [PATCH 1/6] xen: extend XEN_DOMCTL_memory_mapping to handle cacheability

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 22/04/2019 18:33, Stefano Stabellini wrote:
> On Sun, 21 Apr 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 4/20/19 12:20 AM, Stefano Stabellini wrote:
>>> On Wed, 27 Feb 2019, Julien Grall wrote:
>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>> index 30cfb01..5b8fcc5 100644
>>>>> --- a/xen/arch/arm/p2m.c
>>>>> +++ b/xen/arch/arm/p2m.c
>>>>> @@ -1068,9 +1068,24 @@ int unmap_regions_p2mt(struct domain *d,
>>>>>     int map_mmio_regions(struct domain *d,
>>>>>                          gfn_t start_gfn,
>>>>>                          unsigned long nr,
>>>>> -                     mfn_t mfn)
>>>>> +                     mfn_t mfn,
>>>>> +                     uint32_t cache_policy)
>>>>
>>>> Rather than extending map_mmio_regions, I would prefer if we kill this
>>>> function (and unmap_mmio_mmio_regions) and instead use map_regions_p2mt.
>>>>
>>>> This means the conversation to p2mt should be done in the DOMCTL handling.
>>>
>>> map_regions_p2mt is an arm specific function. map_mmio_regions is the
>>> common function across x86 and arm, called from common code.
>>
>> I really dislike the idea to have two functions doing exactly the same but
>> have different parameters.
> 
> In all fairness, it was already the case before this patch series:

Not really...

> 
>   int map_mmio_regions(struct domain *d,
>                        gfn_t start_gfn,
>                        unsigned long nr,
>                        mfn_t mfn);

... this one always map with p2m_mmio_dev ...

>   int map_regions_p2mt(struct domain *d,
>                        gfn_t gfn,
>                        unsigned long nr,
>                        mfn_t mfn,
>                        p2m_type_t p2mt);

... this one allow you to configure the p2m type.

Anyway, I wasn't already happy with that. Now, you have two functions 
that will allow you to configure it. How does a user know which one to use?

> 
>> If map_regions_p2mt can't be used in the common code, then I would like that
>> map_mmio_regions to be renamed to arch_domctl_map_mmio_regions (or something
>> similar). So it is pretty clear it should be not used in other places.
>>
>> All the other callers of map_mmio_regions should be replaced with
>> map_regions_p2mt.
> 
> Either way works, but I would like an agreement with the x86 maintainers
> before I make any changes to the common interface.

Just to be clear, the suggested interface as it stands for Arm is a 
no-go. I don't want to turn a bad interface to one that is worst and 
more confusing.

Cheers,

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