[Xen-devel] [PATCH v4 6/7] xen-pciback: short-circuit read path used for merging write values
There's no point calling xen_pcibk_config_read() here - all it'll do is return whatever conf_space_read() returns for the field which was found here (and which would be found there again). Also there's no point clearing tmp_val before the call. Signed-off-by: Jan Beulich --- drivers/xen/xen-pciback/conf_space.c |6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) --- 4.7-rc6-xen-pciback.orig/drivers/xen/xen-pciback/conf_space.c +++ 4.7-rc6-xen-pciback/drivers/xen/xen-pciback/conf_space.c @@ -230,10 +230,8 @@ int xen_pcibk_config_write(struct pci_de field_end = OFFSET(cfg_entry) + field->size; if (req_end > field_start && field_end > req_start) { - tmp_val = 0; - - err = xen_pcibk_config_read(dev, field_start, - field->size, &tmp_val); + err = conf_space_read(dev, cfg_entry, field_start, + &tmp_val); if (err) break; ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 7/7] xen-pciback: drop superfluous variables
req_start is simply an alias of the "offset" function parameter, and req_end is being used just once in each function. (And both variables were loop invariant anyway, so should at least have got initialized outside the loop.) Signed-off-by: Jan Beulich --- drivers/xen/xen-pciback/conf_space.c | 16 ++-- 1 file changed, 6 insertions(+), 10 deletions(-) --- 4.7-rc6-xen-pciback.orig/drivers/xen/xen-pciback/conf_space.c +++ 4.7-rc6-xen-pciback/drivers/xen/xen-pciback/conf_space.c @@ -148,7 +148,7 @@ int xen_pcibk_config_read(struct pci_dev struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(dev); const struct config_field_entry *cfg_entry; const struct config_field *field; - int req_start, req_end, field_start, field_end; + int field_start, field_end; /* if read fails for any reason, return 0 * (as if device didn't respond) */ u32 value = 0, tmp_val; @@ -178,12 +178,10 @@ int xen_pcibk_config_read(struct pci_dev list_for_each_entry(cfg_entry, &dev_data->config_fields, list) { field = cfg_entry->field; - req_start = offset; - req_end = offset + size; field_start = OFFSET(cfg_entry); field_end = OFFSET(cfg_entry) + field->size; -if (req_end > field_start && field_end > req_start) { + if (offset + size > field_start && field_end > offset) { err = conf_space_read(dev, cfg_entry, field_start, &tmp_val); if (err) @@ -191,7 +189,7 @@ int xen_pcibk_config_read(struct pci_dev value = merge_value(value, tmp_val, get_mask(field->size), - field_start - req_start); + field_start - offset); } } @@ -211,7 +209,7 @@ int xen_pcibk_config_write(struct pci_de const struct config_field_entry *cfg_entry; const struct config_field *field; u32 tmp_val; - int req_start, req_end, field_start, field_end; + int field_start, field_end; if (unlikely(verbose_request)) printk(KERN_DEBUG @@ -224,19 +222,17 @@ int xen_pcibk_config_write(struct pci_de list_for_each_entry(cfg_entry, &dev_data->config_fields, list) { field = cfg_entry->field; - req_start = offset; - req_end = offset + size; field_start = OFFSET(cfg_entry); field_end = OFFSET(cfg_entry) + field->size; -if (req_end > field_start && field_end > req_start) { + if (offset + size > field_start && field_end > offset) { err = conf_space_read(dev, cfg_entry, field_start, &tmp_val); if (err) break; tmp_val = merge_value(tmp_val, value, get_mask(size), - req_start - field_start); + offset - field_start); err = conf_space_write(dev, cfg_entry, field_start, tmp_val); ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup
On 7/5/2016 8:16 PM, Razvan Cojocaru wrote: On 07/05/16 17:28, Corneliu ZUZU wrote: The arch_vm_event structure is dynamically allocated and freed @ vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack user disables domain monitoring (xc_monitor_disable), which in turn effectively discards any information that was in arch_vm_event.write_data. But this can yield unexpected behavior since if a CR-write was awaiting to be committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data) before xc_monitor_disable is called, then the domain CR write is wrongfully ignored, which of course, in these cases, can easily render a domain crash. To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically allocated and only frees that in vm_event_cleanup_domain, instead of the whole arch_vcpu.vm_event structure, which with this patch will only be freed on vcpu/domain destroyal. Signed-off-by: Corneliu ZUZU Acked-by: Razvan Cojocaru --- Changed since v1: * arch_vcpu.vm_event made pointer again to avoid eating memory from arch_vcpu structure I believe that all acks should be presumed lost on non-trivial changes in a new version (which I believe this qualifies as being, with all the new logic of allocating / deallocating only part of vm_event). Unfortunately I'm out of office until early next week so I can't properly test / thoroughly parse this until then, but we should be extra careful that there are several places in the code where it is assumed that v->arch.vm_event != NULL is the same thing as monitoring being enabled. I'm not saying that they're not treated in this patch (the proper change has certainly been made in emulate.c), I'm just saying that we should be careful that they are. Having said that, I propose a special macro to make this all clearer, something like: #define is_monitor_enabled_for_vcpu(v) \ ( v->arch.vm_event && v->arch.vm_event->emul_read_data ) or equivalent inline functions returning a bool_t. Just a thought. Thanks, Razvan At some point I actually defined that exact macro while constructing this patch, but I decided to delete it afterwards because I thought the code would still be clear without it (i.e. only check emul_read_data when we actually need _that_ to be non-NULL) and because it seemed a bit strange, being possible to have _arch_vm_event allocated_ but having the monitor vm-events subsystem _uninitialized_ (because of .write_data being treated specially). Since the write_data field is also part of the monitor subsystem, conceptually one could say the monitor subsystem is at least _partially_ initialized when that's non-NULL, not uninitialized at all. I'll think of a resolution around this, but in the meantime I there's something more important to settle: I only now notice, it seems to me that the ASSERT(v->arch.vm_event) @ hvm_set_cr0 & such (the one just before calling hvm_monitor_crX) might fail. That's because from what I see in the code the following can happen: - v.arch.vm_event = NULL (vm-event _not_ initialized) - toolstack user calls e.g. xc_moLRnitor_write_ctrlreg(xch, domid, VM_EVENT_X86_CR0, true, true, false) -> do_domctl(XEN_DOMCTL_monitor_op) -> monitor_domctl(XEN_DOMCTL_MONITOR_OP_ENABLE) -> arch_monitor_domctl_event(XEN_DOMCTL_MONITOR_EVENT_WRITE_CTREG) which in turn sets the CR0 bit of d.arch.monitor.write_ctrlreg_enabled _without ever checking if v.arch.vm_event is non-NULL_ - afterwards a CR0 write happens on one of the domain vCPUs and we arrive at that assert without having v.arch.vm_event allocated! I can't test this @ the moment, am I missing something here? I remember taking a look on this code path at some point and idk how I didn't notice something like this, it seems obviously wrong..has something changed recently? I think we need to take a second look over this code-path, I'm also seeing that the proper check _is done_ @ arch_monitor_domctl_op(XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP) (though I think a more proper error return value there would be ENODEV instead of EINVAL). Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/16 - RFC] x86/efi: create new early memory allocator
>>> On 05.07.16 at 20:26, wrote: > On Fri, Apr 15, 2016 at 02:33:12PM +0200, Daniel Kiper wrote: >> 1) We could use native EFI allocation functions (e.g. AllocatePool() >>or AllocatePages()) to get memory chunk. However, later (somewhere >>in __start_xen()) we must copy its contents to safe place or reserve >>this in e820 memory map and map it in Xen virtual address space. > > I have checked Linux kernel code. It allocates buffer for memory map using > EFI API and later reserve it in e820 memory map. Simple. This should work > for us too but... > >>In later case we must also care about conflicts with e.g. crash >>kernel regions which could be quite difficult. > > This is not a problem since Xen can choose dynamically placement of kdump > region during boot phase and there is no requirement to specify it in boot > command line. This means that it will avoid all allocated/reserved regions > including EFI memory map. However, there is one potential problem which > cannot be avoided simply with current EFI spec. I think about conflicts > with trampoline. It must live below 1 MiB. However, there is not something > similar to "AllocateMaxAddress" option for AllocatePages() which would > ask EFI to allocate memory above a given address (Hmmm... Why UEFI designers > did not added such option, e.g. "AllocateMinAddress"? For me it is obvious > thing if we have "AllocateMaxAddress"). Not obvious to me at all. Allowing an upper bound is natural (for both DMA purposes and arbitrary other addressing restrictions). Allowing a lower bound to be specified isn't. > So, it means that we cannot simply > say "give me a memory chunk above 1 MiB". AIUI, Linux guys do not care, > hope that all EFI platforms are smart and AllocatePages() tries hard to > avoid everything below 1 MiB. We can go this way too. However, I am almost > sure that sooner or later we will find crazy platforms which allocate memory > from 0-1 MiB region. We can avoid this by getting EFI memory map, looking for > free regions above 1 MiB and then trying to allocate memory chunk using > AllocatePages() with "AllocateAddress". Does it make sense? I don't see the point of all that, as I don't see why any EFI implementation would want to deviate from the first line principle of satisfying allocation requests as high as possible. Apart from that using (only) EFI allocation mechanisms for obtaining the trampoline area won't work anyway, as we already know there are systems where all of the memory below 1Mb is in use by EFI (mostly with boot kind allocations, i.e. becoming available after ExitBootServices()). >> 2) We may allocate memory area statically somewhere in Xen code which >>could be used as memory pool for early dynamic allocations. Looks >>quite simple. Additionally, it would not depend on EFI at all and >>could be used on legacy BIOS platforms if we need it. However, we >>must carefully choose size of this pool. We do not want increase >>Xen binary size too much and waste too much memory but also we must fit >>at least memory map on x86 EFI platforms. As I saw on small machine, >>e.g. IBM System x3550 M2 with 8 GiB RAM, memory map may contain more >>than 200 entries. Every entry on x86-64 platform is 40 bytes in size. >>So, it means that we need more than 8 KiB for EFI memory map only. >>Additionally, if we want to use this memory pool for Xen and modules >>command line storage (it would be used when xen.efi is executed as EFI >>application) then we should add, I think, about 1 KiB. In this case, >>to be on safe side, we should assume at least 64 KiB pool for early >>memory allocations, which is about 4 times of our earlier calculations. >>However, during discussion on Xen-devel Jan Beulich suggested that >>just in case we should use 1 MiB memory pool like it was in original >>place_string() implementation. So, let's use 1 MiB as it was proposed. >>If we think that we should not waste unallocated memory in the pool >>on running system then we can mark this region as __initdata and move >>all required data to dynamically allocated places somewhere in >> __start_xen(). > > 2a) We can create something like .init.bss and put this thing at the end of > regular .bss section. Then allocate memory chunks starting from lowest > address. After init phase we can free unused memory as in case of > .init.text > or .init.data sections. This way we do not need allocate any space in > image file and freeing of unused memory should be simple. What do you > think about that one? With (again) the caveat of how to size such a region. Bottom line - I continue to be unconvinced that we need something "new" here at all. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] xenstored memory leak
While testing some patches for support of ballooning in Mini-OS by using the xenstore domain I realized that each xl create/destroy pair would increase memory consumption in Mini-OS by about 5kB. Wondering whether this is a xenstore domain only effect I did the same test with xenstored and oxenstored daemons. xenstored showed the same behavior, the "referenced" size showed by the pmap command grew by about 5kB for each create/destroy pair. oxenstored seemed to be even worse in the beginning (about 6kB for each pair), but after about 100 create/destroys the value seemed to be rather stable. Did anyone notice this memory leak before? David, it seems ocaml based xenstore doesn't have such a problem. How mature is the xenstore Mirage-OS variant? Could it be easily integrated into the Xen build? Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [distros-debian-squeeze test] 66523: trouble: blocked/broken
flight 66523 distros-debian-squeeze real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/66523/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: build-armhf 4 capture-logs !broken [st=!broken!] build-armhf-pvops 4 capture-logs !broken [st=!broken!] build-amd64 3 host-install(3) broken REGR. vs. 44428 build-i3863 host-install(3) broken REGR. vs. 44428 build-amd64-pvops 3 host-install(3) broken REGR. vs. 44428 build-i386-pvops 3 host-install(3) broken REGR. vs. 44428 Regressions which are regarded as allowable (not blocking): build-armhf-pvops 3 host-install(3) broken like 44428 build-armhf 3 host-install(3) broken like 44428 Tests which did not succeed, but are not blocking: test-amd64-i386-amd64-squeeze-netboot-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-amd64-squeeze-netboot-pygrub 1 build-check(1)blocked n/a test-amd64-i386-i386-squeeze-netboot-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-i386-squeeze-netboot-pygrub 1 build-check(1) blocked n/a baseline version: flight 44428 jobs: build-amd64 broken build-armhf broken build-i386 broken build-amd64-pvopsbroken build-armhf-pvopsbroken build-i386-pvops broken test-amd64-amd64-amd64-squeeze-netboot-pygrubblocked test-amd64-i386-amd64-squeeze-netboot-pygrub blocked test-amd64-amd64-i386-squeeze-netboot-pygrub blocked test-amd64-i386-i386-squeeze-netboot-pygrub blocked sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers
>>> On 05.07.16 at 20:37, wrote: > +struct vm_event_regs_arm64 { > +uint64_t x0; > +uint64_t x1; > +uint64_t x2; > +uint64_t x3; > +uint64_t x4; > +uint64_t x5; > +uint64_t x6; > +uint64_t x7; > +uint64_t x8; > +uint64_t x9; > +uint64_t x10; > +uint64_t x11; > +uint64_t x12; > +uint64_t x13; > +uint64_t x14; > +uint64_t x15; > +uint64_t x16; > +uint64_t x17; > +uint64_t x18; > +uint64_t x19; > +uint64_t x20; > +uint64_t x21; > +uint64_t x22; > +uint64_t x23; > +uint64_t x24; > +uint64_t x25; > +uint64_t x26; > +uint64_t x27; > +uint64_t x28; > +uint64_t x29; > +uint64_t x30; > +uint64_t pc; > +}; Isn't the stack pointer a fully separate register in aarch64? Not making available something as essential as that seems wrong to me. > @@ -246,10 +311,14 @@ struct vm_event_sharing { > uint32_t _pad; > }; > > +#define VM_EVENT_MAX_DATA_SIZE \ > +(sizeof(struct vm_event_regs_x86) > sizeof(struct vm_event_regs_arm) ? \ > +sizeof(struct vm_event_regs_x86) : sizeof(struct vm_event_regs_arm)) > + > struct vm_event_emul_read_data { > uint32_t size; > /* The struct is used in a union with vm_event_regs_x86. */ > -uint8_t data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)]; > +uint8_t data[VM_EVENT_MAX_DATA_SIZE - sizeof(uint32_t)]; > }; Would there be a problem leaving this alone, i.e. not growing the current limit? I ask because the way VM_EVENT_MAX_DATA_SIZE gets established doesn't look very scalable - just think about how that would look like after half a dozen more architectures got added. > @@ -275,6 +344,7 @@ typedef struct vm_event_st { > union { > union { > struct vm_event_regs_x86 x86; > +struct vm_event_regs_arm arm; > } regs; So the growth in size here then is not really a problem? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] Hypervisor, x86 emulation deprivileged
> -Original Message- > From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of > Andrew Cooper > Sent: 05 July 2016 18:26 > To: Anthony Perard; xen-devel@lists.xen.org; Jan Beulich > Subject: Re: [Xen-devel] [RFC] Hypervisor, x86 emulation deprivileged > > On 05/07/16 12:22, Anthony PERARD wrote: > > Hi, > > > > I've taken over the work from Ben to have a deprivileged mode in the > > hypervisor, but I'm unsure about which direction to take. > > You should begin with an evaluation of available options, identifying > which issues are mitigated, those which are not, and which which new > risks are introduced. > Personally I can't see why we want to invent a new way of executing code in the hypervisor. What's wrong with modelling each emulator as a PV unikernel, running with all the usual domain and vcpu constructs? Doing anything else seems like: a) a lot of work for little gain b) re-inventing the wheel Paul > E.g. > > 1) De-privileging the x86 instruction emulator into hypervisor ring3 > Issues mitigated: > * Out of bounds pointer accesses. > Issues not mitigated: > * On-stack state corruption. > Risks: > * Introduces what is basically 3rd type of vcpu, including all the > subtly that comes user processes. > > > Performance is strictly a secondary consideration; Make something which > works first, then make it fast. Never the opposite way around. > > Another exercise which might be useful is to look at the recent XSAs and > identified which of them could have been mitigated by one of the > suggestions. > > ~Andrew > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 4/6] arm/vm_event: get/set registers
On 07/06/16 10:43, Jan Beulich wrote: On 05.07.16 at 20:37, wrote: >> +struct vm_event_regs_arm64 { >> +uint64_t x0; >> +uint64_t x1; >> +uint64_t x2; >> +uint64_t x3; >> +uint64_t x4; >> +uint64_t x5; >> +uint64_t x6; >> +uint64_t x7; >> +uint64_t x8; >> +uint64_t x9; >> +uint64_t x10; >> +uint64_t x11; >> +uint64_t x12; >> +uint64_t x13; >> +uint64_t x14; >> +uint64_t x15; >> +uint64_t x16; >> +uint64_t x17; >> +uint64_t x18; >> +uint64_t x19; >> +uint64_t x20; >> +uint64_t x21; >> +uint64_t x22; >> +uint64_t x23; >> +uint64_t x24; >> +uint64_t x25; >> +uint64_t x26; >> +uint64_t x27; >> +uint64_t x28; >> +uint64_t x29; >> +uint64_t x30; >> +uint64_t pc; >> +}; > > Isn't the stack pointer a fully separate register in aarch64? Not > making available something as essential as that seems wrong to > me. > >> @@ -246,10 +311,14 @@ struct vm_event_sharing { >> uint32_t _pad; >> }; >> >> +#define VM_EVENT_MAX_DATA_SIZE \ >> +(sizeof(struct vm_event_regs_x86) > sizeof(struct vm_event_regs_arm) ? \ >> +sizeof(struct vm_event_regs_x86) : sizeof(struct vm_event_regs_arm)) >> + >> struct vm_event_emul_read_data { >> uint32_t size; >> /* The struct is used in a union with vm_event_regs_x86. */ >> -uint8_t data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)]; >> +uint8_t data[VM_EVENT_MAX_DATA_SIZE - sizeof(uint32_t)]; >> }; > > Would there be a problem leaving this alone, i.e. not growing the > current limit? I ask because the way VM_EVENT_MAX_DATA_SIZE gets > established doesn't look very scalable - just think about how that > would look like after half a dozen more architectures got added. > >> @@ -275,6 +344,7 @@ typedef struct vm_event_st { >> union { >> union { >> struct vm_event_regs_x86 x86; >> +struct vm_event_regs_arm arm; >> } regs; > > So the growth in size here then is not really a problem? Should be fine for our purposes. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 15/18] arm/altp2m: Add altp2m paging mechanism.
Hi Julien, On 07/04/2016 10:53 PM, Julien Grall wrote: > (CC Tamas) > > Hello Sergej, > > On 04/07/2016 12:45, Sergej Proskurin wrote: >> This commit adds the function p2m_altp2m_lazy_copy implementing the >> altp2m paging mechanism. The function p2m_altp2m_lazy_copy lazily copies >> the hostp2m's mapping into the currently active altp2m view on 2nd stage >> instruction or data access violations. Every altp2m violation generates >> a vm_event. > > I have been working on clean up the abort path (see [1]). Please > rebase your code on top of it. > I will do that, thank you. >> Signed-off-by: Sergej Proskurin >> --- >> Cc: Stefano Stabellini >> Cc: Julien Grall >> --- > > [...] > >> +/* >> + * If the fault is for a not present entry: >> + * if the entry in the host p2m has a valid mfn, copy it and retry >> + * else indicate that outer handler should handle fault >> + * >> + * If the fault is for a present entry: >> + * indicate that outer handler should handle fault >> + */ >> +bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa, >> +unsigned long gva, struct npfec npfec, >> +struct p2m_domain **ap2m) >> +{ >> +struct domain *d = v->domain; >> +struct p2m_domain *hp2m = p2m_get_hostp2m(v->domain); >> +p2m_type_t p2mt; >> +xenmem_access_t xma; >> +paddr_t maddr, mask = 0; >> +gfn_t gfn = _gfn(paddr_to_pfn(gpa)); >> +unsigned int level; >> +unsigned long mattr; >> +int rc = 0; >> + >> +static const p2m_access_t memaccess[] = { >> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac >> +ACCESS(n), >> +ACCESS(r), >> +ACCESS(w), >> +ACCESS(rw), >> +ACCESS(x), >> +ACCESS(rx), >> +ACCESS(wx), >> +ACCESS(rwx), >> +ACCESS(rx2rw), >> +ACCESS(n2rwx), >> +#undef ACCESS >> +}; >> + >> +*ap2m = p2m_get_altp2m(v); >> +if ( *ap2m == NULL) >> +return 0; >> + >> +/* Check if entry is part of the altp2m view */ >> +spin_lock(&(*ap2m)->lock); >> +maddr = __p2m_lookup(*ap2m, gpa, NULL); >> +spin_unlock(&(*ap2m)->lock); >> +if ( maddr != INVALID_PADDR ) >> +return 0; >> + >> +/* Check if entry is part of the host p2m view */ >> +spin_lock(&hp2m->lock); >> +maddr = __p2m_lookup(hp2m, gpa, &p2mt); >> +if ( maddr == INVALID_PADDR ) >> +goto out; >> + >> +rc = __p2m_get_mem_access(hp2m, gfn, &xma); >> +if ( rc ) >> +goto out; >> + >> +rc = p2m_get_gfn_level_and_attr(hp2m, gpa, &level, &mattr); >> +if ( rc ) >> +goto out; > > Can we introduce a function which return the xma, mfn, order, > attribute at once? It will avoid to browse the p2m 3 times which is > really expensive on ARMv7 because the p2m is not mapped in the virtual > address space of Xen. > I was already thinking of at least merging p2m_get_gfn_level_and_attr with __p2m_lookup. But it would also make sense to introduce an entirely new function, which does just that.I believe increasing the overhead of __p2m_lookup would not be a good solution. Thank you. >> +spin_unlock(&hp2m->lock); >> + >> +mask = level_masks[level]; >> + >> +rc = apply_p2m_changes(d, *ap2m, INSERT, >> + pfn_to_paddr(gfn_x(gfn)) & mask, >> + (pfn_to_paddr(gfn_x(gfn)) + >> level_sizes[level]) & mask, >> + maddr & mask, mattr, 0, p2mt, >> + memaccess[xma]); > > The page associated to the MFN is not locked, so another thread could > decide to remove the page from the domain and then the altp2m would > contain an entry to something that does not belong to the domain > anymore. Note that x86 is doing the same. So I am not sure why it is > considered safe there... > If I understand you correctly, unlocking the hp2m->lock after calling apply_p2m_changeswould already solve this issue, right? Thanks. >> +if ( rc ) >> +{ >> +gdprintk(XENLOG_ERR, "failed to set entry for %lx -> %lx p2m >> %lx\n", >> +(unsigned long)pfn_to_paddr(gfn_x(gfn)), (unsigned >> long)(maddr), (unsigned long)*ap2m); >> +domain_crash(hp2m->domain); >> +} >> + >> +return 1; >> + >> +out: >> +spin_unlock(&hp2m->lock); >> +return 0; >> +} >> + >> static void p2m_init_altp2m_helper(struct domain *d, unsigned int i) >> { >> struct p2m_domain *p2m = d->arch.altp2m_p2m[i]; > > [...] > >> @@ -2429,6 +2460,8 @@ static void do_trap_data_abort_guest(struct >> cpu_user_regs *regs, >> const union hsr hsr) >> { >> const struct hsr_dabt dabt = hsr.dabt; >> +struct vcpu *v = current; >> +struct p2m_domain *p2m = NULL; >> int rc; >> mmio_info_t info; >> >> @@ -2449,6 +2482,12 @@ static void do_trap_data_abort_guest(struct >> cpu_user_regs *regs, >> info.gpa = get_faulting_ipa(); >> else >> { >> +
Re: [Xen-devel] [PATCH 17/18] arm/altp2m: Adjust debug information to altp2m.
Hi Julien, On 07/04/2016 10:58 PM, Julien Grall wrote: > Hello Sergej, > > On 04/07/2016 12:45, Sergej Proskurin wrote: >> Signed-off-by: Sergej Proskurin >> --- >> Cc: Stefano Stabellini >> Cc: Julien Grall >> --- >> xen/arch/arm/p2m.c | 6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 96892a5..de97a12 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -51,7 +51,8 @@ static bool_t p2m_mapping(lpae_t pte) >> >> void p2m_dump_info(struct domain *d) >> { >> -struct p2m_domain *p2m = &d->arch.p2m; >> +struct vcpu *v = current; > > This is wrong, p2m_dump_info can be called with d != current->domain. > Please try to look at how the caller may use the function before doing > any modification. > > In this case, I think you want to dump the info for the hostp2m and > every altp2ms. > Fair enough. I will provide the information ofall altp2m views. Thank you. >> +struct p2m_domain *p2m = altp2m_active(d) ? p2m_get_altp2m(v) : >> p2m_get_hostp2m(d); >> >> spin_lock(&p2m->lock); >> printk("p2m mappings for domain %d (vmid %d):\n", >> @@ -71,7 +72,8 @@ void memory_type_changed(struct domain *d) >> >> void dump_p2m_lookup(struct domain *d, paddr_t addr) >> { >> -struct p2m_domain *p2m = &d->arch.p2m; >> +struct vcpu *v = current; > > Ditto. > Ok, thankyou. >> +struct p2m_domain *p2m = altp2m_active(d) ? p2m_get_altp2m(v) : >> p2m_get_hostp2m(d); >> >> printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr); >> >> > > Regards, > Cheers, ~Sergej ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 18/18] arm/altp2m: Extend xen-access for altp2m on ARM.
Hi Razvan, On 07/04/2016 03:38 PM, Razvan Cojocaru wrote: > On 07/04/16 14:45, Sergej Proskurin wrote: >> Signed-off-by: Sergej Proskurin >> --- >> Cc: Razvan Cojocaru >> Cc: Tamas K Lengyel >> Cc: Ian Jackson >> Cc: Wei Liu >> --- >> tools/tests/xen-access/xen-access.c | 11 +-- >> 1 file changed, 9 insertions(+), 2 deletions(-) > Fair enough, looks like a trivial change. > > Acked-by: Razvan Cojocaru > Great, thank you. Cheers, ~Sergej ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 02/18] arm/altp2m: Add first altp2m HVMOP stubs.
Hi Julien, On 07/05/2016 12:19 PM, Julien Grall wrote: > Hello Sergej, > > On 04/07/16 12:45, Sergej Proskurin wrote: >> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) >> +{ >> +struct xen_hvm_altp2m_op a; >> +struct domain *d = NULL; >> +int rc = 0; >> + >> +if ( !hvm_altp2m_supported() ) >> +return -EOPNOTSUPP; >> + >> +if ( copy_from_guest(&a, arg, 1) ) >> +return -EFAULT; >> + >> +if ( a.pad1 || a.pad2 || >> + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || >> + (a.cmd < HVMOP_altp2m_get_domain_state) || >> + (a.cmd > HVMOP_altp2m_change_gfn) ) >> +return -EINVAL; >> + >> +d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? >> +rcu_lock_domain_by_any_id(a.domain) : >> rcu_lock_current_domain(); >> + >> +if ( d == NULL ) >> +return -ESRCH; >> + >> +if ( (a.cmd != HVMOP_altp2m_get_domain_state) && >> + (a.cmd != HVMOP_altp2m_set_domain_state) && >> + !d->arch.altp2m_active ) >> +{ >> +rc = -EOPNOTSUPP; >> +goto out; >> +} >> + >> +if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) ) >> +goto out; > > I think this is the best place to ask a couple of questions related to > who can access altp2m. Based on this call, a guest is allowed to > manage its own altp2m. Can you explain why we would want a guest to do > that? > On x86, altp2m might be used by the guest in the #VE (Virtualization Exception). On ARM, there is indeed not necessary for a guest to access altp2m. Could you provide me with information, how to best restrict non-privileged guests (not only dom0) from accessing these HVMOPs? Can thisbedone by means of xsm? Thank you. > Also, I have noticed that a guest is allowed to disable ALTP2M on ARM > because it set any param (x86 has some restriction on it). Similarly, > the ALTP2M parameter can be set multiple time. > Same here. Cheers, ~Sergej ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [Question] Do we support limiting IOPS
Hi, I was searching for limiting IOPS for the disk attached to XenServer VMs. And I couldn't find a way to limit the IOPS. So, I was wondering whether XenServer actually supports limiting IOPS or not. If yes, what are the ways, both user and programmatical, to do so. Thanks & Regards, Ganesh. DISCLAIMER == This e-mail may contain privileged and confidential information which is the property of Accelerite, a Persistent Systems business. It is intended only for the use of the individual or entity to which it is addressed. If you are not the intended recipient, you are not authorized to read, retain, copy, print, distribute or use this message. If you have received this communication in error, please notify the sender and delete all copies of this message. Accelerite, a Persistent Systems business does not accept any liability for virus infected mails. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi! Quoting Dirk Behme (2016-06-30 03:32:32) > Some clocks might be used by the Xen hypervisor and not by the Linux > kernel. If these are not registered by the Linux kernel, they might be > disabled by clk_disable_unused() as the kernel doesn't know that they > are used. The clock of the serial console handled by Xen is one > example for this. It might be disabled by clk_disable_unused() which > stops the whole serial output, even from Xen, then. This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me). This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below. > > Up to now, the workaround for this has been to use the Linux kernel > command line parameter 'clk_ignore_unused'. See Xen bug > > http://bugs.xenproject.org/xen/bug/45 clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating. Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them. > > too. > > To fix this, we will add the "unused" clocks in Xen to the hypervisor > node. The Linux kernel has to register the clocks from the hypervisor > node, then. > > Therefore, check if there is a "clocks" entry in the hypervisor node > and if so register the given clocks to the Linux kernel clock > framework and with this mark them as used. This prevents the clocks > from being disabled. > > Signed-off-by: Dirk Behme > --- > Changes in v2: > - Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git > for-linus-4.8 > - Add changes to Documentation/devicetree/bindings/arm/xen.txt > > Documentation/devicetree/bindings/arm/xen.txt | 11 +++ > arch/arm/xen/enlighten.c | 47 > +++ > 2 files changed, 58 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/xen.txt > b/Documentation/devicetree/bindings/arm/xen.txt > index c9b9321..55dfd3b 100644 > --- a/Documentation/devicetree/bindings/arm/xen.txt > +++ b/Documentation/devicetree/bindings/arm/xen.txt > @@ -17,6 +17,17 @@ the following properties: >A GIC node is also required. >This property is unnecessary when booting Dom0 using ACPI. > > +Optional properties: > + > +- clocks: one or more clocks to be registered. s/registered/consumed/ For appropriate DT binding script to steal I picked one at random: Documentation/devicetree/bindings/clock/ti/clockdomain.txt > + Xen hypervisor drivers might replace native drivers, resulting in > + clocks not registered by these native drivers. To avoid that these > + unregistered clocks are disabled, then, e.g. by clk_disable_unused(), > + register them in the hypervisor node. > + An example for this are the clocks of the serial driver. If the clocks > + used by the serial hardware interface are not registered by the serial > + driver the serial output might stop once clk_disable_unused() is called. > + > To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" > node > under /hypervisor with following parameters: > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > index 47acb36..5c546d0 100644 > --- a/arch/arm/xen/enlighten.c > +++ b/arch/arm/xen/enlighten.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include s/clk-provider.h/clk.h/ clk-provider.h is only used for providers and this bit of code is a consumer. > #include > #include > #include > @@ -444,6 +445,52 @@ static int __init xen_pm_init(void) > } > late_initcall(xen_pm_init); > > +/* > + * Check if we want to register some clocks, that they > + * are not freed because unused by clk_disable_unused(). > + * E.g. the serial console clock. > + */ > +static int __init xen_arm_register_clks(void) > +{ > + struct clk *clk; > + struct device_node *xen_node; > + unsigned int i, count; > + int ret = 0; > + > + xen_node = of_find_compatible_node(NULL, NULL, "xen,xen"); > + if (!xen_node) { > + pr_err("Xen support was detected before, but it has > disappeared\n"); > + return -EINVAL; > + } > + > + count = of_clk_get_parent_count(xen_node); > + if (!count) > + goto out; > + > + for (i = 0; i < count; i++) { > + clk = of_clk_get(xen_node, i); Is there a struct device we can use here? It would be better to use devm_clk_get if possible. Regards, Mike > + if (IS_ERR(clk)) { > + pr_err("Xen fa
[Xen-devel] Kernel panic on Xen virtualisation in Debian
Hello ! I am posting Xen virtualisation bug links to this e-mail address, because I wasn't able to find the Xen specific bugtracker list. This bug has been discovered in 2015 and so far it hasn't been resolved through the Debian/Kernel bug lists. I submit the links to bug reports for you. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=804079 https://bugzilla.kernel.org/show_bug.cgi?id=107171 Kind regards, Jan Prunk -- Jan Prunk http://prunk.si PGP Pubkey http://prunk.si/0x9FD7F151.txt ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 0/7] xen-pciback: misc cleanup
On 06/07/16 07:47, Jan Beulich wrote: > The first five patches are the result of the requested split of > "xen-pciback: clean up {bar,rom}_init()", with Boris'es R-b > dropped despite there not being any functional change (the > mechanical change appears too significant to retain it). The > remaining two are a follow-up to the recent "xen/pciback: Fix > conf_space read/write overlap check." and I hope the folding > in of the formatting correction of two lines touched anyway is > going to be acceptable this time. Applied to for-linus-4.8, thanks. David ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 16/17] libxc/xc_dom_arm: Copy ACPI tables to guest space
Hi Stefano, On 05/07/16 18:13, Stefano Stabellini wrote: On Thu, 23 Jun 2016, Julien Grall wrote: On 23/06/2016 04:17, Shannon Zhao wrote: From: Shannon Zhao Copy all the ACPI tables to guest space so that UEFI or guest could access them. Signed-off-by: Shannon Zhao --- tools/libxc/xc_dom_arm.c | 51 1 file changed, 51 insertions(+) diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c index 64a8b67..6a0a5b7 100644 --- a/tools/libxc/xc_dom_arm.c +++ b/tools/libxc/xc_dom_arm.c @@ -63,6 +63,47 @@ static int setup_pgtables_arm(struct xc_dom_image *dom) /* */ +static int xc_dom_copy_acpi(struct xc_dom_image *dom) +{ +int rc, i; +uint32_t pages_num = ROUNDUP(dom->acpitable_size, XC_PAGE_SHIFT) >> + XC_PAGE_SHIFT; +const xen_pfn_t base = GUEST_ACPI_BASE >> XC_PAGE_SHIFT; +xen_pfn_t *p2m; +void *acpi_pages; + +p2m = malloc(pages_num * sizeof(*p2m)); +for (i = 0; i < pages_num; i++) +p2m[i] = base + i; + +rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid, + pages_num, 0, 0, p2m); H... it looks like this is working because libxl is setting the maximum size of the domain with some slack (1MB). However, I guess the slack was for something else. Wei, Stefano, Ian, can you confirm? If I recall correctly, the slack is a magic value coming from the ancient history of toolstacks. Does it mean we would need to update the slack to take into account the ACPI blob? Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 07/17] libxl/arm: Construct ACPI GTDT table
Hi Stefano, On 05/07/16 17:42, Stefano Stabellini wrote: On Mon, 27 Jun 2016, Julien Grall wrote: On 27/06/16 02:44, Shannon Zhao wrote: On 2016/6/24 0:26, Julien Grall wrote: On 23/06/16 04:16, Shannon Zhao wrote: From: Shannon Zhao Construct GTDT table with the interrupt information of timers. Signed-off-by: Shannon Zhao --- tools/libxl/libxl_arm_acpi.c | 28 1 file changed, 28 insertions(+) diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index d5ffedf..de863f4 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -39,6 +39,9 @@ typedef uint64_t u64; #define ACPI_BUILD_APPNAME6 "XenARM" #define ACPI_BUILD_APPNAME4 "Xen " +#define ACPI_LEVEL_SENSITIVE(u8) 0x00 +#define ACPI_ACTIVE_LOW (u8) 0x01 + Why did not you include actypes.h rather than define these two defines? If we include actypes.h, there will be some compiling errors. ../../xen/include/acpi/actypes.h:55:2: error: #error ACPI_MACHINE_WIDTH not defined #error ACPI_MACHINE_WIDTH not defined ^ ../../xen/include/acpi/actypes.h:130:9: error: unknown type name 'COMPILER_DEPENDENT_UINT64' typedef COMPILER_DEPENDENT_UINT64 UINT64; ^ ../../xen/include/acpi/actypes.h:131:9: error: unknown type name 'COMPILER_DEPENDENT_INT64' typedef COMPILER_DEPENDENT_INT64 INT64; ^ ../../xen/include/acpi/actypes.h:202:2: error: #error unknown ACPI_MACHINE_WIDTH #error unknown ACPI_MACHINE_WIDTH ^ ../../xen/include/acpi/actypes.h:207:9: error: unknown type name 'acpi_native_uint' typedef acpi_native_uint acpi_size; ^ ../../xen/include/acpi/actypes.h:617:3: error: unknown type name 'acpi_io_address' acpi_io_address pblk_address; Yeah, it maybe can be solved by defining ACPI_MACHINE_WIDTH and COMPILER_DEPENDENT_INT64 here, but since we only needs ACPI_LEVEL_SENSITIVE and ACPI_ACTIVE_LOW, I think it's ok to define them here. We should avoid to redefine value as much as possible. The 2 missing values are easy to define (see below) so there is no point to redefine in a less obvious way: no comment to explain what the values are for, and only a part of the set defined. #define ACPI_MACHINE_WIDTH BITS_PER_LONG #define COMPILER_DEPENDENT_INT64 int64_t Wei, Ian, Stefano, do you have any opinions? I think you are right that we should avoid redefining ACPI_LEVEL_SENSITIVE and ACPI_ACTIVE_LOW. But I think if possible we should also avoid redefining ACPI_MACHINE_WIDTH and COMPILER_DEPENDENT_INT64. If possible, we should include the header file with those definitions too (acpi/platform/acenv.h ?). acenv.h is including aclinux.h with contains a lot of hypervisor specific include. So we would need to rework the include to use it in the toolstack. Or maybe this can be found in /usr/include/? -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 6/7] minor fixes (formatting, comments, unused includes etc.)
Hi Corneliu, On 05/07/16 15:30, Corneliu ZUZU wrote: Minor fixes: - remove some empty lines - remove some unused includes - multi-line comment fixes - 80-columns formatting fixes Signed-off-by: Corneliu ZUZU Acked-by: Julien Grall Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 18/20] libxl/acpi: Add ACPI e820 entry
(CC Stefano) Hi Boris, On 05/07/16 20:05, Boris Ostrovsky wrote: Add entry for ACPI tables created for PVHv2 guests to e820 map. Signed-off-by: Boris Ostrovsky --- New patch tools/libxc/include/xc_dom.h |4 tools/libxl/libxl_dom.c |8 tools/libxl/libxl_x86.c | 11 +++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h index 6cb10c4..ec2da14 100644 --- a/tools/libxc/include/xc_dom.h +++ b/tools/libxc/include/xc_dom.h @@ -102,6 +102,10 @@ struct xc_dom_image { xen_vaddr_t virt_alloc_end; xen_vaddr_t bsd_symtab_start; +/* ACPI tables (PVHv2 only) */ +xen_pfn_t acpi_pfn; +xen_pfn_t acpi_pages; + It would be good if we can share the fields with ARM (see [1]). Regards, [1] https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00301.html -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable test] 96688: regressions - FAIL
flight 96688 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/96688/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-xsm 6 xen-boot fail REGR. vs. 96611 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-credit2 15 guest-start/debian.repeatfail like 96558 build-i386-rumpuserxen6 xen-buildfail like 96611 build-amd64-rumpuserxen 6 xen-buildfail like 96611 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 96611 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 96611 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 96611 test-amd64-amd64-xl-rtds 9 debian-install fail like 96611 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 96611 Tests which did not succeed, but are not blocking: test-amd64-amd64-rumpuserxen-amd64 1 build-check(1) blocked n/a test-amd64-i386-rumpuserxen-i386 1 build-check(1) blocked n/a test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass version targeted for testing: xen fea586d801f75317cb8cf593e8beba842391da62 baseline version: xen bb4f41b3dff831faaf5a3248e0ecd123024d7f8f Last test of basis96611 2016-07-04 01:58:03 Z2 days Failing since 96634 2016-07-04 12:50:54 Z1 days3 attempts Testing same since96688 2016-07-05 18:49:37 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Corneliu ZUZU Jan Beulich Kevin Tian Razvan Cojocaru Tamas K Lengyel Wei Liu jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt
Re: [Xen-devel] [PATCH v2 3/3] xen: support runqueue steal time on xen
On Wed, 6 Jul 2016, Juergen Gross wrote: > Up to now reading the stolen time of a remote cpu was not possible in a > performant way under Xen. This made support of runqueue steal time via > paravirt_steal_rq_enabled impossible. > > With the addition of an appropriate hypervisor interface this is now > possible, so add the support. > > Signed-off-by: Juergen Gross Reviewed-by: Stefano Stabellini > V2: removed static variable as requested by Stefano Stabellini > --- > drivers/xen/time.c | 40 +++- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/drivers/xen/time.c b/drivers/xen/time.c > index 2257b66..a7fe35b 100644 > --- a/drivers/xen/time.c > +++ b/drivers/xen/time.c > @@ -47,27 +47,31 @@ static u64 get64(const u64 *p) > return ret; > } > > -/* > - * Runstate accounting > - */ > -void xen_get_runstate_snapshot(struct vcpu_runstate_info *res) > +static void xen_get_runstate_snapshot_cpu(struct vcpu_runstate_info *res, > + unsigned int cpu) > { > u64 state_time; > struct vcpu_runstate_info *state; > > BUG_ON(preemptible()); > > - state = this_cpu_ptr(&xen_runstate); > + state = per_cpu_ptr(&xen_runstate, cpu); > > - /* > - * The runstate info is always updated by the hypervisor on > - * the current CPU, so there's no need to use anything > - * stronger than a compiler barrier when fetching it. > - */ > do { > state_time = get64(&state->state_entry_time); > + rmb(); /* Hypervisor might update data. */ > *res = READ_ONCE(*state); > - } while (get64(&state->state_entry_time) != state_time); > + rmb(); /* Hypervisor might update data. */ > + } while (get64(&state->state_entry_time) != state_time || > + (state_time & XEN_RUNSTATE_UPDATE)); > +} > + > +/* > + * Runstate accounting > + */ > +void xen_get_runstate_snapshot(struct vcpu_runstate_info *res) > +{ > + xen_get_runstate_snapshot_cpu(res, smp_processor_id()); > } > > /* return true when a vcpu could run but has no real cpu to run on */ > @@ -80,8 +84,7 @@ static u64 xen_steal_clock(int cpu) > { > struct vcpu_runstate_info state; > > - BUG_ON(cpu != smp_processor_id()); > - xen_get_runstate_snapshot(&state); > + xen_get_runstate_snapshot_cpu(&state, cpu); > return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline]; > } > > @@ -98,11 +101,14 @@ void xen_setup_runstate_info(int cpu) > > void __init xen_time_setup_guest(void) > { > + bool xen_runstate_remote; > + > + xen_runstate_remote = !HYPERVISOR_vm_assist(VMASST_CMD_enable, > + VMASST_TYPE_runstate_update_flag); > + > pv_time_ops.steal_clock = xen_steal_clock; > > static_key_slow_inc(¶virt_steal_enabled); > - /* > - * We can't set paravirt_steal_rq_enabled as this would require the > - * capability to read another cpu's runstate info. > - */ > + if (xen_runstate_remote) > + static_key_slow_inc(¶virt_steal_rq_enabled); > } > -- > 2.6.6 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 16/17] libxc/xc_dom_arm: Copy ACPI tables to guest space
On Wed, 6 Jul 2016, Julien Grall wrote: > Hi Stefano, > > On 05/07/16 18:13, Stefano Stabellini wrote: > > On Thu, 23 Jun 2016, Julien Grall wrote: > > > On 23/06/2016 04:17, Shannon Zhao wrote: > > > > From: Shannon Zhao > > > > > > > > Copy all the ACPI tables to guest space so that UEFI or guest could > > > > access them. > > > > > > > > Signed-off-by: Shannon Zhao > > > > --- > > > > tools/libxc/xc_dom_arm.c | 51 > > > > > > > > 1 file changed, 51 insertions(+) > > > > > > > > diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c > > > > index 64a8b67..6a0a5b7 100644 > > > > --- a/tools/libxc/xc_dom_arm.c > > > > +++ b/tools/libxc/xc_dom_arm.c > > > > @@ -63,6 +63,47 @@ static int setup_pgtables_arm(struct xc_dom_image > > > > *dom) > > > > > > > > /* > > > > > > > > */ > > > > > > > > +static int xc_dom_copy_acpi(struct xc_dom_image *dom) > > > > +{ > > > > +int rc, i; > > > > +uint32_t pages_num = ROUNDUP(dom->acpitable_size, XC_PAGE_SHIFT) >> > > > > + XC_PAGE_SHIFT; > > > > +const xen_pfn_t base = GUEST_ACPI_BASE >> XC_PAGE_SHIFT; > > > > +xen_pfn_t *p2m; > > > > +void *acpi_pages; > > > > + > > > > +p2m = malloc(pages_num * sizeof(*p2m)); > > > > +for (i = 0; i < pages_num; i++) > > > > +p2m[i] = base + i; > > > > + > > > > +rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid, > > > > + pages_num, 0, 0, p2m); > > > > > > H... it looks like this is working because libxl is setting the > > > maximum > > > size of the domain with some slack (1MB). However, I guess the slack was > > > for > > > something else. Wei, Stefano, Ian, can you confirm? > > > > If I recall correctly, the slack is a magic value coming from the > > ancient history of toolstacks. > > Does it mean we would need to update the slack to take into account the ACPI > blob? Yes, we need to take into account the ACPI blob. Probably not in the slack but directly in mam_memkb. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 07/17] libxl/arm: Construct ACPI GTDT table
On Wed, 6 Jul 2016, Julien Grall wrote: > Hi Stefano, > > On 05/07/16 17:42, Stefano Stabellini wrote: > > On Mon, 27 Jun 2016, Julien Grall wrote: > > > On 27/06/16 02:44, Shannon Zhao wrote: > > > > On 2016/6/24 0:26, Julien Grall wrote: > > > > > On 23/06/16 04:16, Shannon Zhao wrote: > > > > > > From: Shannon Zhao > > > > > > > > > > > > Construct GTDT table with the interrupt information of timers. > > > > > > > > > > > > Signed-off-by: Shannon Zhao > > > > > > --- > > > > > > tools/libxl/libxl_arm_acpi.c | 28 > > > > > > 1 file changed, 28 insertions(+) > > > > > > > > > > > > diff --git a/tools/libxl/libxl_arm_acpi.c > > > > > > b/tools/libxl/libxl_arm_acpi.c > > > > > > index d5ffedf..de863f4 100644 > > > > > > --- a/tools/libxl/libxl_arm_acpi.c > > > > > > +++ b/tools/libxl/libxl_arm_acpi.c > > > > > > @@ -39,6 +39,9 @@ typedef uint64_t u64; > > > > > > #define ACPI_BUILD_APPNAME6 "XenARM" > > > > > > #define ACPI_BUILD_APPNAME4 "Xen " > > > > > > > > > > > > +#define ACPI_LEVEL_SENSITIVE(u8) 0x00 > > > > > > +#define ACPI_ACTIVE_LOW (u8) 0x01 > > > > > > + > > > > > > > > > > Why did not you include actypes.h rather than define these two > > > > > defines? > > > > If we include actypes.h, there will be some compiling errors. > > > > > > > > ../../xen/include/acpi/actypes.h:55:2: error: #error ACPI_MACHINE_WIDTH > > > > not defined > > > >#error ACPI_MACHINE_WIDTH not defined > > > > ^ > > > > ../../xen/include/acpi/actypes.h:130:9: error: unknown type name > > > > 'COMPILER_DEPENDENT_UINT64' > > > >typedef COMPILER_DEPENDENT_UINT64 UINT64; > > > >^ > > > > ../../xen/include/acpi/actypes.h:131:9: error: unknown type name > > > > 'COMPILER_DEPENDENT_INT64' > > > >typedef COMPILER_DEPENDENT_INT64 INT64; > > > >^ > > > > ../../xen/include/acpi/actypes.h:202:2: error: #error unknown > > > > ACPI_MACHINE_WIDTH > > > >#error unknown ACPI_MACHINE_WIDTH > > > > ^ > > > > ../../xen/include/acpi/actypes.h:207:9: error: unknown type name > > > > 'acpi_native_uint' > > > >typedef acpi_native_uint acpi_size; > > > >^ > > > > ../../xen/include/acpi/actypes.h:617:3: error: unknown type name > > > > 'acpi_io_address' > > > > acpi_io_address pblk_address; > > > > > > > > Yeah, it maybe can be solved by defining ACPI_MACHINE_WIDTH and > > > > COMPILER_DEPENDENT_INT64 here, but since we only needs > > > > ACPI_LEVEL_SENSITIVE and ACPI_ACTIVE_LOW, I think it's ok to define them > > > > here. > > > > > > We should avoid to redefine value as much as possible. The 2 missing > > > values > > > are easy to define (see below) so there is no point to redefine in a less > > > obvious way: no comment to explain what the values are for, and only a > > > part of > > > the set defined. > > > > > > #define ACPI_MACHINE_WIDTH BITS_PER_LONG > > > #define COMPILER_DEPENDENT_INT64 int64_t > > > > > > Wei, Ian, Stefano, do you have any opinions? > > > > I think you are right that we should avoid redefining > > ACPI_LEVEL_SENSITIVE and ACPI_ACTIVE_LOW. But I think if possible we > > should also avoid redefining ACPI_MACHINE_WIDTH and > > COMPILER_DEPENDENT_INT64. If possible, we should include the header file > > with those definitions too (acpi/platform/acenv.h ?). > > acenv.h is including aclinux.h with contains a lot of hypervisor specific > include. So we would need to rework the include to use it in the toolstack. > > Or maybe this can be found in /usr/include/? I was thinking of the one in the Xen tree. If it is not possible to include it, then OK. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 04/17] xen: Use the typesafe mfn and gfn in map_mmio_regions...
On Tue, 28 Jun 2016, Julien Grall wrote: > to avoid mixing machine frame with guest frame. > > Signed-off-by: Julien Grall > Acked-by: Jan Beulich Acked-by: Stefano Stabellini > --- > Cc: Stefano Stabellini > Cc: Jan Beulich > Cc: Andrew Cooper > Cc: George Dunlap > Cc: Ian Jackson > Cc: Konrad Rzeszutek Wilk > Cc: Tim Deegan > Cc: Wei Liu > > Changes in v3: > - Use mfn_add when it is possible > - Add Jan's acked-by > --- > xen/arch/arm/domain_build.c | 4 ++-- > xen/arch/arm/gic-v2.c| 4 ++-- > xen/arch/arm/p2m.c | 22 +++--- > xen/arch/arm/platforms/exynos5.c | 8 > xen/arch/arm/platforms/omap5.c | 16 > xen/arch/arm/vgic-v2.c | 4 ++-- > xen/arch/x86/mm/p2m.c| 18 ++ > xen/common/domctl.c | 4 ++-- > xen/include/xen/p2m-common.h | 8 > 9 files changed, 45 insertions(+), 43 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 9035486..49185f0 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1036,9 +1036,9 @@ static int map_range_to_domain(const struct > dt_device_node *dev, > if ( need_mapping ) > { > res = map_mmio_regions(d, > - paddr_to_pfn(addr), > + _gfn(paddr_to_pfn(addr)), > DIV_ROUND_UP(len, PAGE_SIZE), > - paddr_to_pfn(addr)); > + _mfn(paddr_to_pfn(addr))); > if ( res < 0 ) > { > printk(XENLOG_ERR "Unable to map 0x%"PRIx64 > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 4e2f4c7..3893ece 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -601,9 +601,9 @@ static int gicv2_map_hwdown_extra_mappings(struct domain > *d) > d->domain_id, v2m_data->addr, v2m_data->size, > v2m_data->spi_start, v2m_data->nr_spis); > > -ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr), > +ret = map_mmio_regions(d, _gfn(paddr_to_pfn(v2m_data->addr)), > DIV_ROUND_UP(v2m_data->size, PAGE_SIZE), > -paddr_to_pfn(v2m_data->addr)); > +_mfn(paddr_to_pfn(v2m_data->addr))); > if ( ret ) > { > printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n", > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 0395a40..34563bb 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d, > } > > int map_mmio_regions(struct domain *d, > - unsigned long start_gfn, > + gfn_t start_gfn, > unsigned long nr, > - unsigned long mfn) > + mfn_t mfn) > { > return apply_p2m_changes(d, INSERT, > - pfn_to_paddr(start_gfn), > - pfn_to_paddr(start_gfn + nr), > - pfn_to_paddr(mfn), > + pfn_to_paddr(gfn_x(start_gfn)), > + pfn_to_paddr(gfn_x(start_gfn) + nr), > + pfn_to_paddr(mfn_x(mfn)), > MATTR_DEV, 0, p2m_mmio_direct, > d->arch.p2m.default_access); > } > > int unmap_mmio_regions(struct domain *d, > - unsigned long start_gfn, > + gfn_t start_gfn, > unsigned long nr, > - unsigned long mfn) > + mfn_t mfn) > { > return apply_p2m_changes(d, REMOVE, > - pfn_to_paddr(start_gfn), > - pfn_to_paddr(start_gfn + nr), > - pfn_to_paddr(mfn), > + pfn_to_paddr(gfn_x(start_gfn)), > + pfn_to_paddr(gfn_x(start_gfn) + nr), > + pfn_to_paddr(mfn_x(mfn)), > MATTR_DEV, 0, p2m_invalid, > d->arch.p2m.default_access); > } > @@ -1280,7 +1280,7 @@ int map_dev_mmio_region(struct domain *d, > if ( !(nr && iomem_access_permitted(d, mfn, mfn + nr - 1)) ) > return 0; > > -res = map_mmio_regions(d, start_gfn, nr, mfn); > +res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn)); > if ( res < 0 ) > { > printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n", > diff --git a/xen/arch/arm/platforms/exynos5.c > b/xen/arch/arm/platforms/exynos5.c > index bf4964d..c43934f 100644 > --- a/xen/arch/arm/platforms/exynos5.c > +++ b/xen/arch/arm/platforms/exynos5.c > @@ -83,12 +83,12 @@ static int exynos5_init_time(void) > static
Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c
On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote: > >>> On 05.07.16 at 20:33, wrote: > > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: > >> >>> On 25.05.16 at 18:45, wrote: > >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: > >> >> >>> On 15.04.16 at 14:33, wrote: > >> >> > --- a/xen/arch/x86/efi/stub.c > >> >> > +++ b/xen/arch/x86/efi/stub.c > >> >> > @@ -8,6 +8,14 @@ > >> >> > const bool_t efi_enabled = 0; > >> >> > #endif > >> >> > > >> >> > +struct efi __read_mostly efi = { > >> >> > + .acpi= EFI_INVALID_TABLE_ADDR, > >> >> > + .acpi20 = EFI_INVALID_TABLE_ADDR, > >> >> > + .mps = EFI_INVALID_TABLE_ADDR, > >> >> > + .smbios = EFI_INVALID_TABLE_ADDR, > >> >> > + .smbios3 = EFI_INVALID_TABLE_ADDR > >> >> > +}; > >> >> > >> >> I don't view duplicating this here as a good approach - you'd better > >> >> move the existing instance elsewhere. If this was a temporary thing > >> >> (until a later patch), it might be acceptable, but since building > >> >> without > >> >> EFI support will need to remain an option (for people using older tool > >> >> chains), I don't expect a later patch to remove this. > >> > > >> > Do you think about separate C file which should contain efi struct > >> > and should be included in stub.c and runtime.c? Or anything else? > >> > >> A separate file seems to be overkill. Just move it to some other > >> existing file; I'm sure some sensible place can be found. > > > > This solution is not perfect, however, I cannot find better place for > > efi struct. If you have one then drop me a line. > > common/kernel.c or common/lib.c. This means that we must delete efi struct initialization from xen/common/efi/runtime.c and xen/arch/x86/efi/stub.c and put it in one of both files mentioned by you. Is it OK for you? Daniel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 07/17] xen: Use a typesafe to define INVALID_GFN
On Tue, 28 Jun 2016, Julien Grall wrote: > Also take the opportunity to convert arch/x86/debug.c to the typesafe gfn. > > Signed-off-by: Julien Grall For the ARM bits Acked-by: Stefano Stabellini > --- > Cc: Mukesh Rathor > Cc: Jan Beulich > Cc: Andrew Cooper > Cc: Paul Durrant > Cc: Boris Ostrovsky > Cc: Suravee Suthikulpanit > Cc: Jun Nakajima > Cc: Kevin Tian > Cc: George Dunlap > Cc: Tim Deegan > Cc: Feng Wu > > Changes in v5: > - Patch added > --- > xen/arch/arm/p2m.c | 4 ++-- > xen/arch/x86/debug.c| 18 +- > xen/arch/x86/domain.c | 2 +- > xen/arch/x86/hvm/emulate.c | 7 --- > xen/arch/x86/hvm/hvm.c | 6 +++--- > xen/arch/x86/hvm/ioreq.c| 8 > xen/arch/x86/hvm/svm/nestedsvm.c| 2 +- > xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- > xen/arch/x86/mm/altp2m.c| 2 +- > xen/arch/x86/mm/hap/guest_walk.c| 10 +- > xen/arch/x86/mm/hap/nested_ept.c| 2 +- > xen/arch/x86/mm/p2m-pod.c | 6 +++--- > xen/arch/x86/mm/p2m.c | 18 +- > xen/arch/x86/mm/shadow/common.c | 2 +- > xen/arch/x86/mm/shadow/multi.c | 2 +- > xen/arch/x86/mm/shadow/private.h| 2 +- > xen/drivers/passthrough/amd/iommu_map.c | 2 +- > xen/drivers/passthrough/vtd/iommu.c | 4 ++-- > xen/drivers/passthrough/x86/iommu.c | 2 +- > xen/include/asm-x86/guest_pt.h | 4 ++-- > xen/include/asm-x86/p2m.h | 2 +- > xen/include/xen/mm.h| 2 +- > 22 files changed, 57 insertions(+), 56 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d690602..c938dde 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -479,7 +479,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t > gfn, > } > > /* If request to get default access. */ > -if ( gfn_x(gfn) == INVALID_GFN ) > +if ( gfn_eq(gfn, INVALID_GFN) ) > { > *access = memaccess[p2m->default_access]; > return 0; > @@ -1879,7 +1879,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, > uint32_t nr, > p2m->mem_access_enabled = true; > > /* If request to set default access. */ > -if ( gfn_x(gfn) == INVALID_GFN ) > +if ( gfn_eq(gfn, INVALID_GFN) ) > { > p2m->default_access = a; > return 0; > diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c > index 3479f7c..1ce0e89 100644 > --- a/xen/arch/x86/debug.c > +++ b/xen/arch/x86/debug.c > @@ -44,8 +44,7 @@ typedef unsigned char dbgbyte_t; > > /* Returns: mfn for the given (hvm guest) vaddr */ > static mfn_t > -dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, > -unsigned long *gfn) > +dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, gfn_t *gfn) > { > mfn_t mfn; > uint32_t pfec = PFEC_page_present; > @@ -53,14 +52,14 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int > toaddr, > > DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id); > > -*gfn = paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec); > -if ( *gfn == INVALID_GFN ) > +*gfn = _gfn(paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec)); > +if ( gfn_eq(*gfn, INVALID_GFN) ) > { > DBGP2("kdb:bad gfn from gva_to_gfn\n"); > return INVALID_MFN; > } > > -mfn = get_gfn(dp, *gfn, &gfntype); > +mfn = get_gfn(dp, gfn_x(*gfn), &gfntype); > if ( p2m_is_readonly(gfntype) && toaddr ) > { > DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype); > @@ -72,7 +71,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, > > if ( mfn_eq(mfn, INVALID_MFN) ) > { > -put_gfn(dp, *gfn); > +put_gfn(dp, gfn_x(*gfn)); > *gfn = INVALID_GFN; > } > > @@ -165,7 +164,8 @@ unsigned int dbg_rw_guest_mem(struct domain *dp, void * > __user gaddr, > char *va; > unsigned long addr = (unsigned long)gaddr; > mfn_t mfn; > -unsigned long gfn = INVALID_GFN, pagecnt; > +gfn_t gfn = INVALID_GFN; > +unsigned long pagecnt; > > pagecnt = min_t(long, PAGE_SIZE - (addr & ~PAGE_MASK), len); > > @@ -189,8 +189,8 @@ unsigned int dbg_rw_guest_mem(struct domain *dp, void * > __user gaddr, > } > > unmap_domain_page(va); > -if ( gfn != INVALID_GFN ) > -put_gfn(dp, gfn); > +if ( !gfn_eq(gfn, INVALID_GFN) ) > +put_gfn(dp, gfn_x(gfn)); > > addr += pagecnt; > buf += pagecnt; > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index bb59247..c8c7e2d 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -783,7 +783,7 @@ int arch_domain_soft_reset(struct domain *d) > * gfn == INVALID_GFN indicates that the shared_info page was never
Re: [Xen-devel] [PATCH v5 06/17] xen: Use a typesafe to define INVALID_MFN
On Tue, 28 Jun 2016, Julien Grall wrote: > Also take the opportunity to convert arch/x86/debug.c to the typesafe > mfn. > > Signed-off-by: Julien Grall > > --- > Cc: Christoph Egger > Cc: Liu Jinsong > Cc: Jan Beulich > Cc: Andrew Cooper > Cc: Mukesh Rathor > Cc: Paul Durrant > Cc: Jun Nakajima > Cc: Kevin Tian > Cc: George Dunlap > Cc: Tim Deegan > > Changes in v5: > - Patch added > --- > xen/arch/arm/p2m.c | 4 ++-- For the ARM bits: Acked-by: Stefano Stabellini > xen/arch/x86/cpu/mcheck/mce.c | 2 +- > xen/arch/x86/debug.c| 50 --- > xen/arch/x86/hvm/hvm.c | 6 ++--- > xen/arch/x86/hvm/viridian.c | 6 ++--- > xen/arch/x86/hvm/vmx/vmx.c | 2 +- > xen/arch/x86/mm/guest_walk.c| 4 ++-- > xen/arch/x86/mm/hap/hap.c | 4 ++-- > xen/arch/x86/mm/p2m-ept.c | 6 ++--- > xen/arch/x86/mm/p2m-pod.c | 18 +++--- > xen/arch/x86/mm/p2m-pt.c| 18 +++--- > xen/arch/x86/mm/p2m.c | 52 > - > xen/arch/x86/mm/paging.c| 12 +- > xen/arch/x86/mm/shadow/common.c | 44 +- > xen/arch/x86/mm/shadow/multi.c | 36 ++-- > xen/common/domain.c | 6 ++--- > xen/common/grant_table.c| 6 ++--- > xen/include/xen/mm.h| 2 +- > 18 files changed, 140 insertions(+), 138 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 34563bb..d690602 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1461,7 +1461,7 @@ int relinquish_p2m_mapping(struct domain *d) > return apply_p2m_changes(d, RELINQUISH, >pfn_to_paddr(p2m->lowest_mapped_gfn), >pfn_to_paddr(p2m->max_mapped_gfn), > - pfn_to_paddr(INVALID_MFN), > + pfn_to_paddr(mfn_x(INVALID_MFN)), >MATTR_MEM, 0, p2m_invalid, >d->arch.p2m.default_access); > } > @@ -1476,7 +1476,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t > start_mfn, xen_pfn_t end_mfn) > return apply_p2m_changes(d, CACHEFLUSH, > pfn_to_paddr(start_mfn), > pfn_to_paddr(end_mfn), > - pfn_to_paddr(INVALID_MFN), > + pfn_to_paddr(mfn_x(INVALID_MFN)), > MATTR_MEM, 0, p2m_invalid, > d->arch.p2m.default_access); > } > diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c > index edcbe48..2695b0c 100644 > --- a/xen/arch/x86/cpu/mcheck/mce.c > +++ b/xen/arch/x86/cpu/mcheck/mce.c > @@ -1455,7 +1455,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) > gfn = PFN_DOWN(gaddr); > mfn = mfn_x(get_gfn(d, gfn, &t)); > > -if ( mfn == INVALID_MFN ) > +if ( mfn == mfn_x(INVALID_MFN) ) > { > put_gfn(d, gfn); > put_domain(d); > diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c > index 58cae22..3479f7c 100644 > --- a/xen/arch/x86/debug.c > +++ b/xen/arch/x86/debug.c > @@ -43,11 +43,11 @@ typedef unsigned long dbgva_t; > typedef unsigned char dbgbyte_t; > > /* Returns: mfn for the given (hvm guest) vaddr */ > -static unsigned long > +static mfn_t > dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, > unsigned long *gfn) > { > -unsigned long mfn; > +mfn_t mfn; > uint32_t pfec = PFEC_page_present; > p2m_type_t gfntype; > > @@ -60,16 +60,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int > toaddr, > return INVALID_MFN; > } > > -mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); > +mfn = get_gfn(dp, *gfn, &gfntype); > if ( p2m_is_readonly(gfntype) && toaddr ) > { > DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype); > mfn = INVALID_MFN; > } > else > -DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn); > +DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", > + vaddr, dp->domain_id, mfn_x(mfn)); > > -if ( mfn == INVALID_MFN ) > +if ( mfn_eq(mfn, INVALID_MFN) ) > { > put_gfn(dp, *gfn); > *gfn = INVALID_GFN; > @@ -91,7 +92,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, > * mode. > * Returns: mfn for the given (pv guest) vaddr > */ > -static unsigned long > +static mfn_t > dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val) > { > l4_pgentry_t l4e, *l4t; > @@ -99,31 +100,31 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t > pgd3val) > l2_pgentry_t l2e, *l2t; > l1_pgentry_t l1e, *l1t; > unsigned long cr3 = (pgd3val ?
Re: [Xen-devel] [PATCH v5 08/17] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn
On Tue, 28 Jun 2016, Julien Grall wrote: > The prototype and the declaration of p2m_lookup disagree on how the > function should be used. One expect a frame number whilst the other > an address. > > Thankfully, everyone is using with an address today. However, most of > the callers have to convert a guest frame to an address. Modify > the interface to take a guest physical frame in parameter and return > a machine frame. > > Whilst modifying the interface, use typesafe gfn and mfn for clarity > and catching possible misusage. > > Signed-off-by: Julien Grall Acked-by: Stefano Stabellini > --- > Changes in v4: > - Use INVALID_MFN_T when possible > --- > xen/arch/arm/p2m.c| 43 +++ > xen/arch/arm/traps.c | 21 +++-- > xen/include/asm-arm/p2m.h | 7 +++ > 3 files changed, 37 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index c938dde..54a363a 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d) > } > > /* > - * Lookup the MFN corresponding to a domain's PFN. > + * Lookup the MFN corresponding to a domain's GFN. > * > * There are no processor functions to do a stage 2 only lookup therefore we > * do a a software walk. > */ > -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) > +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) > { > struct p2m_domain *p2m = &d->arch.p2m; > +const paddr_t paddr = pfn_to_paddr(gfn_x(gfn)); > const unsigned int offsets[4] = { > zeroeth_table_offset(paddr), > first_table_offset(paddr), > @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t > paddr, p2m_type_t *t) > ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK > }; > lpae_t pte, *map; > -paddr_t maddr = INVALID_PADDR; > +mfn_t mfn = INVALID_MFN; > paddr_t mask = 0; > p2m_type_t _t; > unsigned int level, root_table; > @@ -216,21 +217,22 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t > paddr, p2m_type_t *t) > { > ASSERT(mask); > ASSERT(pte.p2m.type != p2m_invalid); > -maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask); > +mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) | > +(paddr & ~mask))); > *t = pte.p2m.type; > } > > err: > -return maddr; > +return mfn; > } > > -paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) > +mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) > { > -paddr_t ret; > +mfn_t ret; > struct p2m_domain *p2m = &d->arch.p2m; > > spin_lock(&p2m->lock); > -ret = __p2m_lookup(d, paddr, t); > +ret = __p2m_lookup(d, gfn, t); > spin_unlock(&p2m->lock); > > return ret; > @@ -493,8 +495,9 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t > gfn, > * No setting was found in the Radix tree. Check if the > * entry exists in the page-tables. > */ > -paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL); > -if ( INVALID_PADDR == maddr ) > +mfn_t mfn = __p2m_lookup(d, gfn, NULL); > + > +if ( mfn_eq(mfn, INVALID_MFN) ) > return -ESRCH; > > /* If entry exists then its rwx. */ > @@ -1483,8 +1486,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t > start_mfn, xen_pfn_t end_mfn) > > mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn) > { > -paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL); > -return _mfn(p >> PAGE_SHIFT); > +return p2m_lookup(d, gfn, NULL); > } > > /* > @@ -1498,8 +1500,8 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned > long flag) > { > long rc; > paddr_t ipa; > -unsigned long maddr; > -unsigned long mfn; > +gfn_t gfn; > +mfn_t mfn; > xenmem_access_t xma; > p2m_type_t t; > struct page_info *page = NULL; > @@ -1508,11 +1510,13 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, > unsigned long flag) > if ( rc < 0 ) > goto err; > > +gfn = _gfn(paddr_to_pfn(ipa)); > + > /* > * We do this first as this is faster in the default case when no > * permission is set on the page. > */ > -rc = __p2m_get_mem_access(current->domain, _gfn(paddr_to_pfn(ipa)), > &xma); > +rc = __p2m_get_mem_access(current->domain, gfn, &xma); > if ( rc < 0 ) > goto err; > > @@ -1561,12 +1565,11 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, > unsigned long flag) > * We had a mem_access permission limiting the access, but the page type > * could also be limiting, so we need to check that as well. > */ > -maddr = __p2m_lookup(current->domain, ipa, &t); > -if ( maddr == INVALID_PADDR ) > +mfn = __p2m_lookup(current->domai
Re: [Xen-devel] [PATCH v5 09/17] xen/arm: Rework the interface of p2m_cache_flush and use typesafe gfn
On Tue, 28 Jun 2016, Julien Grall wrote: > p2m_cache_flush is expecting GFNs in parameter and not MFNs. Rename > the variable to *gfn* and use typesafe to avoid possible misusage. > > Also, modify the prototype of the function to describe the range > using the start and the number of GFNs. This will avoid to wonder > whether the end if inclusive or exclusive. > > Note that the type of the parameters 'start' is changed from xen_pfn_t > (aka uint64_t) to gfn_t (aka unsigned long). This means that a truncation > will occur for ARM32. It is fine because it will always be encoded on 28 > bits maximum (40 bits address). > > Signed-off-by: Julien Grall Acked-by: Stefano Stabellini > --- > Changes in v4: > - This patch was originally called "xen/arm: p2m_cache_flush: > Use the correct terminology and typesafe gfn" > - Describe the range using the start and the number of GFNs. > > Changes in v3: > - Add a word in the commit message about the truncation. > > Changes in v2: > - Drop _gfn suffix > --- > xen/arch/arm/domctl.c | 2 +- > xen/arch/arm/p2m.c| 11 ++- > xen/include/asm-arm/p2m.h | 2 +- > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c > index 30453d8..f61f98a 100644 > --- a/xen/arch/arm/domctl.c > +++ b/xen/arch/arm/domctl.c > @@ -30,7 +30,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct > domain *d, > if ( e < s ) > return -EINVAL; > > -return p2m_cache_flush(d, s, e); > +return p2m_cache_flush(d, _gfn(s), domctl->u.cacheflush.nr_pfns); > } > case XEN_DOMCTL_bind_pt_irq: > { > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 54a363a..1cfb62b 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1469,16 +1469,17 @@ int relinquish_p2m_mapping(struct domain *d) >d->arch.p2m.default_access); > } > > -int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn) > +int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr) > { > struct p2m_domain *p2m = &d->arch.p2m; > +gfn_t end = gfn_add(start, nr); > > -start_mfn = MAX(start_mfn, p2m->lowest_mapped_gfn); > -end_mfn = MIN(end_mfn, p2m->max_mapped_gfn); > +start = gfn_max(start, _gfn(p2m->lowest_mapped_gfn)); > +end = gfn_min(end, _gfn(p2m->max_mapped_gfn)); > > return apply_p2m_changes(d, CACHEFLUSH, > - pfn_to_paddr(start_mfn), > - pfn_to_paddr(end_mfn), > + pfn_to_paddr(gfn_x(start)), > + pfn_to_paddr(gfn_x(end)), > pfn_to_paddr(mfn_x(INVALID_MFN)), > MATTR_MEM, 0, p2m_invalid, > d->arch.p2m.default_access); > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index f204482..8a96e68 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -139,7 +139,7 @@ void p2m_dump_info(struct domain *d); > mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t); > > /* Clean & invalidate caches corresponding to a region of guest address > space */ > -int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t > end_mfn); > +int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); > > /* Setup p2m RAM mapping for domain d from start-end. */ > int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); > -- > 1.9.1 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-coverity test] 96711: all pass - PUSHED
flight 96711 xen-unstable-coverity real [real] http://logs.test-lab.xenproject.org/osstest/logs/96711/ Perfect :-) All tests in this flight passed version targeted for testing: xen fea586d801f75317cb8cf593e8beba842391da62 baseline version: xen bb4f41b3dff831faaf5a3248e0ecd123024d7f8f Last test of basis96571 2016-07-03 09:18:39 Z3 days Testing same since96711 2016-07-06 09:19:27 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Corneliu ZUZU Jan Beulich Kevin Tian Razvan Cojocaru Tamas K Lengyel Wei Liu jobs: coverity-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : + branch=xen-unstable-coverity + revision=fea586d801f75317cb8cf593e8beba842391da62 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-coverity fea586d801f75317cb8cf593e8beba842391da62 + branch=xen-unstable-coverity + revision=fea586d801f75317cb8cf593e8beba842391da62 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=xen + xenbranch=xen-unstable-coverity + qemuubranch=qemu-upstream-unstable-coverity + qemuubranch=qemu-upstream-unstable + '[' xxen = xlinux ']' + linuxbranch= + '[' xqemu-upstream-unstable = x ']' + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable-coverity + prevxenbranch=xen-4.7-testing + '[' xfea586d801f75317cb8cf593e8beba842391da62 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/rumpuser-xen.git ++ : git ++ : git://xenbits.xen.org/rumpuser-xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git +++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src +++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src +++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src '[fetch=try]' +++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src +++ local 'options=[fetch=try]' getconfig GitCacheProxy perl -e ' use Osstest; readglobalconfig(); print $c{"GitCacheProxy"} or die $!; ' +++ local cache=git://cache:9419/ +++ '[' xgit://cache:9419/ '!=' x ']' +++ echo 'git://cache:9419/https://github.com/rumpkernel/rumpkernel-netbsd-src%20[fetch=try]' ++ : 'git://cache:9419/https://github.com/rumpkernel/rumpkernel-netbsd-src%20[fetch=try]' ++ : git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/o
Re: [Xen-devel] [PATCH v5 10/17] xen/arm: map_regions_rw_cache: Map the region with p2m->default_access
On Tue, 28 Jun 2016, Julien Grall wrote: > The parameter 'access' is used by memaccess to restrict temporarily the > permission. This parameter should not be used for other purpose (such > as restricting permanently the permission). > > The type p2m_mmio_direct will map the region Read-Write and > non-executable. Note that this is already the current behavior with the > combination of the type and the access. So there is no functional > change. > > Signed-off-by: Julien Grall I could be mistaken, but isn't default_access actually p2m_access_rwx? > --- > Cc: Shannon Zhao > > This patch is a candidate for Xen 4.7. Currently this function is > only used to map ACPI regions. > > I am wondering if we should introduce a new p2m type for it. And map > this region RO (I am not sure why a guest would want to > modify this region). > > Changes in v4: > - Patch added > --- > xen/arch/arm/p2m.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 1cfb62b..fcc4513 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1231,7 +1231,7 @@ int map_regions_rw_cache(struct domain *d, > pfn_to_paddr(start_gfn + nr), > pfn_to_paddr(mfn), > MATTR_MEM, 0, p2m_mmio_direct, > - p2m_access_rw); > + d->arch.p2m.default_access); > } > > int unmap_regions_rw_cache(struct domain *d, > @@ -1244,7 +1244,7 @@ int unmap_regions_rw_cache(struct domain *d, > pfn_to_paddr(start_gfn + nr), > pfn_to_paddr(mfn), > MATTR_MEM, 0, p2m_invalid, > - p2m_access_rw); > + d->arch.p2m.default_access); > } > > int map_mmio_regions(struct domain *d, > -- > 1.9.1 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 11/17] xen/arm: dom0_build: Remove dead code in allocate_memory
On Tue, 28 Jun 2016, Julien Grall wrote: > The code to allocate memory when dom0 does not use direct mapping is > relying on the presence of memory node in the DT. > > However, they are not present when booting using UEFI or when using > ACPI. > > Rather than fixing the code, remove it because dom0 is always direct > memory mapped and therefore the code is never tested. Also add a > check to avoid disabling direct memory mapped and not implementing > the associated RAM bank allocation. > > Signed-off-by: Julien Grall Acked-by: Stefano Stabellini > --- > Changes in v4: > - Patch added > --- > xen/arch/arm/domain_build.c | 58 > ++--- > 1 file changed, 7 insertions(+), 51 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 49185f0..923f48a 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -235,7 +235,7 @@ fail: > * (as described above) we allow higher allocations and continue until > * that runs out (or we have allocated sufficient dom0 memory). > */ > -static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) > +static void allocate_memory(struct domain *d, struct kernel_info *kinfo) > { > const unsigned int min_low_order = > get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); > @@ -247,6 +247,12 @@ static void allocate_memory_11(struct domain *d, struct > kernel_info *kinfo) > bool_t lowmem = is_32bit_domain(d); > unsigned int bits; > > +/* > + * TODO: Implement memory bank allocation when DOM0 is not direct > + * mapped > + */ > +BUG_ON(!dom0_11_mapping); > + > printk("Allocating 1:1 mappings totalling %ldMB for dom0:\n", > /* Don't want format this as PRIpaddr (16 digit hex) */ > (unsigned long)(kinfo->unassigned_mem >> 20)); > @@ -343,56 +349,6 @@ static void allocate_memory_11(struct domain *d, struct > kernel_info *kinfo) > } > } > > -static void allocate_memory(struct domain *d, struct kernel_info *kinfo) > -{ > - > -struct dt_device_node *memory = NULL; > -const void *reg; > -u32 reg_len, reg_size; > -unsigned int bank = 0; > - > -if ( dom0_11_mapping ) > -return allocate_memory_11(d, kinfo); > - > -while ( (memory = dt_find_node_by_type(memory, "memory")) ) > -{ > -int l; > - > -dt_dprintk("memory node\n"); > - > -reg_size = dt_cells_to_size(dt_n_addr_cells(memory) + > dt_n_size_cells(memory)); > - > -reg = dt_get_property(memory, "reg", ®_len); > -if ( reg == NULL ) > -panic("Memory node has no reg property"); > - > -for ( l = 0; > - kinfo->unassigned_mem > 0 && l + reg_size <= reg_len > - && kinfo->mem.nr_banks < NR_MEM_BANKS; > - l += reg_size ) > -{ > -paddr_t start, size; > - > -if ( dt_device_get_address(memory, bank, &start, &size) ) > -panic("Unable to retrieve the bank %u for %s", > - bank, dt_node_full_name(memory)); > - > -if ( size > kinfo->unassigned_mem ) > -size = kinfo->unassigned_mem; > - > -printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", > - start, start + size); > -if ( p2m_populate_ram(d, start, start + size) < 0 ) > -panic("Failed to populate P2M"); > -kinfo->mem.bank[kinfo->mem.nr_banks].start = start; > -kinfo->mem.bank[kinfo->mem.nr_banks].size = size; > -kinfo->mem.nr_banks++; > - > -kinfo->unassigned_mem -= size; > -} > -} > -} > - > static int write_properties(struct domain *d, struct kernel_info *kinfo, > const struct dt_device_node *node) > { > -- > 1.9.1 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 12/17] xen/arm: p2m: Remove unused operation ALLOCATE
On Tue, 28 Jun 2016, Julien Grall wrote: > The operation ALLOCATE is unused. If we ever need it, it could be > reimplemented with INSERT. > > Signed-off-by: Julien Grall Acked-by: Stefano Stabellini > --- > Changes in v4: > - Patch added > --- > xen/arch/arm/p2m.c| 67 > ++- > xen/include/asm-arm/p2m.h | 3 --- > 2 files changed, 2 insertions(+), 68 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index fcc4513..f11094e 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -547,7 +547,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain > *p2m, unsigned long pfn, > > enum p2m_operation { > INSERT, > -ALLOCATE, > REMOVE, > RELINQUISH, > CACHEFLUSH, > @@ -667,7 +666,6 @@ static int apply_one_level(struct domain *d, > { > const paddr_t level_size = level_sizes[level]; > const paddr_t level_mask = level_masks[level]; > -const paddr_t level_shift = level_shifts[level]; > > struct p2m_domain *p2m = &d->arch.p2m; > lpae_t pte; > @@ -678,58 +676,6 @@ static int apply_one_level(struct domain *d, > > switch ( op ) > { > -case ALLOCATE: > -ASSERT(level < 3 || !p2m_valid(orig_pte)); > -ASSERT(*maddr == 0); > - > -if ( p2m_valid(orig_pte) ) > -return P2M_ONE_DESCEND; > - > -if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) && > - /* We only create superpages when mem_access is not in use. */ > - (level == 3 || (level < 3 && !p2m->mem_access_enabled)) ) > -{ > -struct page_info *page; > - > -page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0); > -if ( page ) > -{ > -rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a); > -if ( rc < 0 ) > -{ > -free_domheap_page(page); > -return rc; > -} > - > -pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a); > -if ( level < 3 ) > -pte.p2m.table = 0; > -p2m_write_pte(entry, pte, flush_cache); > -p2m->stats.mappings[level]++; > - > -*addr += level_size; > - > -return P2M_ONE_PROGRESS; > -} > -else if ( level == 3 ) > -return -ENOMEM; > -} > - > -/* L3 is always suitably aligned for mapping (handled, above) */ > -BUG_ON(level == 3); > - > -/* > - * If we get here then we failed to allocate a sufficiently > - * large contiguous region for this level (which can't be > - * L3) or mem_access is in use. Create a page table and > - * continue to descend so we try smaller allocations. > - */ > -rc = p2m_create_table(d, entry, 0, flush_cache); > -if ( rc < 0 ) > -return rc; > - > -return P2M_ONE_DESCEND; > - > case INSERT: > if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) && > /* > @@ -1169,7 +1115,7 @@ static int apply_p2m_changes(struct domain *d, > } > } > > -if ( op == ALLOCATE || op == INSERT ) > +if ( op == INSERT ) > { > p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn); > p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn); > @@ -1197,7 +1143,7 @@ out: > > spin_unlock(&p2m->lock); > > -if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) && > +if ( rc < 0 && ( op == INSERT ) && > addr != start_gpaddr ) > { > BUG_ON(addr == end_gpaddr); > @@ -1212,15 +1158,6 @@ out: > return rc; > } > > -int p2m_populate_ram(struct domain *d, > - paddr_t start, > - paddr_t end) > -{ > -return apply_p2m_changes(d, ALLOCATE, start, end, > - 0, MATTR_MEM, 0, p2m_ram_rw, > - d->arch.p2m.default_access); > -} > - > int map_regions_rw_cache(struct domain *d, > unsigned long start_gfn, > unsigned long nr, > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 8a96e68..4752161 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -141,9 +141,6 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t > *t); > /* Clean & invalidate caches corresponding to a region of guest address > space */ > int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); > > -/* Setup p2m RAM mapping for domain d from start-end. */ > -int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); > - > int map_regions_rw_cache(struct domain *d, > unsigned long start_gfn, > unsigned long nr_mfns, > -- > 1.9.1 > __
Re: [Xen-devel] [PATCH v5 14/17] xen/arm: Use the typesafes mfn and gfn in map_regions_rw_cache ...
On Tue, 28 Jun 2016, Julien Grall wrote: > to avoid mixing machine frame with guest frame. Also rename the > parameters of the function and drop pointless PAGE_MASK in the caller. > > Signed-off-by: Julien Grall Acked-by: Stefano Stabellini > --- > Changes in v4: > - Patch added > --- > xen/arch/arm/domain_build.c | 8 > xen/arch/arm/p2m.c | 20 ++-- > xen/include/asm-arm/p2m.h | 12 ++-- > 3 files changed, 20 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 923f48a..60db9e4 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1522,9 +1522,9 @@ static void acpi_map_other_tables(struct domain *d) > addr = acpi_gbl_root_table_list.tables[i].address; > size = acpi_gbl_root_table_list.tables[i].length; > res = map_regions_rw_cache(d, > - paddr_to_pfn(addr & PAGE_MASK), > + _gfn(paddr_to_pfn(addr)), > DIV_ROUND_UP(size, PAGE_SIZE), > - paddr_to_pfn(addr & PAGE_MASK)); > + _mfn(paddr_to_pfn(addr))); > if ( res ) > { > panic(XENLOG_ERR "Unable to map ACPI region 0x%"PRIx64 > @@ -1878,9 +1878,9 @@ static int prepare_acpi(struct domain *d, struct > kernel_info *kinfo) > > /* Map the EFI and ACPI tables to Dom0 */ > rc = map_regions_rw_cache(d, > - paddr_to_pfn(d->arch.efi_acpi_gpa), > + _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)), >PFN_UP(d->arch.efi_acpi_len), > - > paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))); > + > _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table; > if ( rc != 0 ) > { > printk(XENLOG_ERR "Unable to map EFI/ACPI table 0x%"PRIx64 > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 5ffc3df..0fdd11f 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1159,27 +1159,27 @@ out: > } > > int map_regions_rw_cache(struct domain *d, > - unsigned long start_gfn, > + gfn_t gfn, > unsigned long nr, > - unsigned long mfn) > + mfn_t mfn) > { > return apply_p2m_changes(d, INSERT, > - pfn_to_paddr(start_gfn), > - pfn_to_paddr(start_gfn + nr), > - pfn_to_paddr(mfn), > + pfn_to_paddr(gfn_x(gfn)), > + pfn_to_paddr(gfn_x(gfn) + nr), > + pfn_to_paddr(mfn_x(mfn)), > MATTR_MEM, 0, p2m_mmio_direct, > d->arch.p2m.default_access); > } > > int unmap_regions_rw_cache(struct domain *d, > - unsigned long start_gfn, > + gfn_t gfn, > unsigned long nr, > - unsigned long mfn) > + mfn_t mfn) > { > return apply_p2m_changes(d, REMOVE, > - pfn_to_paddr(start_gfn), > - pfn_to_paddr(start_gfn + nr), > - pfn_to_paddr(mfn), > + pfn_to_paddr(gfn_x(gfn)), > + pfn_to_paddr(gfn_x(gfn) + nr), > + pfn_to_paddr(mfn_x(mfn)), > MATTR_MEM, 0, p2m_invalid, > d->arch.p2m.default_access); > } > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 8d29eda..6e258b9 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -142,14 +142,14 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, > p2m_type_t *t); > int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); > > int map_regions_rw_cache(struct domain *d, > - unsigned long start_gfn, > - unsigned long nr_mfns, > - unsigned long mfn); > + gfn_t gfn, > + unsigned long nr, > + mfn_t mfn); > > int unmap_regions_rw_cache(struct domain *d, > - unsigned long start_gfn, > - unsigned long nr_mfns, > - unsigned long mfn); > + gfn_t gfn, > + unsigned long nr, > + mfn_t mfn); > > int map_dev_mmio_region(struct domain *d, > gfn_t gfn, > -- > 1.9.1 > ___ Xen-devel
Re: [Xen-devel] [PATCH v5 15/17] xen/arm: p2m: Introduce helpers to insert and remove mapping
On Tue, 28 Jun 2016, Julien Grall wrote: > More the half of the arguments of INSERT and REMOVE are the same for > each callers. Simplify the callers of apply_p2m_changes by adding new > helpers which will fill common arguments with default values. > > Signed-off-by: Julien Grall I don't see much value in this patch. It looks good because it is before "Rework the interface of apply_p2m_changes and use typesafe" in this series (therefore eliminates a bunch of temporary casts), but otherwise I don't think it would be much of an improvement. > --- > Changes in v5: > - Add missing Signed-off-by > > Changes in v4: > - Patch added > --- > xen/arch/arm/p2m.c | 70 > -- > 1 file changed, 36 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 0fdd11f..a5b584b 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1158,17 +1158,40 @@ out: > return rc; > } > > +static inline int p2m_insert_mapping(struct domain *d, > + gfn_t start_gfn, > + unsigned long nr, > + mfn_t mfn, > + int mattr, p2m_type_t t) > +{ > +return apply_p2m_changes(d, INSERT, > + pfn_to_paddr(gfn_x(start_gfn)), > + pfn_to_paddr(gfn_x(start_gfn) + nr), > + pfn_to_paddr(mfn_x(mfn)), > + mattr, 0, t, d->arch.p2m.default_access); > +} > + > +static inline int p2m_remove_mapping(struct domain *d, > + gfn_t start_gfn, > + unsigned long nr, > + mfn_t mfn) > +{ > +return apply_p2m_changes(d, REMOVE, > + pfn_to_paddr(gfn_x(start_gfn)), > + pfn_to_paddr(gfn_x(start_gfn) + nr), > + pfn_to_paddr(mfn_x(mfn)), > + /* arguments below not used when removing > mapping */ > + MATTR_MEM, 0, p2m_invalid, > + d->arch.p2m.default_access); > +} > + > int map_regions_rw_cache(struct domain *d, > gfn_t gfn, > unsigned long nr, > mfn_t mfn) > { > -return apply_p2m_changes(d, INSERT, > - pfn_to_paddr(gfn_x(gfn)), > - pfn_to_paddr(gfn_x(gfn) + nr), > - pfn_to_paddr(mfn_x(mfn)), > - MATTR_MEM, 0, p2m_mmio_direct, > - d->arch.p2m.default_access); > +return p2m_insert_mapping(d, gfn, nr, mfn, > + MATTR_MEM, p2m_mmio_direct); > } > > int unmap_regions_rw_cache(struct domain *d, > @@ -1176,12 +1199,7 @@ int unmap_regions_rw_cache(struct domain *d, > unsigned long nr, > mfn_t mfn) > { > -return apply_p2m_changes(d, REMOVE, > - pfn_to_paddr(gfn_x(gfn)), > - pfn_to_paddr(gfn_x(gfn) + nr), > - pfn_to_paddr(mfn_x(mfn)), > - MATTR_MEM, 0, p2m_invalid, > - d->arch.p2m.default_access); > +return p2m_remove_mapping(d, gfn, nr, mfn); > } > > int map_mmio_regions(struct domain *d, > @@ -1189,12 +1207,8 @@ int map_mmio_regions(struct domain *d, > unsigned long nr, > mfn_t mfn) > { > -return apply_p2m_changes(d, INSERT, > - pfn_to_paddr(gfn_x(start_gfn)), > - pfn_to_paddr(gfn_x(start_gfn) + nr), > - pfn_to_paddr(mfn_x(mfn)), > - MATTR_DEV, 0, p2m_mmio_direct, > - d->arch.p2m.default_access); > +return p2m_insert_mapping(d, start_gfn, nr, mfn, > + MATTR_MEM, p2m_mmio_direct); > } > > int unmap_mmio_regions(struct domain *d, > @@ -1202,12 +1216,7 @@ int unmap_mmio_regions(struct domain *d, > unsigned long nr, > mfn_t mfn) > { > -return apply_p2m_changes(d, REMOVE, > - pfn_to_paddr(gfn_x(start_gfn)), > - pfn_to_paddr(gfn_x(start_gfn) + nr), > - pfn_to_paddr(mfn_x(mfn)), > - MATTR_DEV, 0, p2m_invalid, > - d->arch.p2m.default_access); > +return p2m_remove_mapping(d, start_gfn, nr, mfn); > } > > int map_dev_mmio_region(struct domain *d, > @@ -1237,22 +1246,15 @@ int guest_physmap_add_entry(struct domain *d, > unsigne
Re: [Xen-devel] [PATCH 0/3] libxl: add framework for device types
Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"): > Instead of duplicate coding for each device type (vtpms, usbctrls, ...) > especially on domain creation introduce a framework for that purpose. I think something like this is a jolly good idea. Thanks a lot! The rough shape - a set of structs with what amount to method calls - seems a good direction to be going in. I have some comments/questions. I saw this in 2/3: > +for (i = 0; i < d_config->num_pcidevs; i++) { > +rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1); > +if (rc < 0) { > +LOG(ERROR, "libxl_device_pci_add failed: %d", rc); > +goto out; > +} > +} > + And there is similar code in 3/3 for dtdevs. Could that be lifted away somehow ? (You'd have to take some care about the types, sadly; ie, I think libxl__device_pci_add might have to start to take a void*; maybe some macros could make things typesafe?) In 1/3: > +struct libxl_device_type libxl__usbctrl_devtype = { > +.type = "usbctrl", > +.num_offset = offsetof(libxl_domain_config, num_usbctrls), > +.add= libxl__add_usbctrls, > +}; And then num_offset is used like this: > +if (*(int *)((void *)d_config + dt->num_offset) > 0) { This is a fine approach but I would prefer it if the there were a bit more type safety, particularly in the parts that have to occur once for each device type. For example, there is nothing stopping one using this pattern with a num_* field which is not an int. Perhaps there should be a macro for generating the libxl_device_type contents ? It could perhaps take `usbctrls' and make `num_usbctrls' out of it using token pasting. Also these structs should be static const, I think. Thanks again! Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 17/17] xen/arm: p2m: Rework the interface of apply_p2m_changes and use typesafe
On Tue, 28 Jun 2016, Julien Grall wrote: > Most of the callers of apply_p2m_changes have a GFN, a MFN and the > number of frame to change in hand. > > Rather than asking each caller to convert the frame to an address, > rework the interfaces to pass the GFN, MFN and the number of frame. > > Note that it would be possible to do more clean-up in apply_p2m_changes, > but this will be done in a follow-up series. > > Signed-off-by: Julien Grall > > --- > Changes in v4: > - Patch added > --- > xen/arch/arm/p2m.c | 62 > -- > 1 file changed, 28 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 9fdc417..bb33a72 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -906,25 +906,26 @@ static void update_reference_mapping(struct page_info > *page, > > static int apply_p2m_changes(struct domain *d, > enum p2m_operation op, > - paddr_t start_gpaddr, > - paddr_t end_gpaddr, > - paddr_t maddr, > + gfn_t sgfn, > + unsigned long nr, > + mfn_t smfn, > int mattr, > uint32_t mask, > p2m_type_t t, > p2m_access_t a) > { > +paddr_t start_gpaddr = pfn_to_paddr(gfn_x(sgfn)); > +paddr_t end_gpaddr = pfn_to_paddr(gfn_x(sgfn) + nr); > +paddr_t maddr = pfn_to_paddr(mfn_x(smfn)); > int rc, ret; > struct p2m_domain *p2m = &d->arch.p2m; > lpae_t *mappings[4] = { NULL, NULL, NULL, NULL }; > struct page_info *pages[4] = { NULL, NULL, NULL, NULL }; > -paddr_t addr, orig_maddr = maddr; > +paddr_t addr; > unsigned int level = 0; > unsigned int cur_root_table = ~0; > unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 }; > unsigned int count = 0; > -const unsigned long sgfn = paddr_to_pfn(start_gpaddr), > -egfn = paddr_to_pfn(end_gpaddr); > const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000; > const bool_t preempt = !is_idle_vcpu(current); > bool_t flush = false; > @@ -986,9 +987,9 @@ static int apply_p2m_changes(struct domain *d, > * Preempt setting mem_access permissions as required by > XSA-89, > * if it's not the last iteration. > */ > -uint32_t progress = paddr_to_pfn(addr) - sgfn + 1; > +uint32_t progress = paddr_to_pfn(addr) - gfn_x(sgfn) + 1; > > -if ( (egfn - sgfn) > progress && !(progress & mask) ) > +if ( nr > progress && !(progress & mask) ) > { > rc = progress; > goto out; > @@ -1117,8 +1118,9 @@ static int apply_p2m_changes(struct domain *d, > > if ( op == INSERT ) > { > -p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, _gfn(egfn)); > -p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, _gfn(sgfn)); > +p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, > + gfn_add(sgfn, nr)); > +p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn); > } > > rc = 0; > @@ -1127,7 +1129,7 @@ out: > if ( flush ) > { > flush_tlb_domain(d); > -ret = iommu_iotlb_flush(d, sgfn, egfn - sgfn); > +ret = iommu_iotlb_flush(d, gfn_x(sgfn), nr); > if ( !rc ) > rc = ret; > } > @@ -1146,12 +1148,14 @@ out: > if ( rc < 0 && ( op == INSERT ) && > addr != start_gpaddr ) > { > +unsigned long gfn = paddr_to_pfn(addr); > + > BUG_ON(addr == end_gpaddr); > /* > * addr keeps the address of the end of the last > successfully-inserted > * mapping. > */ > -apply_p2m_changes(d, REMOVE, start_gpaddr, addr, orig_maddr, > +apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn, Worth considering a gfn_sub (we already have gfn_add)? In any case Acked-by: Stefano Stabellini >mattr, 0, p2m_invalid, d->arch.p2m.default_access); > } > > @@ -1164,10 +1168,7 @@ static inline int p2m_insert_mapping(struct domain *d, > mfn_t mfn, > int mattr, p2m_type_t t) > { > -return apply_p2m_changes(d, INSERT, > - pfn_to_paddr(gfn_x(start_gfn)), > - pfn_to_paddr(gfn_x(start_gfn) + nr), > - pfn_to_paddr(mfn_x(mfn)), > +return apply_p2m_changes(d, INSERT, start_gfn, nr, mfn, > mattr, 0, t, d->arch.p2m.default_access); > } > > @@ -1176,10 +1177,7 @@ static inline int p2m_remove_mapping(struct domain *d, > unsigned
Re: [Xen-devel] [PATCH v1 20/20] libxl/acpi: Build ACPI tables for HVMlite guests
(CC Stefano) Hi Boris, On 05/07/16 20:05, Boris Ostrovsky wrote: diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h index 34a853c..7c6536b 100644 --- a/tools/libxl/libxl_arch.h +++ b/tools/libxl/libxl_arch.h @@ -62,4 +62,7 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc, uint32_t domid, struct xc_dom_image *dom); +int libxl__dom_load_acpi(libxl__gc *gc, +libxl_domain_build_info *info, +struct xc_dom_image *dom); This file contains arch specific function with are called by the generic libxl code. I don't think this is the right file for an x86 specific function which has a generic name. IHMO, this should go in libxl_x86_acpi.h with "x86" in the name and with '_hidden' attribute. #endif [...] +static int populate_acpi_pages(struct xc_dom_image *dom, + xen_pfn_t *extents, + unsigned int num_pages, + struct acpi_ctxt *ctxt) +{ +int rc; +xc_interface *xch = dom->xch; +uint32_t domid = dom->guest_domid; +unsigned long idx, first_high_idx = (1ull << (32 - ctxt->page_shift)); + +for (; num_pages; num_pages--, extents++) { + +if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) == 1) It looks like this is working because libxl is setting the maximum size of the domain with some slack (1MB). You might get in trouble if the slack is reduced or used by someone or the ACPI blob is increasing. +continue; + +if (dom->highmem_end) { +idx = --dom->highmem_end; +if ( idx == first_high_idx ) +dom->highmem_end = 0; +} else +idx = --dom->lowmem_end; + +rc = xc_domain_add_to_physmap(xch, domid, + XENMAPSPACE_gmfn, + idx, *extents); +if (rc) +return rc; +} + +return 0; +} + +int libxl__dom_load_acpi(libxl__gc *gc, +libxl_domain_build_info *info, +struct xc_dom_image *dom) +{ +struct acpi_config config = {0}; +struct acpi_ctxt ctxt; +uint32_t domid = dom->guest_domid; +xc_interface *xch = dom->xch; +int rc, i, acpi_pages_num; +xen_pfn_t extent, *extents; +void *acpi_pages, *guest_acpi_pages = NULL; +unsigned long page_mask; + +if ((info->type != LIBXL_DOMAIN_TYPE_HVM) || +(info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE)) +return 0; + +ctxt.page_size = XC_DOM_PAGE_SIZE(dom); +ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom); +page_mask = (1UL << ctxt.page_shift) - 1; + +ctxt.mem_ops.alloc = mem_alloc; +ctxt.mem_ops.v2p = virt_to_phys; + +rc = init_acpi_config(gc, dom, info, &config); +if (rc) { +LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc); +return rc; +} + +/* Map page that will hold RSDP */ +extent = RSDP_ADDRESS >> ctxt.page_shift; +rc = populate_acpi_pages(dom, &extent, 1, &ctxt); +if (rc) { +LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", +__FUNCTION__, rc); +goto out; +} +config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, + ctxt.page_size, + PROT_READ | PROT_WRITE, + RSDP_ADDRESS >> ctxt.page_shift); +if (!config.rsdp) { +LOG(ERROR, "%s: Can't map acpi_physical", __FUNCTION__); +rc = -1; +goto out; +} + +/* Map acpi_info page */ +extent = ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift; +rc = populate_acpi_pages(dom, &extent, 1, &ctxt); +if (rc) { +LOG(ERROR, "%s: populate_acpi_pages for acpi_info failed with %d", +__FUNCTION__, rc); +goto out; +} + +config.ainfop = (unsigned long)xc_map_foreign_range(xch, domid, +ctxt.page_size, +PROT_READ | PROT_WRITE, +ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift); Loading the ACPI blob on ARM will be very similar except for the base address. So it would be nice to share some code with it. However, as mentioned in the ACPI thread [1], all the blobs are generally loaded by libxc and not libxl. This is more true on ARM because the guest address space is controlled by libxc (the position of all the blob are decided by it). Regards, [1] https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00040.html -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.x
Re: [Xen-devel] [PATCH v5 16/17] xen/arm: p2m: Use typesafe gfn for {max, lowest}_mapped_gfn
On Tue, 28 Jun 2016, Julien Grall wrote: > Signed-off-by: Julien Grall Acked-by: Stefano Stabellini > Changes in v4: > - Patch added > --- > xen/arch/arm/mm.c | 2 +- > xen/arch/arm/p2m.c| 18 +- > xen/include/asm-arm/p2m.h | 4 ++-- > 3 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index b5fc034..4e256c2 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -1004,7 +1004,7 @@ int page_is_ram_type(unsigned long mfn, unsigned long > mem_type) > > unsigned long domain_get_maximum_gpfn(struct domain *d) > { > -return d->arch.p2m.max_mapped_gfn; > +return gfn_x(d->arch.p2m.max_mapped_gfn); > } > > void share_xen_page_with_guest(struct page_info *page, > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index a5b584b..9fdc417 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -976,7 +976,7 @@ static int apply_p2m_changes(struct domain *d, > * This is set in preempt_count_limit. > * > */ > -p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT; > +p2m->lowest_mapped_gfn = _gfn(addr >> PAGE_SHIFT); > rc = -ERESTART; > goto out; > > @@ -1117,8 +1117,8 @@ static int apply_p2m_changes(struct domain *d, > > if ( op == INSERT ) > { > -p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn); > -p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn); > +p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, _gfn(egfn)); > +p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, _gfn(sgfn)); > } > > rc = 0; > @@ -1383,8 +1383,8 @@ int p2m_init(struct domain *d) > > p2m->root = NULL; > > -p2m->max_mapped_gfn = 0; > -p2m->lowest_mapped_gfn = ULONG_MAX; > +p2m->max_mapped_gfn = _gfn(0); > +p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); > > p2m->default_access = p2m_access_rwx; > p2m->mem_access_enabled = false; > @@ -1401,8 +1401,8 @@ int relinquish_p2m_mapping(struct domain *d) > struct p2m_domain *p2m = &d->arch.p2m; > > return apply_p2m_changes(d, RELINQUISH, > - pfn_to_paddr(p2m->lowest_mapped_gfn), > - pfn_to_paddr(p2m->max_mapped_gfn), > + pfn_to_paddr(gfn_x(p2m->lowest_mapped_gfn)), > + pfn_to_paddr(gfn_x(p2m->max_mapped_gfn)), >pfn_to_paddr(mfn_x(INVALID_MFN)), >MATTR_MEM, 0, p2m_invalid, >d->arch.p2m.default_access); > @@ -1413,8 +1413,8 @@ int p2m_cache_flush(struct domain *d, gfn_t start, > unsigned long nr) > struct p2m_domain *p2m = &d->arch.p2m; > gfn_t end = gfn_add(start, nr); > > -start = gfn_max(start, _gfn(p2m->lowest_mapped_gfn)); > -end = gfn_min(end, _gfn(p2m->max_mapped_gfn)); > +start = gfn_max(start, p2m->lowest_mapped_gfn); > +end = gfn_min(end, p2m->max_mapped_gfn); > > return apply_p2m_changes(d, CACHEFLUSH, > pfn_to_paddr(gfn_x(start)), > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 6e258b9..34096bc 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -34,13 +34,13 @@ struct p2m_domain { > /* Highest guest frame that's ever been mapped in the p2m > * Only takes into account ram and foreign mapping > */ > -unsigned long max_mapped_gfn; > +gfn_t max_mapped_gfn; > > /* Lowest mapped gfn in the p2m. When releasing mapped gfn's in a > * preemptible manner this is update to track recall where to > * resume the search. Apart from during teardown this can only > * decrease. */ > -unsigned long lowest_mapped_gfn; > +gfn_t lowest_mapped_gfn; > > /* Gather some statistics for information purposes only */ > struct { > -- > 1.9.1 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 10/17] xen/arm: map_regions_rw_cache: Map the region with p2m->default_access
(CC Tamas) On 06/07/16 11:43, Stefano Stabellini wrote: On Tue, 28 Jun 2016, Julien Grall wrote: The parameter 'access' is used by memaccess to restrict temporarily the permission. This parameter should not be used for other purpose (such as restricting permanently the permission). The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change. Signed-off-by: Julien Grall I could be mistaken, but isn't default_access actually p2m_access_rwx? By default, the access is p2m_access_rwx. However this can be changed by memaccess and the new default value is stored in arch->p2m.default_access. I have CCed Tamas to confirm that. Note that this is how the other calls are done. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] libxc: wrapper for log level sysctl
Wei Liu writes ("[PATCH v2 3/5] libxc: wrapper for log level sysctl"): > Signed-off-by: Wei Liu I have no objection to this new wrapper but the actual code is very tedious. Is this really the least repetitive way of writing this ? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/5] xen/console: allow log level threshold adjustments
Wei Liu writes ("[PATCH v2 2/5] xen/console: allow log level threshold adjustments"): > ... from serial console and via sysctl so that one doesn't always need > to reboot to see more / less messages. > > Note that upper thresholds driven from the serial console are sticky, > i.e. while they get adjusted upwards when the lower threshold would > otherwise end up above the upper one, they don't get adjusted when > reducing the lower one. Full flexibility is available only via the > sysctl interface. > > Signed-off-by: Jan Beulich > > Rework the sysctl interface to pass in / out strings directly. Provide > some helper functions to transform from log level numbers to strings and > vice verse. Lower and upper bounds are checked. Add XSM hook. Is this really the best way to do this ? This is an awful lot of hypervisor code. Perhaps instead there should be a way to request the table of level strings, and userspace could do the conversion ? Or, given that this is a sysctl, so does not need a stable ABI, the table of loglevel strings could be provided in the .h file in the form of a suitable lifted list #define, and at runtime be implicit in the ABI version. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 4/5] libxl: introduce APIs to get and set log level
Wei Liu writes ("[PATCH v2 4/5] libxl: introduce APIs to get and set log level"): > Signed-off-by: Wei Liu ... > +int libxl_set_log_level(libxl_ctx *ctx, bool guest, > +char *lower_thresh, char *upper_thresh) I think the log level names should be exposed as a list to the caller. It might be better to allow the caller to specify an integer. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 12/16 - RFC] x86/efi: create new early memory allocator
On Wed, Jul 06, 2016 at 01:22:24AM -0600, Jan Beulich wrote: > >>> On 05.07.16 at 20:26, wrote: > > On Fri, Apr 15, 2016 at 02:33:12PM +0200, Daniel Kiper wrote: > >> 1) We could use native EFI allocation functions (e.g. AllocatePool() > >>or AllocatePages()) to get memory chunk. However, later (somewhere > >>in __start_xen()) we must copy its contents to safe place or reserve > >>this in e820 memory map and map it in Xen virtual address space. > > > > I have checked Linux kernel code. It allocates buffer for memory map using > > EFI API and later reserve it in e820 memory map. Simple. This should work > > for us too but... > > > >>In later case we must also care about conflicts with e.g. crash > >>kernel regions which could be quite difficult. > > > > This is not a problem since Xen can choose dynamically placement of kdump > > region during boot phase and there is no requirement to specify it in boot > > command line. This means that it will avoid all allocated/reserved regions > > including EFI memory map. However, there is one potential problem which > > cannot be avoided simply with current EFI spec. I think about conflicts > > with trampoline. It must live below 1 MiB. However, there is not something > > similar to "AllocateMaxAddress" option for AllocatePages() which would > > ask EFI to allocate memory above a given address (Hmmm... Why UEFI designers > > did not added such option, e.g. "AllocateMinAddress"? For me it is obvious > > thing if we have "AllocateMaxAddress"). > > Not obvious to me at all. Allowing an upper bound is natural (for > both DMA purposes and arbitrary other addressing restrictions). > Allowing a lower bound to be specified isn't. I think that I have shown above that on some platforms this could be useful option. > > So, it means that we cannot simply > > say "give me a memory chunk above 1 MiB". AIUI, Linux guys do not care, > > hope that all EFI platforms are smart and AllocatePages() tries hard to > > avoid everything below 1 MiB. We can go this way too. However, I am almost > > sure that sooner or later we will find crazy platforms which allocate memory > > from 0-1 MiB region. We can avoid this by getting EFI memory map, looking > > for > > free regions above 1 MiB and then trying to allocate memory chunk using > > AllocatePages() with "AllocateAddress". Does it make sense? > > I don't see the point of all that, as I don't see why any EFI > implementation would want to deviate from the first line principle > of satisfying allocation requests as high as possible. In general this is good idea. However, I have not seen such requirement in UEFI spec. So, I suppose that bad things may happen on some EFI implementations and that is why I proposed a bit "smarter" approach. On the other hand if Linux does allocations in "simple" way (just AllocatePages() call) then it means that this solution works on most platforms if not all. So, probably "simple" solution would work for us too. Anyway, I think that it is worth considering all potential issues if we are aware of them in advance. > Apart from that using (only) EFI allocation mechanisms for > obtaining the trampoline area won't work anyway, as we already > know there are systems where all of the memory below 1Mb is > in use by EFI (mostly with boot kind allocations, i.e. becoming > available after ExitBootServices()). I know about that. However, I am talking here about memory allocation for EFI memory map. As I said above this region may potentially (well, it looks that probability is low but as I said earlier we should think and discuss this issue here) conflict with trampoline region. Though I am not saying how this region (for trampoline) should be allocated because current solution works well. > >> 2) We may allocate memory area statically somewhere in Xen code which > >>could be used as memory pool for early dynamic allocations. Looks > >>quite simple. Additionally, it would not depend on EFI at all and > >>could be used on legacy BIOS platforms if we need it. However, we > >>must carefully choose size of this pool. We do not want increase > >>Xen binary size too much and waste too much memory but also we must fit > >>at least memory map on x86 EFI platforms. As I saw on small machine, > >>e.g. IBM System x3550 M2 with 8 GiB RAM, memory map may contain more > >>than 200 entries. Every entry on x86-64 platform is 40 bytes in size. > >>So, it means that we need more than 8 KiB for EFI memory map only. > >>Additionally, if we want to use this memory pool for Xen and modules > >>command line storage (it would be used when xen.efi is executed as EFI > >>application) then we should add, I think, about 1 KiB. In this case, > >>to be on safe side, we should assume at least 64 KiB pool for early > >>memory allocations, which is about 4 times of our earlier calculations. > >>However, during discussion on Xen-devel Jan Beulich suggested that > >>
Re: [Xen-devel] [PATCH v5 10/17] xen/arm: map_regions_rw_cache: Map the region with p2m->default_access
On Wed, 6 Jul 2016, Julien Grall wrote: > (CC Tamas) > > On 06/07/16 11:43, Stefano Stabellini wrote: > > On Tue, 28 Jun 2016, Julien Grall wrote: > > > The parameter 'access' is used by memaccess to restrict temporarily the > > > permission. This parameter should not be used for other purpose (such > > > as restricting permanently the permission). > > > > > > The type p2m_mmio_direct will map the region Read-Write and > > > non-executable. Note that this is already the current behavior with the > > > combination of the type and the access. So there is no functional > > > change. > > > > > > Signed-off-by: Julien Grall > > > > I could be mistaken, but isn't default_access actually p2m_access_rwx? > > By default, the access is p2m_access_rwx. However this can be changed by > memaccess and the new default value is stored in arch->p2m.default_access. I > have CCed Tamas to confirm that. > > Note that this is how the other calls are done. This patch replaces p2m_access_rw with default_access, which is p2m_access_rwx by default. Is it actually indended? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] xenbits "official" repo for XTF (was Re: [PATCH 0/2] xtf: add launcher (+1 bugfix)
On 01/07/2016 20:04, "Ian Jackson" wrote: >David Vrabel writes ("Re: [Xen-devel] xenbits "official" repo for XTF >(was Re: [PATCH 0/2] xtf: add launcher (+1 bugfix)"): >> On 20/06/16 18:03, Ian Jackson wrote: >> > Hopefully we can find one that Andrew likes and that's acceptable to >> > the committers. >> > >> > I suggest >> > xen-microvm-test-framework >> > xen-microvm-test-suite >> > xtf-microvm-suite >> >> "xtf" > >iwj@xenbits:~$ ls ~xen/git >kemari qemu-xen-4.1-testing.git >libvirt.gitqemu-xen-4.2-testing.git >linux-2.6-xen.git qemu-xen-4.3-testing.git >linux-pvops.gitqemu-xen-4.4-testing.git >livepatch-build-tools.git qemu-xen-4.5-testing.git >mini-os.gitqemu-xen-4.6-testing.git >osstestqemu-xen.git >osstest.gitqemu-xen-traditional.git >ovmf.git qemu-xen-unstable.git >people raisin.git >pvdrivers rumpuser-xen.git >qemu-upstream-4.2-testing.git seabios.git >qemu-upstream-4.3-testing.git staging >qemu-upstream-4.4-testing.git xcp >qemu-upstream-4.5-testing.git xenalyze.git >qemu-upstream-4.6-testing.git xenclient >qemu-upstream-unstable.git xen.git >qemu-xen-3.3-testing.git xen.git-aside >qemu-xen-3.4-testing.git xenrt-citrix >qemu-xen-4.0-testing.git xentesttools >iwj@xenbits:~$ > >> It seems unfair to give Andrew's project a clunky (repo) name because >> osstest is not sufficiently discoverable. > >Is that a complaint that it's too long ? > >I could live with "xtf", although I think it's rather too short. Let me just summarise my understanding of the discussion: A) The label xtf has been around for a while, thus changing it may be confusing B) Generally there is concern about not being able to discover test related technology easily in the git tree C) More verbose repository names seem to be a little long D) None of the other proposals on the table seem to be much clearer / more accurate I couldn't tell from the thread who really is in favour. As far as I can tell 1 committer is in favour of using xtf.git 2 committer have voiced concrete concerns A number of other committers and Linux maintainers have provided some input, but have not been very clear about their position. Looking at the above, it occurs to me that, this whole area seems to be a little inconsistent anyway and could do with a little house-keeping. We have - osstest.git - there also is osstest/*.git which seems to be odd and seems to have been inactive for a while (not very clear to me what these do) - and we have old and inactive xentesttools/*.git - and we are adding a new repo for XTF Maybe, moving everything test related under testing/* or test/* would be sensible, but that would cause some disruption (not sure how bad that would be). It would address A, B and D. It wouldn't make C much worse than now. We already follow a similar pattern for out-of-tree via pvdrivers/* It does in fact occur to me that some of the older inactive repos should be archived somehow: candidates seem to be kemari/*, xentesttools/*, xenclient/*, xcp/*, ... pollute the namespace. If we are concerned about the namespace, we should address this at some point. There are also some inactive top level git repos such as linux-2.5-xen.git and quite a few inactive people repos. >Lars, can you please advise what process we need to use to come to >closure on this decision ? Really we need to come to some sort of proposal. My gut feel is that maybe this would most quickly resolved in a short IRC meeting, where we discuss this issue and reduce it down to a concrete proposal (IRC log could then be posted) and then put it to a vote, if need be. Otherwise this discussion will drag on for a while as e-mail. Lars ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 15/17] xen/arm: p2m: Introduce helpers to insert and remove mapping
Hi Stefano, On 06/07/16 11:59, Stefano Stabellini wrote: On Tue, 28 Jun 2016, Julien Grall wrote: More the half of the arguments of INSERT and REMOVE are the same for each callers. Simplify the callers of apply_p2m_changes by adding new helpers which will fill common arguments with default values. Signed-off-by: Julien Grall I don't see much value in this patch. It looks good because it is before "Rework the interface of apply_p2m_changes and use typesafe" in this series (therefore eliminates a bunch of temporary casts), but otherwise I don't think it would be much of an improvement. A lot of the parameters of apply_p2m_changes may not apply to a specific set of operations. Every time we have to add a new parameters for a specific operations, we have to modify all the callers. Also, with this patch, it is easier to understand what does every function without caring of dummy parameters. For instance if we take the example of INSERT: return apply_p2m_changes(d, INSERT, pfn_to_paddr(gfn_x(start_gfn)), pfn_to_paddr(gfn_x(start_gfn) + nr), pfn_to_paddr(mfn_x(mfn)), MATTR_DEV, 0, p2m_mmio_direct, d->arch.p2m.default_access); Aside the pfn_to_paddr(*) which will be clean-up in a follow-up patch, 2 of thoses parameters (2, 7 and 9) are common to all the callers. It is hard to understand what means '0' without looking carefully at apply_p2m_changes. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 10/17] xen/arm: map_regions_rw_cache: Map the region with p2m->default_access
On 06/07/16 12:17, Stefano Stabellini wrote: On Wed, 6 Jul 2016, Julien Grall wrote: (CC Tamas) On 06/07/16 11:43, Stefano Stabellini wrote: On Tue, 28 Jun 2016, Julien Grall wrote: The parameter 'access' is used by memaccess to restrict temporarily the permission. This parameter should not be used for other purpose (such as restricting permanently the permission). The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change. Signed-off-by: Julien Grall I could be mistaken, but isn't default_access actually p2m_access_rwx? By default, the access is p2m_access_rwx. However this can be changed by memaccess and the new default value is stored in arch->p2m.default_access. I have CCed Tamas to confirm that. Note that this is how the other calls are done. This patch replaces p2m_access_rw with default_access, which is p2m_access_rwx by default. Is it actually indended? Yes, I explained why in the commit message. "The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change." Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V3 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
Hi Shanker, On 28/06/16 15:33, Shanker Donthineni wrote: On 06/28/2016 08:51 AM, Shanker Donthineni wrote: Hi Julien, On 06/28/2016 05:40 AM, Julien Grall wrote: Hello Shanker, On 27/06/16 21:33, Shanker Donthineni wrote: @@ -1397,6 +1408,36 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, } static int __init +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header, + const unsigned long end) +{ +struct acpi_madt_generic_interrupt *processor; +u32 size; + +processor = (struct acpi_madt_generic_interrupt *)header; +if ( !(processor->flags & ACPI_MADT_ENABLED) ) +return 0; You did not answer to my question on previous version of this patch. You said that "Disabled GICC entries should be skipped because its Redistributor region is not always-on power domain." However from my understanding, an usable CPU may have his Redistributor in the not always-on power domain. So the issue would the same, correct? The gicv3_populate_rdist() is not supposed to read GICR registers if the the associated hardware GICR block is in power-off state. The CPU accesses to disabled GICR region leads to either a system hang or an unexpected behavior. The description of flag ACPI_MADT_ENABLED in ACPI-6.1 says "If zero, this processor in unusable, and the operating system support will not attempt to use it". Ok. The patch looks good to me then. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 10/17] xen/arm: map_regions_rw_cache: Map the region with p2m->default_access
On 06/07/16 12:22, Julien Grall wrote: On 06/07/16 12:17, Stefano Stabellini wrote: On Wed, 6 Jul 2016, Julien Grall wrote: (CC Tamas) On 06/07/16 11:43, Stefano Stabellini wrote: On Tue, 28 Jun 2016, Julien Grall wrote: The parameter 'access' is used by memaccess to restrict temporarily the permission. This parameter should not be used for other purpose (such as restricting permanently the permission). The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change. Signed-off-by: Julien Grall I could be mistaken, but isn't default_access actually p2m_access_rwx? By default, the access is p2m_access_rwx. However this can be changed by memaccess and the new default value is stored in arch->p2m.default_access. I have CCed Tamas to confirm that. Note that this is how the other calls are done. This patch replaces p2m_access_rw with default_access, which is p2m_access_rwx by default. Is it actually indended? Yes, I explained why in the commit message. "The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change." Thinking a bit more, I will reword the commit message with: "The parameter 'access' used by memaccess to restrict temporarily the permission. This parameter should not be for other purpose (such as restricting permanently the permission). Instead, we should use the default_access provided by memaccess. When it is not enabled, the access will be p2m_access_rwx (i.e no restriction applied). The type p2m_mmio_direct will map the region read-write and non-executable before any further restriction by memaccess. note that this is already the current behavior with the combination of the type and the access. So there is no functional change". Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 17/17] xen/arm: p2m: Rework the interface of apply_p2m_changes and use typesafe
Hi Stefano, On 06/07/16 12:06, Stefano Stabellini wrote: On Tue, 28 Jun 2016, Julien Grall wrote: @@ -1146,12 +1148,14 @@ out: if ( rc < 0 && ( op == INSERT ) && addr != start_gpaddr ) { +unsigned long gfn = paddr_to_pfn(addr); + BUG_ON(addr == end_gpaddr); /* * addr keeps the address of the end of the last successfully-inserted * mapping. */ -apply_p2m_changes(d, REMOVE, start_gpaddr, addr, orig_maddr, +apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn, Worth considering a gfn_sub (we already have gfn_add)? I thought about it. However the prototype of gfn_sub and gfn_add would be different. The former is subtracting two gfns whilst the latter is add a value to a gfn. So I think it would be confusing for the user. Although, I am open to any other suggestion. Note that I am working on a series to rework the P2M code for supporting Break-Before-Make. And a lot of this code will be different. In any case Acked-by: Stefano Stabellini Thank you! Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c
>>> On 06.07.16 at 12:27, wrote: > On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote: >> >>> On 05.07.16 at 20:33, wrote: >> > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: >> >> >>> On 25.05.16 at 18:45, wrote: >> >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: >> >> >> >>> On 15.04.16 at 14:33, wrote: >> >> >> > --- a/xen/arch/x86/efi/stub.c >> >> >> > +++ b/xen/arch/x86/efi/stub.c >> >> >> > @@ -8,6 +8,14 @@ >> >> >> > const bool_t efi_enabled = 0; >> >> >> > #endif >> >> >> > >> >> >> > +struct efi __read_mostly efi = { >> >> >> > +.acpi= EFI_INVALID_TABLE_ADDR, >> >> >> > +.acpi20 = EFI_INVALID_TABLE_ADDR, >> >> >> > +.mps = EFI_INVALID_TABLE_ADDR, >> >> >> > +.smbios = EFI_INVALID_TABLE_ADDR, >> >> >> > +.smbios3 = EFI_INVALID_TABLE_ADDR >> >> >> > +}; >> >> >> >> >> >> I don't view duplicating this here as a good approach - you'd better >> >> >> move the existing instance elsewhere. If this was a temporary thing >> >> >> (until a later patch), it might be acceptable, but since building >> >> >> without >> >> >> EFI support will need to remain an option (for people using older tool >> >> >> chains), I don't expect a later patch to remove this. >> >> > >> >> > Do you think about separate C file which should contain efi struct >> >> > and should be included in stub.c and runtime.c? Or anything else? >> >> >> >> A separate file seems to be overkill. Just move it to some other >> >> existing file; I'm sure some sensible place can be found. >> > >> > This solution is not perfect, however, I cannot find better place for >> > efi struct. If you have one then drop me a line. >> >> common/kernel.c or common/lib.c. > > This means that we must delete efi struct initialization from > xen/common/efi/runtime.c and xen/arch/x86/efi/stub.c and put > it in one of both files mentioned by you. Is it OK for you? Note how in my original reply I said "move the existing instance elsewhere". Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 0/3] xen: add full support for CONFIG_PARAVIRT_TIME_ACCOUNTING
On 06/07/16 06:00, Juergen Gross wrote: > With most recent Xen hypervisor (4.8) it is now possible to add full > support of CONFIG_PARAVIRT_TIME_ACCOUNTING. > > To be applied on top of commit ed2f61fdd2356c2a1d1239aa1507ce4e2e059306 > "xen: add steal_clock support on x86" of kernel/git/xen/tip.git > > Runtime tested on x86_64, compile tested on arm64. Applied to for-linus-4.8, thanks. David ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/3] libxl: add framework for device types
On 06/07/16 13:04, Ian Jackson wrote: > Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"): >> Instead of duplicate coding for each device type (vtpms, usbctrls, ...) >> especially on domain creation introduce a framework for that purpose. > > I think something like this is a jolly good idea. Thanks a lot! > > The rough shape - a set of structs with what amount to method calls - > seems a good direction to be going in. > > I have some comments/questions. > > I saw this in 2/3: > >> +for (i = 0; i < d_config->num_pcidevs; i++) { >> +rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1); >> +if (rc < 0) { >> +LOG(ERROR, "libxl_device_pci_add failed: %d", rc); >> +goto out; >> +} >> +} >> + > > And there is similar code in 3/3 for dtdevs. Could that be lifted > away somehow ? (You'd have to take some care about the types, sadly; > ie, I think libxl__device_pci_add might have to start to take a > void*; maybe some macros could make things typesafe?) I thought about this idea already. I think we would end up with more code which would be rather unpleasant to read. Main reason is the need for a dtdev wrapper function and the pci backend creation. > In 1/3: > >> +struct libxl_device_type libxl__usbctrl_devtype = { >> +.type = "usbctrl", >> +.num_offset = offsetof(libxl_domain_config, num_usbctrls), >> +.add= libxl__add_usbctrls, >> +}; > > And then num_offset is used like this: > >> +if (*(int *)((void *)d_config + dt->num_offset) > 0) { > > This is a fine approach but I would prefer it if the there were a bit > more type safety, particularly in the parts that have to occur once > for each device type. > > For example, there is nothing stopping one using this pattern with a > num_* field which is not an int. > > Perhaps there should be a macro for generating the libxl_device_type > contents ? It could perhaps take `usbctrls' and make `num_usbctrls' > out of it using token pasting. I think 'usbctrl' as the macro parameter would be just fine: it would allow to generate the complete struct: #define DEFINE_DEVICE_STRUCT(name) \ const libxl_device_type libxl__ ## name ## _devtype = { \ .type = # name , \ .num_offset = offsetof(libxl_domain_config, num_ ## name ## s), \ .add= libxl__add_ ## name ## s, \ } DEFINE_DEVICE_STRUCT(usbctrl); > Also these structs should be static const, I think. static: sometimes, const: yes. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 17/17] xen/arm: p2m: Rework the interface of apply_p2m_changes and use typesafe
On 06/07/16 12:56, Julien Grall wrote: > Hi Stefano, > > On 06/07/16 12:06, Stefano Stabellini wrote: >> On Tue, 28 Jun 2016, Julien Grall wrote: >>> @@ -1146,12 +1148,14 @@ out: >>> if ( rc < 0 && ( op == INSERT ) && >>>addr != start_gpaddr ) >>> { >>> +unsigned long gfn = paddr_to_pfn(addr); >>> + >>> BUG_ON(addr == end_gpaddr); >>> /* >>>* addr keeps the address of the end of the last >>> successfully-inserted >>>* mapping. >>>*/ >>> -apply_p2m_changes(d, REMOVE, start_gpaddr, addr, orig_maddr, >>> +apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn, >> >> Worth considering a gfn_sub (we already have gfn_add)? > > I thought about it. However the prototype of gfn_sub and gfn_add would > be different. > > The former is subtracting two gfns whilst the latter is add a value to > a gfn. So I think it would be confusing for the user. Although, I am > open to any other suggestion. This is the one problem with these boxed types; we have to reimplement all the base operations ourselves. I would caution against introducing too many helpers. The manual unbox and manipulate does work, and it should only be the very common variations which should be abstracted. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/3] libxl: add framework for device types
Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"): > On 06/07/16 13:04, Ian Jackson wrote: > >> +for (i = 0; i < d_config->num_pcidevs; i++) { > >> +rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1); > >> +if (rc < 0) { > >> +LOG(ERROR, "libxl_device_pci_add failed: %d", rc); > >> +goto out; > >> +} > >> +} > >> + > > > > And there is similar code in 3/3 for dtdevs. Could that be lifted > > away somehow ? (You'd have to take some care about the types, sadly; > > ie, I think libxl__device_pci_add might have to start to take a > > void*; maybe some macros could make things typesafe?) > > I thought about this idea already. I think we would end up with more > code which would be rather unpleasant to read. Main reason is the > need for a dtdev wrapper function and the pci backend creation. I'm not sure what you mean by dtdev wrapper function. As for pci backend, there could be a separate hook for "after adding all devices of this type". But if you don't think this is feasible I won't insist on it. The approach you have is already a big improvement. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/3] libxl: add framework for device types
On 06/07/16 14:47, Ian Jackson wrote: > Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device > types"): >> On 06/07/16 13:04, Ian Jackson wrote: +for (i = 0; i < d_config->num_pcidevs; i++) { +rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1); +if (rc < 0) { +LOG(ERROR, "libxl_device_pci_add failed: %d", rc); +goto out; +} +} + >>> >>> And there is similar code in 3/3 for dtdevs. Could that be lifted >>> away somehow ? (You'd have to take some care about the types, sadly; >>> ie, I think libxl__device_pci_add might have to start to take a >>> void*; maybe some macros could make things typesafe?) >> >> I thought about this idea already. I think we would end up with more >> code which would be rather unpleasant to read. Main reason is the >> need for a dtdev wrapper function and the pci backend creation. > > I'm not sure what you mean by dtdev wrapper function. The loop for dtdev calls a xc_ function with different parameters than the one for pci. We'd need a libxl__device_dtdev_add() wrapper function to do the xc_ call. > As for pci backend, there could be a separate hook for "after adding > all devices of this type". Right. And summing up the additional hooks and queries whether they are specified sums up to more overhead than the current version. > But if you don't think this is feasible I won't insist on it. The > approach you have is already a big improvement. Thanks. I'm planning to add more to it (e.g. I'd like to get rid of the MERGE() macro in libxl_retrieve_domain_configuration() in favor of a device type hook in order to be able to have _all_ stuff for one type in one source file. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c
On Wed, Jul 06, 2016 at 06:00:47AM -0600, Jan Beulich wrote: > >>> On 06.07.16 at 12:27, wrote: > > On Wed, Jul 06, 2016 at 12:55:42AM -0600, Jan Beulich wrote: > >> >>> On 05.07.16 at 20:33, wrote: > >> > On Fri, May 27, 2016 at 02:16:09AM -0600, Jan Beulich wrote: > >> >> >>> On 25.05.16 at 18:45, wrote: > >> >> > On Wed, May 25, 2016 at 01:03:42AM -0600, Jan Beulich wrote: > >> >> >> >>> On 15.04.16 at 14:33, wrote: > >> >> >> > --- a/xen/arch/x86/efi/stub.c > >> >> >> > +++ b/xen/arch/x86/efi/stub.c > >> >> >> > @@ -8,6 +8,14 @@ > >> >> >> > const bool_t efi_enabled = 0; > >> >> >> > #endif > >> >> >> > > >> >> >> > +struct efi __read_mostly efi = { > >> >> >> > + .acpi= EFI_INVALID_TABLE_ADDR, > >> >> >> > + .acpi20 = EFI_INVALID_TABLE_ADDR, > >> >> >> > + .mps = EFI_INVALID_TABLE_ADDR, > >> >> >> > + .smbios = EFI_INVALID_TABLE_ADDR, > >> >> >> > + .smbios3 = EFI_INVALID_TABLE_ADDR > >> >> >> > +}; > >> >> >> > >> >> >> I don't view duplicating this here as a good approach - you'd better > >> >> >> move the existing instance elsewhere. If this was a temporary thing > >> >> >> (until a later patch), it might be acceptable, but since building > >> >> >> without > >> >> >> EFI support will need to remain an option (for people using older > >> >> >> tool > >> >> >> chains), I don't expect a later patch to remove this. > >> >> > > >> >> > Do you think about separate C file which should contain efi struct > >> >> > and should be included in stub.c and runtime.c? Or anything else? > >> >> > >> >> A separate file seems to be overkill. Just move it to some other > >> >> existing file; I'm sure some sensible place can be found. > >> > > >> > This solution is not perfect, however, I cannot find better place for > >> > efi struct. If you have one then drop me a line. > >> > >> common/kernel.c or common/lib.c. > > > > This means that we must delete efi struct initialization from > > xen/common/efi/runtime.c and xen/arch/x86/efi/stub.c and put > > it in one of both files mentioned by you. Is it OK for you? > > Note how in my original reply I said "move the existing instance > elsewhere". OK, thanks. I will do that. Daniel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 01/14] xen: Use the typesafe mfn and gfn in map_mmio_regions...
to avoid mixing machine frame with guest frame. Signed-off-by: Julien Grall Acked-by: Jan Beulich Acked-by: Stefano Stabellini --- Cc: Stefano Stabellini Cc: Jan Beulich Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Konrad Rzeszutek Wilk Cc: Tim Deegan Cc: Wei Liu Changes in v6: - Add Stefano's acked-by Changes in v3: - Use mfn_add when it is possible - Add Jan's acked-by --- xen/arch/arm/domain_build.c | 4 ++-- xen/arch/arm/gic-v2.c| 4 ++-- xen/arch/arm/p2m.c | 22 +++--- xen/arch/arm/platforms/exynos5.c | 8 xen/arch/arm/platforms/omap5.c | 16 xen/arch/arm/vgic-v2.c | 4 ++-- xen/arch/x86/mm/p2m.c| 18 ++ xen/common/domctl.c | 4 ++-- xen/include/xen/p2m-common.h | 8 9 files changed, 45 insertions(+), 43 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9035486..49185f0 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1036,9 +1036,9 @@ static int map_range_to_domain(const struct dt_device_node *dev, if ( need_mapping ) { res = map_mmio_regions(d, - paddr_to_pfn(addr), + _gfn(paddr_to_pfn(addr)), DIV_ROUND_UP(len, PAGE_SIZE), - paddr_to_pfn(addr)); + _mfn(paddr_to_pfn(addr))); if ( res < 0 ) { printk(XENLOG_ERR "Unable to map 0x%"PRIx64 diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 4e2f4c7..3893ece 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -601,9 +601,9 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d) d->domain_id, v2m_data->addr, v2m_data->size, v2m_data->spi_start, v2m_data->nr_spis); -ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr), +ret = map_mmio_regions(d, _gfn(paddr_to_pfn(v2m_data->addr)), DIV_ROUND_UP(v2m_data->size, PAGE_SIZE), -paddr_to_pfn(v2m_data->addr)); +_mfn(paddr_to_pfn(v2m_data->addr))); if ( ret ) { printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n", diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 0395a40..34563bb 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d, } int map_mmio_regions(struct domain *d, - unsigned long start_gfn, + gfn_t start_gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, INSERT, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_DEV, 0, p2m_mmio_direct, d->arch.p2m.default_access); } int unmap_mmio_regions(struct domain *d, - unsigned long start_gfn, + gfn_t start_gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_DEV, 0, p2m_invalid, d->arch.p2m.default_access); } @@ -1280,7 +1280,7 @@ int map_dev_mmio_region(struct domain *d, if ( !(nr && iomem_access_permitted(d, mfn, mfn + nr - 1)) ) return 0; -res = map_mmio_regions(d, start_gfn, nr, mfn); +res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn)); if ( res < 0 ) { printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n", diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c index bf4964d..c43934f 100644 --- a/xen/arch/arm/platforms/exynos5.c +++ b/xen/arch/arm/platforms/exynos5.c @@ -83,12 +83,12 @@ static int exynos5_init_time(void) static int exynos5250_specific_mapping(struct domain *d) { /* Map the chip ID */ -map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID), 1, - paddr_to_pfn(EXYNOS5_PA_CHIPID)); +map_mmio_regions(d, _gfn(paddr_t
[Xen-devel] [PATCH v6 05/14] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn
The prototype and the declaration of p2m_lookup disagree on how the function should be used. One expect a frame number whilst the other an address. Thankfully, everyone is using with an address today. However, most of the callers have to convert a guest frame to an address. Modify the interface to take a guest physical frame in parameter and return a machine frame. Whilst modifying the interface, use typesafe gfn and mfn for clarity and catching possible misusage. Signed-off-by: Julien Grall Acked-by: Stefano Stabellini --- Changes in v6: - Add Stefano's acked-by Changes in v4: - Use INVALID_MFN_T when possible --- xen/arch/arm/p2m.c| 43 +++ xen/arch/arm/traps.c | 21 +++-- xen/include/asm-arm/p2m.h | 7 +++ 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index c938dde..54a363a 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d) } /* - * Lookup the MFN corresponding to a domain's PFN. + * Lookup the MFN corresponding to a domain's GFN. * * There are no processor functions to do a stage 2 only lookup therefore we * do a a software walk. */ -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) { struct p2m_domain *p2m = &d->arch.p2m; +const paddr_t paddr = pfn_to_paddr(gfn_x(gfn)); const unsigned int offsets[4] = { zeroeth_table_offset(paddr), first_table_offset(paddr), @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK }; lpae_t pte, *map; -paddr_t maddr = INVALID_PADDR; +mfn_t mfn = INVALID_MFN; paddr_t mask = 0; p2m_type_t _t; unsigned int level, root_table; @@ -216,21 +217,22 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) { ASSERT(mask); ASSERT(pte.p2m.type != p2m_invalid); -maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask); +mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) | +(paddr & ~mask))); *t = pte.p2m.type; } err: -return maddr; +return mfn; } -paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) +mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) { -paddr_t ret; +mfn_t ret; struct p2m_domain *p2m = &d->arch.p2m; spin_lock(&p2m->lock); -ret = __p2m_lookup(d, paddr, t); +ret = __p2m_lookup(d, gfn, t); spin_unlock(&p2m->lock); return ret; @@ -493,8 +495,9 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, * No setting was found in the Radix tree. Check if the * entry exists in the page-tables. */ -paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL); -if ( INVALID_PADDR == maddr ) +mfn_t mfn = __p2m_lookup(d, gfn, NULL); + +if ( mfn_eq(mfn, INVALID_MFN) ) return -ESRCH; /* If entry exists then its rwx. */ @@ -1483,8 +1486,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn) mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn) { -paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL); -return _mfn(p >> PAGE_SHIFT); +return p2m_lookup(d, gfn, NULL); } /* @@ -1498,8 +1500,8 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) { long rc; paddr_t ipa; -unsigned long maddr; -unsigned long mfn; +gfn_t gfn; +mfn_t mfn; xenmem_access_t xma; p2m_type_t t; struct page_info *page = NULL; @@ -1508,11 +1510,13 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) if ( rc < 0 ) goto err; +gfn = _gfn(paddr_to_pfn(ipa)); + /* * We do this first as this is faster in the default case when no * permission is set on the page. */ -rc = __p2m_get_mem_access(current->domain, _gfn(paddr_to_pfn(ipa)), &xma); +rc = __p2m_get_mem_access(current->domain, gfn, &xma); if ( rc < 0 ) goto err; @@ -1561,12 +1565,11 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) * We had a mem_access permission limiting the access, but the page type * could also be limiting, so we need to check that as well. */ -maddr = __p2m_lookup(current->domain, ipa, &t); -if ( maddr == INVALID_PADDR ) +mfn = __p2m_lookup(current->domain, gfn, &t); +if ( mfn_eq(mfn, INVALID_MFN) ) goto err; -mfn = maddr >> PAGE_SHIFT; -if ( !mfn_valid(mfn) ) +if ( !mfn_valid(mfn_x(mfn)) ) goto err; /* @@ -1575,7 +1578,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long f
[Xen-devel] [PATCH v6 04/14] xen: Use a typesafe to define INVALID_GFN
Also take the opportunity to convert arch/x86/debug.c to the typesafe gfn. Signed-off-by: Julien Grall Reviewed-by: Andrew Cooper Acked-by: Stefano Stabellini --- Cc: Mukesh Rathor Cc: Jan Beulich Cc: Paul Durrant Cc: Boris Ostrovsky Cc: Suravee Suthikulpanit Cc: Jun Nakajima Cc: Kevin Tian Cc: George Dunlap Cc: Tim Deegan Cc: Feng Wu Changes in v6: - Add Stefano's acked-by for ARM bits - Remove set of brackets when it is not necessary - Add Andrew's reviewed-by Changes in v5: - Patch added --- xen/arch/arm/p2m.c | 4 ++-- xen/arch/x86/debug.c| 18 +- xen/arch/x86/domain.c | 2 +- xen/arch/x86/hvm/emulate.c | 7 --- xen/arch/x86/hvm/hvm.c | 6 +++--- xen/arch/x86/hvm/ioreq.c| 8 xen/arch/x86/hvm/svm/nestedsvm.c| 2 +- xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- xen/arch/x86/mm/altp2m.c| 2 +- xen/arch/x86/mm/hap/guest_walk.c| 10 +- xen/arch/x86/mm/hap/nested_ept.c| 2 +- xen/arch/x86/mm/p2m-pod.c | 6 +++--- xen/arch/x86/mm/p2m.c | 18 +- xen/arch/x86/mm/shadow/common.c | 2 +- xen/arch/x86/mm/shadow/multi.c | 2 +- xen/arch/x86/mm/shadow/private.h| 2 +- xen/drivers/passthrough/amd/iommu_map.c | 2 +- xen/drivers/passthrough/vtd/iommu.c | 4 ++-- xen/drivers/passthrough/x86/iommu.c | 2 +- xen/include/asm-x86/guest_pt.h | 4 ++-- xen/include/asm-x86/p2m.h | 2 +- xen/include/xen/mm.h| 2 +- 22 files changed, 57 insertions(+), 56 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d690602..c938dde 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -479,7 +479,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, } /* If request to get default access. */ -if ( gfn_x(gfn) == INVALID_GFN ) +if ( gfn_eq(gfn, INVALID_GFN) ) { *access = memaccess[p2m->default_access]; return 0; @@ -1879,7 +1879,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, p2m->mem_access_enabled = true; /* If request to set default access. */ -if ( gfn_x(gfn) == INVALID_GFN ) +if ( gfn_eq(gfn, INVALID_GFN) ) { p2m->default_access = a; return 0; diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c index 9213ea7..3030022 100644 --- a/xen/arch/x86/debug.c +++ b/xen/arch/x86/debug.c @@ -44,8 +44,7 @@ typedef unsigned char dbgbyte_t; /* Returns: mfn for the given (hvm guest) vaddr */ static mfn_t -dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, -unsigned long *gfn) +dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, gfn_t *gfn) { mfn_t mfn; uint32_t pfec = PFEC_page_present; @@ -53,14 +52,14 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id); -*gfn = paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec); -if ( *gfn == INVALID_GFN ) +*gfn = _gfn(paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec)); +if ( gfn_eq(*gfn, INVALID_GFN) ) { DBGP2("kdb:bad gfn from gva_to_gfn\n"); return INVALID_MFN; } -mfn = get_gfn(dp, *gfn, &gfntype); +mfn = get_gfn(dp, gfn_x(*gfn), &gfntype); if ( p2m_is_readonly(gfntype) && toaddr ) { DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype); @@ -72,7 +71,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, if ( mfn_eq(mfn, INVALID_MFN) ) { -put_gfn(dp, *gfn); +put_gfn(dp, gfn_x(*gfn)); *gfn = INVALID_GFN; } @@ -165,7 +164,8 @@ unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr, char *va; unsigned long addr = (unsigned long)gaddr; mfn_t mfn; -unsigned long gfn = INVALID_GFN, pagecnt; +gfn_t gfn = INVALID_GFN; +unsigned long pagecnt; pagecnt = min_t(long, PAGE_SIZE - (addr & ~PAGE_MASK), len); @@ -189,8 +189,8 @@ unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr, } unmap_domain_page(va); -if ( gfn != INVALID_GFN ) -put_gfn(dp, gfn); +if ( !gfn_eq(gfn, INVALID_GFN) ) +put_gfn(dp, gfn_x(gfn)); addr += pagecnt; buf += pagecnt; diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index bb59247..c8c7e2d 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -783,7 +783,7 @@ int arch_domain_soft_reset(struct domain *d) * gfn == INVALID_GFN indicates that the shared_info page was never mapped * to the domain's address space and there is nothing to replace. */ -if ( gfn == INVALID_GFN ) +if ( gfn == gfn_x(INVALID_GFN) ) goto ex
[Xen-devel] [PATCH v6 12/14] xen/arm: p2m: Introduce helpers to insert and remove mapping
More the half of the arguments of INSERT and REMOVE are the same for each callers. Simplify the callers of apply_p2m_changes by adding new helpers which will fill common arguments with default values. Signed-off-by: Julien Grall --- Changes in v5: - Add missing Signed-off-by Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 70 -- 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 2ba9477..b98eff4 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1158,17 +1158,40 @@ out: return rc; } +static inline int p2m_insert_mapping(struct domain *d, + gfn_t start_gfn, + unsigned long nr, + mfn_t mfn, + int mattr, p2m_type_t t) +{ +return apply_p2m_changes(d, INSERT, + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), + mattr, 0, t, d->arch.p2m.default_access); +} + +static inline int p2m_remove_mapping(struct domain *d, + gfn_t start_gfn, + unsigned long nr, + mfn_t mfn) +{ +return apply_p2m_changes(d, REMOVE, + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), + /* arguments below not used when removing mapping */ + MATTR_MEM, 0, p2m_invalid, + d->arch.p2m.default_access); +} + int map_regions_rw_cache(struct domain *d, gfn_t gfn, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(gfn)), - pfn_to_paddr(gfn_x(gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_MEM, 0, p2m_mmio_direct, - d->arch.p2m.default_access); +return p2m_insert_mapping(d, gfn, nr, mfn, + MATTR_MEM, p2m_mmio_direct); } int unmap_regions_rw_cache(struct domain *d, @@ -1176,12 +1199,7 @@ int unmap_regions_rw_cache(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gfn_x(gfn)), - pfn_to_paddr(gfn_x(gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_MEM, 0, p2m_invalid, - d->arch.p2m.default_access); +return p2m_remove_mapping(d, gfn, nr, mfn); } int map_mmio_regions(struct domain *d, @@ -1189,12 +1207,8 @@ int map_mmio_regions(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_DEV, 0, p2m_mmio_direct, - d->arch.p2m.default_access); +return p2m_insert_mapping(d, start_gfn, nr, mfn, + MATTR_MEM, p2m_mmio_direct); } int unmap_mmio_regions(struct domain *d, @@ -1202,12 +1216,7 @@ int unmap_mmio_regions(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_DEV, 0, p2m_invalid, - d->arch.p2m.default_access); +return p2m_remove_mapping(d, start_gfn, nr, mfn); } int map_dev_mmio_region(struct domain *d, @@ -1237,22 +1246,15 @@ int guest_physmap_add_entry(struct domain *d, unsigned long page_order, p2m_type_t t) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(gfn)), - pfn_to_paddr(gfn_x(gfn) + (1 << page_order)), - pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, t, - d->arch.p2m.default_access); +return p2m_insert_mapping(d, gfn, (1 << page_order), mfn, + MATTR_MEM, t); } void guest_physmap_remove_page(struct domain *d,
[Xen-devel] [PATCH v6 10/14] xen/arm: Use the typesafes mfn and gfn in map_dev_mmio_region...
to avoid mixing machine frame with guest frame. Also drop the prefix start_. Signed-off-by: Julien Grall --- Changes in v6: - Qualify what is being mapped - Use PRI_mfn Changes in v4: - Patch added --- xen/arch/arm/mm.c | 2 +- xen/arch/arm/p2m.c| 12 ++-- xen/include/asm-arm/p2m.h | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 0e408f8..b5fc034 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1145,7 +1145,7 @@ int xenmem_add_to_physmap_one( if ( extra.res0 ) return -EOPNOTSUPP; -rc = map_dev_mmio_region(d, gfn_x(gfn), 1, idx); +rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx)); return rc; default: diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index f11094e..5fe1b91 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1211,20 +1211,20 @@ int unmap_mmio_regions(struct domain *d, } int map_dev_mmio_region(struct domain *d, -unsigned long start_gfn, +gfn_t gfn, unsigned long nr, -unsigned long mfn) +mfn_t mfn) { int res; -if ( !(nr && iomem_access_permitted(d, mfn, mfn + nr - 1)) ) +if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) ) return 0; -res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn)); +res = map_mmio_regions(d, gfn, nr, mfn); if ( res < 0 ) { -printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n", - mfn, mfn + nr - 1, d->domain_id); +printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n", + mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id); return res; } diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 4752161..8d29eda 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -152,9 +152,9 @@ int unmap_regions_rw_cache(struct domain *d, unsigned long mfn); int map_dev_mmio_region(struct domain *d, -unsigned long start_gfn, +gfn_t gfn, unsigned long nr, -unsigned long mfn); +mfn_t mfn); int guest_physmap_add_entry(struct domain *d, gfn_t gfn, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 00/14] xen/arm: Use the typesafes gfn and mfn
Hello all, Some of the ARM functions are mixing gfn vs mfn and even physical vs frame. To avoid more confusion, this patch series makes use of the terminology described in xen/include/xen/mm.h and the associated typesafe. This series requires the patch [1] to be applied beforehand. I pushed a branch with this patch and this series applied on xenbits: git://xenbits.xen.org/people/julieng/xen-unstable.git branch typesafe-v6 For all the changes see in each patch. Yours sincerely, [1] http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg01744.html Cc: Andrew Cooper Cc: Boris Ostrovsky Cc: Christoph Egger Cc: Feng Wu Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Jun Nakajima Cc: Kevin Tian Cc: Konrad Rzeszutek Wilk Cc: Liu Jinsong Cc: Mukesh Rathor Cc: Paul Durrant Cc: Shannon Zhao Cc: Stefano Stabellini Cc: Suravee Suthikulpanit Cc: Tim Deegan Cc: Wei Liu Julien Grall (14): xen: Use the typesafe mfn and gfn in map_mmio_regions... xen/passthrough: x86: Use INVALID_GFN rather than INVALID_MFN xen: Use a typesafe to define INVALID_MFN xen: Use a typesafe to define INVALID_GFN xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn xen/arm: Rework the interface of p2m_cache_flush and use typesafe gfn xen/arm: map_regions_rw_cache: Map the region with p2m->default_access xen/arm: dom0_build: Remove dead code in allocate_memory xen/arm: p2m: Remove unused operation ALLOCATE xen/arm: Use the typesafes mfn and gfn in map_dev_mmio_region... xen/arm: Use the typesafes mfn and gfn in map_regions_rw_cache ... xen/arm: p2m: Introduce helpers to insert and remove mapping xen/arm: p2m: Use typesafe gfn for {max,lowest}_mapped_gfn xen/arm: p2m: Rework the interface of apply_p2m_changes and use typesafe xen/arch/arm/domain_build.c | 70 ++--- xen/arch/arm/domctl.c | 2 +- xen/arch/arm/gic-v2.c | 4 +- xen/arch/arm/mm.c | 4 +- xen/arch/arm/p2m.c | 263 xen/arch/arm/platforms/exynos5.c| 8 +- xen/arch/arm/platforms/omap5.c | 16 +- xen/arch/arm/traps.c| 21 +-- xen/arch/arm/vgic-v2.c | 4 +- xen/arch/x86/cpu/mcheck/mce.c | 2 +- xen/arch/x86/debug.c| 72 - xen/arch/x86/domain.c | 2 +- xen/arch/x86/hvm/emulate.c | 7 +- xen/arch/x86/hvm/hvm.c | 12 +- xen/arch/x86/hvm/ioreq.c| 8 +- xen/arch/x86/hvm/svm/nestedsvm.c| 2 +- xen/arch/x86/hvm/viridian.c | 12 +- xen/arch/x86/hvm/vmx/vmx.c | 8 +- xen/arch/x86/mm/altp2m.c| 2 +- xen/arch/x86/mm/guest_walk.c| 4 +- xen/arch/x86/mm/hap/guest_walk.c| 10 +- xen/arch/x86/mm/hap/hap.c | 4 +- xen/arch/x86/mm/hap/nested_ept.c| 2 +- xen/arch/x86/mm/p2m-ept.c | 6 +- xen/arch/x86/mm/p2m-pod.c | 24 +-- xen/arch/x86/mm/p2m-pt.c| 18 +-- xen/arch/x86/mm/p2m.c | 90 +-- xen/arch/x86/mm/paging.c| 12 +- xen/arch/x86/mm/shadow/common.c | 45 +++--- xen/arch/x86/mm/shadow/multi.c | 38 ++--- xen/arch/x86/mm/shadow/private.h| 2 +- xen/common/domain.c | 6 +- xen/common/domctl.c | 4 +- xen/common/grant_table.c| 6 +- xen/drivers/passthrough/amd/iommu_map.c | 2 +- xen/drivers/passthrough/vtd/iommu.c | 4 +- xen/drivers/passthrough/x86/iommu.c | 2 +- xen/include/asm-arm/p2m.h | 32 ++-- xen/include/asm-x86/guest_pt.h | 4 +- xen/include/asm-x86/p2m.h | 2 +- xen/include/xen/mm.h| 4 +- xen/include/xen/p2m-common.h| 8 +- 42 files changed, 371 insertions(+), 477 deletions(-) -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 02/14] xen/passthrough: x86: Use INVALID_GFN rather than INVALID_MFN
A variable containing a guest frame should be compared to INVALID_GFN and not INVALID_MFN. Signed-off-by: Julien Grall Reviewed-by: Jan Beulich Reviewed-by: Andrew Cooper --- Cc: Suravee Suthikulpanit Changes in v6: - Fix typo in the commit message - Add Andrew's and Jan' reviewed-by Changes in v5: - Patch added --- xen/drivers/passthrough/amd/iommu_map.c | 2 +- xen/drivers/passthrough/x86/iommu.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c index 1b914ba..c758459 100644 --- a/xen/drivers/passthrough/amd/iommu_map.c +++ b/xen/drivers/passthrough/amd/iommu_map.c @@ -555,7 +555,7 @@ static int update_paging_mode(struct domain *d, unsigned long gfn) unsigned long old_root_mfn; struct domain_iommu *hd = dom_iommu(d); -if ( gfn == INVALID_MFN ) +if ( gfn == INVALID_GFN ) return -EADDRNOTAVAIL; ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH)); diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index a18a608..cd435d7 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -61,7 +61,7 @@ int arch_iommu_populate_page_table(struct domain *d) unsigned long mfn = page_to_mfn(page); unsigned long gfn = mfn_to_gmfn(d, mfn); -if ( gfn != INVALID_MFN ) +if ( gfn != INVALID_GFN ) { ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH)); BUG_ON(SHARED_M2P(gfn)); -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 14/14] xen/arm: p2m: Rework the interface of apply_p2m_changes and use typesafe
Most of the callers of apply_p2m_changes have a GFN, a MFN and the number of frame to change in hand. Rather than asking each caller to convert the frame to an address, rework the interfaces to pass the GFN, MFN and the number of frame. Note that it would be possible to do more clean-up in apply_p2m_changes, but this will be done in a follow-up series. Signed-off-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 62 -- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index c7f6766..ce1c1e0 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -906,25 +906,26 @@ static void update_reference_mapping(struct page_info *page, static int apply_p2m_changes(struct domain *d, enum p2m_operation op, - paddr_t start_gpaddr, - paddr_t end_gpaddr, - paddr_t maddr, + gfn_t sgfn, + unsigned long nr, + mfn_t smfn, int mattr, uint32_t mask, p2m_type_t t, p2m_access_t a) { +paddr_t start_gpaddr = pfn_to_paddr(gfn_x(sgfn)); +paddr_t end_gpaddr = pfn_to_paddr(gfn_x(sgfn) + nr); +paddr_t maddr = pfn_to_paddr(mfn_x(smfn)); int rc, ret; struct p2m_domain *p2m = &d->arch.p2m; lpae_t *mappings[4] = { NULL, NULL, NULL, NULL }; struct page_info *pages[4] = { NULL, NULL, NULL, NULL }; -paddr_t addr, orig_maddr = maddr; +paddr_t addr; unsigned int level = 0; unsigned int cur_root_table = ~0; unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 }; unsigned int count = 0; -const unsigned long sgfn = paddr_to_pfn(start_gpaddr), -egfn = paddr_to_pfn(end_gpaddr); const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000; const bool_t preempt = !is_idle_vcpu(current); bool_t flush = false; @@ -986,9 +987,9 @@ static int apply_p2m_changes(struct domain *d, * Preempt setting mem_access permissions as required by XSA-89, * if it's not the last iteration. */ -uint32_t progress = paddr_to_pfn(addr) - sgfn + 1; +uint32_t progress = paddr_to_pfn(addr) - gfn_x(sgfn) + 1; -if ( (egfn - sgfn) > progress && !(progress & mask) ) +if ( nr > progress && !(progress & mask) ) { rc = progress; goto out; @@ -1117,8 +1118,9 @@ static int apply_p2m_changes(struct domain *d, if ( op == INSERT ) { -p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, _gfn(egfn)); -p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, _gfn(sgfn)); +p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, + gfn_add(sgfn, nr)); +p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn); } rc = 0; @@ -1127,7 +1129,7 @@ out: if ( flush ) { flush_tlb_domain(d); -ret = iommu_iotlb_flush(d, sgfn, egfn - sgfn); +ret = iommu_iotlb_flush(d, gfn_x(sgfn), nr); if ( !rc ) rc = ret; } @@ -1146,12 +1148,14 @@ out: if ( rc < 0 && ( op == INSERT ) && addr != start_gpaddr ) { +unsigned long gfn = paddr_to_pfn(addr); + BUG_ON(addr == end_gpaddr); /* * addr keeps the address of the end of the last successfully-inserted * mapping. */ -apply_p2m_changes(d, REMOVE, start_gpaddr, addr, orig_maddr, +apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn, mattr, 0, p2m_invalid, d->arch.p2m.default_access); } @@ -1164,10 +1168,7 @@ static inline int p2m_insert_mapping(struct domain *d, mfn_t mfn, int mattr, p2m_type_t t) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), +return apply_p2m_changes(d, INSERT, start_gfn, nr, mfn, mattr, 0, t, d->arch.p2m.default_access); } @@ -1176,10 +1177,7 @@ static inline int p2m_remove_mapping(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), +return apply_p2m_changes(d, REMOVE, start_gfn, nr, mfn, /* argumen
[Xen-devel] [PATCH v6 11/14] xen/arm: Use the typesafes mfn and gfn in map_regions_rw_cache ...
to avoid mixing machine frame with guest frame. Also rename the parameters of the function and drop pointless PAGE_MASK in the caller. Signed-off-by: Julien Grall Acked-by: Stefano Stabellini --- Changes in v6: - Add Stefano's acked-by Changes in v4: - Patch added --- xen/arch/arm/domain_build.c | 8 xen/arch/arm/p2m.c | 20 ++-- xen/include/asm-arm/p2m.h | 12 ++-- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 923f48a..60db9e4 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1522,9 +1522,9 @@ static void acpi_map_other_tables(struct domain *d) addr = acpi_gbl_root_table_list.tables[i].address; size = acpi_gbl_root_table_list.tables[i].length; res = map_regions_rw_cache(d, - paddr_to_pfn(addr & PAGE_MASK), + _gfn(paddr_to_pfn(addr)), DIV_ROUND_UP(size, PAGE_SIZE), - paddr_to_pfn(addr & PAGE_MASK)); + _mfn(paddr_to_pfn(addr))); if ( res ) { panic(XENLOG_ERR "Unable to map ACPI region 0x%"PRIx64 @@ -1878,9 +1878,9 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo) /* Map the EFI and ACPI tables to Dom0 */ rc = map_regions_rw_cache(d, - paddr_to_pfn(d->arch.efi_acpi_gpa), + _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)), PFN_UP(d->arch.efi_acpi_len), - paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))); + _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table; if ( rc != 0 ) { printk(XENLOG_ERR "Unable to map EFI/ACPI table 0x%"PRIx64 diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 5fe1b91..2ba9477 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1159,27 +1159,27 @@ out: } int map_regions_rw_cache(struct domain *d, - unsigned long start_gfn, + gfn_t gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, INSERT, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(gfn)), + pfn_to_paddr(gfn_x(gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, p2m_mmio_direct, d->arch.p2m.default_access); } int unmap_regions_rw_cache(struct domain *d, - unsigned long start_gfn, + gfn_t gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(gfn)), + pfn_to_paddr(gfn_x(gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); } diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 8d29eda..6e258b9 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -142,14 +142,14 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t); int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); int map_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr_mfns, - unsigned long mfn); + gfn_t gfn, + unsigned long nr, + mfn_t mfn); int unmap_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr_mfns, - unsigned long mfn); + gfn_t gfn, + unsigned long nr, + mfn_t mfn); int map_dev_mmio_region(struct domain *d, gfn_t gfn, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 03/14] xen: Use a typesafe to define INVALID_MFN
Also take the opportunity to convert arch/x86/debug.c to the typesafe mfn and use proper printf format for MFN/GFN when the code around is modified. Signed-off-by: Julien Grall Reviewed-by: Andrew Cooper Acked-by: Stefano Stabellini --- Cc: Christoph Egger Cc: Liu Jinsong Cc: Jan Beulich Cc: Mukesh Rathor Cc: Paul Durrant Cc: Jun Nakajima Cc: Kevin Tian Cc: George Dunlap Cc: Tim Deegan Changes in v6: - Add Stefano's acked-by for ARM bits - Use PRI_mfn and PRI_gfn - Remove set of brackets when it is not necessary - Use mfn_add when possible - Add Andrew's reviewed-by Changes in v5: - Patch added --- xen/arch/arm/p2m.c | 4 +-- xen/arch/x86/cpu/mcheck/mce.c | 2 +- xen/arch/x86/debug.c| 58 + xen/arch/x86/hvm/hvm.c | 6 ++--- xen/arch/x86/hvm/viridian.c | 12 - xen/arch/x86/hvm/vmx/vmx.c | 2 +- xen/arch/x86/mm/guest_walk.c| 4 +-- xen/arch/x86/mm/hap/hap.c | 4 +-- xen/arch/x86/mm/p2m-ept.c | 6 ++--- xen/arch/x86/mm/p2m-pod.c | 18 ++--- xen/arch/x86/mm/p2m-pt.c| 18 ++--- xen/arch/x86/mm/p2m.c | 54 +++--- xen/arch/x86/mm/paging.c| 12 - xen/arch/x86/mm/shadow/common.c | 43 +++--- xen/arch/x86/mm/shadow/multi.c | 36 - xen/common/domain.c | 6 ++--- xen/common/grant_table.c| 6 ++--- xen/include/xen/mm.h| 2 +- 18 files changed, 147 insertions(+), 146 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 34563bb..d690602 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1461,7 +1461,7 @@ int relinquish_p2m_mapping(struct domain *d) return apply_p2m_changes(d, RELINQUISH, pfn_to_paddr(p2m->lowest_mapped_gfn), pfn_to_paddr(p2m->max_mapped_gfn), - pfn_to_paddr(INVALID_MFN), + pfn_to_paddr(mfn_x(INVALID_MFN)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); } @@ -1476,7 +1476,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn) return apply_p2m_changes(d, CACHEFLUSH, pfn_to_paddr(start_mfn), pfn_to_paddr(end_mfn), - pfn_to_paddr(INVALID_MFN), + pfn_to_paddr(mfn_x(INVALID_MFN)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); } diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c index edcbe48..2695b0c 100644 --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -1455,7 +1455,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) gfn = PFN_DOWN(gaddr); mfn = mfn_x(get_gfn(d, gfn, &t)); -if ( mfn == INVALID_MFN ) +if ( mfn == mfn_x(INVALID_MFN) ) { put_gfn(d, gfn); put_domain(d); diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c index 58cae22..9213ea7 100644 --- a/xen/arch/x86/debug.c +++ b/xen/arch/x86/debug.c @@ -43,11 +43,11 @@ typedef unsigned long dbgva_t; typedef unsigned char dbgbyte_t; /* Returns: mfn for the given (hvm guest) vaddr */ -static unsigned long +static mfn_t dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, unsigned long *gfn) { -unsigned long mfn; +mfn_t mfn; uint32_t pfec = PFEC_page_present; p2m_type_t gfntype; @@ -60,16 +60,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, return INVALID_MFN; } -mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); +mfn = get_gfn(dp, *gfn, &gfntype); if ( p2m_is_readonly(gfntype) && toaddr ) { DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype); mfn = INVALID_MFN; } else -DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn); +DBGP2("X: vaddr:%lx domid:%d mfn:%#"PRI_mfn"\n", + vaddr, dp->domain_id, mfn_x(mfn)); -if ( mfn == INVALID_MFN ) +if ( mfn_eq(mfn, INVALID_MFN) ) { put_gfn(dp, *gfn); *gfn = INVALID_GFN; @@ -91,7 +92,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, * mode. * Returns: mfn for the given (pv guest) vaddr */ -static unsigned long +static mfn_t dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val) { l4_pgentry_t l4e, *l4t; @@ -99,31 +100,31 @@ dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val) l2_pgentry_t l2e, *l2t; l1_pgentry_t l1e, *l1t; unsigned long cr3 = (pgd3val ? pgd3val : dp->vcpu
[Xen-devel] [PATCH v6 13/14] xen/arm: p2m: Use typesafe gfn for {max, lowest}_mapped_gfn
Signed-off-by: Julien Grall Acked-by: Stefano Stabellini --- Changes in v6: - Add Stefano's acked-by Changes in v4: - Patch added --- xen/arch/arm/mm.c | 2 +- xen/arch/arm/p2m.c| 18 +- xen/include/asm-arm/p2m.h | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index b5fc034..4e256c2 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1004,7 +1004,7 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type) unsigned long domain_get_maximum_gpfn(struct domain *d) { -return d->arch.p2m.max_mapped_gfn; +return gfn_x(d->arch.p2m.max_mapped_gfn); } void share_xen_page_with_guest(struct page_info *page, diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index b98eff4..c7f6766 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -976,7 +976,7 @@ static int apply_p2m_changes(struct domain *d, * This is set in preempt_count_limit. * */ -p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT; +p2m->lowest_mapped_gfn = _gfn(addr >> PAGE_SHIFT); rc = -ERESTART; goto out; @@ -1117,8 +1117,8 @@ static int apply_p2m_changes(struct domain *d, if ( op == INSERT ) { -p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn); -p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn); +p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, _gfn(egfn)); +p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, _gfn(sgfn)); } rc = 0; @@ -1383,8 +1383,8 @@ int p2m_init(struct domain *d) p2m->root = NULL; -p2m->max_mapped_gfn = 0; -p2m->lowest_mapped_gfn = ULONG_MAX; +p2m->max_mapped_gfn = _gfn(0); +p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); p2m->default_access = p2m_access_rwx; p2m->mem_access_enabled = false; @@ -1401,8 +1401,8 @@ int relinquish_p2m_mapping(struct domain *d) struct p2m_domain *p2m = &d->arch.p2m; return apply_p2m_changes(d, RELINQUISH, - pfn_to_paddr(p2m->lowest_mapped_gfn), - pfn_to_paddr(p2m->max_mapped_gfn), + pfn_to_paddr(gfn_x(p2m->lowest_mapped_gfn)), + pfn_to_paddr(gfn_x(p2m->max_mapped_gfn)), pfn_to_paddr(mfn_x(INVALID_MFN)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); @@ -1413,8 +1413,8 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr) struct p2m_domain *p2m = &d->arch.p2m; gfn_t end = gfn_add(start, nr); -start = gfn_max(start, _gfn(p2m->lowest_mapped_gfn)); -end = gfn_min(end, _gfn(p2m->max_mapped_gfn)); +start = gfn_max(start, p2m->lowest_mapped_gfn); +end = gfn_min(end, p2m->max_mapped_gfn); return apply_p2m_changes(d, CACHEFLUSH, pfn_to_paddr(gfn_x(start)), diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 6e258b9..34096bc 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -34,13 +34,13 @@ struct p2m_domain { /* Highest guest frame that's ever been mapped in the p2m * Only takes into account ram and foreign mapping */ -unsigned long max_mapped_gfn; +gfn_t max_mapped_gfn; /* Lowest mapped gfn in the p2m. When releasing mapped gfn's in a * preemptible manner this is update to track recall where to * resume the search. Apart from during teardown this can only * decrease. */ -unsigned long lowest_mapped_gfn; +gfn_t lowest_mapped_gfn; /* Gather some statistics for information purposes only */ struct { -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 08/14] xen/arm: dom0_build: Remove dead code in allocate_memory
The code to allocate memory when dom0 does not use direct mapping is relying on the presence of memory node in the DT. However, they are not present when booting using UEFI or when using ACPI. Rather than fixing the code, remove it because dom0 is always direct memory mapped and therefore the code is never tested. Also add a check to avoid disabling direct memory mapped and not implementing the associated RAM bank allocation. Signed-off-by: Julien Grall Acked-by: Stefano Stabellini --- Changes in v6: - Add Stefano's acked-by Changes in v4: - Patch added --- xen/arch/arm/domain_build.c | 58 ++--- 1 file changed, 7 insertions(+), 51 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 49185f0..923f48a 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -235,7 +235,7 @@ fail: * (as described above) we allow higher allocations and continue until * that runs out (or we have allocated sufficient dom0 memory). */ -static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) +static void allocate_memory(struct domain *d, struct kernel_info *kinfo) { const unsigned int min_low_order = get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); @@ -247,6 +247,12 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) bool_t lowmem = is_32bit_domain(d); unsigned int bits; +/* + * TODO: Implement memory bank allocation when DOM0 is not direct + * mapped + */ +BUG_ON(!dom0_11_mapping); + printk("Allocating 1:1 mappings totalling %ldMB for dom0:\n", /* Don't want format this as PRIpaddr (16 digit hex) */ (unsigned long)(kinfo->unassigned_mem >> 20)); @@ -343,56 +349,6 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) } } -static void allocate_memory(struct domain *d, struct kernel_info *kinfo) -{ - -struct dt_device_node *memory = NULL; -const void *reg; -u32 reg_len, reg_size; -unsigned int bank = 0; - -if ( dom0_11_mapping ) -return allocate_memory_11(d, kinfo); - -while ( (memory = dt_find_node_by_type(memory, "memory")) ) -{ -int l; - -dt_dprintk("memory node\n"); - -reg_size = dt_cells_to_size(dt_n_addr_cells(memory) + dt_n_size_cells(memory)); - -reg = dt_get_property(memory, "reg", ®_len); -if ( reg == NULL ) -panic("Memory node has no reg property"); - -for ( l = 0; - kinfo->unassigned_mem > 0 && l + reg_size <= reg_len - && kinfo->mem.nr_banks < NR_MEM_BANKS; - l += reg_size ) -{ -paddr_t start, size; - -if ( dt_device_get_address(memory, bank, &start, &size) ) -panic("Unable to retrieve the bank %u for %s", - bank, dt_node_full_name(memory)); - -if ( size > kinfo->unassigned_mem ) -size = kinfo->unassigned_mem; - -printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", - start, start + size); -if ( p2m_populate_ram(d, start, start + size) < 0 ) -panic("Failed to populate P2M"); -kinfo->mem.bank[kinfo->mem.nr_banks].start = start; -kinfo->mem.bank[kinfo->mem.nr_banks].size = size; -kinfo->mem.nr_banks++; - -kinfo->unassigned_mem -= size; -} -} -} - static int write_properties(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 07/14] xen/arm: map_regions_rw_cache: Map the region with p2m->default_access
The parameter 'access' is used by memaccess to restrict temporarily the permission. This parameter should not be used for other purpose (such as restricting permanently the permission). Instead, we should use the default access requested by memacess. When it is not enabled, the access will be p2m_access_rwx (i.e no restriction applied). The type p2m_mmio_direct will map the region read-write and non-executable before any further restriction by memaccess. Note that this is already the resulting permission with the curreent combination of the type and the access. So there is no functional change. Signed-off-by: Julien Grall --- Cc: Shannon Zhao This patch is a candidate for Xen 4.7. Currently this function is only used to map ACPI regions. I am wondering if we should introduce a new p2m type for it. And map this region RO (I am not sure why a guest would want to modify this region). Changes in v2: - Reword the commit message Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 1cfb62b..fcc4513 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1231,7 +1231,7 @@ int map_regions_rw_cache(struct domain *d, pfn_to_paddr(start_gfn + nr), pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_mmio_direct, - p2m_access_rw); + d->arch.p2m.default_access); } int unmap_regions_rw_cache(struct domain *d, @@ -1244,7 +1244,7 @@ int unmap_regions_rw_cache(struct domain *d, pfn_to_paddr(start_gfn + nr), pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid, - p2m_access_rw); + d->arch.p2m.default_access); } int map_mmio_regions(struct domain *d, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 06/14] xen/arm: Rework the interface of p2m_cache_flush and use typesafe gfn
p2m_cache_flush is expecting GFNs in parameter and not MFNs. Rename the variable to *gfn* and use typesafe to avoid possible misusage. Also, modify the prototype of the function to describe the range using the start and the number of GFNs. This will avoid to wonder whether the end if inclusive or exclusive. Note that the type of the parameters 'start' is changed from xen_pfn_t (aka uint64_t) to gfn_t (aka unsigned long). This means that a truncation will occur for ARM32. It is fine because it will always be encoded on 28 bits maximum (40 bits address). Signed-off-by: Julien Grall Acked-by: Stefano Stabellini --- Changes in v6: - Add Stefano's acked-by Changes in v4: - This patch was originally called "xen/arm: p2m_cache_flush: Use the correct terminology and typesafe gfn" - Describe the range using the start and the number of GFNs. Changes in v3: - Add a word in the commit message about the truncation. Changes in v2: - Drop _gfn suffix --- xen/arch/arm/domctl.c | 2 +- xen/arch/arm/p2m.c| 11 ++- xen/include/asm-arm/p2m.h | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c index 30453d8..f61f98a 100644 --- a/xen/arch/arm/domctl.c +++ b/xen/arch/arm/domctl.c @@ -30,7 +30,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d, if ( e < s ) return -EINVAL; -return p2m_cache_flush(d, s, e); +return p2m_cache_flush(d, _gfn(s), domctl->u.cacheflush.nr_pfns); } case XEN_DOMCTL_bind_pt_irq: { diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 54a363a..1cfb62b 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1469,16 +1469,17 @@ int relinquish_p2m_mapping(struct domain *d) d->arch.p2m.default_access); } -int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn) +int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr) { struct p2m_domain *p2m = &d->arch.p2m; +gfn_t end = gfn_add(start, nr); -start_mfn = MAX(start_mfn, p2m->lowest_mapped_gfn); -end_mfn = MIN(end_mfn, p2m->max_mapped_gfn); +start = gfn_max(start, _gfn(p2m->lowest_mapped_gfn)); +end = gfn_min(end, _gfn(p2m->max_mapped_gfn)); return apply_p2m_changes(d, CACHEFLUSH, - pfn_to_paddr(start_mfn), - pfn_to_paddr(end_mfn), + pfn_to_paddr(gfn_x(start)), + pfn_to_paddr(gfn_x(end)), pfn_to_paddr(mfn_x(INVALID_MFN)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index f204482..8a96e68 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -139,7 +139,7 @@ void p2m_dump_info(struct domain *d); mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t); /* Clean & invalidate caches corresponding to a region of guest address space */ -int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn); +int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); /* Setup p2m RAM mapping for domain d from start-end. */ int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 09/14] xen/arm: p2m: Remove unused operation ALLOCATE
The operation ALLOCATE is unused. If we ever need it, it could be reimplemented with INSERT. Signed-off-by: Julien Grall Acked-by: Stefano Stabellini --- Changes in v6: - Add Stefano's acked-by Changes in v4: - Patch added --- xen/arch/arm/p2m.c| 67 ++- xen/include/asm-arm/p2m.h | 3 --- 2 files changed, 2 insertions(+), 68 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index fcc4513..f11094e 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -547,7 +547,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn, enum p2m_operation { INSERT, -ALLOCATE, REMOVE, RELINQUISH, CACHEFLUSH, @@ -667,7 +666,6 @@ static int apply_one_level(struct domain *d, { const paddr_t level_size = level_sizes[level]; const paddr_t level_mask = level_masks[level]; -const paddr_t level_shift = level_shifts[level]; struct p2m_domain *p2m = &d->arch.p2m; lpae_t pte; @@ -678,58 +676,6 @@ static int apply_one_level(struct domain *d, switch ( op ) { -case ALLOCATE: -ASSERT(level < 3 || !p2m_valid(orig_pte)); -ASSERT(*maddr == 0); - -if ( p2m_valid(orig_pte) ) -return P2M_ONE_DESCEND; - -if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) && - /* We only create superpages when mem_access is not in use. */ - (level == 3 || (level < 3 && !p2m->mem_access_enabled)) ) -{ -struct page_info *page; - -page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0); -if ( page ) -{ -rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a); -if ( rc < 0 ) -{ -free_domheap_page(page); -return rc; -} - -pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a); -if ( level < 3 ) -pte.p2m.table = 0; -p2m_write_pte(entry, pte, flush_cache); -p2m->stats.mappings[level]++; - -*addr += level_size; - -return P2M_ONE_PROGRESS; -} -else if ( level == 3 ) -return -ENOMEM; -} - -/* L3 is always suitably aligned for mapping (handled, above) */ -BUG_ON(level == 3); - -/* - * If we get here then we failed to allocate a sufficiently - * large contiguous region for this level (which can't be - * L3) or mem_access is in use. Create a page table and - * continue to descend so we try smaller allocations. - */ -rc = p2m_create_table(d, entry, 0, flush_cache); -if ( rc < 0 ) -return rc; - -return P2M_ONE_DESCEND; - case INSERT: if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) && /* @@ -1169,7 +1115,7 @@ static int apply_p2m_changes(struct domain *d, } } -if ( op == ALLOCATE || op == INSERT ) +if ( op == INSERT ) { p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn); p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn); @@ -1197,7 +1143,7 @@ out: spin_unlock(&p2m->lock); -if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) && +if ( rc < 0 && ( op == INSERT ) && addr != start_gpaddr ) { BUG_ON(addr == end_gpaddr); @@ -1212,15 +1158,6 @@ out: return rc; } -int p2m_populate_ram(struct domain *d, - paddr_t start, - paddr_t end) -{ -return apply_p2m_changes(d, ALLOCATE, start, end, - 0, MATTR_MEM, 0, p2m_ram_rw, - d->arch.p2m.default_access); -} - int map_regions_rw_cache(struct domain *d, unsigned long start_gfn, unsigned long nr, diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 8a96e68..4752161 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -141,9 +141,6 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t); /* Clean & invalidate caches corresponding to a region of guest address space */ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); -/* Setup p2m RAM mapping for domain d from start-end. */ -int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); - int map_regions_rw_cache(struct domain *d, unsigned long start_gfn, unsigned long nr_mfns, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 03/14] xen: Use a typesafe to define INVALID_MFN
(CC Elena). On 06/07/16 14:01, Julien Grall wrote: Also take the opportunity to convert arch/x86/debug.c to the typesafe mfn and use proper printf format for MFN/GFN when the code around is modified. Signed-off-by: Julien Grall Reviewed-by: Andrew Cooper Acked-by: Stefano Stabellini --- Cc: Christoph Egger Cc: Liu Jinsong Cc: Jan Beulich Cc: Mukesh Rathor I forgot to update the CC list since GDSX maintainership was take over by Elena. Sorry for that. Cc: Paul Durrant Cc: Jun Nakajima Cc: Kevin Tian Cc: George Dunlap Cc: Tim Deegan Changes in v6: - Add Stefano's acked-by for ARM bits - Use PRI_mfn and PRI_gfn - Remove set of brackets when it is not necessary - Use mfn_add when possible - Add Andrew's reviewed-by Changes in v5: - Patch added --- xen/arch/arm/p2m.c | 4 +-- xen/arch/x86/cpu/mcheck/mce.c | 2 +- xen/arch/x86/debug.c| 58 + xen/arch/x86/hvm/hvm.c | 6 ++--- xen/arch/x86/hvm/viridian.c | 12 - xen/arch/x86/hvm/vmx/vmx.c | 2 +- xen/arch/x86/mm/guest_walk.c| 4 +-- xen/arch/x86/mm/hap/hap.c | 4 +-- xen/arch/x86/mm/p2m-ept.c | 6 ++--- xen/arch/x86/mm/p2m-pod.c | 18 ++--- xen/arch/x86/mm/p2m-pt.c| 18 ++--- xen/arch/x86/mm/p2m.c | 54 +++--- xen/arch/x86/mm/paging.c| 12 - xen/arch/x86/mm/shadow/common.c | 43 +++--- xen/arch/x86/mm/shadow/multi.c | 36 - xen/common/domain.c | 6 ++--- xen/common/grant_table.c| 6 ++--- xen/include/xen/mm.h| 2 +- 18 files changed, 147 insertions(+), 146 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 34563bb..d690602 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1461,7 +1461,7 @@ int relinquish_p2m_mapping(struct domain *d) return apply_p2m_changes(d, RELINQUISH, pfn_to_paddr(p2m->lowest_mapped_gfn), pfn_to_paddr(p2m->max_mapped_gfn), - pfn_to_paddr(INVALID_MFN), + pfn_to_paddr(mfn_x(INVALID_MFN)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); } @@ -1476,7 +1476,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn) return apply_p2m_changes(d, CACHEFLUSH, pfn_to_paddr(start_mfn), pfn_to_paddr(end_mfn), - pfn_to_paddr(INVALID_MFN), + pfn_to_paddr(mfn_x(INVALID_MFN)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); } diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c index edcbe48..2695b0c 100644 --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -1455,7 +1455,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) gfn = PFN_DOWN(gaddr); mfn = mfn_x(get_gfn(d, gfn, &t)); -if ( mfn == INVALID_MFN ) +if ( mfn == mfn_x(INVALID_MFN) ) { put_gfn(d, gfn); put_domain(d); diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c index 58cae22..9213ea7 100644 --- a/xen/arch/x86/debug.c +++ b/xen/arch/x86/debug.c @@ -43,11 +43,11 @@ typedef unsigned long dbgva_t; typedef unsigned char dbgbyte_t; /* Returns: mfn for the given (hvm guest) vaddr */ -static unsigned long +static mfn_t dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, unsigned long *gfn) { -unsigned long mfn; +mfn_t mfn; uint32_t pfec = PFEC_page_present; p2m_type_t gfntype; @@ -60,16 +60,17 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, return INVALID_MFN; } -mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); +mfn = get_gfn(dp, *gfn, &gfntype); if ( p2m_is_readonly(gfntype) && toaddr ) { DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype); mfn = INVALID_MFN; } else -DBGP2("X: vaddr:%lx domid:%d mfn:%lx\n", vaddr, dp->domain_id, mfn); +DBGP2("X: vaddr:%lx domid:%d mfn:%#"PRI_mfn"\n", + vaddr, dp->domain_id, mfn_x(mfn)); -if ( mfn == INVALID_MFN ) +if ( mfn_eq(mfn, INVALID_MFN) ) { put_gfn(dp, *gfn); *gfn = INVALID_GFN; @@ -91,7 +92,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, * mode. * Returns: mfn for the given (pv guest) vaddr */ -static unsigned long +static mfn_t dbg_pv_va2mfn(dbgva_t vaddr, struct domain *dp, uint64_t pgd3val) { l4_pgent
Re: [Xen-devel] [PATCH v6 04/14] xen: Use a typesafe to define INVALID_GFN
(CC Elena) On 06/07/16 14:01, Julien Grall wrote: Also take the opportunity to convert arch/x86/debug.c to the typesafe gfn. Signed-off-by: Julien Grall Reviewed-by: Andrew Cooper Acked-by: Stefano Stabellini --- Cc: Mukesh Rathor I forgot to update the CC list since GDSX maintainership was taken over by Elena. Sorry for that. Regards, Cc: Jan Beulich Cc: Paul Durrant Cc: Boris Ostrovsky Cc: Suravee Suthikulpanit Cc: Jun Nakajima Cc: Kevin Tian Cc: George Dunlap Cc: Tim Deegan Cc: Feng Wu Changes in v6: - Add Stefano's acked-by for ARM bits - Remove set of brackets when it is not necessary - Add Andrew's reviewed-by Changes in v5: - Patch added --- xen/arch/arm/p2m.c | 4 ++-- xen/arch/x86/debug.c| 18 +- xen/arch/x86/domain.c | 2 +- xen/arch/x86/hvm/emulate.c | 7 --- xen/arch/x86/hvm/hvm.c | 6 +++--- xen/arch/x86/hvm/ioreq.c| 8 xen/arch/x86/hvm/svm/nestedsvm.c| 2 +- xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- xen/arch/x86/mm/altp2m.c| 2 +- xen/arch/x86/mm/hap/guest_walk.c| 10 +- xen/arch/x86/mm/hap/nested_ept.c| 2 +- xen/arch/x86/mm/p2m-pod.c | 6 +++--- xen/arch/x86/mm/p2m.c | 18 +- xen/arch/x86/mm/shadow/common.c | 2 +- xen/arch/x86/mm/shadow/multi.c | 2 +- xen/arch/x86/mm/shadow/private.h| 2 +- xen/drivers/passthrough/amd/iommu_map.c | 2 +- xen/drivers/passthrough/vtd/iommu.c | 4 ++-- xen/drivers/passthrough/x86/iommu.c | 2 +- xen/include/asm-x86/guest_pt.h | 4 ++-- xen/include/asm-x86/p2m.h | 2 +- xen/include/xen/mm.h| 2 +- 22 files changed, 57 insertions(+), 56 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d690602..c938dde 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -479,7 +479,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, } /* If request to get default access. */ -if ( gfn_x(gfn) == INVALID_GFN ) +if ( gfn_eq(gfn, INVALID_GFN) ) { *access = memaccess[p2m->default_access]; return 0; @@ -1879,7 +1879,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, p2m->mem_access_enabled = true; /* If request to set default access. */ -if ( gfn_x(gfn) == INVALID_GFN ) +if ( gfn_eq(gfn, INVALID_GFN) ) { p2m->default_access = a; return 0; diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c index 9213ea7..3030022 100644 --- a/xen/arch/x86/debug.c +++ b/xen/arch/x86/debug.c @@ -44,8 +44,7 @@ typedef unsigned char dbgbyte_t; /* Returns: mfn for the given (hvm guest) vaddr */ static mfn_t -dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, -unsigned long *gfn) +dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, gfn_t *gfn) { mfn_t mfn; uint32_t pfec = PFEC_page_present; @@ -53,14 +52,14 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, DBGP2("vaddr:%lx domid:%d\n", vaddr, dp->domain_id); -*gfn = paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec); -if ( *gfn == INVALID_GFN ) +*gfn = _gfn(paging_gva_to_gfn(dp->vcpu[0], vaddr, &pfec)); +if ( gfn_eq(*gfn, INVALID_GFN) ) { DBGP2("kdb:bad gfn from gva_to_gfn\n"); return INVALID_MFN; } -mfn = get_gfn(dp, *gfn, &gfntype); +mfn = get_gfn(dp, gfn_x(*gfn), &gfntype); if ( p2m_is_readonly(gfntype) && toaddr ) { DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype); @@ -72,7 +71,7 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr, if ( mfn_eq(mfn, INVALID_MFN) ) { -put_gfn(dp, *gfn); +put_gfn(dp, gfn_x(*gfn)); *gfn = INVALID_GFN; } @@ -165,7 +164,8 @@ unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr, char *va; unsigned long addr = (unsigned long)gaddr; mfn_t mfn; -unsigned long gfn = INVALID_GFN, pagecnt; +gfn_t gfn = INVALID_GFN; +unsigned long pagecnt; pagecnt = min_t(long, PAGE_SIZE - (addr & ~PAGE_MASK), len); @@ -189,8 +189,8 @@ unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr, } unmap_domain_page(va); -if ( gfn != INVALID_GFN ) -put_gfn(dp, gfn); +if ( !gfn_eq(gfn, INVALID_GFN) ) +put_gfn(dp, gfn_x(gfn)); addr += pagecnt; buf += pagecnt; diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index bb59247..c8c7e2d 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -783,7 +783,7 @@ int arch_domain_soft_reset(struct domain *d) * gfn == INVALID_GF
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 06/07/16 02:34, Michael Turquette wrote: Hi! Hello Michael, Quoting Dirk Behme (2016-06-30 03:32:32) Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me). This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating. Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them. I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you". Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Wed, 6 Jul 2016, Julien Grall wrote: > On 06/07/16 02:34, Michael Turquette wrote: > > Hi! > > Hello Michael, > > > Quoting Dirk Behme (2016-06-30 03:32:32) > > > Some clocks might be used by the Xen hypervisor and not by the Linux > > > kernel. If these are not registered by the Linux kernel, they might be > > > disabled by clk_disable_unused() as the kernel doesn't know that they > > > are used. The clock of the serial console handled by Xen is one > > > example for this. It might be disabled by clk_disable_unused() which > > > stops the whole serial output, even from Xen, then. > > > > This whole thread had me confused until I realized that it all boiled > > down to some nomenclature issues (for me). > > > > This code does not _register_ any clocks. It simply gets them and > > enables them, which is what every other clk consumer in the Linux kernel > > does. More details below. > > > > > > > > Up to now, the workaround for this has been to use the Linux kernel > > > command line parameter 'clk_ignore_unused'. See Xen bug > > > > > > http://bugs.xenproject.org/xen/bug/45 > > > > clk_ignore_unused is a band-aid, not a proper medical solution. Setting > > that flag will not turn clocks on for you, nor will it guarantee that > > those clocks are never turned off in the future. It looks like you > > figured this out correctly in the patch below but it is worth repeating. > > > > Also the new CLK_IS_CRITICAL flag might be of interest to you, but that > > flag only exists as a way to enable clocks that must be enabled for the > > system to function (hence, "critical") AND when those same clocks do not > > have an accompanying Linux driver to consume them and enable them. > > I don't think we want the kernel to enable the clock for the hypervisor. We > want to tell the kernel "don't touch at all to this clock, it does not belong > to you". Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices. Is that correct? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] blktap2: Invalid logic detecting unaligned buffers in vhd_write_block
On Wed, Apr 6, 2016 at 5:51 PM, Ross Philipson wrote: > On 03/29/2016 07:26 AM, George Dunlap wrote: >> On Fri, Mar 25, 2016 at 4:34 PM, Ross Philipson >> wrote: >>> On 03/25/2016 12:11 PM, Wei Liu wrote: On Thu, Mar 17, 2016 at 06:10:59PM -0400, Ross Philipson wrote: > It seems the logic is meant to detect sector unaligned buffers for block > writes. The NOTing of the logic instead masks off any unaligned bits and > also would cause the function to always fail. It seems the function is not > used in any of the tools so that is probably why the problem is not seen. > In the vhd_read_block function it is correct. > > Signed-off-by: Ross Philipson This seems to fall under tools umbrella. I've look at blktap2 code, the reasoning is convincing to me so: Acked-by: Wei Liu But I've never used blktap2 and we don't normally test it so I would also wait a bit longer to see if anybody objects to this change. OOI if no code was using this, how did you discover this problem? >>> >>> We have an old fork of blktap2 from way back when. I was working to extract >>> our >>> changes and turn them into patches so we could rebase on upstream blktap2. >>> Someone long ago found that - I have no idea how but it was a valid fix so I >>> upstreamed it. >>> >>> There are a number of other ones that were found that are still in upstream >>> blktap2 - I plan to send them too. >> >> Ross, >> >> Instead of cross-porting fixes from XenServer's blktap3 to the >> bitrotting blktap2, would you consider taking up my work on replacing >> blktap2 with building blktap3 as an external project? > > George, a few questions below to get started... > >> >> The basic shape of things that need to happen for that are: >> >> 1. Allow qemu to provide emulated devices for disks which use hotplug >> scripts (just checked in last week) >> >> 2. Do the tweaks necessary to allow blktap3 to be built as an >> independent project (outside the XenServer build system) > > I am not sure where the latest and greatest blktap3 work can be found. I have > found some old repos that have activity up until the work stopped on that > project in 2013. Can you give me some pointers on this? > > Also it seems the overall idea is to not have blktap3 (or any blktap) live in > the Xen tree but rather be external. Where would blktap3 live? > >> >> 3. Switch libxl to use the already-in-tree block-tap script (which >> calls tapctl) rather than linking against the blktap library. > > I believe I found this work: > > http://lists.xenproject.org/archives/html/xen-devel/2015-07/msg00805.html > > It looks like 4 of the set made it in and patches 3 & 5 still need work? I can > re-base them. Hey Ross, Sorry for dropping this on the floor -- are you still interested in this work? If so I'll take some time to summarize where I got to. :-) -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen: arm64: Add support for Renesas RCar Gen3 H3 Salvator-X platform
Hi Dirk, On 06/07/16 07:33, Dirk Behme wrote: Could you share the U-Boot commands how you load and esp. start Xen? For loading you use TFTP? How do you start Xen with U-Boot, then? I think we have to pass the device tree address in x0 and the Linux kernel image address in x2. How do you do this with an U-Boot command? U-boot can load Xen from TFTP or from the SD-card. This is the same as booting a baremetal kernel with U-boot. There is a section on the wiki page to explain how to create the device-tree node for the boot modules [1] and the allwinner page [2] gives a full example how to boot Xen with U-boot via tftp (Note that it could easily be adapted to load from the SD-Card). Regards, [1] http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Boot_Modules [2] http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/Allwinner#Boot_script -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 06/07/16 14:16, Stefano Stabellini wrote: On Wed, 6 Jul 2016, Julien Grall wrote: On 06/07/16 02:34, Michael Turquette wrote: Hi! Hello Michael, Quoting Dirk Behme (2016-06-30 03:32:32) Some clocks might be used by the Xen hypervisor and not by the Linux kernel. If these are not registered by the Linux kernel, they might be disabled by clk_disable_unused() as the kernel doesn't know that they are used. The clock of the serial console handled by Xen is one example for this. It might be disabled by clk_disable_unused() which stops the whole serial output, even from Xen, then. This whole thread had me confused until I realized that it all boiled down to some nomenclature issues (for me). This code does not _register_ any clocks. It simply gets them and enables them, which is what every other clk consumer in the Linux kernel does. More details below. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 clk_ignore_unused is a band-aid, not a proper medical solution. Setting that flag will not turn clocks on for you, nor will it guarantee that those clocks are never turned off in the future. It looks like you figured this out correctly in the patch below but it is worth repeating. Also the new CLK_IS_CRITICAL flag might be of interest to you, but that flag only exists as a way to enable clocks that must be enabled for the system to function (hence, "critical") AND when those same clocks do not have an accompanying Linux driver to consume them and enable them. I don't think we want the kernel to enable the clock for the hypervisor. We want to tell the kernel "don't touch at all to this clock, it does not belong to you". Right, and that's why I was suggesting that another way to do this would be to set the "status" to "disabled" in Xen: so that Linux would leave the clock alone. But in that case Linux would not be happy to see disabled clocks which are actually supposed to be used by some devices. Is that correct? The problem is not "whether Linux would be happy or not", but the meaning of this property. Based on the ePAPR, "status = disabled" indicates that the device is not presently operational, but it might become operational later (for example, something is not plugged in, or switched off). An operating system which read this property should interpret as "the clock should not be used". However, with your suggestion the OS would need to differentiate between "the clock may be used by the hypervisor" and "the clock is not wired up/present/...". Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [ovmf test] 96691: regressions - FAIL
flight 96691 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/96691/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. vs. 94748 test-amd64-i386-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. vs. 94748 version targeted for testing: ovmf e58f1ae5c9689200865cf4fdd7d6e8a9d28e3cf3 baseline version: ovmf dc99315b8732b6e3032d01319d3f534d440b43d0 Last test of basis94748 2016-05-24 22:43:25 Z 42 days Failing since 94750 2016-05-25 03:43:08 Z 42 days 93 attempts Testing same since96675 2016-07-05 13:25:43 Z1 days2 attempts People who touched revisions under test: Anandakrishnan Loganathan Ard Biesheuvel Bret Barkelew Bruce Cran Bruce Cran Chao Zhang Cinnamon Shia Cohen, Eugene Dandan Bi Darbin Reyes david wei Eric Dong Eugene Cohen Evan Lloyd Evgeny Yakovlev Feng Tian Fu Siyuan Fu, Siyuan Gary Li Gary Lin Giri P Mudusuru Hao Wu Hegde Nagaraj P hegdenag Heyi Guo Jan D?bro? Jan Dabros Jeff Fan Jiaxin Wu Jiewen Yao Joe Zhou Jordan Justen Katie Dellaquila Laszlo Ersek Liming Gao Lu, ShifeiX A lushifex Marcin Wojtas Mark Rutland Marvin H?user Marvin Haeuser Maurice Ma Michael Zimmermann Qiu Shumin Ruiyu Ni Ryan Harkin Sami Mujawar Satya Yarlagadda Shannon Zhao Sriram Subramanian Star Zeng Sunny Wang Tapan Shah Thomas Palmer Yarlagadda, Satya P Yonghong Zhu Zhang Lubo Zhang, Chao B jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 8275 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 02/18] arm/altp2m: Add first altp2m HVMOP stubs.
Hello Sergej, On 06/07/16 10:14, Sergej Proskurin wrote: On 07/05/2016 12:19 PM, Julien Grall wrote: Hello Sergej, On 04/07/16 12:45, Sergej Proskurin wrote: +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) +{ +struct xen_hvm_altp2m_op a; +struct domain *d = NULL; +int rc = 0; + +if ( !hvm_altp2m_supported() ) +return -EOPNOTSUPP; + +if ( copy_from_guest(&a, arg, 1) ) +return -EFAULT; + +if ( a.pad1 || a.pad2 || + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || + (a.cmd < HVMOP_altp2m_get_domain_state) || + (a.cmd > HVMOP_altp2m_change_gfn) ) +return -EINVAL; + +d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? +rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain(); + +if ( d == NULL ) +return -ESRCH; + +if ( (a.cmd != HVMOP_altp2m_get_domain_state) && + (a.cmd != HVMOP_altp2m_set_domain_state) && + !d->arch.altp2m_active ) +{ +rc = -EOPNOTSUPP; +goto out; +} + +if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) ) +goto out; I think this is the best place to ask a couple of questions related to who can access altp2m. Based on this call, a guest is allowed to manage its own altp2m. Can you explain why we would want a guest to do that? On x86, altp2m might be used by the guest in the #VE (Virtualization Exception). On ARM, there is indeed not necessary for a guest to access altp2m. Could you provide me with information, how to best restrict non-privileged guests (not only dom0) from accessing these HVMOPs? Can thisbedone by means of xsm? Thank you. This does not looks safe for both x86 and ARM. From my understanding a malware would be able to modify an altp2m, switching between 2 view... which would lead to remove the entire purpose of altp2m. When XSM is not enabled (this is the default on Xen), XSM_TARGET allows the guest (see xsm_default_action) to call the operations. So I am not convince XSM is the right way to go. Also, I have noticed that a guest is allowed to disable ALTP2M on ARM because it set any param (x86 has some restriction on it). Similarly, the ALTP2M parameter can be set multiple time. Same here. Give a look how x86 restrict the write to HVMOP_set_param. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] xenstored memory leak
On 06/07/16 08:31, Juergen Gross wrote: > While testing some patches for support of ballooning in Mini-OS by using > the xenstore domain I realized that each xl create/destroy pair would > increase memory consumption in Mini-OS by about 5kB. Wondering whether > this is a xenstore domain only effect I did the same test with xenstored > and oxenstored daemons. > > xenstored showed the same behavior, the "referenced" size showed by the > pmap command grew by about 5kB for each create/destroy pair. > > oxenstored seemed to be even worse in the beginning (about 6kB for each > pair), but after about 100 create/destroys the value seemed to be > rather stable. Do you mean that after a while, you see oxenstored not leaking any further memory, even with new domains being created? Ocaml is a garbage collected languague, so you would expect the process to get larger until the GC decides to kick in. > > Did anyone notice this memory leak before? We have not encountered this in XenServer stress scenarios. (It is entirely possible that this specific to something xl does which Xapi doesn't.) ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On Wed, Jul 06, 2016 at 02:16:18PM +0100, Stefano Stabellini wrote: > On Wed, 6 Jul 2016, Julien Grall wrote: > > On 06/07/16 02:34, Michael Turquette wrote: > > > Hi! > > > > Hello Michael, > > > > > Quoting Dirk Behme (2016-06-30 03:32:32) > > > > Some clocks might be used by the Xen hypervisor and not by the Linux > > > > kernel. If these are not registered by the Linux kernel, they might be > > > > disabled by clk_disable_unused() as the kernel doesn't know that they > > > > are used. The clock of the serial console handled by Xen is one > > > > example for this. It might be disabled by clk_disable_unused() which > > > > stops the whole serial output, even from Xen, then. > > > > > > This whole thread had me confused until I realized that it all boiled > > > down to some nomenclature issues (for me). > > > > > > This code does not _register_ any clocks. It simply gets them and > > > enables them, which is what every other clk consumer in the Linux kernel > > > does. More details below. > > > > > > > > > > > Up to now, the workaround for this has been to use the Linux kernel > > > > command line parameter 'clk_ignore_unused'. See Xen bug > > > > > > > > http://bugs.xenproject.org/xen/bug/45 > > > > > > clk_ignore_unused is a band-aid, not a proper medical solution. Setting > > > that flag will not turn clocks on for you, nor will it guarantee that > > > those clocks are never turned off in the future. It looks like you > > > figured this out correctly in the patch below but it is worth repeating. > > > > > > Also the new CLK_IS_CRITICAL flag might be of interest to you, but that > > > flag only exists as a way to enable clocks that must be enabled for the > > > system to function (hence, "critical") AND when those same clocks do not > > > have an accompanying Linux driver to consume them and enable them. > > > > I don't think we want the kernel to enable the clock for the hypervisor. We > > want to tell the kernel "don't touch at all to this clock, it does not > > belong > > to you". > > Right, and that's why I was suggesting that another way to do this would > be to set the "status" to "disabled" in Xen: so that Linux would leave > the clock alone. But in that case Linux would not be happy to see > disabled clocks which are actually supposed to be used by some devices. If you were to do that, that would cover the entire clock-controller, not necessarily for the individual clock line (as this does not necessarily have a node of its own). So you'd prevent the use of other clocks owned by that controller. That's also not sufficient, as you'd have to do the same for resources required to keep that clock active (parent clocks from different controllers, regulators, GPIOs, etc). I don't think that will work other than in very basic cases. Thanks, Mark. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] xenstored memory leak
On 06/07/16 15:48, Andrew Cooper wrote: > On 06/07/16 08:31, Juergen Gross wrote: >> While testing some patches for support of ballooning in Mini-OS by using >> the xenstore domain I realized that each xl create/destroy pair would >> increase memory consumption in Mini-OS by about 5kB. Wondering whether >> this is a xenstore domain only effect I did the same test with xenstored >> and oxenstored daemons. >> >> xenstored showed the same behavior, the "referenced" size showed by the >> pmap command grew by about 5kB for each create/destroy pair. >> >> oxenstored seemed to be even worse in the beginning (about 6kB for each >> pair), but after about 100 create/destroys the value seemed to be >> rather stable. > > Do you mean that after a while, you see oxenstored not leaking any > further memory, even with new domains being created? In my test: yes. I did: while true do xl create minios.xl sleep 3 xl shutdown minios sleep 2 done After about 200 iterations memory usage with oxenstored was stable. I stopped the loop after more than 1000 iterations. > Ocaml is a garbage collected languague, so you would expect the process > to get larger until the GC decides to kick in. Okay. This explains the pattern. >> Did anyone notice this memory leak before? > > We have not encountered this in XenServer stress scenarios. You are using oxenstored, right? The real leak is in xenstored only. > (It is entirely possible that this specific to something xl does which > Xapi doesn't.) I doubt that. I'm seeing the leak with the C-variant of xenstore, both as daemon and as stubdom. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] xenstored memory leak
On 06/07/16 14:55, Juergen Gross wrote: > On 06/07/16 15:48, Andrew Cooper wrote: >> On 06/07/16 08:31, Juergen Gross wrote: >>> While testing some patches for support of ballooning in Mini-OS by using >>> the xenstore domain I realized that each xl create/destroy pair would >>> increase memory consumption in Mini-OS by about 5kB. Wondering whether >>> this is a xenstore domain only effect I did the same test with xenstored >>> and oxenstored daemons. >>> >>> xenstored showed the same behavior, the "referenced" size showed by the >>> pmap command grew by about 5kB for each create/destroy pair. >>> >>> oxenstored seemed to be even worse in the beginning (about 6kB for each >>> pair), but after about 100 create/destroys the value seemed to be >>> rather stable. >> Do you mean that after a while, you see oxenstored not leaking any >> further memory, even with new domains being created? > In my test: yes. I did: > > while true > do > xl create minios.xl > sleep 3 > xl shutdown minios > sleep 2 > done > > After about 200 iterations memory usage with oxenstored was stable. I > stopped the loop after more than 1000 iterations. > >> Ocaml is a garbage collected languague, so you would expect the process >> to get larger until the GC decides to kick in. > Okay. This explains the pattern. > >>> Did anyone notice this memory leak before? >> We have not encountered this in XenServer stress scenarios. > You are using oxenstored, right? The real leak is in xenstored only. Correct. > >> (It is entirely possible that this specific to something xl does which >> Xapi doesn't.) > I doubt that. I'm seeing the leak with the C-variant of xenstore, both > as daemon and as stubdom. Right, and we haven't used C xenstored in the last decade. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen: arm64: Add support for Renesas RCar Gen3 H3 Salvator-X platform
On 06.07.2016 15:17, Julien Grall wrote: Hi Dirk, On 06/07/16 07:33, Dirk Behme wrote: Could you share the U-Boot commands how you load and esp. start Xen? For loading you use TFTP? How do you start Xen with U-Boot, then? I think we have to pass the device tree address in x0 and the Linux kernel image address in x2. How do you do this with an U-Boot command? U-boot can load Xen from TFTP or from the SD-card. This is the same as booting a baremetal kernel with U-boot. There is a section on the wiki page to explain how to create the device-tree node for the boot modules [1] and the allwinner page [2] gives a full example how to boot Xen with U-boot via tftp (Note that it could easily be adapted to load from the SD-Card). Regards, [1] http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Boot_Modules [2] http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/Allwinner#Boot_script Hmm, sorry, I still seem to miss anything :( I've loaded xen/xen with U-Boot to 0x4A00 and then try to start it as described in [2] above: => bootz 0x4A00 - 0x4800 Bad Linux ARM zImage magic! I'm not sure why xen/xen is assumed to be a zImage? To verify that the address is correct, I can use U-Boot's go command (which then will fail, later, as I can't pass the device tree address): => go 0x4A00 ## Starting application at 0x4A00 ... - UART enabled - - CPU booting - - Current EL 0008 - - Xen starting at EL2 - - Zero BSS - - Setting up control registers - - Turning on paging - - Ready - (XEN) (XEN) (XEN) Panic on CPU 0: (XEN) No valid device tree (XEN) (XEN) (XEN) (XEN) Reboot in five seconds... Best regards Dirk ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen: arm64: Add support for Renesas RCar Gen3 H3 Salvator-X platform
On 06/07/16 15:03, Dirk Behme wrote: On 06.07.2016 15:17, Julien Grall wrote: Hi Dirk, On 06/07/16 07:33, Dirk Behme wrote: Could you share the U-Boot commands how you load and esp. start Xen? For loading you use TFTP? How do you start Xen with U-Boot, then? I think we have to pass the device tree address in x0 and the Linux kernel image address in x2. How do you do this with an U-Boot command? U-boot can load Xen from TFTP or from the SD-card. This is the same as booting a baremetal kernel with U-boot. There is a section on the wiki page to explain how to create the device-tree node for the boot modules [1] and the allwinner page [2] gives a full example how to boot Xen with U-boot via tftp (Note that it could easily be adapted to load from the SD-Card). Regards, [1] http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Boot_Modules [2] http://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/Allwinner#Boot_script Hmm, sorry, I still seem to miss anything :( I've loaded xen/xen with U-Boot to 0x4A00 and then try to start it as described in [2] above: => bootz 0x4A00 - 0x4800 Bad Linux ARM zImage magic! I'm not sure why xen/xen is assumed to be a zImage? Oh, sorry I meant to say that bootz should be replaced by booti but forgot to write it down. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] xenbits "official" repo for XTF (was Re: [PATCH 0/2] xtf: add launcher (+1 bugfix)
> Looking at the above, it occurs to me that, this whole area seems to be a > little inconsistent anyway and could do with a little house-keeping. We > have > - osstest.git > - there also is osstest/*.git which seems to be odd and seems to have been > inactive for a while (not very clear to me what these do) > - and we have old and inactive xentesttools/*.git They are not inactive. I have some patches for it and we are using it (Boris and me). > - and we are adding a new repo for XTF > > > Maybe, moving everything test related under testing/* or test/* would be > sensible, but that would cause some disruption (not sure how bad that > would be). It would address A, B and D. It wouldn't make C much worse than > now. I don't mind the xentesttools going under a 'test' directory. > > We already follow a similar pattern for out-of-tree via pvdrivers/* > > It does in fact occur to me that some of the older inactive repos should > be archived somehow: candidates seem to be kemari/*, xentesttools/*, > xenclient/*, xcp/*, ... pollute the namespace. If we are concerned about > the namespace, we should address this at some point. There are also some > inactive top level git repos such as linux-2.5-xen.git and quite a few > inactive people repos. The Linux ones, the linux-pvops.git could go. > > >Lars, can you please advise what process we need to use to come to > >closure on this decision ? > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Kernel panic on Xen virtualisation in Debian
On Mon, Jul 4, 2016 at 7:06 PM, Jan Prunk wrote: > Hello ! > > I am posting Xen virtualisation bug links to this e-mail address, > because I wasn't able to find the Xen specific bugtracker list. > This bug has been discovered in 2015 and so far it hasn't been > resolved through the Debian/Kernel bug lists. I submit the > links to bug reports for you. > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=804079 The serial log at the bottom looks like there was a crash in the ipv6 handling as the result of a packet delivery, perhaps? David / Wei, do you have any ideas? Not sure who else has worked on the netback side of things. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 15/18] arm/altp2m: Add altp2m paging mechanism.
On 06/07/16 09:33, Sergej Proskurin wrote: On 07/04/2016 10:53 PM, Julien Grall wrote: Can we introduce a function which return the xma, mfn, order, attribute at once? It will avoid to browse the p2m 3 times which is really expensive on ARMv7 because the p2m is not mapped in the virtual address space of Xen. I was already thinking of at least merging p2m_get_gfn_level_and_attr with __p2m_lookup. But it would also make sense to introduce an entirely new function, which does just that.I believe increasing the overhead of __p2m_lookup would not be a good solution. Thank you. How the overhead of __p2m_lookup will be increased? level, mattr and type is already available in p2m_lookup. For the mem access, as you pass a pointer you can retrieve the access only when the pointer is not NULL. +spin_unlock(&hp2m->lock); + +mask = level_masks[level]; + +rc = apply_p2m_changes(d, *ap2m, INSERT, + pfn_to_paddr(gfn_x(gfn)) & mask, + (pfn_to_paddr(gfn_x(gfn)) + level_sizes[level]) & mask, + maddr & mask, mattr, 0, p2mt, + memaccess[xma]); The page associated to the MFN is not locked, so another thread could decide to remove the page from the domain and then the altp2m would contain an entry to something that does not belong to the domain anymore. Note that x86 is doing the same. So I am not sure why it is considered safe there... If I understand you correctly, unlocking the hp2m->lock after calling apply_p2m_changeswould already solve this issue, right? Thanks. That would solve it. However, you will add a lot of contention on hp2m when each vCPU is using a different empty view. +if ( rc ) +{ +gdprintk(XENLOG_ERR, "failed to set entry for %lx -> %lx p2m %lx\n", +(unsigned long)pfn_to_paddr(gfn_x(gfn)), (unsigned long)(maddr), (unsigned long)*ap2m); +domain_crash(hp2m->domain); +} + +return 1; + +out: +spin_unlock(&hp2m->lock); +return 0; +} + static void p2m_init_altp2m_helper(struct domain *d, unsigned int i) { struct p2m_domain *p2m = d->arch.altp2m_p2m[i]; [...] @@ -2429,6 +2460,8 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, const union hsr hsr) { const struct hsr_dabt dabt = hsr.dabt; +struct vcpu *v = current; +struct p2m_domain *p2m = NULL; int rc; mmio_info_t info; @@ -2449,6 +2482,12 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, info.gpa = get_faulting_ipa(); else { +/* + * When using altp2m, this flush is required to get rid of old TLB + * entries and use the new, lazily copied, ap2m entries. + */ +flush_tlb_local(); Can you give more details why this flush is required? Without the flush, the guest crashed to an unresolved data abort. To be more precise, after the first lazy-copy of a mapping from the hostp2m to the currently active altp2m view, the system crashed because it was not able to find the new mapping in its altp2m table. The explicit flush solved this issue quite nicely. This explicit flush likely means there is another issue in the code. This flush should not be necessary. Can you give more details about the exact data abort problem? Is it because the translation VA -> IPA is failing? As I answer your question, I am starting to think that the crash was a result of of a lack of a memory barrier because even with the old (hostp2m's) TLBs present, the translation would not present an issue (the mapping would be the same as the p2m entry is simply copied from the hostp2m to the active altp2m view). Also, the guest would reuse the old TLBs, the system would have used the access rights of the hostp2m, and hence again be trapped by Xen. I am sorry but I don't understand what you are trying to say. I will try to solve this issue by means of a memory barrier, thank you. Can you details where you think there is a lack of memory barrier? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 11/18] arm/altp2m: Make flush_tlb_domain ready for altp2m.
On 05/07/16 21:21, Sergej Proskurin wrote: I agree on this point as well. However, we should maybe think of another name for flush_tlb_domain. Especially, if we do not flush the entire domain (including all currently active altp2m views). What do you think? This function is only used within p2m.c, although it is exported. What about renaming this function to flush_tlb_p2m and instead pass the p2m in parameter? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 14/18] arm/altp2m: Add HVMOP_altp2m_set_mem_access.
On 05/07/16 22:55, Sergej Proskurin wrote: Hello Julien, On 07/05/2016 02:49 PM, Julien Grall wrote: Hello Sergej, On 04/07/16 12:45, Sergej Proskurin wrote: +static inline +int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, + struct p2m_domain *ap2m, p2m_access_t a, + gfn_t gfn) +{ [...] +/* Set mem access attributes - currently supporting only one (4K) page. */ +mask = level_masks[3]; +return apply_p2m_changes(d, ap2m, INSERT, + gpa & mask, + (gpa + level_sizes[level]) & mask, + maddr & mask, mattr, 0, p2mt, a); The operation INSERT will remove the previous mapping and decrease page reference for foreign mapping (see p2m_put_l3_page). So if you set the memory access for this kind of page, the reference count will be wrong afterward. I see your point. As far as I understand, the purpose of the function p2m_put_l3_page to simply decrement the ref count of the particular pte and free the page if its' ref count reaches zero. Since p2m_put_l3_page is called only twice in p2m.c, we could insert a check (p2m_is_hostp2m/altp2m) making sure that the ref count is decremented only if the p2m in question is the hostp2m. This will make sure that the ref count is maintained correctly. Why don't you use the operation MEMACCESS? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 11/18] arm/altp2m: Make flush_tlb_domain ready for altp2m.
Hi Julien, On 07/06/2016 04:28 PM, Julien Grall wrote: > > > On 05/07/16 21:21, Sergej Proskurin wrote: >> I agree on this point as well. However, we should maybe think of another >> name for flush_tlb_domain. Especially, if we do not flush the entire >> domain (including all currently active altp2m views). What do you think? > > This function is only used within p2m.c, although it is exported. What > about renaming this function to flush_tlb_p2m and instead pass the p2m > in parameter? > > Regards, > That sounds good to me :) I will do that. Thank you. Cheers, ~Sergej ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 01/20] hvmloader: Provide hvmloader_acpi_build_tables()
On Tue, Jul 05, 2016 at 03:05:00PM -0400, Boris Ostrovsky wrote: > In preparation for moving out ACPI builder make all > BIOSes call hvmloader_acpi_build_tables() instead of > calling ACPI code directly. > > No functional changes. > > Signed-off-by: Boris Ostrovsky Reviewed-by: Konrad Rzeszutek Wilk > --- > > Changes in v1: > * Added last sentence to commit message > > tools/firmware/hvmloader/ovmf.c|2 +- > tools/firmware/hvmloader/rombios.c |2 +- > tools/firmware/hvmloader/seabios.c |2 +- > tools/firmware/hvmloader/util.c|7 +++ > tools/firmware/hvmloader/util.h|4 > 5 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c > index db9fa7a..74fec56 100644 > --- a/tools/firmware/hvmloader/ovmf.c > +++ b/tools/firmware/hvmloader/ovmf.c > @@ -125,7 +125,7 @@ static void ovmf_acpi_build_tables(void) > .dsdt_15cpu_len = 0 > }; > > -acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS); > +hvmloader_acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS); > } > > static void ovmf_create_smbios_tables(void) > diff --git a/tools/firmware/hvmloader/rombios.c > b/tools/firmware/hvmloader/rombios.c > index 1f15b94..1f8fc2d 100644 > --- a/tools/firmware/hvmloader/rombios.c > +++ b/tools/firmware/hvmloader/rombios.c > @@ -180,7 +180,7 @@ static void rombios_acpi_build_tables(void) > .dsdt_15cpu_len = dsdt_15cpu_len, > }; > > -acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS); > +hvmloader_acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS); > } > > static void rombios_create_mp_tables(void) > diff --git a/tools/firmware/hvmloader/seabios.c > b/tools/firmware/hvmloader/seabios.c > index c6b3d9f..9f66a79 100644 > --- a/tools/firmware/hvmloader/seabios.c > +++ b/tools/firmware/hvmloader/seabios.c > @@ -100,7 +100,7 @@ static void seabios_acpi_build_tables(void) > .dsdt_15cpu_len = 0, > }; > > -acpi_build_tables(&config, rsdp); > +hvmloader_acpi_build_tables(&config, rsdp); > add_table(rsdp); > } > > diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c > index 9382709..9af29b1 100644 > --- a/tools/firmware/hvmloader/util.c > +++ b/tools/firmware/hvmloader/util.c > @@ -21,6 +21,7 @@ > #include "config.h" > #include "hypercall.h" > #include "ctype.h" > +#include "acpi/acpi2_0.h" > #include > #include > #include > @@ -856,6 +857,12 @@ int hpet_exists(unsigned long hpet_base) > return ((hpet_id >> 16) == 0x8086); > } > > +void hvmloader_acpi_build_tables(struct acpi_config *config, > + unsigned int physical) > +{ > +acpi_build_tables(config, physical); > +} > + > /* > * Local variables: > * mode: C > diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h > index 3126817..b226df4 100644 > --- a/tools/firmware/hvmloader/util.h > +++ b/tools/firmware/hvmloader/util.h > @@ -273,6 +273,10 @@ extern struct e820map memory_map; > bool check_overlap(uint64_t start, uint64_t size, > uint64_t reserved_start, uint64_t reserved_size); > > +struct acpi_config; > +void hvmloader_acpi_build_tables(struct acpi_config *config, > + unsigned int physical); > + > #endif /* __HVMLOADER_UTIL_H__ */ > > /* > -- > 1.7.1 > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 3/4] libxl: refactor domcreate_attach_dtdev() to use device type framework
Signed-off-by: Juergen Gross --- tools/libxl/libxl_create.c | 68 +- 1 file changed, 31 insertions(+), 37 deletions(-) diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 354c73f..828f254 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -742,9 +742,6 @@ static void domcreate_bootloader_done(libxl__egc *egc, static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs, int ret); -static void domcreate_attach_dtdev(libxl__egc *egc, libxl__multidev *multidev, - int ret); - static void domcreate_console_available(libxl__egc *egc, libxl__domain_create_state *dcs); @@ -1399,12 +1396,39 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, domcreate_complete(egc, dcs, ret); } +static void libxl__add_dtdevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid, + libxl_domain_config *d_config, + libxl__multidev *multidev) +{ +AO_GC; +libxl__ao_device *aodev = libxl__multidev_prepare(multidev); +int i, rc = 0; + +for (i = 0; i < d_config->num_dtdevs; i++) { +const libxl_device_dtdev *dtdev = &d_config->dtdevs[i]; + +LOG(DEBUG, "Assign device \"%s\" to dom%u", dtdev->path, domid); +rc = xc_assign_dt_device(CTX->xch, domid, dtdev->path); +if (rc < 0) { +LOG(ERROR, "xc_assign_dtdevice failed: %d", rc); +goto out; +} +} + +out: +aodev->rc = rc; +aodev->callback(egc, aodev); +} + +static DEFINE_DEVICE_TYPE_STRUCT(dtdev); + static const struct libxl_device_type *device_type_tbl[] = { &libxl__nic_devtype, &libxl__vtpm_devtype, &libxl__usbctrl_devtype, &libxl__usbdev_devtype, &libxl__pcidev_devtype, +&libxl__dtdev_devtype, }; static void domcreate_attach_devices(libxl__egc *egc, @@ -1439,7 +1463,10 @@ static void domcreate_attach_devices(libxl__egc *egc, return; } -domcreate_attach_dtdev(egc, multidev, 0); +domcreate_console_available(egc, dcs); + +domcreate_complete(egc, dcs, 0); + return; error_out: @@ -1479,39 +1506,6 @@ error_out: domcreate_complete(egc, dcs, ret); } -static void domcreate_attach_dtdev(libxl__egc *egc, - libxl__multidev *multidev, - int ret) -{ -libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev); -STATE_AO_GC(dcs->ao); -int i; -int domid = dcs->guest_domid; - -/* convenience aliases */ -libxl_domain_config *const d_config = dcs->guest_config; - -for (i = 0; i < d_config->num_dtdevs; i++) { -const libxl_device_dtdev *dtdev = &d_config->dtdevs[i]; - -LOG(DEBUG, "Assign device \"%s\" to dom%u", dtdev->path, domid); -ret = xc_assign_dt_device(CTX->xch, domid, dtdev->path); -if (ret < 0) { -LOG(ERROR, "xc_assign_dtdevice failed: %d", ret); -goto error_out; -} -} - -domcreate_console_available(egc, dcs); - -domcreate_complete(egc, dcs, 0); -return; - -error_out: -assert(ret); -domcreate_complete(egc, dcs, ret); -} - static void domcreate_complete(libxl__egc *egc, libxl__domain_create_state *dcs, int rc) -- 2.6.6 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 1/4] libxl: add framework for device types
Instead of duplicate coding for each device type (vtpms, usbctrls, ...) especially on domain creation introduce a framework for that purpose. Signed-off-by: Juergen Gross --- V2: - add macro to fill struct libxl__device_type as suggested by Ian Jackson - make struct libxl__device_type variables const as requested by Ian Jackson --- tools/libxl/libxl.c | 3 + tools/libxl/libxl_create.c | 163 +-- tools/libxl/libxl_internal.h | 20 ++ tools/libxl/libxl_pvusb.c| 4 ++ 4 files changed, 76 insertions(+), 114 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 1c81239..b3deef0 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -7434,6 +7434,9 @@ out: return rc; } +DEFINE_DEVICE_TYPE_STRUCT(nic); +DEFINE_DEVICE_TYPE_STRUCT(vtpm); + /* * Local variables: * mode: C diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 1b99472..5e05f6f 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -742,12 +742,6 @@ static void domcreate_bootloader_done(libxl__egc *egc, static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs, int ret); -static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *multidev, - int ret); -static void domcreate_attach_usbctrls(libxl__egc *egc, - libxl__multidev *multidev, int ret); -static void domcreate_attach_usbdevs(libxl__egc *egc, libxl__multidev *multidev, - int ret); static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs, int ret); static void domcreate_attach_dtdev(libxl__egc *egc, @@ -1407,6 +1401,53 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, domcreate_complete(egc, dcs, ret); } +static const struct libxl_device_type *device_type_tbl[] = { +&libxl__nic_devtype, +&libxl__vtpm_devtype, +&libxl__usbctrl_devtype, +&libxl__usbdev_devtype, +}; + +static void domcreate_attach_devices(libxl__egc *egc, + libxl__multidev *multidev, + int ret) +{ +libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev); +STATE_AO_GC(dcs->ao); +int domid = dcs->guest_domid; +libxl_domain_config *const d_config = dcs->guest_config; +const struct libxl_device_type *dt; + +if (ret) { +LOG(ERROR, "unable to add %s devices", +device_type_tbl[dcs->device_type_idx]->type); +goto error_out; +} + +dcs->device_type_idx++; +if (dcs->device_type_idx < ARRAY_SIZE(device_type_tbl)) { +dt = device_type_tbl[dcs->device_type_idx]; +if (*(int *)((void *)d_config + dt->num_offset) > 0) { +/* Attach devices */ +libxl__multidev_begin(ao, &dcs->multidev); +dcs->multidev.callback = domcreate_attach_devices; +dt->add(egc, ao, domid, d_config, &dcs->multidev); +libxl__multidev_prepared(egc, &dcs->multidev, 0); +return; +} + +domcreate_attach_devices(egc, &dcs->multidev, 0); +return; +} + +domcreate_attach_pci(egc, multidev, 0); +return; + +error_out: +assert(ret); +domcreate_complete(egc, dcs, ret); +} + static void domcreate_devmodel_started(libxl__egc *egc, libxl__dm_spawn_state *dmss, int ret) @@ -1430,113 +1471,8 @@ static void domcreate_devmodel_started(libxl__egc *egc, } } -/* Plug nic interfaces */ -if (d_config->num_nics > 0) { -/* Attach nics */ -libxl__multidev_begin(ao, &dcs->multidev); -dcs->multidev.callback = domcreate_attach_vtpms; -libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev); -libxl__multidev_prepared(egc, &dcs->multidev, 0); -return; -} - -domcreate_attach_vtpms(egc, &dcs->multidev, 0); -return; - -error_out: -assert(ret); -domcreate_complete(egc, dcs, ret); -} - -static void domcreate_attach_vtpms(libxl__egc *egc, - libxl__multidev *multidev, - int ret) -{ - libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev); - STATE_AO_GC(dcs->ao); - int domid = dcs->guest_domid; - - libxl_domain_config* const d_config = dcs->guest_config; - - if(ret) { - LOG(ERROR, "unable to add nic devices"); - goto error_out; - } - -/* Plug vtpm devices */ - if (d_config->num_vtpms > 0) { - /* Attach vtpms */ - libxl__multidev_begin(ao, &dcs->multidev); - dcs->multidev.callback = domcreate_attach_usbctrls; - libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev); - libxl__multidev_prepare
[Xen-devel] [PATCH v2 2/4] libxl: refactor domcreate_attach_pci() to use device type framework
Signed-off-by: Juergen Gross --- tools/libxl/libxl_create.c | 54 ++-- tools/libxl/libxl_internal.h | 1 + tools/libxl/libxl_pci.c | 32 ++ 3 files changed, 40 insertions(+), 47 deletions(-) diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 5e05f6f..354c73f 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -742,10 +742,8 @@ static void domcreate_bootloader_done(libxl__egc *egc, static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs, int ret); -static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs, - int ret); -static void domcreate_attach_dtdev(libxl__egc *egc, - libxl__domain_create_state *dcs); +static void domcreate_attach_dtdev(libxl__egc *egc, libxl__multidev *multidev, + int ret); static void domcreate_console_available(libxl__egc *egc, libxl__domain_create_state *dcs); @@ -1406,6 +1404,7 @@ static const struct libxl_device_type *device_type_tbl[] = { &libxl__vtpm_devtype, &libxl__usbctrl_devtype, &libxl__usbdev_devtype, +&libxl__pcidev_devtype, }; static void domcreate_attach_devices(libxl__egc *egc, @@ -1440,7 +1439,7 @@ static void domcreate_attach_devices(libxl__egc *egc, return; } -domcreate_attach_pci(egc, multidev, 0); +domcreate_attach_dtdev(egc, multidev, 0); return; error_out: @@ -1480,52 +1479,13 @@ error_out: domcreate_complete(egc, dcs, ret); } -static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev, - int ret) -{ -libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev); -STATE_AO_GC(dcs->ao); -int i; -int domid = dcs->guest_domid; - -/* convenience aliases */ -libxl_domain_config *const d_config = dcs->guest_config; - -if (ret) { -goto error_out; -} - -for (i = 0; i < d_config->num_pcidevs; i++) { -ret = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1); -if (ret < 0) { -LOG(ERROR, "libxl_device_pci_add failed: %d", ret); -goto error_out; -} -} - -if (d_config->num_pcidevs > 0) { -ret = libxl__create_pci_backend(gc, domid, d_config->pcidevs, -d_config->num_pcidevs); -if (ret < 0) { -LOG(ERROR, "libxl_create_pci_backend failed: %d", ret); -goto error_out; -} -} - -domcreate_attach_dtdev(egc, dcs); -return; - -error_out: -assert(ret); -domcreate_complete(egc, dcs, ret); -} - static void domcreate_attach_dtdev(libxl__egc *egc, - libxl__domain_create_state *dcs) + libxl__multidev *multidev, + int ret) { +libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev); STATE_AO_GC(dcs->ao); int i; -int ret; int domid = dcs->guest_domid; /* convenience aliases */ diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index d16161a..79ce392 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -3408,6 +3408,7 @@ extern const struct libxl_device_type libxl__nic_devtype; extern const struct libxl_device_type libxl__vtpm_devtype; extern const struct libxl_device_type libxl__usbctrl_devtype; extern const struct libxl_device_type libxl__usbdev_devtype; +extern const struct libxl_device_type libxl__pcidev_devtype; /*- Domain destruction -*/ /* Domain destruction has been split into two functions: diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index 236bdd0..9676687 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -1291,6 +1291,36 @@ out: return rc; } +static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid, + libxl_domain_config *d_config, + libxl__multidev *multidev) +{ +AO_GC; +libxl__ao_device *aodev = libxl__multidev_prepare(multidev); +int i, rc = 0; + +for (i = 0; i < d_config->num_pcidevs; i++) { +rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1); +if (rc < 0) { +LOG(ERROR, "libxl_device_pci_add failed: %d", rc); +goto out; +} +} + +if (d_config->num_pcidevs > 0) { +rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs, +d_config->num_pcidevs); +if (rc < 0) { +LOG(ERROR, "libxl_create_pci_backend failed: %d", rc); +goto out; +} +} + +out: +aodev->rc = rc; +aodev->callback(egc, aodev); +} + static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid