|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V2 13/25] x86/vvtd: Set Interrupt Remapping Table Pointer through GCMD
On Wed, Aug 09, 2017 at 04:34:14PM -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>
> ---
> xen/drivers/passthrough/vtd/iommu.h | 9 ++++-
> xen/drivers/passthrough/vtd/vvtd.c | 73
> +++++++++++++++++++++++++++++++++++++
> 2 files changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.h
> b/xen/drivers/passthrough/vtd/iommu.h
> index 55f3b6e..102b4f3 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -192,9 +192,16 @@
> #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_BIT 24
Those kind of defines are usually suffixed with _SHIFT, not _BIT.
> +#define DMA_GSTS_SIRTPS (((u64)1) << DMA_GSTS_SIRTPS_BIT)
> #define DMA_GSTS_CFIS (((u64)1) <<23)
>
> +/* IRTA_REG */
> +#define DMA_IRTA_ADDR(val) (val & ~0xfffULL)
> +#define DMA_IRTA_EIME(val) (!!(val & (1 << 11)))
No need for the outer parentheses.
> +#define DMA_IRTA_S(val) (val & 0xf)
> +#define DMA_IRTA_SIZE(val) (1UL << (DMA_IRTA_S(val) + 1))
All those defines above seem kind of magic. Can we maybe get a small
comment describing it's meaning?
> /* 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 94680e6..8e8dbe6 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -46,6 +46,13 @@ struct vvtd {
> uint64_t length;
> /* Point back to the owner domain */
> struct domain *domain;
> + /* Is in Extended Interrupt Mode? */
> + bool eim;
> + /* Max remapping entries in IRT */
> + int irt_max_entry;
unsigned int.
> + /* Interrupt remapping table base gfn */
> + uint64_t irt;
If it's a gfn you should use gfn_t.
> +
> struct hvm_hw_vvtd_regs *regs;
> struct page_info *regs_page;
> };
> @@ -82,6 +89,11 @@ static inline struct vvtd *vcpu_vvtd(struct vcpu *v)
> return domain_vvtd(v->domain);
> }
>
> +static inline void __vvtd_set_bit(struct vvtd *vvtd, uint32_t reg, int nr)
No leading underscores, and unsigned int for nr.
> +{
> + return __set_bit(nr, (uint32_t *)&vvtd->regs->data[reg]);
Why the return?
> +}
> +
> static inline void vvtd_set_reg(struct vvtd *vtd, uint32_t reg,
> uint32_t value)
> {
> @@ -108,6 +120,44 @@ static inline uint8_t vvtd_get_reg_byte(struct vvtd
> *vtd, uint32_t reg)
> vvtd_set_reg(vvtd, (reg) + 4, (val) >> 32); \
> } while(0)
>
> +static int vvtd_handle_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
> +{
> + uint64_t irta;
> +
> + if ( !(val & DMA_GCMD_SIRTP) )
> + return X86EMUL_OKAY;
> +
> + vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG, irta);
> + vvtd->irt = DMA_IRTA_ADDR(irta) >> PAGE_SHIFT;
> + vvtd->irt_max_entry = DMA_IRTA_SIZE(irta);
> + vvtd->eim = DMA_IRTA_EIME(irta);
> + VVTD_DEBUG(VVTD_DBG_RW, "Update IR info (addr=%lx eim=%d size=%d).",
> + vvtd->irt, vvtd->eim, vvtd->irt_max_entry);
> + __vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_BIT);
> +
> + return X86EMUL_OKAY;
> +}
> +
> +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 & 0x96ffffff; /* reset the one-shot bits */
Some kind of define for this magic value is needed.
> + changed = orig ^ val;
> +
> + if ( !changed )
> + return X86EMUL_OKAY;
> + if ( (changed & (changed - 1)) )
> + VVTD_DEBUG(VVTD_DBG_RW, "Guest attempts to update multiple fields "
> + "of GCMD_REG in one write transation.");
Since this is a debug message it would be good to print at least the
value of changed, or possibly even better the values of both orig and
val.
> +
> + if ( changed & DMA_GCMD_SIRTP )
> + vvtd_handle_gcmd_sirtp(vvtd, val);
> +
> + return X86EMUL_OKAY;
> +}
> +
> static int vvtd_range(struct vcpu *v, unsigned long addr)
> {
> struct vvtd *vvtd = vcpu_vvtd(v);
> @@ -165,12 +215,18 @@ static int vvtd_write(struct vcpu *v, unsigned long
> addr,
> {
> switch ( offset_aligned )
> {
> + case DMAR_GCMD_REG:
> + ret = vvtd_write_gcmd(vvtd, val);
> + break;
> +
> 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_aligned, val);
> ret = X86EMUL_OKAY;
> break;
> @@ -179,6 +235,20 @@ static int vvtd_write(struct vcpu *v, unsigned long addr,
> break;
> }
> }
> + else /* len == 8 */
> + {
> + switch ( offset_aligned )
> + {
> + case DMAR_IRTA_REG:
> + vvtd_set_reg_quad(vvtd, DMAR_IRTA_REG, val);
> + ret = X86EMUL_OKAY;
> + break;
> +
> + default:
> + ret = X86EMUL_UNHANDLEABLE;
Same here, you should not return X86EMUL_UNHANDLEABLE but instead
mimic what native hardware would do in such case.
> + break;
> + }
> + }
>
> return ret;
> }
> @@ -235,6 +305,9 @@ static int vvtd_create(struct domain *d, struct viommu
> *viommu)
> vvtd->length = viommu->length;
> vvtd->domain = d;
> vvtd->status = VIOMMU_STATUS_DEFAULT;
> + vvtd->eim = 0;
> + vvtd->irt = 0;
> + vvtd->irt_max_entry = 0;
Maybe you should consider using xzalloc which will already initialize
this to 0.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |