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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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.

> @@ -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).

> --- 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.

Jan