[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |