Re: [RFC][PATCH 07/13] xen/passthrough: extend hypercall to support rdm reservation policy

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

On 2015/4/16 23:40, Tim Deegan wrote:
> Hi,
>
> At 17:21 +0800 on 10 Apr (1428686518), Tiejun Chen wrote:
>> +/*
>> + * In some cases, e.g. add a device to hwdomain, and remove a device from
>> + * user domain, 'try' is fine enough since this is always safe to hwdomain.
>> + */
>> +#define XEN_DOMCTL_PCIDEV_RDM_DEFAULT XEN_DOMCTL_PCIDEV_RDM_TRY
>
> Do we need a way to change this default?

As I said in its comment here, this behavior should be rarely changed in 
the future, and even this really needs to balance something, I just 
think this way is enough and simply.

>
>>   static int rmrr_identity_mapping(struct domain *d, bool_t map,
>> -                                 const struct acpi_rmrr_unit *rmrr)
>> +                                 const struct acpi_rmrr_unit *rmrr,
>> +                                 u32 flag)
>>   {
>>       unsigned long base_pfn = rmrr->base_address >> PAGE_SHIFT_4K;
>>       unsigned long end_pfn = PAGE_ALIGN_4K(rmrr->end_address) >> PAGE_SHIFT_4K;
>> @@ -1851,7 +1857,14 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
>>           if ( !is_hardware_domain(d) )
>>           {
>>               if ( (err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw)) )
>> -                return err;
>> +            {
>> +                if ( flag == XEN_DOMCTL_PCIDEV_RDM_TRY )
>> +                {
>> +                    printk(XENLOG_G_WARNING "Some devices may work failed .\n");
>
> This is a bit cryptic.  How about:
>   "RMRR map failed.  Device %04x:%02x:%02x.%u and domain %d may be unstable.",
> (and pass in the devfn from the caller so we can print the details of
> the device).

Got it but we can't get SBDF here directly.

So just now we can have this line.

         {
             if ( flag == XEN_DOMCTL_PCIDEV_RDM_TRY )
                 dprintk(XENLOG_ERR VTDPREFIX,
                         "RMRR mapping failed to pfn:%"PRIx64""
                         " so Dom%d may be unstable.\n",
                         base_pfn, d->domain_id);
             else
                 return err;
         }

Certainly, we can extend rmrr_identity_mapping() to own its associated 
SBDF as an input parameter (and bring some syncs) if you still think 
this is necessary.

>
>> @@ -493,6 +493,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t);
>>   /* XEN_DOMCTL_deassign_device */
>>   struct xen_domctl_assign_device {
>>       uint32_t  machine_sbdf;   /* machine PCI ID of assigned device */
>> +    /* IN */
>> +#define XEN_DOMCTL_PCIDEV_RDM_TRY       0
>> +#define XEN_DOMCTL_PCIDEV_RDM_FORCE     1
>
> "STRICT" might be a better word than "FORCE" (here and everywhere
> else).  "FORCE" sounds like either Xen will assign the device even if
> it's unsafe,  which is the opposite of what's meant IIUC.

This is definitely fine to me but this is derived from our policy based 
on that previous design,

Global RDM parameter:
     rdm = [ 'host, reserve=force/try' ]
Per-device RDM parameter:
     pci = [ 'sbdf, rdm_reserve=force/try' ]

Please refer to patch #1. So I guess we need a further agreement or 
comments from other guys :)

Thanks
Tiejun