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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.11.14 at 09:12, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/11/20 15:31, Jan Beulich wrote:
>>>>> On 19.11.14 at 02:26, <tiejun.chen@xxxxxxxxx> wrote:
>>> int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
>>> {
>>>       struct acpi_rmrr_unit *rmrr;
>>>       int rc = 0;
>>>       unsigned int i;
>>>       u16 bdf;
>>>
>>>       for_each_rmrr_device ( rmrr, bdf, i )
>>>       {
>>>           rc = func(PFN_DOWN(rmrr->base_address),
>>>                              PFN_UP(rmrr->end_address) -
>>>                               PFN_DOWN(rmrr->base_address),
>>>                              PCI_SBDF(rmrr->segment, bdf),
>>>                             ctxt);
>>>           /* Hit this entry so just go next. */
>>>           if ( rc == 1 )
>>>               i = rmrr->scope.devices_cnt;
>>>           else if ( rc < 0 )
>>>               return rc;
>>>       }
>>>
>>>       return rc;
>>> }
>>
>> Better. Another improvement would be make it not depend on the
>> internal workings of for_each_rmrr_device()... And in any case you
> 
> Yes but as you see,
> 
> #define for_each_rmrr_device(rmrr, bdf, idx)            \
>      list_for_each_entry(rmrr, &acpi_rmrr_units, list)   \
>          /* assume there never is a bdf == 0 */          \
>          for (idx = 0; (bdf = rmrr->scope.devices[idx]) && \
>                   idx < rmrr->scope.devices_cnt; idx++)
> 
> So,
>      for_each_rmrr_device ( rmrr, bdf, i )
>      {
>          rc = func(...);
>          /* Hit this entry so just go next. */
>          if ( rc > 0 )
>              i = rmrr->scope.devices_cnt;
> 
> If you don't intend to reset something of this internal working, its 
> hard to go next rmrr entry. Maybe you already have idea, so could you 
> give me some hints?

Have a second struct acpi_rmrr_unit pointer, starting out as NULL
and getting set to the current one when the callback returns a
positive value. Skip further iterations as long as both pointers
match.

>> should not special case 1 - just return when rc is negative and skip
>> the rest of the current RMRR when it's positive. And of course make
>> the function's final return value predictable.
>>
> 
> Like this,
> 
>          /* Hit this entry so just go next. */
>          if ( rc > 0 )
>              xxxx;
>          else if ( rc < 0 )
>              return rc;
>      }

Yes, albeit swapping the order (and dropping the "else" along with
adding unlikely() to the error case) would be preferred.

Jan


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