[3/6] x86/p2m: set_{foreign,mmio}_p2m_entry() are HVM-only

Commit Message

Jan Beulich Dec. 15, 2020, 4:26 p.m. UTC

Extend a respective #ifdef from inside set_typed_p2m_entry() to around
all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety
check path.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Dec. 17, 2020, 7:54 p.m. UTC | #1

On 15/12/2020 16:26, Jan Beulich wrote:
> Extend a respective #ifdef from inside set_typed_p2m_entry() to around
> all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety
> check path.
>
> Signed-off-by: Jan Beulich <[e-mail]>

As the code currently stands, yes.  However, I'm not sure I agree
conceptually.

The p2m APIs are either a common interface to use, or HVM-specific.

PV guests don't actually have a p2m, but some of the APIs are used from
common code (e.g. copy_to/from_guest()), and some p2m concepts are
special cased as identity for PV (technically paging_mode_translate()),
while other concepts, such as foreign/mmio, which do exist for both PV
and HVM guests, are handled with totally different API sets for PV and HVM.

This is a broken mess of an abstraction.  I suspect some of it has to do
with PV autotranslate mode in the past, but that doesn't alter the fact
that we have a totally undocumented and error prone set of APIs here.

Either P2M's should (fully) be the common abstraction (despite not being
a real object for PV guests), or they should should be a different set
of APIs which is the common abstraction, and P2Ms should move being
exclusively for HVM guests.

(It's also very obvious by all the CONFIG_X86 ifdefary that we've got
arch specifics in our common code, and that is another aspect of the API
mess which needs handling.)

I'm honestly not sure which of these would be better, but I'm fairly
sure that either would be better than what we've currently got.  I
certainly think it would be better to have a plan for improvement, to
guide patches like this.

~Andrew

Jan Beulich Dec. 18, 2020, 8:58 a.m. UTC | #2

On 17.12.2020 20:54, Andrew Cooper wrote:
> On 15/12/2020 16:26, Jan Beulich wrote:
>> Extend a respective #ifdef from inside set_typed_p2m_entry() to around
>> all three functions. Add ASSERT_UNREACHABLE() to the latter one's safety
>> check path.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> As the code currently stands, yes.  However, I'm not sure I agree
> conceptually.
> 
> The p2m APIs are either a common interface to use, or HVM-specific.
> 
> PV guests don't actually have a p2m, but some of the APIs are used from
> common code (e.g. copy_to/from_guest()), and some p2m concepts are
> special cased as identity for PV (technically paging_mode_translate()),
> while other concepts, such as foreign/mmio, which do exist for both PV
> and HVM guests, are handled with totally different API sets for PV and HVM.
> 
> This is a broken mess of an abstraction.  I suspect some of it has to do
> with PV autotranslate mode in the past, but that doesn't alter the fact
> that we have a totally undocumented and error prone set of APIs here.
> 
> Either P2M's should (fully) be the common abstraction (despite not being
> a real object for PV guests), or they should should be a different set
> of APIs which is the common abstraction, and P2Ms should move being
> exclusively for HVM guests.
> 
> (It's also very obvious by all the CONFIG_X86 ifdefary that we've got
> arch specifics in our common code, and that is another aspect of the API
> mess which needs handling.)
> 
> I'm honestly not sure which of these would be better, but I'm fairly
> sure that either would be better than what we've currently got.  I
> certainly think it would be better to have a plan for improvement, to
> guide patches like this.

Well, by the end of this series fairly large parts of p2m.c are inside
#ifdef CONFIG_HVM. I would have thought the route is clear - eventually
p2m.c should get built only when HVM is enabled. This change is simply
getting us one tiny step closer.

Otoh, when considering common code, hiding PV specifics inside the p2m
functions may turn out better, as else we may need another layer around
them (like effectively we already have with e.g.
guest_physmap_{add,remove}_page(), which I think would need to move out
of p2m.c if that was to become HVM-only as a whole) ...

Jan

Patch

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1257,6 +1257,8 @@  int p2m_finish_type_change(struct domain
     return rc;
 }
 
+#ifdef CONFIG_HVM
+
 /*
  * Returns:
  *    0              for success
@@ -1277,7 +1279,10 @@  static int set_typed_p2m_entry(struct do
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     if ( !paging_mode_translate(d) )
+    {
+        ASSERT_UNREACHABLE();
         return -EIO;
+    }
 
     gfn_lock(p2m, gfn, order);
     omfn = p2m->get_entry(p2m, gfn, &ot, &a, 0, &cur_order, NULL);
@@ -1308,7 +1313,6 @@  static int set_typed_p2m_entry(struct do
     if ( rc )
         gdprintk(XENLOG_ERR, "p2m_set_entry: %#lx:%u -> %d (0x%"PRI_mfn")\n",
                  gfn_l, order, rc, mfn_x(mfn));
-#ifdef CONFIG_HVM
     else if ( p2m_is_pod(ot) )
     {
         pod_lock(p2m);
@@ -1316,7 +1320,6 @@  static int set_typed_p2m_entry(struct do
         BUG_ON(p2m->pod.entry_count < 0);
         pod_unlock(p2m);
     }
-#endif
     gfn_unlock(p2m, gfn, order);
 
     return rc;
@@ -1341,6 +1344,8 @@  int set_mmio_p2m_entry(struct domain *d,
                                p2m_get_hostp2m(d)->default_access);
 }
 
+#endif /* CONFIG_HVM */
+
 int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
                            p2m_access_t p2ma, unsigned int flag)
 {