Re: [v8][PATCH 02/17] introduce XEN_DOMCTL_set_rdm

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

On 2014/12/4 23:33, Jan Beulich wrote:
>>>> On 01.12.14 at 10:24, <tiejun.chen@intel.com> wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -34,6 +34,7 @@
>>   #include <xen/tasklet.h>
>>   #include <xsm/xsm.h>
>>   #include <asm/msi.h>
>> +#include <xen/stdbool.h>
>
> Please don't - we use bool_t in the hypervisor, not bool. The header

Yes.

> only exists for source code shared with the tools.

Looks this could be fine,

d->arch.hvm_domain.pci_force = xdsr->flags & PCI_DEV_RDM_CHECK;

>
>> @@ -1553,6 +1554,44 @@ int iommu_do_pci_domctl(
>>           }
>>           break;
>>
>> +    case XEN_DOMCTL_set_rdm:
>> +    {
>> +        struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm;
>> +        struct xen_guest_pcidev_info *pcidevs = NULL;
>> +        struct domain *d = rcu_lock_domain_by_any_id(domctl->domain);
>
> "d" gets passed into this function - no need to shadow the variable

You're right.

> and (wrongly) re-obtain the pointer.
>
>> +
>> +        if ( d == NULL )
>> +            return -ESRCH;
>> +
>> +        d->arch.hvm_domain.pci_force =
>> +                            xdsr->flags & PCI_DEV_RDM_CHECK ? true : false;
>> +        d->arch.hvm_domain.num_pcidevs = xdsr->num_pcidevs;
>
> You shouldn't set the count before setting the pointer.

Will reorder them.

>
>> +        d->arch.hvm_domain.pcidevs = NULL;
>> +
>> +        if ( xdsr->num_pcidevs )
>> +        {
>> +            pcidevs = xmalloc_array(xen_guest_pcidev_info_t,
>> +                                    xdsr->num_pcidevs);
>
> New domctl-s must not represent security risks: xdsr->num_pcidevs
> can be (almost) arbitrarily large - do you really want to allow such
> huge allocations? A reasonable upper bound could for example be

Sorry, as you know this num_pcidevs is from tools, and actually it share 
that result from that existing hypercall, assign_device, while parsing 
'pci=[]', so I couldn't understand why this can be (almost" arbitrarily 
large.

> the total number of PCI devices the hypervisor knows about.

I take a quick look at this but looks we have no this exact value that 
we can get directly.

>
>> +            if ( pcidevs == NULL )
>> +            {
>> +                rcu_unlock_domain(d);
>> +                return -ENOMEM;
>> +            }
>> +
>> +            if ( copy_from_guest(pcidevs, xdsr->pcidevs,
>> +                                 xdsr->num_pcidevs*sizeof(*pcidevs)) )
>> +            {
>> +                xfree(pcidevs);
>> +                rcu_unlock_domain(d);
>> +                return -EFAULT;
>> +            }
>> +        }
>> +
>> +        d->arch.hvm_domain.pcidevs = pcidevs;
>
> If the operation gets issued more than once for a given domain,
> you're leaking the old pointer here. Overall should think a bit
> more about this multiple use case (or outright disallow it).

Currently this should be disallowed, so I will do this,

     case XEN_DOMCTL_set_rdm:
     {
         struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm;
         struct xen_guest_pcidev_info *pcidevs = NULL;

         if ( d->arch.hvm_domain.pcidevs )
             break;
	...

>
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -674,6 +674,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
>>                           "  RMRR region: base_addr %"PRIx64
>>                           " end_address %"PRIx64"\n",
>>                           rmrru->base_address, rmrru->end_address);
>> +            /*
>> +             * TODO: we may provide a precise paramter just to reserve
>> +             * RMRR range specific to one device.
>> +             */
>> +            dprintk(XENLOG_WARNING VTDPREFIX,
>> +                    "So please set pci_rdmforce to reserve these ranges"
>> +                    " if you need such a device in hotplug case.\n");
>
> It makes no sense to use dprintk() here. I also don't see how this
> message relates to whatever may have been logged immediately
> before, so the wording ("So please set ...") is questionable. Nor is the
> reference to "hotplug case" meaningful here - in this context, only
> physical (host) device hotplug can be meant without further
> qualification. In the end I think trying to log something here is just
> wrong - simply drop the message and make sure whatever you want

Okay.

> to say can be found easily by looking elsewhere.

Maybe we can print something in case when we can't set those identified 
mapping successfully, but its too late...

>
>> --- a/xen/include/asm-x86/hvm/domain.h
>> +++ b/xen/include/asm-x86/hvm/domain.h
>> @@ -90,6 +90,10 @@ struct hvm_domain {
>>       /* Cached CF8 for guest PCI config cycles */
>>       uint32_t                pci_cf8;
>>
>> +    bool_t                  pci_force;
>> +    uint32_t                num_pcidevs;
>> +    struct xen_guest_pcidev_info      *pcidevs;
>
> Without a comment all these field names are pretty questionable.

Yeah, I try to add some comments,

     /* A global flag, we need to check/reserve all Reserved Device 
Memory. */
     bool_t                  pci_force;
     /*
      * If pci_force is 0, this represents how many pci devices we need
      * to check/reserve Reserved Device Memory.
      * If pci_force is 1, this is always 0.
      */
     uint32_t                num_pcidevs;
     /* This represents those pci devices instances when num_pcidevs != 
0. */
     struct xen_guest_pcidev_info      *pcidevs;

Thanks
Tiejun