Re: [Xen-devel] [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps

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

On 2014/11/12 16:55, Jan Beulich wrote:
> > > > On 12.11.14 at 04:05, <tiejun.chen@xxxxxxxxx> wrote:
> > I don't see any feedback to this point, so I think you still prefer we
> > should do all check in the callback function.
> As a draft this looks reasonable, but there are various bugs to be
> dealt with along with cosmetic issues (I'll point out the former, but
> I'm tired of pointing out the latter once again - please go back to
> earlier reviews of patches to refresh e.g. what types to use for
> loop variables).
>
> > I tried to address this but obviously we have to pass each 'pdf' to
> > callback functions,
> Yes, but at the generic IOMMU layer this shouldn't be named "bdf",
> but something more neutral (maybe "id"). And you again lost the
Okay.

> segment there.
I think we don't need segment since when we passthrough a device, that 
domain doesn't matter with the real segment in phydev.
> > @@ -36,9 +40,24 @@ static int get_reserved_device_memory(xen_pfn_t start,
> >            if ( rdm.start_pfn != start || rdm.nr_pages != nr )
> >                return -ERANGE;
> >
> > -        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
> > -                                     &rdm, 1) )
> > -            return -EFAULT;
> > +        if ( d->arch.hvm_domain.pci_force )
> > +        {
> > +            if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
> > +                                         &rdm, 1) )
> > +                return -EFAULT;
> > +        }
> > +        else
> > +        {
> > +            for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
> > +            {
> > +                pt_bdf = PCI_BDF2(d->arch.hvm_domain.pcidevs[i].bus,
> > +                                  d->arch.hvm_domain.pcidevs[i].devfn);
> > +                if ( pt_bdf == bdf )
> > +                    if ( __copy_to_compat_offset(grdm->map.buffer, 
> > grdm->used_entries,
> > +                                                 &rdm, 1) )
> > +                        return -EFAULT;
> I think d->arch.hvm_domain.pcidevs[] shouldn't contain duplicates,
> and hence there's no point continuing the loop if you found a match.
You're right.

> > +            }
> > +        }
> >        }
> >
> >        ++grdm->used_entries;
> This should no longer get incremented unconditionally.
Yes.

> > @@ -314,6 +333,7 @@ int compat_memory_op(unsigned int cmd,
> > XEN_GUEST_HANDLE_PARAM(void) compat)
> >                    return -EFAULT;
> >
> >                grdm.used_entries = 0;
> > +            grdm.domain = current->domain;
> >                rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
> >                                                      &grdm);
> Maybe I misguided you with an earlier response, or maybe the
> earlier reply was in a different context: There's no point
> communicating current->domain to the callback function; there
> would be a point communicating the domain if it was _not_
> always current->domain.
I will look into this.

Thanks
Tiejun

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