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

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, April 23, 2015 10:00 PM
> 
> >>> On 23.04.15 at 14:32, <tiejun.chen@intel.com> wrote:
> > On 2015/4/16 23:40, Tim Deegan wrote:
> >> At 17:21 +0800 on 10 Apr (1428686518), Tiejun Chen wrote:
> >>> @@ -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.
> 
> I don't think we can, since a single RMRR may be associated with
> more than one device.
> 
> >>> @@ -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 :)
> 
> I think I'd prefer strict/relaxed over force/try, but I certainly can
> live with the latter.
> 

agree the former is clearer. 

Thanks
Kevin