Re: [Xen-devel] [PATCH V3 12/29] x86/vvtd: Add MMIO handler for VVTD

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

On Fri, Oct 20, 2017 at 10:58:32AM +0800, Chao Gao wrote:
> On Thu, Oct 19, 2017 at 12:34:54PM +0100, Roger Pau Monné wrote:
> >On Thu, Sep 21, 2017 at 11:01:53PM -0400, Lan Tianyu wrote:
> >> +    return 0;
> >> +}
> >> +
> >> +static int vvtd_read(struct vcpu *v, unsigned long addr,
> >> +                     unsigned int len, unsigned long *pval)
> >> +{
> >> +    struct vvtd *vvtd = domain_vvtd(v->domain);
> >> +    unsigned int offset = addr - vvtd->base_addr;
> >> +
> >> +    vvtd_info("Read offset %x len %d\n", offset, len);
> >> +
> >> +    if ( (len != 4 && len != 8) || (offset & (len - 1)) )
> >
> >What value does hardware return when performing unaligned reads or
> >reads with wrong size?
> 
> According to VT-d spec section 10.2, "Software must access 64-bit and
> 128-bit registers as either aligned quadwords or aligned doublewords".
> I am afraid there is no specific hardware action for unaligned access
> information. We can treat it as undefined? Then do nothing.
> But I did see windows driver has such accesses. We need to add a
> workaround for windows later.

I would recommend that you do *pval = ~0ul; in that case then.

> >
> >Here you return with pval not set, which is dangerous.
> 
> Indeed. But I need check whether the pval is initialized by the caller.
> If that, it is safe.

Yes, this was recently fixed as part of a XSA, but I would rather
prefer to set pval here in the error case.

> >
> >> +        return X86EMUL_OKAY;
> >> +
> >> +    if ( len == 4 )
> >> +        *pval = vvtd_get_reg(vvtd, offset);
> >> +    else
> >> +        *pval = vvtd_get_reg_quad(vvtd, offset);
> >
> >...yet here you don't check for offset < 1024.
> >
> >> +
> >> +    return X86EMUL_OKAY;
> >> +}
> >> +
> >> +static int vvtd_write(struct vcpu *v, unsigned long addr,
> >> +                      unsigned int len, unsigned long val)
> >> +{
> >> +    struct vvtd *vvtd = domain_vvtd(v->domain);
> >> +    unsigned int offset = addr - vvtd->base_addr;
> >> +
> >> +    vvtd_info("Write offset %x len %d val %lx\n", offset, len, val);
> >> +
> >> +    if ( (len != 4 && len != 8) || (offset & (len - 1)) )
> >> +        return X86EMUL_OKAY;
> >> +
> >> +    if ( len == 4 )
> >> +    {
> >> +        switch ( offset )
> >> +        {
> >> +        case DMAR_IEDATA_REG:
> >> +        case DMAR_IEADDR_REG:
> >> +        case DMAR_IEUADDR_REG:
> >> +        case DMAR_FEDATA_REG:
> >> +        case DMAR_FEADDR_REG:
> >> +        case DMAR_FEUADDR_REG:
> >> +            vvtd_set_reg(vvtd, offset, val);
> >
> >Hm, so you are using a full page when you only care for 6 4B
> >registers? Seem like quite of a waste of memory.
> 
> Registers are added here when according features are introduced.

Even at the end of the series it doesn't seem like you are adding
support to 256 registers. From the code it seems like you allow writes
to 16 4B registers, and although you allow read access to all the
register space it seems quite dubious that you need 256 registers.
Hence the question about trying to minimize memory usage to what's
really needed.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel