Re: [PATCH v4 09/28] x86/vvtd: Set Interrupt Remapping Table Pointer through GCMD

From: Chao Gao <chao.gao@intel.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>,
	xen-devel@lists.xen.org, "Tim Deegan" <tim@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Wei Liu" <wei.liu2@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@eu.citrix.com>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Lan Tianyu" <tianyu.lan@intel.com>
Subject: Re: [PATCH v4 09/28] x86/vvtd: Set Interrupt Remapping Table Pointer through GCMD
Date: Sun, 11 Feb 2018 13:09:31 +0800	[thread overview]
Message-ID: <20180211050929.GA124683@skl-4s-chao.sh.intel.com> (raw)
In-Reply-To: <20180211043410.GA122555@skl-4s-chao.sh.intel.com>

On Sun, Feb 11, 2018 at 12:34:11PM +0800, Chao Gao wrote:
>On Fri, Feb 09, 2018 at 04:59:11PM +0000, Roger Pau Monné wrote:
>>On Fri, Nov 17, 2017 at 02:22:16PM +0800, Chao Gao wrote:
>>> Software sets SIRTP field of GCMD 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>
>>> 
>>> ---
>>> v4:
>>>  - declare eim_enabled as bool and irt as gfn_t
>>>  - rename vvtd_handle_gcmd_sirtp() to write_gcmd_sirtp()
>>> 
>>> v3:
>>>  - ignore unaligned r/w of vt-d hardware registers and return X86EMUL_OK
>>> ---
>>>  xen/drivers/passthrough/vtd/iommu.h | 16 ++++++-
>>>  xen/drivers/passthrough/vtd/vvtd.c  | 86 +++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 100 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
>>> index f2ef3dd..8579843 100644
>>> --- a/xen/drivers/passthrough/vtd/iommu.h
>>> +++ b/xen/drivers/passthrough/vtd/iommu.h
>>> @@ -48,7 +48,8 @@
>>>  #define DMAR_IQT_REG            0x88 /* invalidation queue tail */
>>>  #define DMAR_IQA_REG            0x90 /* invalidation queue addr */
>>>  #define DMAR_IECTL_REG          0xa0 /* invalidation event control register */
>>> -#define DMAR_IRTA_REG           0xb8 /* intr remap */
>>> +#define DMAR_IRTA_REG           0xb8 /* base address of intr remap table */
>>> +#define DMAR_IRTUA_REG          0xbc /* upper address of intr remap table */
>>>  
>>>  #define OFFSET_STRIDE        (9)
>>>  #define dmar_readl(dmar, reg) readl((dmar) + (reg))
>>> @@ -150,6 +151,9 @@
>>>  #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)
>>>  #define DMA_GSTS_RTPS   (((u64)1) << 30)
>>> @@ -157,10 +161,18 @@
>>>  #define DMA_GSTS_AFLS   (((u64)1) << 28)
>>>  #define DMA_GSTS_WBFS   (((u64)1) << 27)
>>>  #define DMA_GSTS_QIES   (((u64)1) <<26)
>>> +#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_SIRTPS (((u64)1) << 24)
>>>  #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 d78d878..f0476fe 100644
>>> --- a/xen/drivers/passthrough/vtd/vvtd.c
>>> +++ b/xen/drivers/passthrough/vtd/vvtd.c
>>> @@ -36,6 +36,12 @@
>>>  #define VVTD_MAX_OFFSET VVTD_FRCD_END
>>>  
>>>  struct hvm_hw_vvtd {
>>> +    bool eim_enabled;
>>> +
>>> +    /* Interrupt remapping table base gfn and the max of entries */
>>> +    uint16_t irt_max_entry;
>>> +    gfn_t irt;
>>> +
>>>      uint32_t regs[VVTD_MAX_OFFSET/sizeof(uint32_t)];
>>>  };
>>>  
>>> @@ -73,6 +79,16 @@ boolean_runtime_param("viommu_verbose", viommu_verbose);
>>>  
>>>  #define VVTD_REG_POS(vvtd, offset) &(vvtd->hw.regs[offset/sizeof(uint32_t)])
>>>  
>>> +static inline void vvtd_set_bit(struct vvtd *vvtd, uint32_t reg, int nr)
>>> +{
>>> +    __set_bit(nr, VVTD_REG_POS(vvtd, reg));
>>> +}
>>> +
>>> +static inline void vvtd_clear_bit(struct vvtd *vvtd, uint32_t reg, int nr)
>>> +{
>>> +    __clear_bit(nr, VVTD_REG_POS(vvtd, reg));
>>> +}
>>> +
>>>  static inline void vvtd_set_reg(struct vvtd *vvtd, uint32_t reg, uint32_t value)
>>>  {
>>>      *VVTD_REG_POS(vvtd, reg) = value;
>>> @@ -102,6 +118,52 @@ static void *domain_vvtd(const struct domain *d)
>>>          return NULL;
>>>  }
>>>  
>>> +static void write_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>>> +{
>>> +    uint64_t irta = vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG);
>>> +
>>> +    if ( !(val & DMA_GCMD_SIRTP) )
>>
>>I think you likely want to do put_gfn here (see my comment below).
>>
>>> +        return;
>>> +
>>> +    /*
>>> +     * Hardware clears this bit when software sets the SIRTPS field in
>>> +     * the Global Command register and sets it when hardware completes
>>> +     * the 'Set Interrupt Remap Table Pointer' operation.
>>> +     */
>>> +    vvtd_clear_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_SHIFT);
>>> +
>>> +    if ( gfn_x(vvtd->hw.irt) != PFN_DOWN(DMA_IRTA_ADDR(irta)) ||
>>> +         vvtd->hw.irt_max_entry != DMA_IRTA_SIZE(irta) )
>>> +    {
>>> +        vvtd->hw.irt = _gfn(PFN_DOWN(DMA_IRTA_ADDR(irta)));
>>
>>I'm not sure about the usage of this gfn (I guess I will figure out in
>>further patches), but I think you should probably use get_gfn so that
>>you take a reference to it. Using PFN_DOWN and _gfn is clearly
>>defeating the purpose of the whole gfn infrastructure.
>>
>>Note that you then need to use put_gfn when releasing it.
>
>The steps to enable interrupt remapping is:
>1. write to IRTA. Software should write the physcial address of interrupt
>remapping table to this register.
>2. write GCMD with SIRTP set. According to VT-d spec 10.4.4, software
>sets SIRTP to set/update the interrupt reampping table pointer used by
>hardware.
>3. write GCMD with IRE set.
>
>In this version, we get a reference in step3 (in next patch, through
>map/unmap guest IRT) other than in step2. The benefit is when guest
>tries to write SIRTP many times before enabling interrupt remapping,
>vvtd doesn't need to perform map/unmap guest IRT each time.

Oops, I should corret it here. In this version, we get a reference in step2
when mapping guest interrupt remapping table (in next patch, I will move
it to this patch). Needn't use get_gfn() here to get a reference here
again.

Thanks
Chao

>
>>
>>> +        vvtd->hw.irt_max_entry = DMA_IRTA_SIZE(irta);
>>> +        vvtd->hw.eim_enabled = !!(irta & IRTA_EIME);
>>> +        vvtd_info("Update IR info (addr=%lx eim=%d size=%d)\n",
>>> +                  gfn_x(vvtd->hw.irt), vvtd->hw.eim_enabled,
>>> +                  vvtd->hw.irt_max_entry);
>>> +    }
>>> +    vvtd_set_bit(vvtd, DMAR_GSTS_REG, DMA_GSTS_SIRTPS_SHIFT);
>>> +}
>>> +
>>> +static void 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;
>>> +
>>> +    if ( changed & (changed - 1) )
>>> +        vvtd_info("Write %x to GCMD (current %x), updating multiple fields",
>>> +                  val, orig);
>>
>>I'm not sure I see the purpose of the above message.
>
>I will remove this. My original throught is when we could get a warning
>that guest driver doesn't completely follow VT-d spec 10.4.4:
>If multiple control fields in this register need to be modified,
>software much serialize the modification through multiple writes to this
>register.
>
>Thanks
>Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel