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

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

Sorry for this delay response.

On 2015/4/20 21:36, Jan Beulich wrote:
>>>> On 10.04.15 at 11:21, <tiejun.chen@intel.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1793,8 +1793,14 @@ static void iommu_set_pgd(struct domain *d)
>>       hd->arch.pgd_maddr = pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));
>>   }
>>
>> +/*
>> + * 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
>
> Then why invent this one instead of just using ..._TRY at the respective
> call sites.

I just want to own one place to guarantee this behavior if we want to 
change something...

But now I can use XEN_DOMCTL_PCIDEV_RDM_TRY directly.

>
>> @@ -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");
>> +                }
>
> Iirc someone else already pointed out that this message needs to
> change to something understandable. Perhaps it should also log
> the PFN causing the error. And the braces here should be dropped
> (if you inverted the condition you wouldn't even need an "else"; or
> wait - this shouldn't be just "else", as you shouldn't imply anything
> other than ..._TRY means ..._FORCE, albeit the respective check
> perhaps belongs in the generic IOMMU code, not here).

I guess we already achieved this agreement in other thread with 
discussion involved with you and Tim.

>
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -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
>> +    uint32_t  sbdf_flag;   /* flag of assigned device */
>
> Why do you call this "sbdf_flag" - it holds nothing SBDF-like.
>

I can remove 'sbdf_' to make sure this is not confused.

Thanks
Tiejun