viridian cleanup

-----Original Message-----
From: Paul Durrant [mailto:[email protected]]
Sent: 29 October 2018 18:02
To: [email protected]
Cc: Paul Durrant <[email protected]>; Jan Beulich <[email protected]>; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>
Subject: [PATCH 1/2] x86/mm/p2m: don't needlessly limit MMIO mapping order to 4k

The P2M common code currently restricts the MMIO mapping order of any domain with IOMMU mappings, but that is not using shared tables, to 4k. This has been shown to have a huge performance cost when passing through a PCI device with a very large BAR (e.g. NVIDIA P40), increasing the guest boot time from ~20s to several minutes when iommu=no-sharept is specified on the Xen command line.

The limitation was added by commit c3c756bd "x86/p2m: use large pages for MMIO mappings" however the underlying implementations of p2m-

Quoted 1 line

set_entry

for Intel and AMD are coded to cope with mapping orders higher than 4k, even though the IOMMU mapping function is itself currently limited to 4k, so there is no real need to limit the order passed into the method, other than to avoid a potential DoS caused by a long running hypercall.

In practice, mmio_order() already strictly disallows 1G mappings since the if clause in question starts with:

if ( 0 /*
* Don't use 1Gb pages, to limit the iteration count in
* set_typed_p2m_entry() when it needs to zap M2P entries
* for a RAM range.
*/ &&

With this patch applied (and hence 2M mappings in use) the VM boot time is restored to something reasonable. Not as fast as without iommu=no-sharept, but within a few seconds of it.

NOTE: This patch takes the opportunity to shorten a couple of > 80
character lines in the code.

Signed-off-by Paul Durrant <[email protected]>

Acked-by George Dunlap <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

v2:
- Add an extra to the if clause disallowing 1G mappings to make sure they are not used if need_iommu_pt_sync() is true, even though the check is currently moot (see main comment).

---
 xen/arch/x86/mm/p2m.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

xen/arch/x86/mm/p2m.c +4 -7

index a00a3c1bff..f972b4819d 100644
@@ -2081,14 +2081,11 @@ static unsigned int mmio_order(const struct domain
 *d,
                                unsigned long start_fn, unsigned long nr)
 {
     /*
-     * Note that the !iommu_use_hap_pt() here has three effects:
-     * - cover iommu_{,un}map_page() not having an "order" input yet,
-     * - exclude shadow mode (which doesn't support large MMIO mappings),
-     * - exclude PV guests, should execution reach this code for such.
-     * So be careful when altering this.
+     * PV guests or shadow-mode HVM guests must be restricted to 4k
+     * mappings.
      */
-    if ( !iommu_use_hap_pt(d) ||
-         (start_fn & ((1UL << PAGE_ORDER_2M) - 1)) || !(nr >>
 PAGE_ORDER_2M) )
+    if ( !hap_enabled(d) || (start_fn & ((1UL << PAGE_ORDER_2M) - 1)) ||
+         !(nr >> PAGE_ORDER_2M) )
         return PAGE_ORDER_4K;

     if ( 0 /*

xen/arch/x86/mm/p2m.c +0 -1

@@ -2096,8 +2093,12 @@ static unsigned int mmio_order(const struct domain
 *d,
             * set_typed_p2m_entry() when it needs to zap M2P entries
             * for a RAM range.
             */ &&
-         !(start_fn & ((1UL << PAGE_ORDER_1G) - 1)) && (nr >>

PAGE_ORDER_1G) &&

xen/arch/x86/mm/p2m.c +6 -2

-         hap_has_1gb )
+         !(start_fn & ((1UL << PAGE_ORDER_1G) - 1)) &&
+         (nr >> PAGE_ORDER_1G) &&
+         hap_has_1gb &&
+         /* disable 1G mappings if we need to keep the IOMMU in sync */
+         !need_iommu_pt_sync(d)
+        )
         return PAGE_ORDER_1G;
 
     if ( hap_has_2mb )
--

2.11.0

Quoted 1 line

Quoted 1 line · On 29.10.18 at 19:02, <[email protected]> wrote:

Subsequent patches will introduce support for more viridian enlightenments which will make a single source module quite lengthy.

This patch therefore creates a new arch/x86/hvm/viridian sub-directory and moves viridian.c into that.

The patch also fixes some bad whitespace whilst moving the code.

Signed-off-by Paul Durrant <[email protected]>

Quoted 1 line

Quoted 1 line · On 29.10.18 at 19:02, <[email protected]> wrote:

Subsequent patches will introduce support for more viridian enlightenments which will make a single source module quite lengthy.

This patch therefore creates a new arch/x86/hvm/viridian sub-directory and moves viridian.c into that.

The patch also fixes some bad whitespace whilst moving the code.

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/Makefile                  | 2 +-
 xen/arch/x86/hvm/viridian/Makefile         | 1 +
 xen/arch/x86/hvm/{ => viridian}/viridian.c | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/Makefile
 rename xen/arch/x86/hvm/{ => viridian}/viridian.c (99%)

-----Original Message-----
From: Jan Beulich [mailto:[email protected]]
Sent: 31 October 2018 10:18
To: Paul Durrant <[email protected]>
Cc: Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; xen-devel <[email protected]>
Subject: Re: [PATCH 1/8] viridian: move the code into its own sub- directory

Quoted 2 lines

Quoted 1 line

Quoted 1 line · On 29.10.18 at 19:02, <[email protected]> wrote:

Subsequent patches will introduce support for more viridian

enlightenments

Quoted 3 lines

which will make a single source module quite lengthy.

This patch therefore creates a new arch/x86/hvm/viridian sub-directory

and

Quoted 9 lines

moves viridian.c into that.

The patch also fixes some bad whitespace whilst moving the code.

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

Quoted 7 lines

---
 xen/arch/x86/hvm/Makefile                  | 2 +-
 xen/arch/x86/hvm/viridian/Makefile         | 1 +
 xen/arch/x86/hvm/{ => viridian}/viridian.c | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/Makefile
 rename xen/arch/x86/hvm/{ => viridian}/viridian.c (99%)

Actually, I now realize that my ack is dependent on you also adjusting ./MAINTAINERS here, or else you will cease to be maintainer of this code.

Jan

-----Original Message-----
From: Paul Durrant [mailto:[email protected]]
Sent: 29 October 2018 18:02
To: [email protected]
Cc: Paul Durrant <[email protected]>; Jan Beulich <[email protected]>; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; George Dunlap <[email protected]>; Ian Jackson <[email protected]>; Julien Grall <[email protected]>; Konrad Rzeszutek Wilk <[email protected]>; Stefano Stabellini <[email protected]>; Tim (Xen.org) <[email protected]>; Jun Nakajima <[email protected]>; Kevin Tian <[email protected]>
Subject: [PATCH 2/2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()

The P2M code currently contains many loops to deal with the fact that, while it may be require to handle page orders greater than 4k, the IOMMU map and unmap functions do not.
This patch adds a page_order parameter to those functions and implements the necessary loops within. This allows the P2M code to be sunstantially simplified.

NOTE: This patch does not modify the underlying vendor IOMMU
implementations to deal with page orders of more than 4k.

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: George Dunlap <[email protected]>
Cc: Ian Jackson <[email protected]>
Cc: Julien Grall <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Stefano Stabellini <[email protected]>
Cc: Tim Deegan <[email protected]>
Cc: Jun Nakajima <[email protected]>
Cc: Kevin Tian <[email protected]>

---
 xen/arch/x86/mm.c                   |  4 ++-
 xen/arch/x86/mm/p2m-ept.c           | 30 ++---------------
 xen/arch/x86/mm/p2m-pt.c            | 47 ++++++---------------------
 xen/arch/x86/mm/p2m.c               | 49 ++++------------------------
 xen/arch/x86/x86_64/mm.c            |  4 ++-
 xen/common/grant_table.c            |  7 ++--
 xen/drivers/passthrough/iommu.c     | 65 ++++++++++++++++++++++++--------
-----
 xen/drivers/passthrough/x86/iommu.c |  2 +-
 xen/include/xen/iommu.h             |  8 +++--
 9 files changed, 80 insertions(+), 136 deletions(-)

xen/arch/x86/mm.c +3 -1

index c53bc86a68..f0ae016e7a 100644
@@ -2794,9 +2794,11 @@ static int _get_page_type(struct page_info *page,
 unsigned long type,
             mfn_t mfn = page_to_mfn(page);
 
             if ( (x & PGT_type_mask) == PGT_writable_page )
-                iommu_ret = iommu_unmap_page(d, _dfn(mfn_x(mfn)));
+                iommu_ret = iommu_unmap_page(d, _dfn(mfn_x(mfn)),
+                                             PAGE_ORDER_4K);
             else if ( type == PGT_writable_page )
                 iommu_ret = iommu_map_page(d, _dfn(mfn_x(mfn)), mfn,
+                                           PAGE_ORDER_4K,
                                            IOMMUF_readable |
                                            IOMMUF_writable);

         }

xen/arch/x86/mm/p2m-ept.c +3 -27

index 407e299e50..656c8dd3ac 100644
@@ -880,33 +880,9 @@ out:
         if ( iommu_use_hap_pt(d) )
             rc = iommu_pte_flush(d, gfn, &ept_entry->epte, order,
 vtd_pte_present);
         else if ( need_iommu_pt_sync(d) )
-        {
-            dfn_t dfn = _dfn(gfn);
-
-            if ( iommu_flags )
-                for ( i = 0; i < (1 << order); i++ )
-                {
-                    rc = iommu_map_page(d, dfn_add(dfn, i),
-                                        mfn_add(mfn, i), iommu_flags);
-                    if ( unlikely(rc) )
-                    {
-                        while ( i-- )
-                            /* If statement to satisfy __must_check. */
-                            if ( iommu_unmap_page(p2m->domain,
-                                                  dfn_add(dfn, i)) )
-                                continue;
-
-                        break;
-                    }
-                }
-            else
-                for ( i = 0; i < (1 << order); i++ )
-                {
-                    ret = iommu_unmap_page(d, dfn_add(dfn, i));
-                    if ( !rc )
-                        rc = ret;
-                }
-        }
+            rc = iommu_flags ?
+                iommu_map_page(d, _dfn(gfn), mfn, order, iommu_flags) :
+                iommu_unmap_page(d, _dfn(gfn), order);
     }
 

     unmap_domain_page(table);

xen/arch/x86/mm/p2m-pt.c +2 -1

index 55df18501e..b264a97bd8 100644
@@ -477,10 +477,11 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_,
 mfn_t mfn,
                  unsigned int page_order, p2m_type_t p2mt, p2m_access_t
 p2ma,
                  int sve)
 {
+    struct domain *d = p2m->domain;
     /* XXX -- this might be able to be faster iff current->domain == d */
     void *table;
     unsigned long gfn = gfn_x(gfn_);
-    unsigned long i, gfn_remainder = gfn;
+    unsigned long gfn_remainder = gfn;
     l1_pgentry_t *p2m_entry, entry_content;

     /* Intermediate table to free if we're replacing it with a superpage.
*/
     l1_pgentry_t intermediate_entry = l1e_empty();

xen/arch/x86/mm/p2m-pt.c +1 -1

@@ -515,7 +516,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_,
 mfn_t mfn,
         t.gfn = gfn;
         t.mfn = mfn_x(mfn);
         t.p2mt = p2mt;
-        t.d = p2m->domain->domain_id;
+        t.d = d->domain_id;
         t.order = page_order;
 

         __trace_var(TRC_MEM_SET_P2M_ENTRY, 0, sizeof(t), &t);

xen/arch/x86/mm/p2m-pt.c +7 -35

@@ -683,41 +684,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_,
 mfn_t mfn,
     {
         ASSERT(rc == 0);
 
-        if ( iommu_use_hap_pt(p2m->domain) )
-        {
-            if ( iommu_old_flags )
-                amd_iommu_flush_pages(p2m->domain, gfn, page_order);
-        }
-        else if ( need_iommu_pt_sync(p2m->domain) )
-        {
-            dfn_t dfn = _dfn(gfn);
-
-            if ( iommu_pte_flags )
-                for ( i = 0; i < (1UL << page_order); i++ )
-                {
-                    rc = iommu_map_page(p2m->domain, dfn_add(dfn, i),
-                                        mfn_add(mfn, i),
 iommu_pte_flags);
-                    if ( unlikely(rc) )
-                    {
-                        while ( i-- )
-                            /* If statement to satisfy __must_check. */
-                            if ( iommu_unmap_page(p2m->domain,
-                                                  dfn_add(dfn, i)) )
-                                continue;
-
-                        break;
-                    }
-                }
-            else
-                for ( i = 0; i < (1UL << page_order); i++ )
-                {
-                    int ret = iommu_unmap_page(p2m->domain,
-                                               dfn_add(dfn, i));
-
-                    if ( !rc )
-                        rc = ret;
-                }
-        }
+        if ( need_iommu_pt_sync(p2m->domain) )
+            rc = iommu_pte_flags ?
+                iommu_map_page(d, _dfn(gfn), mfn, page_order,
+                               iommu_pte_flags) :
+                iommu_unmap_page(d, _dfn(gfn), page_order);
+        else if ( iommu_use_hap_pt(d) && iommu_old_flags )
+            amd_iommu_flush_pages(p2m->domain, gfn, page_order);
     }

     /*

xen/arch/x86/mm/p2m.c +2 -18

index f972b4819d..e5880d646b 100644
@@ -718,24 +718,8 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long
 gfn_l, unsigned long mfn,
     p2m_access_t a;
 
     if ( !paging_mode_translate(p2m->domain) )
-    {
-        int rc = 0;
-
-        if ( need_iommu_pt_sync(p2m->domain) )
-        {
-            dfn_t dfn = _dfn(mfn);
-
-            for ( i = 0; i < (1 << page_order); i++ )
-            {
-                int ret = iommu_unmap_page(p2m->domain, dfn_add(dfn, i));
-
-                if ( !rc )
-                    rc = ret;
-            }
-        }
-
-        return rc;
-    }
+        return need_iommu_pt_sync(p2m->domain) ?
+            iommu_unmap_page(p2m->domain, _dfn(mfn), page_order) : 0;
 
     ASSERT(gfn_locked_by_me(p2m, gfn));

     P2M_DEBUG("removing gfn=%#lx mfn=%#lx\n", gfn_l, mfn);

xen/arch/x86/mm/p2m.c +3 -22

@@ -781,28 +765,9 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn,
 mfn_t mfn,
     int rc = 0;
 
     if ( !paging_mode_translate(d) )
-    {
-        if ( need_iommu_pt_sync(d) && t == p2m_ram_rw )
-        {
-            dfn_t dfn = _dfn(mfn_x(mfn));
-
-            for ( i = 0; i < (1 << page_order); i++ )
-            {
-                rc = iommu_map_page(d, dfn_add(dfn, i), mfn_add(mfn, i),
-                                    IOMMUF_readable|IOMMUF_writable);
-                if ( rc != 0 )
-                {
-                    while ( i-- > 0 )
-                        /* If statement to satisfy __must_check. */
-                        if ( iommu_unmap_page(d, dfn_add(dfn, i)) )
-                            continue;
-
-                    return rc;
-                }
-            }
-        }
-        return 0;
-    }
+        return (need_iommu_pt_sync(d) && t == p2m_ram_rw) ?
+            iommu_map_page(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
+                           IOMMUF_readable | IOMMUF_writable) : 0;
 
     /* foreign pages are added thru p2m_add_foreign */

     if ( p2m_is_foreign(t) )

xen/arch/x86/mm/p2m.c +2 -2

@@ -1173,7 +1138,7 @@ int set_identity_p2m_entry(struct domain *d,
 unsigned long gfn_l,
     {
         if ( !need_iommu_pt_sync(d) )
             return 0;
-        return iommu_map_page(d, _dfn(gfn_l), _mfn(gfn_l),
+        return iommu_map_page(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
                               IOMMUF_readable | IOMMUF_writable);
     }

@@ -1264,7 +1229,7 @@ int clear_identity_p2m_entry(struct domain *d,
 unsigned long gfn_l)
     {
         if ( !need_iommu_pt_sync(d) )
             return 0;
-        return iommu_unmap_page(d, _dfn(gfn_l));
+        return iommu_unmap_page(d, _dfn(gfn_l), PAGE_ORDER_4K);
     }
 

     gfn_lock(p2m, gfn, 0);

xen/arch/x86/x86_64/mm.c +3 -1

index 543ea030e3..c0ce5673ba 100644
@@ -1437,13 +1437,15 @@ int memory_add(unsigned long spfn, unsigned long
 epfn, unsigned int pxm)
     {
         for ( i = spfn; i < epfn; i++ )
             if ( iommu_map_page(hardware_domain, _dfn(i), _mfn(i),
+                                PAGE_ORDER_4K,
                                 IOMMUF_readable | IOMMUF_writable) )
                 break;
         if ( i != epfn )
         {
             while (i-- > old_max)
                 /* If statement to satisfy __must_check. */
-                if ( iommu_unmap_page(hardware_domain, _dfn(i)) )
+                if ( iommu_unmap_page(hardware_domain, _dfn(i),
+                                      PAGE_ORDER_4K) )
                     continue;
 

             goto destroy_m2p;

xen/common/grant_table.c +5 -2

index 878e668bf5..971c6b100c 100644
@@ -1142,12 +1142,14 @@ map_grant_ref(
         {
             if ( !(kind & MAPKIND_WRITE) )
                 err = iommu_map_page(ld, _dfn(mfn_x(mfn)), mfn,
+                                     PAGE_ORDER_4K,
                                      IOMMUF_readable | IOMMUF_writable);
         }
         else if ( act_pin && !old_pin )
         {
             if ( !kind )
                 err = iommu_map_page(ld, _dfn(mfn_x(mfn)), mfn,
+                                     PAGE_ORDER_4K,
                                      IOMMUF_readable);
         }
         if ( err )

@@ -1396,10 +1398,11 @@ unmap_common(
 
         kind = mapkind(lgt, rd, op->mfn);
         if ( !kind )
-            err = iommu_unmap_page(ld, _dfn(mfn_x(op->mfn)));
+            err = iommu_unmap_page(ld, _dfn(mfn_x(op->mfn)),
+                                   PAGE_ORDER_4K);
         else if ( !(kind & MAPKIND_WRITE) )
             err = iommu_map_page(ld, _dfn(mfn_x(op->mfn)), op->mfn,
-                                 IOMMUF_readable);
+                                 PAGE_ORDER_4K, IOMMUF_readable);
 
         double_gt_unlock(lgt, rgt);
 

xen/drivers/passthrough/iommu.c +43 -22

index 8b438ae4bc..40db9e7849 100644
@@ -305,50 +305,71 @@ void iommu_domain_destroy(struct domain *d)
 }
 
 int iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
-                   unsigned int flags)
+                   unsigned int page_order, unsigned int flags)
 {
     const struct domain_iommu *hd = dom_iommu(d);
-    int rc;
+    unsigned long i;
 
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    rc = hd->platform_ops->map_page(d, dfn, mfn, flags);
-    if ( unlikely(rc) )
+    for ( i = 0; i < (1ul << page_order); i++ )
     {
-        if ( !d->is_shutting_down && printk_ratelimit() )
-            printk(XENLOG_ERR
-                   "d%d: IOMMU mapping dfn %"PRI_dfn" to mfn %"PRI_mfn"
 failed: %d\n",
-                   d->domain_id, dfn_x(dfn), mfn_x(mfn), rc);
+        int ignored, rc = hd->platform_ops->map_page(d, dfn_add(dfn, i),
+                                                     mfn_add(mfn, i),
+                                                     flags);
 
-        if ( !is_hardware_domain(d) )
-            domain_crash(d);
+        if ( unlikely(rc) )
+        {
+            while (i--)
+            {
+                /* assign to something to avoid compiler warning */
+                ignored = hd->platform_ops->unmap_page(d, dfn_add(dfn,
 i));
+            }
+
+            if ( !d->is_shutting_down && printk_ratelimit() )
+                printk(XENLOG_ERR
+                       "d%d: IOMMU order %u mapping dfn %"PRI_dfn" to mfn
 %"PRI_mfn" failed: %d\n",
+                       d->domain_id, page_order, dfn_x(dfn), mfn_x(mfn),
+                       rc);
+
+            if ( !is_hardware_domain(d) )
+                domain_crash(d);
+
+            return rc;
+        }
     }
 
-    return rc;
+    return 0;
 }
 
-int iommu_unmap_page(struct domain *d, dfn_t dfn)
+int iommu_unmap_page(struct domain *d, dfn_t dfn, unsigned int
 page_order)
 {
     const struct domain_iommu *hd = dom_iommu(d);
-    int rc;
+    unsigned long i;
 
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    rc = hd->platform_ops->unmap_page(d, dfn);
-    if ( unlikely(rc) )
+    for ( i = 0; i < (1ul << page_order); i++ )
     {
-        if ( !d->is_shutting_down && printk_ratelimit() )
-            printk(XENLOG_ERR
-                   "d%d: IOMMU unmapping dfn %"PRI_dfn" failed: %d\n",
-                   d->domain_id, dfn_x(dfn), rc);
+        int rc = hd->platform_ops->unmap_page(d, dfn_add(dfn, i));
 
-        if ( !is_hardware_domain(d) )
-            domain_crash(d);
+        if ( unlikely(rc) )
+        {
+            if ( !d->is_shutting_down && printk_ratelimit() )
+                printk(XENLOG_ERR
+                       "d%d: IOMMU unmapping dfn %"PRI_dfn" failed:
 %d\n",
+                       d->domain_id, dfn_x(dfn), rc);
+
+            if ( !is_hardware_domain(d) )
+                domain_crash(d);
+
+            return rc;
+        }
     }
 
-    return rc;
+    return 0;
 }
 
 int iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,

xen/drivers/passthrough/x86/iommu.c +1 -1

index b20bad17de..4fd3eb1dca 100644
@@ -239,7 +239,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain
 *d)
         if ( paging_mode_translate(d) )
             rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
         else
-            rc = iommu_map_page(d, _dfn(pfn), _mfn(pfn),
+            rc = iommu_map_page(d, _dfn(pfn), _mfn(pfn), PAGE_ORDER_4K,
                                 IOMMUF_readable | IOMMUF_writable);
         if ( rc )

printk(XENLOG_WARNING " d%d: IOMMU mapping failed: %d\n",

xen/include/xen/iommu.h +5 -4

index c75333c077..a003d82204 100644
@@ -89,9 +89,11 @@ void iommu_teardown(struct domain *d);
 #define IOMMUF_readable  (1u<<_IOMMUF_readable)
 #define _IOMMUF_writable 1
 #define IOMMUF_writable  (1u<<_IOMMUF_writable)
-int __must_check iommu_map_page(struct domain *d, dfn_t dfn,
-                                mfn_t mfn, unsigned int flags);
-int __must_check iommu_unmap_page(struct domain *d, dfn_t dfn);
+int __must_check iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
+                                unsigned int page_order,
+                                unsigned int flags);
+int __must_check iommu_unmap_page(struct domain *d, dfn_t dfn,
+                                  unsigned int page_order);
 int __must_check iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t
 *mfn,
                                    unsigned int *flags);
 
--

2.11.0

They're not really useful so maintaining them is pointless.

Signed-off-by Paul Durrant <[email protected]>

Microsoft has a habit of re-numbering sections in the spec. so avoid referring to section numbers in comments. Also remove the URL for the spec. from the boilerplate... Again, Microsoft has a habit of changing these too.

This patch also cleans up some > 80 character lines.

Purely cosmetic. No functional change.

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/viridian.c | 82 ++++++++++++++++--------------------
 xen/include/asm-x86/hvm/viridian.h   |  4 --
 2 files changed, 36 insertions(+), 50 deletions(-)

xen/arch/x86/hvm/viridian/viridian.c +2 -6

index c5722d6992..db166d41c5 100644
@@ -1,12 +1,8 @@
-/******************************************************************************
+/**************************************************************************
  * viridian.c
  *
  * An implementation of some Viridian enlightenments. See Microsoft's
- * Hypervisor Top Level Functional Specification (v5.0a) at:
- *
- * https://github.com/Microsoft/Virtualization-Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0.pdf
- *
- * for more information.
+ * Hypervisor Top Level Functional Specification for more information.
  */
 
 #include <xen/sched.h>

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 16:34
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 3/8] viridian: remove comments referencing section number in the spec.

Quoted 5 lines · Paul Durrant

Microsoft has a habit of re-numbering sections in the spec. so avoid referring to section numbers in comments. Also remove the URL for the spec. from the boilerplate... Again, Microsoft has a habit of changing these too.

Quoted 5 lines

This patch also cleans up some > 80 character lines.

Purely cosmetic. No functional change.

Signed-off-by Paul Durrant <[email protected]>

Reviewed-by Roger Pau Monné <[email protected]>

Just one nit below.

Quoted 6 lines

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/viridian.c | 82 ++++++++++++++++-------------

-------

Review

Quoted 4 lines

 xen/include/asm-x86/hvm/viridian.h   |  4 --
 2 files changed, 36 insertions(+), 50 deletions(-)

(quoted diff) +0 -0

b/xen/arch/x86/hvm/viridian/viridian.c

xen/arch/x86/hvm/viridian/viridian.c line 1

Quoted 5 lines

index c5722d6992..db166d41c5 100644

xen/arch/x86/hvm/viridian/viridian.c +0 -1

@@ -1,12 +1,8 @@
-

/*************************************************************************
*****

Quoted 1 line

+/************************************************************************ **

Quoted 6 lines

* viridian.c
*
* An implementation of some Viridian enlightenments. See Microsoft's
- * Hypervisor Top Level Functional Specification (v5.0a) at:
- *
- * https://github.com/Microsoft/Virtualization-

Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional%20Spec ification%20v5.0.pdf

Quoted 9 lines

- *
- * for more information.
+ * Hypervisor Top Level Functional Specification for more information. */

#include <xen/sched.h>

xen/arch/x86/hvm/viridian/viridian.c +0 -0

@@ -103,10 +99,7 @@
 #define HV_FLUSH_ALL_PROCESSORS 1
 

xen/arch/x86/hvm/viridian/viridian.c line 103

Quoted 7 lines

xen/arch/x86/hvm/viridian/viridian.c +1 -4

 /*
- * Viridian Partition Privilege Flags.
- *
- * This is taken from section 4.2.2 of the specification, and fixed for
- * style and correctness.
+ * Viridian Partition Privilege Flags
  */

This being a single line comment now should be /* ... */.

Thanks, Roger.

The 'viridian_vp_assist', 'viridian_hypercall_gpa' and 'viridian_reference_tsc' union types are identical in layout. The layout is also common throughout the specification [1].

This patch declares a common 'viridian_page_msr' type and converts the rest of the code to use that type for both the hypercall and VP assist pages.

Also, rename 'viridian_guest_os_id' to 'viridian_guest_os_id_msr' since it also is a union representing an MSR value.

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 16:40
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 4/8] viridian: remove duplicate union types

Quoted 5 lines · Paul Durrant

The 'viridian_vp_assist', 'viridian_hypercall_gpa' and 'viridian_reference_tsc' union types are identical in layout. The layout is also common throughout the specification [1].

This patch declares a common 'viridian_page_msr' type and converts the

rest

Quoted 1 line

of the code to use that type for both the hypercall and VP assist pages.

I agree with the unification of the viridian_page_msr struct.

Quoted 1 line

Also, rename 'viridian_guest_os_id' to 'viridian_guest_os_id_msr' since

it

Quoted 1 line

also is a union representing an MSR value.

                               ^ a

IMO (and I'm not that familiar with the code) adding those _msr prefixes here doesn't seem to add any value to the code, there's nothing MSR specific in those structs, apart from their size.

Is there a plan to add a new structure like viridian_page_mmio or similar that requires such prefixes to be present?

Thanks, Roger.

Quoted 21 lines

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 16:40
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 4/8] viridian: remove duplicate union types

Quoted 5 lines · Paul Durrant

The 'viridian_vp_assist', 'viridian_hypercall_gpa' and 'viridian_reference_tsc' union types are identical in layout. The layout is also common throughout the specification [1].

This patch declares a common 'viridian_page_msr' type and converts the

rest

Quoted 1 line

of the code to use that type for both the hypercall and VP assist pages.

I agree with the unification of the viridian_page_msr struct.

Quoted 9 lines

Quoted 1 line

Also, rename 'viridian_guest_os_id' to 'viridian_guest_os_id_msr' since

it

Quoted 1 line

also is a union representing an MSR value.

                               ^ a

IMO (and I'm not that familiar with the code) adding those _msr prefixes here doesn't seem to add any value to the code, there's nothing MSR specific in those structs, apart from their size.

There is actually... they are unions that represent the layout of an MSR, e.g See section 2.6 "Reporting the Guest OS Identity".

Quoted 3 lines

Is there a plan to add a new structure like viridian_page_mmio or similar that requires such prefixes to be present?

No, I'm not anticipating Windows communicating via an MMIO region, but I's still like the call out something that represents a bit-field encoding of an MSR.

...into new 'synic' module.

The SynIC (synthetic interrupt controller) is specified [1] to be a super- set of a virtualized LAPIC, and its definition encompasses all enlightenments related to virtual interrupt control.

This patch reduces the size of the main viridian source module by giving these enlightenments their own module. This is done in anticipation of implementation of more such enlightenments and a desire not to further lengthen then main source module when this work is done.

Whilst moving the code:

- Fix various style issues.
- Move the MSR definitions into the header (since they are now needed in more than one source module).

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/Makefile   |   1 +
 xen/arch/x86/hvm/viridian/synic.c    | 224 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/viridian/viridian.c | 229 ++---------------------------------
 xen/include/asm-x86/hvm/viridian.h   |  76 ++++++++++++
 4 files changed, 311 insertions(+), 219 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/synic.c

xen/arch/x86/hvm/viridian/Makefile +1 -0

index 09fd0a5f3c..fca8e16e20 100644
@@ -1 +1,2 @@
+obj-y += synic.o
 obj-y += viridian.o

xen/arch/x86/hvm/viridian/synic.c new file +2 -0

new file mode 100644
index 0000000000..3f043f34ee
@@ -0,0 +1,224 @@
+/***************************************************************************
+ * synic.c

xen/arch/x86/hvm/viridian/synic.c new file +3 -4

@@ -989,14 +786,13 @@ HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_DOMAIN, viridian_save_domain_ctxt,
 
 static int viridian_save_vcpu_ctxt(struct vcpu *v, hvm_domain_context_t *h)
 {
-    struct hvm_viridian_vcpu_context ctxt = {
-        .vp_assist_msr = v->arch.hvm.viridian.vp_assist.msr.raw,
-        .vp_assist_pending = v->arch.hvm.viridian.vp_assist.pending,
-    };
+    struct hvm_viridian_vcpu_context ctxt = {};
 
     if ( !is_viridian_domain(v->domain) )
         return 0;
 
+    viridian_synic_save_vcpu_ctxt(v, &ctxt);
+
     return hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt);
 }
 

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 16:52
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 5/8] viridian: separate interrupt related enlightenment implementations...

Quoted 3 lines · Paul Durrant

...into new 'synic' module.

The SynIC (synthetic interrupt controller) is specified [1] to be a

super-

Quoted 8 lines

set of a virtualized LAPIC, and its definition encompasses all enlightenments related to virtual interrupt control.

This patch reduces the size of the main viridian source module by giving these enlightenments their own module. This is done in anticipation of implementation of more such enlightenments and a desire not to further lengthen then main source module when this work is done.

Quoted 7 lines

Whilst moving the code:

- Fix various style issues.
- Move the MSR definitions into the header (since they are now needed in more than one source module).

[1] https://github.com/MicrosoftDocs/Virtualization-

Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specif ication%20v5.0C.pdf

Quoted 9 lines

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/Makefile   |   1 +
 xen/arch/x86/hvm/viridian/synic.c    | 224

++++++++++++++++++++++++++++++++++

Quoted 1 line

 xen/arch/x86/hvm/viridian/viridian.c | 229 ++--------------------------

-------

Review

Quoted 5 lines

 xen/include/asm-x86/hvm/viridian.h   |  76 ++++++++++++
 4 files changed, 311 insertions(+), 219 deletions(-)
 create mode 100644 xen/arch/x86/hvm/viridian/synic.c

(quoted diff) +0 -0

b/xen/arch/x86/hvm/viridian/Makefile

Review

Quoted 7 lines

index 09fd0a5f3c..fca8e16e20 100644

xen/arch/x86/hvm/viridian/Makefile +1 -0

@@ -1 +1,2 @@
+obj-y += synic.o
 obj-y += viridian.o

(quoted diff) +0 -0

b/xen/arch/x86/hvm/viridian/synic.c

xen/arch/x86/hvm/viridian/synic.c

Quoted 6 lines

new file mode 100644
index 0000000000..3f043f34ee

xen/arch/x86/hvm/viridian/synic.c new file +0 -0

@@ -0,0 +1,224 @@

+/************************************************************************
***

xen/arch/x86/hvm/viridian/synic.c line 12

Quoted 11 lines

xen/arch/x86/hvm/viridian/synic.c new file +11 -0

+ * synic.c
+ *
+ * An implementation of some interrupt related Viridian enlightenments.
+ * See Microsoft's Hypervisor Top Level Functional Specification.
+ * for more information.
+ */
+
+#include <xen/sched.h>
+#include <xen/version.h>
+#include <xen/hypercall.h>
+#include <xen/domain_page.h>

Could you sort the above alphabetically?

And I usually add a newline between xen/ and asm/ includes, but I don't think that's coding style.

xen/arch/x86/hvm/viridian/synic.c HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_DOMAIN,

Quoted 1 line

xen/arch/x86/hvm/viridian/synic.c new file +0 -0

@@ -989,14 +786,13 @@ HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_DOMAIN,

viridian_save_domain_ctxt,

xen/arch/x86/hvm/viridian/synic.c HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_DOMAIN, · line 786

Quoted 2 lines

xen/arch/x86/hvm/viridian/synic.c new file +0 -0

 static int viridian_save_vcpu_ctxt(struct vcpu *v, hvm_domain_context_t

*h)

xen/arch/x86/hvm/viridian/synic.c static int viridian_load_vcpu_ctxt(struct domain

Quoted 16 lines

xen/arch/x86/hvm/viridian/synic.c new file +3 -4

 {
-    struct hvm_viridian_vcpu_context ctxt = {
-        .vp_assist_msr = v->arch.hvm.viridian.vp_assist.msr.raw,
-        .vp_assist_pending = v->arch.hvm.viridian.vp_assist.pending,
-    };
+    struct hvm_viridian_vcpu_context ctxt = {};
 
     if ( !is_viridian_domain(v->domain) )
         return 0;
 
+    viridian_synic_save_vcpu_ctxt(v, &ctxt);
+
     return hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt);
 }
 

@@ -1020,12 +816,7 @@ static int viridian_load_vcpu_ctxt(struct domain

*d,

xen/arch/x86/hvm/viridian/synic.c static int viridian_load_vcpu_ctxt(struct domain · line 819

Quoted 10 lines

xen/arch/x86/hvm/viridian/synic.c new file +1 -6

     if ( memcmp(&ctxt._pad, zero_page, sizeof(ctxt._pad)) )
         return -EINVAL;
 
-    v->arch.hvm.viridian.vp_assist.msr.raw = ctxt.vp_assist_msr;
-    if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled &&
-         !v->arch.hvm.viridian.vp_assist.va )
-        initialize_vp_assist(v);
-
-    v->arch.hvm.viridian.vp_assist.pending = !!ctxt.vp_assist_pending;
+    viridian_synic_load_vcpu_ctxt(v, &ctxt);

Seeing that now viridian_{load/save}_vcpu_ctxt is mostly a wrapper around viridian_synic_{save/load}_vcpu_ctxt, is there any reason to not remove those and declare the save/restore handling of the vidirian vcpu in the new synic file?

Is there more stuff not synic related coming up that needs save/restore?

Thanks, Roger.

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 17:03
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 6/8] viridian: separate time related enlightenment implementations...

xen/include/asm-x86/hvm/viridian.h viridian_hypercall(struct cpu_user_regs *regs); · line 149

Quoted 6 lines · Paul Durrant

xen/include/asm-x86/hvm/viridian.h +2 -0

@@ -154,6 +145,14 @@ viridian_hypercall(struct cpu_user_regs *regs);
 void viridian_time_ref_count_freeze(struct domain *d);
 void viridian_time_ref_count_thaw(struct domain *d);
 
+int viridian_time_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val);
+int viridian_time_rdmsr(const struct vcpu *v, uint32_t idx, uint64_t

*val);

xen/include/asm-x86/hvm/viridian.h viridian_hypercall(struct cpu_user_regs *regs); · line 154

Quoted 5 lines

xen/include/asm-x86/hvm/viridian.h +5 -0

+
+void viridian_time_save_domain_ctxt(
+    const struct domain *d, struct hvm_viridian_domain_context *ctxt);
+void viridian_time_load_domain_ctxt(
+    struct domain *d, struct hvm_viridian_domain_context *ctxt);

Can ctxt be constifyed in the load case?

Thanks, Roger.

The specification [1] defines a type so we should use it, rather than just OR-ing and AND-ing magic bits.

No functional change.

NOTE: The type defined in the specification does include an anonymous
sub-struct in the page type but, as we currently use only the first element, the struct declaration has been omitted.

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/synic.c  | 52 +++++++++++++++++++++++---------------
 xen/include/asm-x86/hvm/viridian.h |  2 +-
 2 files changed, 33 insertions(+), 21 deletions(-)

xen/arch/x86/hvm/viridian/synic.c +0 -0

index 3f043f34ee..735c4c897c 100644

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 17:09
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the 'virtual VP assist page'

Quoted 1 line · Paul Durrant

The specification [1] defines a type so we should use it, rather than

just

Quoted 6 lines

OR-ing and AND-ing magic bits.

No functional change.

NOTE: The type defined in the specification does include an anonymous
sub-struct in the page type but, as we currently use only the

first

Quoted 3 lines

element, the struct declaration has been omitted.

[1] https://github.com/MicrosoftDocs/Virtualization-

Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specif ication%20v5.0C.pdf

Quoted 8 lines

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/synic.c  | 52 +++++++++++++++++++++++--------

-------

Review

Quoted 4 lines

 xen/include/asm-x86/hvm/viridian.h |  2 +-
 2 files changed, 33 insertions(+), 21 deletions(-)

(quoted diff) +0 -0

b/xen/arch/x86/hvm/viridian/synic.c

Quoted 7 lines

index 3f043f34ee..735c4c897c 100644

xen/arch/x86/hvm/viridian/synic.c +0 -0

@@ -13,6 +13,18 @@
 #include <asm/apic.h>
 #include <asm/hvm/support.h>
 

xen/arch/x86/hvm/viridian/synic.c line 18

Quoted 3 lines

xen/arch/x86/hvm/viridian/synic.c +3 -0

+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+    uint32_t no_eoi:1;

Maybe bool:1 so you can use true/false?

Thanks, Roger.

Quoted 46 lines

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 17:09
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the 'virtual VP assist page'

Quoted 1 line · Paul Durrant

The specification [1] defines a type so we should use it, rather than

just

Quoted 6 lines

OR-ing and AND-ing magic bits.

No functional change.

NOTE: The type defined in the specification does include an anonymous
sub-struct in the page type but, as we currently use only the

first

Quoted 3 lines

element, the struct declaration has been omitted.

[1] https://github.com/MicrosoftDocs/Virtualization-

Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specif ication%20v5.0C.pdf

Quoted 8 lines

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/synic.c  | 52 +++++++++++++++++++++++--------

-------

Review

Quoted 4 lines

 xen/include/asm-x86/hvm/viridian.h |  2 +-
 2 files changed, 33 insertions(+), 21 deletions(-)

(quoted diff) +0 -0

b/xen/arch/x86/hvm/viridian/synic.c

Quoted 7 lines

index 3f043f34ee..735c4c897c 100644

xen/arch/x86/hvm/viridian/synic.c +0 -0

@@ -13,6 +13,18 @@
 #include <asm/apic.h>
 #include <asm/hvm/support.h>
 

Quoted 6 lines

xen/arch/x86/hvm/viridian/synic.c line 18

Quoted 3 lines

xen/arch/x86/hvm/viridian/synic.c +3 -0

+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+    uint32_t no_eoi:1;

Maybe bool:1 so you can use true/false?

No, I'm very specifically using a 32-bit bitfield to match what the spec. says.

-----Original Message-----
From: Roger Pau Monne
Sent: 31 October 2018 08:54
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan Beulich <[email protected]>
Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the 'virtual VP assist page'

Quoted 6 lines · Paul Durrant

Quoted 6 lines

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 17:09
To: Paul Durrant <[email protected]>
Cc: [email protected]; Andrew Cooper <[email protected]>; Wei Liu <[email protected]>; Jan

Beulich

Quoted 6 lines

Quoted 6 lines

<[email protected]>
Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the 'virtual VP assist page'

Quoted 1 line · Paul Durrant

The specification [1] defines a type so we should use it, rather

than

Quoted 6 lines

Quoted 6 lines

just

Quoted 5 lines

OR-ing and AND-ing magic bits.

No functional change.

NOTE: The type defined in the specification does include an

anonymous

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

sub-struct in the page type but, as we currently use only the

first

Quoted 3 lines

element, the struct declaration has been omitted.

[1] https://github.com/MicrosoftDocs/Virtualization-

Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specif

Quoted 9 lines

Quoted 9 lines

ication%20v5.0C.pdf

Quoted 8 lines

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Wei Liu <[email protected]>

---
 xen/arch/x86/hvm/viridian/synic.c  | 52 +++++++++++++++++++++++----

----

Quoted 13 lines

Quoted 13 lines

-------

Review

Quoted 4 lines

 xen/include/asm-x86/hvm/viridian.h |  2 +-
 2 files changed, 33 insertions(+), 21 deletions(-)

(quoted diff) +0 -0

b/xen/arch/x86/hvm/viridian/synic.c

Quoted 7 lines

index 3f043f34ee..735c4c897c 100644

xen/arch/x86/hvm/viridian/synic.c +0 -0

@@ -13,6 +13,18 @@
 #include <asm/apic.h>
 #include <asm/hvm/support.h>
 

Quoted 8 lines

Quoted 6 lines

xen/arch/x86/hvm/viridian/synic.c line 18

Quoted 3 lines

xen/arch/x86/hvm/viridian/synic.c +3 -0

+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+    uint32_t no_eoi:1;

Maybe bool:1 so you can use true/false?

No, I'm very specifically using a 32-bit bitfield to match what the

spec. says.

Right, but no_eoi is a single flag on that bitfield, unless I'm missing something I think you could just use:

typedef union _HV_VIRTUAL_APIC_ASSIST

{
    struct {
        bool no_eoi:1;
    } fields;
    uint32_t raw;
} HV_VIRTUAL_APIC_ASSIST;

If you wish to access the raw value as a uint32_t while keeping access to individual flags easy. This union also has the advantage that adding new fields won't require you to adjust the size of the reserved_zero field.

Thanks, Roger.

Quoted 36 lines

Quoted 1 line · On 31.10.18 at 10:27, <[email protected]> wrote:

From: Roger Pau Monne
Sent: 31 October 2018 08:54

Quoted 10 lines · Paul Durrant

Quoted 10 lines

From: Roger Pau Monne
Sent: 30 October 2018 17:09

Quoted 6 lines · Paul Durrant

xen/arch/x86/hvm/viridian/synic.c +0 -0

@@ -13,6 +13,18 @@
 #include <asm/apic.h>
 #include <asm/hvm/support.h>
 

Quoted 8 lines

Quoted 6 lines

xen/arch/x86/hvm/viridian/synic.c line 18

Quoted 3 lines

xen/arch/x86/hvm/viridian/synic.c +3 -0

+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+    uint32_t no_eoi:1;

Maybe bool:1 so you can use true/false?

No, I'm very specifically using a 32-bit bitfield to match what the

spec. says.

Right, but no_eoi is a single flag on that bitfield, unless I'm missing something I think you could just use:

typedef union _HV_VIRTUAL_APIC_ASSIST

{
    struct {
        bool no_eoi:1;
    } fields;
    uint32_t raw;
} HV_VIRTUAL_APIC_ASSIST;

Quoted 5 lines

If you wish to access the raw value as a uint32_t while keeping access to individual flags easy. This union also has the advantage that adding new fields won't require you to adjust the size of the reserved_zero field.

Agreed it's easier from a coding PoV, but I still prefer to stick with bitfield definitions that span the full 32-bits to make it line up with the spec. (currently section 10.3.5). If Microsoft had actually put a struct definition there then I would use that, but as it is the layout illustration is all there is and I want to match it as closely as I can.

-----Original Message-----
From: Jan Beulich [mailto:[email protected]]
Sent: 31 October 2018 09:42
To: Paul Durrant <[email protected]>
Cc: Andrew Cooper <[email protected]>; Roger Pau Monne <[email protected]>; Wei Liu <[email protected]>; xen-devel <xen- [email protected]>
Subject: RE: [Xen-devel] [PATCH 7/8] viridian: define type for the 'virtual VP assist page'

Quoted 36 lines

Quoted 36 lines

Quoted 1 line · On 31.10.18 at 10:27, <[email protected]> wrote:

From: Roger Pau Monne
Sent: 31 October 2018 08:54

Quoted 10 lines · Paul Durrant

Quoted 10 lines

From: Roger Pau Monne
Sent: 30 October 2018 17:09

Quoted 6 lines · Paul Durrant

xen/arch/x86/hvm/viridian/synic.c +0 -0

@@ -13,6 +13,18 @@
 #include <asm/apic.h>
 #include <asm/hvm/support.h>
 

Quoted 8 lines

Quoted 6 lines

xen/arch/x86/hvm/viridian/synic.c line 18

Quoted 3 lines

xen/arch/x86/hvm/viridian/synic.c +3 -0

+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+    uint32_t no_eoi:1;

Maybe bool:1 so you can use true/false?

No, I'm very specifically using a 32-bit bitfield to match what the

spec. says.

Right, but no_eoi is a single flag on that bitfield, unless I'm missing something I think you could just use:

typedef union _HV_VIRTUAL_APIC_ASSIST

{
    struct {
        bool no_eoi:1;
    } fields;
    uint32_t raw;
} HV_VIRTUAL_APIC_ASSIST;

Quoted 8 lines

Quoted 5 lines

If you wish to access the raw value as a uint32_t while keeping access to individual flags easy. This union also has the advantage that adding new fields won't require you to adjust the size of the reserved_zero field.

Agreed it's easier from a coding PoV, but I still prefer to stick with bitfield definitions that span the full 32-bits to make it line up with

the

Quoted 2 lines

spec. (currently section 10.3.5). If Microsoft had actually put a struct definition there then I would use that, but as it is the layout

illustration

Quoted 1 line

is all there is and I want to match it as closely as I can.

I'm afraid I disagree with this view of yours: A field of the form "uint32_t x:1" does not reserve the following 31 bits. That's in part because types other than plain, signed, or unsigned int as well as bool aren't allowed by the base C standard anyway for bit fields; allowing them is a (quite common) compiler extension (and there are actually quirks when it comes to using types wider than int, but a bit count not specifying more bits than an int can hold). Just look at the resulting code of this example:

#include <stddef.h>
#include <stdint.h>

struct s {
	uint32_t x:1;

	char c;
};

unsigned test(void) {
	return offsetof(struct s, c);
}

Jan

Quoted 63 lines

Quoted 1 line · On 31.10.18 at 11:01, <[email protected]> wrote:

From: Jan Beulich [mailto:[email protected]]
Sent: 31 October 2018 09:42

Quoted 36 lines

Quoted 36 lines

Quoted 1 line · On 31.10.18 at 10:27, <[email protected]> wrote:

From: Roger Pau Monne
Sent: 31 October 2018 08:54

Quoted 10 lines · Paul Durrant

Quoted 10 lines

From: Roger Pau Monne
Sent: 30 October 2018 17:09

Quoted 6 lines · Paul Durrant

xen/arch/x86/hvm/viridian/synic.c +0 -0

@@ -13,6 +13,18 @@
 #include <asm/apic.h>
 #include <asm/hvm/support.h>
 

Quoted 8 lines

Quoted 6 lines

xen/arch/x86/hvm/viridian/synic.c line 18

Quoted 3 lines

xen/arch/x86/hvm/viridian/synic.c +3 -0

+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+    uint32_t no_eoi:1;

Maybe bool:1 so you can use true/false?

No, I'm very specifically using a 32-bit bitfield to match what the

spec. says.

Right, but no_eoi is a single flag on that bitfield, unless I'm missing something I think you could just use:

typedef union _HV_VIRTUAL_APIC_ASSIST

{
    struct {
        bool no_eoi:1;
    } fields;
    uint32_t raw;
} HV_VIRTUAL_APIC_ASSIST;

Quoted 8 lines

Quoted 5 lines

If you wish to access the raw value as a uint32_t while keeping access to individual flags easy. This union also has the advantage that adding new fields won't require you to adjust the size of the reserved_zero field.

Agreed it's easier from a coding PoV, but I still prefer to stick with bitfield definitions that span the full 32-bits to make it line up with

the

Quoted 2 lines

spec. (currently section 10.3.5). If Microsoft had actually put a struct definition there then I would use that, but as it is the layout

illustration

Quoted 1 line

is all there is and I want to match it as closely as I can.

I'm afraid I disagree with this view of yours: A field of the form "uint32_t x:1" does not reserve the following 31 bits. That's in part because types other than plain, signed, or unsigned int as well as bool aren't allowed by the base C standard anyway for bit fields; allowing them is a (quite common) compiler extension (and there are actually quirks when it comes to using types wider than int, but a bit count not specifying more bits than an int can hold). Just look at the resulting code of this example:

Quoted 5 lines

#include <stddef.h>
#include <stdint.h>

struct s {
	uint32_t x:1;

Yes, the offset of c is 1. But adding "uint32_t y:31;" here makes the offset
4. My definition was:

typedef struct _HV_VIRTUAL_APIC_ASSIST

{
    uint32_t no_eoi:1;
    uint32_t reserved_zero:31;
} HV_VIRTUAL_APIC_ASSIST;

... which I plan to stick with.

The 'vp_assist' page is currently an example of a guest page which needs to be kept mapped throughout the life-time of a guest, but there are other such examples in the specifiction [1]. This patch therefore introduces a generic 'viridian_page' type and converts the current vp_assist/apic_assist related code to use it. Subsequent patches implementing other enlightments can then also make use of it.

This patch also renames the 'vp_assist_pending' field in struct hvm_viridian_vcpu_context to 'apic_assist_pending' to more accurately reflect its meaning. The term 'vp_assist' applies to the whole page rather than just the EOI-avoidance enlightenment. New versons of the specification have defined data structures for other enlightenments within the same page.

No functional change.

[1] https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0C.pdf

Signed-off-by Paul Durrant <[email protected]>

---
Cc: Ian Jackson <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/misc/xen-hvmctx.c                |  4 +--
 xen/arch/x86/hvm/viridian/synic.c      | 47 ++++++++++++++++------------------
 xen/include/asm-x86/hvm/viridian.h     | 13 ++++++----
 xen/include/public/arch-x86/hvm/save.h |  2 +-
 4 files changed, 33 insertions(+), 33 deletions(-)

tools/misc/xen-hvmctx.c +0 -0

index 40e77851be..a4efadf307 100644

-----Original Message-----
From: Roger Pau Monne
Sent: 30 October 2018 17:18
To: Paul Durrant <[email protected]>
Cc: [email protected]; Wei Liu <[email protected]>; Ian Jackson <[email protected]>; Jan Beulich <[email protected]>; Andrew Cooper <[email protected]>
Subject: Re: [Xen-devel] [PATCH 8/8] viridian: introduce struct viridian_page

Quoted 1 line · Paul Durrant

The 'vp_assist' page is currently an example of a guest page which needs

to

Quoted 3 lines

be kept mapped throughout the life-time of a guest, but there are other such examples in the specifiction [1]. This patch therefore introduces a generic 'viridian_page' type and converts the current

vp_assist/apic_assist

Quoted 1 line

related code to use it. Subsequent patches implementing other

enlightments

Quoted 5 lines

can then also make use of it.

This patch also renames the 'vp_assist_pending' field in struct hvm_viridian_vcpu_context to 'apic_assist_pending' to more accurately reflect its meaning. The term 'vp_assist' applies to the whole page

rather

Quoted 1 line

than just the EOI-avoidance enlightenment. New versons of the

specification

Quoted 1 line

have defined data structures for other enlightenments within the same

page.

Quoted 3 lines

No functional change.

Quoted 1 line

[1] https://github.com/MicrosoftDocs/Virtualization-

Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specif ication%20v5.0C.pdf

Quoted 2 lines

Signed-off-by Paul Durrant <[email protected]>

Reviewed-by Roger Pau Monné <[email protected]>

Quoted 8 lines

---
Cc: Ian Jackson <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Jan Beulich <[email protected]>
Cc: Andrew Cooper <[email protected]>

---
 tools/misc/xen-hvmctx.c                |  4 +--
 xen/arch/x86/hvm/viridian/synic.c      | 47 ++++++++++++++++-----------

-------

tools/misc/xen-hvmctx.c static void dump_viridian_vcpu(void) · line 373

Quoted 13 lines

 xen/include/asm-x86/hvm/viridian.h     | 13 ++++++----
 xen/include/public/arch-x86/hvm/save.h |  2 +-
 4 files changed, 33 insertions(+), 33 deletions(-)

tools/misc/xen-hvmctx.c +0 -1

index 40e77851be..a4efadf307 100644
@@ -370,9 +370,9 @@ static void dump_viridian_vcpu(void)
 {
     HVM_SAVE_TYPE(VIRIDIAN_VCPU) p;
     READ(p);
-    printf("    VIRIDIAN_VCPU: vp_assist_msr 0x%llx, vp_assist_pending

%s\n",

tools/misc/xen-hvmctx.c static void dump_viridian_vcpu(void) · line 373

Quoted 1 line

tools/misc/xen-hvmctx.c +1 -0

+    printf("    VIRIDIAN_VCPU: vp_assist_msr 0x%llx,

apic_assist_pending %s\n",

tools/misc/xen-hvmctx.c static void dump_viridian_vcpu(void) · line 375

Quoted 3 lines

tools/misc/xen-hvmctx.c +1 -1

 	   (unsigned long long) p.vp_assist_msr,
-	   p.vp_assist_pending ? "true" : "false");
+	   p.apic_assist_pending ? "true" : "false");

There's some hardcoded tabs in the above line, would you mind fixing them since you are already touching this code?

Thanks, Roger.