[PATCH v2 0/2] x86/p2m: type checking adjustments

[PATCH v2 0/2] x86/p2m: type checking adjustments

[Jan Beulich]

While the first change is a bug fix (for, admittedly, a case which
apparently hasn't occurred in practice, or else we would have had
bug reports), it already puts in place an instance of what the 2nd
patch is proposing for perhaps wider use.

1: make p2m_get_page_from_gfn() handle grant and shared cases better
2: aid the compiler in folding p2m_is_...()

Jan

[PATCH v2 1/2] x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly

[Jan Beulich]

Grant P2M entries, which are covered by p2m_is_any_ram(), wouldn't pass
the get_page() unless the grant was a local one. These need to take the
same path as foreign entries. Just the assertion there is not valid for
local grants, and hence it triggering needs to be avoided.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Using | instead of || helps the compiler fold the two p2m_is_*().
---
v2: The shared case was fine; limit to grant adjustment.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -357,11 +357,11 @@ struct page_info *p2m_get_page_from_gfn(
              && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
         {
             page = mfn_to_page(mfn);
-            if ( unlikely(p2m_is_foreign(*t)) )
+            if ( unlikely(p2m_is_foreign(*t) | p2m_is_grant(*t)) )
             {
                 struct domain *fdom = page_get_owner_and_reference(page);
 
-                ASSERT(fdom != p2m->domain);
+                ASSERT(!p2m_is_foreign(*t) || fdom != p2m->domain);
                 if ( fdom == NULL )
                     page = NULL;
             }

[PATCH v2 2/2] x86/p2m: aid the compiler in folding p2m_is_...()

[Jan Beulich]

By using | instead of || or (in the negated form) && chances increase
for the compiler to recognize that both predicates can actually be
folded into an expression requiring just a single branch (via OR-ing
together the respective P2M_*_TYPES constants).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: The 3-way checks look to be a general problem for gcc, but even in
     some 2-way cases it doesn't manage to fold the expressions. Hence
     it's worth considering to go farther with this transformation, as
     long as the idea isn't disliked in general.
---
v2: Re-base over change to earlier patch.

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -379,7 +379,7 @@ struct page_info *p2m_get_page_from_gfn(
             return page;
 
         /* Error path: not a suitable GFN at all */
-        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
+        if ( !(p2m_is_ram(*t) | p2m_is_paging(*t) | p2m_is_pod(*t)) &&
              !mem_sharing_is_fork(p2m->domain) )
             return NULL;
     }
@@ -568,7 +568,7 @@ p2m_remove_entry(struct p2m_domain *p2m,
     for ( i = 0; i < (1UL << page_order); ++i )
     {
         p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0, NULL, NULL);
-        if ( !p2m_is_hole(t) && !p2m_is_special(t) && !p2m_is_shared(t) )
+        if ( !(p2m_is_hole(t) | p2m_is_special(t) | p2m_is_shared(t)) )
         {
             set_gpfn_from_mfn(mfn_x(mfn) + i, gfn_x(gfn) + i);
             paging_mark_pfn_dirty(p2m->domain, _pfn(gfn_x(gfn) + i));

Re: [PATCH v2 2/2] x86/p2m: aid the compiler in folding p2m_is_...()

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> wrote:

> By using | instead of || or (in the negated form) && chances increase
> for the compiler to recognize that both predicates can actually be
> folded into an expression requiring just a single branch (via OR-ing
> together the respective P2M_*_TYPES constants).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>

Sorry for the delay.  Git complains that this patch is malformed:

error: `git apply --index`: error: corrupt patch at line 28

Similar complaint from patchew when it was posted:

https://patchew.org/Xen/5d6c927e-7d7c-5754-e7eb-65d1e70f6222@suse.com/

 -George

[-- Attachment #2: Type: text/html, Size: 1322 bytes --]

Re: [PATCH v2 2/2] x86/p2m: aid the compiler in folding p2m_is_...()

[Jan Beulich]

On 01.02.2024 14:32, George Dunlap wrote:
> On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> By using | instead of || or (in the negated form) && chances increase
>> for the compiler to recognize that both predicates can actually be
>> folded into an expression requiring just a single branch (via OR-ing
>> together the respective P2M_*_TYPES constants).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
> 
> Sorry for the delay.  Git complains that this patch is malformed:
> 
> error: `git apply --index`: error: corrupt patch at line 28
> 
> Similar complaint from patchew when it was posted:
> 
> https://patchew.org/Xen/5d6c927e-7d7c-5754-e7eb-65d1e70f6222@suse.com/

Not sure what to say. The patch surely is well-formed. It applies fine
using patch (when not taken from email). When taken from email, patch
mentions that it strips CRs (I'm running my email client on Windows),
but the saved email still applies fine. "git am" indeed is unhappy
when taking the plain file as saved from email, albeit here with an
error different from yours. If I edit the saved email to retain just
the From: and Subject: tags, all is fine.

I can't tell what git doesn't like. The error messages (the one you
see and the one I got) tell me nothing. I'm also not aware of there
being a requirement that patches I send via email need to be
"git am"-able (unlike in xsa.git, where I edit patches enough to be
suitable for that), nor am I aware how I would convince my email
client and/or server to omit whatever git doesn't like or to add
whatever git is missing.

Bottom line - your response would be actionable by me only in so far
as I could switch to using "git send-email". Which I'm afraid I'm not
going to do unless left with no other choice. The way I've been
sending patches has worked well for over 20 years, and for different
projects. (I'm aware Andrew has some special "Jan" command to apply
patches I send, but I don't know any specifics.)

Jan

Re: [PATCH v2 2/2] x86/p2m: aid the compiler in folding p2m_is_...()

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 01.02.2024 14:32, George Dunlap wrote:
> > On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> >> By using | instead of || or (in the negated form) && chances increase
> >> for the compiler to recognize that both predicates can actually be
> >> folded into an expression requiring just a single branch (via OR-ing
> >> together the respective P2M_*_TYPES constants).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >
> > Sorry for the delay.  Git complains that this patch is malformed:
> >
> > error: `git apply --index`: error: corrupt patch at line 28
> >
> > Similar complaint from patchew when it was posted:
> >
> > https://patchew.org/Xen/5d6c927e-7d7c-5754-e7eb-65d1e70f6222@suse.com/
>
> Not sure what to say. The patch surely is well-formed. It applies fine
> using patch (when not taken from email). When taken from email, patch
> mentions that it strips CRs (I'm running my email client on Windows),
> but the saved email still applies fine. "git am" indeed is unhappy
> when taking the plain file as saved from email, albeit here with an
> error different from yours. If I edit the saved email to retain just
> the From: and Subject: tags, all is fine.
>

That still doesn't work for me.


> I can't tell what git doesn't like. The error messages (the one you
> see and the one I got) tell me nothing.


The raw email looks like this:

```
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -379,7 +379,7 @@ struct page_info *p2m_get_page_from_gfn(
             return page;
=20
         /* Error path: not a suitable GFN at all */
-        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
+        if ( !(p2m_is_ram(*t) | p2m_is_paging(*t) | p2m_is_pod(*t)) &&
              !mem_sharing_is_fork(p2m->domain) )
             return NULL;
     }
```

Note the "=20" at the beginning of the empty line.  Why `patch` handles it
but `git am` doesn't, who knows.


> I'm also not aware of there
> being a requirement that patches I send via email need to be
> "git am"-able (unlike in xsa.git, where I edit patches enough to be
> suitable for that), nor am I aware how I would convince my email
> client and/or server to omit whatever git doesn't like or to add
> whatever git is missing.
>
> Bottom line - your response would be actionable by me only in so far
> as I could switch to using "git send-email". Which I'm afraid I'm not
> going to do unless left with no other choice. The way I've been
> sending patches has worked well for over 20 years, and for different
> projects. (I'm aware Andrew has some special "Jan" command to apply
> patches I send, but I don't know any specifics.)
>

In the general case, I'm not going to review a patch without being able to
see it in context; and it's not reasonable to expect reviewers to have
specific contributor-specific scripts for doing so.  If we run into this
issue in the future, and you want my review, you may have to post a git
tree somewhere, or attach the patch as an attachment or something.  (Or you
can try to figure out why `git am` isn't working and try to upstream a fix.)

That said, in this case, context isn't really necessary to understand the
change, so it won't be necessary.

The logic of the change is obviously correct; but it definitely reduces the
readability.  I kind of feel like whether this sort of optimization is
worth the benefits is more a general x86 maintainer policy decision.  Maybe
we can talk about it at the next maintainer's meeting I'll be at?

 -George

[-- Attachment #2: Type: text/html, Size: 5036 bytes --]

Re: [PATCH v2 2/2] x86/p2m: aid the compiler in folding p2m_is_...()

[Jan Beulich]

On 07.02.2024 04:07, George Dunlap wrote:
> On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 01.02.2024 14:32, George Dunlap wrote:
>>> On Thu, Jun 23, 2022 at 12:54 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>>> By using | instead of || or (in the negated form) && chances increase
>>>> for the compiler to recognize that both predicates can actually be
>>>> folded into an expression requiring just a single branch (via OR-ing
>>>> together the respective P2M_*_TYPES constants).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>
>>> Sorry for the delay.  Git complains that this patch is malformed:
>>>
>>> error: `git apply --index`: error: corrupt patch at line 28
>>>
>>> Similar complaint from patchew when it was posted:
>>>
>>> https://patchew.org/Xen/5d6c927e-7d7c-5754-e7eb-65d1e70f6222@suse.com/
>>
>> Not sure what to say. The patch surely is well-formed. It applies fine
>> using patch (when not taken from email). When taken from email, patch
>> mentions that it strips CRs (I'm running my email client on Windows),
>> but the saved email still applies fine. "git am" indeed is unhappy
>> when taking the plain file as saved from email, albeit here with an
>> error different from yours. If I edit the saved email to retain just
>> the From: and Subject: tags, all is fine.
>>
> 
> That still doesn't work for me.
> 
> 
>> I can't tell what git doesn't like. The error messages (the one you
>> see and the one I got) tell me nothing.
> 
> 
> The raw email looks like this:
> 
> ```
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -379,7 +379,7 @@ struct page_info *p2m_get_page_from_gfn(
>              return page;
> =20
>          /* Error path: not a suitable GFN at all */
> -        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
> +        if ( !(p2m_is_ram(*t) | p2m_is_paging(*t) | p2m_is_pod(*t)) &&
>               !mem_sharing_is_fork(p2m->domain) )
>              return NULL;
>      }
> ```
> 
> Note the "=20" at the beginning of the empty line.  Why `patch` handles it
> but `git am` doesn't, who knows.

Hmm. Nothing like that seen when I save that mail. Plus I recall having
an issue with this when applying patches coming from Shawn, where those
=20 got in the way, but only if I pruned the saved email before handing
to "git am".

>> I'm also not aware of there
>> being a requirement that patches I send via email need to be
>> "git am"-able (unlike in xsa.git, where I edit patches enough to be
>> suitable for that), nor am I aware how I would convince my email
>> client and/or server to omit whatever git doesn't like or to add
>> whatever git is missing.
>>
>> Bottom line - your response would be actionable by me only in so far
>> as I could switch to using "git send-email". Which I'm afraid I'm not
>> going to do unless left with no other choice. The way I've been
>> sending patches has worked well for over 20 years, and for different
>> projects. (I'm aware Andrew has some special "Jan" command to apply
>> patches I send, but I don't know any specifics.)
>>
> 
> In the general case, I'm not going to review a patch without being able to
> see it in context; and it's not reasonable to expect reviewers to have
> specific contributor-specific scripts for doing so.  If we run into this
> issue in the future, and you want my review, you may have to post a git
> tree somewhere, or attach the patch as an attachment or something.  (Or you
> can try to figure out why `git am` isn't working and try to upstream a fix.)

Based on my own observation mentioned above, I assume "git am" is capable
of dealing with the =20, provided some specific further encoding
specification is present in the mail. Which I'd then have to assume is
missing from what Thunderbird sends, or the =20 is being introduced
without Thunderbird being involved.

> That said, in this case, context isn't really necessary to understand the
> change, so it won't be necessary.
> 
> The logic of the change is obviously correct; but it definitely reduces the
> readability.  I kind of feel like whether this sort of optimization is
> worth the benefits is more a general x86 maintainer policy decision.  Maybe
> we can talk about it at the next maintainer's meeting I'll be at?

I see no problem doing so.

Jan

Re: [PATCH v2 1/2] x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On Thu, Jun 23, 2022 at 7:54 PM Jan Beulich <jbeulich@suse.com> wrote:

> Grant P2M entries, which are covered by p2m_is_any_ram(), wouldn't pass
> the get_page() unless the grant was a local one. These need to take the
> same path as foreign entries. Just the assertion there is not valid for
> local grants, and hence it triggering needs to be avoided.
>

I think I'd say:

---
The 'fast' path of p2m_get_page_from_gfn handles three cases: normal ram,
foreign p2m entries, and grant map entries.  For normal ram and grant table
entries, get_page() is called, but for foreign entries,
page_get_owner_and_reference() is called, since the current domain is
expected not to be the owner.

Unfortunately, grant maps are *also* generally expected to be owned by
foreign domains; so this function will fail for any p2m entry containing a
grant map that doesn't happen to be local.

Have grant maps take the same path as foreign entries.  Since grants may
actually be either foreign or local, adjust the assertion to allow for this.
---

One more comment...


> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Using | instead of || helps the compiler fold the two p2m_is_*().
> ---
> v2: The shared case was fine; limit to grant adjustment.
>
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -357,11 +357,11 @@ struct page_info *p2m_get_page_from_gfn(
>               && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
>          {
>              page = mfn_to_page(mfn);
> -            if ( unlikely(p2m_is_foreign(*t)) )
> +            if ( unlikely(p2m_is_foreign(*t) | p2m_is_grant(*t)) )
>

I'm not a fan of this.  If you replace it with || you can have my R-b
immediately; otherwise we'll have to wait until we can discuss our general
policy on this sort of thing at the x86 maintainer's call.

 -George

[-- Attachment #2: Type: text/html, Size: 2665 bytes --]

Re: [PATCH v2 1/2] x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly

[Jan Beulich]

On 08.02.2024 07:32, George Dunlap wrote:
> On Thu, Jun 23, 2022 at 7:54 PM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> Grant P2M entries, which are covered by p2m_is_any_ram(), wouldn't pass
>> the get_page() unless the grant was a local one. These need to take the
>> same path as foreign entries. Just the assertion there is not valid for
>> local grants, and hence it triggering needs to be avoided.
>>
> 
> I think I'd say:
> 
> ---
> The 'fast' path of p2m_get_page_from_gfn handles three cases: normal ram,
> foreign p2m entries, and grant map entries.  For normal ram and grant table
> entries, get_page() is called, but for foreign entries,
> page_get_owner_and_reference() is called, since the current domain is
> expected not to be the owner.
> 
> Unfortunately, grant maps are *also* generally expected to be owned by
> foreign domains; so this function will fail for any p2m entry containing a
> grant map that doesn't happen to be local.
> 
> Have grant maps take the same path as foreign entries.  Since grants may
> actually be either foreign or local, adjust the assertion to allow for this.
> ---

Sure, thanks, I can use this, but then I'd perhaps ought to add your
S-o-b instead of ...

> One more comment...
> 
> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Using | instead of || helps the compiler fold the two p2m_is_*().
>> ---
>> v2: The shared case was fine; limit to grant adjustment.
>>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -357,11 +357,11 @@ struct page_info *p2m_get_page_from_gfn(
>>               && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
>>          {
>>              page = mfn_to_page(mfn);
>> -            if ( unlikely(p2m_is_foreign(*t)) )
>> +            if ( unlikely(p2m_is_foreign(*t) | p2m_is_grant(*t)) )
>>
> 
> I'm not a fan of this.  If you replace it with || you can have my R-b

... R-b, requiring yet someone else's ack?

> immediately; otherwise we'll have to wait until we can discuss our general
> policy on this sort of thing at the x86 maintainer's call.

I prefer to wait. Considering that even leaving aside the use of
p2m_is_...() "if ( a || b )" is equivalent to "if ( a | b )" (with a and
b of suitable types, of course), and typically requiring less branches
(on x86 at least; architectures with predicated insns of course are
different), personally I'd see us make more use of this in general.
(Hence also the post-commit-message remark.) But yes, Misra in principle
doesn't like such (we've already deviated the underlying pattern,
though).

Of course the compiler is generally in a position to do such a
transformation itself. Just that in at least this specific case I did
observe it not to. I didn't check simpler cases any time halfway
recently.

Jan

Re: [PATCH v2 1/2] x86/p2m: make p2m_get_page_from_gfn() handle grant case correctly

[George Dunlap]

On Thu, Feb 8, 2024 at 4:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.02.2024 07:32, George Dunlap wrote:
> > On Thu, Jun 23, 2022 at 7:54 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> >> Grant P2M entries, which are covered by p2m_is_any_ram(), wouldn't pass
> >> the get_page() unless the grant was a local one. These need to take the
> >> same path as foreign entries. Just the assertion there is not valid for
> >> local grants, and hence it triggering needs to be avoided.
> >>
> >
> > I think I'd say:
> >
> > ---
> > The 'fast' path of p2m_get_page_from_gfn handles three cases: normal ram,
> > foreign p2m entries, and grant map entries.  For normal ram and grant table
> > entries, get_page() is called, but for foreign entries,
> > page_get_owner_and_reference() is called, since the current domain is
> > expected not to be the owner.
> >
> > Unfortunately, grant maps are *also* generally expected to be owned by
> > foreign domains; so this function will fail for any p2m entry containing a
> > grant map that doesn't happen to be local.
> >
> > Have grant maps take the same path as foreign entries.  Since grants may
> > actually be either foreign or local, adjust the assertion to allow for this.
> > ---
>
> Sure, thanks, I can use this, but then I'd perhaps ought to add your
> S-o-b instead of ...
<snip>
> ... R-b, requiring yet someone else's ack?

Legally I think the SoB is more for the provenance of the code than
the commit messages; so it would mainly be to credit me, which I'm not
particularly fussed by.

That said, we did just put something in MAINTAINERS about how to deal
with this situation; You sending the patch implicitly approves all the
changes I made, so then if I give an R-b, that approves all the
changes you made, satisfying the requirements.

 -George

Re: [PATCH v2 2/2] x86/p2m: aid the compiler in folding p2m_is_...()

[Jan Beulich]

On 07.02.2024 04:07, George Dunlap wrote:
> On Thu, Feb 1, 2024 at 10:15 PM Jan Beulich <jbeulich@suse.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -379,7 +379,7 @@ struct page_info *p2m_get_page_from_gfn(
>              return page;
> =20
>          /* Error path: not a suitable GFN at all */
> -        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
> +        if ( !(p2m_is_ram(*t) | p2m_is_paging(*t) | p2m_is_pod(*t)) &&
>               !mem_sharing_is_fork(p2m->domain) )
>              return NULL;
>      }
> ```
> 
> Note the "=20" at the beginning of the empty line.  Why `patch` handles it
> but `git am` doesn't, who knows.
> 
> 
>> I'm also not aware of there
>> being a requirement that patches I send via email need to be
>> "git am"-able (unlike in xsa.git, where I edit patches enough to be
>> suitable for that), nor am I aware how I would convince my email
>> client and/or server to omit whatever git doesn't like or to add
>> whatever git is missing.
>>
>> Bottom line - your response would be actionable by me only in so far
>> as I could switch to using "git send-email". Which I'm afraid I'm not
>> going to do unless left with no other choice. The way I've been
>> sending patches has worked well for over 20 years, and for different
>> projects. (I'm aware Andrew has some special "Jan" command to apply
>> patches I send, but I don't know any specifics.)
>>
> 
> In the general case, I'm not going to review a patch without being able to
> see it in context; and it's not reasonable to expect reviewers to have
> specific contributor-specific scripts for doing so.  If we run into this
> issue in the future, and you want my review, you may have to post a git
> tree somewhere, or attach the patch as an attachment or something.  (Or you
> can try to figure out why `git am` isn't working and try to upstream a fix.)
> 
> That said, in this case, context isn't really necessary to understand the
> change, so it won't be necessary.
> 
> The logic of the change is obviously correct; but it definitely reduces the
> readability.  I kind of feel like whether this sort of optimization is
> worth the benefits is more a general x86 maintainer policy decision.  Maybe
> we can talk about it at the next maintainer's meeting I'll be at?

While you weren't able to be there, I brought this up nevertheless, and
both Andrew and Roger agreed with you. Therefore I'll drop this patch
and adjust the other one.

Jan