Re: [Xen-devel] [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.12.14 at 10:12, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/12/9 16:19, Jan Beulich wrote:
>>>>> On 09.12.14 at 08:47, <tiejun.chen@xxxxxxxxx> wrote:
>>> On 2014/12/8 16:51, Jan Beulich wrote:
>>>> The whole "if-copy-unlock-and-return-EFAULT-otherwise-increment"
>>>> is identical and can be factored out pretty easily afaict.
>>>
>>> What about this?
>>>
>>> struct get_reserved_device_memory {
>>>       struct xen_reserved_device_memory_map map;
>>>       unsigned int used_entries;
>>>       struct domain *domain;
>>> };
>>>
>>> static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
>>>                                         u32 id, void *ctxt)
>>> {
>>>       struct get_reserved_device_memory *grdm = ctxt;
>>>       struct domain *d = grdm->domain;
>>>       unsigned int i, hit_one = 0;
>>>       u32 sbdf;
>>>       struct xen_reserved_device_memory rdm = {
>>>           .start_pfn = start, .nr_pages = nr
>>>       };
>>>
>>>       if ( !d->arch.hvm_domain.pci_force )
>>>       {
>>>           for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
>>>           {
>>>               sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[i].seg,
>>>                                d->arch.hvm_domain.pcidevs[i].bus,
>>>                                d->arch.hvm_domain.pcidevs[i].devfn);
>>>               if ( sbdf == id )
>>>               {
>>>                   hit_one = 1;
>>>                   break;
>>>               }
>>>           }
>>>
>>>           if ( !hit_one )
>>>               return 0;
>>>       }
>>
>> Why do you always pick other than the simplest possible solution?
> 
> I don't intend it to be, but I may go a complicated way, even a wrong 
> way, based on my understanding. But as one main maintainer, if you 
> always say to me in such a reproachful word more than once, I have to 
> consider you may hint constantly I'm not a suitable candidate to finish 
> this. Its fair to me, I'd really like to quit this to ask my manager if 
> it can deliver to other guy to make sure this can move forward.
> 
>> You don't need a separate variable here, you can simply check
>> whether i reached d->arch.hvm_domain.num_pcidevs after the
>> loop. And even if you added a variable, it would want to be a
> 
> Are you saying this?
> 
>       if ( i == d->arch.hvm_domain.num_pcidevs )
>               return 0;

Yes. Or use >=.

> But if the last one happens to one hit, 'i' is equal to 
> d->arch.hvm_domain.num_pcidevs.

No, when the last one hits, i == d->arch.hvm_domain.num_pcidevs - 1.

Jan


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