Re: [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/11/12 16:55, Jan Beulich wrote:
>>>> On 12.11.14 at 04:05, <tiejun.chen@intel.com> wrote:
>> I don't see any feedback to this point, so I think you still prefer we
>> should do all check in the callback function.
>
> As a draft this looks reasonable, but there are various bugs to be
> dealt with along with cosmetic issues (I'll point out the former, but
> I'm tired of pointing out the latter once again - please go back to
> earlier reviews of patches to refresh e.g. what types to use for
> loop variables).
>
>> I tried to address this but obviously we have to pass each 'pdf' to
>> callback functions,
>
> Yes, but at the generic IOMMU layer this shouldn't be named "bdf",
> but something more neutral (maybe "id"). And you again lost the

Okay.

> segment there.

I think we don't need segment since when we passthrough a device, that 
domain doesn't matter with the real segment in phydev.

>
>> @@ -36,9 +40,24 @@ static int get_reserved_device_memory(xen_pfn_t start,
>>            if ( rdm.start_pfn != start || rdm.nr_pages != nr )
>>                return -ERANGE;
>>
>> -        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
>> -                                     &rdm, 1) )
>> -            return -EFAULT;
>> +        if ( d->arch.hvm_domain.pci_force )
>> +        {
>> +            if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
>> +                                         &rdm, 1) )
>> +                return -EFAULT;
>> +        }
>> +        else
>> +        {
>> +            for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
>> +            {
>> +                pt_bdf = PCI_BDF2(d->arch.hvm_domain.pcidevs[i].bus,
>> +                                  d->arch.hvm_domain.pcidevs[i].devfn);
>> +                if ( pt_bdf == bdf )
>> +                    if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
>> +                                                 &rdm, 1) )
>> +                        return -EFAULT;
>
> I think d->arch.hvm_domain.pcidevs[] shouldn't contain duplicates,
> and hence there's no point continuing the loop if you found a match.

You're right.

>
>> +            }
>> +        }
>>        }
>>
>>        ++grdm->used_entries;
>
> This should no longer get incremented unconditionally.

Yes.

>
>> @@ -314,6 +333,7 @@ int compat_memory_op(unsigned int cmd,
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>                    return -EFAULT;
>>
>>                grdm.used_entries = 0;
>> +            grdm.domain = current->domain;
>>                rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
>>                                                      &grdm);
>>
>
> Maybe I misguided you with an earlier response, or maybe the
> earlier reply was in a different context: There's no point
> communicating current->domain to the callback function; there
> would be a point communicating the domain if it was _not_
> always current->domain.
>

I will look into this.

Thanks
Tiejun