Re: [Xen-devel] [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.14 at 14:24, <avanzini.arianna@xxxxxxxxx> wrote:
> On 03/25/2014 10:33 AM, Jan Beulich wrote:
>>>>> On 25.03.14 at 03:02, <avanzini.arianna@xxxxxxxxx> wrote:
>>> +int unmap_mmio_regions(struct domain *d,
>>> +                       unsigned long start_gfn,
>>> +                       unsigned long end_gfn,
>>> +                       unsigned long mfn)
>>> +{
>>> +    int ret = 0;
>>> +    unsigned long nr_mfns = end_gfn - start_gfn + 1;
>>> +    unsigned long i;
>>> +
>>> +    if ( !paging_mode_translate(d) )
>>> +        return 0;
>>> +
>>> +    for ( i = 0; i < nr_mfns; i++ )
>>> +        ret |= !clear_mmio_p2m_entry(d, start_gfn + i);
>>> +
>>> +    return ret;
>>> +}
>> 
>> Either you keep the function returning a boolean value (but then
>> please the more conventional 1=success 0=failure), in which case
>> the function return type should be bool_t, or you make the
>> function return a proper error code (and propagate that as
>> necessary in the caller).
>> 
>> Furthermore the mfn parameter is unused here (and effectively
>> unused also in the ARM variant - it's meaningful only when passing
>> INSERT to apply_p2m_changes()), so please drop it.
>> 
> 
> Thank you for the detailed feedback, I will certainly try to address the 
> issues
> you pointed out. Sorry if I bother you with a question on this hunk.
> Patch 0002 added to the REMOVE case of apply_p2m_changes() a consistency 
> check
> which makes use of the mfn parameter.

That must have been an ARM-only patch then, which I likely only
briefly looked at.

> More in detail, the function should check
> whether the mappings to be removed actually involve the gfn and mfn ranges
> passed as parameters.
> Do you think it is acceptable to keep the mfn parameter to this purpose? In 
> case
> you think it is useful/necessary, would you prefer a similar check to be
> performed also someplace in the x86-related code?

Yes - if the check is deemed useful (which I'm not that certain about)
then it should be done in both x86 and ARM (if not doable in common
code in the first place).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel