Re: [Xen-devel] Locking on vm-event operations (monitor)
On 7/22/2016 4:00 PM, Razvan Cojocaru wrote: On 07/22/2016 03:32 PM, Corneliu ZUZU wrote: Look @ hvm_do_resume(): 1. If you, as a toolstack user, get at sane step no. 6: "Uninitialize everything (no events are possible here because of steps 4-5)." 2. But just before you do that, a hvm_do_resume() happens on an arbitrary vCPU of the domain and gets to this point: if ( unlikely(v->arch.vm_event) ) { // ---> let's say you are now at this point, just after the above non-NULL check struct monitor_write_data *w = >arch.vm_event->write_data; 3. and before proceeding further, the uninitialization sequence _completes_, i.e. this was done in vm_event_cleanup_domain(): for_each_vcpu ( d, v ) { xfree(v->arch.vm_event); v->arch.vm_event = NULL; } 4. and then in hvm_do_resume() this gets to be done: struct monitor_write_data *w = >arch.vm_event->write_data; [...] if ( w->do_write.msr ) { hvm_msr_write_intercept(w->msr, w->value, 0); w->do_write.msr = 0; } then you'll have a beautiful NULL-pointer access hypervisor crash. As I've said before, of course I agree with that Sorry, I thought by your previous statement "I assume that the reason why this has not been addressed in the past is that introspection applications are expected to be well-behaved" you were inferring that with a 'sane' toolstack user, as you defined it, it won't be possible to have a hypervisor crash. I did (please see below). Then, again, you'd be wrong, the hvm_do_resume() was just one example, in a similar fashion this can happen in a number of other places. Details will be given in the patch-series. - isn't that what a previous patch you've submitted addressed? I'm not sure what patch you're referring to, I don't remember ever addressing these issues before.. I am talking about your patch "x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes", which, if I'm not mistaken, addresses the above problem (since we won't xfree(v->arch.vm_event) on monitor uninit anymore). Heh, I wouldn't call fixing problem A and by -coincidence- with that fix also having -totally unrelated- problem B go away "addressing problem B". I didn't even mention these locking issues in that patch, so how could I have addressed them there? With the above problem fixed, the workflow I suggested should be fine. The fact of the matter remains that the current state of staging (which is what this thread applies to) still has the hvm_do_resume() issue as well as the others. Again, I would prefer things to be rock solid, so personally I encourage your patch and hope we get it in. Thanks, Razvan Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Locking on vm-event operations (monitor)
On 7/22/2016 3:10 PM, Razvan Cojocaru wrote: On 07/22/2016 02:39 PM, Corneliu ZUZU wrote: On 7/22/2016 1:29 PM, Razvan Cojocaru wrote: On 07/22/2016 01:17 PM, Corneliu ZUZU wrote: On 7/22/2016 12:55 PM, Razvan Cojocaru wrote: On 07/22/2016 12:27 PM, Corneliu ZUZU wrote: Hi, I've been inspecting vm-event code parts to try and understand when and why domain pausing/locking is done. If I understood correctly, domain pausing is done solely to force all the vCPUs of that domain to see a configuration update and act upon it (e.g. in the case of a XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG that enables CR3 monitoring, domain pausing/unpausing ensures immediate enabling of CR3 load-exiting for all vCPUs), not to synchronize concurrent operations (lock-behavior). As for locking, I see that for example vm_event_init_domain(), vm_event_cleanup_domain() and monitor_domctl() are all protected by the domctl-lock, but I don't think that's enough. Here are a few code-paths that led me to believe that: * do_hvm_op()->monitor_guest_request() reads d.monitor.guest_request_* resources, but it doesn't seem to take the domctl lock, so it seems possible for that to happen _while_ those resources are initialized/cleaned-up * monitor_guest_request() also calls monitor_traps()->...->vm_event_wait_slot()->...->vm_event_grab_slot() which attempts a vm_event_ring_lock(ved), which could also be called _while_ that's initialized (vm_event_enable()) or cleaned-up (vm_event_disable()) * hvm_monitor_cr() - e.g. on the code-path vmx_vmexit_handler(EXIT_REASON_CR_ACCESS)->vmx_cr_access(VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR)->hvm_mov_to_cr()->hvm_set_cr0()->hvm_monitor_crX() there doesn't seem to be taken into account the possibility of a concurrent monitor_init_domain()/monitor_cleanup_domain() Am I missing something with these conclusions? Your conclusions look correct, but I assume that the reason why this has not been addressed in the past is that introspection applications are expected to be well-behaved. I wouldn't think that was the rationale (considering that the risk is crashing the hypervisor as a whole; also see below), I'd rather think this simply went unnoticed. Specifically, in a codebase where the choice between uint64_t and long int matters speed-wise, and where unlikely()s also matter, an extra lock may be an issue. The typical flow of an introspection application is: 1. Initialize everything. 2. Subscribe to relevant events. 3. Event processing loop. 4. Unsubscribe from events. 5. Do a last-run of event processing (already queued in the ring buffer). 6. Uninitialize everything (no events are possible here because of steps 4-5). And even if an introspection application behaves as sanely as you say, the current layout of the code still doesn't guarantee bug-free behavior. That's because for one, there are in-guest events (that trigger access of vm-events resources) that are completely asynchronous in relation to 2-6, for example a HVMOP_guest_request_vm_event (subsequently calling monitor_guest_request()). Not true. As part of step 4 d->monitor.guest_request_enabled will become 0, so no sensitive code will run after that: I wasn't being rigorous with 2-6 above (with that I only meant to say 'there can't be any vm-events before everything is initialized') and it seems true that at least in the monitor_guest_request() case it might not be able to produce a hypervisor crash (although there are a number of ugly inconsistencies on that code path). But nonetheless a hypervisor crash can _still_ happen even with a _sane_ toolstack user. Look @ hvm_do_resume(): 1. If you, as a toolstack user, get at sane step no. 6: "Uninitialize everything (no events are possible here because of steps 4-5)." 2. But just before you do that, a hvm_do_resume() happens on an arbitrary vCPU of the domain and gets to this point: if ( unlikely(v->arch.vm_event) ) { // ---> let's say you are now at this point, just after the above non-NULL check struct monitor_write_data *w = >arch.vm_event->write_data; 3. and before proceeding further, the uninitialization sequence _completes_, i.e. this was done in vm_event_cleanup_domain(): for_each_vcpu ( d, v ) { xfree(v->arch.vm_event); v->arch.vm_event = NULL; } 4. and then in hvm_do_resume() this gets to be done: struct monitor_write_data *w = >arch.vm_event->write_data; [...] if ( w->do_write.msr ) { hvm_msr_write_intercept(w->msr, w->value, 0); w->do_write.msr = 0; } then you'll have a beautiful NULL-pointer access hypervisor crash. As I've said before, of course I agree with that Sorry, I thought by your previous statement "I assume that the reason why this has not been addressed in the past is that introspection applications are expected to be well-behaved" you were
Re: [Xen-devel] Locking on vm-event operations (monitor)
On 7/22/2016 2:13 PM, Andrew Cooper wrote: On 22/07/16 11:31, Corneliu ZUZU wrote: On 7/22/2016 12:51 PM, Andrew Cooper wrote: On 22/07/16 10:27, Corneliu ZUZU wrote: Hi, I've been inspecting vm-event code parts to try and understand when and why domain pausing/locking is done. If I understood correctly, domain pausing is done solely to force all the vCPUs of that domain to see a configuration update and act upon it (e.g. in the case of a XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG that enables CR3 monitoring, domain pausing/unpausing ensures immediate enabling of CR3 load-exiting for all vCPUs), not to synchronize concurrent operations (lock-behavior). Correct. Without the vcpu/domain pause, a vcpu could be midway through a vmexit path with the monitor configuration changing under its feet. OTOH, it could be running in guest context, and only receive the update one scheduling timeslice later. As for locking, I see that for example vm_event_init_domain(), vm_event_cleanup_domain() and monitor_domctl() are all protected by the domctl-lock, but I don't think that's enough. Here are a few code-paths that led me to believe that: * do_hvm_op()->monitor_guest_request() reads d.monitor.guest_request_* resources, but it doesn't seem to take the domctl lock, so it seems possible for that to happen _while_ those resources are initialized/cleaned-up * monitor_guest_request() also calls monitor_traps()->...->vm_event_wait_slot()->...->vm_event_grab_slot() which attempts a vm_event_ring_lock(ved), which could also be called _while_ that's initialized (vm_event_enable()) or cleaned-up (vm_event_disable()) * hvm_monitor_cr() - e.g. on the code-path vmx_vmexit_handler(EXIT_REASON_CR_ACCESS)->vmx_cr_access(VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR)->hvm_mov_to_cr()->hvm_set_cr0()->hvm_monitor_crX() there doesn't seem to be taken into account the possibility of a concurrent monitor_init_domain()/monitor_cleanup_domain() Am I missing something with these conclusions? As a resolution for this, I've been thinking of adding a 'subsys_lock' field in the vm_event_domain structure, either spinlock or rw-lock, which would be initialised/uninitialised when d.vm_event is allocated/freed (domain_create()/complete_domain_destroy()). I don't think you are missing anything. It seems like a monitor lock is needed. FWIW, the domctl lock is only used at the moment because it was easy, and worked when everything was a domctl. Being a global spinlock, it is a severe bottlekneck for concurrent domain management, which I need to find some time to break apart, so the less reliance on it the better from my point of view. ~Andrew I was thinking of not touching the way the domctl lock is acquired in these cases at all, but rather leave that as it is and introduce the rw-lock separately. Are you suggesting I should also change do_domctl to skip domctl_lock_acquire()-ing if op->cmd is XEN_DOMCTL_vm_event_op ? That would certainly be a first step towards amending that bottleneck you mentioned, but it would break consistency of locking-behavior for domctls. I think it would also add significant complexity to what I'll be doing so I hope it wouldn't be a problem if we leave that for a future patch. Sorry - I didn't intend to suggest altering the domctl lock. Just that adding in a new monitor lock and using it everywhere where appropriate would be a good step. Breaking the domctl lock is a very large can of worms, which you definitely won't want to open while attempting to do something else. ~Andrew Yep, agreed. Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Locking on vm-event operations (monitor)
On 7/22/2016 1:29 PM, Razvan Cojocaru wrote: On 07/22/2016 01:17 PM, Corneliu ZUZU wrote: On 7/22/2016 12:55 PM, Razvan Cojocaru wrote: On 07/22/2016 12:27 PM, Corneliu ZUZU wrote: Hi, I've been inspecting vm-event code parts to try and understand when and why domain pausing/locking is done. If I understood correctly, domain pausing is done solely to force all the vCPUs of that domain to see a configuration update and act upon it (e.g. in the case of a XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG that enables CR3 monitoring, domain pausing/unpausing ensures immediate enabling of CR3 load-exiting for all vCPUs), not to synchronize concurrent operations (lock-behavior). As for locking, I see that for example vm_event_init_domain(), vm_event_cleanup_domain() and monitor_domctl() are all protected by the domctl-lock, but I don't think that's enough. Here are a few code-paths that led me to believe that: * do_hvm_op()->monitor_guest_request() reads d.monitor.guest_request_* resources, but it doesn't seem to take the domctl lock, so it seems possible for that to happen _while_ those resources are initialized/cleaned-up * monitor_guest_request() also calls monitor_traps()->...->vm_event_wait_slot()->...->vm_event_grab_slot() which attempts a vm_event_ring_lock(ved), which could also be called _while_ that's initialized (vm_event_enable()) or cleaned-up (vm_event_disable()) * hvm_monitor_cr() - e.g. on the code-path vmx_vmexit_handler(EXIT_REASON_CR_ACCESS)->vmx_cr_access(VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR)->hvm_mov_to_cr()->hvm_set_cr0()->hvm_monitor_crX() there doesn't seem to be taken into account the possibility of a concurrent monitor_init_domain()/monitor_cleanup_domain() Am I missing something with these conclusions? Your conclusions look correct, but I assume that the reason why this has not been addressed in the past is that introspection applications are expected to be well-behaved. I wouldn't think that was the rationale (considering that the risk is crashing the hypervisor as a whole; also see below), I'd rather think this simply went unnoticed. Specifically, in a codebase where the choice between uint64_t and long int matters speed-wise, and where unlikely()s also matter, an extra lock may be an issue. The typical flow of an introspection application is: 1. Initialize everything. 2. Subscribe to relevant events. 3. Event processing loop. 4. Unsubscribe from events. 5. Do a last-run of event processing (already queued in the ring buffer). 6. Uninitialize everything (no events are possible here because of steps 4-5). And even if an introspection application behaves as sanely as you say, the current layout of the code still doesn't guarantee bug-free behavior. That's because for one, there are in-guest events (that trigger access of vm-events resources) that are completely asynchronous in relation to 2-6, for example a HVMOP_guest_request_vm_event (subsequently calling monitor_guest_request()). Not true. As part of step 4 d->monitor.guest_request_enabled will become 0, so no sensitive code will run after that: I wasn't being rigorous with 2-6 above (with that I only meant to say 'there can't be any vm-events before everything is initialized') and it seems true that at least in the monitor_guest_request() case it might not be able to produce a hypervisor crash (although there are a number of ugly inconsistencies on that code path). But nonetheless a hypervisor crash can _still_ happen even with a _sane_ toolstack user. Look @ hvm_do_resume(): 1. If you, as a toolstack user, get at sane step no. 6: "Uninitialize everything (no events are possible here because of steps 4-5)." 2. But just before you do that, a hvm_do_resume() happens on an arbitrary vCPU of the domain and gets to this point: if ( unlikely(v->arch.vm_event) ) { // ---> let's say you are now at this point, just after the above non-NULL check struct monitor_write_data *w = >arch.vm_event->write_data; 3. and before proceeding further, the uninitialization sequence _completes_, i.e. this was done in vm_event_cleanup_domain(): for_each_vcpu ( d, v ) { xfree(v->arch.vm_event); v->arch.vm_event = NULL; } 4. and then in hvm_do_resume() this gets to be done: struct monitor_write_data *w = >arch.vm_event->write_data; [...] if ( w->do_write.msr ) { hvm_msr_write_intercept(w->msr, w->value, 0); w->do_write.msr = 0; } then you'll have a beautiful NULL-pointer access hypervisor crash. 129 void monitor_guest_request(void) 130 { 131 struct vcpu *curr = current; 132 struct domain *d = curr->domain; 133 134 if ( d->monitor.guest_request_enabled ) 135 { 136 vm_event_request_t req = { 137 .reason = VM_EVENT_REASON_GUEST_REQUEST, 138 .vcpu_id = curr->vcpu_id, 139 }
Re: [Xen-devel] Locking on vm-event operations (monitor)
On 7/22/2016 12:51 PM, Andrew Cooper wrote: On 22/07/16 10:27, Corneliu ZUZU wrote: Hi, I've been inspecting vm-event code parts to try and understand when and why domain pausing/locking is done. If I understood correctly, domain pausing is done solely to force all the vCPUs of that domain to see a configuration update and act upon it (e.g. in the case of a XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG that enables CR3 monitoring, domain pausing/unpausing ensures immediate enabling of CR3 load-exiting for all vCPUs), not to synchronize concurrent operations (lock-behavior). Correct. Without the vcpu/domain pause, a vcpu could be midway through a vmexit path with the monitor configuration changing under its feet. OTOH, it could be running in guest context, and only receive the update one scheduling timeslice later. As for locking, I see that for example vm_event_init_domain(), vm_event_cleanup_domain() and monitor_domctl() are all protected by the domctl-lock, but I don't think that's enough. Here are a few code-paths that led me to believe that: * do_hvm_op()->monitor_guest_request() reads d.monitor.guest_request_* resources, but it doesn't seem to take the domctl lock, so it seems possible for that to happen _while_ those resources are initialized/cleaned-up * monitor_guest_request() also calls monitor_traps()->...->vm_event_wait_slot()->...->vm_event_grab_slot() which attempts a vm_event_ring_lock(ved), which could also be called _while_ that's initialized (vm_event_enable()) or cleaned-up (vm_event_disable()) * hvm_monitor_cr() - e.g. on the code-path vmx_vmexit_handler(EXIT_REASON_CR_ACCESS)->vmx_cr_access(VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR)->hvm_mov_to_cr()->hvm_set_cr0()->hvm_monitor_crX() there doesn't seem to be taken into account the possibility of a concurrent monitor_init_domain()/monitor_cleanup_domain() Am I missing something with these conclusions? As a resolution for this, I've been thinking of adding a 'subsys_lock' field in the vm_event_domain structure, either spinlock or rw-lock, which would be initialised/uninitialised when d.vm_event is allocated/freed (domain_create()/complete_domain_destroy()). I don't think you are missing anything. It seems like a monitor lock is needed. FWIW, the domctl lock is only used at the moment because it was easy, and worked when everything was a domctl. Being a global spinlock, it is a severe bottlekneck for concurrent domain management, which I need to find some time to break apart, so the less reliance on it the better from my point of view. ~Andrew I was thinking of not touching the way the domctl lock is acquired in these cases at all, but rather leave that as it is and introduce the rw-lock separately. Are you suggesting I should also change do_domctl to skip domctl_lock_acquire()-ing if op->cmd is XEN_DOMCTL_vm_event_op ? That would certainly be a first step towards amending that bottleneck you mentioned, but it would break consistency of locking-behavior for domctls. I think it would also add significant complexity to what I'll be doing so I hope it wouldn't be a problem if we leave that for a future patch. Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Locking on vm-event operations (monitor)
On 7/22/2016 12:55 PM, Razvan Cojocaru wrote: On 07/22/2016 12:27 PM, Corneliu ZUZU wrote: Hi, I've been inspecting vm-event code parts to try and understand when and why domain pausing/locking is done. If I understood correctly, domain pausing is done solely to force all the vCPUs of that domain to see a configuration update and act upon it (e.g. in the case of a XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG that enables CR3 monitoring, domain pausing/unpausing ensures immediate enabling of CR3 load-exiting for all vCPUs), not to synchronize concurrent operations (lock-behavior). As for locking, I see that for example vm_event_init_domain(), vm_event_cleanup_domain() and monitor_domctl() are all protected by the domctl-lock, but I don't think that's enough. Here are a few code-paths that led me to believe that: * do_hvm_op()->monitor_guest_request() reads d.monitor.guest_request_* resources, but it doesn't seem to take the domctl lock, so it seems possible for that to happen _while_ those resources are initialized/cleaned-up * monitor_guest_request() also calls monitor_traps()->...->vm_event_wait_slot()->...->vm_event_grab_slot() which attempts a vm_event_ring_lock(ved), which could also be called _while_ that's initialized (vm_event_enable()) or cleaned-up (vm_event_disable()) * hvm_monitor_cr() - e.g. on the code-path vmx_vmexit_handler(EXIT_REASON_CR_ACCESS)->vmx_cr_access(VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR)->hvm_mov_to_cr()->hvm_set_cr0()->hvm_monitor_crX() there doesn't seem to be taken into account the possibility of a concurrent monitor_init_domain()/monitor_cleanup_domain() Am I missing something with these conclusions? Your conclusions look correct, but I assume that the reason why this has not been addressed in the past is that introspection applications are expected to be well-behaved. I wouldn't think that was the rationale (considering that the risk is crashing the hypervisor as a whole; also see below), I'd rather think this simply went unnoticed. Specifically, in a codebase where the choice between uint64_t and long int matters speed-wise, and where unlikely()s also matter, an extra lock may be an issue. The typical flow of an introspection application is: 1. Initialize everything. 2. Subscribe to relevant events. 3. Event processing loop. 4. Unsubscribe from events. 5. Do a last-run of event processing (already queued in the ring buffer). 6. Uninitialize everything (no events are possible here because of steps 4-5). And even if an introspection application behaves as sanely as you say, the current layout of the code still doesn't guarantee bug-free behavior. That's because for one, there are in-guest events (that trigger access of vm-events resources) that are completely asynchronous in relation to 2-6, for example a HVMOP_guest_request_vm_event (subsequently calling monitor_guest_request()). As a resolution for this, I've been thinking of adding a 'subsys_lock' field in the vm_event_domain structure, either spinlock or rw-lock, which would be initialised/uninitialised when d.vm_event is allocated/freed (domain_create()/complete_domain_destroy()). I have nothing against this. Having as many assurances as possible that things will work is definitely a plus in my book - with the comment that I would prefer a rwlock to an ordinary spinlock Yep, I'd prefer that too. , and that "subsys_lock" sounds obscure to me, although I admit that I can't think of a good name at the moment. I'm referring to monitor, share & paging as "subsystems" of the whole "vm-events" subsystem (in that sense the 3 aforementioned are "sub-subsystems"). To me it's acceptable, but I'm open to better names. Thanks, Razvan Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] Locking on vm-event operations (monitor)
Hi, I've been inspecting vm-event code parts to try and understand when and why domain pausing/locking is done. If I understood correctly, domain pausing is done solely to force all the vCPUs of that domain to see a configuration update and act upon it (e.g. in the case of a XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG that enables CR3 monitoring, domain pausing/unpausing ensures immediate enabling of CR3 load-exiting for all vCPUs), not to synchronize concurrent operations (lock-behavior). As for locking, I see that for example vm_event_init_domain(), vm_event_cleanup_domain() and monitor_domctl() are all protected by the domctl-lock, but I don't think that's enough. Here are a few code-paths that led me to believe that: * do_hvm_op()->monitor_guest_request() reads d.monitor.guest_request_* resources, but it doesn't seem to take the domctl lock, so it seems possible for that to happen _while_ those resources are initialized/cleaned-up * monitor_guest_request() also calls monitor_traps()->...->vm_event_wait_slot()->...->vm_event_grab_slot() which attempts a vm_event_ring_lock(ved), which could also be called _while_ that's initialized (vm_event_enable()) or cleaned-up (vm_event_disable()) * hvm_monitor_cr() - e.g. on the code-path vmx_vmexit_handler(EXIT_REASON_CR_ACCESS)->vmx_cr_access(VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR)->hvm_mov_to_cr()->hvm_set_cr0()->hvm_monitor_crX() there doesn't seem to be taken into account the possibility of a concurrent monitor_init_domain()/monitor_cleanup_domain() Am I missing something with these conclusions? As a resolution for this, I've been thinking of adding a 'subsys_lock' field in the vm_event_domain structure, either spinlock or rw-lock, which would be initialised/uninitialised when d.vm_event is allocated/freed (domain_create()/complete_domain_destroy()). Let me know what you think, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 03/16] x86/monitor: mechanical renames
On 7/18/2016 9:07 PM, Andrew Cooper wrote: On 15/07/16 08:18, Corneliu ZUZU wrote: On 7/12/2016 9:10 AM, Corneliu ZUZU wrote: On 7/11/2016 7:43 PM, Tamas K Lengyel wrote: On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU <cz...@bitdefender.com> wrote: On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <cz...@bitdefender.com> wrote: Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()- don't have an 'arch_' prefix. Apply the same rule for monitor functions - originally the only two monitor functions that had an 'arch_' prefix were arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that prefix because -they had a counterpart function in common code-, that being monitor_domctl(). This should actually be the other way around - ie adding the arch_ prefix to vm_event functions that lack it. Given that the majority of the arch-specific functions called from common-code don't have an 'arch_' prefix unless they have a common counterpart, I was guessing that was the rule. It made sense in my head since I saw in that the intention of avoiding naming conflicts (i.e you can't have monitor_domctl() both on the common-side and on the arch-side, so prepend 'arch_' to the latter). I noticed you guys also 'skip' the prefix when sending patches, so that reinforced my assumption. Having the arch_ prefix is helpful to know that the function is dealing with the arch specific structs and not common. Personally I don't see much use in 'knowing that the function is dealing with the arch structs' from the call-site and you can tell that from the implementation-site just by looking at the path of its source file. Also, the code is pretty much localized in the arch directory anyway so usually one wouldn't have to go back and forth between common and arch that often. What really bothers me though is always having to read 'arch_' when spelling a function-name and also that it makes the function name longer without much benefit. Your suggestion of adding it to pretty much all functions that make up the interface to common just adds to that headache. :-D Similarly that's why we have the hvm_ prefix for functions in hvm/monitor. 'hvm_' doesn't seem to me more special than 'monitor_', for instance, but maybe that's just me. Let this also be the rule for future 'arch_' functions additions, and with this patch remove the 'arch_' prefix from the monitor functions that don't have a counterpart in common-code (all but those 2 aforementioned). Even if there are no common counter-parts to the function, the arch_ prefix should remain, so I won't be able to ack this patch. Tamas Having said the above, are you still of the same opinion? Yes, I am. While it's not a hard rule to always apply these prefix, it does make sense to have them so I don't see benefit in removing the existing prefixes. Well, for one the benefit would be not confusing developers by creating inconsistencies: what's the rule here, i.e. why isn't a function such as alloc_domain_struct prefixed w/ 'arch_' but arch_domain_create is? The reason seems to be the latter having a common counterpart while the former not, at least that's what I see being done all over the code-base. Also, I've done this before and you seemed to agree: https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html (Q1). You also suggested creating arch-specific functions without the prefix: https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html . Why the sudden change of heart? 2ndly and obviously, removing the prefixes would make function names shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and then read "vm_event_vcpu_unpause"). 3rd reason is that adding the prefix to -all- arch-specific functions called from common would mean having a lot new functions with the prefix. I'd read the prefix over and over again and at some point I'd get annoyed and say "ok, ok, it's arch_, I get it; why use this prefix so much again?". 4th reason is that the advantage of telling that the function accesses arch structures is much too little considering that idk, 50% of the codebase is arch-specific, so it doesn't provide much information, this categorization is too broad to deserve a special prefix. Whereas using the prefix only for functions that do have a common counterpart gives you the extra information that the 'operation' is only -partly- arch-specific, i.e. to see the whole picture, look @ the common-side implementation. Keep in mind that we'd also be -losing that information- if we were to apply the 'go with arch_ for all' rule.. (this could be a 5th reason) Adding arch_ prefix to the ones that don't already have one is optional, I was just pointing out that if you really feel like standardizing the naming convention, that's where I would like things to move towards to. Tamas I don't think I'd say this patch "standa
Re: [Xen-devel] [PATCH v5 0/7] adjustments
On 7/15/2016 4:05 PM, Andrew Cooper wrote: On 15/07/16 11:41, Corneliu ZUZU wrote: Corneliu ZUZU (7): 1/7: asm-arm/atomic.h: fix arm32|arm64 macros duplication Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * remove paragraph(s) from README.LinuxPrimitives file * no empty line additions 2/7: asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Changed: 3/7: asm-arm/atomic.h: reorder macros to match x86-side Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Acked-by: Julien Grall <julien.gr...@arm.com> Changed: 4/7: asm/atomic.h: common prototyping (add xen/atomic.h) Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Acked-by: Julien Grall <julien.gr...@arm.com> Changed: * removed paragraph(s) in README.LinuxPrimitives file 5/7: xen/atomic.h: fix: make atomic_read() param const Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Reviewed-by: Julien Grall <julien.gr...@arm.com> Changed: 6/7: asm-arm/atomic.h: atomic_{inc,dec}_return: macros to inline functions Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * README.LinuxPrimitives not updated anymore 7/7: asm/atomic.h: implement missing and add common prototypes Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Acked-by: Julien Grall <julien.gr...@arm.com> Changed: xen/arch/arm/README.LinuxPrimitives | 8 ++ xen/include/asm-arm/arm32/atomic.h | 13 --- xen/include/asm-arm/arm64/atomic.h | 13 --- xen/include/asm-arm/atomic.h| 59 -- xen/include/asm-x86/atomic.h| 151 +++--- xen/include/xen/atomic.h| 207 6 files changed, 302 insertions(+), 149 deletions(-) create mode 100644 xen/include/xen/atomic.h Committed. Thankyou very much for untangling this rats nest. ~Andrew Thanks too! As an unrelated request, could you please provide your opinion on the question I've asked @ "[PATCH 03/16] x86/monitor: mechanical renames" ? I'm not expecting a specific answer, I just need to know what to do next. Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 7/7] asm/atomic.h: implement missing and add common prototypes
ARM (): * add atomic_add_unless() wrapper over __atomic_add_unless() (for common-code interface, i.e. ) X86 (): * implement missing functions atomic_{sub,inc,dec}_return(), atomic_add_unless() * implement missing macro atomic_xchg() COMMON (): * add prototypes for the aforementioned newly implemented X86 functions in common Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Acked-by: Julien Grall <julien.gr...@arm.com> --- Changed since v4: --- xen/include/asm-arm/atomic.h | 5 + xen/include/asm-x86/atomic.h | 27 +++ xen/include/xen/atomic.h | 36 3 files changed, 68 insertions(+) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index f060c5a..22a5036 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -170,6 +170,11 @@ static inline int atomic_add_negative(int i, atomic_t *v) return atomic_add_return(i, v) < 0; } +static inline int atomic_add_unless(atomic_t *v, int a, int u) +{ +return __atomic_add_unless(v, a, u); +} + #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) #endif /* __ARCH_ARM_ATOMIC__ */ diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 1729e29..101eded 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -126,6 +126,11 @@ static inline void atomic_sub(int i, atomic_t *v) : "ir" (i), "m" (*(volatile int *)>counter) ); } +static inline int atomic_sub_return(int i, atomic_t *v) +{ +return arch_fetch_and_add(>counter, -i) - i; +} + static inline int atomic_sub_and_test(int i, atomic_t *v) { unsigned char c; @@ -145,6 +150,11 @@ static inline void atomic_inc(atomic_t *v) : "m" (*(volatile int *)>counter) ); } +static inline int atomic_inc_return(atomic_t *v) +{ +return atomic_add_return(1, v); +} + static inline int atomic_inc_and_test(atomic_t *v) { unsigned char c; @@ -164,6 +174,11 @@ static inline void atomic_dec(atomic_t *v) : "m" (*(volatile int *)>counter) ); } +static inline int atomic_dec_return(atomic_t *v) +{ +return atomic_sub_return(1, v); +} + static inline int atomic_dec_and_test(atomic_t *v) { unsigned char c; @@ -186,4 +201,16 @@ static inline int atomic_add_negative(int i, atomic_t *v) return c; } +static inline int atomic_add_unless(atomic_t *v, int a, int u) +{ +int c, old; + +c = atomic_read(v); +while (c != u && (old = atomic_cmpxchg(v, c, c + a)) != c) +c = old; +return c; +} + +#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) + #endif /* __ARCH_X86_ATOMIC__ */ diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 6827468..529213e 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -111,6 +111,15 @@ static inline int atomic_add_return(int i, atomic_t *v); static inline void atomic_sub(int i, atomic_t *v); /** + * atomic_sub_return - sub integer and return + * @i: integer value to sub + * @v: pointer of type atomic_t + * + * Atomically subtracts @i from @v and returns @v - @i. + */ +static inline int atomic_sub_return(int i, atomic_t *v); + +/** * atomic_sub_and_test - subtract value from variable and test result * @i: integer value to subtract * @v: pointer of type atomic_t @@ -130,6 +139,14 @@ static inline int atomic_sub_and_test(int i, atomic_t *v); static inline void atomic_inc(atomic_t *v); /** + * atomic_inc_return - increment atomic variable and return + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 and returns @v + 1. + */ +static inline int atomic_inc_return(atomic_t *v); + +/** * atomic_inc_and_test - increment and test * @v: pointer of type atomic_t * @@ -148,6 +165,14 @@ static inline int atomic_inc_and_test(atomic_t *v); static inline void atomic_dec(atomic_t *v); /** + * atomic_dec_return - decrement atomic variable and return + * @v: pointer of type atomic_t + * + * Atomically decrements @v by 1 and returns @v - 1. + */ +static inline int atomic_dec_return(atomic_t *v); + +/** * atomic_dec_and_test - decrement and test * @v: pointer of type atomic_t * @@ -168,4 +193,15 @@ static inline int atomic_dec_and_test(atomic_t *v); */ static inline int atomic_add_negative(int i, atomic_t *v); +/** + * atomic_add_unless - add to atomic variable unless it has a specified value + * @v: pointer of type atomic_t + * @a: integer value to add + * @u: integer value @v must -not- be for the add to be performed + * + * If @v != @u, adds @a to @v and returns @v + @a. + * Otherwise returns @u (== @v). + */ +static inline int atomic_add_unless(atomic_t *v, int a, int u); + #endif /* __XEN_ATOMIC_H__ */ -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 6/7] asm-arm/atomic.h: atomic_{inc, dec}_return: macros to inline functions
Turn atomic_inc_return and atomic_dec_return atomic.h macros to inline functions. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v4: * README.LinuxPrimitives not updated anymore --- xen/include/asm-arm/atomic.h | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 91f05c8..f060c5a 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -130,9 +130,6 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) - static inline int atomic_sub_and_test(int i, atomic_t *v) { return atomic_sub_return(i, v) == 0; @@ -143,6 +140,11 @@ static inline void atomic_inc(atomic_t *v) atomic_add(1, v); } +static inline int atomic_inc_return(atomic_t *v) +{ +return atomic_add_return(1, v); +} + static inline int atomic_inc_and_test(atomic_t *v) { return atomic_add_return(1, v) == 0; @@ -153,6 +155,11 @@ static inline void atomic_dec(atomic_t *v) atomic_sub(1, v); } +static inline int atomic_dec_return(atomic_t *v) +{ +return atomic_sub_return(1, v); +} + static inline int atomic_dec_and_test(atomic_t *v) { return atomic_sub_return(1, v) == 0; -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 5/7] xen/atomic.h: fix: make atomic_read() param const
This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Reviewed-by: Julien Grall <julien.gr...@arm.com> --- Changed since v4: --- xen/include/asm-arm/atomic.h | 2 +- xen/include/asm-x86/atomic.h | 2 +- xen/include/xen/atomic.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index b81e50d..91f05c8 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -102,7 +102,7 @@ void __bad_atomic_size(void); * strex/ldrex monitor on some implementations. The reason we can use it for * atomic_set() is the clrex or dummy strex done on every exception return. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return *(volatile int *)>counter; } diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 3e99b03..1729e29 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -80,7 +80,7 @@ void __bad_atomic_size(void); } \ }) -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(>counter); } diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index d072912..6827468 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -32,7 +32,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v); +static inline int atomic_read(const atomic_t *v); /** * _atomic_read - read atomic variable non-atomically -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 4/7] asm/atomic.h: common prototyping (add xen/atomic.h)
Create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. Removed outdated comment ("NB. I've [...]"). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Acked-by: Julien Grall <julien.gr...@arm.com> --- Changed since v4: * removed paragraph(s) in README.LinuxPrimitives file --- xen/include/asm-arm/atomic.h | 45 xen/include/asm-x86/atomic.h | 103 +- xen/include/xen/atomic.h | 171 +++ 3 files changed, 202 insertions(+), 117 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index a6e1b37..b81e50d 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -141,12 +133,35 @@ static inline void _atomic_set(atomic_t *v, int i) #define atomic_inc_return(v)(atomic_add_return(1, v)) #define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) -#define atomic_inc(v) atomic_add(1, v) -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec(v) atomic_sub(1, v) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_add_negative(i,v)(atomic_add_return(i, v) < 0) +static inline int atomic_sub_and_test(int i, atomic_t *v) +{ +return atomic_sub_return(i, v) == 0; +} + +static inline void atomic_inc(atomic_t *v) +{ +atomic_add(1, v); +} + +static inline int atomic_inc_and_test(atomic_t *v) +{ +return atomic_add_return(1, v) == 0; +} + +static inline void atomic_dec(atomic_t *v) +{ +atomic_sub(1, v); +} + +static inline int atomic_dec_and_test(atomic_t *v) +{ +return atomic_sub_return(1, v) == 0; +} + +static inline int atomic_add_negative(int i, atomic_t *v) +{ +return atomic_add_return(i, v) < 0; +} #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 5f9f2dd..3e99b03 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_X86_ATOMIC__ #include +#include #include #define build_read_atomic(name, size, type, reg, barrier) \ @@ -79,56 +80,21 @@ void __bad_atomic_size(void); } \ }) -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } - -/** - * atomic_read - read atomic variable - * @v: pointer of type atomic_t - * - * Atomically reads the value of @v. - */ static inline int atomic_read(atomic_t *v) { return read_atomic(>counter); } -/** - * _atomic_read - read atomic variable non-atomically - * @v atomic_t - * - * Non-atomically reads the value of @v - */ static inline int _atomic_read(atomic_t v) { return v.counter; } -/** - * atomic_set - set atomic variable - * @v: pointer of type atomic_t - * @i: required value - * - * Atomically sets the value of @v to @i. - */ static inline void atomic_set(atomic_t *v, int i) { write_atomic(>counter, i); } -/** - * _atomic_set - set atomic variable non-atomically - * @v: pointer of type atomic_t - * @i: required value - * - * Non-atomically sets the value of @v to @i. - */ static inline void _atomic_set(atomic_t *v, int i) { v->counter = i; @@ -139,13 +105,6 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new) return cmpxchg(>counter, old, new); } -/** - * atomic_add - add integer to atomic variable - * @i: integer value to add - * @v: pointer of type atomic_t - * - * Atomically adds @i to @v. - */ static inline void atomic_add(int i,
[Xen-devel] [PATCH v5 2/7] asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement
Place atomic_inc_and_test() implementation after atomic_inc(). Also empty line fix. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v4: --- xen/include/asm-x86/atomic.h | 39 +++ 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index d246b70..5f9f2dd 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -110,7 +110,6 @@ static inline int _atomic_read(atomic_t v) return v.counter; } - /** * atomic_set - set atomic variable * @v: pointer of type atomic_t @@ -217,6 +216,25 @@ static inline void atomic_inc(atomic_t *v) } /** + * atomic_inc_and_test - increment and test + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 + * and returns true if the result is zero, or false for all + * other cases. + */ +static inline int atomic_inc_and_test(atomic_t *v) +{ +unsigned char c; + +asm volatile ( +"lock; incl %0; sete %1" +: "=m" (*(volatile int *)>counter), "=qm" (c) +: "m" (*(volatile int *)>counter) : "memory" ); +return c != 0; +} + +/** * atomic_dec - decrement atomic variable * @v: pointer of type atomic_t * @@ -250,25 +268,6 @@ static inline int atomic_dec_and_test(atomic_t *v) } /** - * atomic_inc_and_test - increment and test - * @v: pointer of type atomic_t - * - * Atomically increments @v by 1 - * and returns true if the result is zero, or false for all - * other cases. - */ -static inline int atomic_inc_and_test(atomic_t *v) -{ -unsigned char c; - -asm volatile ( -"lock; incl %0; sete %1" -: "=m" (*(volatile int *)>counter), "=qm" (c) -: "m" (*(volatile int *)>counter) : "memory" ); -return c != 0; -} - -/** * atomic_add_negative - add and test if negative * @v: pointer of type atomic_t * @i: integer value to add -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 1/7] asm-arm/atomic.h: fix arm32|arm64 macros duplication
Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v4: * remove paragraph(s) from README.LinuxPrimitives file * no empty line additions --- xen/arch/arm/README.LinuxPrimitives | 8 xen/include/asm-arm/arm32/atomic.h | 13 - xen/include/asm-arm/arm64/atomic.h | 13 - xen/include/asm-arm/atomic.h| 13 + 4 files changed, 21 insertions(+), 26 deletions(-) diff --git a/xen/arch/arm/README.LinuxPrimitives b/xen/arch/arm/README.LinuxPrimitives index 3115f51..028e872 100644 --- a/xen/arch/arm/README.LinuxPrimitives +++ b/xen/arch/arm/README.LinuxPrimitives @@ -23,6 +23,10 @@ atomics: last sync @ v3.16-rc6 (last commit: 8715466b6027) linux/arch/arm64/include/asm/atomic.h xen/include/asm-arm/arm64/atomic.h +The following functions were taken from Linux: +atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), +atomic_cmpxchg(), __atomic_add_unless() + - mem*: last sync @ v3.16-rc6 (last commit: d875c9b37240) @@ -91,6 +95,10 @@ atomics: last sync @ v3.16-rc6 (last commit: 030d0178bdbd) linux/arch/arm/include/asm/atomic.h xen/include/asm-arm/arm32/atomic.h +The following functions were taken from Linux: +atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), +atomic_cmpxchg(), __atomic_add_unless() + - mem*: last sync @ v3.16-rc6 (last commit: d98b90ea22b0) diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h index 7ec712f..c03eb68 100644 --- a/xen/include/asm-arm/arm32/atomic.h +++ b/xen/include/asm-arm/arm32/atomic.h @@ -147,19 +147,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return oldval; } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* __ARCH_ARM_ARM32_ATOMIC__ */ /* * Local variables: diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h index b49219e..bce38d4 100644 --- a/xen/include/asm-arm/arm64/atomic.h +++ b/xen/include/asm-arm/arm64/atomic.h @@ -113,8 +113,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new) return oldval; } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - static inline int __atomic_add_unless(atomic_t *v, int a, int u) { int c, old; @@ -125,17 +123,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return c; } -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* * Local variables: diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 29ab265..18cb2e1 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -138,6 +138,19 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif +#define atomic_inc(v) atomic_add(1, v) +#define atomic_dec(v) atomic_sub(1, v) + +#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) +#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) +#define atomic_inc_return(v)(atomic_add_return(1, v)) +#define atomic_dec_return(v)(atomic_sub_return(1, v)) +#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) + +#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) + +#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) + #endif /* __ARCH_ARM_ATOMIC__ */ /* * Local variables: -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v5 0/7] adjustments
Corneliu ZUZU (7): 1/7: asm-arm/atomic.h: fix arm32|arm64 macros duplication Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * remove paragraph(s) from README.LinuxPrimitives file * no empty line additions 2/7: asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Changed: 3/7: asm-arm/atomic.h: reorder macros to match x86-side Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Acked-by: Julien Grall <julien.gr...@arm.com> Changed: 4/7: asm/atomic.h: common prototyping (add xen/atomic.h) Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Acked-by: Julien Grall <julien.gr...@arm.com> Changed: * removed paragraph(s) in README.LinuxPrimitives file 5/7: xen/atomic.h: fix: make atomic_read() param const Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Reviewed-by: Julien Grall <julien.gr...@arm.com> Changed: 6/7: asm-arm/atomic.h: atomic_{inc,dec}_return: macros to inline functions Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * README.LinuxPrimitives not updated anymore 7/7: asm/atomic.h: implement missing and add common prototypes Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Acked-by: Julien Grall <julien.gr...@arm.com> Changed: xen/arch/arm/README.LinuxPrimitives | 8 ++ xen/include/asm-arm/arm32/atomic.h | 13 --- xen/include/asm-arm/arm64/atomic.h | 13 --- xen/include/asm-arm/atomic.h| 59 -- xen/include/asm-x86/atomic.h| 151 +++--- xen/include/xen/atomic.h| 207 6 files changed, 302 insertions(+), 149 deletions(-) create mode 100644 xen/include/xen/atomic.h -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 1/7] asm-arm/atomic.h: fix arm32|arm64 macros duplication
On 7/15/2016 1:06 PM, Julien Grall wrote: On 15/07/16 10:55, Corneliu ZUZU wrote: On 7/15/2016 12:28 PM, Julien Grall wrote: Hi Corneliu, On 15/07/16 07:48, Corneliu ZUZU wrote: Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. Adjust README.LinuxPrimitives in the process. Also empty line fixes. Why do you add empty lines? A little picky today, aren't we? :-) Same as usual ;). [...] They are not necessary nor coding style requirement nor in Linux headers. Please don't introduce changes without a valid reason. I just peeked in the Linux source tree and I noticed there are also headers there with an empty line between the file-comment and #ifndef. Plus, this is the Xen code-base and I don't see why I'd look in the Linux source tree to determine rules that apply to the Xen source-tree. Because files taken from Linux respect Linux coding style. I did the mistake on other files to diverge (such as the SMMU code) and it was a pain to re-sync it later. So I prefer to have a strict rule on it, even for cosmetic changes. Regards, Makes sense and much better than "Please don't introduce changes without a valid reason". ;-) Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 4/7] asm/atomic.h: common prototyping (add xen/atomic.h)
On 7/15/2016 12:30 PM, Julien Grall wrote: On 15/07/16 07:50, Corneliu ZUZU wrote: Create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process (also updated README.LinuxPrimitives file). Removed outdated comment ("NB. I've [...]"). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v3: * update README.LinuxPrimitives file --- xen/arch/arm/README.LinuxPrimitives | 14 +-- xen/include/asm-arm/atomic.h| 45 ++ xen/include/asm-x86/atomic.h| 103 +- xen/include/xen/atomic.h| 171 4 files changed, 210 insertions(+), 123 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/arch/arm/README.LinuxPrimitives b/xen/arch/arm/README.LinuxPrimitives index 4906593..2fcdfa4 100644 --- a/xen/arch/arm/README.LinuxPrimitives +++ b/xen/arch/arm/README.LinuxPrimitives @@ -27,10 +27,11 @@ The following functions were taken from Linux: atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), atomic_cmpxchg(), __atomic_add_unless() -Also, the following macros which were in the meantime moved to asm-arm/atomic.h: -atomic_xchg(v, new), atomic_inc(v), atomic_dec(v), +Also, the following macros which were in the meantime moved to asm-arm/atomic.h +and most of them turned to inline functions: +atomic_xchg(v, new) [still macro], atomic_inc(v), atomic_dec(v), atomic_inc_and_test(v), atomic_dec_and_test(v), -atomic_inc_return(v), atomic_dec_return(v), +atomic_inc_return(v) [still macro], atomic_dec_return(v) [still macro], atomic_sub_and_test(i, v), atomic_add_negative(i,v) As mentioned in patch #1, those functions are not easily sync-able after this patch. Please drop them from README.LinuxPrimitives. Ack. - @@ -105,10 +106,11 @@ The following functions were taken from Linux: atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), atomic_cmpxchg(), __atomic_add_unless() -Also, the following macros which were in the meantime moved to asm-arm/atomic.h: -atomic_xchg(v, new), atomic_inc(v), atomic_dec(v), +Also, the following macros which were in the meantime moved to asm-arm/atomic.h +and most of them turned to inline functions: +atomic_xchg(v, new) [still macro], atomic_inc(v), atomic_dec(v), atomic_inc_and_test(v), atomic_dec_and_test(v), -atomic_inc_return(v), atomic_dec_return(v), +atomic_inc_return(v) [still macro], atomic_dec_return(v) [still macro], atomic_sub_and_test(i, v), atomic_add_negative(i,v) Ditto. With that: Acked-by: Julien Grall <julien.gr...@arm.com> Regards, Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 7/7] asm/atomic.h: implement missing and add common prototypes
ARM (): * add atomic_add_unless() wrapper over __atomic_add_unless() (for common-code interface, i.e. ) X86 (): * implement missing functions atomic_{sub,inc,dec}_return(), atomic_add_unless() * implement missing macro atomic_xchg() COMMON (): * add prototypes for the aforementioned newly implemented X86 functions in common Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v3: * add wrapper over arm-side __atomic_add_unless() instead of removing the leading underscores --- xen/include/asm-arm/atomic.h | 5 + xen/include/asm-x86/atomic.h | 27 +++ xen/include/xen/atomic.h | 36 3 files changed, 68 insertions(+) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index c69aae6..b6aaf57 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -170,6 +170,11 @@ static inline int atomic_add_negative(int i, atomic_t *v) return atomic_add_return(i, v) < 0; } +static inline int atomic_add_unless(atomic_t *v, int a, int u) +{ +return __atomic_add_unless(v, a, u); +} + #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) #endif /* __ARCH_ARM_ATOMIC__ */ diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 1729e29..101eded 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -126,6 +126,11 @@ static inline void atomic_sub(int i, atomic_t *v) : "ir" (i), "m" (*(volatile int *)>counter) ); } +static inline int atomic_sub_return(int i, atomic_t *v) +{ +return arch_fetch_and_add(>counter, -i) - i; +} + static inline int atomic_sub_and_test(int i, atomic_t *v) { unsigned char c; @@ -145,6 +150,11 @@ static inline void atomic_inc(atomic_t *v) : "m" (*(volatile int *)>counter) ); } +static inline int atomic_inc_return(atomic_t *v) +{ +return atomic_add_return(1, v); +} + static inline int atomic_inc_and_test(atomic_t *v) { unsigned char c; @@ -164,6 +174,11 @@ static inline void atomic_dec(atomic_t *v) : "m" (*(volatile int *)>counter) ); } +static inline int atomic_dec_return(atomic_t *v) +{ +return atomic_sub_return(1, v); +} + static inline int atomic_dec_and_test(atomic_t *v) { unsigned char c; @@ -186,4 +201,16 @@ static inline int atomic_add_negative(int i, atomic_t *v) return c; } +static inline int atomic_add_unless(atomic_t *v, int a, int u) +{ +int c, old; + +c = atomic_read(v); +while (c != u && (old = atomic_cmpxchg(v, c, c + a)) != c) +c = old; +return c; +} + +#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) + #endif /* __ARCH_X86_ATOMIC__ */ diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 6827468..529213e 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -111,6 +111,15 @@ static inline int atomic_add_return(int i, atomic_t *v); static inline void atomic_sub(int i, atomic_t *v); /** + * atomic_sub_return - sub integer and return + * @i: integer value to sub + * @v: pointer of type atomic_t + * + * Atomically subtracts @i from @v and returns @v - @i. + */ +static inline int atomic_sub_return(int i, atomic_t *v); + +/** * atomic_sub_and_test - subtract value from variable and test result * @i: integer value to subtract * @v: pointer of type atomic_t @@ -130,6 +139,14 @@ static inline int atomic_sub_and_test(int i, atomic_t *v); static inline void atomic_inc(atomic_t *v); /** + * atomic_inc_return - increment atomic variable and return + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 and returns @v + 1. + */ +static inline int atomic_inc_return(atomic_t *v); + +/** * atomic_inc_and_test - increment and test * @v: pointer of type atomic_t * @@ -148,6 +165,14 @@ static inline int atomic_inc_and_test(atomic_t *v); static inline void atomic_dec(atomic_t *v); /** + * atomic_dec_return - decrement atomic variable and return + * @v: pointer of type atomic_t + * + * Atomically decrements @v by 1 and returns @v - 1. + */ +static inline int atomic_dec_return(atomic_t *v); + +/** * atomic_dec_and_test - decrement and test * @v: pointer of type atomic_t * @@ -168,4 +193,15 @@ static inline int atomic_dec_and_test(atomic_t *v); */ static inline int atomic_add_negative(int i, atomic_t *v); +/** + * atomic_add_unless - add to atomic variable unless it has a specified value + * @v: pointer of type atomic_t + * @a: integer value to add + * @u: integer value @v must -not- be for the add to be performed + * + * If @v != @u, adds @a to @v and returns @v + @a. + * Otherwise returns @u (== @v). + */ +static inline int atomic_add_unless(atomic_t *v, int a, int u); + #endif /* __XEN_ATOMIC_H__ */ -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 5/7] xen/atomic.h: fix: make atomic_read() param const
This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v3: --- xen/include/asm-arm/atomic.h | 2 +- xen/include/asm-x86/atomic.h | 2 +- xen/include/xen/atomic.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index a79420a..78dad29 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -102,7 +102,7 @@ void __bad_atomic_size(void); * strex/ldrex monitor on some implementations. The reason we can use it for * atomic_set() is the clrex or dummy strex done on every exception return. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return *(volatile int *)>counter; } diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 3e99b03..1729e29 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -80,7 +80,7 @@ void __bad_atomic_size(void); } \ }) -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(>counter); } diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index d072912..6827468 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -32,7 +32,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v); +static inline int atomic_read(const atomic_t *v); /** * _atomic_read - read atomic variable non-atomically -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 4/7] asm/atomic.h: common prototyping (add xen/atomic.h)
Create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process (also updated README.LinuxPrimitives file). Removed outdated comment ("NB. I've [...]"). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v3: * update README.LinuxPrimitives file --- xen/arch/arm/README.LinuxPrimitives | 14 +-- xen/include/asm-arm/atomic.h| 45 ++ xen/include/asm-x86/atomic.h| 103 +- xen/include/xen/atomic.h| 171 4 files changed, 210 insertions(+), 123 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/arch/arm/README.LinuxPrimitives b/xen/arch/arm/README.LinuxPrimitives index 4906593..2fcdfa4 100644 --- a/xen/arch/arm/README.LinuxPrimitives +++ b/xen/arch/arm/README.LinuxPrimitives @@ -27,10 +27,11 @@ The following functions were taken from Linux: atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), atomic_cmpxchg(), __atomic_add_unless() -Also, the following macros which were in the meantime moved to asm-arm/atomic.h: -atomic_xchg(v, new), atomic_inc(v), atomic_dec(v), +Also, the following macros which were in the meantime moved to asm-arm/atomic.h +and most of them turned to inline functions: +atomic_xchg(v, new) [still macro], atomic_inc(v), atomic_dec(v), atomic_inc_and_test(v), atomic_dec_and_test(v), -atomic_inc_return(v), atomic_dec_return(v), +atomic_inc_return(v) [still macro], atomic_dec_return(v) [still macro], atomic_sub_and_test(i, v), atomic_add_negative(i,v) - @@ -105,10 +106,11 @@ The following functions were taken from Linux: atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), atomic_cmpxchg(), __atomic_add_unless() -Also, the following macros which were in the meantime moved to asm-arm/atomic.h: -atomic_xchg(v, new), atomic_inc(v), atomic_dec(v), +Also, the following macros which were in the meantime moved to asm-arm/atomic.h +and most of them turned to inline functions: +atomic_xchg(v, new) [still macro], atomic_inc(v), atomic_dec(v), atomic_inc_and_test(v), atomic_dec_and_test(v), -atomic_inc_return(v), atomic_dec_return(v), +atomic_inc_return(v) [still macro], atomic_dec_return(v) [still macro], atomic_sub_and_test(i, v), atomic_add_negative(i,v) - diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 620c636..a79420a 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -141,12 +133,35 @@ static inline void _atomic_set(atomic_t *v, int i) #define atomic_inc_return(v)(atomic_add_return(1, v)) #define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) -#define atomic_inc(v) atomic_add(1, v) -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec(v) atomic_sub(1, v) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_add_negative(i,v)(atomic_add_return(i, v) < 0) +static inline int atomic_sub_and_test(int i, atomic_t *v) +{ +return atomic_sub_return(i, v) == 0; +} + +static inline void atomic_inc(atomic_t *v) +{ +atomic_add(1, v); +} + +static inline int atomic_inc_and_test(atomic_t *v) +{ +return atomic_add_return(1, v) == 0; +} + +static inline void atomic_dec(atomic_t *v) +{ +atomic_sub(1, v); +} + +static inline int atomic_dec_and_test(atomic_t *v) +{ +return atomic_sub_return(1, v) == 0; +} + +static inline int atomic_add_negative(int i, atomic_t *v) +{ +return atomic_add_return(i, v) < 0; +} #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) diff --git a/xen/include/asm-x8
[Xen-devel] [PATCH v4 2/7] asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement
Place atomic_inc_and_test() implementation after atomic_inc(). Also empty line fix. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v3: --- xen/include/asm-x86/atomic.h | 39 +++ 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index d246b70..5f9f2dd 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -110,7 +110,6 @@ static inline int _atomic_read(atomic_t v) return v.counter; } - /** * atomic_set - set atomic variable * @v: pointer of type atomic_t @@ -217,6 +216,25 @@ static inline void atomic_inc(atomic_t *v) } /** + * atomic_inc_and_test - increment and test + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 + * and returns true if the result is zero, or false for all + * other cases. + */ +static inline int atomic_inc_and_test(atomic_t *v) +{ +unsigned char c; + +asm volatile ( +"lock; incl %0; sete %1" +: "=m" (*(volatile int *)>counter), "=qm" (c) +: "m" (*(volatile int *)>counter) : "memory" ); +return c != 0; +} + +/** * atomic_dec - decrement atomic variable * @v: pointer of type atomic_t * @@ -250,25 +268,6 @@ static inline int atomic_dec_and_test(atomic_t *v) } /** - * atomic_inc_and_test - increment and test - * @v: pointer of type atomic_t - * - * Atomically increments @v by 1 - * and returns true if the result is zero, or false for all - * other cases. - */ -static inline int atomic_inc_and_test(atomic_t *v) -{ -unsigned char c; - -asm volatile ( -"lock; incl %0; sete %1" -: "=m" (*(volatile int *)>counter), "=qm" (c) -: "m" (*(volatile int *)>counter) : "memory" ); -return c != 0; -} - -/** * atomic_add_negative - add and test if negative * @v: pointer of type atomic_t * @i: integer value to add -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 1/7] asm-arm/atomic.h: fix arm32|arm64 macros duplication
Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. Adjust README.LinuxPrimitives in the process. Also empty line fixes. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v3: * update README.LinuxPrimitives file --- xen/arch/arm/README.LinuxPrimitives | 20 xen/include/asm-arm/arm32/atomic.h | 15 ++- xen/include/asm-arm/arm64/atomic.h | 15 ++- xen/include/asm-arm/atomic.h| 14 ++ 4 files changed, 38 insertions(+), 26 deletions(-) diff --git a/xen/arch/arm/README.LinuxPrimitives b/xen/arch/arm/README.LinuxPrimitives index 3115f51..4906593 100644 --- a/xen/arch/arm/README.LinuxPrimitives +++ b/xen/arch/arm/README.LinuxPrimitives @@ -23,6 +23,16 @@ atomics: last sync @ v3.16-rc6 (last commit: 8715466b6027) linux/arch/arm64/include/asm/atomic.h xen/include/asm-arm/arm64/atomic.h +The following functions were taken from Linux: +atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), +atomic_cmpxchg(), __atomic_add_unless() + +Also, the following macros which were in the meantime moved to asm-arm/atomic.h: +atomic_xchg(v, new), atomic_inc(v), atomic_dec(v), +atomic_inc_and_test(v), atomic_dec_and_test(v), +atomic_inc_return(v), atomic_dec_return(v), +atomic_sub_and_test(i, v), atomic_add_negative(i,v) + - mem*: last sync @ v3.16-rc6 (last commit: d875c9b37240) @@ -91,6 +101,16 @@ atomics: last sync @ v3.16-rc6 (last commit: 030d0178bdbd) linux/arch/arm/include/asm/atomic.h xen/include/asm-arm/arm32/atomic.h +The following functions were taken from Linux: +atomic_add(), atomic_add_return(), atomic_sub(), atomic_sub_return(), +atomic_cmpxchg(), __atomic_add_unless() + +Also, the following macros which were in the meantime moved to asm-arm/atomic.h: +atomic_xchg(v, new), atomic_inc(v), atomic_dec(v), +atomic_inc_and_test(v), atomic_dec_and_test(v), +atomic_inc_return(v), atomic_dec_return(v), +atomic_sub_and_test(i, v), atomic_add_negative(i,v) + - mem*: last sync @ v3.16-rc6 (last commit: d98b90ea22b0) diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h index 7ec712f..78de60f 100644 --- a/xen/include/asm-arm/arm32/atomic.h +++ b/xen/include/asm-arm/arm32/atomic.h @@ -8,6 +8,7 @@ * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. */ + #ifndef __ARCH_ARM_ARM32_ATOMIC__ #define __ARCH_ARM_ARM32_ATOMIC__ @@ -147,20 +148,8 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return oldval; } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* __ARCH_ARM_ARM32_ATOMIC__ */ + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h index b49219e..d640bef 100644 --- a/xen/include/asm-arm/arm64/atomic.h +++ b/xen/include/asm-arm/arm64/atomic.h @@ -19,6 +19,7 @@ * You should have received a copy of the GNU General Public License * along with this program. If not, see <http://www.gnu.org/licenses/>. */ + #ifndef __ARCH_ARM_ARM64_ATOMIC #define __ARCH_ARM_ARM64_ATOMIC @@ -113,8 +114,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new) return oldval; } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - static inline int __atomic_add_unless(atomic_t *v, int a, int u) { int c, old; @@ -125,18 +124,8 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return c; } -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 29ab265..32771e9 100644 --- a/xen/include/asm-
[Xen-devel] [PATCH v4 0/7] adjustments
Corneliu ZUZU (7): 1/7: asm-arm/atomic.h: fix arm32|arm64 macros duplication Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * update README.LinuxPrimitives file 2/7: asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Changed: 3/7: asm-arm/atomic.h: reorder macros to match x86-side Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: 4/7: asm/atomic.h: common prototyping (add xen/atomic.h) Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * update README.LinuxPrimitives file 5/7: xen/atomic.h: fix: make atomic_read() param const Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: 6/7: asm-arm/atomic.h: atomic_{inc,dec}_return: macros to inline functions Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * update README.LinuxPrimitives file 7/7: asm/atomic.h: implement missing and add common prototypes Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Changed: * add wrapper over arm-side __atomic_add_unless() instead of removing the leading underscores xen/arch/arm/README.LinuxPrimitives | 24 + xen/include/asm-arm/arm32/atomic.h | 15 +-- xen/include/asm-arm/arm64/atomic.h | 15 +-- xen/include/asm-arm/atomic.h| 60 +-- xen/include/asm-x86/atomic.h| 151 +++--- xen/include/xen/atomic.h| 207 6 files changed, 323 insertions(+), 149 deletions(-) create mode 100644 xen/include/xen/atomic.h -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/5] asm-arm/atomic.h: fix arm32|arm64 macros duplication
On 7/14/2016 12:26 PM, Julien Grall wrote: On 14/07/16 06:11, Corneliu ZUZU wrote: On 7/13/2016 10:12 PM, Julien Grall wrote: Hi Corneliu, On 13/07/2016 15:18, Corneliu ZUZU wrote: Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. asm-arm/arm*/atomic.h were a copy from Linux. I don't mind if we diverge, however the file xen/arch/arm/README.primitives needs to be update to mention the divergence with Linux. Regards, Julien, AFAICT the README.LinuxPrimitives file specifies the Linux kernel version from which the arm{32,64}/atomic.h files were imported as well as the respective commit in the -Linux kernel- tree. I suppose that information needn't be updated. Could you be more specific on how I should modify that file? To specify which helpers has been taken from Linux in those files. Until now, it was quite easy to figure out that we took all atomic_* helpers. Regards, Ok, will look into that. I suppose also adding: diff -u linux/arch/arm64/include/asm/atomic.h xen/include/asm-arm/arm64/atomic.h diff -u linux/arch/arm/include/asm/atomic.h xen/include/asm-arm/arm32/atomic.h as it's done for the others helps? Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 7/8] arm/atomic.h: remove '__' prefix for __atomic_add_unless()
On 7/14/2016 1:00 PM, Julien Grall wrote: Hi Corneliu, On 14/07/16 10:14, Corneliu ZUZU wrote: The built-in leading underscores ('__') don't serve any purpose, so rename __atomic_add_unless() -> atomic_add_unless(). The leading underscores are from the Linux code. We decided to keep those files unmodified to help syncing atomic code. So I am not in favor of dropping the '__'. If you want to use these functions in the common code, then a wrapper in "asm-arm/atomic.h" should be introduced. Regards, Ok, great. Either add a wrapper or maybe keeping the underscores on X86 too and not including it's prototype in . What would you prefer? Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 8/8] asm-x86/atomic.h: implement missing, add common prototypes
On 7/14/2016 12:44 PM, Andrew Cooper wrote: On 14/07/16 10:39, Corneliu ZUZU wrote: On 7/14/2016 12:29 PM, Andrew Cooper wrote: On 14/07/16 10:14, Corneliu ZUZU wrote: - implement missing functions atomic_{sub,inc,dec}_return(), atomic_add_unless() on X86 and also add prototypes for them in common - add missing macro atomic_xchg for X86 Signed-off-by: Corneliu ZUZU<cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> I'm not sure about atomic_sub_return(int i, atomic_t *v) implementation - is it ok? (calling arch_fetch_and_add(>counter, -i) with 2nd argument negative). Looks ok to me. Everything is signed integers in the internals. There is a boundary condition when passing INT_MIN, but those exist elsewhere as well. ~Andrew Thanks, thought so too - there should be no difference bit-wise when adding an unsigned int converted from an int vs adding the int directly. Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 8/8] asm-x86/atomic.h: implement missing, add common prototypes
On 7/14/2016 12:29 PM, Andrew Cooper wrote: On 14/07/16 10:14, Corneliu ZUZU wrote: - implement missing functions atomic_{sub,inc,dec}_return(), atomic_add_unless() on X86 and also add prototypes for them in common - add missing macro atomic_xchg for X86 Signed-off-by: Corneliu ZUZU<cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> I'm not sure about atomic_sub_return(int i, atomic_t *v) implementation - is it ok? (calling arch_fetch_and_add(>counter, -i) with 2nd argument negative). Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 5/8] fix: make atomic_read() param const
On 7/14/2016 12:26 PM, Andrew Cooper wrote: On 14/07/16 10:13, Corneliu ZUZU wrote: This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> You have dropped a Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> from v2. ~Andrew Oh, that's right, sorry. Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 5/8] fix: make atomic_read() param const
On 7/14/2016 12:26 PM, Andrew Cooper wrote: On 14/07/16 10:13, Corneliu ZUZU wrote: This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> You have dropped a Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> from v2. ~Andrew Intentionally dropped, he reviewed the empty line fixes in ARM atomic.h - which were now moved to 1/8 (which includes Reviewed-by: [...]). Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 8/8] asm-x86/atomic.h: implement missing, add common prototypes
- implement missing functions atomic_{sub,inc,dec}_return(), atomic_add_unless() on X86 and also add prototypes for them in common - add missing macro atomic_xchg for X86 Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-x86/atomic.h | 27 +++ xen/include/xen/atomic.h | 36 2 files changed, 63 insertions(+) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 1729e29..101eded 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -126,6 +126,11 @@ static inline void atomic_sub(int i, atomic_t *v) : "ir" (i), "m" (*(volatile int *)>counter) ); } +static inline int atomic_sub_return(int i, atomic_t *v) +{ +return arch_fetch_and_add(>counter, -i) - i; +} + static inline int atomic_sub_and_test(int i, atomic_t *v) { unsigned char c; @@ -145,6 +150,11 @@ static inline void atomic_inc(atomic_t *v) : "m" (*(volatile int *)>counter) ); } +static inline int atomic_inc_return(atomic_t *v) +{ +return atomic_add_return(1, v); +} + static inline int atomic_inc_and_test(atomic_t *v) { unsigned char c; @@ -164,6 +174,11 @@ static inline void atomic_dec(atomic_t *v) : "m" (*(volatile int *)>counter) ); } +static inline int atomic_dec_return(atomic_t *v) +{ +return atomic_sub_return(1, v); +} + static inline int atomic_dec_and_test(atomic_t *v) { unsigned char c; @@ -186,4 +201,16 @@ static inline int atomic_add_negative(int i, atomic_t *v) return c; } +static inline int atomic_add_unless(atomic_t *v, int a, int u) +{ +int c, old; + +c = atomic_read(v); +while (c != u && (old = atomic_cmpxchg(v, c, c + a)) != c) +c = old; +return c; +} + +#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) + #endif /* __ARCH_X86_ATOMIC__ */ diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 6827468..529213e 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -111,6 +111,15 @@ static inline int atomic_add_return(int i, atomic_t *v); static inline void atomic_sub(int i, atomic_t *v); /** + * atomic_sub_return - sub integer and return + * @i: integer value to sub + * @v: pointer of type atomic_t + * + * Atomically subtracts @i from @v and returns @v - @i. + */ +static inline int atomic_sub_return(int i, atomic_t *v); + +/** * atomic_sub_and_test - subtract value from variable and test result * @i: integer value to subtract * @v: pointer of type atomic_t @@ -130,6 +139,14 @@ static inline int atomic_sub_and_test(int i, atomic_t *v); static inline void atomic_inc(atomic_t *v); /** + * atomic_inc_return - increment atomic variable and return + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 and returns @v + 1. + */ +static inline int atomic_inc_return(atomic_t *v); + +/** * atomic_inc_and_test - increment and test * @v: pointer of type atomic_t * @@ -148,6 +165,14 @@ static inline int atomic_inc_and_test(atomic_t *v); static inline void atomic_dec(atomic_t *v); /** + * atomic_dec_return - decrement atomic variable and return + * @v: pointer of type atomic_t + * + * Atomically decrements @v by 1 and returns @v - 1. + */ +static inline int atomic_dec_return(atomic_t *v); + +/** * atomic_dec_and_test - decrement and test * @v: pointer of type atomic_t * @@ -168,4 +193,15 @@ static inline int atomic_dec_and_test(atomic_t *v); */ static inline int atomic_add_negative(int i, atomic_t *v); +/** + * atomic_add_unless - add to atomic variable unless it has a specified value + * @v: pointer of type atomic_t + * @a: integer value to add + * @u: integer value @v must -not- be for the add to be performed + * + * If @v != @u, adds @a to @v and returns @v + @a. + * Otherwise returns @u (== @v). + */ +static inline int atomic_add_unless(atomic_t *v, int a, int u); + #endif /* __XEN_ATOMIC_H__ */ -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 7/8] arm/atomic.h: remove '__' prefix for __atomic_add_unless()
The built-in leading underscores ('__') don't serve any purpose, so rename __atomic_add_unless() -> atomic_add_unless(). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/arm32/atomic.h | 2 +- xen/include/asm-arm/arm64/atomic.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h index 78de60f..dc95518 100644 --- a/xen/include/asm-arm/arm32/atomic.h +++ b/xen/include/asm-arm/arm32/atomic.h @@ -121,7 +121,7 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new) return oldval; } -static inline int __atomic_add_unless(atomic_t *v, int a, int u) +static inline int atomic_add_unless(atomic_t *v, int a, int u) { int oldval, newval; unsigned long tmp; diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h index d640bef..f0e83be 100644 --- a/xen/include/asm-arm/arm64/atomic.h +++ b/xen/include/asm-arm/arm64/atomic.h @@ -114,7 +114,7 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new) return oldval; } -static inline int __atomic_add_unless(atomic_t *v, int a, int u) +static inline int atomic_add_unless(atomic_t *v, int a, int u) { int c, old; -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 6/8] asm-arm/atomic.h: atomic_{inc, dec}_return: macros to inline functions
Turn atomic_inc_return and atomic_dec_return atomic.h macros to inline functions. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/atomic.h | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 78dad29..c69aae6 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -130,9 +130,6 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) - static inline int atomic_sub_and_test(int i, atomic_t *v) { return atomic_sub_return(i, v) == 0; @@ -143,6 +140,11 @@ static inline void atomic_inc(atomic_t *v) atomic_add(1, v); } +static inline int atomic_inc_return(atomic_t *v) +{ +return atomic_add_return(1, v); +} + static inline int atomic_inc_and_test(atomic_t *v) { return atomic_add_return(1, v) == 0; @@ -153,6 +155,11 @@ static inline void atomic_dec(atomic_t *v) atomic_sub(1, v); } +static inline int atomic_dec_return(atomic_t *v) +{ +return atomic_sub_return(1, v); +} + static inline int atomic_dec_and_test(atomic_t *v) { return atomic_sub_return(1, v) == 0; -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 5/8] fix: make atomic_read() param const
This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v2: --- xen/include/asm-arm/atomic.h | 2 +- xen/include/asm-x86/atomic.h | 2 +- xen/include/xen/atomic.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index a79420a..78dad29 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -102,7 +102,7 @@ void __bad_atomic_size(void); * strex/ldrex monitor on some implementations. The reason we can use it for * atomic_set() is the clrex or dummy strex done on every exception return. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return *(volatile int *)>counter; } diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 3e99b03..1729e29 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -80,7 +80,7 @@ void __bad_atomic_size(void); } \ }) -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(>counter); } diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index d072912..6827468 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -32,7 +32,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v); +static inline int atomic_read(const atomic_t *v); /** * _atomic_read - read atomic variable non-atomically -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 4/8] asm/atomic.h: common prototyping (add xen/atomic.h)
Create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. Removed outdated comment ("NB. I've [...]"). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v2: * sample code for atomic_cmpxchg() (doc-comment in ) updated to use atomic_read() instead of directly accessing v.counter --- xen/include/asm-arm/atomic.h | 45 xen/include/asm-x86/atomic.h | 103 +- xen/include/xen/atomic.h | 171 +++ 3 files changed, 202 insertions(+), 117 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 620c636..a79420a 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -141,12 +133,35 @@ static inline void _atomic_set(atomic_t *v, int i) #define atomic_inc_return(v)(atomic_add_return(1, v)) #define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) -#define atomic_inc(v) atomic_add(1, v) -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec(v) atomic_sub(1, v) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_add_negative(i,v)(atomic_add_return(i, v) < 0) +static inline int atomic_sub_and_test(int i, atomic_t *v) +{ +return atomic_sub_return(i, v) == 0; +} + +static inline void atomic_inc(atomic_t *v) +{ +atomic_add(1, v); +} + +static inline int atomic_inc_and_test(atomic_t *v) +{ +return atomic_add_return(1, v) == 0; +} + +static inline void atomic_dec(atomic_t *v) +{ +atomic_sub(1, v); +} + +static inline int atomic_dec_and_test(atomic_t *v) +{ +return atomic_sub_return(1, v) == 0; +} + +static inline int atomic_add_negative(int i, atomic_t *v) +{ +return atomic_add_return(i, v) < 0; +} #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 5f9f2dd..3e99b03 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_X86_ATOMIC__ #include +#include #include #define build_read_atomic(name, size, type, reg, barrier) \ @@ -79,56 +80,21 @@ void __bad_atomic_size(void); } \ }) -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } - -/** - * atomic_read - read atomic variable - * @v: pointer of type atomic_t - * - * Atomically reads the value of @v. - */ static inline int atomic_read(atomic_t *v) { return read_atomic(>counter); } -/** - * _atomic_read - read atomic variable non-atomically - * @v atomic_t - * - * Non-atomically reads the value of @v - */ static inline int _atomic_read(atomic_t v) { return v.counter; } -/** - * atomic_set - set atomic variable - * @v: pointer of type atomic_t - * @i: required value - * - * Atomically sets the value of @v to @i. - */ static inline void atomic_set(atomic_t *v, int i) { write_atomic(>counter, i); } -/** - * _atomic_set - set atomic variable non-atomically - * @v: pointer of type atomic_t - * @i: required value - * - * Non-atomically sets the value of @v to @i. - */ static inline void _atomic_set(atomic_t *v, int i) { v->counter = i; @@ -139,13 +105,6 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new) return cmpxchg(>counter, old, new); } -/** - * atomic_add - add integer to atomic variable - * @i: integer value to add - * @v: pointer of type atomic_t - * - * Atomically adds @i to @v. - */ static inline void atomic_add(int i, atomic_t *v) { asm volatile ( @@ -
[Xen-devel] [PATCH v3 3/8] asm-arm/atomic.h: reorder macros to match x86-side
Reorder macro definitions to match x86-side. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v2: --- xen/include/asm-arm/atomic.h | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 32771e9..620c636 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -138,16 +138,15 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) +#define atomic_inc_return(v)(atomic_add_return(1, v)) +#define atomic_dec_return(v)(atomic_sub_return(1, v)) + +#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) +#define atomic_inc(v) atomic_add(1, v) +#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) +#define atomic_dec(v) atomic_sub(1, v) +#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) +#define atomic_add_negative(i,v)(atomic_add_return(i, v) < 0) #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 2/8] asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement
Place atomic_inc_and_test() implementation after atomic_inc(). Also empty line fix. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- Changed since v2: arm empty line fixes moved from here --- xen/include/asm-x86/atomic.h | 39 +++ 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index d246b70..5f9f2dd 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -110,7 +110,6 @@ static inline int _atomic_read(atomic_t v) return v.counter; } - /** * atomic_set - set atomic variable * @v: pointer of type atomic_t @@ -217,6 +216,25 @@ static inline void atomic_inc(atomic_t *v) } /** + * atomic_inc_and_test - increment and test + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 + * and returns true if the result is zero, or false for all + * other cases. + */ +static inline int atomic_inc_and_test(atomic_t *v) +{ +unsigned char c; + +asm volatile ( +"lock; incl %0; sete %1" +: "=m" (*(volatile int *)>counter), "=qm" (c) +: "m" (*(volatile int *)>counter) : "memory" ); +return c != 0; +} + +/** * atomic_dec - decrement atomic variable * @v: pointer of type atomic_t * @@ -250,25 +268,6 @@ static inline int atomic_dec_and_test(atomic_t *v) } /** - * atomic_inc_and_test - increment and test - * @v: pointer of type atomic_t - * - * Atomically increments @v by 1 - * and returns true if the result is zero, or false for all - * other cases. - */ -static inline int atomic_inc_and_test(atomic_t *v) -{ -unsigned char c; - -asm volatile ( -"lock; incl %0; sete %1" -: "=m" (*(volatile int *)>counter), "=qm" (c) -: "m" (*(volatile int *)>counter) : "memory" ); -return c != 0; -} - -/** * atomic_add_negative - add and test if negative * @v: pointer of type atomic_t * @i: integer value to add -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 1/8] asm-arm/atomic.h: fix arm32|arm64 macros duplication
Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. Also empty line fixes. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> --- Changed since v2: * also moved mistakenly omitted atomic_xchg() * arm empty line fixes moved here --- xen/include/asm-arm/arm32/atomic.h | 15 ++- xen/include/asm-arm/arm64/atomic.h | 15 ++- xen/include/asm-arm/atomic.h | 14 ++ 3 files changed, 18 insertions(+), 26 deletions(-) diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h index 7ec712f..78de60f 100644 --- a/xen/include/asm-arm/arm32/atomic.h +++ b/xen/include/asm-arm/arm32/atomic.h @@ -8,6 +8,7 @@ * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. */ + #ifndef __ARCH_ARM_ARM32_ATOMIC__ #define __ARCH_ARM_ARM32_ATOMIC__ @@ -147,20 +148,8 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return oldval; } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* __ARCH_ARM_ARM32_ATOMIC__ */ + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h index b49219e..d640bef 100644 --- a/xen/include/asm-arm/arm64/atomic.h +++ b/xen/include/asm-arm/arm64/atomic.h @@ -19,6 +19,7 @@ * You should have received a copy of the GNU General Public License * along with this program. If not, see <http://www.gnu.org/licenses/>. */ + #ifndef __ARCH_ARM_ARM64_ATOMIC #define __ARCH_ARM_ARM64_ATOMIC @@ -113,8 +114,6 @@ static inline int atomic_cmpxchg(atomic_t *ptr, int old, int new) return oldval; } -#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) - static inline int __atomic_add_unless(atomic_t *v, int a, int u) { int c, old; @@ -125,18 +124,8 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return c; } -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 29ab265..32771e9 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -138,7 +138,21 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif +#define atomic_inc(v) atomic_add(1, v) +#define atomic_dec(v) atomic_sub(1, v) + +#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) +#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) +#define atomic_inc_return(v)(atomic_add_return(1, v)) +#define atomic_dec_return(v)(atomic_sub_return(1, v)) +#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) + +#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) + +#define atomic_xchg(v, new) (xchg(&((v)->counter), new)) + #endif /* __ARCH_ARM_ATOMIC__ */ + /* * Local variables: * mode: C -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 0/8] adjustments
Corneliu ZUZU (8): 1/8: asm-arm/atomic.h: fix arm32|arm64 macros duplication Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: * also moved mistakenly omitted atomic_xchg() * arm empty line fixes moved here 2/8: asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement Changed: * arm empty line fixes moved from here (to 1/8) 3/8: asm-arm/atomic.h: reorder macros to match x86-side Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> Changed: 4/8: asm/atomic.h: common prototyping (add xen/atomic.h) Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Changed: * sample code for atomic_cmpxchg() (doc-comment in ) updated to use atomic_read() instead of directly accessing v.counter 5/8: fix: make atomic_read() param const Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Changed: New in this version: 6/8: asm-arm/atomic.h: atomic_{inc,dec}_return: macros to inline functions 7/8: arm/atomic.h: remove '__' prefix for __atomic_add_unless() 8/8: asm-x86/atomic.h: implement missing, add common prototypes xen/include/asm-arm/arm32/atomic.h | 17 +-- xen/include/asm-arm/arm64/atomic.h | 17 +-- xen/include/asm-arm/atomic.h | 55 -- xen/include/asm-x86/atomic.h | 151 +++ xen/include/xen/atomic.h | 207 + 5 files changed, 296 insertions(+), 151 deletions(-) create mode 100644 xen/include/xen/atomic.h -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/5] asm-arm/atomic.h: fix arm32|arm64 macros duplication
On 7/13/2016 10:12 PM, Julien Grall wrote: Hi Corneliu, On 13/07/2016 15:18, Corneliu ZUZU wrote: Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. asm-arm/arm*/atomic.h were a copy from Linux. I don't mind if we diverge, however the file xen/arch/arm/README.primitives needs to be update to mention the divergence with Linux. Regards, Julien, AFAICT the README.LinuxPrimitives file specifies the Linux kernel version from which the arm{32,64}/atomic.h files were imported as well as the respective commit in the -Linux kernel- tree. I suppose that information needn't be updated. Could you be more specific on how I should modify that file? Thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 4/5] asm/atomic.h: common prototyping (add xen/atomic.h)
On 7/13/2016 10:49 PM, Andrew Cooper wrote: On 13/07/16 20:33, Corneliu ZUZU wrote: On 7/13/2016 10:01 PM, Stefano Stabellini wrote: On Wed, 13 Jul 2016, Corneliu ZUZU wrote: Create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. Removed outdated comment ("NB. I've [...]"). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v1: * removed comments that were duplicate between asm-x86/atomic.h and xen/atomic.h * remove outdated comment ("NB. [...]") * add atomic_cmpxchg doc-comment * don't use yoda condition --- xen/include/asm-arm/atomic.h | 45 xen/include/asm-x86/atomic.h | 103 +- xen/include/xen/atomic.h | 171 +++ 3 files changed, 202 insertions(+), 117 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index e8f7340..01af43b 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -141,12 +133,35 @@ static inline void _atomic_set(atomic_t *v, int i) #define atomic_inc_return(v)(atomic_add_return(1, v)) #define atomic_dec_return(v)(atomic_sub_return(1, v)) What about atomic_inc_return and atomic_dec_return? Doesn't it make sense to do this for all of them, since we are at it? I believe there are also a couple more which are #define'd. Those don't seem to be implemented for X86 and neither are they referred from common code (probably because they really aren't defined for X86). Apologies - this is definitely turning into the rats nest I warned you it might. As far as I am concerned, I only think it is important to have the static inline prototypes for the versions which are actually common between architectures. Those are the ones we don't want to break accidentally. However, for the helpers which are not common, having them consistently static inline would be a distinct improvement over mixed inline/defines. (static inline functions are superior to macros in many ways.) ~Andrew Absolutely no problem! The rats nest you imagined before this series was a different one :P . This really doesn't seem like it would require much effort. What I could also try to do is to actually implement them on X86 as well. Would that be preferable? I think the complete functions that are missing there are: atomic_sub_return, atomic_{inc,dec}_return, atomic_xchg, __atomic_add_unless. Please confirm this list. As for how I'd implement them: atomic_sub_return() - would "return arch_fetch_and_add(>counter, -i) - i;" - similarly to what atomic_add_return() does - do? atomic_{inc,dec}_return() - would "return atomic_{add,sub}_return(1, v)" respectively as on ARM do? atomic_xchg() - since xchg() is also available on X86, would "return xchg(&((v)->counter), new);" as on ARM do? Stefano, Julien: note that I'd have to implement this for ARM64, so, since xchg is -also- available on ARM64, would the above implementation also do in that case? __atomic_add_unless() - again, everything this depends on in the ARM64 version is already available on X86 too; would it also be advised to rename it to atomic_add_unless()? Why the built-in leading underscores '__'? Is this function used anywhere? Thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/5] asm-arm/atomic.h: fix arm32|arm64 macros duplication
Hi Julien, On 7/13/2016 10:12 PM, Julien Grall wrote: Hi Corneliu, On 13/07/2016 15:18, Corneliu ZUZU wrote: Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. asm-arm/arm*/atomic.h were a copy from Linux. I don't mind if we diverge, however the file xen/arch/arm/README.primitives needs to be update to mention the divergence with Linux. Regards, Thanks for pointing that out, I didn't know these files were referred in such a way. What is the purpose of README.LinuxPrimitives and how must I update it to align with these patches? Cheers, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 4/5] asm/atomic.h: common prototyping (add xen/atomic.h)
On 7/13/2016 10:01 PM, Stefano Stabellini wrote: On Wed, 13 Jul 2016, Corneliu ZUZU wrote: Create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. Removed outdated comment ("NB. I've [...]"). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v1: * removed comments that were duplicate between asm-x86/atomic.h and xen/atomic.h * remove outdated comment ("NB. [...]") * add atomic_cmpxchg doc-comment * don't use yoda condition --- xen/include/asm-arm/atomic.h | 45 xen/include/asm-x86/atomic.h | 103 +- xen/include/xen/atomic.h | 171 +++ 3 files changed, 202 insertions(+), 117 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index e8f7340..01af43b 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -141,12 +133,35 @@ static inline void _atomic_set(atomic_t *v, int i) #define atomic_inc_return(v)(atomic_add_return(1, v)) #define atomic_dec_return(v)(atomic_sub_return(1, v)) What about atomic_inc_return and atomic_dec_return? Doesn't it make sense to do this for all of them, since we are at it? I believe there are also a couple more which are #define'd. Those don't seem to be implemented for X86 and neither are they referred from common code (probably because they really aren't defined for X86). -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) -#define atomic_inc(v) atomic_add(1, v) -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec(v) atomic_sub(1, v) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_add_negative(i,v)(atomic_add_return(i, v) < 0) +static inline int atomic_sub_and_test(int i, atomic_t *v) +{ +return atomic_sub_return(i, v) == 0; +} + +static inline void atomic_inc(atomic_t *v) +{ +atomic_add(1, v); +} + +static inline int atomic_inc_and_test(atomic_t *v) +{ +return atomic_add_return(1, v) == 0; +} + +static inline void atomic_dec(atomic_t *v) +{ +atomic_sub(1, v); +} + +static inline int atomic_dec_and_test(atomic_t *v) +{ +return atomic_sub_return(1, v) == 0; +} + +static inline int atomic_add_negative(int i, atomic_t *v) +{ +return atomic_add_return(i, v) < 0; +} #endif /* __ARCH_ARM_ATOMIC__ */ Thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 4/5] asm/atomic.h: common prototyping (add xen/atomic.h)
Create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. Removed outdated comment ("NB. I've [...]"). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v1: * removed comments that were duplicate between asm-x86/atomic.h and xen/atomic.h * remove outdated comment ("NB. [...]") * add atomic_cmpxchg doc-comment * don't use yoda condition --- xen/include/asm-arm/atomic.h | 45 xen/include/asm-x86/atomic.h | 103 +- xen/include/xen/atomic.h | 171 +++ 3 files changed, 202 insertions(+), 117 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index e8f7340..01af43b 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -141,12 +133,35 @@ static inline void _atomic_set(atomic_t *v, int i) #define atomic_inc_return(v)(atomic_add_return(1, v)) #define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) -#define atomic_inc(v) atomic_add(1, v) -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec(v) atomic_sub(1, v) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_add_negative(i,v)(atomic_add_return(i, v) < 0) +static inline int atomic_sub_and_test(int i, atomic_t *v) +{ +return atomic_sub_return(i, v) == 0; +} + +static inline void atomic_inc(atomic_t *v) +{ +atomic_add(1, v); +} + +static inline int atomic_inc_and_test(atomic_t *v) +{ +return atomic_add_return(1, v) == 0; +} + +static inline void atomic_dec(atomic_t *v) +{ +atomic_sub(1, v); +} + +static inline int atomic_dec_and_test(atomic_t *v) +{ +return atomic_sub_return(1, v) == 0; +} + +static inline int atomic_add_negative(int i, atomic_t *v) +{ +return atomic_add_return(i, v) < 0; +} #endif /* __ARCH_ARM_ATOMIC__ */ diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 5f9f2dd..3e99b03 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_X86_ATOMIC__ #include +#include #include #define build_read_atomic(name, size, type, reg, barrier) \ @@ -79,56 +80,21 @@ void __bad_atomic_size(void); } \ }) -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } - -/** - * atomic_read - read atomic variable - * @v: pointer of type atomic_t - * - * Atomically reads the value of @v. - */ static inline int atomic_read(atomic_t *v) { return read_atomic(>counter); } -/** - * _atomic_read - read atomic variable non-atomically - * @v atomic_t - * - * Non-atomically reads the value of @v - */ static inline int _atomic_read(atomic_t v) { return v.counter; } -/** - * atomic_set - set atomic variable - * @v: pointer of type atomic_t - * @i: required value - * - * Atomically sets the value of @v to @i. - */ static inline void atomic_set(atomic_t *v, int i) { write_atomic(>counter, i); } -/** - * _atomic_set - set atomic variable non-atomically - * @v: pointer of type atomic_t - * @i: required value - * - * Non-atomically sets the value of @v to @i. - */ static inline void _atomic_set(atomic_t *v, int i) { v->counter = i; @@ -139,13 +105,6 @@ static inline int atomic_cmpxchg(atomic_t *v, int old, int new) return cmpxchg(>counter, old, new); } -/** - * atomic_add - add integer to atomic variable - * @i: integer value to add - * @v: pointer of type atomic_t - * - * Atomically adds @i to @v. - */ static inline void atomic_add(int i
[Xen-devel] [PATCH v2 5/5] fix: make atomic_read() param const
This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> --- Changed since v1: --- xen/include/asm-arm/atomic.h | 2 +- xen/include/asm-x86/atomic.h | 2 +- xen/include/xen/atomic.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 01af43b..25a33cb 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -102,7 +102,7 @@ void __bad_atomic_size(void); * strex/ldrex monitor on some implementations. The reason we can use it for * atomic_set() is the clrex or dummy strex done on every exception return. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return *(volatile int *)>counter; } diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 3e99b03..1729e29 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -80,7 +80,7 @@ void __bad_atomic_size(void); } \ }) -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(>counter); } diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 3517bc9..d97f91d 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -32,7 +32,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v); +static inline int atomic_read(const atomic_t *v); /** * _atomic_read - read atomic variable non-atomically -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 3/5] asm-arm/atomic.h: reorder macros to match x86-side
Reorder macro definitions to match x86-side. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/atomic.h | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 19a87a5..e8f7340 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -138,16 +138,15 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) +#define atomic_inc_return(v)(atomic_add_return(1, v)) +#define atomic_dec_return(v)(atomic_sub_return(1, v)) + +#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) +#define atomic_inc(v) atomic_add(1, v) +#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) +#define atomic_dec(v) atomic_sub(1, v) +#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) +#define atomic_add_negative(i,v)(atomic_add_return(i, v) < 0) #endif /* __ARCH_ARM_ATOMIC__ */ -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 2/5] asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement
Place atomic_inc_and_test() implementation after atomic_inc(). Also remove unneeded empty line and add a needed one. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/atomic.h | 1 + xen/include/asm-x86/atomic.h | 39 +++ 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 41d1b6c..19a87a5 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -150,6 +150,7 @@ static inline void _atomic_set(atomic_t *v, int i) #define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) #endif /* __ARCH_ARM_ATOMIC__ */ + /* * Local variables: * mode: C diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index d246b70..5f9f2dd 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -110,7 +110,6 @@ static inline int _atomic_read(atomic_t v) return v.counter; } - /** * atomic_set - set atomic variable * @v: pointer of type atomic_t @@ -217,6 +216,25 @@ static inline void atomic_inc(atomic_t *v) } /** + * atomic_inc_and_test - increment and test + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 + * and returns true if the result is zero, or false for all + * other cases. + */ +static inline int atomic_inc_and_test(atomic_t *v) +{ +unsigned char c; + +asm volatile ( +"lock; incl %0; sete %1" +: "=m" (*(volatile int *)>counter), "=qm" (c) +: "m" (*(volatile int *)>counter) : "memory" ); +return c != 0; +} + +/** * atomic_dec - decrement atomic variable * @v: pointer of type atomic_t * @@ -250,25 +268,6 @@ static inline int atomic_dec_and_test(atomic_t *v) } /** - * atomic_inc_and_test - increment and test - * @v: pointer of type atomic_t - * - * Atomically increments @v by 1 - * and returns true if the result is zero, or false for all - * other cases. - */ -static inline int atomic_inc_and_test(atomic_t *v) -{ -unsigned char c; - -asm volatile ( -"lock; incl %0; sete %1" -: "=m" (*(volatile int *)>counter), "=qm" (c) -: "m" (*(volatile int *)>counter) : "memory" ); -return c != 0; -} - -/** * atomic_add_negative - add and test if negative * @v: pointer of type atomic_t * @i: integer value to add -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 1/5] asm-arm/atomic.h: fix arm32|arm64 macros duplication
Move duplicate macros between asm-arm/arm32/atomic.h and asm-arm/arm64/atomic.h to asm-arm/atomic.h. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/arm32/atomic.h | 11 --- xen/include/asm-arm/arm64/atomic.h | 11 --- xen/include/asm-arm/atomic.h | 11 +++ 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h index 7ec712f..be08ff1 100644 --- a/xen/include/asm-arm/arm32/atomic.h +++ b/xen/include/asm-arm/arm32/atomic.h @@ -149,17 +149,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* __ARCH_ARM_ARM32_ATOMIC__ */ /* * Local variables: diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h index b49219e..80d07bf 100644 --- a/xen/include/asm-arm/arm64/atomic.h +++ b/xen/include/asm-arm/arm64/atomic.h @@ -125,17 +125,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return c; } -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* * Local variables: diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 29ab265..41d1b6c 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -138,6 +138,17 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif +#define atomic_inc(v) atomic_add(1, v) +#define atomic_dec(v) atomic_sub(1, v) + +#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) +#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) +#define atomic_inc_return(v)(atomic_add_return(1, v)) +#define atomic_dec_return(v)(atomic_sub_return(1, v)) +#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) + +#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) + #endif /* __ARCH_ARM_ATOMIC__ */ /* * Local variables: -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 0/5] adjustments
Corneliu ZUZU (5): asm-arm/atomic.h: fix arm32|arm64 macros duplication asm-x86/atomic.h: minor: proper atomic_inc_and_test() placement asm-arm/atomic.h: reorder macros to match x86-side asm/atomic.h: common prototyping (add xen/atomic.h) fix: make atomic_read() param const xen/include/asm-arm/arm32/atomic.h | 11 --- xen/include/asm-arm/arm64/atomic.h | 11 --- xen/include/asm-arm/atomic.h | 46 +++--- xen/include/asm-x86/atomic.h | 128 +++ xen/include/xen/atomic.h | 171 + 5 files changed, 220 insertions(+), 147 deletions(-) create mode 100644 xen/include/xen/atomic.h -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)
On 7/13/2016 3:12 PM, Andrew Cooper wrote: On 13/07/16 12:23, Corneliu ZUZU wrote: +typedef struct { int counter; } atomic_t; + +#define ATOMIC_INIT(i) { (i) } + +/** + * atomic_read - read atomic variable + * @v: pointer of type atomic_t + * + * Atomically reads the value of @v. + */ +static inline int atomic_read(atomic_t *v); + +/** + * _atomic_read - read atomic variable non-atomically + * @v atomic_t + * + * Non-atomically reads the value of @v + */ +static inline int _atomic_read(atomic_t v); + +/** + * atomic_set - set atomic variable + * @v: pointer of type atomic_t + * @i: required value + * + * Atomically sets the value of @v to @i. + */ +static inline void atomic_set(atomic_t *v, int i); + +/** + * _atomic_set - set atomic variable non-atomically + * @v: pointer of type atomic_t + * @i: required value + * + * Non-atomically sets the value of @v to @i. + */ +static inline void _atomic_set(atomic_t *v, int i); + Is it ok if I also omit repeating the comments which are already @ xen/atomic.h from the arch-side asm/atomic.h ? Thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)
Hi Julien, On 7/13/2016 3:14 PM, Julien Grall wrote: Hi Corneliu, On 13/07/2016 12:23, Corneliu ZUZU wrote: Following Andrew Cooper's suggestion, create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. You forgot to mention that you moved the code from asm-arm/arm{32,64}/atomic.h to asm-arm/arm.h. Furthermore, this change should really be a separate patch. Noted, will do. Also includes a minor adjustment asm-x86/atomic.h: reorder atomic_inc_and_test() to follow after atomic_inc(). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> [...] diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 29ab265..8e8c5d1 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -138,6 +130,85 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif +#define atomic_inc_return(v)(atomic_add_return(1, v)) +#define atomic_dec_return(v)(atomic_sub_return(1, v)) + +/** + * atomic_sub_and_test - subtract value from variable and test result + * @i: integer value to subtract + * @v: pointer of type atomic_t + * + * Atomically subtracts @i from @v and returns + * true if the result is zero, or false for all + * other cases. + */ +static inline int atomic_sub_and_test(int i, atomic_t *v) +{ +return 0 == atomic_sub_return(i, v); +} Please don't use yoda condition. It's less readable and compiler have safety nowadays to prevent using "=". Oh yeah, given that Xen's compiled with -Wall -Werror that's not necessary. Ack. + +/** + * atomic_inc - increment atomic variable + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1. + */ +static inline void atomic_inc(atomic_t *v) +{ +atomic_add(1, v); +} Regards, Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)
On 7/13/2016 3:12 PM, Andrew Cooper wrote: On 13/07/16 12:23, Corneliu ZUZU wrote: Following Andrew Cooper's suggestion, create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. Also includes a minor adjustment asm-x86/atomic.h: reorder atomic_inc_and_test() to follow after atomic_inc(). Suggested-by: Andrew Cooper <andrew.coop...@citrix.com> Nice, didn't know that. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> Thanks for doing this! diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h new file mode 100644 index 000..5d5c051 --- /dev/null +++ b/xen/include/xen/atomic.h @@ -0,0 +1,155 @@ +/* + * include/xen/atomic.h + * + * Common atomic operations entities (atomic_t, function prototypes). + * Include _from_ arch-side . + * + * Copyright (c) 2016 Bitdefender S.R.L. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef __XEN_ATOMIC_H__ +#define __XEN_ATOMIC_H__ + +/* + * NB. I've pushed the volatile qualifier into the operations. This allows + * fast accessors such as _atomic_read() and _atomic_set() which don't give + * the compiler a fit. + */ I would recommend simply dropping this paragraph. Is is very out of date. Ack. +typedef struct { int counter; } atomic_t; + +#define ATOMIC_INIT(i) { (i) } + +/** + * atomic_read - read atomic variable + * @v: pointer of type atomic_t + * + * Atomically reads the value of @v. + */ +static inline int atomic_read(atomic_t *v); + +/** + * _atomic_read - read atomic variable non-atomically + * @v atomic_t + * + * Non-atomically reads the value of @v + */ +static inline int _atomic_read(atomic_t v); + +/** + * atomic_set - set atomic variable + * @v: pointer of type atomic_t + * @i: required value + * + * Atomically sets the value of @v to @i. + */ +static inline void atomic_set(atomic_t *v, int i); + +/** + * _atomic_set - set atomic variable non-atomically + * @v: pointer of type atomic_t + * @i: required value + * + * Non-atomically sets the value of @v to @i. + */ +static inline void _atomic_set(atomic_t *v, int i); + /** * atomic_cmpxchg - compare and exchange an atomic variable *... */ Ack. +static inline int atomic_cmpxchg(atomic_t *v, int old, int new); With those changes, Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> Thanks too for the prompt review, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] fix: make atomic_read() param const
This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/atomic.h | 2 +- xen/include/asm-x86/atomic.h | 2 +- xen/include/xen/atomic.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 8e8c5d1..cd86cf0 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -102,7 +102,7 @@ void __bad_atomic_size(void); * strex/ldrex monitor on some implementations. The reason we can use it for * atomic_set() is the clrex or dummy strex done on every exception return. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return *(volatile int *)>counter; } diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 9bf4bc1..a16f0f6 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -86,7 +86,7 @@ void __bad_atomic_size(void); * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(>counter); } diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 5d5c051..6cbc6d8 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -37,7 +37,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v); +static inline int atomic_read(const atomic_t *v); /** * _atomic_read - read atomic variable non-atomically -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] asm/atomic.h: common prototyping (add xen/atomic.h)
Following Andrew Cooper's suggestion, create a common-side to establish, among others, prototypes of atomic functions called from common-code. Done to avoid introducing inconsistencies between arch-side headers when we make subtle changes to one of them. Some arm-side macros had to be turned into inline functions in the process. Also includes a minor adjustment asm-x86/atomic.h: reorder atomic_inc_and_test() to follow after atomic_inc(). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/arm32/atomic.h | 11 --- xen/include/asm-arm/arm64/atomic.h | 11 --- xen/include/asm-arm/atomic.h | 89 ++--- xen/include/asm-x86/atomic.h | 49 +--- xen/include/xen/atomic.h | 155 + 5 files changed, 255 insertions(+), 60 deletions(-) create mode 100644 xen/include/xen/atomic.h diff --git a/xen/include/asm-arm/arm32/atomic.h b/xen/include/asm-arm/arm32/atomic.h index 7ec712f..be08ff1 100644 --- a/xen/include/asm-arm/arm32/atomic.h +++ b/xen/include/asm-arm/arm32/atomic.h @@ -149,17 +149,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) #define atomic_xchg(v, new) (xchg(&((v)->counter), new)) -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* __ARCH_ARM_ARM32_ATOMIC__ */ /* * Local variables: diff --git a/xen/include/asm-arm/arm64/atomic.h b/xen/include/asm-arm/arm64/atomic.h index b49219e..80d07bf 100644 --- a/xen/include/asm-arm/arm64/atomic.h +++ b/xen/include/asm-arm/arm64/atomic.h @@ -125,17 +125,6 @@ static inline int __atomic_add_unless(atomic_t *v, int a, int u) return c; } -#define atomic_inc(v) atomic_add(1, v) -#define atomic_dec(v) atomic_sub(1, v) - -#define atomic_inc_and_test(v) (atomic_add_return(1, v) == 0) -#define atomic_dec_and_test(v) (atomic_sub_return(1, v) == 0) -#define atomic_inc_return(v)(atomic_add_return(1, v)) -#define atomic_dec_return(v)(atomic_sub_return(1, v)) -#define atomic_sub_and_test(i, v) (atomic_sub_return(i, v) == 0) - -#define atomic_add_negative(i,v) (atomic_add_return(i, v) < 0) - #endif /* * Local variables: diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index 29ab265..8e8c5d1 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -2,6 +2,7 @@ #define __ARCH_ARM_ATOMIC__ #include +#include #include #include @@ -95,15 +96,6 @@ void __bad_atomic_size(void); default: __bad_atomic_size(); break;\ } \ }) - -/* - * NB. I've pushed the volatile qualifier into the operations. This allows - * fast accessors such as _atomic_read() and _atomic_set() which don't give - * the compiler a fit. - */ -typedef struct { int counter; } atomic_t; - -#define ATOMIC_INIT(i) { (i) } /* * On ARM, ordinary assignment (str instruction) doesn't clear the local @@ -138,6 +130,85 @@ static inline void _atomic_set(atomic_t *v, int i) # error "unknown ARM variant" #endif +#define atomic_inc_return(v)(atomic_add_return(1, v)) +#define atomic_dec_return(v)(atomic_sub_return(1, v)) + +/** + * atomic_sub_and_test - subtract value from variable and test result + * @i: integer value to subtract + * @v: pointer of type atomic_t + * + * Atomically subtracts @i from @v and returns + * true if the result is zero, or false for all + * other cases. + */ +static inline int atomic_sub_and_test(int i, atomic_t *v) +{ +return 0 == atomic_sub_return(i, v); +} + +/** + * atomic_inc - increment atomic variable + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1. + */ +static inline void atomic_inc(atomic_t *v) +{ +atomic_add(1, v); +} + +/** + * atomic_inc_and_test - increment and test + * @v: pointer of type atomic_t + * + * Atomically increments @v by 1 + * and returns true if the result is zero, or false for all + * other cases. + */ +static inline int atomic_inc_and_test(atomic_t *v) +{ +return 0 == atomic_add_return(1, v); +} + +/** + * atomic_dec - decrement atomic variable + * @v: pointer of type atomic_t + * + * Atomically decrements @v by 1. + */ +static inline void atomic_dec(atomic_t *v) +{ +atomic_sub(1, v); +} + +/** + * atomic_dec_and_test - decrement and test + * @v: pointer of type atomic_t + * + * Atomically decrements @v by 1 and + * returns true if the result is 0, or false for all other + * cases. + */ +static inline int at
[Xen-devel] [PATCH 0/2] adjustments
Corneliu ZUZU (2): asm/atomic.h: common prototyping (add xen/atomic.h) fix: make atomic_read() param const xen/include/asm-arm/arm32/atomic.h | 11 --- xen/include/asm-arm/arm64/atomic.h | 11 --- xen/include/asm-arm/atomic.h | 91 +++--- xen/include/asm-x86/atomic.h | 51 +--- xen/include/xen/atomic.h | 155 + 5 files changed, 257 insertions(+), 62 deletions(-) create mode 100644 xen/include/xen/atomic.h -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
On 7/9/2016 9:57 PM, Corneliu ZUZU wrote: On 7/9/2016 9:26 PM, Tamas K Lengyel wrote: diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index ae1dcb4..7663da2 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo) @@ -503,6 +504,20 @@ typedef enum __packed { SMAP_CHECK_DISABLED,/* disable the check */ } smap_check_policy_t; +/* + * Should we emulate the next matching instruction on VCPU resume + * after a vm_event? + */ +struct arch_vm_event_monitor { This should be named struct arch_vcpu_monitor. Good idea. +uint32_t emulate_flags; +struct vm_event_emul_read_data emul_read_data; This should probably get renamed as well at some point to struct monitor_emul_read_data. Ack. +struct monitor_write_data write_data; +}; + +struct arch_vm_event { +struct arch_vm_event_monitor *monitor; +}; IMHO there is not much point in defining struct arch_vm_event this way, we could just as well store the pointer to the arch_monitor directly in arch_vcpu as we do right now. I stated the reason for that in the commit message (see 3rd '*'), Jan insists it would be preferable to occupy just one pointer in arch_vcpu (which would still hold if we do as you suggest but I was wondering how probable would it be in the future for one of the paging/share vm-event subsystems to need per-vCPU resources). But personally, yeah, I would have preferred what you suggest as well.. Zuzu. Tamas, any update on this? Do you think we should think now about per-vCPU resources being occupied in the future by paging/share or leave that for later and do what you suggested? Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 02/16] x86: fix: make atomic_read() param const
On 7/12/2016 3:49 PM, Andrew Cooper wrote: On 12/07/16 11:38, Corneliu ZUZU wrote: On 7/12/2016 1:22 PM, Andrew Cooper wrote: On 12/07/16 11:11, Corneliu ZUZU wrote: If you are really feeling up to it, having a common xen/atomic.h with typedef struct { int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } and some prototypes such as: static inline int atomic_read(const atomic_t *v); would be great, but this looks like it has the possibility to turn into a rats nest. If it does, then just doubling up this code for arm is ok. ~Andrew Yes, that might be more complicated than we expect and I don't know if making code such as this common would be a good idea, usually these functions are always architecture-specific. I only suggested making the prototype common, not the implementation. As such, the issue you accidentally introduced would become a hard build failure on affected architectures, rather than a subtle build failure in common code at some point in the future. ~Andrew Oh, I see, good idea, I've just tested it and it works, what did you have in mind when you said it could cause problems? The build issues would come at some point later when someone attempts to atomic_read() a constant atomic_t in common code, when the ARM build would break. ~Andrew Ooh, no, I was asking what you meant when you said "this looks like it has the possibility to turn into a rats nest" in your first message, not the thing about hard build failure.. Ah. sorry. You would have to invert all the includes of atomic.h to include rather than , and have xen/atomic.h include asm/atomic.h towards the end, such that the common prototypes are first. I just suspect that this might not be completely trivial to untangle (of course, I could also be wrong). ~Andrew I did it the other way - included from . Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 02/16] x86: fix: make atomic_read() param const
On 7/12/2016 1:22 PM, Andrew Cooper wrote: On 12/07/16 11:11, Corneliu ZUZU wrote: If you are really feeling up to it, having a common xen/atomic.h with typedef struct { int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } and some prototypes such as: static inline int atomic_read(const atomic_t *v); would be great, but this looks like it has the possibility to turn into a rats nest. If it does, then just doubling up this code for arm is ok. ~Andrew Yes, that might be more complicated than we expect and I don't know if making code such as this common would be a good idea, usually these functions are always architecture-specific. I only suggested making the prototype common, not the implementation. As such, the issue you accidentally introduced would become a hard build failure on affected architectures, rather than a subtle build failure in common code at some point in the future. ~Andrew Oh, I see, good idea, I've just tested it and it works, what did you have in mind when you said it could cause problems? The build issues would come at some point later when someone attempts to atomic_read() a constant atomic_t in common code, when the ARM build would break. ~Andrew Ooh, no, I was asking what you meant when you said "this looks like it has the possibility to turn into a rats nest" in your first message, not the thing about hard build failure.. Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 02/16] x86: fix: make atomic_read() param const
On 7/12/2016 1:22 PM, Andrew Cooper wrote: On 12/07/16 11:11, Corneliu ZUZU wrote: If you are really feeling up to it, having a common xen/atomic.h with typedef struct { int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } and some prototypes such as: static inline int atomic_read(const atomic_t *v); would be great, but this looks like it has the possibility to turn into a rats nest. If it does, then just doubling up this code for arm is ok. ~Andrew Yes, that might be more complicated than we expect and I don't know if making code such as this common would be a good idea, usually these functions are always architecture-specific. I only suggested making the prototype common, not the implementation. As such, the issue you accidentally introduced would become a hard build failure on affected architectures, rather than a subtle build failure in common code at some point in the future. ~Andrew Oh, I see, good idea, I've just tested it and it works, what did you have in mind when you said it could cause problems? The build issues would come at some point later when someone attempts to atomic_read() a constant atomic_t in common code, when the ARM build would break. ~Andrew Sorry, could you rephrase this? When the ARM build would break (i.e. fail, I presume) because of what? Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 02/16] x86: fix: make atomic_read() param const
On 7/12/2016 12:42 PM, Andrew Cooper wrote: On 12/07/16 06:11, Corneliu ZUZU wrote: Hi Andrew, On 7/11/2016 6:18 PM, Andrew Cooper wrote: On 09/07/16 05:12, Corneliu ZUZU wrote: This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> This is a good improvement, but you must make an identical adjustment to the arm code, otherwise you will end up with subtle build failures. Right, didn't even realize it was X86-specific. It isn't x86 specific. (Which is what I presume you meant to say.) Heh, yes, depends how you look at it, I was referring precisely to (the implementation of) the function I've modified (which was X86-specific, called from common, which meant there's an ARM one as well). If you are really feeling up to it, having a common xen/atomic.h with typedef struct { int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } and some prototypes such as: static inline int atomic_read(const atomic_t *v); would be great, but this looks like it has the possibility to turn into a rats nest. If it does, then just doubling up this code for arm is ok. ~Andrew Yes, that might be more complicated than we expect and I don't know if making code such as this common would be a good idea, usually these functions are always architecture-specific. I only suggested making the prototype common, not the implementation. As such, the issue you accidentally introduced would become a hard build failure on affected architectures, rather than a subtle build failure in common code at some point in the future. ~Andrew Oh, I see, good idea, I've just tested it and it works, what did you have in mind when you said it could cause problems? Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 05/16] x86/monitor: relocate code more appropriately
On 7/12/2016 10:45 AM, Tian, Kevin wrote: From: Corneliu ZUZU [mailto:cz...@bitdefender.com] Sent: Monday, July 11, 2016 2:19 PM +static inline +void monitor_ctrlreg_adjust_traps(struct domain *d, unsigned int index) +{ +/* For now, VMX only. */ +ASSERT(cpu_has_vmx); + +/* Other CRs than CR3 are always trapped. */ +if ( VM_EVENT_X86_CR3 == index ) +vmx_vm_event_update_cr3_traps(d); [Kevin wrote]: Please add above into a hvm function instead of directly calling vmx in common file. (other arch can have it unimplemented). Then you don't need change this common code again later when other archs are added --- This is not common code, it's in arch/x86/monitor.c (?) and currently, as the above ASSERT indicates, only VMX is supported. If in the future support for SVM for example will be added, then the hvm move you suggest must be done (Jan also suggested this). Or, I only now realize, if you guys prefer doing this now I could add a vm_event_update_cr3_traps field in hvm_function_table, but BUG() in the SVM one.. The latter is desired. Instead of BUG, it makes more sense to return error on an arch which doesn't register the callback. Thanks Kevin Will do. Thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 03/16] x86/monitor: mechanical renames
On 7/11/2016 7:43 PM, Tamas K Lengyel wrote: On Sat, Jul 9, 2016 at 12:46 PM, Corneliu ZUZU <cz...@bitdefender.com> wrote: On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <cz...@bitdefender.com> wrote: Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()- don't have an 'arch_' prefix. Apply the same rule for monitor functions - originally the only two monitor functions that had an 'arch_' prefix were arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that prefix because -they had a counterpart function in common code-, that being monitor_domctl(). This should actually be the other way around - ie adding the arch_ prefix to vm_event functions that lack it. Given that the majority of the arch-specific functions called from common-code don't have an 'arch_' prefix unless they have a common counterpart, I was guessing that was the rule. It made sense in my head since I saw in that the intention of avoiding naming conflicts (i.e you can't have monitor_domctl() both on the common-side and on the arch-side, so prepend 'arch_' to the latter). I noticed you guys also 'skip' the prefix when sending patches, so that reinforced my assumption. Having the arch_ prefix is helpful to know that the function is dealing with the arch specific structs and not common. Personally I don't see much use in 'knowing that the function is dealing with the arch structs' from the call-site and you can tell that from the implementation-site just by looking at the path of its source file. Also, the code is pretty much localized in the arch directory anyway so usually one wouldn't have to go back and forth between common and arch that often. What really bothers me though is always having to read 'arch_' when spelling a function-name and also that it makes the function name longer without much benefit. Your suggestion of adding it to pretty much all functions that make up the interface to common just adds to that headache. :-D Similarly that's why we have the hvm_ prefix for functions in hvm/monitor. 'hvm_' doesn't seem to me more special than 'monitor_', for instance, but maybe that's just me. Let this also be the rule for future 'arch_' functions additions, and with this patch remove the 'arch_' prefix from the monitor functions that don't have a counterpart in common-code (all but those 2 aforementioned). Even if there are no common counter-parts to the function, the arch_ prefix should remain, so I won't be able to ack this patch. Tamas Having said the above, are you still of the same opinion? Yes, I am. While it's not a hard rule to always apply these prefix, it does make sense to have them so I don't see benefit in removing the existing prefixes. Well, for one the benefit would be not confusing developers by creating inconsistencies: what's the rule here, i.e. why isn't a function such as alloc_domain_struct prefixed w/ 'arch_' but arch_domain_create is? The reason seems to be the latter having a common counterpart while the former not, at least that's what I see being done all over the code-base. Also, I've done this before and you seemed to agree: https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57617.html (Q1). You also suggested creating arch-specific functions without the prefix: https://www.mail-archive.com/xen-devel%40lists.xen.org/msg57336.html . Why the sudden change of heart? 2ndly and obviously, removing the prefixes would make function names shorter and clearer (e.g. -read- "arch_vm_event_vcpu_unpause" and then read "vm_event_vcpu_unpause"). 3rd reason is that adding the prefix to -all- arch-specific functions called from common would mean having a lot new functions with the prefix. I'd read the prefix over and over again and at some point I'd get annoyed and say "ok, ok, it's arch_, I get it; why use this prefix so much again?". 4th reason is that the advantage of telling that the function accesses arch structures is much too little considering that idk, >50% of the codebase is arch-specific, so it doesn't provide much information, this categorization is too broad to deserve a special prefix. Whereas using the prefix only for functions that do have a common counterpart gives you the extra information that the 'operation' is only -partly- arch-specific, i.e. to see the whole picture, look @ the common-side implementation. Keep in mind that we'd also be -losing that information- if we were to apply the 'go with arch_ for all' rule.. (this could be a 5th reason) Adding arch_ prefix to the ones that don't already have one is optional, I was just pointing out that if you really feel like standardizing the naming convention, that's where I would like things to move towards to. Tamas I don't think I'd say this patch "standardizes the naming convention" but rather "fixes code that doesn't conform to the -
Re: [Xen-devel] [PATCH 02/16] x86: fix: make atomic_read() param const
Hi Andrew, On 7/11/2016 6:18 PM, Andrew Cooper wrote: On 09/07/16 05:12, Corneliu ZUZU wrote: This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> This is a good improvement, but you must make an identical adjustment to the arm code, otherwise you will end up with subtle build failures. Right, didn't even realize it was X86-specific. If you are really feeling up to it, having a common xen/atomic.h with typedef struct { int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } and some prototypes such as: static inline int atomic_read(const atomic_t *v); would be great, but this looks like it has the possibility to turn into a rats nest. If it does, then just doubling up this code for arm is ok. ~Andrew Yes, that might be more complicated than we expect and I don't know if making code such as this common would be a good idea, usually these functions are always architecture-specific. It might be better to keep them separate - they don't add much anyway since their implementation is short - than risk unexpected different behavior on a future arch. But then again I don't know much details of their implementation, so anyway, I'd surely prefer to do this kind of change in a separate patch. --- xen/include/asm-x86/atomic.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index d246b70..0b250c8 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(>counter); } @@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v) * * Non-atomically reads the value of @v */ -static inline int _atomic_read(atomic_t v) +static inline int _atomic_read(const atomic_t v) { return v.counter; } Thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
On 7/12/2016 12:27 AM, Tamas K Lengyel wrote: On Mon, Jul 11, 2016 at 2:20 PM, Corneliu ZUZU <cz...@bitdefender.com> wrote: On 7/11/2016 7:38 PM, Tamas K Lengyel wrote: diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 9a9734a..7ef30f1 100644 [snip] I keep seeing '[snip]' lately but I don't know what it means. Placeholder for "I've cut some of your patch content here to shorten the message I'm sending". diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h index 2171d04..605caf0 100644 --- a/xen/include/xen/monitor.h +++ b/xen/include/xen/monitor.h @@ -22,12 +22,15 @@ #ifndef __XEN_MONITOR_H__ #define __XEN_MONITOR_H__ -#include - -struct domain; -struct xen_domctl_monitor_op; +#include int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); + +static inline bool_t monitor_domain_initialised(const struct domain *d) +{ +return d->monitor.initialised; This should be !!d->monitor.initialised. It's the value of a bit, thus should be 0 or 1. Am I missing something? Using a bitfield is effectively doing a bitmask for the bit(s). However, returning the value after applying a bitmask is not necessarily 0/1 (ie. bool_t). For example I ran into problems with this in the past (see https://lists.xen.org/archives/html/xen-devel/2015-08/msg01948.html). Tamas The example you provided actually returns a value &-ed with a flag (bitmask), of course it returns either 0 or the flag itself :-). AFAIK a single-bit item in a bitfield (note a -bitfield-, not e.g. an unsigned int &-ed with (1<<x)) will always be either 0 or 1. I would assume as well but I'm not actually sure if that's guaranteed or if it's only compiler defined behavior. Tamas As per http://en.cppreference.com/w/c/language/bit_field . "The number of bits in a bit field (width) sets the limit to the range of values it can hold" So if it's width is 1, it can either be 0 or 1. Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
On 7/11/2016 7:38 PM, Tamas K Lengyel wrote: diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 9a9734a..7ef30f1 100644 [snip] I keep seeing '[snip]' lately but I don't know what it means. Placeholder for "I've cut some of your patch content here to shorten the message I'm sending". diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h index 2171d04..605caf0 100644 --- a/xen/include/xen/monitor.h +++ b/xen/include/xen/monitor.h @@ -22,12 +22,15 @@ #ifndef __XEN_MONITOR_H__ #define __XEN_MONITOR_H__ -#include - -struct domain; -struct xen_domctl_monitor_op; +#include int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); + +static inline bool_t monitor_domain_initialised(const struct domain *d) +{ +return d->monitor.initialised; This should be !!d->monitor.initialised. It's the value of a bit, thus should be 0 or 1. Am I missing something? Using a bitfield is effectively doing a bitmask for the bit(s). However, returning the value after applying a bitmask is not necessarily 0/1 (ie. bool_t). For example I ran into problems with this in the past (see https://lists.xen.org/archives/html/xen-devel/2015-08/msg01948.html). Tamas The example you provided actually returns a value &-ed with a flag (bitmask), of course it returns either 0 or the flag itself :-). AFAIK a single-bit item in a bitfield (note a -bitfield-, not e.g. an unsigned int &-ed with (1<
Re: [Xen-devel] [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization
This was 1/8 in the old v3-series "x86/vm-event: Adjustments & fixes". Transferring code-review from "Tian, Kevin <kevin.t...@intel.com>" (July 11, 2016). On 7/9/2016 7:12 AM, Corneliu ZUZU wrote: Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control was modified before actually calling vmx_update_cpu_exec_control(v). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/hvm/vmx/vmx.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 0776d12..0798245 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1433,8 +1433,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) if ( paging_mode_hap(v->domain) ) { /* Manage GUEST_CR3 when CR0.PE=0. */ +uint32_t old_ctls = v->arch.hvm_vmx.exec_control; uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING); + v->arch.hvm_vmx.exec_control &= ~cr3_ctls; if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) v->arch.hvm_vmx.exec_control |= cr3_ctls; @@ -1444,7 +1446,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING; -vmx_update_cpu_exec_control(v); +if ( old_ctls != v->arch.hvm_vmx.exec_control ) +vmx_update_cpu_exec_control(v); } if ( !nestedhvm_vcpu_in_guestmode(v) ) [Kevin wrote]: Acked-by: Kevin Tian <kevin.t...@intel.com> --- Thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 05/16] x86/monitor: relocate code more appropriately
This was 2/8 in the old v3-series "x86/vm-event: Adjustments & fixes". Transferring code-review from "Tian, Kevin <kevin.t...@intel.com>" (July 11, 2016) and responding... On 7/9/2016 7:15 AM, Corneliu ZUZU wrote: For readability: * Add function monitor_ctrlreg_write_data() (in x86/monitor.c) and separate handling of monitor_write_data there (previously done directly in hvm_do_resume()). * Separate enabling/disabling of CPU_BASED_CR3_LOAD_EXITING for CR3 monitor vm-events from CR0 node @ vmx_update_guest_cr(v, 0) into newly added function vmx_vm_event_update_cr3_traps(d); also, for a better interface, create generic functions monitor_ctrlreg_adjust_traps() and monitor_ctrlreg_disable_traps() (in x86/monitor.c) which deal with setting/unsetting any traps for cr-write monitor vm-events. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/hvm/hvm.c| 46 ++ xen/arch/x86/hvm/vmx/vmx.c| 59 ++--- xen/arch/x86/monitor.c| 68 +++ xen/include/asm-x86/hvm/vmx/vmx.h | 1 + xen/include/asm-x86/monitor.h | 2 ++ 5 files changed, 135 insertions(+), 41 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 7f99087..79ba185 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -473,8 +473,6 @@ void hvm_do_resume(struct vcpu *v) if ( unlikely(v->arch.vm_event) ) { -struct monitor_write_data *w = >arch.vm_event->write_data; - if ( unlikely(v->arch.vm_event->emulate_flags) ) { enum emul_kind kind = EMUL_KIND_NORMAL; @@ -492,29 +490,7 @@ void hvm_do_resume(struct vcpu *v) v->arch.vm_event->emulate_flags = 0; } -if ( w->do_write.msr ) -{ -hvm_msr_write_intercept(w->msr, w->value, 0); -w->do_write.msr = 0; -} - -if ( w->do_write.cr0 ) -{ -hvm_set_cr0(w->cr0, 0); -w->do_write.cr0 = 0; -} - -if ( w->do_write.cr4 ) -{ -hvm_set_cr4(w->cr4, 0); -w->do_write.cr4 = 0; -} - -if ( w->do_write.cr3 ) -{ -hvm_set_cr3(w->cr3, 0); -w->do_write.cr3 = 0; -} +monitor_ctrlreg_write_data(v); } /* Inject pending hw/sw trap */ @@ -2204,7 +2180,10 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer) if ( hvm_monitor_crX(CR0, value, old_value) ) { -/* The actual write will occur in hvm_do_resume(), if permitted. */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.cr0 = 1; v->arch.vm_event->write_data.cr0 = value; @@ -2306,7 +2285,10 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer) if ( hvm_monitor_crX(CR3, value, old) ) { -/* The actual write will occur in hvm_do_resume(), if permitted. */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.cr3 = 1; v->arch.vm_event->write_data.cr3 = value; @@ -2386,7 +2368,10 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer) if ( hvm_monitor_crX(CR4, value, old_cr) ) { -/* The actual write will occur in hvm_do_resume(), if permitted. */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.cr4 = 1; v->arch.vm_event->write_data.cr4 = value; @@ -3765,7 +3750,10 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, { ASSERT(v->arch.vm_event); -/* The actual write will occur in hvm_do_resume() (if permitted). */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.msr = 1; v->arch.vm_event->write_data.msr = msr; v->arch.vm_event->write_data.value = msr_content; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 0798245..7a31582 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1441,11 +1441,6 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) v->arch.hvm_vmx.exec_control |= cr3_ctls; -/* Trap CR3 update
Re: [Xen-devel] [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
On 7/11/2016 5:54 AM, Tian, Kevin wrote: what's the difference between this series and earlier one? [PATCH v3 0/8] x86/vm-event: Adjustments & fixes looks you have some patches cross-posted (e.g. 1/16)... Corneliu ZUZU (16): x86/vmx_update_guest_cr: minor optimization x86: fix: make atomic_read() param const x86/monitor: mechanical renames x86/monitor: relocate vm_event_register_write_resume() function to monitor code x86/monitor: relocate code more appropriately x86/monitor: fix: set msr_bitmap to NULL after xfree x86/vm-event: fix: call cleanup when init fails, to free partial allocs x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs arm/monitor: move d->monitor cleanup to monitor_cleanup_domain() x86/vm-event: centralize vcpu-destroy cleanup in vm-events code x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub x86/monitor: introduce writes_pending field in monitor_write_data x86/monitor: clarify separation between monitor subsys and vm-event as a whole x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail -- 2.5.0 Hi Kevin, I'm sorry about the confusion, the _older v3 series is superseded by this 16-patches one_. I thought the latter was too complex to consider as a v4 over the former, but I should have at least stated something to lead you guys to that conclusion (and to specify changes..). The patches that are still in this series from the older one, the changes and the acks are as following: - P 1/16, was 1/8 in v3 * changed since v3: nothing * acked-by in old v3: Acked-by: Kevin Tian <kevin.t...@intel.com> - P 5/16, was 2/8 in v3 * changed since v3: - title slightly changed - renames due to 3/16: arch_monitor_write_data()->monitor_ctrlreg_write_data(), write_ctrlreg_adjust_traps()->monitor_ctrlreg_adjust_traps(), write_ctrlreg_disable_traps()->monitor_ctrlreg_disable_traps() - added 'const' attribute to "struct *domain" param of vmx_vm_event_update_cr3_traps - 'index' param of monitor_ctrlreg_adjust_traps now of type 'unsigned int' * acked-by in old v3: no-one - P 15/16, was 3/8 in v3: * changed since v3 (IMO changes are too many and depends on previous patches in this 16-p-series and it would be better if the old v3 one is simply forgotten-about): - separated fields in arch_vm_event in another structure and arch_vm_event only holds a pointer to that struct (arch_vm_event->monitor->...) - see 14/16 for details - arch_vm_event.monitor freed entirely when the pending writes are committed (monitor_ctrlreg_write_data()) or when the domain/vcpu is destroyed * acked-by in old v3: no-one - P 16/16, was 4/8 in v3: * changed since v3: use ENODEV error code instead of ENOSYS * acked-by in old v3: no-one The rest of patches in v3 (P5-P8) already made it to staging. Sorry again and thanks, Zuzu C. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
On 7/9/2016 9:26 PM, Tamas K Lengyel wrote: diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index ae1dcb4..7663da2 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #define has_32bit_shinfo(d)((d)->arch.has_32bit_shinfo) @@ -503,6 +504,20 @@ typedef enum __packed { SMAP_CHECK_DISABLED,/* disable the check */ } smap_check_policy_t; +/* + * Should we emulate the next matching instruction on VCPU resume + * after a vm_event? + */ +struct arch_vm_event_monitor { This should be named struct arch_vcpu_monitor. Good idea. +uint32_t emulate_flags; +struct vm_event_emul_read_data emul_read_data; This should probably get renamed as well at some point to struct monitor_emul_read_data. Ack. +struct monitor_write_data write_data; +}; + +struct arch_vm_event { +struct arch_vm_event_monitor *monitor; +}; IMHO there is not much point in defining struct arch_vm_event this way, we could just as well store the pointer to the arch_monitor directly in arch_vcpu as we do right now. I stated the reason for that in the commit message (see 3rd '*'), Jan insists it would be preferable to occupy just one pointer in arch_vcpu (which would still hold if we do as you suggest but I was wondering how probable would it be in the future for one of the paging/share vm-event subsystems to need per-vCPU resources). But personally, yeah, I would have preferred what you suggest as well.. Zuzu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code
On 7/9/2016 9:14 PM, Tamas K Lengyel wrote: On Fri, Jul 8, 2016 at 10:14 PM, Corneliu ZUZU <cz...@bitdefender.com> wrote: vm_event_register_write_resume is part of the monitor subsystem, relocate and rename appropriately. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c | 38 ++ xen/arch/x86/vm_event.c| 37 - xen/common/vm_event.c | 2 +- xen/include/asm-arm/monitor.h | 6 ++ xen/include/asm-arm/vm_event.h | 6 -- xen/include/asm-x86/monitor.h | 2 ++ xen/include/asm-x86/vm_event.h | 2 -- 7 files changed, 47 insertions(+), 46 deletions(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 043815a..06d21b1 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -20,6 +20,7 @@ */ #include +#include I'm not sure, is this include necessary? Tamas Yes, otherwise it complains about dereferencing an incomplete-typed pointer (arch_vm_event). Zuzu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 03/16] x86/monitor: mechanical renames
On 7/9/2016 9:10 PM, Tamas K Lengyel wrote: On Fri, Jul 8, 2016 at 10:13 PM, Corneliu ZUZU <cz...@bitdefender.com> wrote: Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()- don't have an 'arch_' prefix. Apply the same rule for monitor functions - originally the only two monitor functions that had an 'arch_' prefix were arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that prefix because -they had a counterpart function in common code-, that being monitor_domctl(). This should actually be the other way around - ie adding the arch_ prefix to vm_event functions that lack it. Given that the majority of the arch-specific functions called from common-code don't have an 'arch_' prefix unless they have a common counterpart, I was guessing that was the rule. It made sense in my head since I saw in that the intention of avoiding naming conflicts (i.e you can't have monitor_domctl() both on the common-side and on the arch-side, so prepend 'arch_' to the latter). I noticed you guys also 'skip' the prefix when sending patches, so that reinforced my assumption. Having the arch_ prefix is helpful to know that the function is dealing with the arch specific structs and not common. Personally I don't see much use in 'knowing that the function is dealing with the arch structs' from the call-site and you can tell that from the implementation-site just by looking at the path of its source file. Also, the code is pretty much localized in the arch directory anyway so usually one wouldn't have to go back and forth between common and arch that often. What really bothers me though is always having to read 'arch_' when spelling a function-name and also that it makes the function name longer without much benefit. Your suggestion of adding it to pretty much all functions that make up the interface to common just adds to that headache. :-D Similarly that's why we have the hvm_ prefix for functions in hvm/monitor. 'hvm_' doesn't seem to me more special than 'monitor_', for instance, but maybe that's just me. Let this also be the rule for future 'arch_' functions additions, and with this patch remove the 'arch_' prefix from the monitor functions that don't have a counterpart in common-code (all but those 2 aforementioned). Even if there are no common counter-parts to the function, the arch_ prefix should remain, so I won't be able to ack this patch. Tamas Having said the above, are you still of the same opinion? Zuzu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
On 7/9/2016 8:34 PM, Tamas K Lengyel wrote: diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index aeee435..4a29cad 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -62,6 +62,8 @@ int monitor_init_domain(struct domain *d) return -ENOMEM; } +d->monitor.initialised = 1; + return 0; } @@ -78,6 +80,7 @@ void monitor_cleanup_domain(struct domain *d) memset(>arch.monitor, 0, sizeof(d->arch.monitor)); memset(>monitor, 0, sizeof(d->monitor)); +d->monitor.initialised = 0; So d->monitor is already memset to zero above, thus this is not needed here. Yep. } void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp) diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 9a9734a..7ef30f1 100644 [snip] I keep seeing '[snip]' lately but I don't know what it means. diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h index 2171d04..605caf0 100644 --- a/xen/include/xen/monitor.h +++ b/xen/include/xen/monitor.h @@ -22,12 +22,15 @@ #ifndef __XEN_MONITOR_H__ #define __XEN_MONITOR_H__ -#include - -struct domain; -struct xen_domctl_monitor_op; +#include int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); + +static inline bool_t monitor_domain_initialised(const struct domain *d) +{ +return d->monitor.initialised; This should be !!d->monitor.initialised. It's the value of a bit, thus should be 0 or 1. Am I missing something? +} + void monitor_guest_request(void); int monitor_traps(struct vcpu *v, bool_t sync, vm_event_request_t *req); Cheers, Tamas Thanks, Zuzu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail
On 7/9/2016 7:23 AM, Corneliu ZUZU wrote: Enforce presence of a monitor vm-event subscriber when the toolstack user calls xc_monitor_write_ctrlreg() (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl). Without this change, "ASSERT(monitor_domain_initialised(v->domain));" @ hvm_set_cr0() and such would fail if the toolstack user calls xc_monitor_write_ctrlreg(...) w/ enable = true, without first calling xc_monitor_enable(). Also adjust returned error code for similar check from -EINVAL to more descriptive -ENODEV (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c| 4 xen/include/asm-x86/monitor.h | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 05a2f0d..4cf018a 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -324,6 +324,10 @@ int arch_monitor_domctl_event(struct domain *d, unsigned int ctrlreg_bitmask; bool_t old_status; +/* Meaningless without a monitor vm-events subscriber. */ +if ( unlikely(!monitor_domain_initialised(d)) ) +return -ENODEV; + /* sanity check: avoid left-shift undefined behavior */ if ( unlikely(mop->u.mov_to_cr.index > 31) ) return -EINVAL; diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 11497ef..a6022db 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -47,7 +47,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) if ( likely(monitor_domain_initialised(d)) ) d->arch.mem_access_emulate_each_rep = !!mop->event; else -rc = -EINVAL; +rc = -ENODEV; domain_unpause(d); break; I might have forgotten to think about domain pausing (for all patches), where it needs to be done. I'll leave that for v2 (obviously), I just wanted to let you know in case you guys have feedback on the matter until then. Zuzu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 16/16] x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail
Enforce presence of a monitor vm-event subscriber when the toolstack user calls xc_monitor_write_ctrlreg() (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl). Without this change, "ASSERT(monitor_domain_initialised(v->domain));" @ hvm_set_cr0() and such would fail if the toolstack user calls xc_monitor_write_ctrlreg(...) w/ enable = true, without first calling xc_monitor_enable(). Also adjust returned error code for similar check from -EINVAL to more descriptive -ENODEV (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c| 4 xen/include/asm-x86/monitor.h | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 05a2f0d..4cf018a 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -324,6 +324,10 @@ int arch_monitor_domctl_event(struct domain *d, unsigned int ctrlreg_bitmask; bool_t old_status; +/* Meaningless without a monitor vm-events subscriber. */ +if ( unlikely(!monitor_domain_initialised(d)) ) +return -ENODEV; + /* sanity check: avoid left-shift undefined behavior */ if ( unlikely(mop->u.mov_to_cr.index > 31) ) return -EINVAL; diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 11497ef..a6022db 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -47,7 +47,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) if ( likely(monitor_domain_initialised(d)) ) d->arch.mem_access_emulate_each_rep = !!mop->event; else -rc = -EINVAL; +rc = -ENODEV; domain_unpause(d); break; -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 15/16] x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes
The arch_vm_event.monitor structure is dynamically allocated and freed e.g. when the toolstack user calls xc_monitor_disable(), which in turn effectively discards any information that was in arch_vm_event.monitor->write_data. But this can yield unexpected behavior since if a CR-write was awaiting to be committed (non-zero write_data.writes_pending) on the scheduling tail (hvm_do_resume()->monitor_ctrlreg_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.monitor->emul_read_data dynamically allocated and only frees the entire arch_vm_event.monitor in monitor_cleanup_domain() if there are no pending CR-writes, otherwise it only frees emul_read_data * if there were pending CR-writes when monitor_cleanup_domain() was called, arch_vm_event.monitor will eventually be freed either after the CR-writes are committed (monitor_ctrlreg_write_data()) or when the domain/vcpu is destroyed * calls monitor_ctrlreg_write_data() even if the monitor subsystem is not initialized Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/hvm/emulate.c | 4 ++-- xen/arch/x86/hvm/hvm.c | 7 ++- xen/arch/x86/mm/p2m.c| 2 +- xen/arch/x86/monitor.c | 49 +++- xen/include/asm-x86/domain.h | 3 +-- 5 files changed, 54 insertions(+), 11 deletions(-) diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 7a9c6af..e08d9c6 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -77,9 +77,9 @@ static int set_context_data(void *buffer, unsigned int size) if ( unlikely(monitor_domain_initialised(curr->domain)) ) { struct arch_vm_event_monitor *vem = curr->arch.vm_event->monitor; -unsigned int safe_size = min(size, vem->emul_read_data.size); +unsigned int safe_size = min(size, vem->emul_read_data->size); -memcpy(buffer, vem->emul_read_data.data, safe_size); +memcpy(buffer, vem->emul_read_data->data, safe_size); memset(buffer + safe_size, 0, size - safe_size); return X86EMUL_OKAY; } diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 1bfd9d4..a2568fd 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -488,9 +488,14 @@ void hvm_do_resume(struct vcpu *v) vem->emulate_flags = 0; } +} +/* + * Handle pending monitor CR-writes. Note that the monitor subsystem + * might be uninitialized. + */ +if ( unlikely(v->arch.vm_event && v->arch.vm_event->monitor) ) monitor_ctrlreg_write_data(v); -} /* Inject pending hw/sw trap */ if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 92c0576..b280dc0 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1645,7 +1645,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v, vem->emulate_flags = violation ? rsp->flags : 0; if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) ) -vem->emul_read_data = rsp->data.emul_read_data; +*vem->emul_read_data = rsp->data.emul_read_data; } } diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index c3123bc..05a2f0d 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -56,6 +56,7 @@ int monitor_init_domain(struct domain *d) { int rc = 0; struct vcpu *v; +struct arch_vm_event_monitor *vem; /* Already initialised? */ if ( unlikely(monitor_domain_initialised(d)) ) @@ -65,10 +66,22 @@ int monitor_init_domain(struct domain *d) for_each_vcpu(d, v) { ASSERT(v->arch.vm_event); -ASSERT(!v->arch.vm_event->monitor); -v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor); -if ( unlikely(!v->arch.vm_event->monitor) ) +if ( likely(!v->arch.vm_event->monitor) ) +{ +v->arch.vm_event->monitor = xzalloc(struct arch_vm_event_monitor); +if ( unlikely(!v->arch.vm_event->monitor) ) +{ +rc = -ENOMEM; +goto err; +} +} + +vem = v->arch.vm_event->monitor; + +ASSERT(!vem->emul_read_data); +vem->emul_read_data = xzalloc(struct vm_event_emul_read_data); +if ( unlikely(!vem->emul_read_data) ) { rc = -ENOMEM; goto err; @@ -100,6 +113,7 @@ err: void monitor_cleanup_domain(struct domain *d) { struct vcpu *v; +struct arch_vm_event_monitor *vem; xfree(d->arch.monitor.msr_bitmap); d->arch.monitor.msr_bitmap = NULL; @@ -109,8 +123,22 @@ void monitor_cleanup_domai
[Xen-devel] [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
Move fields in arch_vm_event in a structure called arch_vm_event_monitor and refactor arch_vm_event to only hold a pointer to an instance of the aforementioned. Done for a number of reasons: * to make the separation of monitor resources from other vm-event resources clearly visible * to be able to selectively allocate/free monitor-only resources in monitor init & cleanup stubs * did _not_ do a v->arch.vm_event -to- v->arch.monitor transformation because we want the guarantee of not occupying more than 1 pointer in arch_vcpu, i.e. if in the future e.g. paging vm-events would need per-vcpu resources, one would make an arch_vm_event_paging structure and add a pointer to an instance of it as a arch_vm_event.paging field, in which case we will still have only 1 pointer (arch_vm_event) consuming memory in arch_vcpu for vm-event resources Note also that this breaks the old implication 'vm-event initialized -> monitor resources (which were in arch_vm_event) initialized', so 2 extra checks on monitor_domain_initialised() had to be added: i) in vm_event_resume() before calling monitor_ctrlreg_write_resume(), also an ASSERT in monitor_ctrlreg_write_resume() ii) in vm_event_resume() before calling mem_access_resume(), also an ASSERT on that code-path in p2m_mem_access_emulate_check() Finally, also note that the definition of arch_vm_event had to be moved to asm-x86/domain.h due to a cyclic header dependency it caused between asm-x86/vm_event.h and asm-x86/monitor.h. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/hvm/emulate.c | 8 +++--- xen/arch/x86/hvm/hvm.c | 31 xen/arch/x86/mm/p2m.c | 7 -- xen/arch/x86/monitor.c | 55 -- xen/arch/x86/vm_event.c| 17 +++-- xen/common/vm_event.c | 17 + xen/include/asm-x86/domain.h | 15 xen/include/asm-x86/monitor.h | 7 ++ xen/include/asm-x86/vm_event.h | 13 +++--- 9 files changed, 128 insertions(+), 42 deletions(-) diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index a0094e9..7a9c6af 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -76,10 +76,10 @@ static int set_context_data(void *buffer, unsigned int size) if ( unlikely(monitor_domain_initialised(curr->domain)) ) { -unsigned int safe_size = -min(size, curr->arch.vm_event->emul_read_data.size); +struct arch_vm_event_monitor *vem = curr->arch.vm_event->monitor; +unsigned int safe_size = min(size, vem->emul_read_data.size); -memcpy(buffer, curr->arch.vm_event->emul_read_data.data, safe_size); +memcpy(buffer, vem->emul_read_data.data, safe_size); memset(buffer + safe_size, 0, size - safe_size); return X86EMUL_OKAY; } @@ -524,7 +524,7 @@ static int hvmemul_virtual_to_linear( * vm_event being triggered for repeated writes to a whole page. */ if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) && - current->arch.vm_event->emulate_flags != 0 ) + current->arch.vm_event->monitor->emulate_flags != 0 ) max_reps = 1; /* diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 46fed33..1bfd9d4 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -65,7 +65,6 @@ #include #include #include -#include #include #include #include @@ -473,21 +472,21 @@ void hvm_do_resume(struct vcpu *v) if ( unlikely(monitor_domain_initialised(v->domain)) ) { -if ( unlikely(v->arch.vm_event->emulate_flags) ) +struct arch_vm_event_monitor *vem = v->arch.vm_event->monitor; + +if ( unlikely(vem->emulate_flags) ) { enum emul_kind kind = EMUL_KIND_NORMAL; -if ( v->arch.vm_event->emulate_flags & - VM_EVENT_FLAG_SET_EMUL_READ_DATA ) +if ( vem->emulate_flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA ) kind = EMUL_KIND_SET_CONTEXT; -else if ( v->arch.vm_event->emulate_flags & - VM_EVENT_FLAG_EMULATE_NOWRITE ) +else if ( vem->emulate_flags & VM_EVENT_FLAG_EMULATE_NOWRITE ) kind = EMUL_KIND_NOWRITE; hvm_mem_access_emulate_one(kind, TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE); -v->arch.vm_event->emulate_flags = 0; +vem->emulate_flags = 0; } monitor_ctrlreg_write_data(v); @@ -2184,8 +2183,8 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer) * The actual write will occur in monitor_ctrlreg_write_data(), if * permitted. */ -v->arch.vm_event->write_dat
[Xen-devel] [PATCH 13/16] x86/monitor: introduce writes_pending field in monitor_write_data
Introduce writes_pending field in monitor_write_data structure to slightly optimize code @ monitor_write_data(): avoid having to check each bit when -all- bits are zero (which is likely). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c | 3 +++ xen/include/asm-x86/domain.h | 17 +++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index c558f46..0763ed7 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -127,6 +127,9 @@ void monitor_ctrlreg_write_data(struct vcpu *v) { struct monitor_write_data *w = >arch.vm_event->write_data; +if ( likely(!w->writes_pending) ) +return; + if ( w->do_write.msr ) { hvm_msr_write_intercept(w->msr, w->value, 0); diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 8f64ae9..ae1dcb4 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -260,12 +260,17 @@ struct pv_domain }; struct monitor_write_data { -struct { -unsigned int msr : 1; -unsigned int cr0 : 1; -unsigned int cr3 : 1; -unsigned int cr4 : 1; -} do_write; +union { +struct { +unsigned int msr : 1; +unsigned int cr0 : 1; +unsigned int cr3 : 1; +unsigned int cr4 : 1; +} do_write; + +/* Non-zero when at least one of do_write fields is 1. */ +unsigned int writes_pending; +}; uint32_t msr; uint64_t value; -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 12/16] x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub
Move cleanup of mem_access_emulate_each_rep to monitor_cleanup_domain() as the field is part of the monitor subsystem's resources. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c | 3 +++ xen/arch/x86/vm_event.c | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 4a29cad..c558f46 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -80,6 +80,9 @@ void monitor_cleanup_domain(struct domain *d) memset(>arch.monitor, 0, sizeof(d->arch.monitor)); memset(>monitor, 0, sizeof(d->monitor)); + +d->arch.mem_access_emulate_each_rep = 0; + d->monitor.initialised = 0; } diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index bb9c0a0..e2b258b 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -68,8 +68,6 @@ void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved) /* Per-vcpu uninitializations. */ for_each_vcpu ( d, v ) vm_event_cleanup_vcpu_destroy(v); - -d->arch.mem_access_emulate_each_rep = 0; } void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v) -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 11/16] x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys
As it can be seen looking @ the vm_event_per_domain structure, there are 3 defined vm-event subsystems: share, paging and monitor. In a number of places in the codebase, the monitor vm-events subsystem is mistakenly confounded with the vm-event subsystem as a whole: i.e. it is wrongfully checked if v->arch.vm_event is allocated to determine if the monitor subsystem is initialized. To fix that, add an 'initialised' bit in d->monitor which will determine if the monitor subsystem is initialised, create an inline stub monitor_domain_initialised() and use that instead where it applies. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/hvm/emulate.c| 3 ++- xen/arch/x86/hvm/hvm.c| 10 +- xen/arch/x86/monitor.c| 3 +++ xen/include/asm-arm/monitor.h | 3 ++- xen/include/asm-x86/monitor.h | 9 - xen/include/xen/monitor.h | 11 +++ xen/include/xen/sched.h | 1 + 7 files changed, 24 insertions(+), 16 deletions(-) diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 855af4d..a0094e9 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -73,7 +74,7 @@ static int set_context_data(void *buffer, unsigned int size) { struct vcpu *curr = current; -if ( curr->arch.vm_event ) +if ( unlikely(monitor_domain_initialised(curr->domain)) ) { unsigned int safe_size = min(size, curr->arch.vm_event->emul_read_data.size); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 79ba185..46fed33 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -471,7 +471,7 @@ void hvm_do_resume(struct vcpu *v) if ( !handle_hvm_io_completion(v) ) return; -if ( unlikely(v->arch.vm_event) ) +if ( unlikely(monitor_domain_initialised(v->domain)) ) { if ( unlikely(v->arch.vm_event->emulate_flags) ) { @@ -2176,7 +2176,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer) if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR0)) ) { -ASSERT(v->arch.vm_event); +ASSERT(monitor_domain_initialised(v->domain)); if ( hvm_monitor_crX(CR0, value, old_value) ) { @@ -2281,7 +2281,7 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer) if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) ) { -ASSERT(v->arch.vm_event); +ASSERT(monitor_domain_initialised(v->domain)); if ( hvm_monitor_crX(CR3, value, old) ) { @@ -2364,7 +2364,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer) if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR4)) ) { -ASSERT(v->arch.vm_event); +ASSERT(monitor_domain_initialised(v->domain)); if ( hvm_monitor_crX(CR4, value, old_cr) ) { @@ -3748,7 +3748,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, if ( may_defer && unlikely(monitored_msr(v->domain, msr)) ) { -ASSERT(v->arch.vm_event); +ASSERT(monitor_domain_initialised(v->domain)); /* * The actual write will occur in monitor_ctrlreg_write_data(), if diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index aeee435..4a29cad 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -62,6 +62,8 @@ int monitor_init_domain(struct domain *d) return -ENOMEM; } +d->monitor.initialised = 1; + return 0; } @@ -78,6 +80,7 @@ void monitor_cleanup_domain(struct domain *d) memset(>arch.monitor, 0, sizeof(d->arch.monitor)); memset(>monitor, 0, sizeof(d->monitor)); +d->monitor.initialised = 0; } void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp) diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 9a9734a..7ef30f1 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -48,13 +48,14 @@ int arch_monitor_domctl_event(struct domain *d, static inline int monitor_init_domain(struct domain *d) { -/* No arch-specific domain initialization on ARM. */ +d->monitor.initialised = 1; return 0; } static inline void monitor_cleanup_domain(struct domain *d) { memset(>monitor, 0, sizeof(d->monitor)); +d->monitor.initialised = 0; } static inline diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 3ae24dd..4014f8d 100644 --- a/xe
[Xen-devel] [PATCH 10/16] x86/vm-event: centralize vcpu-destroy cleanup in vm-events code
Move vm-event cleanup sequence on vcpu destroyal to vm_event.h along with the other vm-event functions. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/domain.c | 4 ++-- xen/arch/x86/vm_event.c| 5 + xen/include/asm-x86/vm_event.h | 10 ++ 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index bb59247..2a702be 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -56,6 +56,7 @@ #include #include #include +#include #include #include #include @@ -492,8 +493,7 @@ int vcpu_initialise(struct vcpu *v) void vcpu_destroy(struct vcpu *v) { -xfree(v->arch.vm_event); -v->arch.vm_event = NULL; +vm_event_cleanup_vcpu_destroy(v); if ( is_pv_32bit_vcpu(v) ) { diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index e795e5e..bb9c0a0 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -67,10 +67,7 @@ void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved) /* Per-vcpu uninitializations. */ for_each_vcpu ( d, v ) -{ -xfree(v->arch.vm_event); -v->arch.vm_event = NULL; -} +vm_event_cleanup_vcpu_destroy(v); d->arch.mem_access_emulate_each_rep = 0; } diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h index 934f385..c1b82ab 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -35,6 +35,16 @@ int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved); void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved); +/* Called on vCPU destroy. */ +static inline void vm_event_cleanup_vcpu_destroy(struct vcpu *v) +{ +if ( likely(!v->arch.vm_event) ) +return; + +xfree(v->arch.vm_event); +v->arch.vm_event = NULL; +} + void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v); void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp); -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 09/16] arm/monitor: move d->monitor cleanup to monitor_cleanup_domain()
d->monitor is a monitor subsystem resource, clean it up in the proper stub. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-arm/monitor.h | 2 +- xen/include/asm-arm/vm_event.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 5a0fc65..9a9734a 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -54,7 +54,7 @@ static inline int monitor_init_domain(struct domain *d) static inline void monitor_cleanup_domain(struct domain *d) { -/* No arch-specific domain cleanup on ARM. */ +memset(>monitor, 0, sizeof(d->monitor)); } static inline diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h index 93fc4db..3a8f585 100644 --- a/xen/include/asm-arm/vm_event.h +++ b/xen/include/asm-arm/vm_event.h @@ -39,8 +39,6 @@ void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved) /* Uninitialize specified subsystem. */ if ( >vm_event->monitor == ved ) monitor_cleanup_domain(d); - -memset(>monitor, 0, sizeof(d->monitor)); } static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v) -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 08/16] x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs
For clarity: do init/cleanup of the vm-events subsystems (e.g. functions monitor_init,cleanup_domain()) from the vm-event_init,cleanup_domain() functions. Done by passing-on the ved param from vm_event_enable,disable() functions down to the vm_event_init,cleanup_domain() functions. This makes it easier to follow the init/cleanup sequences for all vm-events subsystems. In the process, we also replace the vec param of vm_event_enable() with a port param, since the op/mode fields of vec are already handled before the function is called. This was also done because conceptually it was strange relying on the ved param in vm_event_init,cleanup_domain() to determine the target vm-event subsystem when we already had vec->mode in vm_event_enable(). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c | 5 + xen/arch/x86/vm_event.c| 15 --- xen/common/vm_event.c | 18 +++--- xen/include/asm-arm/vm_event.h | 15 --- xen/include/asm-x86/vm_event.h | 4 ++-- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 8f41f21..aeee435 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -52,6 +52,7 @@ static inline void monitor_ctrlreg_disable_traps(struct domain *d) } } +/* Implicitly serialized by the domctl lock. */ int monitor_init_domain(struct domain *d) { if ( likely(!d->arch.monitor.msr_bitmap) ) @@ -64,6 +65,10 @@ int monitor_init_domain(struct domain *d) return 0; } +/* + * Implicitly serialized by the domctl lock, + * or on domain cleanup paths only. + */ void monitor_cleanup_domain(struct domain *d) { xfree(d->arch.monitor.msr_bitmap); diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 22c63ea..e795e5e 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -18,10 +18,11 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ +#include #include /* Implicitly serialized by the domctl lock. */ -int vm_event_init_domain(struct domain *d) +int vm_event_init_domain(struct domain *d, struct vm_event_domain *ved) { int rc = 0; struct vcpu *v; @@ -40,10 +41,14 @@ int vm_event_init_domain(struct domain *d) } } +/* Initialize specified subsystem. */ +if ( >vm_event->monitor == ved ) +rc = monitor_init_domain(d); + err: if ( unlikely(rc) ) /* On fail cleanup whatever resources have been partially allocated. */ -vm_event_cleanup_domain(d); +vm_event_cleanup_domain(d, ved); return rc; } @@ -52,10 +57,14 @@ err: * Implicitly serialized by the domctl lock, * or on domain cleanup paths only. */ -void vm_event_cleanup_domain(struct domain *d) +void vm_event_cleanup_domain(struct domain *d, struct vm_event_domain *ved) { struct vcpu *v; +/* Uninitialize specified subsystem. */ +if ( >vm_event->monitor == ved ) +monitor_cleanup_domain(d); + /* Per-vcpu uninitializations. */ for_each_vcpu ( d, v ) { diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index e60e1f2..dafe7bf 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -41,8 +41,8 @@ static int vm_event_enable( struct domain *d, -xen_domctl_vm_event_op_t *vec, struct vm_event_domain *ved, +unsigned long xen_port, int pause_flag, int param, xen_event_channel_notification_t notification_fn) @@ -64,7 +64,7 @@ static int vm_event_enable( vm_event_ring_lock_init(ved); vm_event_ring_lock(ved); -rc = vm_event_init_domain(d); +rc = vm_event_init_domain(d, ved); if ( rc < 0 ) goto err; @@ -83,7 +83,7 @@ static int vm_event_enable( if ( rc < 0 ) goto err; -ved->xen_port = vec->port = rc; +ved->xen_port = xen_port = rc; /* Prepare ring buffer */ FRONT_RING_INIT(>front_ring, @@ -232,7 +232,7 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain *ved) destroy_ring_for_helper(>ring_page, ved->ring_pg_struct); -vm_event_cleanup_domain(d); +vm_event_cleanup_domain(d, ved); vm_event_ring_unlock(ved); } @@ -630,7 +630,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, break; /* domain_pause() not required here, see XSA-99 */ -rc = vm_event_enable(d, vec, ved, _VPF_mem_paging, +rc = vm_event_enable(d, ved, vec->port, _VPF_mem_paging, HVM_PARAM_PAGING_RING_PFN, mem_paging_notification); } @@ -669,10 +669,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, { case XEN_VM_EVENT_ENABLE: /* domain_pause() not required
[Xen-devel] [PATCH 07/16] x86/vm-event: fix: call cleanup when init fails, to free partial allocs
From vm_event_init_domain(d), call vm_event_cleanup_domain(d) if per-vcpu allocations failed -at some point- to cleanup partial allocations, if any were done along the way. Also minor adjustments: add unlikely in if's and add some comments. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/vm_event.c | 20 +++- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 6e19df8..22c63ea 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -23,20 +23,29 @@ /* Implicitly serialized by the domctl lock. */ int vm_event_init_domain(struct domain *d) { +int rc = 0; struct vcpu *v; +/* Per-vcpu initializations. */ for_each_vcpu ( d, v ) { -if ( v->arch.vm_event ) +if ( unlikely(v->arch.vm_event) ) continue; v->arch.vm_event = xzalloc(struct arch_vm_event); - -if ( !v->arch.vm_event ) -return -ENOMEM; +if ( unlikely(!v->arch.vm_event) ) +{ +rc = -ENOMEM; +goto err; +} } -return 0; +err: +if ( unlikely(rc) ) +/* On fail cleanup whatever resources have been partially allocated. */ +vm_event_cleanup_domain(d); + +return rc; } /* @@ -47,6 +56,7 @@ void vm_event_cleanup_domain(struct domain *d) { struct vcpu *v; +/* Per-vcpu uninitializations. */ for_each_vcpu ( d, v ) { xfree(v->arch.vm_event); -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 06/16] x86/monitor: fix: set msr_bitmap to NULL after xfree
Fix: set d->arch.monitor.msr_bitmap to NULL after xfree, as the equivalence of it being NULL and xfreed is repeatedly presumed in the codebase. Along with this change, also properly reposition an 'if' targeting the aforementioned msr_bitmap when it is allocated and add likely/unlikely accordingly. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 29188e5..8f41f21 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -54,11 +54,12 @@ static inline void monitor_ctrlreg_disable_traps(struct domain *d) int monitor_init_domain(struct domain *d) { -if ( !d->arch.monitor.msr_bitmap ) +if ( likely(!d->arch.monitor.msr_bitmap) ) +{ d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap); - -if ( !d->arch.monitor.msr_bitmap ) -return -ENOMEM; +if ( unlikely(!d->arch.monitor.msr_bitmap) ) +return -ENOMEM; +} return 0; } @@ -66,6 +67,7 @@ int monitor_init_domain(struct domain *d) void monitor_cleanup_domain(struct domain *d) { xfree(d->arch.monitor.msr_bitmap); +d->arch.monitor.msr_bitmap = NULL; monitor_ctrlreg_disable_traps(d); -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 05/16] x86/monitor: relocate code more appropriately
For readability: * Add function monitor_ctrlreg_write_data() (in x86/monitor.c) and separate handling of monitor_write_data there (previously done directly in hvm_do_resume()). * Separate enabling/disabling of CPU_BASED_CR3_LOAD_EXITING for CR3 monitor vm-events from CR0 node @ vmx_update_guest_cr(v, 0) into newly added function vmx_vm_event_update_cr3_traps(d); also, for a better interface, create generic functions monitor_ctrlreg_adjust_traps() and monitor_ctrlreg_disable_traps() (in x86/monitor.c) which deal with setting/unsetting any traps for cr-write monitor vm-events. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/hvm/hvm.c| 46 ++ xen/arch/x86/hvm/vmx/vmx.c| 59 ++--- xen/arch/x86/monitor.c| 68 +++ xen/include/asm-x86/hvm/vmx/vmx.h | 1 + xen/include/asm-x86/monitor.h | 2 ++ 5 files changed, 135 insertions(+), 41 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 7f99087..79ba185 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -473,8 +473,6 @@ void hvm_do_resume(struct vcpu *v) if ( unlikely(v->arch.vm_event) ) { -struct monitor_write_data *w = >arch.vm_event->write_data; - if ( unlikely(v->arch.vm_event->emulate_flags) ) { enum emul_kind kind = EMUL_KIND_NORMAL; @@ -492,29 +490,7 @@ void hvm_do_resume(struct vcpu *v) v->arch.vm_event->emulate_flags = 0; } -if ( w->do_write.msr ) -{ -hvm_msr_write_intercept(w->msr, w->value, 0); -w->do_write.msr = 0; -} - -if ( w->do_write.cr0 ) -{ -hvm_set_cr0(w->cr0, 0); -w->do_write.cr0 = 0; -} - -if ( w->do_write.cr4 ) -{ -hvm_set_cr4(w->cr4, 0); -w->do_write.cr4 = 0; -} - -if ( w->do_write.cr3 ) -{ -hvm_set_cr3(w->cr3, 0); -w->do_write.cr3 = 0; -} +monitor_ctrlreg_write_data(v); } /* Inject pending hw/sw trap */ @@ -2204,7 +2180,10 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer) if ( hvm_monitor_crX(CR0, value, old_value) ) { -/* The actual write will occur in hvm_do_resume(), if permitted. */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.cr0 = 1; v->arch.vm_event->write_data.cr0 = value; @@ -2306,7 +2285,10 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer) if ( hvm_monitor_crX(CR3, value, old) ) { -/* The actual write will occur in hvm_do_resume(), if permitted. */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.cr3 = 1; v->arch.vm_event->write_data.cr3 = value; @@ -2386,7 +2368,10 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer) if ( hvm_monitor_crX(CR4, value, old_cr) ) { -/* The actual write will occur in hvm_do_resume(), if permitted. */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.cr4 = 1; v->arch.vm_event->write_data.cr4 = value; @@ -3765,7 +3750,10 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, { ASSERT(v->arch.vm_event); -/* The actual write will occur in hvm_do_resume() (if permitted). */ +/* + * The actual write will occur in monitor_ctrlreg_write_data(), if + * permitted. + */ v->arch.vm_event->write_data.do_write.msr = 1; v->arch.vm_event->write_data.msr = msr; v->arch.vm_event->write_data.value = msr_content; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 0798245..7a31582 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1441,11 +1441,6 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) v->arch.hvm_vmx.exec_control |= cr3_ctls; -/* Trap CR3 updates if CR3 memory events are enabled. */ -if ( v->domain->arch.monitor.write_ctrlreg_enabled & - monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) -v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING; - if ( old_ctls != v->arch.hvm_vmx.exec
[Xen-devel] [PATCH 04/16] x86/monitor: relocate vm_event_register_write_resume() function to monitor code
vm_event_register_write_resume is part of the monitor subsystem, relocate and rename appropriately. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c | 38 ++ xen/arch/x86/vm_event.c| 37 - xen/common/vm_event.c | 2 +- xen/include/asm-arm/monitor.h | 6 ++ xen/include/asm-arm/vm_event.h | 6 -- xen/include/asm-x86/monitor.h | 2 ++ xen/include/asm-x86/vm_event.h | 2 -- 7 files changed, 47 insertions(+), 46 deletions(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 043815a..06d21b1 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -20,6 +20,7 @@ */ #include +#include #include int monitor_init_domain(struct domain *d) @@ -41,6 +42,43 @@ void monitor_cleanup_domain(struct domain *d) memset(>monitor, 0, sizeof(d->monitor)); } +void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp) +{ +if ( rsp->flags & VM_EVENT_FLAG_DENY ) +{ +struct monitor_write_data *w; + +ASSERT(v->arch.vm_event); + +/* deny flag requires the vCPU to be paused */ +if ( !atomic_read(>vm_event_pause_count) ) +return; + +w = >arch.vm_event->write_data; + +switch ( rsp->reason ) +{ +case VM_EVENT_REASON_MOV_TO_MSR: +w->do_write.msr = 0; +break; +case VM_EVENT_REASON_WRITE_CTRLREG: +switch ( rsp->u.write_ctrlreg.index ) +{ +case VM_EVENT_X86_CR0: +w->do_write.cr0 = 0; +break; +case VM_EVENT_X86_CR3: +w->do_write.cr3 = 0; +break; +case VM_EVENT_X86_CR4: +w->do_write.cr4 = 0; +break; +} +break; +} +} +} + static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr) { ASSERT(d->arch.monitor.msr_bitmap && msr); diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index e938ca3..6e19df8 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -66,43 +66,6 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v) hvm_toggle_singlestep(v); } -void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp) -{ -if ( rsp->flags & VM_EVENT_FLAG_DENY ) -{ -struct monitor_write_data *w; - -ASSERT(v->arch.vm_event); - -/* deny flag requires the vCPU to be paused */ -if ( !atomic_read(>vm_event_pause_count) ) -return; - -w = >arch.vm_event->write_data; - -switch ( rsp->reason ) -{ -case VM_EVENT_REASON_MOV_TO_MSR: -w->do_write.msr = 0; -break; -case VM_EVENT_REASON_WRITE_CTRLREG: -switch ( rsp->u.write_ctrlreg.index ) -{ -case VM_EVENT_X86_CR0: -w->do_write.cr0 = 0; -break; -case VM_EVENT_X86_CR3: -w->do_write.cr3 = 0; -break; -case VM_EVENT_X86_CR4: -w->do_write.cr4 = 0; -break; -} -break; -} -} -} - void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp) { ASSERT(atomic_read(>vm_event_pause_count)); diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index b4f9fd3..e60e1f2 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -397,7 +397,7 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved) case VM_EVENT_REASON_MOV_TO_MSR: #endif case VM_EVENT_REASON_WRITE_CTRLREG: -vm_event_register_write_resume(v, ); +monitor_ctrlreg_write_resume(v, ); break; #ifdef CONFIG_HAS_MEM_ACCESS diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index c72ee42..5a0fc65 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -57,6 +57,12 @@ static inline void monitor_cleanup_domain(struct domain *d) /* No arch-specific domain cleanup on ARM. */ } +static inline +void monitor_ctrlreg_write_resume(struct vcpu *v, vm_event_response_t *rsp) +{ +/* Not supported on ARM. */ +} + static inline uint32_t monitor_get_capabilities(struct domain *d) { uint32_t capabilities = 0; diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h index ccc4b60..0129b04 100644 --- a/xen/include/asm-arm/vm_event.h +++ b/xen/include/asm-arm/vm_event.h @@ -40,12 +40,6 @@ static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v) } static inline -void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp) -{ -/* Not supported on ARM. */ -} - -static in
[Xen-devel] [PATCH 03/16] x86/monitor: mechanical renames
Arch-specific vm-event functions in x86/vm_event.h -e.g. vm_event_init_domain()- don't have an 'arch_' prefix. Apply the same rule for monitor functions - originally the only two monitor functions that had an 'arch_' prefix were arch_monitor_domctl_event() and arch_monitor_domctl_op(), but I gave them that prefix because -they had a counterpart function in common code-, that being monitor_domctl(). Let this also be the rule for future 'arch_' functions additions, and with this patch remove the 'arch_' prefix from the monitor functions that don't have a counterpart in common-code (all but those 2 aforementioned). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/monitor.c| 4 ++-- xen/common/monitor.c | 4 ++-- xen/common/vm_event.c | 4 ++-- xen/include/asm-arm/monitor.h | 8 +++- xen/include/asm-x86/monitor.h | 12 ++-- 5 files changed, 15 insertions(+), 17 deletions(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 205df41..043815a 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -22,7 +22,7 @@ #include #include -int arch_monitor_init_domain(struct domain *d) +int monitor_init_domain(struct domain *d) { if ( !d->arch.monitor.msr_bitmap ) d->arch.monitor.msr_bitmap = xzalloc(struct monitor_msr_bitmap); @@ -33,7 +33,7 @@ int arch_monitor_init_domain(struct domain *d) return 0; } -void arch_monitor_cleanup_domain(struct domain *d) +void monitor_cleanup_domain(struct domain *d) { xfree(d->arch.monitor.msr_bitmap); diff --git a/xen/common/monitor.c b/xen/common/monitor.c index c73d1d5..5219238 100644 --- a/xen/common/monitor.c +++ b/xen/common/monitor.c @@ -50,12 +50,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop) if ( unlikely(mop->event > 31) ) return -EINVAL; /* Check if event type is available. */ -if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) ) +if ( unlikely(!(monitor_get_capabilities(d) & (1U << mop->event))) ) return -EOPNOTSUPP; break; case XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES: -mop->event = arch_monitor_get_capabilities(d); +mop->event = monitor_get_capabilities(d); return 0; default: diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 941345b..b4f9fd3 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -669,7 +669,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, { case XEN_VM_EVENT_ENABLE: /* domain_pause() not required here, see XSA-99 */ -rc = arch_monitor_init_domain(d); +rc = monitor_init_domain(d); if ( rc ) break; rc = vm_event_enable(d, vec, ved, _VPF_mem_access, @@ -682,7 +682,7 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec, { domain_pause(d); rc = vm_event_disable(d, ved); -arch_monitor_cleanup_domain(d); +monitor_cleanup_domain(d); domain_unpause(d); } break; diff --git a/xen/include/asm-arm/monitor.h b/xen/include/asm-arm/monitor.h index 4af707a..c72ee42 100644 --- a/xen/include/asm-arm/monitor.h +++ b/xen/include/asm-arm/monitor.h @@ -46,20 +46,18 @@ int arch_monitor_domctl_event(struct domain *d, return -EOPNOTSUPP; } -static inline -int arch_monitor_init_domain(struct domain *d) +static inline int monitor_init_domain(struct domain *d) { /* No arch-specific domain initialization on ARM. */ return 0; } -static inline -void arch_monitor_cleanup_domain(struct domain *d) +static inline void monitor_cleanup_domain(struct domain *d) { /* No arch-specific domain cleanup on ARM. */ } -static inline uint32_t arch_monitor_get_capabilities(struct domain *d) +static inline uint32_t monitor_get_capabilities(struct domain *d) { uint32_t capabilities = 0; diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 0501ca2..c5ae7ef 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -60,7 +60,10 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) return rc; } -static inline uint32_t arch_monitor_get_capabilities(struct domain *d) +int arch_monitor_domctl_event(struct domain *d, + struct xen_domctl_monitor_op *mop); + +static inline uint32_t monitor_get_capabilities(struct domain *d) { uint32_t capabilities = 0; @@ -84,12 +87,9 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d) return capabilities; } -int arch_monitor_domctl_event(struct domain *d, - struct xen_domctl_monitor_op *mop); - -int arch_monitor_init_domain(struct
[Xen-devel] [PATCH 02/16] x86: fix: make atomic_read() param const
This wouldn't let me make a param of a function that used atomic_read() const. Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/include/asm-x86/atomic.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index d246b70..0b250c8 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -94,7 +94,7 @@ typedef struct { int counter; } atomic_t; * * Atomically reads the value of @v. */ -static inline int atomic_read(atomic_t *v) +static inline int atomic_read(const atomic_t *v) { return read_atomic(>counter); } @@ -105,7 +105,7 @@ static inline int atomic_read(atomic_t *v) * * Non-atomically reads the value of @v */ -static inline int _atomic_read(atomic_t v) +static inline int _atomic_read(const atomic_t v) { return v.counter; } -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 01/16] x86/vmx_update_guest_cr: minor optimization
Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control was modified before actually calling vmx_update_cpu_exec_control(v). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- xen/arch/x86/hvm/vmx/vmx.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 0776d12..0798245 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1433,8 +1433,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) if ( paging_mode_hap(v->domain) ) { /* Manage GUEST_CR3 when CR0.PE=0. */ +uint32_t old_ctls = v->arch.hvm_vmx.exec_control; uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING); + v->arch.hvm_vmx.exec_control &= ~cr3_ctls; if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) v->arch.hvm_vmx.exec_control |= cr3_ctls; @@ -1444,7 +1446,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING; -vmx_update_cpu_exec_control(v); +if ( old_ctls != v->arch.hvm_vmx.exec_control ) +vmx_update_cpu_exec_control(v); } if ( !nestedhvm_vcpu_in_guestmode(v) ) -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 00/16] x86/vm-event: numerous adjustments & fixes
Adjustments & fixes to the vm-events code, mostly monitor-related. As I hadn't got the time/proper context to test these changes on a real machine, for now please consider them only for review, I'll let you know when and how actual testing went. Thanks, Corneliu ZUZU (16): x86/vmx_update_guest_cr: minor optimization x86: fix: make atomic_read() param const x86/monitor: mechanical renames x86/monitor: relocate vm_event_register_write_resume() function to monitor code x86/monitor: relocate code more appropriately x86/monitor: fix: set msr_bitmap to NULL after xfree x86/vm-event: fix: call cleanup when init fails, to free partial allocs x86/vm-event: call monitor init & cleanup funcs from respective vm_event funcs arm/monitor: move d->monitor cleanup to monitor_cleanup_domain() x86/vm-event: centralize vcpu-destroy cleanup in vm-events code x86/monitor: fix: treat -monitor- properly, as a subsys of the vm-event subsys x86/vm-event: fix: move cleanup of mem_access_emulate_each_rep to monitor stub x86/monitor: introduce writes_pending field in monitor_write_data x86/monitor: clarify separation between monitor subsys and vm-event as a whole x86/monitor: fix: don't compromise a monitor_write_data with pending CR writes x86/monitor: fix: xc_monitor _write_ctrlreg w/o previous _enable must fail xen/arch/x86/domain.c | 4 +- xen/arch/x86/hvm/emulate.c| 11 +- xen/arch/x86/hvm/hvm.c| 92 xen/arch/x86/hvm/vmx/vmx.c| 64 +-- xen/arch/x86/mm/p2m.c | 7 +- xen/arch/x86/monitor.c| 218 +++--- xen/arch/x86/vm_event.c | 82 +++--- xen/common/monitor.c | 4 +- xen/common/vm_event.c | 37 --- xen/include/asm-arm/monitor.h | 17 +-- xen/include/asm-arm/vm_event.h| 21 ++-- xen/include/asm-x86/atomic.h | 4 +- xen/include/asm-x86/domain.h | 31 -- xen/include/asm-x86/hvm/vmx/vmx.h | 1 + xen/include/asm-x86/monitor.h | 32 -- xen/include/asm-x86/vm_event.h| 27 ++--- xen/include/xen/monitor.h | 11 +- xen/include/xen/sched.h | 1 + 18 files changed, 473 insertions(+), 191 deletions(-) -- 2.5.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 6:50 PM, Tamas K Lengyel wrote: diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 205df41..42f31bf 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c [snip] So with this code-portion appropriately being relocated here I think we should also rename.. +void arch_monitor_write_data(struct vcpu *v) +{ +struct monitor_write_data *w = >arch.vm_event->write_data; .. arch.vm_event to arch.monitor to match the subsystem it is used by. Thanks, Tamas Which is (almost) exactly what I've done in (another..) patch-series I'll be sending today I hope (and a few more things). ;-) Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] Ping: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
On 7/6/2016 6:49 PM, Corneliu ZUZU wrote: Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control was modified before actually calling vmx_update_cpu_exec_control(v). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- Changed since v2: --- xen/arch/x86/hvm/vmx/vmx.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index df19579..8ab074f 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) if ( paging_mode_hap(v->domain) ) { /* Manage GUEST_CR3 when CR0.PE=0. */ +uint32_t old_ctls = v->arch.hvm_vmx.exec_control; uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING); + v->arch.hvm_vmx.exec_control &= ~cr3_ctls; if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) v->arch.hvm_vmx.exec_control |= cr3_ctls; @@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING; -vmx_update_cpu_exec_control(v); +if ( old_ctls != v->arch.hvm_vmx.exec_control ) +vmx_update_cpu_exec_control(v); } if ( !nestedhvm_vcpu_in_guestmode(v) ) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 2:53 PM, Jan Beulich wrote: On 08.07.16 at 13:33,wrote: On 7/8/2016 1:37 PM, Jan Beulich wrote: On 08.07.16 at 12:22, wrote: On 7/8/2016 10:21 AM, Jan Beulich wrote: On 06.07.16 at 17:50, wrote: +/* + * If CR0.PE=0, CR3 load exiting must remain enabled. + * See vmx_update_guest_cr code motion for cr = 0. + */ +if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) +continue; The first sentence of the comment should be brought in line with this condition. Would this do (aligned with the above observation): " /* * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0). */ " ? Not really: The condition checks whether paging is enabled and whether it's an unrestricted guest. The comment talks about protected mode being enabled. Hah you're right, I only now notice, that comment has actually been adopted (although I don't remember from where, I wonder if it was meantime removed and I only now see), I always thought it said "CR0.PG = 0"... So... " /* * If domain paging is disabled (CR0.PG=0) and * the domain is not in real-mode, then CR3 load-exiting * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0). */ " ? Looks reasonable. +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index) Unless there is a particular reason for this uint8_t, please convert to unsigned int. The particular reason is cr-indexes being uint8_t typed (see typeof(xen_domctl_monitor_op.mov_to_cr.index)). But I will change it to unsigned int if you prefer (maybe you could explain the preference though). No use of fixed width types when fixed width types aren't really required. Generally generated code is less efficient when having to deal with fixed width types. Strange, I would have thought the compiler would properly (and easily) deal with such efficiency issues. In this case the compiler may well do (as the function is static inline), but in other cases it's simply not allowed to. In order to not misguide people cloning existing code we should thus try to limit the number of bad examples (which in particular mean not introducing any new ones). Jan Ack. Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
On 7/8/2016 2:48 PM, Jan Beulich wrote: On 08.07.16 at 13:39, <cz...@bitdefender.com> wrote: On 7/6/2016 6:49 PM, Corneliu ZUZU wrote: Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control was modified before actually calling vmx_update_cpu_exec_control(v). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- Changed since v2: --- xen/arch/x86/hvm/vmx/vmx.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index df19579..8ab074f 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) if ( paging_mode_hap(v->domain) ) { /* Manage GUEST_CR3 when CR0.PE=0. */ +uint32_t old_ctls = v->arch.hvm_vmx.exec_control; uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING); + v->arch.hvm_vmx.exec_control &= ~cr3_ctls; if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) v->arch.hvm_vmx.exec_control |= cr3_ctls; @@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING; -vmx_update_cpu_exec_control(v); +if ( old_ctls != v->arch.hvm_vmx.exec_control ) +vmx_update_cpu_exec_control(v); } if ( !nestedhvm_vcpu_in_guestmode(v) ) I'm wondering if you could ack this as well (if there's nothing wrong w/ it) and push it to be done with it. :-) Well, if you had pinged the patch at least once, I probably would. I don't, however, recall having seen any such ping (only resends). Jan So is a 'ping' (haven't done that before) still necessary? Is a ping a simple 'reply-to-all' including 'Ping: ' in the subject? :-) Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
On 7/6/2016 6:49 PM, Corneliu ZUZU wrote: Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control was modified before actually calling vmx_update_cpu_exec_control(v). Signed-off-by: Corneliu ZUZU <cz...@bitdefender.com> --- Changed since v2: --- xen/arch/x86/hvm/vmx/vmx.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index df19579..8ab074f 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) if ( paging_mode_hap(v->domain) ) { /* Manage GUEST_CR3 when CR0.PE=0. */ +uint32_t old_ctls = v->arch.hvm_vmx.exec_control; uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING); + v->arch.hvm_vmx.exec_control &= ~cr3_ctls; if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) v->arch.hvm_vmx.exec_control |= cr3_ctls; @@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr) monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING; -vmx_update_cpu_exec_control(v); +if ( old_ctls != v->arch.hvm_vmx.exec_control ) +vmx_update_cpu_exec_control(v); } if ( !nestedhvm_vcpu_in_guestmode(v) ) Jan, I'm wondering if you could ack this as well (if there's nothing wrong w/ it) and push it to be done with it. :-) Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 1:37 PM, Jan Beulich wrote: On 08.07.16 at 12:22,wrote: On 7/8/2016 10:21 AM, Jan Beulich wrote: On 06.07.16 at 17:50, wrote: The title of this patch keeps confusing me - which code motion is being relocated here? As the commit message clearly states, the code motions that are being relocated are: Again this sentence makes no sense to me: I can't see how "code motions" can be "relocated", just like I don't see how you could move a move. But maybe it's just me... Hah, sorry, I'm not very good expressivity-wise, a weakness I'm aware of and which makes me pick up expressions I notice other people use (in this case those of maintainers). I think you were the one I noticed using the expression back in an older patch-series I've sent and I thought by "code-motion" you meant simply "that which some code does, tries to accomplish and the code itself". 1) handling of monitor_write_data @ hvm_do_resume 2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting CR3 load-exiting for cr-write monitor vm-events, i.e. the comment: /* Trap CR3 updates if CR3 memory events are enabled. */ and what's removed from under it. By 'relocation' I meant making that code vm-event specific (moving it to vm-event specific files). Yes, that what I've guessed. +{ +struct vcpu *v; +struct arch_vmx_struct *avmx; +unsigned int cr3_bitmask; +bool_t cr3_vmevent, cr3_ldexit; + +/* domain must be paused */ +ASSERT(atomic_read(>pause_count)); Comment style. As in change to "/* Domain must be paused. */"? Yes, as mandated by ./CODING_STYLE. +/* non-hap domains trap CR3 writes unconditionally */ +if ( !paging_mode_hap(d) ) +{ +#ifndef NDEBUG +for_each_vcpu ( d, v ) +ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING); +#endif +return; +} + +cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3); +cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask); + +for_each_vcpu ( d, v ) +{ +avmx = >arch.hvm_vmx; +cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING); + +if ( cr3_vmevent == cr3_ldexit ) +continue; + +/* + * If CR0.PE=0, CR3 load exiting must remain enabled. + * See vmx_update_guest_cr code motion for cr = 0. + */ Same as for the title - what code motion is this referring to? In a code comment you clearly shouldn't be referring to anything the patch effects, only to its result. The "vmx_update_guest_cr code motion for cr = 0", that's what's referring to. So I guess my problem really is that I don't understand what a "code motion" is (other than the act of moving code from one place to another). Again, sorry, will try to rephrase all of this properly :-). 'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters. In other words, see what's happening in the function 'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand why CR3 load-exiting must remain enabled when CR0.PE=0. +if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) +continue; The first sentence of the comment should be brought in line with this condition. Would this do (aligned with the above observation): " /* * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0). */ " ? Not really: The condition checks whether paging is enabled and whether it's an unrestricted guest. The comment talks about protected mode being enabled. Hah you're right, I only now notice, that comment has actually been adopted (although I don't remember from where, I wonder if it was meantime removed and I only now see), I always thought it said "CR0.PG = 0"... So... " /* * If domain paging is disabled (CR0.PG=0) and * the domain is not in real-mode, then CR3 load-exiting * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0). */ " ? +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index) Unless there is a particular reason for this uint8_t, please convert to unsigned int. The particular reason is cr-indexes being uint8_t typed (see typeof(xen_domctl_monitor_op.mov_to_cr.index)). But I will change it to unsigned int if you prefer (maybe you could explain the preference though). No use of fixed width types when fixed width types aren't really required. Generally generated code is less efficient when having to deal with fixed width types. Strange, I would have thought the compiler would properly (and easily) deal with such efficiency issues. +{ +/* vmx only */ +ASSERT(cpu_has_vmx); Comment style (more below). Should perhaps also get "for now" or some such added. As in "/* For now, VMX only. */"? For example,
Re: [Xen-devel] [PATCH v3 7/8] minor fixes (formatting, comments, unused includes etc.)
On 7/8/2016 10:56 AM, Jan Beulich wrote: On 06.07.16 at 17:55,wrote: --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -31,7 +31,6 @@ #include #include #include -#include #include #include #include @@ -63,11 +62,9 @@ #include #include #include -#include #include #include #include -#include #include #include #include The inclusion of asm/vm_event.h should really be removed in patch 2, I had to drop it here, to get the patch to apply ahead of the earlier ones in this series. --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -21,7 +21,6 @@ #include #include -#include #include static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index) Similarly I had to drop this hunk, which suggests that one of the earlier patches needlessly adds that #include (or it gets validly added and then a later patch makes it unnecessary again). Jan Thanks for applying these ahead, it spares me having to include them in future series. As for the issue, yeah that's probably what happened along the way although it was to be expected (applying patches from a series 'unsequentially') so again, thanks for the effort. Will keep in mind having to take care of the changes that didn't make it in a next series. Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup
On 7/8/2016 10:35 AM, Jan Beulich wrote: On 06.07.16 at 17:51,wrote: @@ -492,8 +493,12 @@ int vcpu_initialise(struct vcpu *v) void vcpu_destroy(struct vcpu *v) { -xfree(v->arch.vm_event); -v->arch.vm_event = NULL; +if ( unlikely(v->arch.vm_event) ) +{ +xfree(v->arch.vm_event->emul_read_data); +xfree(v->arch.vm_event); +v->arch.vm_event = NULL; +} Considering the repeat of this pattern ... @@ -52,8 +58,25 @@ void vm_event_cleanup_domain(struct domain *d) for_each_vcpu ( d, v ) { -xfree(v->arch.vm_event); -v->arch.vm_event = NULL; +if ( likely(!v->arch.vm_event) ) +continue; + +/* + * Only xfree the entire arch_vm_event if write_data was fully handled. + * Otherwise defer entire xfree until domain/vcpu destroyal. + */ +if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) ) +{ +xfree(v->arch.vm_event->emul_read_data); +xfree(v->arch.vm_event); +v->arch.vm_event = NULL; +continue; +} ... here, please consider making this another helper (inline?) function. Yeah, I'm sending a separate patch today which will invalidate some of these changes (then a v4 above that one). +/* write_data not fully handled, only xfree emul_read_data */ Comment style again (and more below). Ack, assuming you mean 'capital letter, end with dot'. --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v, unsigned int port) /* Clean up on domain destruction */ void vm_event_cleanup(struct domain *d) { +struct vcpu *v; + #ifdef CONFIG_HAS_MEM_PAGING if ( d->vm_event->paging.ring_page ) { @@ -560,6 +562,16 @@ void vm_event_cleanup(struct domain *d) (void)vm_event_disable(d, >vm_event->share); } #endif + +for_each_vcpu ( d, v ) +{ +if ( unlikely(v->arch.vm_event) ) +{ +/* vm_event->emul_read_data freed in vm_event_cleanup_domain */ Perhaps worthwhile adding a respective ASSERT()? Good point, ack. +static inline bool_t vm_event_vcpu_initialised(struct vcpu *v) +{ +return (v->arch.vm_event && v->arch.vm_event->emul_read_data); +} + +static inline bool_t vm_event_domain_initialised(struct domain *d) +{ +return (d->max_vcpus && d->vcpu[0] && +vm_event_vcpu_initialised(d->vcpu[0])); +} Both functions' parameters should be const. Pointless parentheses in both return statements. Jan Ack (although the parenthesis were there strictly for aesthetics, but that's subjective). Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 10:21 AM, Jan Beulich wrote: On 06.07.16 at 17:50,wrote: The title of this patch keeps confusing me - which code motion is being relocated here? As the commit message clearly states, the code motions that are being relocated are: 1) handling of monitor_write_data @ hvm_do_resume 2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting CR3 load-exiting for cr-write monitor vm-events, i.e. the comment: /* Trap CR3 updates if CR3 memory events are enabled. */ and what's removed from under it. By 'relocation' I meant making that code vm-event specific (moving it to vm-event specific files). +void vmx_vm_event_update_cr3_traps(struct domain *d) Is there anything in this function preventing the parameter to be const? Nope, will do. +{ +struct vcpu *v; +struct arch_vmx_struct *avmx; +unsigned int cr3_bitmask; +bool_t cr3_vmevent, cr3_ldexit; + +/* domain must be paused */ +ASSERT(atomic_read(>pause_count)); Comment style. As in change to "/* Domain must be paused. */"? +/* non-hap domains trap CR3 writes unconditionally */ +if ( !paging_mode_hap(d) ) +{ +#ifndef NDEBUG +for_each_vcpu ( d, v ) +ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING); +#endif +return; +} + +cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3); +cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask); + +for_each_vcpu ( d, v ) +{ +avmx = >arch.hvm_vmx; +cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING); + +if ( cr3_vmevent == cr3_ldexit ) +continue; + +/* + * If CR0.PE=0, CR3 load exiting must remain enabled. + * See vmx_update_guest_cr code motion for cr = 0. + */ Same as for the title - what code motion is this referring to? In a code comment you clearly shouldn't be referring to anything the patch effects, only to its result. The "vmx_update_guest_cr code motion for cr = 0", that's what's referring to. 'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters. In other words, see what's happening in the function 'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand why CR3 load-exiting must remain enabled when CR0.PE=0. +if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) +continue; The first sentence of the comment should be brought in line with this condition. Would this do (aligned with the above observation): " /* * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0). */ " ? +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index) Unless there is a particular reason for this uint8_t, please convert to unsigned int. The particular reason is cr-indexes being uint8_t typed (see typeof(xen_domctl_monitor_op.mov_to_cr.index)). But I will change it to unsigned int if you prefer (maybe you could explain the preference though). +{ +/* vmx only */ +ASSERT(cpu_has_vmx); Comment style (more below). Should perhaps also get "for now" or some such added. As in "/* For now, VMX only. */"? +static inline void write_ctrlreg_disable_traps(struct domain *d) +{ +unsigned int old = d->arch.monitor.write_ctrlreg_enabled; +d->arch.monitor.write_ctrlreg_enabled = 0; + +if ( old ) +{ +/* vmx only */ +ASSERT(cpu_has_vmx); Wouldn't this better move ahead of the if()? +/* was CR3 load-exiting enabled due to monitoring? */ +if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) ) And then this if() alone would suffice. No, it would be wrong because that ASSERT may not hold if "old == 0", i.e. we only ASSERT the implication "CR-write vm-events can be enabled -> vmx domain", but since the function is called by arch_monitor_cleanup_domain, putting the ASSERT before the if() would change that implication to "(any) monitor vm-events available -> vmx domain", assertion which wouldn't be proper TBD here. @@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d) void arch_monitor_cleanup_domain(struct domain *d) { xfree(d->arch.monitor.msr_bitmap); - +write_ctrlreg_disable_traps(d); memset(>arch.monitor, 0, sizeof(d->arch.monitor)); memset(>monitor, 0, sizeof(d->monitor)); } Rather than deleting the blank line, perhaps better to insert another one after your addition? Jan Ack. Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 6/8] x86/vm-event: minor ASSERT fix, add 'unlikely'
On 7/7/2016 11:27 AM, Jan Beulich wrote: On 06.07.16 at 17:54,wrote: --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -475,7 +475,7 @@ void hvm_do_resume(struct vcpu *v) if ( unlikely(v->arch.vm_event) ) { -if ( v->arch.vm_event->emulate_flags ) +if ( unlikely(v->arch.vm_event->emulate_flags) ) { enum emul_kind kind = EMUL_KIND_NORMAL; Acked-by: Jan Beulich --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -96,14 +96,16 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp) { if ( rsp->flags & VM_EVENT_FLAG_DENY ) { -struct monitor_write_data *w = >arch.vm_event->write_data; +struct monitor_write_data *w; -ASSERT(w); +ASSERT(v->arch.vm_event); /* deny flag requires the vCPU to be paused */ if ( !atomic_read(>vm_event_pause_count) ) return; +w = >arch.vm_event->write_data; + switch ( rsp->reason ) { case VM_EVENT_REASON_MOV_TO_MSR: I'd have preferred for you to leave alone the initializer, but we'll see what the maintainers are going to say. Jan It's 'cleaner' this way, doesn't it? Not assigning a pointer to a possibly invalid address... Anyway, I'm preparing a v4, I'll probably drop this change if you won't *subdue* to ack it (kidding). Thanks, Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel