| Message ID | 20170327091059.8452-5-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/03/17 10:10, Wei Liu wrote: > We will later split out PV specific code to pv/mm.c. > > No functional change. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> Please could you take the time to correctly mfn_t/gfn_t the calls. Having both addr and frame, and them being otherwise unqualified, is confusing to read. One idea I had while starting the hypercall work was to introduce a "struct guest_type_ops" to contain some function pointers for options we perform on all guests, irrespective of type. My first candidate for splitting this way was the hypercall page writing. This splitting here is subtly different. Its more "struct paging_type_op", but still common operations we would need to perform for guests. I was thinking that ops structures like this would be cleaner to isolate than needing an explicit dispatch functions such as create_grant_host_mapping() below. Thoughts, (seeing as this is the first time I have floated this idea on xen-devel) ? ~Andrew
>>> On 29.03.17 at 12:27, <andrew.cooper3@citrix.com> wrote: > One idea I had while starting the hypercall work was to introduce a > "struct guest_type_ops" to contain some function pointers for options we > perform on all guests, irrespective of type. My first candidate for > splitting this way was the hypercall page writing. > > This splitting here is subtly different. Its more "struct > paging_type_op", but still common operations we would need to perform > for guests. > > I was thinking that ops structures like this would be cleaner to isolate > than needing an explicit dispatch functions such as > create_grant_host_mapping() below. > > Thoughts, (seeing as this is the first time I have floated this idea on > xen-devel) ? I think that's reasonable as long as we don't go too far with this (indirect calls after all generally having a higher overhead than mis-predicted branches). Jan
On 29/03/17 11:45, Jan Beulich wrote: >>>> On 29.03.17 at 12:27, <andrew.cooper3@citrix.com> wrote: >> One idea I had while starting the hypercall work was to introduce a >> "struct guest_type_ops" to contain some function pointers for options we >> perform on all guests, irrespective of type. My first candidate for >> splitting this way was the hypercall page writing. >> >> This splitting here is subtly different. Its more "struct >> paging_type_op", but still common operations we would need to perform >> for guests. >> >> I was thinking that ops structures like this would be cleaner to isolate >> than needing an explicit dispatch functions such as >> create_grant_host_mapping() below. >> >> Thoughts, (seeing as this is the first time I have floated this idea on >> xen-devel) ? > I think that's reasonable as long as we don't go too far with this > (indirect calls after all generally having a higher overhead than > mis-predicted branches). Without any #ifdefary in a source tree, that is fine. It is harder however if you want to conditionally compile things out. A alternative option would be to have a static inline in a header file which contains the appropriate #ifdefary internally. ~Andrew
On Wed, Mar 29, 2017 at 11:49:54AM +0100, Andrew Cooper wrote: > On 29/03/17 11:45, Jan Beulich wrote: > >>>> On 29.03.17 at 12:27, <andrew.cooper3@citrix.com> wrote: > >> One idea I had while starting the hypercall work was to introduce a > >> "struct guest_type_ops" to contain some function pointers for options we > >> perform on all guests, irrespective of type. My first candidate for > >> splitting this way was the hypercall page writing. > >> > >> This splitting here is subtly different. Its more "struct > >> paging_type_op", but still common operations we would need to perform > >> for guests. > >> > >> I was thinking that ops structures like this would be cleaner to isolate > >> than needing an explicit dispatch functions such as > >> create_grant_host_mapping() below. > >> > >> Thoughts, (seeing as this is the first time I have floated this idea on > >> xen-devel) ? > > I think that's reasonable as long as we don't go too far with this > > (indirect calls after all generally having a higher overhead than > > mis-predicted branches). > > Without any #ifdefary in a source tree, that is fine. It is harder > however if you want to conditionally compile things out. > > A alternative option would be to have a static inline in a header file > which contains the appropriate #ifdefary internally. > Both sound reasonable to me. I slightly prefer guest_op_ops because it seems cleaner. The overhead concern applies to hot path code -- I don't think most guest type specific hypercalls qualify. Wei. > ~Andrew
On Wed, Mar 29, 2017 at 11:27:49AM +0100, Andrew Cooper wrote: > On 27/03/17 10:10, Wei Liu wrote: > > We will later split out PV specific code to pv/mm.c. > > > > No functional change. > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > Please could you take the time to correctly mfn_t/gfn_t the calls. > Having both addr and frame, and them being otherwise unqualified, is > confusing to read. > I'm not sure what you want here. The create_grant_host_mapping function is hooked into grant table code, which is using "unsigned long" everywhere. Modifying it here would require fixing all call sites as well. It'd be better be done in a separate patch. Wei.
On 03/04/17 09:40, Wei Liu wrote: > On Wed, Mar 29, 2017 at 11:27:49AM +0100, Andrew Cooper wrote: >> On 27/03/17 10:10, Wei Liu wrote: >>> We will later split out PV specific code to pv/mm.c. >>> >>> No functional change. >>> >>> Signed-off-by: Wei Liu <wei.liu2@citrix.com> >> Please could you take the time to correctly mfn_t/gfn_t the calls. >> Having both addr and frame, and them being otherwise unqualified, is >> confusing to read. >> > I'm not sure what you want here. > > The create_grant_host_mapping function is hooked into grant table code, > which is using "unsigned long" everywhere. > > Modifying it here would require fixing all call sites as well. It'd be > better be done in a separate patch. Separate patch is fine, but I purposefully want to avoid moving code into pv/ and leaving it with style problems. (i.e. take the time to ensure the status quo improves). Fix it then move it, or move it then fix it, but please don't move it and leave it unfixed. This covers other areas like bool correctness, mfn_t/gfn_t, const correctness, comment styles, whitespace, etc. ~Andrew
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 4bf94227cc..b44d44c782 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4242,15 +4242,12 @@ static int create_grant_p2m_mapping(uint64_t addr, unsigned long frame, return GNTST_okay; } -int create_grant_host_mapping(uint64_t addr, unsigned long frame, - unsigned int flags, unsigned int cache_flags) +static int create_grant_pv_mapping(uint64_t addr, unsigned long frame, + unsigned int flags, unsigned int cache_flags) { l1_pgentry_t pte; uint32_t grant_pte_flags; - if ( paging_mode_external(current->domain) ) - return create_grant_p2m_mapping(addr, frame, flags, cache_flags); - grant_pte_flags = _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_GNTTAB; if ( cpu_has_nx ) @@ -4273,6 +4270,15 @@ int create_grant_host_mapping(uint64_t addr, unsigned long frame, return create_grant_va_mapping(addr, pte, current); } +int create_grant_host_mapping(uint64_t addr, unsigned long frame, + unsigned int flags, unsigned int cache_flags) +{ + if ( paging_mode_external(current->domain) ) + return create_grant_p2m_mapping(addr, frame, flags, cache_flags); + + return create_grant_pv_mapping(addr, frame, flags, cache_flags); +} + static int replace_grant_p2m_mapping( uint64_t addr, unsigned long frame, uint64_t new_addr, unsigned int flags) {
We will later split out PV specific code to pv/mm.c. No functional change. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- xen/arch/x86/mm.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)