| Message ID | 20181106191454.22143-5-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add xentrace support | expand |
Hello Julien, вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише: > @@ -275,7 +280,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn, > static inline struct page_info *get_page_from_gfn( > struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q) > { > - return p2m_get_page_from_gfn(d, _gfn(gfn), t); > + mfn_t mfn; > + p2m_type_t _t; Me personally, do not like introducing intermediate `_t` variable. IMO, constructs like following: > + if ( !t ) > + t = &_t; > + > + *t = p2m_invalid; make the code harder to understand than simple checking `t` for nul before assigning it a value. Sincerely, Andrii Anisov.
On 11/15/18 11:33 AM, Andrii Anisov wrote: > Hello Julien, Hi, > > вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише: >> @@ -275,7 +280,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn, >> static inline struct page_info *get_page_from_gfn( >> struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q) >> { >> - return p2m_get_page_from_gfn(d, _gfn(gfn), t); >> + mfn_t mfn; >> + p2m_type_t _t; > > Me personally, do not like introducing intermediate `_t` variable. > IMO, constructs like following: > >> + if ( !t ) >> + t = &_t; >> + >> + *t = p2m_invalid; > > make the code harder to understand than simple checking `t` for nul > before assigning it a > value. If I drop _t then I need to add if ( *t ) in 3 places in that code. So I don't think the approach is any better. Feel free to suggest a solution that does not require to check 't' everywhere. Cheers,
чт, 15 лист. 2018 о 13:40 Julien Grall <julien.grall@arm.com> пише: > If I drop _t then I need to add if ( *t ) in 3 places in that code. So I > don't think the approach is any better. Ouch, I kept in my mind two places. Something like: if ( t ) *t = p2m_invalid; .... if ( t ) *t = ( page->u.inuse.type_info & PGT_writable_page ) ? p2m_ram_rw : p2m_ram_ro; ..... Sincerely, Andrii Anisov.
On 11/15/18 12:02 PM, Andrii Anisov wrote: > чт, 15 лист. 2018 о 13:40 Julien Grall <julien.grall@arm.com> пише: >> If I drop _t then I need to add if ( *t ) in 3 places in that code. So I >> don't think the approach is any better. > Ouch, I kept in my mind two places. > > Something like: > > if ( t ) > *t = p2m_invalid; > .... > if ( t ) > *t = ( page->u.inuse.type_info & PGT_writable_page ) ? p2m_ram_rw > : p2m_ram_ro; If it was fitting in a single line maybe. In that context, I find it more difficult to read. So I would prefer to stick with _t which is quite common within the p2m code base so far. Cheers,
> So I would prefer to stick with _t which is quite common within the p2m > code base so far. I've found a similar code only in one place - p2m_get_entry() function. And it is, at least, somehow commented there: ... /* Allow t to be NULL */ t = t ?: &_t; *t = p2m_invalid; ... But IMO, it is really confusing to write a code to calculate and store a value which clearly is not needed by a caller. From another hand, I'm not sure if a compiler would be intelligent enough to factor out the odd code from execution pass on the incoming null pointer. I'm sorry, but I can't pass my RB for `_t`. Sincerely, Andrii Anisov.
Hi, On 11/15/18 1:19 PM, Andrii Anisov wrote: >> So I would prefer to stick with _t which is quite common within the p2m >> code base so far. > > I've found a similar code only in one place - p2m_get_entry() > function. And it is, at least, somehow commented there: > ... > /* Allow t to be NULL */ > t = t ?: &_t; > > *t = p2m_invalid; > ... And in x86 code... > > But IMO, it is really confusing to write a code to calculate and store > a value which clearly is not needed by a caller. I disagree, you directly know that t can be NULL. Although, I agree that a comment would help to understand. > From another hand, I'm not sure if a compiler would be intelligent > enough to factor out the odd code from execution pass on the incoming > null pointer. You can't assume the compiler will inline even with the inline keyword. > > I'm sorry, but I can't pass my RB for `_t`. I think this request is unreasonable because this is a matter of taste. While this is a nice feature to have in Xen 4.12 and address a long standing open issue on Arm. I don't require it and I am not inclined to address bikeshedding. So I will keep aside for now until someone cares about it. Cheers,
On Thu, 15 Nov 2018, Julien Grall wrote: > Hi, > > On 11/15/18 1:19 PM, Andrii Anisov wrote: > > > So I would prefer to stick with _t which is quite common within the p2m > > > code base so far. > > > > I've found a similar code only in one place - p2m_get_entry() > > function. And it is, at least, somehow commented there: > > ... > > /* Allow t to be NULL */ > > t = t ?: &_t; > > > > *t = p2m_invalid; > > ... > > And in x86 code... > > > > > But IMO, it is really confusing to write a code to calculate and store > > a value which clearly is not needed by a caller. > > I disagree, you directly know that t can be NULL. Although, I agree that a > comment would help to understand. > > > From another hand, I'm not sure if a compiler would be intelligent > > enough to factor out the odd code from execution pass on the incoming > > null pointer. > > You can't assume the compiler will inline even with the inline keyword. > > > > > I'm sorry, but I can't pass my RB for `_t`. > > I think this request is unreasonable because this is a matter of taste. > > While this is a nice feature to have in Xen 4.12 and address a long standing > open issue on Arm. I don't require it and I am not inclined to address > bikeshedding. > > So I will keep aside for now until someone cares about it. Let's try to be reasonable and have constructive conversations. If we do have to disagree, let's disagree on meaningful design decisions, not silly code style issues :-) Andrii, when something can be done equally well in two different ways, especially when it is a matter of style, I think it is best to express our preference, but not to force a contributor in a particular direction, unless specifically stated in CODING_STYLE or equivalent docs. We don't have written rules about code reviews, but if we had, I think this should be one of them. (I would love to have written rules about code reviews.) Julien, usually in situations like this, it is quicker to change the code than to argue. I personally don't care and wouldn't ask you to change it, but if a member of the community reads the code and has an adverse reaction, it is always worth reconsidering the approach, because others might find it antagonizing too. Given the choice, it is best to go for something obvious to most, so if a new contributor sends a patch to change, it is more likely to be correct from the start.
Hi Stefano, On 11/15/18 6:44 PM, Stefano Stabellini wrote: > On Thu, 15 Nov 2018, Julien Grall wrote: >> Hi, >> >> On 11/15/18 1:19 PM, Andrii Anisov wrote: >>>> So I would prefer to stick with _t which is quite common within the p2m >>>> code base so far. >>> >>> I've found a similar code only in one place - p2m_get_entry() >>> function. And it is, at least, somehow commented there: >>> ... >>> /* Allow t to be NULL */ >>> t = t ?: &_t; >>> >>> *t = p2m_invalid; >>> ... >> >> And in x86 code... >> >>> >>> But IMO, it is really confusing to write a code to calculate and store >>> a value which clearly is not needed by a caller. >> >> I disagree, you directly know that t can be NULL. Although, I agree that a >> comment would help to understand. >> >>> From another hand, I'm not sure if a compiler would be intelligent >>> enough to factor out the odd code from execution pass on the incoming >>> null pointer. >> >> You can't assume the compiler will inline even with the inline keyword. >> >>> >>> I'm sorry, but I can't pass my RB for `_t`. >> >> I think this request is unreasonable because this is a matter of taste. >> >> While this is a nice feature to have in Xen 4.12 and address a long standing >> open issue on Arm. I don't require it and I am not inclined to address >> bikeshedding. >> >> So I will keep aside for now until someone cares about it. > > Let's try to be reasonable and have constructive conversations. If we do > have to disagree, let's disagree on meaningful design decisions, not > silly code style issues :-) > > Andrii, when something can be done equally well in two different ways, > especially when it is a matter of style, I think it is best to express > our preference, but not to force a contributor in a particular > direction, unless specifically stated in CODING_STYLE or equivalent > docs. We don't have written rules about code reviews, but if we had, > I think this should be one of them. (I would love to have written rules > about code reviews.) > > Julien, usually in situations like this, it is quicker to change the > code than to argue. I personally don't care and wouldn't ask you to > change it, but if a member of the community reads the code and has an > adverse reaction, it is always worth reconsidering the approach, because > others might find it antagonizing too. Given the choice, it is best to > go for something obvious to most, so if a new contributor sends a patch > to change, it is more likely to be correct from the start. I agree with your point here but this is also valid in the other way. If the suggested alternative provokes an adverse reaction to the contributor, then why should he use it? As I wrote earlier one trying to use ternary condition split over 2 lines is not making the code more obvious. So I am not entirely sure why I should be forced to write such code? Cheers,
On Thu, 15 Nov 2018, Julien Grall wrote: > Hi Stefano, > > On 11/15/18 6:44 PM, Stefano Stabellini wrote: > > On Thu, 15 Nov 2018, Julien Grall wrote: > > > Hi, > > > > > > On 11/15/18 1:19 PM, Andrii Anisov wrote: > > > > > So I would prefer to stick with _t which is quite common within the > > > > > p2m > > > > > code base so far. > > > > > > > > I've found a similar code only in one place - p2m_get_entry() > > > > function. And it is, at least, somehow commented there: > > > > ... > > > > /* Allow t to be NULL */ > > > > t = t ?: &_t; > > > > > > > > *t = p2m_invalid; > > > > ... > > > > > > And in x86 code... > > > > > > > > > > > But IMO, it is really confusing to write a code to calculate and store > > > > a value which clearly is not needed by a caller. > > > > > > I disagree, you directly know that t can be NULL. Although, I agree that a > > > comment would help to understand. > > > > > > > From another hand, I'm not sure if a compiler would be intelligent > > > > enough to factor out the odd code from execution pass on the incoming > > > > null pointer. > > > > > > You can't assume the compiler will inline even with the inline keyword. > > > > > > > > > > > I'm sorry, but I can't pass my RB for `_t`. > > > > > > I think this request is unreasonable because this is a matter of taste. > > > > > > While this is a nice feature to have in Xen 4.12 and address a long > > > standing > > > open issue on Arm. I don't require it and I am not inclined to address > > > bikeshedding. > > > > > > So I will keep aside for now until someone cares about it. > > > > Let's try to be reasonable and have constructive conversations. If we do > > have to disagree, let's disagree on meaningful design decisions, not > > silly code style issues :-) > > > > Andrii, when something can be done equally well in two different ways, > > especially when it is a matter of style, I think it is best to express > > our preference, but not to force a contributor in a particular > > direction, unless specifically stated in CODING_STYLE or equivalent > > docs. We don't have written rules about code reviews, but if we had, > > I think this should be one of them. (I would love to have written rules > > about code reviews.) > > > > Julien, usually in situations like this, it is quicker to change the > > code than to argue. I personally don't care and wouldn't ask you to > > change it, but if a member of the community reads the code and has an > > adverse reaction, it is always worth reconsidering the approach, because > > others might find it antagonizing too. Given the choice, it is best to > > go for something obvious to most, so if a new contributor sends a patch > > to change, it is more likely to be correct from the start. > > I agree with your point here but this is also valid in the other way. If the > suggested alternative provokes an adverse reaction to the contributor, then > why should he use it? > > As I wrote earlier one trying to use ternary condition split over 2 lines is > not making the code more obvious. So I am not entirely sure why I should be > forced to write such code? I don't think you should be. You can find another way. For instance, the whole thing could be reduce to one more "if" condition: if ( t != NULL ) { *t = p2m_invalid; if ( page->u.inuse.type_info & PGT_writable_page ) *t = p2m_ram_rw; else *t = p2m_ram_ro; } be creative :-)
Hi, On 11/15/18 7:48 PM, Stefano Stabellini wrote: > On Thu, 15 Nov 2018, Julien Grall wrote: >> Hi Stefano, >> >> On 11/15/18 6:44 PM, Stefano Stabellini wrote: >>> On Thu, 15 Nov 2018, Julien Grall wrote: >>>> Hi, >>>> >>>> On 11/15/18 1:19 PM, Andrii Anisov wrote: >>>>>> So I would prefer to stick with _t which is quite common within the >>>>>> p2m >>>>>> code base so far. >>>>> >>>>> I've found a similar code only in one place - p2m_get_entry() >>>>> function. And it is, at least, somehow commented there: >>>>> ... >>>>> /* Allow t to be NULL */ >>>>> t = t ?: &_t; >>>>> >>>>> *t = p2m_invalid; >>>>> ... >>>> >>>> And in x86 code... >>>> >>>>> >>>>> But IMO, it is really confusing to write a code to calculate and store >>>>> a value which clearly is not needed by a caller. >>>> >>>> I disagree, you directly know that t can be NULL. Although, I agree that a >>>> comment would help to understand. >>>> >>>>> From another hand, I'm not sure if a compiler would be intelligent >>>>> enough to factor out the odd code from execution pass on the incoming >>>>> null pointer. >>>> >>>> You can't assume the compiler will inline even with the inline keyword. >>>> >>>>> >>>>> I'm sorry, but I can't pass my RB for `_t`. >>>> >>>> I think this request is unreasonable because this is a matter of taste. >>>> >>>> While this is a nice feature to have in Xen 4.12 and address a long >>>> standing >>>> open issue on Arm. I don't require it and I am not inclined to address >>>> bikeshedding. >>>> >>>> So I will keep aside for now until someone cares about it. >>> >>> Let's try to be reasonable and have constructive conversations. If we do >>> have to disagree, let's disagree on meaningful design decisions, not >>> silly code style issues :-) >>> >>> Andrii, when something can be done equally well in two different ways, >>> especially when it is a matter of style, I think it is best to express >>> our preference, but not to force a contributor in a particular >>> direction, unless specifically stated in CODING_STYLE or equivalent >>> docs. We don't have written rules about code reviews, but if we had, >>> I think this should be one of them. (I would love to have written rulesAs >>> about code reviews.) >>> >>> Julien, usually in situations like this, it is quicker to change the >>> code than to argue. I personally don't care and wouldn't ask you to >>> change it, but if a member of the community reads the code and has an >>> adverse reaction, it is always worth reconsidering the approach, because >>> others might find it antagonizing too. Given the choice, it is best to >>> go for something obvious to most, so if a new contributor sends a patch >>> to change, it is more likely to be correct from the start. >> >> I agree with your point here but this is also valid in the other way. If the >> suggested alternative provokes an adverse reaction to the contributor, then >> why should he use it? >> >> As I wrote earlier one trying to use ternary condition split over 2 lines is >> not making the code more obvious. So I am not entirely sure why I should be >> forced to write such code? > > I don't think you should be. You can find another way. For instance, the > whole thing could be reduce to one more "if" condition: > > if ( t != NULL ) > { > *t = p2m_invalid; > if ( page->u.inuse.type_info & PGT_writable_page ) > *t = p2m_ram_rw; > else > *t = p2m_ram_ro; > } > > be creative :-) Well the code suggested is different from what I intended :). t should be set to invalid before the check mfn_valid/get_page. So this needs at least 2 checks. 2 places were t != NULL needs to be explained. As you said this is a matter of taste. If someone disagree on the coding style, then he should suggest an alternative way fitting the requirement. Cheers,
чт, 15 лист. 2018 о 17:05 Julien Grall <julien.grall@arm.com> пише: > And in x86 code... Sorry, did not check. > You can't assume the compiler will inline even with the inline keyword. For sure! > > I'm sorry, but I can't pass my RB for `_t`. > > I think this request is unreasonable because this is a matter of taste. I would not say it is a request. I'm just saying that I personally do not like that code, it confuses me, so I can't pass RB for it. Sorry for that :( I really hope someone else can come up with RB for it. Sincerely, Andrii Anisov.
Hello Julien, чт, 15 лист. 2018 о 22:20 Julien Grall <julien.grall@arm.com> пише: > Well the code suggested is different from what I intended :). t should > be set to invalid before the check mfn_valid/get_page. So this needs at > least 2 checks. 2 places were t != NULL needs to be explained. Well, I guess checking a pointer in order to avoid null pointer dereference is pretty self-explaining. Even if it is done twice. And the code itself would give a clear idea that we do calculate (and assign) type value only in case a caller provided us a valid pointer. For sure, I do not insist on a ternary operator usage. Sincerely, Andrii Anisov.
Hello Julien, It is me again. On 15.11.18 17:05, Julien Grall wrote: > On 11/15/18 1:19 PM, Andrii Anisov wrote: >>> So I would prefer to stick with _t which is quite common within the p2m >>> code base so far. >> >> I've found a similar code only in one place - p2m_get_entry() >> function. And it is, at least, somehow commented there: >> ... >> /* Allow t to be NULL */ >> t = t ?: &_t; >> >> *t = p2m_invalid; >> ... > > And in x86 code... I've taken care to look into the x86 p2m code about that construction. I've found it in x86's `p2m_get_page_from_gfn` function. And it looks reasonable over there. Because that value is not only might be returned to a caller. But is needed by the function code itself to take decisions and passing it to called functions (which do not do a check for the null pointer). Your code (as well as `p2m_get_entry`) does not actually use that value. So I see no need to calculate or keep it if it is not needed by a caller.
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index cec821c3a3..e31b47a394 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1283,7 +1283,7 @@ int xenmem_add_to_physmap_one( } mfn = page_to_mfn(page); - t = p2m_map_foreign_rw; + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro; rcu_unlock_domain(od); break; diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index b32b16cd94..0941be9e41 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -450,6 +450,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) break; case p2m_iommu_map_ro: + case p2m_map_foreign_ro: case p2m_grant_map_ro: case p2m_invalid: e->p2m.xn = 1; diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 5f7f31186d..7c67806056 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -118,6 +118,7 @@ enum p2m_type { p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */ + p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */ p2m_grant_map_rw, /* Read/write grant mapping */ p2m_grant_map_ro, /* Read-only grant mapping */ /* The types below are only used to decide the page attribute in the P2M */ @@ -137,12 +138,16 @@ enum p2m_type { #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) | \ p2m_to_mask(p2m_grant_map_ro)) +/* Foreign mappings types */ +#define P2M_FOREIGN_TYPES (p2m_to_mask(p2m_map_foreign_rw) | \ + p2m_to_mask(p2m_map_foreign_ro)) + /* Useful predicates */ #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES) -#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign_rw)) +#define p2m_is_foreign(_t) (p2m_to_mask(_t) & P2M_FOREIGN_TYPES) #define p2m_is_any_ram(_t) (p2m_to_mask(_t) & \ (P2M_RAM_TYPES | P2M_GRANT_TYPES | \ - p2m_to_mask(p2m_map_foreign_rw))) + P2M_FOREIGN_TYPES)) static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) @@ -275,7 +280,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn, static inline struct page_info *get_page_from_gfn( struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q) { - return p2m_get_page_from_gfn(d, _gfn(gfn), t); + mfn_t mfn; + p2m_type_t _t; + struct page_info *page; + + /* + * Special case for DOMID_XEN as it is the only domain so far that is + * not auto-translated. + */ + if ( unlikely(d != dom_xen) ) + return p2m_get_page_from_gfn(d, _gfn(gfn), t); + + if ( !t ) + t = &_t; + + *t = p2m_invalid; + + /* + * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the + * page. + */ + mfn = _mfn(gfn); + page = mfn_to_page(mfn); + + if ( !mfn_valid(mfn) || !get_page(page, d) ) + return NULL; + + if ( page->u.inuse.type_info & PGT_writable_page ) + *t = p2m_ram_rw; + else + *t = p2m_ram_ro; + + return page; } int get_page_type(struct page_info *page, unsigned long type);
Current, foreign mappings can only be read-write. A follow-up patch will extend foreign mapping for Xen backend memory (via XEN_DOMID), some of that memory should only be read accessible for the mapping domain. Introduce a new p2m_type to cater read-only foreign mappings. For now, the decision between the two foreign mapping type is based on the type of the guest page mapped. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/mm.c | 2 +- xen/arch/arm/p2m.c | 1 + xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 41 insertions(+), 4 deletions(-)