|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 10/28] x86/vvtd: Enable Interrupt Remapping through GCMD
On Fri, Nov 17, 2017 at 02:22:17PM +0800, Chao Gao wrote:
> Software writes this field to enable/disable interrupt reampping. This
> patch emulate IRES field of GCMD. Currently, Guest's whole IRT are
> mapped to Xen permanently for the latency of delivering interrupt. And
> the old mapping is undone if present when trying to set up a new one.
>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
> v4:
> - map guest's interrupt reampping table to Xen permanently rather than
> mapping one specific page on demand.
> ---
> xen/drivers/passthrough/vtd/iommu.h | 3 +-
> xen/drivers/passthrough/vtd/vvtd.c | 98
> +++++++++++++++++++++++++++++++++++++
> 2 files changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.h
> b/xen/drivers/passthrough/vtd/iommu.h
> index 8579843..9c59aeb 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -161,9 +161,10 @@
> #define DMA_GSTS_AFLS (((u64)1) << 28)
> #define DMA_GSTS_WBFS (((u64)1) << 27)
> #define DMA_GSTS_QIES (((u64)1) <<26)
> +#define DMA_GSTS_IRES_SHIFT 25
> +#define DMA_GSTS_IRES (((u64)1) << DMA_GSTS_IRES_SHIFT)
We are trying to avoid more use-cases of u64. Also, didn't you clean
that file in a previous patch? Why was this not properly adjusted to
use UL or uint64_t there?
> #define DMA_GSTS_SIRTPS_SHIFT 24
> #define DMA_GSTS_SIRTPS (((u64)1) << DMA_GSTS_SIRTPS_SHIFT)
> -#define DMA_GSTS_IRES (((u64)1) <<25)
> #define DMA_GSTS_CFIS (((u64)1) <<23)
>
> /* IRTA_REG */
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c
> b/xen/drivers/passthrough/vtd/vvtd.c
> index f0476fe..06e522a 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -24,6 +24,7 @@
> #include <xen/xmalloc.h>
> #include <asm/current.h>
> #include <asm/hvm/domain.h>
> +#include <asm/p2m.h>
>
> #include "iommu.h"
>
> @@ -37,6 +38,7 @@
>
> struct hvm_hw_vvtd {
> bool eim_enabled;
> + bool intremap_enabled;
>
> /* Interrupt remapping table base gfn and the max of entries */
> uint16_t irt_max_entry;
> @@ -52,6 +54,7 @@ struct vvtd {
> struct domain *domain;
>
> struct hvm_hw_vvtd hw;
> + void *irt_base;
> };
>
> /* Setting viommu_verbose enables debugging messages of vIOMMU */
> @@ -118,6 +121,77 @@ static void *domain_vvtd(const struct domain *d)
> return NULL;
> }
>
> +static void *map_guest_pages(struct domain *d, uint64_t gfn, uint32_t nr)
^ gfn_t
Also, this function and unmap_guest_pages look generic enough to be
placed somewhere else, like p2m.c maybe?
> +{
> + mfn_t *mfn = xmalloc_array(mfn_t, nr);
> + void* ret;
> + int i;
> +
> + if ( !mfn )
> + return NULL;
> +
> + for ( i = 0; i < nr; i++)
> + {
> + struct page_info *p = get_page_from_gfn(d, gfn + i, NULL, P2M_ALLOC);
> +
> + if ( !p || !get_page_type(p, PGT_writable_page) )
> + {
> + if ( p )
> + put_page(p);
> + goto undo;
> + }
> +
> + mfn[i] = _mfn(page_to_mfn(p));
Please use the type-safe version of page_to_mfn, by adding the
following at the top of the file:
/* Override macros from asm/mm.h to make them work with mfn_t */
#undef mfn_to_page
#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
#undef page_to_mfn
#define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
> + }
> +
> + ret = vmap(mfn, nr);
> + if ( ret == NULL )
> + goto undo;
> + xfree(mfn);
You can move the xfree(mfn) before the check, and then you can remove
it from the undo label.
And since the undo label is just used once, what about doing
ret = vmap(mfn, nr);
xfree(mfn);
if ( !ret )
{
while ( i-- )
put_page_and_type(mfn_to_page(mfn_x(mfn[i])));
....
> +
> + return ret;
> +
> + undo:
> + for ( ; --i >= 0; )
> + put_page_and_type(mfn_to_page(mfn_x(mfn[i])));
> + xfree(mfn);
> + gprintk(XENLOG_ERR, "Failed to map guest pages %lx nr %x\n", gfn, nr);
> +
> + return NULL;
> +}
> +
> +static void unmap_guest_pages(void *va, uint32_t nr)
unsigned long please.
> +{
> + unsigned long *mfn = xmalloc_array(unsigned long, nr);
> + int i;
> + void *va_copy = va;
> +
> + if ( !mfn )
> + {
> + printk("%s %d: No free memory\n", __FILE__, __LINE__);
> + return;
> + }
> +
> + for ( i = 0; i < nr; i++, va += PAGE_SIZE)
> + mfn[i] = domain_page_map_to_mfn(va);
> +
> + vunmap(va_copy);
> +
> + for ( i = 0; i < nr; i++)
> + put_page_and_type(mfn_to_page(mfn[i]));
> +}
> +
> +static void write_gcmd_ire(struct vvtd *vvtd, uint32_t val)
> +{
> + bool set = val & DMA_GCMD_IRE;
> +
> + vvtd_info("%sable Interrupt Remapping\n", set ? "En" : "Dis");
> +
> + vvtd->hw.intremap_enabled = set;
> + (set ? vvtd_set_bit : vvtd_clear_bit)
> + (vvtd, DMAR_GSTS_REG, DMA_GSTS_IRES_SHIFT);
> +}
> +
> static void write_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
> {
> uint64_t irta = vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG);
> @@ -131,16 +205,29 @@ static void write_gcmd_sirtp(struct vvtd *vvtd,
> uint32_t val)
> * the 'Set Interrupt Remap Table Pointer' operation.
> */
> vvtd_clear_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_SHIFT);
> + if ( vvtd->hw.intremap_enabled )
> + vvtd_info("Update Interrupt Remapping Table when active\n");
>
> if ( gfn_x(vvtd->hw.irt) != PFN_DOWN(DMA_IRTA_ADDR(irta)) ||
> vvtd->hw.irt_max_entry != DMA_IRTA_SIZE(irta) )
> {
> + if ( vvtd->irt_base )
> + {
> + unmap_guest_pages(vvtd->irt_base,
> + PFN_UP(vvtd->hw.irt_max_entry *
> + sizeof(struct iremap_entry)));
> + vvtd->irt_base = NULL;
> + }
Shouldn't this be done when sirtp is switched off, instead of when
it's updated?
What happens in the following scenario:
- Guest writes gfn to irta.
- Guest enables sirtps.
- Guest disables sirtps.
- Guest tries to balloon out the page used in irta.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |