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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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:
> >> +        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.

Oh, I see, so the reference should be dropped when IRE is cleared,
since IRTA can be set multiple times without IRE set, which shouldn't
result in the page tables being mapped.

One thing that I don't really quite like about all this implementation
is that you allocate space for all the registers in 'regs', and yet
you keep adding more fields to the struct, like eim_enabled or
irt_max_entry which can all be obtained from 'regs' itself.

IMO, I think you should keep data in a single place to avoid it
getting out of sync. So either you use 'regs' for everything, or you
drop 'regs' completely and simply use per-register custom fields that
you add to hvm_hw_vvtd when they are needed.

I think the later would be clearer, but I haven't reviewed the whole
series yet.

> >> +        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.

Oh, I see, I didn't know the spec only allows changing one bit at a
time. What does real hardware do when multiple bits are changed at the
same write?

Is some kind of error triggered?

I think this is likely helpful, but should be a WARN or ERROR log
message, not an info one.

Thanks. Roger.

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