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
> segment there.
>
>> @@ -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.
>

I take a look at this again. Seems we shouldn't just check bdf since as 
you know its possible to occupy one entry by multiple different BDFs, so 
we have to filter-to-keep one entry. Instead, I really hope we'd check 
to expose before we do the hypercall.

BTW, I already ping Yang in local to look that possibility to reorder 
the sequence of the device assignment and the memory population in iommu 
side.

Thanks
Tiejun