[PATCH] xen: introduce XENFEAT_direct_mapped and XENFEAT_not_direct_mapped

[PATCH] xen: introduce XENFEAT_direct_mapped and XENFEAT_not_direct_mapped

[Stefano Stabellini]

Introduce two feature flags to tell the domain whether it is
direct-mapped or not. It allows the guest kernel to make informed
decisions on things such as swiotlb-xen enablement.

Currently, only Dom0 on ARM is direct-mapped, see:
xen/include/asm-arm/domain.h:is_domain_direct_mapped
xen/include/asm-x86/domain.h:is_domain_direct_mapped

However, given that it is theoretically possible to have direct-mapped
domains on x86 too, the two new feature flags are arch-neutral.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: jbeulich@suse.com
CC: andrew.cooper3@citrix.com
CC: julien@xen.org
---
 xen/common/kernel.c           | 4 ++++
 xen/include/public/features.h | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 7a345ae45e..6ca1377dec 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -560,6 +560,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                              (1U << XENFEAT_hvm_callback_vector) |
                              (has_pirq(d) ? (1U << XENFEAT_hvm_pirqs) : 0);
 #endif
+            if ( is_domain_direct_mapped(d) )
+                fi.submap |= (1U << XENFEAT_direct_mapped);
+            else
+                fi.submap |= (1U << XENFEAT_not_direct_mapped);
             break;
         default:
             return -EINVAL;
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index 1613b2aab8..18e3e61df0 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -114,6 +114,13 @@
  */
 #define XENFEAT_linux_rsdp_unrestricted   15
 
+/*
+ * A direct-mapped (or 1:1 mapped) domain is a domain for which its
+ * local pages have gfn == mfn.
+ */
+#define XENFEAT_not_direct_mapped       16
+#define XENFEAT_direct_mapped           17
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
2.17.1

Re: [PATCH] xen: introduce XENFEAT_direct_mapped and XENFEAT_not_direct_mapped

[Jan Beulich]

On 25.02.2021 02:22, Stefano Stabellini wrote:
> --- a/xen/include/public/features.h
> +++ b/xen/include/public/features.h
> @@ -114,6 +114,13 @@
>   */
>  #define XENFEAT_linux_rsdp_unrestricted   15
>  
> +/*
> + * A direct-mapped (or 1:1 mapped) domain is a domain for which its
> + * local pages have gfn == mfn.
> + */
> +#define XENFEAT_not_direct_mapped       16
> +#define XENFEAT_direct_mapped           17

Why two new values? Absence of XENFEAT_direct_mapped requires
implying not-direct-mapped by the consumer anyway, doesn't it?

Further, quoting xen/mm.h: "For a non-translated guest which
is aware of Xen, gfn == mfn." This to me implies that PV would
need to get XENFEAT_direct_mapped set; not sure whether this
simply means x86'es is_domain_direct_mapped() is wrong, but if
it is, uses elsewhere in the code would likely need changing.

Also, nit: Please keep the right sides aligned with #define-s
higher up in the file.

Jan

Re: [PATCH] xen: introduce XENFEAT_direct_mapped and XENFEAT_not_direct_mapped

[Stefano Stabellini]

On Thu, 25 Feb 2021, Jan Beulich wrote:
> On 25.02.2021 02:22, Stefano Stabellini wrote:
> > --- a/xen/include/public/features.h
> > +++ b/xen/include/public/features.h
> > @@ -114,6 +114,13 @@
> >   */
> >  #define XENFEAT_linux_rsdp_unrestricted   15
> >  
> > +/*
> > + * A direct-mapped (or 1:1 mapped) domain is a domain for which its
> > + * local pages have gfn == mfn.
> > + */
> > +#define XENFEAT_not_direct_mapped       16
> > +#define XENFEAT_direct_mapped           17
> 
> Why two new values? Absence of XENFEAT_direct_mapped requires
> implying not-direct-mapped by the consumer anyway, doesn't it?

That's because if we add both flags we can avoid all unpleasant guessing
games in the guest kernel.

If one flag or the other flag is set, we can make an informed decision.

But if neither flag is set, it means we are running on an older Xen,
and we fall back on the current checks.


> Further, quoting xen/mm.h: "For a non-translated guest which
> is aware of Xen, gfn == mfn." This to me implies that PV would
> need to get XENFEAT_direct_mapped set; not sure whether this
> simply means x86'es is_domain_direct_mapped() is wrong, but if
> it is, uses elsewhere in the code would likely need changing.

That's a good point, I didn't think about x86 PV. I think the two flags
are needed for autotranslated guests. I don't know for sure what is best
for non-autotranslated guests.

Maybe we could say that XENFEAT_not_direct_mapped and
XENFEAT_direct_mapped only apply to XENFEAT_auto_translated_physmap
guests. And it would match the implementation of
is_domain_direct_mapped().

For non XENFEAT_auto_translated_physmap guests we could either do:

- neither flag is set
- set XENFEAT_direct_mapped (without changing the implementation of
  is_domain_direct_mapped)

What do you think? I am happy either way.


> Also, nit: Please keep the right sides aligned with #define-s
> higher up in the file.

OK

Re: [PATCH] xen: introduce XENFEAT_direct_mapped and XENFEAT_not_direct_mapped

[Jan Beulich]

On 25.02.2021 21:51, Stefano Stabellini wrote:
> On Thu, 25 Feb 2021, Jan Beulich wrote:
>> On 25.02.2021 02:22, Stefano Stabellini wrote:
>>> --- a/xen/include/public/features.h
>>> +++ b/xen/include/public/features.h
>>> @@ -114,6 +114,13 @@
>>>   */
>>>  #define XENFEAT_linux_rsdp_unrestricted   15
>>>  
>>> +/*
>>> + * A direct-mapped (or 1:1 mapped) domain is a domain for which its
>>> + * local pages have gfn == mfn.
>>> + */
>>> +#define XENFEAT_not_direct_mapped       16
>>> +#define XENFEAT_direct_mapped           17
>>
>> Why two new values? Absence of XENFEAT_direct_mapped requires
>> implying not-direct-mapped by the consumer anyway, doesn't it?
> 
> That's because if we add both flags we can avoid all unpleasant guessing
> games in the guest kernel.
> 
> If one flag or the other flag is set, we can make an informed decision.
> 
> But if neither flag is set, it means we are running on an older Xen,
> and we fall back on the current checks.

Oh, okay - if there's guesswork to avoid, then I see the point.
Maybe mention in the description?

>> Further, quoting xen/mm.h: "For a non-translated guest which
>> is aware of Xen, gfn == mfn." This to me implies that PV would
>> need to get XENFEAT_direct_mapped set; not sure whether this
>> simply means x86'es is_domain_direct_mapped() is wrong, but if
>> it is, uses elsewhere in the code would likely need changing.
> 
> That's a good point, I didn't think about x86 PV. I think the two flags
> are needed for autotranslated guests. I don't know for sure what is best
> for non-autotranslated guests.
> 
> Maybe we could say that XENFEAT_not_direct_mapped and
> XENFEAT_direct_mapped only apply to XENFEAT_auto_translated_physmap
> guests. And it would match the implementation of
> is_domain_direct_mapped().

I'm having trouble understanding this last sentence, and hence I'm
not sure I understand the rest in the way you may mean it. Neither
x86'es nor Arm's is_domain_direct_mapped() has any check towards a
guest being translated (obviously such a check would be redundant
on Arm).

> For non XENFEAT_auto_translated_physmap guests we could either do:
> 
> - neither flag is set
> - set XENFEAT_direct_mapped (without changing the implementation of
>   is_domain_direct_mapped)
> 
> What do you think? I am happy either way.

I'm happy either way as well; suitably described perhaps setting
XENFEAT_direct_mapped when !paging_mode_translate() would be
slightly more "natural". But a spelled out and enforced
dependency upon XENFEAT_auto_translated_physmap would too be fine
with me.

Jan

Re: [PATCH] xen: introduce XENFEAT_direct_mapped and XENFEAT_not_direct_mapped

[Stefano Stabellini]

On Fri, 26 Feb 2021, Jan Beulich wrote:
> On 25.02.2021 21:51, Stefano Stabellini wrote:
> > On Thu, 25 Feb 2021, Jan Beulich wrote:
> >> On 25.02.2021 02:22, Stefano Stabellini wrote:
> >>> --- a/xen/include/public/features.h
> >>> +++ b/xen/include/public/features.h
> >>> @@ -114,6 +114,13 @@
> >>>   */
> >>>  #define XENFEAT_linux_rsdp_unrestricted   15
> >>>  
> >>> +/*
> >>> + * A direct-mapped (or 1:1 mapped) domain is a domain for which its
> >>> + * local pages have gfn == mfn.
> >>> + */
> >>> +#define XENFEAT_not_direct_mapped       16
> >>> +#define XENFEAT_direct_mapped           17
> >>
> >> Why two new values? Absence of XENFEAT_direct_mapped requires
> >> implying not-direct-mapped by the consumer anyway, doesn't it?
> > 
> > That's because if we add both flags we can avoid all unpleasant guessing
> > games in the guest kernel.
> > 
> > If one flag or the other flag is set, we can make an informed decision.
> > 
> > But if neither flag is set, it means we are running on an older Xen,
> > and we fall back on the current checks.
> 
> Oh, okay - if there's guesswork to avoid, then I see the point.
> Maybe mention in the description?

Yes, I can do that.


> >> Further, quoting xen/mm.h: "For a non-translated guest which
> >> is aware of Xen, gfn == mfn." This to me implies that PV would
> >> need to get XENFEAT_direct_mapped set; not sure whether this
> >> simply means x86'es is_domain_direct_mapped() is wrong, but if
> >> it is, uses elsewhere in the code would likely need changing.
> > 
> > That's a good point, I didn't think about x86 PV. I think the two flags
> > are needed for autotranslated guests. I don't know for sure what is best
> > for non-autotranslated guests.
> > 
> > Maybe we could say that XENFEAT_not_direct_mapped and
> > XENFEAT_direct_mapped only apply to XENFEAT_auto_translated_physmap
> > guests. And it would match the implementation of
> > is_domain_direct_mapped().
> 
> I'm having trouble understanding this last sentence, and hence I'm
> not sure I understand the rest in the way you may mean it. Neither
> x86'es nor Arm's is_domain_direct_mapped() has any check towards a
> guest being translated (obviously such a check would be redundant
> on Arm).

I meant that we are not explicitely checking for auto_translated in
neither version of is_domain_direct_mapped(), but it is sort of implied.


> > For non XENFEAT_auto_translated_physmap guests we could either do:
> > 
> > - neither flag is set
> > - set XENFEAT_direct_mapped (without changing the implementation of
> >   is_domain_direct_mapped)
> > 
> > What do you think? I am happy either way.
> 
> I'm happy either way as well; suitably described perhaps setting
> XENFEAT_direct_mapped when !paging_mode_translate() would be
> slightly more "natural". But a spelled out and enforced
> dependency upon XENFEAT_auto_translated_physmap would too be fine
> with me.

OK. I'll go with:

            if ( is_domain_direct_mapped(d) || !paging_mode_translate(d) )
                fi.submap |= (1U << XENFEAT_direct_mapped);
            else
                fi.submap |= (1U << XENFEAT_not_direct_mapped);


With an appropriate explanation.