| Message ID | 1506049330-11196-14-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 21, 2017 at 11:01:54PM -0400, Lan Tianyu wrote: > From: Chao Gao <chao.gao@intel.com> > > 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@intel.com> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > > --- > 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.
On Thu, Oct 19, 2017 at 12:56:45PM +0100, Roger Pau Monné wrote: >On Thu, Sep 21, 2017 at 11:01:54PM -0400, Lan Tianyu wrote: >> From: Chao Gao <chao.gao@intel.com> >> >> 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@intel.com> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> >> --- >> @@ -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? I think guest should be responsible to avoid concurrency. Xen only needs to not be fooled (crashed) by a malicious guest. > >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. Will use only one switch statement and maybe add if-else for the cases which can be accessed with different size. Thanks chao
>>> On 20.10.17 at 06:08, <chao.gao@intel.com> wrote: > On Thu, Oct 19, 2017 at 12:56:45PM +0100, Roger Pau Monné wrote: >>On Thu, Sep 21, 2017 at 11:01:54PM -0400, Lan Tianyu wrote: >>> @@ -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? > > I think guest should be responsible to avoid concurrency. > Xen only needs to not be fooled (crashed) by a malicious guest. But can you assure this without doing some locking yourself? Jan
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 /* 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; + uint32_t irt_max_entry; + /* Interrupt remapping table base gfn */ + uint64_t irt; +}; + 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; 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); + vvtd_info("Update IR info (addr=%lx eim=%d size=%d).", + 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)," + "it would lead to update multiple fields", + 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); + break; + + default: + break; + } + } return X86EMUL_OKAY; }