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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.12.14 at 08:11, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/12/4 23:50, Jan Beulich wrote:
>>>>> On 01.12.14 at 10:24, <tiejun.chen@xxxxxxxxx> wrote:
>>>
>>> -        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
>>> -                                     &rdm, 1) )
>>> -            return -EFAULT;
>>> +    if ( d )
>>> +    {
>>> +        if ( d->arch.hvm_domain.pci_force )
>>
>> You didn't verify that the domain is a HVM/PVH one.
> 
> Is this, ASSERT(is_hvm_domain(grdm.domain)), correct?

Certainly not, or do you want to crash the hypervisor because of bad
tools input?

>>> +        {
>>> +            if ( grdm->used_entries < grdm->map.nr_entries )
>>> +            {
>>> +                if ( __copy_to_compat_offset(grdm->map.buffer,
>>> +                                             grdm->used_entries,
>>> +                                             &rdm, 1) )
>>> +                {
>>> +                    rcu_unlock_domain(d);
>>> +                    return -EFAULT;
>>> +                }
>>> +            }
>>> +            ++grdm->used_entries;
>>> +        }
>>> +        else
>>> +        {
>>> +            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 )
>>> +                {
>>> +                    if ( grdm->used_entries < grdm->map.nr_entries )
>>> +                    {
>>> +                        if ( __copy_to_compat_offset(grdm->map.buffer,
>>> +                                                     grdm->used_entries,
>>> +                                                     &rdm, 1) )
>>> +                        {
>>> +                            rcu_unlock_domain(d);
>>> +                            return -EFAULT;
>>> +                        }
>>> +                    }
>>> +                    ++grdm->used_entries;
>>
>> break;
> 
> Added.
> 
>>
>> Also trying to fold code identical on the if and else branches would
>> seem pretty desirable.
> 
> Sorry, I can't see what I'm missing.

The whole "if-copy-unlock-and-return-EFAULT-otherwise-increment"
is identical and can be factored out pretty easily afaict.

>>> @@ -319,9 +358,13 @@ int compat_memory_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>
>>>               if ( !rc && grdm.map.nr_entries < grdm.used_entries )
>>>                   rc = -ENOBUFS;
>>> +
>>>               grdm.map.nr_entries = grdm.used_entries;
>>> -            if ( __copy_to_guest(compat, &grdm.map, 1) )
>>> -                rc = -EFAULT;
>>> +            if ( grdm.map.nr_entries )
>>> +            {
>>> +                if ( __copy_to_guest(compat, &grdm.map, 1) )
>>> +                    rc = -EFAULT;
>>> +            }
>>
>> Why do you need this change?
> 
> If we have no any entries, why do we still copy that?

That's not only a pointless optimization (the counter question being
"Why add an extra conditional when the copying does no harm?"), but
also not subject of this patch. Additionally iirc the field is an IN/OUT,
i.e. when no entries were found you want to tell the caller so.

>>> --- a/xen/drivers/passthrough/vtd/dmar.c
>>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>>> @@ -904,17 +904,33 @@ int platform_supports_x2apic(void)
>>>
>>>   int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
>>>   {
>>> -    struct acpi_rmrr_unit *rmrr;
>>> +    struct acpi_rmrr_unit *rmrr, *rmrr_cur = NULL;
>>>       int rc = 0;
>>> +    unsigned int i;
>>> +    u16 bdf;
>>>
>>> -    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
>>> +    for_each_rmrr_device ( rmrr, bdf, i )
>>>       {
>>> -        rc = func(PFN_DOWN(rmrr->base_address),
>>> -                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
>>> -                  ctxt);
>>> -        if ( rc )
>>> -            break;
>>> +        if ( rmrr != rmrr_cur )
>>> +        {
>>> +            rc = func(PFN_DOWN(rmrr->base_address),
>>> +                      PFN_UP(rmrr->end_address) -
>>> +                        PFN_DOWN(rmrr->base_address),
>>> +                      PCI_SBDF(rmrr->segment, bdf),
>>> +                      ctxt);
>>> +
>>> +            if ( unlikely(rc < 0) )
>>> +                return rc;
>>> +
>>> +            /* Just go next. */
>>> +            if ( !rc )
>>> +                rmrr_cur = rmrr;
>>> +
>>> +            /* Now just return specific to user requirement. */
>>> +            if ( rc > 0 )
>>> +                return rc;
>>
>> Nice that you check for that, but I can't see this case occurring
>> anymore. Did you lose some code? Also please don't write code
> 
> We have three scenarios here:
> 
> #1 rc < 0 means this is an error;
> #2 rc = 0 means the tools caller don't know how many buffers it should 
> construct, so we need to count all entries as 'nr_entries' to return.
> #3 rc > 0 means in some cases, we need to return some specific values, 
> like 1 to indicate we're hitting some RMRR range. Currently, we use gfn 
> to check this in case of memory populating, ept violation handler and 
> mem_access.

Yes, I saw that you make use of this in later patches. It just seemed
suspicious that you don't in this one.

>>> --- a/xen/include/public/memory.h
>>> +++ b/xen/include/public/memory.h
>>> @@ -586,6 +586,11 @@ typedef struct xen_reserved_device_memory
>>> xen_reserved_device_memory_t;
>>>   DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
>>>
>>>   struct xen_reserved_device_memory_map {
>>> +    /*
>>> +     * Domain whose reservation is being changed.
>>> +     * Unprivileged domains can specify only DOMID_SELF.
>>> +     */
>>> +    domid_t        domid;
>>>       /* IN/OUT */
>>>       unsigned int nr_entries;
>>>       /* OUT */
>>
>> Your addition lacks an IN annotation.
> 
> Are you saying something for 'nr_entries'? But I didn't introduce 
> anything to change the original usage. Anyway, I try to improve this,
> 
>      /*
>       * IN: on call the number of entries which can be stored in buffer.
>       * OUT: on return the number of entries which have been stored in
>       * buffer. If on call the number is less the number of all necessary
>       * entries, on return the number of entries which is needed.
>       */
> 

No, I said "your addition lacks ...". And you addition is the "domid"
field.

Jan

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