On 2014/12/4 23:50, Jan Beulich wrote:
On 01.12.14 at 10:24, <tiejun.c...@intel.com> 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@lists.xen.org
http://lists.xen.org/xen-devel