* [PATCH 1/8] viridian: move the code into its own sub-directory
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 9:25 ` Jan Beulich
2018-10-31 10:17 ` Jan Beulich
2018-10-29 18:02 ` [PATCH 1/2] x86/mm/p2m: don't needlessly limit MMIO mapping order to 4k Paul Durrant
` (8 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
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 <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
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%)
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 5e04bc1429..86b106f8e7 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -1,5 +1,6 @@
subdir-y += svm
subdir-y += vmx
+subdir-y += viridian
obj-y += asid.o
obj-y += dm.o
@@ -23,7 +24,6 @@ obj-y += rtc.o
obj-y += save.o
obj-y += stdvga.o
obj-y += vioapic.o
-obj-y += viridian.o
obj-y += vlapic.o
obj-y += vm_event.o
obj-y += vmsi.o
diff --git a/xen/arch/x86/hvm/viridian/Makefile b/xen/arch/x86/hvm/viridian/Makefile
new file mode 100644
index 0000000000..09fd0a5f3c
--- /dev/null
+++ b/xen/arch/x86/hvm/viridian/Makefile
@@ -0,0 +1 @@
+obj-y += viridian.o
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
similarity index 99%
rename from xen/arch/x86/hvm/viridian.c
rename to xen/arch/x86/hvm/viridian/viridian.c
index f42b1f063e..3e9beda831 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -4,7 +4,7 @@
* 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
+ * https://github.com/Microsoft/Virtualization-Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0.pdf
*
* for more information.
*/
@@ -334,7 +334,7 @@ static void dump_reference_tsc(const struct domain *d)
const union viridian_reference_tsc *rt;
rt = &d->arch.hvm.viridian.reference_tsc;
-
+
printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: enabled: %x pfn: %lx\n",
d->domain_id,
rt->fields.enabled, (unsigned long)rt->fields.pfn);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 1/8] viridian: move the code into its own sub-directory
2018-10-29 18:02 ` [PATCH 1/8] viridian: move the code into its own sub-directory Paul Durrant
@ 2018-10-30 9:25 ` Jan Beulich
2018-10-31 10:17 ` Jan Beulich
1 sibling, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2018-10-30 9:25 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, xen-devel
>>> On 29.10.18 at 19:02, <paul.durrant@citrix.com> 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 <paul.durrant@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/8] viridian: move the code into its own sub-directory
2018-10-29 18:02 ` [PATCH 1/8] viridian: move the code into its own sub-directory Paul Durrant
2018-10-30 9:25 ` Jan Beulich
@ 2018-10-31 10:17 ` Jan Beulich
2018-10-31 10:30 ` Paul Durrant
1 sibling, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2018-10-31 10:17 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, xen-devel
>>> On 29.10.18 at 19:02, <paul.durrant@citrix.com> 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 <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 1/8] viridian: move the code into its own sub-directory
2018-10-31 10:17 ` Jan Beulich
@ 2018-10-31 10:30 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-31 10:30 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, Wei Liu, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 31 October 2018 10:18
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; xen-devel <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH 1/8] viridian: move the code into its own sub-
> directory
>
> >>> On 29.10.18 at 19:02, <paul.durrant@citrix.com> 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 <paul.durrant@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> > 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.
>
Oh. Good spot. I'll fold that change into v2 of this patch.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/2] x86/mm/p2m: don't needlessly limit MMIO mapping order to 4k
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
2018-10-29 18:02 ` [PATCH 1/8] viridian: move the code into its own sub-directory Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-29 18:03 ` Paul Durrant
2018-10-29 18:02 ` [PATCH 2/2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page() Paul Durrant
` (7 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
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->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 <paul.durrant@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
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(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a00a3c1bff..f972b4819d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -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 /*
@@ -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) &&
- 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 1/2] x86/mm/p2m: don't needlessly limit MMIO mapping order to 4k
2018-10-29 18:02 ` [PATCH 1/2] x86/mm/p2m: don't needlessly limit MMIO mapping order to 4k Paul Durrant
@ 2018-10-29 18:03 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:03 UTC (permalink / raw)
To: Paul Durrant, xen-devel@lists.xenproject.org
Cc: Andrew Cooper, Wei Liu, Jan Beulich
Apologies. Ignore this as it is just a re-post of a stale patch.
Paul
> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 29 October 2018 18:02
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>
> 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-
> >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 <paul.durrant@citrix.com>
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
>
> 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(-)
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index a00a3c1bff..f972b4819d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -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 /*
> @@ -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) &&
> - 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
2018-10-29 18:02 ` [PATCH 1/8] viridian: move the code into its own sub-directory Paul Durrant
2018-10-29 18:02 ` [PATCH 1/2] x86/mm/p2m: don't needlessly limit MMIO mapping order to 4k Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-29 18:03 ` Paul Durrant
2018-10-29 18:02 ` [PATCH 2/8] viridian: remove MSR perf counters Paul Durrant
` (6 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
Konrad Rzeszutek Wilk, Andrew Cooper, Ian Jackson, George Dunlap,
Tim Deegan, Julien Grall, Paul Durrant, Jan Beulich
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 <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <George.Dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
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(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c53bc86a68..f0ae016e7a 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -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);
}
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 407e299e50..656c8dd3ac 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -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);
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 55df18501e..b264a97bd8 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -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();
@@ -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);
@@ -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);
}
/*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index f972b4819d..e5880d646b 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -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);
@@ -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) )
@@ -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);
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 543ea030e3..c0ce5673ba 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -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;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 878e668bf5..971c6b100c 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -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);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 8b438ae4bc..40db9e7849 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -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,
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index b20bad17de..4fd3eb1dca 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -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",
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index c75333c077..a003d82204 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 2/2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
2018-10-29 18:02 ` [PATCH 2/2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page() Paul Durrant
@ 2018-10-29 18:03 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:03 UTC (permalink / raw)
To: Paul Durrant, xen-devel@lists.xenproject.org
Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson
Apologies. Ignore this as it is just a re-post of a stale patch.
Paul
> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 29 October 2018 18:02
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Julien Grall <julien.grall@arm.com>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Jun Nakajima
> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>
> 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 <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: George Dunlap <George.Dunlap@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> ---
> 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(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index c53bc86a68..f0ae016e7a 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -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);
> }
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 407e299e50..656c8dd3ac 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -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);
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 55df18501e..b264a97bd8 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -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();
> @@ -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);
> @@ -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);
> }
>
> /*
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index f972b4819d..e5880d646b 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -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);
> @@ -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) )
> @@ -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);
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 543ea030e3..c0ce5673ba 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -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;
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 878e668bf5..971c6b100c 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -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);
>
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index 8b438ae4bc..40db9e7849 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -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,
> diff --git a/xen/drivers/passthrough/x86/iommu.c
> b/xen/drivers/passthrough/x86/iommu.c
> index b20bad17de..4fd3eb1dca 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -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",
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index c75333c077..a003d82204 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/8] viridian: remove MSR perf counters
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
` (2 preceding siblings ...)
2018-10-29 18:02 ` [PATCH 2/2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page() Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 16:25 ` Roger Pau Monné
2018-10-29 18:02 ` [PATCH 3/8] viridian: remove comments referencing section number in the spec Paul Durrant
` (5 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
They're not really useful so maintaining them is pointless.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/x86/hvm/viridian/viridian.c | 21 ---------------------
xen/include/asm-x86/perfc_defn.h | 26 --------------------------
2 files changed, 47 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 3e9beda831..c5722d6992 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -11,7 +11,6 @@
#include <xen/sched.h>
#include <xen/version.h>
-#include <xen/perfc.h>
#include <xen/hypercall.h>
#include <xen/domain_page.h>
#include <asm/guest_access.h>
@@ -560,13 +559,11 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
switch ( idx )
{
case HV_X64_MSR_GUEST_OS_ID:
- perfc_incr(mshv_wrmsr_osid);
d->arch.hvm.viridian.guest_os_id.raw = val;
dump_guest_os_id(d);
break;
case HV_X64_MSR_HYPERCALL:
- perfc_incr(mshv_wrmsr_hc_page);
d->arch.hvm.viridian.hypercall_gpa.raw = val;
dump_hypercall(d);
if ( d->arch.hvm.viridian.hypercall_gpa.fields.enabled )
@@ -574,18 +571,15 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
break;
case HV_X64_MSR_VP_INDEX:
- perfc_incr(mshv_wrmsr_vp_index);
break;
case HV_X64_MSR_EOI:
- perfc_incr(mshv_wrmsr_eoi);
vlapic_EOI_set(vcpu_vlapic(v));
break;
case HV_X64_MSR_ICR: {
u32 eax = (u32)val, edx = (u32)(val >> 32);
struct vlapic *vlapic = vcpu_vlapic(v);
- perfc_incr(mshv_wrmsr_icr);
eax &= ~(1 << 12);
edx &= 0xff000000;
vlapic_set_reg(vlapic, APIC_ICR2, edx);
@@ -595,12 +589,10 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
}
case HV_X64_MSR_TPR:
- perfc_incr(mshv_wrmsr_tpr);
vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI, (uint8_t)val);
break;
case HV_X64_MSR_VP_ASSIST_PAGE:
- perfc_incr(mshv_wrmsr_apic_msr);
teardown_vp_assist(v); /* release any previous mapping */
v->arch.hvm.viridian.vp_assist.msr.raw = val;
dump_vp_assist(v);
@@ -612,7 +604,6 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
return X86EMUL_EXCEPTION;
- perfc_incr(mshv_wrmsr_tsc_msr);
d->arch.hvm.viridian.reference_tsc.raw = val;
dump_reference_tsc(d);
if ( d->arch.hvm.viridian.reference_tsc.fields.enabled )
@@ -704,17 +695,14 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
switch ( idx )
{
case HV_X64_MSR_GUEST_OS_ID:
- perfc_incr(mshv_rdmsr_osid);
*val = d->arch.hvm.viridian.guest_os_id.raw;
break;
case HV_X64_MSR_HYPERCALL:
- perfc_incr(mshv_rdmsr_hc_page);
*val = d->arch.hvm.viridian.hypercall_gpa.raw;
break;
case HV_X64_MSR_VP_INDEX:
- perfc_incr(mshv_rdmsr_vp_index);
*val = v->vcpu_id;
break;
@@ -722,7 +710,6 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
if ( viridian_feature_mask(d) & HVMPV_no_freq )
return X86EMUL_EXCEPTION;
- perfc_incr(mshv_rdmsr_tsc_frequency);
*val = (uint64_t)d->arch.tsc_khz * 1000ull;
break;
@@ -730,23 +717,19 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
if ( viridian_feature_mask(d) & HVMPV_no_freq )
return X86EMUL_EXCEPTION;
- perfc_incr(mshv_rdmsr_apic_frequency);
*val = 1000000000ull / APIC_BUS_CYCLE_NS;
break;
case HV_X64_MSR_ICR:
- perfc_incr(mshv_rdmsr_icr);
*val = (((uint64_t)vlapic_get_reg(vcpu_vlapic(v), APIC_ICR2) << 32) |
vlapic_get_reg(vcpu_vlapic(v), APIC_ICR));
break;
case HV_X64_MSR_TPR:
- perfc_incr(mshv_rdmsr_tpr);
*val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
break;
case HV_X64_MSR_VP_ASSIST_PAGE:
- perfc_incr(mshv_rdmsr_apic_msr);
*val = v->arch.hvm.viridian.vp_assist.msr.raw;
break;
@@ -754,7 +737,6 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
return X86EMUL_EXCEPTION;
- perfc_incr(mshv_rdmsr_tsc_msr);
*val = d->arch.hvm.viridian.reference_tsc.raw;
break;
@@ -771,7 +753,6 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
printk(XENLOG_G_INFO "d%d: VIRIDIAN MSR_TIME_REF_COUNT: accessed\n",
d->domain_id);
- perfc_incr(mshv_rdmsr_time_ref_count);
*val = raw_trc_val(d) + trc->off;
break;
}
@@ -876,7 +857,6 @@ int viridian_hypercall(struct cpu_user_regs *regs)
/*
* See section 14.5.1 of the specification.
*/
- perfc_incr(mshv_call_long_wait);
do_sched_op(SCHEDOP_yield, guest_handle_from_ptr(NULL, void));
status = HV_STATUS_SUCCESS;
break;
@@ -895,7 +875,6 @@ int viridian_hypercall(struct cpu_user_regs *regs)
/*
* See sections 9.4.2 and 9.4.4 of the specification.
*/
- perfc_incr(mshv_call_flush);
/* These hypercalls should never use the fast-call convention. */
status = HV_STATUS_INVALID_PARAMETER;
diff --git a/xen/include/asm-x86/perfc_defn.h b/xen/include/asm-x86/perfc_defn.h
index 6db8746906..1a9ea3f89e 100644
--- a/xen/include/asm-x86/perfc_defn.h
+++ b/xen/include/asm-x86/perfc_defn.h
@@ -112,32 +112,6 @@ PERFCOUNTER(shadow_unsync, "shadow OOS unsyncs")
PERFCOUNTER(shadow_unsync_evict, "shadow OOS evictions")
PERFCOUNTER(shadow_resync, "shadow OOS resyncs")
-PERFCOUNTER(mshv_call_sw_addr_space, "MS Hv Switch Address Space")
-PERFCOUNTER(mshv_call_flush_tlb_list, "MS Hv Flush TLB list")
-PERFCOUNTER(mshv_call_flush_tlb_all, "MS Hv Flush TLB all")
-PERFCOUNTER(mshv_call_long_wait, "MS Hv Notify long wait")
-PERFCOUNTER(mshv_call_flush, "MS Hv Flush TLB")
-PERFCOUNTER(mshv_rdmsr_osid, "MS Hv rdmsr Guest OS ID")
-PERFCOUNTER(mshv_rdmsr_hc_page, "MS Hv rdmsr hypercall page")
-PERFCOUNTER(mshv_rdmsr_vp_index, "MS Hv rdmsr vp index")
-PERFCOUNTER(mshv_rdmsr_tsc_frequency, "MS Hv rdmsr TSC frequency")
-PERFCOUNTER(mshv_rdmsr_apic_frequency, "MS Hv rdmsr APIC frequency")
-PERFCOUNTER(mshv_rdmsr_time_ref_count, "MS Hv rdmsr time ref count")
-PERFCOUNTER(mshv_rdmsr_icr, "MS Hv rdmsr icr")
-PERFCOUNTER(mshv_rdmsr_tpr, "MS Hv rdmsr tpr")
-PERFCOUNTER(mshv_rdmsr_apic_assist, "MS Hv rdmsr APIC assist")
-PERFCOUNTER(mshv_rdmsr_apic_msr, "MS Hv rdmsr APIC msr")
-PERFCOUNTER(mshv_rdmsr_tsc_msr, "MS Hv rdmsr TSC msr")
-PERFCOUNTER(mshv_wrmsr_osid, "MS Hv wrmsr Guest OS ID")
-PERFCOUNTER(mshv_wrmsr_hc_page, "MS Hv wrmsr hypercall page")
-PERFCOUNTER(mshv_wrmsr_vp_index, "MS Hv wrmsr vp index")
-PERFCOUNTER(mshv_wrmsr_icr, "MS Hv wrmsr icr")
-PERFCOUNTER(mshv_wrmsr_tpr, "MS Hv wrmsr tpr")
-PERFCOUNTER(mshv_wrmsr_eoi, "MS Hv wrmsr eoi")
-PERFCOUNTER(mshv_wrmsr_apic_assist, "MS Hv wrmsr APIC assist")
-PERFCOUNTER(mshv_wrmsr_apic_msr, "MS Hv wrmsr APIC msr")
-PERFCOUNTER(mshv_wrmsr_tsc_msr, "MS Hv wrmsr TSC msr")
-
PERFCOUNTER(realmode_emulations, "realmode instructions emulated")
PERFCOUNTER(realmode_exits, "vmexits from realmode")
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 3/8] viridian: remove comments referencing section number in the spec.
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
` (3 preceding siblings ...)
2018-10-29 18:02 ` [PATCH 2/8] viridian: remove MSR perf counters Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 16:34 ` Roger Pau Monné
2018-10-29 18:02 ` [PATCH 4/8] viridian: remove duplicate union types Paul Durrant
` (4 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
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 <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/x86/hvm/viridian/viridian.c | 82 ++++++++++++++++--------------------
xen/include/asm-x86/hvm/viridian.h | 4 --
2 files changed, 36 insertions(+), 50 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index c5722d6992..db166d41c5 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -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>
@@ -103,10 +99,7 @@
#define HV_FLUSH_ALL_PROCESSORS 1
/*
- * 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
*/
typedef struct {
/* Access to virtual MSRs */
@@ -168,7 +161,7 @@ typedef union _HV_CRASH_CTL_REG_CONTENTS
#define CPUID4A_MSR_BASED_APIC (1 << 3)
#define CPUID4A_RELAX_TIMER_INT (1 << 5)
-/* Viridian CPUID leaf 6: Implementation HW features detected and in use. */
+/* Viridian CPUID leaf 6: Implementation HW features detected and in use */
#define CPUID6A_APIC_OVERLAY (1 << 0)
#define CPUID6A_MSR_BITMAPS (1 << 1)
#define CPUID6A_NESTED_PAGING (1 << 3)
@@ -204,7 +197,6 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
switch ( leaf )
{
case 0:
- /* See section 2.4.1 of the specification */
res->a = 0x40000006; /* Maximum leaf */
memcpy(&res->b, "Micr", 4);
memcpy(&res->c, "osof", 4);
@@ -212,13 +204,14 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
break;
case 1:
- /* See section 2.4.2 of the specification */
memcpy(&res->a, "Hv#1", 4);
break;
case 2:
- /* Hypervisor information, but only if the guest has set its
- own version number. */
+ /*
+ * Hypervisor information, but only if the guest has set its
+ * own version number.
+ */
if ( d->arch.hvm.viridian.guest_os_id.raw == 0 )
break;
res->a = viridian_build;
@@ -230,9 +223,9 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
case 3:
{
/*
- * Section 2.4.4 details this leaf and states that EAX and EBX
- * are defined to be the low and high parts of the partition
- * privilege mask respectively.
+ * The specification states that EAX and EBX are defined to be
+ * the low and high parts of the partition privilege mask
+ * respectively.
*/
HV_PARTITION_PRIVILEGE_MASK mask = {
.AccessIntrCtrlRegs = 1,
@@ -382,11 +375,6 @@ static void initialize_vp_assist(struct vcpu *v)
ASSERT(!v->arch.hvm.viridian.vp_assist.va);
- /*
- * See section 7.8.7 of the specification for details of this
- * enlightenment.
- */
-
if ( !page )
goto fail;
@@ -409,8 +397,8 @@ static void initialize_vp_assist(struct vcpu *v)
return;
fail:
- gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n", gmfn,
- mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
+ gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
+ gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
}
static void teardown_vp_assist(struct vcpu *v)
@@ -498,14 +486,15 @@ static void update_reference_tsc(struct domain *d, bool_t initialize)
clear_page(p);
/*
- * This enlightenment must be disabled is the host TSC is not invariant.
- * However it is also disabled if vtsc is true (which means rdtsc is being
- * emulated). This generally happens when guest TSC freq and host TSC freq
- * don't match. The TscScale value could be adjusted to cope with this,
- * allowing vtsc to be turned off, but support for this is not yet present
- * in the hypervisor. Thus is it is possible that migrating a Windows VM
- * between hosts of differing TSC frequencies may result in large
- * differences in guest performance.
+ * This enlightenment must be disabled is the host TSC is not
+ * invariant. However it is also disabled if vtsc is true (which
+ * means rdtsc is being emulated). This generally happens when guest
+ * TSC freq and host TSC freq don't match. The TscScale value could be
+ * adjusted to cope with this, allowing vtsc to be turned off, but
+ * support for this is not yet present in the hypervisor. Thus is it
+ * is possible that migrating a Windows VM between hosts of differing
+ * TSC frequencies may result in large differences in guest
+ * performance.
*/
if ( !host_tsc_is_safe() || d->arch.vtsc )
{
@@ -515,10 +504,10 @@ static void update_reference_tsc(struct domain *d, bool_t initialize)
* this mechanism is no longer a reliable source of time and that
* the VM should fall back to a different source.
*
- * Server 2012 (6.2 kernel) and 2012 R2 (6.3 kernel) actually violate
- * the spec. and rely on a value of 0 to indicate that this
- * enlightenment should no longer be used. These two kernel
- * versions are currently the only ones to make use of this
+ * Server 2012 (6.2 kernel) and 2012 R2 (6.3 kernel) actually
+ * violate the specification and rely on a value of 0 to indicate
+ * that this enlightenment should no longer be used. These two
+ * kernel versions are currently the only ones to make use of this
* enlightenment, so just use 0 here.
*/
p->TscSequence = 0;
@@ -646,7 +635,8 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
default:
gdprintk(XENLOG_INFO,
- "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
+ "Write %016"PRIx64" to unimplemented MSR %#x\n", val,
+ idx);
return X86EMUL_EXCEPTION;
}
@@ -872,10 +862,6 @@ int viridian_hypercall(struct cpu_user_regs *regs)
uint64_t vcpu_mask;
} input_params;
- /*
- * See sections 9.4.2 and 9.4.4 of the specification.
- */
-
/* These hypercalls should never use the fast-call convention. */
status = HV_STATUS_INVALID_PARAMETER;
if ( input.fast )
@@ -883,7 +869,8 @@ int viridian_hypercall(struct cpu_user_regs *regs)
/* Get input parameters. */
if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
- sizeof(input_params)) != HVMTRANS_okay )
+ sizeof(input_params)) !=
+ HVMTRANS_okay )
break;
/*
@@ -961,7 +948,8 @@ out:
return HVM_HCALL_completed;
}
-static int viridian_save_domain_ctxt(struct vcpu *v, hvm_domain_context_t *h)
+static int viridian_save_domain_ctxt(struct vcpu *v,
+ hvm_domain_context_t *h)
{
const struct domain *d = v->domain;
struct hvm_viridian_domain_context ctxt = {
@@ -977,7 +965,8 @@ static int viridian_save_domain_ctxt(struct vcpu *v, hvm_domain_context_t *h)
return (hvm_save_entry(VIRIDIAN_DOMAIN, 0, h, &ctxt) != 0);
}
-static int viridian_load_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int viridian_load_domain_ctxt(struct domain *d,
+ hvm_domain_context_t *h)
{
struct hvm_viridian_domain_context ctxt;
@@ -1011,7 +1000,8 @@ static int viridian_save_vcpu_ctxt(struct vcpu *v, hvm_domain_context_t *h)
return hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt);
}
-static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
+static int viridian_load_vcpu_ctxt(struct domain *d,
+ hvm_domain_context_t *h)
{
unsigned int vcpuid = hvm_load_instance(h);
struct vcpu *v;
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 071fb445bb..f6008f9bdb 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -77,10 +77,6 @@ union viridian_reference_tsc
} fields;
};
-/*
- * Type defintion as in Microsoft Hypervisor Top-Level Functional
- * Specification v4.0a, section 15.4.2.
- */
typedef struct _HV_REFERENCE_TSC_PAGE
{
uint32_t TscSequence;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 3/8] viridian: remove comments referencing section number in the spec.
2018-10-29 18:02 ` [PATCH 3/8] viridian: remove comments referencing section number in the spec Paul Durrant
@ 2018-10-30 16:34 ` Roger Pau Monné
2018-10-30 17:07 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-30 16:34 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Mon, Oct 29, 2018 at 06:02:06PM +0000, Paul Durrant wrote:
> 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 <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Just one nit below.
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> xen/arch/x86/hvm/viridian/viridian.c | 82 ++++++++++++++++--------------------
> xen/include/asm-x86/hvm/viridian.h | 4 --
> 2 files changed, 36 insertions(+), 50 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
> index c5722d6992..db166d41c5 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -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>
> @@ -103,10 +99,7 @@
> #define HV_FLUSH_ALL_PROCESSORS 1
>
> /*
> - * 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/8] viridian: remove comments referencing section number in the spec.
2018-10-30 16:34 ` Roger Pau Monné
@ 2018-10-30 17:07 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-30 17:07 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 16:34
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 3/8] viridian: remove comments referencing
> section number in the spec.
>
> On Mon, Oct 29, 2018 at 06:02:06PM +0000, Paul Durrant wrote:
> > 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 <paul.durrant@citrix.com>
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
Thanks.
> Just one nit below.
>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> > xen/arch/x86/hvm/viridian/viridian.c | 82 ++++++++++++++++-------------
> -------
> > xen/include/asm-x86/hvm/viridian.h | 4 --
> > 2 files changed, 36 insertions(+), 50 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/viridian/viridian.c
> b/xen/arch/x86/hvm/viridian/viridian.c
> > index c5722d6992..db166d41c5 100644
> > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > @@ -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%20Spec
> ification%20v5.0.pdf
> > - *
> > - * for more information.
> > + * Hypervisor Top Level Functional Specification for more information.
> > */
> >
> > #include <xen/sched.h>
> > @@ -103,10 +99,7 @@
> > #define HV_FLUSH_ALL_PROCESSORS 1
> >
> > /*
> > - * 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 /* ... */.
>
Yes, good point. I'll fix in v2.
Paul
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 4/8] viridian: remove duplicate union types
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
` (4 preceding siblings ...)
2018-10-29 18:02 ` [PATCH 3/8] viridian: remove comments referencing section number in the spec Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 16:40 ` Roger Pau Monné
2018-10-29 18:02 ` [PATCH 5/8] viridian: separate interrupt related enlightenment implementations Paul Durrant
` (3 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
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.
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 <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/x86/hvm/viridian/viridian.c | 8 ++++----
xen/include/asm-x86/hvm/viridian.h | 36 ++++++++----------------------------
2 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index db166d41c5..55b9071c32 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -288,7 +288,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
static void dump_guest_os_id(const struct domain *d)
{
- const union viridian_guest_os_id *goi;
+ const union viridian_guest_os_id_msr *goi;
goi = &d->arch.hvm.viridian.guest_os_id;
@@ -302,7 +302,7 @@ static void dump_guest_os_id(const struct domain *d)
static void dump_hypercall(const struct domain *d)
{
- const union viridian_hypercall_gpa *hg;
+ const union viridian_page_msr *hg;
hg = &d->arch.hvm.viridian.hypercall_gpa;
@@ -313,7 +313,7 @@ static void dump_hypercall(const struct domain *d)
static void dump_vp_assist(const struct vcpu *v)
{
- const union viridian_vp_assist *va;
+ const union viridian_page_msr *va;
va = &v->arch.hvm.viridian.vp_assist.msr;
@@ -323,7 +323,7 @@ static void dump_vp_assist(const struct vcpu *v)
static void dump_reference_tsc(const struct domain *d)
{
- const union viridian_reference_tsc *rt;
+ const union viridian_page_msr *rt;
rt = &d->arch.hvm.viridian.reference_tsc;
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index f6008f9bdb..359fdf5a83 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -9,8 +9,9 @@
#ifndef __ASM_X86_HVM_VIRIDIAN_H__
#define __ASM_X86_HVM_VIRIDIAN_H__
-union viridian_vp_assist
-{ uint64_t raw;
+union viridian_page_msr
+{
+ uint64_t raw;
struct
{
uint64_t enabled:1;
@@ -22,14 +23,14 @@ union viridian_vp_assist
struct viridian_vcpu
{
struct {
- union viridian_vp_assist msr;
+ union viridian_page_msr msr;
void *va;
bool pending;
} vp_assist;
uint64_t crash_param[5];
};
-union viridian_guest_os_id
+union viridian_guest_os_id_msr
{
uint64_t raw;
struct
@@ -43,16 +44,6 @@ union viridian_guest_os_id
} fields;
};
-union viridian_hypercall_gpa
-{ uint64_t raw;
- struct
- {
- uint64_t enabled:1;
- uint64_t reserved_preserved:11;
- uint64_t pfn:48;
- } fields;
-};
-
struct viridian_time_ref_count
{
unsigned long flags;
@@ -66,17 +57,6 @@ struct viridian_time_ref_count
int64_t off;
};
-union viridian_reference_tsc
-{
- uint64_t raw;
- struct
- {
- uint64_t enabled:1;
- uint64_t reserved_preserved:11;
- uint64_t pfn:48;
- } fields;
-};
-
typedef struct _HV_REFERENCE_TSC_PAGE
{
uint32_t TscSequence;
@@ -88,10 +68,10 @@ typedef struct _HV_REFERENCE_TSC_PAGE
struct viridian_domain
{
- union viridian_guest_os_id guest_os_id;
- union viridian_hypercall_gpa hypercall_gpa;
+ union viridian_guest_os_id_msr guest_os_id;
+ union viridian_page_msr hypercall_gpa;
struct viridian_time_ref_count time_ref_count;
- union viridian_reference_tsc reference_tsc;
+ union viridian_page_msr reference_tsc;
};
void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 4/8] viridian: remove duplicate union types
2018-10-29 18:02 ` [PATCH 4/8] viridian: remove duplicate union types Paul Durrant
@ 2018-10-30 16:40 ` Roger Pau Monné
2018-10-30 17:03 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-30 16:40 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Mon, Oct 29, 2018 at 06:02:07PM +0000, Paul Durrant wrote:
> 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.
I agree with the unification of the viridian_page_msr struct.
> Also, rename 'viridian_guest_os_id' to 'viridian_guest_os_id_msr' since it
> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 4/8] viridian: remove duplicate union types
2018-10-30 16:40 ` Roger Pau Monné
@ 2018-10-30 17:03 ` Paul Durrant
2018-10-31 8:44 ` Roger Pau Monné
0 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-30 17:03 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 16:40
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 4/8] viridian: remove duplicate union
> types
>
> On Mon, Oct 29, 2018 at 06:02:07PM +0000, Paul Durrant wrote:
> > 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.
>
> I agree with the unification of the viridian_page_msr struct.
>
> > Also, rename 'viridian_guest_os_id' to 'viridian_guest_os_id_msr' since
> it
> > 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".
> 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.
Paul
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/8] viridian: remove duplicate union types
2018-10-30 17:03 ` Paul Durrant
@ 2018-10-31 8:44 ` Roger Pau Monné
0 siblings, 0 replies; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-31 8:44 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
On Tue, Oct 30, 2018 at 05:03:54PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne
> > Sent: 30 October 2018 16:40
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> > <jbeulich@suse.com>
> > Subject: Re: [Xen-devel] [PATCH 4/8] viridian: remove duplicate union
> > types
> >
> > On Mon, Oct 29, 2018 at 06:02:07PM +0000, Paul Durrant wrote:
> > > 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.
> >
> > I agree with the unification of the viridian_page_msr struct.
> >
> > > Also, rename 'viridian_guest_os_id' to 'viridian_guest_os_id_msr' since
> > it
> > > 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".
>
> > 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.
OK, that's fine for me.
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 5/8] viridian: separate interrupt related enlightenment implementations...
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
` (5 preceding siblings ...)
2018-10-29 18:02 ` [PATCH 4/8] viridian: remove duplicate union types Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 16:52 ` Roger Pau Monné
2018-10-29 18:02 ` [PATCH 6/8] viridian: separate time " Paul Durrant
` (2 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
...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 <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
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
diff --git a/xen/arch/x86/hvm/viridian/Makefile b/xen/arch/x86/hvm/viridian/Makefile
index 09fd0a5f3c..fca8e16e20 100644
--- a/xen/arch/x86/hvm/viridian/Makefile
+++ b/xen/arch/x86/hvm/viridian/Makefile
@@ -1 +1,2 @@
+obj-y += synic.o
obj-y += viridian.o
diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
new file mode 100644
index 0000000000..3f043f34ee
--- /dev/null
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -0,0 +1,224 @@
+/***************************************************************************
+ * 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>
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+static void dump_vp_assist(const struct vcpu *v)
+{
+ const union viridian_page_msr *va = &v->arch.hvm.viridian.vp_assist.msr;
+
+ if ( !va->fields.enabled )
+ return;
+
+ printk(XENLOG_G_INFO "%pv: VIRIDIAN VP_ASSIST_PAGE: pfn: %lx\n",
+ v, (unsigned long)va->fields.pfn);
+}
+
+static void initialize_vp_assist(struct vcpu *v)
+{
+ struct domain *d = v->domain;
+ unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
+ struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+ void *va;
+
+ ASSERT(!v->arch.hvm.viridian.vp_assist.va);
+
+ if ( !page )
+ goto fail;
+
+ if ( !get_page_type(page, PGT_writable_page) )
+ {
+ put_page(page);
+ goto fail;
+ }
+
+ va = __map_domain_page_global(page);
+ if ( !va )
+ {
+ put_page_and_type(page);
+ goto fail;
+ }
+
+ clear_page(va);
+
+ v->arch.hvm.viridian.vp_assist.va = va;
+ return;
+
+ fail:
+ gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
+ gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
+}
+
+static void teardown_vp_assist(struct vcpu *v)
+{
+ void *va = v->arch.hvm.viridian.vp_assist.va;
+ struct page_info *page;
+
+ if ( !va )
+ return;
+
+ v->arch.hvm.viridian.vp_assist.va = NULL;
+
+ page = mfn_to_page(domain_page_map_to_mfn(va));
+
+ unmap_domain_page_global(va);
+ put_page_and_type(page);
+}
+
+void viridian_apic_assist_set(struct vcpu *v)
+{
+ uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+
+ if ( !va )
+ return;
+
+ /*
+ * If there is already an assist pending then something has gone
+ * wrong and the VM will most likely hang so force a crash now
+ * to make the problem clear.
+ */
+ if ( v->arch.hvm.viridian.vp_assist.pending )
+ domain_crash(v->domain);
+
+ v->arch.hvm.viridian.vp_assist.pending = true;
+ *va |= 1u;
+}
+
+bool viridian_apic_assist_completed(struct vcpu *v)
+{
+ uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+
+ if ( !va )
+ return false;
+
+ if ( v->arch.hvm.viridian.vp_assist.pending &&
+ !(*va & 1u) )
+ {
+ /* An EOI has been avoided */
+ v->arch.hvm.viridian.vp_assist.pending = false;
+ return true;
+ }
+
+ return false;
+}
+
+void viridian_apic_assist_clear(struct vcpu *v)
+{
+ uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+
+ if ( !va )
+ return;
+
+ *va &= ~1u;
+ v->arch.hvm.viridian.vp_assist.pending = false;
+}
+
+int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
+{
+ switch ( idx )
+ {
+ case HV_X64_MSR_EOI:
+ vlapic_EOI_set(vcpu_vlapic(v));
+ break;
+
+ case HV_X64_MSR_ICR: {
+ u32 eax = (u32)val, edx = (u32)(val >> 32);
+ struct vlapic *vlapic = vcpu_vlapic(v);
+ eax &= ~(1 << 12);
+ edx &= 0xff000000;
+ vlapic_set_reg(vlapic, APIC_ICR2, edx);
+ vlapic_ipi(vlapic, eax, edx);
+ vlapic_set_reg(vlapic, APIC_ICR, eax);
+ break;
+ }
+ case HV_X64_MSR_TPR:
+ vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI, (uint8_t)val);
+ break;
+
+ case HV_X64_MSR_VP_ASSIST_PAGE:
+ teardown_vp_assist(v); /* release any previous mapping */
+ v->arch.hvm.viridian.vp_assist.msr.raw = val;
+ dump_vp_assist(v);
+ if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled )
+ initialize_vp_assist(v);
+ break;
+
+ default:
+ gdprintk(XENLOG_INFO,
+ "%s: unimplemented MSR %#x (%016"PRIx64")\n",
+ __func__, idx, val);
+ return X86EMUL_EXCEPTION;
+ }
+
+ return X86EMUL_OKAY;
+}
+
+int viridian_synic_rdmsr(const struct vcpu *v, uint32_t idx, uint64_t *val)
+{
+ switch ( idx )
+ {
+ case HV_X64_MSR_EOI:
+ return X86EMUL_EXCEPTION;
+
+ case HV_X64_MSR_ICR:
+ {
+ uint32_t icr2 = vlapic_get_reg(vcpu_vlapic(v), APIC_ICR2);
+ uint32_t icr = vlapic_get_reg(vcpu_vlapic(v), APIC_ICR);
+
+ *val = ((uint64_t)icr2 << 32) | icr;
+ break;
+ }
+ case HV_X64_MSR_TPR:
+ *val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
+ break;
+
+ case HV_X64_MSR_VP_ASSIST_PAGE:
+ *val = v->arch.hvm.viridian.vp_assist.msr.raw;
+ break;
+
+ default:
+ gdprintk(XENLOG_INFO,
+ "%s: unimplemented MSR %#x\n",
+ __func__, idx);
+ return X86EMUL_EXCEPTION;
+ }
+
+ return X86EMUL_OKAY;
+}
+
+void viridian_synic_save_vcpu_ctxt(const struct vcpu *v,
+ struct hvm_viridian_vcpu_context *ctxt)
+{
+ ctxt->vp_assist_pending = v->arch.hvm.viridian.vp_assist.pending;
+ ctxt->vp_assist_msr = v->arch.hvm.viridian.vp_assist.msr.raw;
+}
+
+void viridian_synic_load_vcpu_ctxt(struct vcpu *v,
+ struct hvm_viridian_vcpu_context *ctxt)
+{
+ v->arch.hvm.viridian.vp_assist.msr.raw = ctxt->vp_assist_msr;
+ if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled )
+ initialize_vp_assist(v);
+
+ v->arch.hvm.viridian.vp_assist.pending = !!ctxt->vp_assist_pending;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 55b9071c32..3e67390707 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -17,72 +17,6 @@
#include <public/sched.h>
#include <public/hvm/hvm_op.h>
-/* Viridian MSR numbers. */
-#define HV_X64_MSR_GUEST_OS_ID 0x40000000
-#define HV_X64_MSR_HYPERCALL 0x40000001
-#define HV_X64_MSR_VP_INDEX 0x40000002
-#define HV_X64_MSR_RESET 0x40000003
-#define HV_X64_MSR_VP_RUNTIME 0x40000010
-#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
-#define HV_X64_MSR_REFERENCE_TSC 0x40000021
-#define HV_X64_MSR_TSC_FREQUENCY 0x40000022
-#define HV_X64_MSR_APIC_FREQUENCY 0x40000023
-#define HV_X64_MSR_EOI 0x40000070
-#define HV_X64_MSR_ICR 0x40000071
-#define HV_X64_MSR_TPR 0x40000072
-#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073
-#define HV_X64_MSR_SCONTROL 0x40000080
-#define HV_X64_MSR_SVERSION 0x40000081
-#define HV_X64_MSR_SIEFP 0x40000082
-#define HV_X64_MSR_SIMP 0x40000083
-#define HV_X64_MSR_EOM 0x40000084
-#define HV_X64_MSR_SINT0 0x40000090
-#define HV_X64_MSR_SINT1 0x40000091
-#define HV_X64_MSR_SINT2 0x40000092
-#define HV_X64_MSR_SINT3 0x40000093
-#define HV_X64_MSR_SINT4 0x40000094
-#define HV_X64_MSR_SINT5 0x40000095
-#define HV_X64_MSR_SINT6 0x40000096
-#define HV_X64_MSR_SINT7 0x40000097
-#define HV_X64_MSR_SINT8 0x40000098
-#define HV_X64_MSR_SINT9 0x40000099
-#define HV_X64_MSR_SINT10 0x4000009A
-#define HV_X64_MSR_SINT11 0x4000009B
-#define HV_X64_MSR_SINT12 0x4000009C
-#define HV_X64_MSR_SINT13 0x4000009D
-#define HV_X64_MSR_SINT14 0x4000009E
-#define HV_X64_MSR_SINT15 0x4000009F
-#define HV_X64_MSR_STIMER0_CONFIG 0x400000B0
-#define HV_X64_MSR_STIMER0_COUNT 0x400000B1
-#define HV_X64_MSR_STIMER1_CONFIG 0x400000B2
-#define HV_X64_MSR_STIMER1_COUNT 0x400000B3
-#define HV_X64_MSR_STIMER2_CONFIG 0x400000B4
-#define HV_X64_MSR_STIMER2_COUNT 0x400000B5
-#define HV_X64_MSR_STIMER3_CONFIG 0x400000B6
-#define HV_X64_MSR_STIMER3_COUNT 0x400000B7
-#define HV_X64_MSR_POWER_STATE_TRIGGER_C1 0x400000C1
-#define HV_X64_MSR_POWER_STATE_TRIGGER_C2 0x400000C2
-#define HV_X64_MSR_POWER_STATE_TRIGGER_C3 0x400000C3
-#define HV_X64_MSR_POWER_STATE_CONFIG_C1 0x400000D1
-#define HV_X64_MSR_POWER_STATE_CONFIG_C2 0x400000D2
-#define HV_X64_MSR_POWER_STATE_CONFIG_C3 0x400000D3
-#define HV_X64_MSR_STATS_PARTITION_RETAIL_PAGE 0x400000E0
-#define HV_X64_MSR_STATS_PARTITION_INTERNAL_PAGE 0x400000E1
-#define HV_X64_MSR_STATS_VP_RETAIL_PAGE 0x400000E2
-#define HV_X64_MSR_STATS_VP_INTERNAL_PAGE 0x400000E3
-#define HV_X64_MSR_GUEST_IDLE 0x400000F0
-#define HV_X64_MSR_SYNTH_DEBUG_CONTROL 0x400000F1
-#define HV_X64_MSR_SYNTH_DEBUG_STATUS 0x400000F2
-#define HV_X64_MSR_SYNTH_DEBUG_SEND_BUFFER 0x400000F3
-#define HV_X64_MSR_SYNTH_DEBUG_RECEIVE_BUFFER 0x400000F4
-#define HV_X64_MSR_SYNTH_DEBUG_PENDING_BUFFER 0x400000F5
-#define HV_X64_MSR_CRASH_P0 0x40000100
-#define HV_X64_MSR_CRASH_P1 0x40000101
-#define HV_X64_MSR_CRASH_P2 0x40000102
-#define HV_X64_MSR_CRASH_P3 0x40000103
-#define HV_X64_MSR_CRASH_P4 0x40000104
-#define HV_X64_MSR_CRASH_CTL 0x40000105
-
/* Viridian Hypercall Status Codes. */
#define HV_STATUS_SUCCESS 0x0000
#define HV_STATUS_INVALID_HYPERCALL_CODE 0x0002
@@ -311,16 +245,6 @@ static void dump_hypercall(const struct domain *d)
hg->fields.enabled, (unsigned long)hg->fields.pfn);
}
-static void dump_vp_assist(const struct vcpu *v)
-{
- const union viridian_page_msr *va;
-
- va = &v->arch.hvm.viridian.vp_assist.msr;
-
- printk(XENLOG_G_INFO "%pv: VIRIDIAN VP_ASSIST_PAGE: enabled: %x pfn: %lx\n",
- v, va->fields.enabled, (unsigned long)va->fields.pfn);
-}
-
static void dump_reference_tsc(const struct domain *d)
{
const union viridian_page_msr *rt;
@@ -366,105 +290,6 @@ static void enable_hypercall_page(struct domain *d)
put_page_and_type(page);
}
-static void initialize_vp_assist(struct vcpu *v)
-{
- struct domain *d = v->domain;
- unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
- struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
- void *va;
-
- ASSERT(!v->arch.hvm.viridian.vp_assist.va);
-
- if ( !page )
- goto fail;
-
- if ( !get_page_type(page, PGT_writable_page) )
- {
- put_page(page);
- goto fail;
- }
-
- va = __map_domain_page_global(page);
- if ( !va )
- {
- put_page_and_type(page);
- goto fail;
- }
-
- clear_page(va);
-
- v->arch.hvm.viridian.vp_assist.va = va;
- return;
-
- fail:
- gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
- gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
-}
-
-static void teardown_vp_assist(struct vcpu *v)
-{
- void *va = v->arch.hvm.viridian.vp_assist.va;
- struct page_info *page;
-
- if ( !va )
- return;
-
- v->arch.hvm.viridian.vp_assist.va = NULL;
-
- page = mfn_to_page(domain_page_map_to_mfn(va));
-
- unmap_domain_page_global(va);
- put_page_and_type(page);
-}
-
-void viridian_apic_assist_set(struct vcpu *v)
-{
- uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
-
- if ( !va )
- return;
-
- /*
- * If there is already an assist pending then something has gone
- * wrong and the VM will most likely hang so force a crash now
- * to make the problem clear.
- */
- if ( v->arch.hvm.viridian.vp_assist.pending )
- domain_crash(v->domain);
-
- v->arch.hvm.viridian.vp_assist.pending = true;
- *va |= 1u;
-}
-
-bool viridian_apic_assist_completed(struct vcpu *v)
-{
- uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
-
- if ( !va )
- return false;
-
- if ( v->arch.hvm.viridian.vp_assist.pending &&
- !(*va & 1u) )
- {
- /* An EOI has been avoided */
- v->arch.hvm.viridian.vp_assist.pending = false;
- return true;
- }
-
- return false;
-}
-
-void viridian_apic_assist_clear(struct vcpu *v)
-{
- uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
-
- if ( !va )
- return;
-
- *va &= ~1u;
- v->arch.hvm.viridian.vp_assist.pending = false;
-}
-
static void update_reference_tsc(struct domain *d, bool_t initialize)
{
unsigned long gmfn = d->arch.hvm.viridian.reference_tsc.fields.pfn;
@@ -563,31 +388,10 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
break;
case HV_X64_MSR_EOI:
- vlapic_EOI_set(vcpu_vlapic(v));
- break;
-
- case HV_X64_MSR_ICR: {
- u32 eax = (u32)val, edx = (u32)(val >> 32);
- struct vlapic *vlapic = vcpu_vlapic(v);
- eax &= ~(1 << 12);
- edx &= 0xff000000;
- vlapic_set_reg(vlapic, APIC_ICR2, edx);
- vlapic_ipi(vlapic, eax, edx);
- vlapic_set_reg(vlapic, APIC_ICR, eax);
- break;
- }
-
+ case HV_X64_MSR_ICR:
case HV_X64_MSR_TPR:
- vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI, (uint8_t)val);
- break;
-
case HV_X64_MSR_VP_ASSIST_PAGE:
- teardown_vp_assist(v); /* release any previous mapping */
- v->arch.hvm.viridian.vp_assist.msr.raw = val;
- dump_vp_assist(v);
- if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled )
- initialize_vp_assist(v);
- break;
+ return viridian_synic_wrmsr(v, idx, val);
case HV_X64_MSR_REFERENCE_TSC:
if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
@@ -710,18 +514,11 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
*val = 1000000000ull / APIC_BUS_CYCLE_NS;
break;
+ case HV_X64_MSR_EOI:
case HV_X64_MSR_ICR:
- *val = (((uint64_t)vlapic_get_reg(vcpu_vlapic(v), APIC_ICR2) << 32) |
- vlapic_get_reg(vcpu_vlapic(v), APIC_ICR));
- break;
-
case HV_X64_MSR_TPR:
- *val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
- break;
-
case HV_X64_MSR_VP_ASSIST_PAGE:
- *val = v->arch.hvm.viridian.vp_assist.msr.raw;
- break;
+ return viridian_synic_rdmsr(v, idx, val);
case HV_X64_MSR_REFERENCE_TSC:
if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
@@ -779,7 +576,7 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
void viridian_vcpu_deinit(struct vcpu *v)
{
- teardown_vp_assist(v);
+ viridian_synic_wrmsr(v, HV_X64_MSR_VP_ASSIST_PAGE, 0);
}
void viridian_domain_deinit(struct domain *d)
@@ -787,7 +584,7 @@ void viridian_domain_deinit(struct domain *d)
struct vcpu *v;
for_each_vcpu ( d, v )
- teardown_vp_assist(v);
+ viridian_vcpu_deinit(v);
}
static DEFINE_PER_CPU(cpumask_t, ipi_cpumask);
@@ -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);
}
@@ -1020,12 +816,7 @@ static int viridian_load_vcpu_ctxt(struct domain *d,
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);
return 0;
}
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 359fdf5a83..3483aedb9a 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -9,6 +9,74 @@
#ifndef __ASM_X86_HVM_VIRIDIAN_H__
#define __ASM_X86_HVM_VIRIDIAN_H__
+#include <asm/hvm/save.h>
+
+/* Viridian MSR numbers. */
+#define HV_X64_MSR_GUEST_OS_ID 0x40000000
+#define HV_X64_MSR_HYPERCALL 0x40000001
+#define HV_X64_MSR_VP_INDEX 0x40000002
+#define HV_X64_MSR_RESET 0x40000003
+#define HV_X64_MSR_VP_RUNTIME 0x40000010
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+#define HV_X64_MSR_REFERENCE_TSC 0x40000021
+#define HV_X64_MSR_TSC_FREQUENCY 0x40000022
+#define HV_X64_MSR_APIC_FREQUENCY 0x40000023
+#define HV_X64_MSR_EOI 0x40000070
+#define HV_X64_MSR_ICR 0x40000071
+#define HV_X64_MSR_TPR 0x40000072
+#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073
+#define HV_X64_MSR_SCONTROL 0x40000080
+#define HV_X64_MSR_SVERSION 0x40000081
+#define HV_X64_MSR_SIEFP 0x40000082
+#define HV_X64_MSR_SIMP 0x40000083
+#define HV_X64_MSR_EOM 0x40000084
+#define HV_X64_MSR_SINT0 0x40000090
+#define HV_X64_MSR_SINT1 0x40000091
+#define HV_X64_MSR_SINT2 0x40000092
+#define HV_X64_MSR_SINT3 0x40000093
+#define HV_X64_MSR_SINT4 0x40000094
+#define HV_X64_MSR_SINT5 0x40000095
+#define HV_X64_MSR_SINT6 0x40000096
+#define HV_X64_MSR_SINT7 0x40000097
+#define HV_X64_MSR_SINT8 0x40000098
+#define HV_X64_MSR_SINT9 0x40000099
+#define HV_X64_MSR_SINT10 0x4000009A
+#define HV_X64_MSR_SINT11 0x4000009B
+#define HV_X64_MSR_SINT12 0x4000009C
+#define HV_X64_MSR_SINT13 0x4000009D
+#define HV_X64_MSR_SINT14 0x4000009E
+#define HV_X64_MSR_SINT15 0x4000009F
+#define HV_X64_MSR_STIMER0_CONFIG 0x400000B0
+#define HV_X64_MSR_STIMER0_COUNT 0x400000B1
+#define HV_X64_MSR_STIMER1_CONFIG 0x400000B2
+#define HV_X64_MSR_STIMER1_COUNT 0x400000B3
+#define HV_X64_MSR_STIMER2_CONFIG 0x400000B4
+#define HV_X64_MSR_STIMER2_COUNT 0x400000B5
+#define HV_X64_MSR_STIMER3_CONFIG 0x400000B6
+#define HV_X64_MSR_STIMER3_COUNT 0x400000B7
+#define HV_X64_MSR_POWER_STATE_TRIGGER_C1 0x400000C1
+#define HV_X64_MSR_POWER_STATE_TRIGGER_C2 0x400000C2
+#define HV_X64_MSR_POWER_STATE_TRIGGER_C3 0x400000C3
+#define HV_X64_MSR_POWER_STATE_CONFIG_C1 0x400000D1
+#define HV_X64_MSR_POWER_STATE_CONFIG_C2 0x400000D2
+#define HV_X64_MSR_POWER_STATE_CONFIG_C3 0x400000D3
+#define HV_X64_MSR_STATS_PARTITION_RETAIL_PAGE 0x400000E0
+#define HV_X64_MSR_STATS_PARTITION_INTERNAL_PAGE 0x400000E1
+#define HV_X64_MSR_STATS_VP_RETAIL_PAGE 0x400000E2
+#define HV_X64_MSR_STATS_VP_INTERNAL_PAGE 0x400000E3
+#define HV_X64_MSR_GUEST_IDLE 0x400000F0
+#define HV_X64_MSR_SYNTH_DEBUG_CONTROL 0x400000F1
+#define HV_X64_MSR_SYNTH_DEBUG_STATUS 0x400000F2
+#define HV_X64_MSR_SYNTH_DEBUG_SEND_BUFFER 0x400000F3
+#define HV_X64_MSR_SYNTH_DEBUG_RECEIVE_BUFFER 0x400000F4
+#define HV_X64_MSR_SYNTH_DEBUG_PENDING_BUFFER 0x400000F5
+#define HV_X64_MSR_CRASH_P0 0x40000100
+#define HV_X64_MSR_CRASH_P1 0x40000101
+#define HV_X64_MSR_CRASH_P2 0x40000102
+#define HV_X64_MSR_CRASH_P3 0x40000103
+#define HV_X64_MSR_CRASH_P4 0x40000104
+#define HV_X64_MSR_CRASH_CTL 0x40000105
+
union viridian_page_msr
{
uint64_t raw;
@@ -93,6 +161,14 @@ void viridian_apic_assist_set(struct vcpu *v);
bool viridian_apic_assist_completed(struct vcpu *v);
void viridian_apic_assist_clear(struct vcpu *v);
+int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val);
+int viridian_synic_rdmsr(const struct vcpu *v, uint32_t idx, uint64_t *val);
+
+void viridian_synic_save_vcpu_ctxt(const struct vcpu *v,
+ struct hvm_viridian_vcpu_context *ctxt);
+void viridian_synic_load_vcpu_ctxt(struct vcpu *v,
+ struct hvm_viridian_vcpu_context *ctxt);
+
#endif /* __ASM_X86_HVM_VIRIDIAN_H__ */
/*
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 5/8] viridian: separate interrupt related enlightenment implementations...
2018-10-29 18:02 ` [PATCH 5/8] viridian: separate interrupt related enlightenment implementations Paul Durrant
@ 2018-10-30 16:52 ` Roger Pau Monné
2018-10-30 17:06 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-30 16:52 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Mon, Oct 29, 2018 at 06:02:08PM +0000, Paul Durrant wrote:
> ...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 <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> 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
>
> diff --git a/xen/arch/x86/hvm/viridian/Makefile b/xen/arch/x86/hvm/viridian/Makefile
> index 09fd0a5f3c..fca8e16e20 100644
> --- a/xen/arch/x86/hvm/viridian/Makefile
> +++ b/xen/arch/x86/hvm/viridian/Makefile
> @@ -1 +1,2 @@
> +obj-y += synic.o
> obj-y += viridian.o
> diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
> new file mode 100644
> index 0000000000..3f043f34ee
> --- /dev/null
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -0,0 +1,224 @@
> +/***************************************************************************
> + * 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.
> @@ -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);
> }
>
> @@ -1020,12 +816,7 @@ static int viridian_load_vcpu_ctxt(struct domain *d,
> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 5/8] viridian: separate interrupt related enlightenment implementations...
2018-10-30 16:52 ` Roger Pau Monné
@ 2018-10-30 17:06 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-30 17:06 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 16:52
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 5/8] viridian: separate interrupt related
> enlightenment implementations...
>
> On Mon, Oct 29, 2018 at 06:02:08PM +0000, Paul Durrant wrote:
> > ...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%20Specif
> ication%20v5.0C.pdf
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> > 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
> >
> > diff --git a/xen/arch/x86/hvm/viridian/Makefile
> b/xen/arch/x86/hvm/viridian/Makefile
> > index 09fd0a5f3c..fca8e16e20 100644
> > --- a/xen/arch/x86/hvm/viridian/Makefile
> > +++ b/xen/arch/x86/hvm/viridian/Makefile
> > @@ -1 +1,2 @@
> > +obj-y += synic.o
> > obj-y += viridian.o
> > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> b/xen/arch/x86/hvm/viridian/synic.c
> > new file mode 100644
> > index 0000000000..3f043f34ee
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > @@ -0,0 +1,224 @@
> >
> +/************************************************************************
> ***
> > + * 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.
>
Sure. It'll look better.
> > @@ -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);
> > }
> >
> > @@ -1020,12 +816,7 @@ static int viridian_load_vcpu_ctxt(struct domain
> *d,
> > 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?
>
There is per-domain time related save/restore, but synic stuff is the only per-cpu data. I'd prefer to keep the wrapper though as there may be other non-synic related data... but I don't have any in the pipeline at the moment.
Paul
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 6/8] viridian: separate time related enlightenment implementations...
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
` (6 preceding siblings ...)
2018-10-29 18:02 ` [PATCH 5/8] viridian: separate interrupt related enlightenment implementations Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 17:03 ` Roger Pau Monné
2018-10-29 18:02 ` [PATCH 7/8] viridian: define type for the 'virtual VP assist page' Paul Durrant
2018-10-29 18:02 ` [PATCH 8/8] viridian: introduce struct viridian_page Paul Durrant
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
...into new 'time' module.
This patch reduces the size of the main viridian source module by
moving time related enlightenments into their own source module. This is
done in anticipation of implementation of more such enightenments and
a desire to not further lengthen the main source module when this work
is done.
While moving the code:
- Move the declaration of HV_REFERENCE_TSC_PAGE from the header file into
the new source module, since it is only used there.
- Clean up a bool_t.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/x86/hvm/viridian/Makefile | 1 +
xen/arch/x86/hvm/viridian/time.c | 244 +++++++++++++++++++++++++++++++++++
xen/arch/x86/hvm/viridian/viridian.c | 174 +------------------------
xen/include/asm-x86/hvm/viridian.h | 17 ++-
4 files changed, 260 insertions(+), 176 deletions(-)
create mode 100644 xen/arch/x86/hvm/viridian/time.c
diff --git a/xen/arch/x86/hvm/viridian/Makefile b/xen/arch/x86/hvm/viridian/Makefile
index fca8e16e20..3ecdffe2f6 100644
--- a/xen/arch/x86/hvm/viridian/Makefile
+++ b/xen/arch/x86/hvm/viridian/Makefile
@@ -1,2 +1,3 @@
obj-y += synic.o
+obj-y += time.o
obj-y += viridian.o
diff --git a/xen/arch/x86/hvm/viridian/time.c b/xen/arch/x86/hvm/viridian/time.c
new file mode 100644
index 0000000000..aa32e93f9f
--- /dev/null
+++ b/xen/arch/x86/hvm/viridian/time.c
@@ -0,0 +1,244 @@
+/***************************************************************************
+ * time.c
+ *
+ * An implementation of some time 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>
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+
+typedef struct _HV_REFERENCE_TSC_PAGE
+{
+ uint32_t TscSequence;
+ uint32_t Reserved1;
+ uint64_t TscScale;
+ int64_t TscOffset;
+ uint64_t Reserved2[509];
+} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
+
+static void dump_reference_tsc(const struct domain *d)
+{
+ const union viridian_page_msr *rt = &d->arch.hvm.viridian.reference_tsc;
+
+ if ( !rt->fields.enabled )
+ return;
+
+ printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: pfn: %lx\n",
+ d->domain_id, (unsigned long)rt->fields.pfn);
+}
+
+static void update_reference_tsc(struct domain *d, bool initialize)
+{
+ unsigned long gmfn = d->arch.hvm.viridian.reference_tsc.fields.pfn;
+ struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+ HV_REFERENCE_TSC_PAGE *p;
+
+ if ( !page || !get_page_type(page, PGT_writable_page) )
+ {
+ if ( page )
+ put_page(page);
+ gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
+ gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
+ return;
+ }
+
+ p = __map_domain_page(page);
+
+ if ( initialize )
+ clear_page(p);
+
+ /*
+ * This enlightenment must be disabled is the host TSC is not invariant.
+ * However it is also disabled if vtsc is true (which means rdtsc is
+ * being emulated). This generally happens when guest TSC freq and host
+ * TSC freq don't match. The TscScale value could be adjusted to cope
+ * with this, allowing vtsc to be turned off, but support for this is
+ * not yet present in the hypervisor. Thus is it is possible that
+ * migrating a Windows VM between hosts of differing TSC frequencies
+ * may result in large differences in guest performance.
+ */
+ if ( !host_tsc_is_safe() || d->arch.vtsc )
+ {
+ /*
+ * The specification states that valid values of TscSequence range
+ * from 0 to 0xFFFFFFFE. The value 0xFFFFFFFF is used to indicate
+ * this mechanism is no longer a reliable source of time and that
+ * the VM should fall back to a different source.
+ *
+ * Server 2012 (6.2 kernel) and 2012 R2 (6.3 kernel) actually
+ * violate the spec. and rely on a value of 0 to indicate that this
+ * enlightenment should no longer be used.
+ */
+ p->TscSequence = 0;
+
+ printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: invalidated\n",
+ d->domain_id);
+ goto out;
+ }
+
+ /*
+ * The guest will calculate reference time according to the following
+ * formula:
+ *
+ * ReferenceTime = ((RDTSC() * TscScale) >> 64) + TscOffset
+ *
+ * Windows uses a 100ns tick, so we need a scale which is cpu
+ * ticks per 100ns shifted left by 64.
+ */
+ p->TscScale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
+
+ p->TscSequence++;
+ if ( p->TscSequence == 0xFFFFFFFF ||
+ p->TscSequence == 0 ) /* Avoid both 'invalid' values */
+ p->TscSequence = 1;
+
+ out:
+ unmap_domain_page(p);
+
+ put_page_and_type(page);
+}
+
+static int64_t raw_trc_val(struct domain *d)
+{
+ uint64_t tsc;
+ struct time_scale tsc_to_ns;
+
+ tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
+
+ /* convert tsc to count of 100ns periods */
+ set_time_scale(&tsc_to_ns, d->arch.tsc_khz * 1000ul);
+ return scale_delta(tsc, &tsc_to_ns) / 100ul;
+}
+
+void viridian_time_ref_count_freeze(struct domain *d)
+{
+ struct viridian_time_ref_count *trc;
+
+ trc = &d->arch.hvm.viridian.time_ref_count;
+
+ if ( test_and_clear_bit(_TRC_running, &trc->flags) )
+ trc->val = raw_trc_val(d) + trc->off;
+}
+
+void viridian_time_ref_count_thaw(struct domain *d)
+{
+ struct viridian_time_ref_count *trc;
+
+ trc = &d->arch.hvm.viridian.time_ref_count;
+
+ if ( !d->is_shutting_down &&
+ !test_and_set_bit(_TRC_running, &trc->flags) )
+ trc->off = (int64_t)trc->val - raw_trc_val(d);
+}
+
+int viridian_time_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
+{
+ struct domain *d = v->domain;
+
+ switch ( idx )
+ {
+ case HV_X64_MSR_REFERENCE_TSC:
+ if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
+ return X86EMUL_EXCEPTION;
+
+ d->arch.hvm.viridian.reference_tsc.raw = val;
+ dump_reference_tsc(d);
+ if ( d->arch.hvm.viridian.reference_tsc.fields.enabled )
+ update_reference_tsc(d, true);
+ break;
+
+ default:
+ gdprintk(XENLOG_INFO,
+ "%s: unimplemented MSR %#x (%016"PRIx64")\n",
+ __func__, idx, val);
+ return X86EMUL_EXCEPTION;
+ }
+
+ return X86EMUL_OKAY;
+}
+
+int viridian_time_rdmsr(const struct vcpu *v, uint32_t idx, uint64_t *val)
+{
+ struct domain *d = v->domain;
+
+ switch ( idx )
+ {
+ case HV_X64_MSR_TSC_FREQUENCY:
+ if ( viridian_feature_mask(d) & HVMPV_no_freq )
+ return X86EMUL_EXCEPTION;
+
+ *val = (uint64_t)d->arch.tsc_khz * 1000ull;
+ break;
+
+ case HV_X64_MSR_APIC_FREQUENCY:
+ if ( viridian_feature_mask(d) & HVMPV_no_freq )
+ return X86EMUL_EXCEPTION;
+
+ *val = 1000000000ull / APIC_BUS_CYCLE_NS;
+ break;
+
+ case HV_X64_MSR_REFERENCE_TSC:
+ if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
+ return X86EMUL_EXCEPTION;
+
+ *val = d->arch.hvm.viridian.reference_tsc.raw;
+ break;
+
+ case HV_X64_MSR_TIME_REF_COUNT:
+ {
+ struct viridian_time_ref_count *trc =
+ &d->arch.hvm.viridian.time_ref_count;
+
+ if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
+ return X86EMUL_EXCEPTION;
+
+ if ( !test_and_set_bit(_TRC_accessed, &trc->flags) )
+ printk(XENLOG_G_INFO "d%d: VIRIDIAN MSR_TIME_REF_COUNT: accessed\n",
+ d->domain_id);
+
+ *val = raw_trc_val(d) + trc->off;
+ break;
+ }
+
+ default:
+ gdprintk(XENLOG_INFO,
+ "%s: unimplemented MSR %#x\n",
+ __func__, idx);
+ return X86EMUL_EXCEPTION;
+ }
+
+ return X86EMUL_OKAY;
+}
+
+void viridian_time_save_domain_ctxt(
+ const struct domain *d, struct hvm_viridian_domain_context *ctxt)
+{
+ ctxt->time_ref_count = d->arch.hvm.viridian.time_ref_count.val;
+ ctxt->reference_tsc = d->arch.hvm.viridian.reference_tsc.raw;
+}
+
+void viridian_time_load_domain_ctxt(
+ struct domain *d, struct hvm_viridian_domain_context *ctxt)
+{
+ d->arch.hvm.viridian.time_ref_count.val = ctxt->time_ref_count;
+ d->arch.hvm.viridian.reference_tsc.raw = ctxt->reference_tsc;
+
+ if ( d->arch.hvm.viridian.reference_tsc.fields.enabled )
+ update_reference_tsc(d, false);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 3e67390707..3635370528 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -245,17 +245,6 @@ static void dump_hypercall(const struct domain *d)
hg->fields.enabled, (unsigned long)hg->fields.pfn);
}
-static void dump_reference_tsc(const struct domain *d)
-{
- const union viridian_page_msr *rt;
-
- rt = &d->arch.hvm.viridian.reference_tsc;
-
- printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: enabled: %x pfn: %lx\n",
- d->domain_id,
- rt->fields.enabled, (unsigned long)rt->fields.pfn);
-}
-
static void enable_hypercall_page(struct domain *d)
{
unsigned long gmfn = d->arch.hvm.viridian.hypercall_gpa.fields.pfn;
@@ -290,80 +279,6 @@ static void enable_hypercall_page(struct domain *d)
put_page_and_type(page);
}
-static void update_reference_tsc(struct domain *d, bool_t initialize)
-{
- unsigned long gmfn = d->arch.hvm.viridian.reference_tsc.fields.pfn;
- struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
- HV_REFERENCE_TSC_PAGE *p;
-
- if ( !page || !get_page_type(page, PGT_writable_page) )
- {
- if ( page )
- put_page(page);
- gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
- gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
- return;
- }
-
- p = __map_domain_page(page);
-
- if ( initialize )
- clear_page(p);
-
- /*
- * This enlightenment must be disabled is the host TSC is not
- * invariant. However it is also disabled if vtsc is true (which
- * means rdtsc is being emulated). This generally happens when guest
- * TSC freq and host TSC freq don't match. The TscScale value could be
- * adjusted to cope with this, allowing vtsc to be turned off, but
- * support for this is not yet present in the hypervisor. Thus is it
- * is possible that migrating a Windows VM between hosts of differing
- * TSC frequencies may result in large differences in guest
- * performance.
- */
- if ( !host_tsc_is_safe() || d->arch.vtsc )
- {
- /*
- * The specification states that valid values of TscSequence range
- * from 0 to 0xFFFFFFFE. The value 0xFFFFFFFF is used to indicate
- * this mechanism is no longer a reliable source of time and that
- * the VM should fall back to a different source.
- *
- * Server 2012 (6.2 kernel) and 2012 R2 (6.3 kernel) actually
- * violate the specification and rely on a value of 0 to indicate
- * that this enlightenment should no longer be used. These two
- * kernel versions are currently the only ones to make use of this
- * enlightenment, so just use 0 here.
- */
- p->TscSequence = 0;
-
- printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: invalidated\n",
- d->domain_id);
- goto out;
- }
-
- /*
- * The guest will calculate reference time according to the following
- * formula:
- *
- * ReferenceTime = ((RDTSC() * TscScale) >> 64) + TscOffset
- *
- * Windows uses a 100ns tick, so we need a scale which is cpu
- * ticks per 100ns shifted left by 64.
- */
- p->TscScale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
-
- p->TscSequence++;
- if ( p->TscSequence == 0xFFFFFFFF ||
- p->TscSequence == 0 ) /* Avoid both 'invalid' values */
- p->TscSequence = 1;
-
- out:
- unmap_domain_page(p);
-
- put_page_and_type(page);
-}
-
int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
{
struct domain *d = v->domain;
@@ -394,14 +309,7 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
return viridian_synic_wrmsr(v, idx, val);
case HV_X64_MSR_REFERENCE_TSC:
- if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
- return X86EMUL_EXCEPTION;
-
- d->arch.hvm.viridian.reference_tsc.raw = val;
- dump_reference_tsc(d);
- if ( d->arch.hvm.viridian.reference_tsc.fields.enabled )
- update_reference_tsc(d, 1);
- break;
+ return viridian_time_wrmsr(v, idx, val);
case HV_X64_MSR_CRASH_P0:
case HV_X64_MSR_CRASH_P1:
@@ -447,39 +355,6 @@ int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
return X86EMUL_OKAY;
}
-static int64_t raw_trc_val(struct domain *d)
-{
- uint64_t tsc;
- struct time_scale tsc_to_ns;
-
- tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
-
- /* convert tsc to count of 100ns periods */
- set_time_scale(&tsc_to_ns, d->arch.tsc_khz * 1000ul);
- return scale_delta(tsc, &tsc_to_ns) / 100ul;
-}
-
-void viridian_time_ref_count_freeze(struct domain *d)
-{
- struct viridian_time_ref_count *trc;
-
- trc = &d->arch.hvm.viridian.time_ref_count;
-
- if ( test_and_clear_bit(_TRC_running, &trc->flags) )
- trc->val = raw_trc_val(d) + trc->off;
-}
-
-void viridian_time_ref_count_thaw(struct domain *d)
-{
- struct viridian_time_ref_count *trc;
-
- trc = &d->arch.hvm.viridian.time_ref_count;
-
- if ( !d->is_shutting_down &&
- !test_and_set_bit(_TRC_running, &trc->flags) )
- trc->off = (int64_t)trc->val - raw_trc_val(d);
-}
-
int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
{
struct domain *d = v->domain;
@@ -500,49 +375,17 @@ int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
*val = v->vcpu_id;
break;
- case HV_X64_MSR_TSC_FREQUENCY:
- if ( viridian_feature_mask(d) & HVMPV_no_freq )
- return X86EMUL_EXCEPTION;
-
- *val = (uint64_t)d->arch.tsc_khz * 1000ull;
- break;
-
- case HV_X64_MSR_APIC_FREQUENCY:
- if ( viridian_feature_mask(d) & HVMPV_no_freq )
- return X86EMUL_EXCEPTION;
-
- *val = 1000000000ull / APIC_BUS_CYCLE_NS;
- break;
-
case HV_X64_MSR_EOI:
case HV_X64_MSR_ICR:
case HV_X64_MSR_TPR:
case HV_X64_MSR_VP_ASSIST_PAGE:
return viridian_synic_rdmsr(v, idx, val);
+ case HV_X64_MSR_TSC_FREQUENCY:
+ case HV_X64_MSR_APIC_FREQUENCY:
case HV_X64_MSR_REFERENCE_TSC:
- if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
- return X86EMUL_EXCEPTION;
-
- *val = d->arch.hvm.viridian.reference_tsc.raw;
- break;
-
case HV_X64_MSR_TIME_REF_COUNT:
- {
- struct viridian_time_ref_count *trc;
-
- trc = &d->arch.hvm.viridian.time_ref_count;
-
- if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
- return X86EMUL_EXCEPTION;
-
- if ( !test_and_set_bit(_TRC_accessed, &trc->flags) )
- printk(XENLOG_G_INFO "d%d: VIRIDIAN MSR_TIME_REF_COUNT: accessed\n",
- d->domain_id);
-
- *val = raw_trc_val(d) + trc->off;
- break;
- }
+ return viridian_time_rdmsr(v, idx, val);
case HV_X64_MSR_CRASH_P0:
case HV_X64_MSR_CRASH_P1:
@@ -750,15 +593,15 @@ static int viridian_save_domain_ctxt(struct vcpu *v,
{
const struct domain *d = v->domain;
struct hvm_viridian_domain_context ctxt = {
- .time_ref_count = d->arch.hvm.viridian.time_ref_count.val,
.hypercall_gpa = d->arch.hvm.viridian.hypercall_gpa.raw,
.guest_os_id = d->arch.hvm.viridian.guest_os_id.raw,
- .reference_tsc = d->arch.hvm.viridian.reference_tsc.raw,
};
if ( !is_viridian_domain(d) )
return 0;
+ viridian_time_save_domain_ctxt(d, &ctxt);
+
return (hvm_save_entry(VIRIDIAN_DOMAIN, 0, h, &ctxt) != 0);
}
@@ -770,13 +613,10 @@ static int viridian_load_domain_ctxt(struct domain *d,
if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
return -EINVAL;
- d->arch.hvm.viridian.time_ref_count.val = ctxt.time_ref_count;
d->arch.hvm.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
d->arch.hvm.viridian.guest_os_id.raw = ctxt.guest_os_id;
- d->arch.hvm.viridian.reference_tsc.raw = ctxt.reference_tsc;
- if ( d->arch.hvm.viridian.reference_tsc.fields.enabled )
- update_reference_tsc(d, 0);
+ viridian_time_load_domain_ctxt(d, &ctxt);
return 0;
}
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 3483aedb9a..dec003f74c 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -125,15 +125,6 @@ struct viridian_time_ref_count
int64_t off;
};
-typedef struct _HV_REFERENCE_TSC_PAGE
-{
- uint32_t TscSequence;
- uint32_t Reserved1;
- uint64_t TscScale;
- int64_t TscOffset;
- uint64_t Reserved2[509];
-} HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
-
struct viridian_domain
{
union viridian_guest_os_id_msr guest_os_id;
@@ -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);
+
+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);
+
void viridian_vcpu_deinit(struct vcpu *v);
void viridian_domain_deinit(struct domain *d);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 6/8] viridian: separate time related enlightenment implementations...
2018-10-29 18:02 ` [PATCH 6/8] viridian: separate time " Paul Durrant
@ 2018-10-30 17:03 ` Roger Pau Monné
2018-10-30 17:08 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-30 17:03 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Mon, Oct 29, 2018 at 06:02:09PM +0000, Paul Durrant wrote:
> @@ -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);
> +
> +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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 6/8] viridian: separate time related enlightenment implementations...
2018-10-30 17:03 ` Roger Pau Monné
@ 2018-10-30 17:08 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-30 17:08 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 17:03
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 6/8] viridian: separate time related
> enlightenment implementations...
>
> On Mon, Oct 29, 2018 at 06:02:09PM +0000, Paul Durrant wrote:
> > @@ -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);
> > +
> > +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?
I guess it could be, yes.
Paul
>
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
` (7 preceding siblings ...)
2018-10-29 18:02 ` [PATCH 6/8] viridian: separate time " Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 17:08 ` Roger Pau Monné
2018-10-29 18:02 ` [PATCH 8/8] viridian: introduce struct viridian_page Paul Durrant
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich
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 <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/x86/hvm/viridian/synic.c | 52 +++++++++++++++++++++++---------------
xen/include/asm-x86/hvm/viridian.h | 2 +-
2 files changed, 33 insertions(+), 21 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
index 3f043f34ee..735c4c897c 100644
--- a/xen/arch/x86/hvm/viridian/synic.c
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -13,6 +13,18 @@
#include <asm/apic.h>
#include <asm/hvm/support.h>
+typedef struct _HV_VIRTUAL_APIC_ASSIST
+{
+ uint32_t no_eoi:1;
+ uint32_t reserved_zero:31;
+} HV_VIRTUAL_APIC_ASSIST;
+
+typedef union _HV_VP_ASSIST_PAGE
+{
+ HV_VIRTUAL_APIC_ASSIST ApicAssist;
+ uint8_t ReservedZBytePadding[PAGE_SIZE];
+} HV_VP_ASSIST_PAGE;
+
static void dump_vp_assist(const struct vcpu *v)
{
const union viridian_page_msr *va = &v->arch.hvm.viridian.vp_assist.msr;
@@ -29,9 +41,9 @@ static void initialize_vp_assist(struct vcpu *v)
struct domain *d = v->domain;
unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
- void *va;
+ HV_VP_ASSIST_PAGE *ptr;
- ASSERT(!v->arch.hvm.viridian.vp_assist.va);
+ ASSERT(!v->arch.hvm.viridian.vp_assist.ptr);
if ( !page )
goto fail;
@@ -42,16 +54,16 @@ static void initialize_vp_assist(struct vcpu *v)
goto fail;
}
- va = __map_domain_page_global(page);
- if ( !va )
+ ptr = __map_domain_page_global(page);
+ if ( !ptr )
{
put_page_and_type(page);
goto fail;
}
- clear_page(va);
+ clear_page(ptr);
- v->arch.hvm.viridian.vp_assist.va = va;
+ v->arch.hvm.viridian.vp_assist.ptr = ptr;
return;
fail:
@@ -61,25 +73,25 @@ static void initialize_vp_assist(struct vcpu *v)
static void teardown_vp_assist(struct vcpu *v)
{
- void *va = v->arch.hvm.viridian.vp_assist.va;
+ HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
struct page_info *page;
- if ( !va )
+ if ( !ptr )
return;
- v->arch.hvm.viridian.vp_assist.va = NULL;
+ v->arch.hvm.viridian.vp_assist.ptr = NULL;
- page = mfn_to_page(domain_page_map_to_mfn(va));
+ page = mfn_to_page(domain_page_map_to_mfn(ptr));
- unmap_domain_page_global(va);
+ unmap_domain_page_global(ptr);
put_page_and_type(page);
}
void viridian_apic_assist_set(struct vcpu *v)
{
- uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+ HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
- if ( !va )
+ if ( !ptr )
return;
/*
@@ -91,18 +103,18 @@ void viridian_apic_assist_set(struct vcpu *v)
domain_crash(v->domain);
v->arch.hvm.viridian.vp_assist.pending = true;
- *va |= 1u;
+ ptr->ApicAssist.no_eoi = 1;
}
bool viridian_apic_assist_completed(struct vcpu *v)
{
- uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+ HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
- if ( !va )
+ if ( !ptr )
return false;
if ( v->arch.hvm.viridian.vp_assist.pending &&
- !(*va & 1u) )
+ !ptr->ApicAssist.no_eoi )
{
/* An EOI has been avoided */
v->arch.hvm.viridian.vp_assist.pending = false;
@@ -114,12 +126,12 @@ bool viridian_apic_assist_completed(struct vcpu *v)
void viridian_apic_assist_clear(struct vcpu *v)
{
- uint32_t *va = v->arch.hvm.viridian.vp_assist.va;
+ HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
- if ( !va )
+ if ( !ptr )
return;
- *va &= ~1u;
+ ptr->ApicAssist.no_eoi = 0;
v->arch.hvm.viridian.vp_assist.pending = false;
}
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index dec003f74c..66aa24c43e 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -92,7 +92,7 @@ struct viridian_vcpu
{
struct {
union viridian_page_msr msr;
- void *va;
+ void *ptr;
bool pending;
} vp_assist;
uint64_t crash_param[5];
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-29 18:02 ` [PATCH 7/8] viridian: define type for the 'virtual VP assist page' Paul Durrant
@ 2018-10-30 17:08 ` Roger Pau Monné
2018-10-30 17:11 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-30 17:08 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
> 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 <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> xen/arch/x86/hvm/viridian/synic.c | 52 +++++++++++++++++++++++---------------
> xen/include/asm-x86/hvm/viridian.h | 2 +-
> 2 files changed, 33 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
> index 3f043f34ee..735c4c897c 100644
> --- a/xen/arch/x86/hvm/viridian/synic.c
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -13,6 +13,18 @@
> #include <asm/apic.h>
> #include <asm/hvm/support.h>
>
> +typedef struct _HV_VIRTUAL_APIC_ASSIST
> +{
> + uint32_t no_eoi:1;
Maybe bool:1 so you can use true/false?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-30 17:08 ` Roger Pau Monné
@ 2018-10-30 17:11 ` Paul Durrant
2018-10-31 8:53 ` Roger Pau Monné
0 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-30 17:11 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 17:09
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the
> 'virtual VP assist page'
>
> On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
> > 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%20Specif
> ication%20v5.0C.pdf
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> > xen/arch/x86/hvm/viridian/synic.c | 52 +++++++++++++++++++++++--------
> -------
> > xen/include/asm-x86/hvm/viridian.h | 2 +-
> > 2 files changed, 33 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> b/xen/arch/x86/hvm/viridian/synic.c
> > index 3f043f34ee..735c4c897c 100644
> > --- a/xen/arch/x86/hvm/viridian/synic.c
> > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > @@ -13,6 +13,18 @@
> > #include <asm/apic.h>
> > #include <asm/hvm/support.h>
> >
> > +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.
Paul
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-30 17:11 ` Paul Durrant
@ 2018-10-31 8:53 ` Roger Pau Monné
2018-10-31 9:27 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-31 8:53 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
On Tue, Oct 30, 2018 at 05:11:30PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne
> > Sent: 30 October 2018 17:09
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> > <jbeulich@suse.com>
> > Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the
> > 'virtual VP assist page'
> >
> > On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
> > > 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%20Specif
> > ication%20v5.0C.pdf
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > ---
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > > xen/arch/x86/hvm/viridian/synic.c | 52 +++++++++++++++++++++++--------
> > -------
> > > xen/include/asm-x86/hvm/viridian.h | 2 +-
> > > 2 files changed, 33 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> > b/xen/arch/x86/hvm/viridian/synic.c
> > > index 3f043f34ee..735c4c897c 100644
> > > --- a/xen/arch/x86/hvm/viridian/synic.c
> > > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > > @@ -13,6 +13,18 @@
> > > #include <asm/apic.h>
> > > #include <asm/hvm/support.h>
> > >
> > > +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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-31 8:53 ` Roger Pau Monné
@ 2018-10-31 9:27 ` Paul Durrant
2018-10-31 9:41 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-31 9:27 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel@lists.xenproject.org, Wei Liu, Jan Beulich,
Andrew Cooper
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 31 October 2018 08:54
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the
> 'virtual VP assist page'
>
> On Tue, Oct 30, 2018 at 05:11:30PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne
> > > Sent: 30 October 2018 17:09
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> > > <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan
> Beulich
> > > <jbeulich@suse.com>
> > > Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the
> > > 'virtual VP assist page'
> > >
> > > On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
> > > > 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%20Specif
> > > ication%20v5.0C.pdf
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > > ---
> > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > > xen/arch/x86/hvm/viridian/synic.c | 52 +++++++++++++++++++++++----
> ----
> > > -------
> > > > xen/include/asm-x86/hvm/viridian.h | 2 +-
> > > > 2 files changed, 33 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> > > b/xen/arch/x86/hvm/viridian/synic.c
> > > > index 3f043f34ee..735c4c897c 100644
> > > > --- a/xen/arch/x86/hvm/viridian/synic.c
> > > > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > > > @@ -13,6 +13,18 @@
> > > > #include <asm/apic.h>
> > > > #include <asm/hvm/support.h>
> > > >
> > > > +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.
>
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.
Paul
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-31 9:27 ` Paul Durrant
@ 2018-10-31 9:41 ` Jan Beulich
2018-10-31 10:01 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2018-10-31 9:41 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 31.10.18 at 10:27, <Paul.Durrant@citrix.com> wrote:
>> From: Roger Pau Monne
>> Sent: 31 October 2018 08:54
>>
>> On Tue, Oct 30, 2018 at 05:11:30PM +0000, Paul Durrant wrote:
>> > > From: Roger Pau Monne
>> > > Sent: 30 October 2018 17:09
>> > >
>> > > On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
>> > > > --- a/xen/arch/x86/hvm/viridian/synic.c
>> > > > +++ b/xen/arch/x86/hvm/viridian/synic.c
>> > > > @@ -13,6 +13,18 @@
>> > > > #include <asm/apic.h>
>> > > > #include <asm/hvm/support.h>
>> > > >
>> > > > +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.
>>
>
> 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.
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-31 9:41 ` Jan Beulich
@ 2018-10-31 10:01 ` Paul Durrant
2018-10-31 10:16 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-31 10:01 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 31 October 2018 09:42
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [Xen-devel] [PATCH 7/8] viridian: define type for the
> 'virtual VP assist page'
>
> >>> On 31.10.18 at 10:27, <Paul.Durrant@citrix.com> wrote:
> >> From: Roger Pau Monne
> >> Sent: 31 October 2018 08:54
> >>
> >> On Tue, Oct 30, 2018 at 05:11:30PM +0000, Paul Durrant wrote:
> >> > > From: Roger Pau Monne
> >> > > Sent: 30 October 2018 17:09
> >> > >
> >> > > On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
> >> > > > --- a/xen/arch/x86/hvm/viridian/synic.c
> >> > > > +++ b/xen/arch/x86/hvm/viridian/synic.c
> >> > > > @@ -13,6 +13,18 @@
> >> > > > #include <asm/apic.h>
> >> > > > #include <asm/hvm/support.h>
> >> > > >
> >> > > > +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.
> >>
> >
> > 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.
>
> 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;
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.
Paul
> char c;
> };
>
> unsigned test(void) {
> return offsetof(struct s, c);
> }
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
2018-10-31 10:01 ` Paul Durrant
@ 2018-10-31 10:16 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2018-10-31 10:16 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 31.10.18 at 11:01, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 31 October 2018 09:42
>>
>> >>> On 31.10.18 at 10:27, <Paul.Durrant@citrix.com> wrote:
>> >> From: Roger Pau Monne
>> >> Sent: 31 October 2018 08:54
>> >>
>> >> On Tue, Oct 30, 2018 at 05:11:30PM +0000, Paul Durrant wrote:
>> >> > > From: Roger Pau Monne
>> >> > > Sent: 30 October 2018 17:09
>> >> > >
>> >> > > On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
>> >> > > > --- a/xen/arch/x86/hvm/viridian/synic.c
>> >> > > > +++ b/xen/arch/x86/hvm/viridian/synic.c
>> >> > > > @@ -13,6 +13,18 @@
>> >> > > > #include <asm/apic.h>
>> >> > > > #include <asm/hvm/support.h>
>> >> > > >
>> >> > > > +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.
>> >>
>> >
>> > 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.
>>
>> 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;
>
> 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.
I'm not overly concerned which way the above is coded;
replacing the first uint32_t by bool would yield exactly the
same code afaict, yet make it (even more) clear that this is
a boolean field.
The approach using a union and a (non-bitfield) "raw"
member (which we use in a number of other places) has its
own benefits, but if there's no need to ever access these
values at their full width, the sticking to the non-union form
is certainly fine. And in the end you're the maintainer of this
code anyway ...
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 8/8] viridian: introduce struct viridian_page
2018-10-29 18:02 [PATCH 0/8] viridian cleanup Paul Durrant
` (8 preceding siblings ...)
2018-10-29 18:02 ` [PATCH 7/8] viridian: define type for the 'virtual VP assist page' Paul Durrant
@ 2018-10-29 18:02 ` Paul Durrant
2018-10-30 17:17 ` Roger Pau Monné
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2018-10-29 18:02 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Paul Durrant, Ian Jackson, Jan Beulich, Andrew Cooper
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 <paul.durrant@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
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(-)
diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c
index 40e77851be..a4efadf307 100644
--- a/tools/misc/xen-hvmctx.c
+++ b/tools/misc/xen-hvmctx.c
@@ -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",
+ printf(" VIRIDIAN_VCPU: vp_assist_msr 0x%llx, apic_assist_pending %s\n",
(unsigned long long) p.vp_assist_msr,
- p.vp_assist_pending ? "true" : "false");
+ p.apic_assist_pending ? "true" : "false");
}
static void dump_vmce_vcpu(void)
diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
index 735c4c897c..f9aa8fd898 100644
--- a/xen/arch/x86/hvm/viridian/synic.c
+++ b/xen/arch/x86/hvm/viridian/synic.c
@@ -36,14 +36,13 @@ static void dump_vp_assist(const struct vcpu *v)
v, (unsigned long)va->fields.pfn);
}
-static void initialize_vp_assist(struct vcpu *v)
+static void initialize_guest_page(struct vcpu *v, struct viridian_page *vp)
{
struct domain *d = v->domain;
- unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
+ unsigned long gmfn = vp->msr.fields.pfn;
struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
- HV_VP_ASSIST_PAGE *ptr;
- ASSERT(!v->arch.hvm.viridian.vp_assist.ptr);
+ ASSERT(!vp->ptr);
if ( !page )
goto fail;
@@ -54,16 +53,14 @@ static void initialize_vp_assist(struct vcpu *v)
goto fail;
}
- ptr = __map_domain_page_global(page);
- if ( !ptr )
+ vp->ptr = __map_domain_page_global(page);
+ if ( !vp->ptr )
{
put_page_and_type(page);
goto fail;
}
- clear_page(ptr);
-
- v->arch.hvm.viridian.vp_assist.ptr = ptr;
+ clear_page(vp->ptr);
return;
fail:
@@ -71,19 +68,18 @@ static void initialize_vp_assist(struct vcpu *v)
gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
}
-static void teardown_vp_assist(struct vcpu *v)
+static void teardown_guest_page(struct viridian_page *vp)
{
- HV_VP_ASSIST_PAGE *ptr = v->arch.hvm.viridian.vp_assist.ptr;
struct page_info *page;
- if ( !ptr )
+ if ( !vp->ptr )
return;
- v->arch.hvm.viridian.vp_assist.ptr = NULL;
+ page = mfn_to_page(domain_page_map_to_mfn(vp->ptr));
- page = mfn_to_page(domain_page_map_to_mfn(ptr));
+ unmap_domain_page_global(vp->ptr);
+ vp->ptr = NULL;
- unmap_domain_page_global(ptr);
put_page_and_type(page);
}
@@ -99,10 +95,10 @@ void viridian_apic_assist_set(struct vcpu *v)
* wrong and the VM will most likely hang so force a crash now
* to make the problem clear.
*/
- if ( v->arch.hvm.viridian.vp_assist.pending )
+ if ( v->arch.hvm.viridian.apic_assist_pending )
domain_crash(v->domain);
- v->arch.hvm.viridian.vp_assist.pending = true;
+ v->arch.hvm.viridian.apic_assist_pending = true;
ptr->ApicAssist.no_eoi = 1;
}
@@ -113,11 +109,11 @@ bool viridian_apic_assist_completed(struct vcpu *v)
if ( !ptr )
return false;
- if ( v->arch.hvm.viridian.vp_assist.pending &&
+ if ( v->arch.hvm.viridian.apic_assist_pending &&
!ptr->ApicAssist.no_eoi )
{
/* An EOI has been avoided */
- v->arch.hvm.viridian.vp_assist.pending = false;
+ v->arch.hvm.viridian.apic_assist_pending = false;
return true;
}
@@ -132,7 +128,7 @@ void viridian_apic_assist_clear(struct vcpu *v)
return;
ptr->ApicAssist.no_eoi = 0;
- v->arch.hvm.viridian.vp_assist.pending = false;
+ v->arch.hvm.viridian.apic_assist_pending = false;
}
int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
@@ -158,11 +154,12 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
break;
case HV_X64_MSR_VP_ASSIST_PAGE:
- teardown_vp_assist(v); /* release any previous mapping */
+ /* release any previous mapping */
+ teardown_guest_page(&v->arch.hvm.viridian.vp_assist);
v->arch.hvm.viridian.vp_assist.msr.raw = val;
dump_vp_assist(v);
if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled )
- initialize_vp_assist(v);
+ initialize_guest_page(v, &v->arch.hvm.viridian.vp_assist);
break;
default:
@@ -211,7 +208,7 @@ int viridian_synic_rdmsr(const struct vcpu *v, uint32_t idx, uint64_t *val)
void viridian_synic_save_vcpu_ctxt(const struct vcpu *v,
struct hvm_viridian_vcpu_context *ctxt)
{
- ctxt->vp_assist_pending = v->arch.hvm.viridian.vp_assist.pending;
+ ctxt->apic_assist_pending = v->arch.hvm.viridian.apic_assist_pending;
ctxt->vp_assist_msr = v->arch.hvm.viridian.vp_assist.msr.raw;
}
@@ -220,9 +217,9 @@ void viridian_synic_load_vcpu_ctxt(struct vcpu *v,
{
v->arch.hvm.viridian.vp_assist.msr.raw = ctxt->vp_assist_msr;
if ( v->arch.hvm.viridian.vp_assist.msr.fields.enabled )
- initialize_vp_assist(v);
+ initialize_guest_page(v, &v->arch.hvm.viridian.vp_assist);
- v->arch.hvm.viridian.vp_assist.pending = !!ctxt->vp_assist_pending;
+ v->arch.hvm.viridian.apic_assist_pending = !!ctxt->apic_assist_pending;
}
/*
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 66aa24c43e..8d5690582e 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -88,13 +88,16 @@ union viridian_page_msr
} fields;
};
+struct viridian_page
+{
+ union viridian_page_msr msr;
+ void *ptr;
+};
+
struct viridian_vcpu
{
- struct {
- union viridian_page_msr msr;
- void *ptr;
- bool pending;
- } vp_assist;
+ struct viridian_page vp_assist;
+ bool apic_assist_pending;
uint64_t crash_param[5];
};
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 4691d4d4aa..80e762c335 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -600,7 +600,7 @@ DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct hvm_viridian_domain_context);
struct hvm_viridian_vcpu_context {
uint64_t vp_assist_msr;
- uint8_t vp_assist_pending;
+ uint8_t apic_assist_pending;
uint8_t _pad[7];
};
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 8/8] viridian: introduce struct viridian_page
2018-10-29 18:02 ` [PATCH 8/8] viridian: introduce struct viridian_page Paul Durrant
@ 2018-10-30 17:17 ` Roger Pau Monné
2018-10-30 17:25 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2018-10-30 17:17 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Wei Liu, Ian Jackson, Jan Beulich, Andrew Cooper
On Mon, Oct 29, 2018 at 06:02:11PM +0000, Paul Durrant wrote:
> 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 <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> 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(-)
>
> diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c
> index 40e77851be..a4efadf307 100644
> --- a/tools/misc/xen-hvmctx.c
> +++ b/tools/misc/xen-hvmctx.c
> @@ -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",
> + printf(" VIRIDIAN_VCPU: vp_assist_msr 0x%llx, apic_assist_pending %s\n",
> (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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 8/8] viridian: introduce struct viridian_page
2018-10-30 17:17 ` Roger Pau Monné
@ 2018-10-30 17:25 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2018-10-30 17:25 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Wei Liu,
Jan Beulich, Ian Jackson
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 17:18
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wei.liu2@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>
> Subject: Re: [Xen-devel] [PATCH 8/8] viridian: introduce struct
> viridian_page
>
> On Mon, Oct 29, 2018 at 06:02:11PM +0000, Paul Durrant wrote:
> > 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%20Specif
> ication%20v5.0C.pdf
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
Thanks.
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > 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(-)
> >
> > diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c
> > index 40e77851be..a4efadf307 100644
> > --- a/tools/misc/xen-hvmctx.c
> > +++ b/tools/misc/xen-hvmctx.c
> > @@ -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",
> > + printf(" VIRIDIAN_VCPU: vp_assist_msr 0x%llx,
> apic_assist_pending %s\n",
> > (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?
Sure, I'll take a look at the whitespace.
Paul
>
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread