|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V3 13/29] x86/vvtd: Set Interrupt Remapping Table Pointer through GCMD
On Thu, Sep 21, 2017 at 11:01:54PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
>
> Software sets this field to set/update the interrupt remapping table pointer
> used by hardware. The interrupt remapping table pointer is specified through
> the Interrupt Remapping Table Address (IRTA_REG) register.
>
> This patch emulates this operation and adds some new fields in VVTD to track
> info (e.g. the table's gfn and max supported entries) of interrupt remapping
> table.
>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
>
> ---
> v3:
> - ignore unaligned r/w of vt-d hardware registers and return X86EMUL_OK
> ---
> xen/drivers/passthrough/vtd/iommu.h | 12 ++++++-
> xen/drivers/passthrough/vtd/vvtd.c | 69
> +++++++++++++++++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.h
> b/xen/drivers/passthrough/vtd/iommu.h
> index ef038c9..a0d5ec8 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -153,6 +153,8 @@
> #define DMA_GCMD_IRE (((u64)1) << 25)
> #define DMA_GCMD_SIRTP (((u64)1) << 24)
> #define DMA_GCMD_CFI (((u64)1) << 23)
> +/* mask of one-shot bits */
> +#define DMA_GCMD_ONE_SHOT_MASK 0x96ffffff
Trailing white space.
>
> /* GSTS_REG */
> #define DMA_GSTS_TES (((u64)1) << 31)
> @@ -162,9 +164,17 @@
> #define DMA_GSTS_WBFS (((u64)1) << 27)
> #define DMA_GSTS_QIES (((u64)1) <<26)
> #define DMA_GSTS_IRES (((u64)1) <<25)
> -#define DMA_GSTS_SIRTPS (((u64)1) << 24)
> +#define DMA_GSTS_SIRTPS_SHIFT 24
> +#define DMA_GSTS_SIRTPS (((u64)1) << DMA_GSTS_SIRTPS_SHIFT)
> #define DMA_GSTS_CFIS (((u64)1) <<23)
>
> +/* IRTA_REG */
> +/* The base of 4KB aligned interrupt remapping table */
> +#define DMA_IRTA_ADDR(val) ((val) & ~0xfffULL)
> +/* The size of remapping table is 2^(x+1), where x is the size field in IRTA
> */
> +#define DMA_IRTA_S(val) (val & 0xf)
> +#define DMA_IRTA_SIZE(val) (1UL << (DMA_IRTA_S(val) + 1))
> +
> /* PMEN_REG */
> #define DMA_PMEN_EPM (((u32)1) << 31)
> #define DMA_PMEN_PRS (((u32)1) << 0)
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c
> b/xen/drivers/passthrough/vtd/vvtd.c
> index a3002c3..6736956 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -32,6 +32,13 @@
> /* Supported capabilities by vvtd */
> unsigned int vvtd_caps = VIOMMU_CAP_IRQ_REMAPPING;
>
> +struct hvm_hw_vvtd_status {
> + uint32_t eim_enabled : 1;
bool maybe?
> + uint32_t irt_max_entry;
> + /* Interrupt remapping table base gfn */
> + uint64_t irt;
If it's a gfn, use gfn_t as the type.
> +};
> +
> union hvm_hw_vvtd_regs {
> uint32_t data32[256];
> uint64_t data64[128];
> @@ -43,6 +50,8 @@ struct vvtd {
> uint64_t length;
> /* Point back to the owner domain */
> struct domain *domain;
> +
> + struct hvm_hw_vvtd_status status;
Why you need a sub-struct for this, can't this just be placed inside
of hvm_hw_vvtd_regs directly?
> union hvm_hw_vvtd_regs *regs;
> struct page_info *regs_page;
> };
> @@ -70,6 +79,11 @@ struct vvtd *domain_vvtd(struct domain *d)
> return (d->viommu) ? d->viommu->priv : NULL;
> }
>
> +static inline void vvtd_set_bit(struct vvtd *vvtd, uint32_t reg, int nr)
> +{
> + __set_bit(nr, &vvtd->regs->data32[reg/sizeof(uint32_t)]);
> +}
> +
> static inline void vvtd_set_reg(struct vvtd *vtd, uint32_t reg, uint32_t
> value)
> {
> vtd->regs->data32[reg/sizeof(uint32_t)] = value;
> @@ -91,6 +105,44 @@ static inline uint64_t vvtd_get_reg_quad(struct vvtd
> *vtd, uint32_t reg)
> return vtd->regs->data64[reg/sizeof(uint64_t)];
> }
>
> +static void vvtd_handle_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
> +{
> + uint64_t irta = vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG);
> +
> + if ( !(val & DMA_GCMD_SIRTP) )
> + return;
> +
> + vvtd->status.irt = DMA_IRTA_ADDR(irta) >> PAGE_SHIFT;
> + vvtd->status.irt_max_entry = DMA_IRTA_SIZE(irta);
> + vvtd->status.eim_enabled = !!(irta & IRTA_EIME);
If you use a bool you don't need the '!!'.
> + vvtd_info("Update IR info (addr=%lx eim=%d size=%d).",
The final '.' is unneeded IMHO.
> + vvtd->status.irt, vvtd->status.eim_enabled,
> + vvtd->status.irt_max_entry);
> + vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_SHIFT);
> +}
> +
> +static int vvtd_write_gcmd(struct vvtd *vvtd, uint32_t val)
> +{
> + uint32_t orig = vvtd_get_reg(vvtd, DMAR_GSTS_REG);
> + uint32_t changed;
> +
> + orig = orig & DMA_GCMD_ONE_SHOT_MASK; /* reset the one-shot bits */
> + changed = orig ^ val;
> +
> + if ( !changed )
> + return X86EMUL_OKAY;
> +
> + if ( changed & (changed - 1) )
> + vvtd_info("Guest attempts to write %x to GCMD (current GSTS is %x),"
Trailing white-space.
> + "it would lead to update multiple fields",
Also try to reduce the size of the message, so it can fit in a single
line.
> + val, orig);
> +
> + if ( changed & DMA_GCMD_SIRTP )
> + vvtd_handle_gcmd_sirtp(vvtd, val);
> +
> + return X86EMUL_OKAY;
> +}
> +
> static int vvtd_in_range(struct vcpu *v, unsigned long addr)
> {
> struct vvtd *vvtd = domain_vvtd(v->domain);
> @@ -135,12 +187,17 @@ static int vvtd_write(struct vcpu *v, unsigned long
> addr,
> {
> switch ( offset )
> {
> + case DMAR_GCMD_REG:
> + return vvtd_write_gcmd(vvtd, val);
> +
> case DMAR_IEDATA_REG:
> case DMAR_IEADDR_REG:
> case DMAR_IEUADDR_REG:
> case DMAR_FEDATA_REG:
> case DMAR_FEADDR_REG:
> case DMAR_FEUADDR_REG:
> + case DMAR_IRTA_REG:
> + case DMAR_IRTA_REG_HI:
> vvtd_set_reg(vvtd, offset, val);
> break;
>
> @@ -148,6 +205,18 @@ static int vvtd_write(struct vcpu *v, unsigned long addr,
> break;
> }
> }
> + else /* len == 8 */
> + {
> + switch ( offset )
> + {
> + case DMAR_IRTA_REG:
> + vvtd_set_reg_quad(vvtd, DMAR_IRTA_REG, val);
I have kind of a generic comment regarding the handlers in general,
which I will just make here. Don't you need some kind of locking to
prevent concurrent read/write accesses to the registers?
Also the 'if' to handle different sized accesses to the same registers
seems quite cumbersome. I would think there's a better way to handle
this with a single switch statement.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |