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

[Arianna Avanzini <avanzini.arianna@gmail.com>]

On 03/25/2014 10:33 AM, Jan Beulich wrote:
>>>> On 25.03.14 at 03:02, <avanzini.arianna@gmail.com> 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. 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?

>> +    case XEN_DOMCTL_memory_mapping:
>> +    {
>> +        unsigned long gfn = op->u.memory_mapping.first_gfn;
>> +        unsigned long mfn = op->u.memory_mapping.first_mfn;
>> +        unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
>> +        unsigned long mfn_end = mfn + nr_mfns;
>> +        unsigned long gfn_end = gfn + nr_mfns;
> 
> Afaics all uses of [gm]fn_end involve subtracting 1 - please do this
> right here if you already introduce these helper variables.
> 
>> --- /dev/null
>> +++ b/xen/include/xen/p2m.h
>> @@ -0,0 +1,16 @@
>> +#ifndef _XEN_P2M_COMMON_H
>> +#define _XEN_P2M_COMMON_H
> 
> You're on the right track with the guard - please name the header
> p2m-common.h, so that it won't violate the common inclusion
> hierarchy of the common header including the corresponding arch
> one. The alternative would be to keep the name, rename the guard,
> and replace all relevant (not necessarily all) #include <asm/p2m.h>
> with #include <xen/p2m.h>.
> 
> Jan
> 


-- 
/*
 * Arianna Avanzini
 * avanzini.arianna@gmail.com
 * 73628@studenti.unimore.it
 */