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

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel