Re: [Xen-devel] [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm

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

On 2014/12/4 23:50, Jan Beulich wrote:
> > > > On 01.12.14 at 10:24, <tiejun.chen@xxxxxxxxx> wrote:
> > --- a/xen/common/compat/memory.c
> > +++ b/xen/common/compat/memory.c
> > @@ -22,27 +22,66 @@ struct get_reserved_device_memory {
> >       unsigned int used_entries;
> >   };
> >
> > -static int get_reserved_device_memory(xen_pfn_t start,
> > -                                      xen_ulong_t nr, void *ctxt)
> > +static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
> > +                                      u32 id, void *ctxt)
> >   {
> >       struct get_reserved_device_memory *grdm = ctxt;
> > +    struct domain *d;
> > +    unsigned int i;
> > +    u32 sbdf;
> > +    struct compat_reserved_device_memory rdm = {
> > +        .start_pfn = start, .nr_pages = nr
> > +    };
> >
> > -    if ( grdm->used_entries < grdm->map.nr_entries )
> > -    {
> > -        struct compat_reserved_device_memory rdm = {
> > -            .start_pfn = start, .nr_pages = nr
> > -        };
> > +    if ( rdm.start_pfn != start || rdm.nr_pages != nr )
> > +        return -ERANGE;
> >
> > -        if ( rdm.start_pfn != start || rdm.nr_pages != nr )
> > -            return -ERANGE;
> > +    d = rcu_lock_domain_by_any_id(grdm->map.domid);
> > +    if ( d == NULL )
> > +        return -ESRCH;
> So why are you doing this in the call back (potentially many times)
> instead of just once in compat_memory_op(), storing the pointer in
> the context structure?
Okay.

> > -        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
> > -                                     &rdm, 1) )
> > -            return -EFAULT;
> > +    if ( d )
> > +    {
> > +        if ( d->arch.hvm_domain.pci_force )
> You didn't verify that the domain is a HVM/PVH one.
Is this, ASSERT(is_hvm_domain(grdm.domain)), correct?

> > +        {
> > +            if ( grdm->used_entries < grdm->map.nr_entries )
> > +            {
> > +                if ( __copy_to_compat_offset(grdm->map.buffer,
> > +                                             grdm->used_entries,
> > +                                             &rdm, 1) )
> > +                {
> > +                    rcu_unlock_domain(d);
> > +                    return -EFAULT;
> > +                }
> > +            }
> > +            ++grdm->used_entries;
> > +        }
> > +        else
> > +        {
> > +            for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
> > +            {
> > +                sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[i].seg,
> > +                                 d->arch.hvm_domain.pcidevs[i].bus,
> > +                                 d->arch.hvm_domain.pcidevs[i].devfn);
> > +                if ( sbdf == id )
> > +                {
> > +                    if ( grdm->used_entries < grdm->map.nr_entries )
> > +                    {
> > +                        if ( __copy_to_compat_offset(grdm->map.buffer,
> > +                                                     grdm->used_entries,
> > +                                                     &rdm, 1) )
> > +                        {
> > +                            rcu_unlock_domain(d);
> > +                            return -EFAULT;
> > +                        }
> > +                    }
> > +                    ++grdm->used_entries;
> break;
Added.

> Also trying to fold code identical on the if and else branches would
> seem pretty desirable.
Sorry, I can't see what I'm missing.

> > @@ -319,9 +358,13 @@ int compat_memory_op(unsigned int cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) compat)
> >
> >               if ( !rc && grdm.map.nr_entries < grdm.used_entries )
> >                   rc = -ENOBUFS;
> > +
> >               grdm.map.nr_entries = grdm.used_entries;
> > -            if ( __copy_to_guest(compat, &grdm.map, 1) )
> > -                rc = -EFAULT;
> > +            if ( grdm.map.nr_entries )
> > +            {
> > +                if ( __copy_to_guest(compat, &grdm.map, 1) )
> > +                    rc = -EFAULT;
> > +            }
> Why do you need this change?
If we have no any entries, why do we still copy that?

> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -904,17 +904,33 @@ int platform_supports_x2apic(void)
> >
> >   int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
> >   {
> > -    struct acpi_rmrr_unit *rmrr;
> > +    struct acpi_rmrr_unit *rmrr, *rmrr_cur = NULL;
> >       int rc = 0;
> > +    unsigned int i;
> > +    u16 bdf;
> >
> > -    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
> > +    for_each_rmrr_device ( rmrr, bdf, i )
> >       {
> > -        rc = func(PFN_DOWN(rmrr->base_address),
> > -                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
> > -                  ctxt);
> > -        if ( rc )
> > -            break;
> > +        if ( rmrr != rmrr_cur )
> > +        {
> > +            rc = func(PFN_DOWN(rmrr->base_address),
> > +                      PFN_UP(rmrr->end_address) -
> > +                        PFN_DOWN(rmrr->base_address),
> > +                      PCI_SBDF(rmrr->segment, bdf),
> > +                      ctxt);
> > +
> > +            if ( unlikely(rc < 0) )
> > +                return rc;
> > +
> > +            /* Just go next. */
> > +            if ( !rc )
> > +                rmrr_cur = rmrr;
> > +
> > +            /* Now just return specific to user requirement. */
> > +            if ( rc > 0 )
> > +                return rc;
> Nice that you check for that, but I can't see this case occurring
> anymore. Did you lose some code? Also please don't write code
We have three scenarios here:

#1 rc < 0 means this is an error;
#2 rc = 0 means the tools caller don't know how many buffers it should 
construct, so we need to count all entries as 'nr_entries' to return.
#3 rc > 0 means in some cases, we need to return some specific values, 
like 1 to indicate we're hitting some RMRR range. Currently, we use gfn 
to check this in case of memory populating, ept violation handler and 
mem_access.
> more complicated than necessary. The above two if()s could be
>
>
> +            if ( rc > 0 )
> +                return rc;
> +
> +            rmrr_cur = rmrr;
>
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -586,6 +586,11 @@ typedef struct xen_reserved_device_memory
> > xen_reserved_device_memory_t;
> >   DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);
> >
> >   struct xen_reserved_device_memory_map {
> > +    /*
> > +     * Domain whose reservation is being changed.
> > +     * Unprivileged domains can specify only DOMID_SELF.
> > +     */
> > +    domid_t        domid;
> >       /* IN/OUT */
> >       unsigned int nr_entries;
> >       /* OUT */
> Your addition lacks an IN annotation.
Are you saying something for 'nr_entries'? But I didn't introduce 
anything to change the original usage. Anyway, I try to improve this,
    /*
     * IN: on call the number of entries which can be stored in buffer.
     * OUT: on return the number of entries which have been stored in
     * buffer. If on call the number is less the number of all necessary
     * entries, on return the number of entries which is needed.
     */


> > --- a/xen/include/xen/pci.h
> > +++ b/xen/include/xen/pci.h
> > @@ -31,6 +31,8 @@
> >   #define PCI_DEVFN2(bdf) ((bdf) & 0xff)
> >   #define PCI_BDF(b,d,f)  ((((b) & 0xff) << 8) | PCI_DEVFN(d,f))
> >   #define PCI_BDF2(b,df)  ((((b) & 0xff) << 8) | ((df) & 0xff))
> > +#define PCI_SBDF(s,bdf) (((s & 0xffff) << 16) | (bdf & 0xffff))
> > +#define PCI_SBDF2(s,b,df) (((s & 0xffff) << 16) | PCI_BDF2(b,df))
> Missing several parentheses.
Okay,

#define PCI_SBDF(s,bdf) ((((s) & 0xffff) << 16) | ((bdf) & 0xffff))
#define PCI_SBDF2(s,b,df) ((((s) & 0xffff) << 16) | PCI_BDF2(b,df))


Thanks
Tiejun

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