Changeset View
Standalone View
sys/dev/iommu/iommu_gas.c
| Show First 20 Lines • Show All 594 Lines • ▼ Show 20 Lines | iommu_gas_free_region(struct iommu_map_entry *entry) | ||||||||
| if (prev == NULL) | if (prev == NULL) | ||||||||
| iommu_gas_rb_insert(domain, domain->first_place); | iommu_gas_rb_insert(domain, domain->first_place); | ||||||||
| if (next == NULL) | if (next == NULL) | ||||||||
| iommu_gas_rb_insert(domain, domain->last_place); | iommu_gas_rb_insert(domain, domain->last_place); | ||||||||
| IOMMU_DOMAIN_UNLOCK(domain); | IOMMU_DOMAIN_UNLOCK(domain); | ||||||||
| } | } | ||||||||
| static void | |||||||||
dougm: I suggest returning 'first' directly, rather than by reference. | |||||||||
| iommu_gas_remove_clip_left(struct iommu_domain *domain, iommu_gaddr_t start, | |||||||||
| iommu_gaddr_t end, struct iommu_map_entry **first, | |||||||||
| struct iommu_map_entry **r) | |||||||||
| { | |||||||||
Done Inline ActionsSo, you want to find the entry with So that's just the first entry with entry->end < start. That is the entry value returned by RB_FIND. So you could just return that. Without lines 618 to 634. dougm: So, you want to find the entry with
entry->start <= start < entry->end
if there is one, for… | |||||||||
Done Inline Actions... I do not think that the _last_ entry with entry->start < start is it, it is that entry, or the one right after it. Let me formulate it differently: it is the entry where we should either start the iteration over entries that needs to be removed, or the entry which must be split, and then the right half of it is the entry we start iteration with. kib: ...
or the first entry with entry->end < start, or NULL (just correcting a type in your reply… | |||||||||
| struct iommu_map_entry *entry, *nentry, fentry; | |||||||||
| IOMMU_DOMAIN_ASSERT_LOCKED(domain); | |||||||||
Done Inline ActionsIf you consider accepting D36010, which I just put out for review, you could implement this without calling insert or remove or having to have two allocated entries to plug in. You'd just have to pass a flag to this function to choose whether 'clip' modified the start field or the end field, then just modify that field and call RB_UPDATE_AUGMENT on the entry. dougm: If you consider accepting D36010, which I just put out for review, you could implement this… | |||||||||
Done Inline ActionsOn second thought, there is a case when you'd need to have one spare entry. If the (start, end) you are removing lie within a single entry (that is, entry->start < start < end < entry->end), then you do need one new entry. You could copy entry into new_entry, change entry->end to start, update augmentation, change new_entry->start, to end, and insert that. So you'd need one allocated node in that case. dougm: On second thought, there is a case when you'd need to have one spare entry. If the (start… | |||||||||
| MPASS(start <= end); | |||||||||
Done Inline ActionsIt's not clear to me where the IOTLB invalidation will occur. alc: It's not clear to me where the IOTLB invalidation will occur. | |||||||||
| MPASS(end <= domain->last_place->end); | |||||||||
| /* | |||||||||
| * Find an entry which contains the supplied guest's address | |||||||||
| * start, or the first entry after the start. Since we | |||||||||
| * asserted that start is below domain end, entry should | |||||||||
| * exist. Then clip it if needed. | |||||||||
Done Inline ActionsIf first_;lace and last_place entries in the GAS mean that entry is not NULL here, ever, you can KASSERT that instead of adding needless safety checks. dougm: If first_;lace and last_place entries in the GAS mean that entry is not NULL here, ever, you… | |||||||||
Done Inline ActionsWe usually do not assert for pointers not equal to NULL, hardware is good at catching such cases, esp. since we disabled mappings at zero and hardware has SMAP. bde@ was esp. against KASSERT(p != NULL...) kind of things. I added asserts that start/end are below the domain top address. kib: We usually do not assert for pointers not equal to NULL, hardware is good at catching such… | |||||||||
| */ | |||||||||
| fentry.start = start + 1; | |||||||||
| fentry.end = start + 1; | |||||||||
Done Inline ActionsSuppose RB_NFIND did not return NULL. Then we get here with start < entry->end. So in every case where RB_NFIND returns a non-NULL entry, we end up returning that entry. So you could just return it without falling into the for-loop. What if RB_NFIND returns NULL? Then you will walk the entire list looking for an entry satisfying start <= entry->start, never finding one and eventually returning NULL, which is what RB_NFIND gave you in the first place. So, we're stuck. I tried to tell you that you didn't need the code after RB_NFIND, and you didn't believe me. Now I've tried to tell you that the code after RB_NFIND accompiishes nothing, and you won't believe me. dougm: Suppose RB_NFIND did not return NULL. Then we get here with start < entry->end.
We fail the… | |||||||||
| entry = RB_NFIND(iommu_gas_entries_tree, &domain->rb_root, &fentry); | |||||||||
| MPASS(start <= entry->start || (entry->start <= start && | |||||||||
| start <= entry->end)); | |||||||||
Done Inline ActionsWe know start < entry->end. We also know that start < entry->start || entry->start <= start, but that's just tautology. dougm: We know start < entry->end.
We also know that start < entry->start || entry->start <= start… | |||||||||
| if (entry->start < start && | |||||||||
| (entry->flags & IOMMU_MAP_ENTRY_RMRR) == 0) { | |||||||||
| if (end < entry->end) { | |||||||||
| nentry = *r; | |||||||||
| *r = NULL; | |||||||||
| *nentry = *entry; | |||||||||
| nentry->start = end; | |||||||||
| } | |||||||||
| entry->end = start; | |||||||||
| RB_UPDATE_AUGMENT(entry, rb_entry); | |||||||||
| if (*r == NULL) | |||||||||
| iommu_gas_rb_insert(domain, nentry); | |||||||||
Done Inline ActionsIs this because you don't want to clip RMRR entries? If so, you could just write in line 644: if (entry->start < start && (entry->flags & IOMMU_MAP_ENTRY_RMRR) == 0) to stop clipping at the left boundary. I don't understand, though, why this requires putting in another loop ahead of the RB_FOREACH_FROM loop. dougm: Is this because you don't want to clip RMRR entries? If so, you could just write in line 644… | |||||||||
| nentry = iommu_gas_entries_tree_RB_NEXT(entry); | |||||||||
| } else { | |||||||||
| nentry = entry; | |||||||||
Done Inline ActionsSince you need to have a variable nentry anyway (see below), how about (to avoid old_end): nentry = *r; *r = NULL; *nentry = *entry; nentry->start = end; } iommu_gas_rb_insert(domain, nentry); You don't really need a return, or a goto here. You'll skip the loop, and the post-loop test, quickly. dougm: Since you need to have a variable nentry anyway (see below), how about (to avoid old_end):
if… | |||||||||
| } | |||||||||
| *first = nentry; | |||||||||
| } | |||||||||
| static bool | |||||||||
Done Inline Actionsold_end is needed because entry->end is overwritten above kib: old_end is needed because entry->end is overwritten above | |||||||||
| iommu_gas_remove_clip_right(struct iommu_domain *domain, | |||||||||
| iommu_gaddr_t end, struct iommu_map_entry *entry, | |||||||||
| struct iommu_map_entry **res, struct iommu_map_entry **r) | |||||||||
| { | |||||||||
Done Inline ActionsThis is goto out instead of return, to not skip INVARIANTS check kib: This is `goto out` instead of `return`, to not skip INVARIANTS check | |||||||||
| struct iommu_map_entry *nentry; | |||||||||
| if (entry->start >= end || (entry->flags & IOMMU_MAP_ENTRY_RMRR) != 0) | |||||||||
| return (false); | |||||||||
Done Inline ActionsMy patch contained an error which remains here. The first and last arguments to RB_FOREACH_FROM can't be the same. You need nentry = entry; or something similar. dougm: My patch contained an error which remains here. The first and last arguments to… | |||||||||
| nentry = *r; | |||||||||
| *r = NULL; | |||||||||
| *nentry = *entry; | |||||||||
| nentry->end = entry->start = end; | |||||||||
| RB_UPDATE_AUGMENT(entry, rb_entry); | |||||||||
| iommu_gas_rb_insert(domain, nentry); | |||||||||
Done Inline ActionsThere are things about the latest revision that I don't understand. For one thing, I don't understand why a new entry is getting inserted when an entry spanning 'end' is encountered. That's not what happens around 'start'; there, a new entry is created only when the entry that spans 'start' also spans 'end'. If this change is good, for some reason, then I'd expect the splitting around 'start' to be the same, and not what we had back when we were deleting elements between 'start' and 'end'. For another thing - if we're not looping for any reason other than to get to 'end', it's better to use RB_NFIND than to loop. I don't really understand why this loop was gutted and a second loop is necessary to remove elements, but there must be one. dougm: There are things about the latest revision that I don't understand. For one thing, I don't… | |||||||||
Done Inline ActionsProblem with previous version WRT invalidation is that to unload we need the actual entry. I changed the code to be a mix between the version you agreed with, and the last one. In particular, it has only one loop instead of two, over the removed range' entries. But I still create up to two clipped entries at start and end, which are unloaded and freed. kib: Problem with previous version WRT invalidation is that to unload we need the actual entry.
I… | |||||||||
| *res = nentry; | |||||||||
Done Inline Actionsiommu_domain_unload() will handle this by indirectly calling iommu_gas_free_space(). alc: iommu_domain_unload() will handle this by indirectly calling iommu_gas_free_space(). | |||||||||
Done Inline ActionsI now think that this is in fact not correct (it was not correct in the previous patch as well, but for different reason, which you pointed out). Imagine that some consumer calls iommu_gase_remove() on a range that intersects with our operating range. I do not see why this is incorrect. Then, the second caller overrides the dmamap_link linkage for the participating map entries, if they are not yet processed by unload. kib: I now think that this is in fact not correct (it was not correct in the previous patch as well… | |||||||||
Done Inline ActionsAgreed. To resolve this issue, I suggest adding an "in transition" flag to the entry. iommu_gas_remove_unmap() skips entries with the flag set, and sets it on entries that are added to the TAILQ. alc: Agreed. To resolve this issue, I suggest adding an "in transition" flag to the entry. | |||||||||
Done Inline Actions
alc: | |||||||||
Done Inline ActionsI guess that you seek the first entry with end <= entry->end, based on the line 715 KASSERT. But that's what RB_NFIND does for you already. So I don't understand the purpose of the code from lines 643 to 660. Or I don't understand the purpose of the function. Replace 'end' with 'start' and 'line 715' with 'line 689', and the same comment applies to the previous function. Maybe a comment to explain these functions would help. dougm: I guess that you seek the first entry with end <= entry->end, based on the line 715 KASSERT. | |||||||||
Done Inline ActionsI added the comments and supposedly fixed the bugs I could find by reading. kib: I added the comments and supposedly fixed the bugs I could find by reading. | |||||||||
| return (true); | |||||||||
Done Inline ActionsI'm confused. Shouldn't this be ==? alc: I'm confused. Shouldn't this be `==`? | |||||||||
Done Inline ActionsDefinitely. kib: Definitely. | |||||||||
| } | |||||||||
Done Inline Actionsiommu_domain_unload() does this for you. alc: iommu_domain_unload() does this for you. | |||||||||
| static void | |||||||||
| iommu_gas_remove_unmap(struct iommu_domain *domain, | |||||||||
| struct iommu_map_entry *entry) | |||||||||
| { | |||||||||
| int error __diagused; | |||||||||
Done Inline Actionsiommu_domain_unload_entry() is going to directly or indirectly (through qi) call iommu_gas_rb_remove() a second time on each entry. Can a _PLACE entry actually be mapped, i.e., not be _UNMAPPED? alc: iommu_domain_unload_entry() is going to directly or indirectly (through qi) call… | |||||||||
Done Inline ActionsI do not think that PLACE can be mapped. I added assert and removed calculation of the argument. Also the iommu_gas_rb_remove() call is eliminated. kib: I do not think that PLACE can be mapped. I added assert and removed calculation of the… | |||||||||
| iommu_gas_rb_remove(domain, entry); | |||||||||
| entry->flags &= ~IOMMU_MAP_ENTRY_MAP; | |||||||||
Done Inline ActionsWhat if entry->end <= clip? dougm: What if entry->end <= clip?
| |||||||||
| if ((entry->flags & IOMMU_MAP_ENTRY_UNMAPPED) != 0) { | |||||||||
| error = domain->ops->unmap(domain, entry->start, | |||||||||
| entry->end - entry->start, 0); | |||||||||
| MPASS(error == 0); | |||||||||
| } | |||||||||
| IOMMU_DOMAIN_UNLOCK(domain); | |||||||||
| iommu_domain_unload_entry(entry, (entry->flags & | |||||||||
| IOMMU_MAP_ENTRY_PLACE) == 0, true); | |||||||||
| IOMMU_DOMAIN_LOCK(domain); | |||||||||
| } | |||||||||
Done Inline ActionsBy "contains the ... address start", I assume you mean an entry with entry->start <= start < entry->end For fentry.start = fentry.end = start, you might get back an entry with entry->end = start. So you should add one to the fentry values to be sure you get back an entry with entry->end > start. But maybe 'contains' means something else here. The comment should be clearer. dougm: By "contains the ... address start", I assume you mean an entry with
entry->start <= start <… | |||||||||
Done Inline ActionsI am not sure what do you mean by 'clearer' there. The terminology for entry containing an address seems to be well-established, for instance there are more than five instances of it in vm_map.c. kib: I am not sure what do you mean by 'clearer' there. The terminology for entry containing an… | |||||||||
| void | |||||||||
| iommu_gas_remove(struct iommu_domain *domain, iommu_gaddr_t start, | |||||||||
| iommu_gaddr_t size) | |||||||||
Done Inline Actions'first' can be entirely replaced by 'nentry'. dougm: 'first' can be entirely replaced by 'nentry'. | |||||||||
| { | |||||||||
| struct iommu_map_entry *r1, *r2, *first, *last, *nentry; | |||||||||
Done Inline ActionsAm I correct in inferring that you introduced this synchronization because the domain lock is released and reacquired in the middle of the below RB_FOREACH_FROM loop? alc: Am I correct in inferring that you introduced this synchronization because the domain lock is… | |||||||||
Done Inline ActionsYes kib: Yes | |||||||||
Done Inline ActionsRather than implementing this new synchronization mechanism, for now, I would suggest changing (1) iommu_gas_remove_unmap() so that it simply adds the entry to a TAILQ (and thus doesn't need to release and reacquire the domain lock) and (2) this function so that it calls iommu_domain_unload() on that TAILQ after the domain lock is released. Later, we could look at adding a new helper function to the arm64- and x86-specific code that might initiate the unmapping and IOTLB invalidation if the latter can be done asynchronously. Otherwise, this new function adds the entry to the above TAILQ for synchronous invalidation after the domain lock is released. alc: Rather than implementing this new synchronization mechanism, for now, I would suggest changing… | |||||||||
| struct iommu_map_entry *entry __diagused; | |||||||||
| iommu_gaddr_t end; | |||||||||
| end = start + size; | |||||||||
| r1 = iommu_gas_alloc_entry(domain, IOMMU_PGF_WAITOK); | |||||||||
| r2 = iommu_gas_alloc_entry(domain, IOMMU_PGF_WAITOK); | |||||||||
Done Inline ActionsI don't think you need to do a find here. You can iterate the loop while entry->end < end, and when the loop is done, 'entry' will be the same as the rentry you're finding here, and you can clip or not at that point. dougm: I don't think you need to do a find here. You can iterate the loop while entry->end < end, and… | |||||||||
| IOMMU_DOMAIN_LOCK(domain); | |||||||||
| while ((domain->flags & IOMMU_DOMAIN_BUSY) != 0) | |||||||||
| msleep(&domain->flags, &domain->lock, 0, "ioubsy", 0); | |||||||||
| domain->flags |= IOMMU_DOMAIN_BUSY; | |||||||||
| iommu_gas_remove_clip_left(domain, start, end, &first, &r1); | |||||||||
| nentry = first; | |||||||||
| RB_FOREACH_FROM(entry, iommu_gas_entries_tree, nentry) { | |||||||||
| if (entry->start >= end) | |||||||||
| break; | |||||||||
| KASSERT(start <= entry->start, | |||||||||
| ("iommu_gas_remove entry (%#jx, %#jx) start %#jx", | |||||||||
| entry->start, entry->end, start)); | |||||||||
| if ((entry->flags & IOMMU_MAP_ENTRY_RMRR) != 0) | |||||||||
| continue; | |||||||||
| iommu_gas_remove_unmap(domain, entry); | |||||||||
| } | |||||||||
| if (iommu_gas_remove_clip_right(domain, end, entry, &last, &r2)) | |||||||||
| iommu_gas_remove_unmap(domain, last); | |||||||||
dougmUnsubmitted Done Inline ActionsIf clip_right returned true, then last is the same as what r2 was before clip_right was called. So you could null r2 here, after unmap, and avoid having a 'last' parameter. dougm: If clip_right returned true, then last is the same as what r2 was before clip_right was called. | |||||||||
| #ifdef INVARIANTS | |||||||||
| RB_FOREACH(entry, iommu_gas_entries_tree, &domain->rb_root) { | |||||||||
| if ((entry->flags & IOMMU_MAP_ENTRY_RMRR) != 0) | |||||||||
| continue; | |||||||||
| KASSERT(entry->end <= start || entry->start >= end, | |||||||||
| ("iommu_gas_remove leftover entry (%#jx, %#jx) range " | |||||||||
| "(%#jx, %#jx)", | |||||||||
| entry->start, entry->end, start, end)); | |||||||||
| } | |||||||||
| #endif | |||||||||
Done Inline ActionsHow about RB_FOREACH(entry, iommu_gas_entries_tree, &domain->rb_root) { ? dougm: How about
RB_FOREACH(entry, iommu_gas_entries_tree, &domain->rb_root) {
? | |||||||||
| domain->flags &= ~IOMMU_DOMAIN_BUSY; | |||||||||
| wakeup(&domain->flags); | |||||||||
| IOMMU_DOMAIN_UNLOCK(domain); | |||||||||
| if (r1 != NULL) | |||||||||
| iommu_gas_free_entry(r1); | |||||||||
| if (r2 != NULL) | |||||||||
| iommu_gas_free_entry(r2); | |||||||||
| } | |||||||||
| int | int | ||||||||
| iommu_gas_map(struct iommu_domain *domain, | iommu_gas_map(struct iommu_domain *domain, | ||||||||
| const struct bus_dma_tag_common *common, iommu_gaddr_t size, int offset, | const struct bus_dma_tag_common *common, iommu_gaddr_t size, int offset, | ||||||||
| u_int eflags, u_int flags, vm_page_t *ma, struct iommu_map_entry **res) | u_int eflags, u_int flags, vm_page_t *ma, struct iommu_map_entry **res) | ||||||||
| { | { | ||||||||
| struct iommu_gas_match_args a; | struct iommu_gas_match_args a; | ||||||||
Done Inline Actionslentry could be NULL, leading to segfault. Same for rentry. dougm: lentry could be NULL, leading to segfault. Same for rentry. | |||||||||
Done Inline ActionsI added the safety checks, but practically it should not because there is always first_place and last_place entries in the GAS. kib: I added the safety checks, but practically it should not because there is always first_place… | |||||||||
| struct iommu_map_entry *entry; | struct iommu_map_entry *entry; | ||||||||
| int error; | int error; | ||||||||
| KASSERT((flags & ~(IOMMU_MF_CANWAIT | IOMMU_MF_CANSPLIT)) == 0, | KASSERT((flags & ~(IOMMU_MF_CANWAIT | IOMMU_MF_CANSPLIT)) == 0, | ||||||||
| ("invalid flags 0x%x", flags)); | ("invalid flags 0x%x", flags)); | ||||||||
| a.domain = domain; | a.domain = domain; | ||||||||
| a.size = size; | a.size = size; | ||||||||
| a.offset = offset; | a.offset = offset; | ||||||||
| a.common = common; | a.common = common; | ||||||||
| a.gas_flags = flags; | a.gas_flags = flags; | ||||||||
| entry = iommu_gas_alloc_entry(domain, | entry = iommu_gas_alloc_entry(domain, | ||||||||
| (flags & IOMMU_MF_CANWAIT) != 0 ? IOMMU_PGF_WAITOK : 0); | (flags & IOMMU_MF_CANWAIT) != 0 ? IOMMU_PGF_WAITOK : 0); | ||||||||
| if (entry == NULL) | if (entry == NULL) | ||||||||
| return (ENOMEM); | return (ENOMEM); | ||||||||
| a.entry = entry; | a.entry = entry; | ||||||||
| IOMMU_DOMAIN_LOCK(domain); | IOMMU_DOMAIN_LOCK(domain); | ||||||||
| while ((domain->flags & IOMMU_DOMAIN_BUSY) != 0) | |||||||||
| msleep(&domain->flags, &domain->lock, 0, "iombsy", 0); | |||||||||
| error = iommu_gas_find_space(&a); | error = iommu_gas_find_space(&a); | ||||||||
| if (error == ENOMEM) { | if (error == ENOMEM) { | ||||||||
| IOMMU_DOMAIN_UNLOCK(domain); | IOMMU_DOMAIN_UNLOCK(domain); | ||||||||
| iommu_gas_free_entry(entry); | iommu_gas_free_entry(entry); | ||||||||
| return (error); | return (error); | ||||||||
| } | } | ||||||||
| #ifdef INVARIANTS | #ifdef INVARIANTS | ||||||||
| if (iommu_check_free) | if (iommu_check_free) | ||||||||
| ▲ Show 20 Lines • Show All 244 Lines • Show Last 20 Lines | |||||||||
I suggest returning 'first' directly, rather than by reference.