| Message ID | 5CF0F5700200007800233DB4@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adjust special domain creation | expand |
Hi Jan, On 5/31/19 10:35 AM, Jan Beulich wrote: > A couple of adjustments are needed to code checking for dom_cow, but > since there are pretty few it is probably better to adjust those than > to set up and keep around a never used domain. > > Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) That's arguably a patch on its own so you can explain why it is tighten. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > While for now this avoids creating the domain on Arm only, Tamas'es > patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. > > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -967,8 +967,8 @@ get_page_from_l1e( > return flip; > } > > - if ( unlikely( (real_pg_owner != pg_owner) && > - (real_pg_owner != dom_cow) ) ) > + if ( unlikely((real_pg_owner != pg_owner) && > + (!dom_cow || (real_pg_owner != dom_cow))) ) > { > /* > * Let privileged domains transfer the right to map their target > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -568,7 +568,8 @@ struct page_info *p2m_get_page_from_gfn( > } > else if ( !get_page(page, p2m->domain) && > /* Page could be shared */ > - (!p2m_is_shared(*t) || !get_page(page, dom_cow)) ) > + (!dom_cow || !p2m_is_shared(*t) || > + !get_page(page, dom_cow)) ) > page = NULL; > } > p2m_read_unlock(p2m); > @@ -941,7 +942,8 @@ guest_physmap_add_entry(struct domain *d > /* Then, look for m->p mappings for this range and deal with them */ > for ( i = 0; i < (1UL << page_order); i++ ) > { > - if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) > + if ( dom_cow && > + page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) > { > /* This is no way to add a shared page to your physmap! */ > gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom%d physmap not allowed.\n", > --- a/xen/arch/x86/pv/emul-priv-op.c > +++ b/xen/arch/x86/pv/emul-priv-op.c > @@ -723,8 +723,8 @@ static int read_cr(unsigned int reg, uns > unmap_domain_page(pl4e); > *val = compat_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn))); > } > - /* PTs should not be shared */ > - BUG_ON(page_get_owner(mfn_to_page(mfn)) == dom_cow); > + /* PTs should be owned by their domains */ > + BUG_ON(page_get_owner(mfn_to_page(mfn)) != currd); > return X86EMUL_OKAY; > } > } > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -74,7 +74,9 @@ integer_param("hardware_dom", hardware_d > /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ > struct domain *__read_mostly dom_xen; > struct domain *__read_mostly dom_io; > +#ifdef CONFIG_HAS_MEM_SHARING > struct domain *__read_mostly dom_cow; > +#endif > > struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; > > @@ -544,12 +546,14 @@ void __init setup_special_domains(void) > dom_io = domain_create(DOMID_IO, NULL, false); > BUG_ON(IS_ERR(dom_io)); > > +#ifdef CONFIG_HAS_MEM_SHARING > /* > * Initialise our COW domain. > * This domain owns sharable pages. > */ > dom_cow = domain_create(DOMID_COW, NULL, false); > BUG_ON(IS_ERR(dom_cow)); > +#endif > } > > void domain_update_node_affinity(struct domain *d) > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -1095,7 +1095,7 @@ map_grant_ref( > host_map_created = true; > } > } > - else if ( owner == rd || owner == dom_cow ) > + else if ( owner == rd || (dom_cow && owner == dom_cow) ) > { > if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) ) > { > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma > > /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ > extern struct domain *dom_xen, *dom_io, *dom_cow; Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? > +#ifndef CONFIG_HAS_MEM_SHARING > +# define dom_cow NULL > +#endif > > enum XENSHARE_flags { > SHARE_rw, > > >
>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: > On 5/31/19 10:35 AM, Jan Beulich wrote: >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >> >> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >> extern struct domain *dom_xen, *dom_io, *dom_cow; > > Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? There's no need to with ... >> +#ifndef CONFIG_HAS_MEM_SHARING >> +# define dom_cow NULL >> +#endif ... this, and this way there's less clutter overall. Jan
Hi, On 5/31/19 10:49 AM, Jan Beulich wrote: >>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >> On 5/31/19 10:35 AM, Jan Beulich wrote: >>> --- a/xen/include/xen/mm.h >>> +++ b/xen/include/xen/mm.h >>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>> >>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>> extern struct domain *dom_xen, *dom_io, *dom_cow; >> >> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? > > There's no need to with ... > >>> +#ifndef CONFIG_HAS_MEM_SHARING >>> +# define dom_cow NULL >>> +#endif > > ... this, and this way there's less clutter overall. I am all for avoiding cluttering but not at the expense of making the code less intuitive. In this case, I would prefer the decleration dom_cow to be guarded. Cheers,
>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: > On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>> --- a/xen/include/xen/mm.h >>>> +++ b/xen/include/xen/mm.h >>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>> >>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>> >>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >> >> There's no need to with ... >> >>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>> +# define dom_cow NULL >>>> +#endif >> >> ... this, and this way there's less clutter overall. > > I am all for avoiding cluttering but not at the expense of making the > code less intuitive. In this case, I would prefer the decleration > dom_cow to be guarded. While it would be easy enough to do, I fail to see how the chosen construct is not "intuitive". In fact I don't think this would be the first instance of a #define overriding a prior declaration. Doing so utilizes one of the very basic C preprocessor principles. Jan
Hi Jan, On 5/31/19 11:03 AM, Jan Beulich wrote: >>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: >> On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>>> --- a/xen/include/xen/mm.h >>>>> +++ b/xen/include/xen/mm.h >>>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>>> >>>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>>> >>>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >>> >>> There's no need to with ... >>> >>>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>>> +# define dom_cow NULL >>>>> +#endif >>> >>> ... this, and this way there's less clutter overall. >> >> I am all for avoiding cluttering but not at the expense of making the >> code less intuitive. In this case, I would prefer the decleration >> dom_cow to be guarded. > > While it would be easy enough to do, I fail to see how the chosen > construct is not "intuitive". Even if I know the pre-preprocessor will do the right thing here, you could spare the developper to trip over this code and wonder why it is first defined and then override with NULL. > In fact I don't think this would be the > first instance of a #define overriding a prior declaration. Doing so > utilizes one of the very basic C preprocessor principles. You are the first one usually to say that some choices in Xen were not correct and therefore no more instance should be added. This is one case where the resulting code is counter-intuitive and ugly. Cheers,
>>> On 31.05.19 at 12:10, <julien.grall@arm.com> wrote: > On 5/31/19 11:03 AM, Jan Beulich wrote: >>>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: >>> On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>>>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>>>> --- a/xen/include/xen/mm.h >>>>>> +++ b/xen/include/xen/mm.h >>>>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>>>> >>>>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>>>> >>>>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >>>> >>>> There's no need to with ... >>>> >>>>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>>>> +# define dom_cow NULL >>>>>> +#endif >>>> >>>> ... this, and this way there's less clutter overall. >>> >>> I am all for avoiding cluttering but not at the expense of making the >>> code less intuitive. In this case, I would prefer the decleration >>> dom_cow to be guarded. >> >> While it would be easy enough to do, I fail to see how the chosen >> construct is not "intuitive". > > Even if I know the pre-preprocessor will do the right thing here, you > could spare the developper to trip over this code and wonder why it is > first defined and then override with NULL. To be honest, an unconditional declaration with a conditional override doesn't leave much to wonder about. I'll wait to see what other maintainers think before deciding either way. >> In fact I don't think this would be the >> first instance of a #define overriding a prior declaration. Doing so >> utilizes one of the very basic C preprocessor principles. > > You are the first one usually to say that some choices in Xen were not > correct and therefore no more instance should be added. Oh, did my earlier reply sound as if I'm not happy about those mentioned instances? I certainly didn't mean it to be that way - some of them I'm liable to have introduced myself, and I continue to approve of them being there. Jan
Hi Jan, On 5/31/19 11:31 AM, Jan Beulich wrote: >>>> On 31.05.19 at 12:10, <julien.grall@arm.com> wrote: >> On 5/31/19 11:03 AM, Jan Beulich wrote: >>>>>> On 31.05.19 at 11:52, <julien.grall@arm.com> wrote: >>>> On 5/31/19 10:49 AM, Jan Beulich wrote: >>>>>>>> On 31.05.19 at 11:42, <julien.grall@arm.com> wrote: >>>>>> On 5/31/19 10:35 AM, Jan Beulich wrote: >>>>>>> --- a/xen/include/xen/mm.h >>>>>>> +++ b/xen/include/xen/mm.h >>>>>>> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >>>>>>> >>>>>>> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >>>>>>> extern struct domain *dom_xen, *dom_io, *dom_cow; >>>>>> >>>>>> Don't you want to protect dom_cow with "#ifdef CONFIG_HAS_MEM_SHARING"? >>>>> >>>>> There's no need to with ... >>>>> >>>>>>> +#ifndef CONFIG_HAS_MEM_SHARING >>>>>>> +# define dom_cow NULL >>>>>>> +#endif >>>>> >>>>> ... this, and this way there's less clutter overall. >>>> >>>> I am all for avoiding cluttering but not at the expense of making the >>>> code less intuitive. In this case, I would prefer the decleration >>>> dom_cow to be guarded. >>> >>> While it would be easy enough to do, I fail to see how the chosen >>> construct is not "intuitive". >> >> Even if I know the pre-preprocessor will do the right thing here, you >> could spare the developper to trip over this code and wonder why it is >> first defined and then override with NULL. > > To be honest, an unconditional declaration with a conditional > override doesn't leave much to wonder about. I'll wait to see > what other maintainers think before deciding either way. > >>> In fact I don't think this would be the >>> first instance of a #define overriding a prior declaration. Doing so >>> utilizes one of the very basic C preprocessor principles. >> >> You are the first one usually to say that some choices in Xen were not >> correct and therefore no more instance should be added. > > Oh, did my earlier reply sound as if I'm not happy about those > mentioned instances? No it was a more generic statement on the stance "It already exists in Xen so it is fine to spread them a bit more". Cheers,
>>> On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: > No it was a more generic statement on the stance "It already exists in > Xen so it is fine to spread them a bit more". Oh, I see. Of course I'm making remarks when what's in the tree is bad (as per e.g. coding style, or if not mentioned there than in my personal opinion). As a result I take note of you thinking this being bad practice, and the two of us disagreeing. I'm certainly willing to adjust non-obvious code to a more obvious shape in various cases, but I think there needs to be a limit as to what language features we decide should not be used in the code base. Overriding declarations (and in some cases even definitions) by macros is a useful thing for general readability in certain cases in my opinion, and while it's not making much of difference here I'd still prefer if I was allowed to get away with this, unless a majority supports your view. IOW - your change request is, as per my own perspective, making the code less easy to read, even if not by much. Jan
Hi Jan, On 31/05/2019 11:46, Jan Beulich wrote: >>>> On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: >> No it was a more generic statement on the stance "It already exists in >> Xen so it is fine to spread them a bit more". > > Oh, I see. Of course I'm making remarks when what's in the tree is > bad (as per e.g. coding style, or if not mentioned there than in my > personal opinion). As a result I take note of you thinking this being > bad practice, and the two of us disagreeing. I'm certainly willing to > adjust non-obvious code to a more obvious shape in various cases, > but I think there needs to be a limit as to what language features > we decide should not be used in the code base. Overriding > declarations (and in some cases even definitions) by macros is a > useful thing for general readability in certain cases in my opinion, > and while it's not making much of difference here I'd still prefer if > I was allowed to get away with this, unless a majority supports > your view. IOW - your change request is, as per my own > perspective, making the code less easy to read, even if not by > much. Let will wait the opinion from the others here. Cheers,
On 31/05/2019 02:35, Jan Beulich wrote: > A couple of adjustments are needed to code checking for dom_cow, but > since there are pretty few it is probably better to adjust those than > to set up and keep around a never used domain. > > Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) Yes - it should be ASSERT_UNREACHABLE()/domain_crash() I'm not fussed if this done as part of this patch, or split out separately. It almost doesn't seem worth splitting out. > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma > > /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ > extern struct domain *dom_xen, *dom_io, *dom_cow; > +#ifndef CONFIG_HAS_MEM_SHARING > +# define dom_cow NULL > +#endif > > enum XENSHARE_flags { > SHARE_rw, > > > What is wrong with #ifdef CONFIG_HAS_MEM_SHARING extern struct domain *dom_cow; #else # define dom_cow NULL #endif which is how we usually express things like this? Sure, its a tiny bit longer to write, but it is easier to follow. ~Andrew
On Fri, 31 May 2019, Julien Grall wrote: > Hi Jan, > > On 31/05/2019 11:46, Jan Beulich wrote: > > > > > On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: > > > No it was a more generic statement on the stance "It already exists in > > > Xen so it is fine to spread them a bit more". > > > > Oh, I see. Of course I'm making remarks when what's in the tree is > > bad (as per e.g. coding style, or if not mentioned there than in my > > personal opinion). As a result I take note of you thinking this being > > bad practice, and the two of us disagreeing. I'm certainly willing to > > adjust non-obvious code to a more obvious shape in various cases, > > but I think there needs to be a limit as to what language features > > we decide should not be used in the code base. Overriding > > declarations (and in some cases even definitions) by macros is a > > useful thing for general readability in certain cases in my opinion, > > and while it's not making much of difference here I'd still prefer if > > I was allowed to get away with this, unless a majority supports > > your view. IOW - your change request is, as per my own > > perspective, making the code less easy to read, even if not by > > much. > > Let will wait the opinion from the others here. My preference is what Andrew suggested: #ifdef CONFIG_HAS_MEM_SHARING extern struct domain *dom_cow; #else define dom_cow NULL #endif and I find Jan's original version harder to read. However, for code style related things, I prefer to "suggest" to other maintainers one way or the other, rather than "request" for a change. If something bother us enough to do something about it, the way to go is to send a patch to CODING_STYLE so that we are all on the same page.
Hi, On 5/31/19 6:27 PM, Stefano Stabellini wrote: > On Fri, 31 May 2019, Julien Grall wrote: >> Hi Jan, >> >> On 31/05/2019 11:46, Jan Beulich wrote: >>>>>> On 31.05.19 at 12:34, <julien.grall@arm.com> wrote: >>>> No it was a more generic statement on the stance "It already exists in >>>> Xen so it is fine to spread them a bit more". >>> >>> Oh, I see. Of course I'm making remarks when what's in the tree is >>> bad (as per e.g. coding style, or if not mentioned there than in my >>> personal opinion). As a result I take note of you thinking this being >>> bad practice, and the two of us disagreeing. I'm certainly willing to >>> adjust non-obvious code to a more obvious shape in various cases, >>> but I think there needs to be a limit as to what language features >>> we decide should not be used in the code base. Overriding >>> declarations (and in some cases even definitions) by macros is a >>> useful thing for general readability in certain cases in my opinion, >>> and while it's not making much of difference here I'd still prefer if >>> I was allowed to get away with this, unless a majority supports >>> your view. IOW - your change request is, as per my own >>> perspective, making the code less easy to read, even if not by >>> much. >> >> Let will wait the opinion from the others here. > > My preference is what Andrew suggested: > > #ifdef CONFIG_HAS_MEM_SHARING > extern struct domain *dom_cow; > #else > define dom_cow NULL > #endif > > and I find Jan's original version harder to read. However, for code > style related things, I prefer to "suggest" to other maintainers one way > or the other, rather than "request" for a change. Note that I wrote "I would prefer" in my e-mail and the agreement was to wait on other view. Cheers,
On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: > > A couple of adjustments are needed to code checking for dom_cow, but > since there are pretty few it is probably better to adjust those than > to set up and keep around a never used domain. > > Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > While for now this avoids creating the domain on Arm only, Tamas'es > patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. Hi Jan, perhaps it would be better to have this patch be applied after my patch? You already acked that one and it could be applied separately from the series that I've sent it as part of. Thanks, Tamas
>>> On 31.05.19 at 19:13, <andrew.cooper3@citrix.com> wrote: > On 31/05/2019 02:35, Jan Beulich wrote: >> A couple of adjustments are needed to code checking for dom_cow, but >> since there are pretty few it is probably better to adjust those than >> to set up and keep around a never used domain. >> >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) > > Yes - it should be ASSERT_UNREACHABLE()/domain_crash() > > I'm not fussed if this done as part of this patch, or split out > separately. It almost doesn't seem worth splitting out. Well, to do both changes at the same time, I'll really split this out into a prereq patch. >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma >> >> /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ >> extern struct domain *dom_xen, *dom_io, *dom_cow; >> +#ifndef CONFIG_HAS_MEM_SHARING >> +# define dom_cow NULL >> +#endif >> >> enum XENSHARE_flags { >> SHARE_rw, > > What is wrong with > > #ifdef CONFIG_HAS_MEM_SHARING > extern struct domain *dom_cow; > #else > # define dom_cow NULL > #endif > > which is how we usually express things like this? Sure, its a tiny bit > longer to write, but it is easier to follow. Well, since you're the second one to ask, I'll switch, despite not agreeing with this. Yet again some use of the C language that apparently needs to be listed in ./CODING_STYLE as unwanted / forbidden. Jan
>>> On 02.06.19 at 02:40, <tamas@tklengyel.com> wrote: > On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> A couple of adjustments are needed to code checking for dom_cow, but >> since there are pretty few it is probably better to adjust those than >> to set up and keep around a never used domain. >> >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> While for now this avoids creating the domain on Arm only, Tamas'es >> patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. > > perhaps it would be better to have this patch be applied after my > patch? You already acked that one and it could be applied separately > from the series that I've sent it as part of. Oh, I didn't realize it is entirely independent of the earlier patches. It would help to point such out in the cover letter, requiring there to be one in the first place. Yet even then - it's lacking an XSM maintainer ack afaics (and it looks as if you didn't even Cc Daniel), and I'd prefer a patch like this to also have an ack from George, even if judging by the files touched this may not be strictly required. Jan
On Mon, Jun 3, 2019 at 2:25 AM Jan Beulich <JBeulich@suse.com> wrote: > > >>> On 02.06.19 at 02:40, <tamas@tklengyel.com> wrote: > > On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: > >> > >> A couple of adjustments are needed to code checking for dom_cow, but > >> since there are pretty few it is probably better to adjust those than > >> to set up and keep around a never used domain. > >> > >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). > >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> While for now this avoids creating the domain on Arm only, Tamas'es > >> patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. > > > > perhaps it would be better to have this patch be applied after my > > patch? You already acked that one and it could be applied separately > > from the series that I've sent it as part of. > > Oh, I didn't realize it is entirely independent of the earlier patches. > It would help to point such out in the cover letter, requiring there > to be one in the first place. Right, I should have added a cover letter. That "series" is more like an assorted collection of fixes rather then a series in a strict sense. Tamas
>>> On 03.06.19 at 21:57, <tamas@tklengyel.com> wrote: > On Mon, Jun 3, 2019 at 2:25 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> >>> On 02.06.19 at 02:40, <tamas@tklengyel.com> wrote: >> > On Fri, May 31, 2019 at 3:35 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> >> >> A couple of adjustments are needed to code checking for dom_cow, but >> >> since there are pretty few it is probably better to adjust those than >> >> to set up and keep around a never used domain. >> >> >> >> Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). >> >> (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) >> >> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- >> >> While for now this avoids creating the domain on Arm only, Tamas'es >> >> patch switching to CONFIG_MEM_SHARING will make x86 leverage this too. >> > >> > perhaps it would be better to have this patch be applied after my >> > patch? You already acked that one and it could be applied separately >> > from the series that I've sent it as part of. >> >> Oh, I didn't realize it is entirely independent of the earlier patches. >> It would help to point such out in the cover letter, requiring there >> to be one in the first place. > > Right, I should have added a cover letter. That "series" is more like > an assorted collection of fixes rather then a series in a strict > sense. In which case an option would have been to send four standalone patches. (I'm definitely no consistent with this myself - there are cases where I collect things into a series, but there are also cases where I send multiple individual patches.) Jan
--- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -967,8 +967,8 @@ get_page_from_l1e( return flip; } - if ( unlikely( (real_pg_owner != pg_owner) && - (real_pg_owner != dom_cow) ) ) + if ( unlikely((real_pg_owner != pg_owner) && + (!dom_cow || (real_pg_owner != dom_cow))) ) { /* * Let privileged domains transfer the right to map their target --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -568,7 +568,8 @@ struct page_info *p2m_get_page_from_gfn( } else if ( !get_page(page, p2m->domain) && /* Page could be shared */ - (!p2m_is_shared(*t) || !get_page(page, dom_cow)) ) + (!dom_cow || !p2m_is_shared(*t) || + !get_page(page, dom_cow)) ) page = NULL; } p2m_read_unlock(p2m); @@ -941,7 +942,8 @@ guest_physmap_add_entry(struct domain *d /* Then, look for m->p mappings for this range and deal with them */ for ( i = 0; i < (1UL << page_order); i++ ) { - if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) + if ( dom_cow && + page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow ) { /* This is no way to add a shared page to your physmap! */ gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom%d physmap not allowed.\n", --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -723,8 +723,8 @@ static int read_cr(unsigned int reg, uns unmap_domain_page(pl4e); *val = compat_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn))); } - /* PTs should not be shared */ - BUG_ON(page_get_owner(mfn_to_page(mfn)) == dom_cow); + /* PTs should be owned by their domains */ + BUG_ON(page_get_owner(mfn_to_page(mfn)) != currd); return X86EMUL_OKAY; } } --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -74,7 +74,9 @@ integer_param("hardware_dom", hardware_d /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ struct domain *__read_mostly dom_xen; struct domain *__read_mostly dom_io; +#ifdef CONFIG_HAS_MEM_SHARING struct domain *__read_mostly dom_cow; +#endif struct vcpu *idle_vcpu[NR_CPUS] __read_mostly; @@ -544,12 +546,14 @@ void __init setup_special_domains(void) dom_io = domain_create(DOMID_IO, NULL, false); BUG_ON(IS_ERR(dom_io)); +#ifdef CONFIG_HAS_MEM_SHARING /* * Initialise our COW domain. * This domain owns sharable pages. */ dom_cow = domain_create(DOMID_COW, NULL, false); BUG_ON(IS_ERR(dom_cow)); +#endif } void domain_update_node_affinity(struct domain *d) --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1095,7 +1095,7 @@ map_grant_ref( host_map_created = true; } } - else if ( owner == rd || owner == dom_cow ) + else if ( owner == rd || (dom_cow && owner == dom_cow) ) { if ( (op->flags & GNTMAP_device_map) && !(op->flags & GNTMAP_readonly) ) { --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -644,6 +644,9 @@ static inline void filtered_flush_tlb_ma /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */ extern struct domain *dom_xen, *dom_io, *dom_cow; +#ifndef CONFIG_HAS_MEM_SHARING +# define dom_cow NULL +#endif enum XENSHARE_flags { SHARE_rw,
A couple of adjustments are needed to code checking for dom_cow, but since there are pretty few it is probably better to adjust those than to set up and keep around a never used domain. Take the opportunity and tighten a BUG_ON() in emul-priv-op.c:read_cr(). (Arguably this perhaps shouldn't be a BUG_ON() in the first place.) Signed-off-by: Jan Beulich <jbeulich@suse.com> --- While for now this avoids creating the domain on Arm only, Tamas'es patch switching to CONFIG_MEM_SHARING will make x86 leverage this too.