[PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Haozhong Zhang]

The 64-bit DMAR fault address is composed of two 32 bits registers
DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
"Software is expected to access 32-bit registers as aligned doublewords",
a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
DMAR_FEUADDR_REG separately in order to update a 64-bit fault address,
rather than a 64-bit write to DMAR_FEADDR_REG.

Though I haven't seen any errors caused by such one 64-bit write on
real machines, it's still better to follow the specification.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index daaed0abbd..067c092214 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1105,7 +1105,9 @@ static void dma_msi_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
 
     spin_lock_irqsave(&iommu->register_lock, flags);
     dmar_writel(iommu->reg, DMAR_FEDATA_REG, msg.data);
-    dmar_writeq(iommu->reg, DMAR_FEADDR_REG, msg.address);
+    dmar_writel(iommu->reg, DMAR_FEADDR_REG, msg.address_lo);
+    if (x2apic_enabled)
+        dmar_writel(iommu->reg, DMAR_FEUADDR_REG, msg.address_hi);
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 }
 
-- 
2.11.0


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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Roger Pau Monné]

On Mon, Sep 11, 2017 at 02:00:48PM +0800, Haozhong Zhang wrote:
> The 64-bit DMAR fault address is composed of two 32 bits registers
> DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> "Software is expected to access 32-bit registers as aligned doublewords",
> a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> DMAR_FEUADDR_REG separately in order to update a 64-bit fault address,
> rather than a 64-bit write to DMAR_FEADDR_REG.
> 
> Though I haven't seen any errors caused by such one 64-bit write on
> real machines, it's still better to follow the specification.

Either the patch description is missing something or the patch is
wrong. You should mention why is the write to the high part of the
address now conditional on x2APIC being enabled, when it didn't use to
be before.

[...]
> -    dmar_writeq(iommu->reg, DMAR_FEADDR_REG, msg.address);
> +    dmar_writel(iommu->reg, DMAR_FEADDR_REG, msg.address_lo);
> +    if (x2apic_enabled)
> +        dmar_writel(iommu->reg, DMAR_FEUADDR_REG, msg.address_hi);
>      spin_unlock_irqrestore(&iommu->register_lock, flags);

Thanks, Roger.

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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Haozhong Zhang]

On 09/11/17 10:38 +0100, Roger Pau Monné wrote:
> On Mon, Sep 11, 2017 at 02:00:48PM +0800, Haozhong Zhang wrote:
> > The 64-bit DMAR fault address is composed of two 32 bits registers
> > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> > "Software is expected to access 32-bit registers as aligned doublewords",
> > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> > DMAR_FEUADDR_REG separately in order to update a 64-bit fault address,
> > rather than a 64-bit write to DMAR_FEADDR_REG.
> > 
> > Though I haven't seen any errors caused by such one 64-bit write on
> > real machines, it's still better to follow the specification.
> 
> Either the patch description is missing something or the patch is
> wrong. You should mention why is the write to the high part of the
> address now conditional on x2APIC being enabled, when it didn't use to
> be before.
>

When x2APIC is disabled, DMAR_FEUADDR_REG is reserved and it's not
necessary to update it. The original code always writes zero to it in
that case, which is also correct.

Haozhong

> [...]
> > -    dmar_writeq(iommu->reg, DMAR_FEADDR_REG, msg.address);
> > +    dmar_writel(iommu->reg, DMAR_FEADDR_REG, msg.address_lo);
> > +    if (x2apic_enabled)
> > +        dmar_writel(iommu->reg, DMAR_FEUADDR_REG, msg.address_hi);
> >      spin_unlock_irqrestore(&iommu->register_lock, flags);
> 
> Thanks, Roger.

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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Tian, Kevin]

> From: Zhang, Haozhong
> Sent: Monday, September 11, 2017 8:13 PM
> 
> On 09/11/17 10:38 +0100, Roger Pau Monné wrote:
> > On Mon, Sep 11, 2017 at 02:00:48PM +0800, Haozhong Zhang wrote:
> > > The 64-bit DMAR fault address is composed of two 32 bits registers
> > > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d
> spec:
> > > "Software is expected to access 32-bit registers as aligned doublewords",
> > > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> > > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
> address,
> > > rather than a 64-bit write to DMAR_FEADDR_REG.
> > >
> > > Though I haven't seen any errors caused by such one 64-bit write on
> > > real machines, it's still better to follow the specification.
> >
> > Either the patch description is missing something or the patch is
> > wrong. You should mention why is the write to the high part of the
> > address now conditional on x2APIC being enabled, when it didn't use to
> > be before.
> >
> 
> When x2APIC is disabled, DMAR_FEUADDR_REG is reserved and it's not
> necessary to update it. The original code always writes zero to it in
> that case, which is also correct.
> 
> Haozhong
> 

Please add a brief comment in the code to make it clear.

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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Jan Beulich]

>>> On 11.09.17 at 08:00, <haozhong.zhang@intel.com> wrote:
> The 64-bit DMAR fault address is composed of two 32 bits registers
> DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> "Software is expected to access 32-bit registers as aligned doublewords",
> a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> DMAR_FEUADDR_REG separately in order to update a 64-bit fault address,
> rather than a 64-bit write to DMAR_FEADDR_REG.
> 
> Though I haven't seen any errors caused by such one 64-bit write on
> real machines, it's still better to follow the specification.

Any sane chipset should split qword accesses into dword ones if
they can't be handled at some layer. Also if you undo something
explicitly done by an earlier commit, please quote that commit
and say what was wrong. After all Kevin as the VT-d maintainer
agreed with the change back then.

Jan


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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, September 11, 2017 6:03 PM
> 
> >>> On 11.09.17 at 08:00, <haozhong.zhang@intel.com> wrote:
> > The 64-bit DMAR fault address is composed of two 32 bits registers
> > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> > "Software is expected to access 32-bit registers as aligned doublewords",
> > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
> address,
> > rather than a 64-bit write to DMAR_FEADDR_REG.
> >
> > Though I haven't seen any errors caused by such one 64-bit write on
> > real machines, it's still better to follow the specification.
> 
> Any sane chipset should split qword accesses into dword ones if
> they can't be handled at some layer. Also if you undo something
> explicitly done by an earlier commit, please quote that commit
> and say what was wrong. After all Kevin as the VT-d maintainer
> agreed with the change back then.
> 
> Jan

I'm OK with this change. As Jan pointed out, please quote earlier
comment in next version.


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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Jan Beulich]

>>> On 18.09.17 at 10:18, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Monday, September 11, 2017 6:03 PM
>> 
>> >>> On 11.09.17 at 08:00, <haozhong.zhang@intel.com> wrote:
>> > The 64-bit DMAR fault address is composed of two 32 bits registers
>> > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
>> > "Software is expected to access 32-bit registers as aligned doublewords",
>> > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
>> > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
>> address,
>> > rather than a 64-bit write to DMAR_FEADDR_REG.
>> >
>> > Though I haven't seen any errors caused by such one 64-bit write on
>> > real machines, it's still better to follow the specification.
>> 
>> Any sane chipset should split qword accesses into dword ones if
>> they can't be handled at some layer. Also if you undo something
>> explicitly done by an earlier commit, please quote that commit
>> and say what was wrong. After all Kevin as the VT-d maintainer
>> agreed with the change back then.
> 
> I'm OK with this change.

Hmm, would you mind explaining? You were also okay with the
change in the opposite direction back then, and we've had no
reports of problems.

Jan

Jan


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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Haozhong Zhang]

On 09/18/17 02:30 -0600, Jan Beulich wrote:
> >>> On 18.09.17 at 10:18, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, September 11, 2017 6:03 PM
> >> 
> >> >>> On 11.09.17 at 08:00, <haozhong.zhang@intel.com> wrote:
> >> > The 64-bit DMAR fault address is composed of two 32 bits registers
> >> > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> >> > "Software is expected to access 32-bit registers as aligned doublewords",
> >> > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> >> > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
> >> address,
> >> > rather than a 64-bit write to DMAR_FEADDR_REG.
> >> >
> >> > Though I haven't seen any errors caused by such one 64-bit write on
> >> > real machines, it's still better to follow the specification.
> >> 
> >> Any sane chipset should split qword accesses into dword ones if
> >> they can't be handled at some layer. Also if you undo something
> >> explicitly done by an earlier commit, please quote that commit
> >> and say what was wrong. After all Kevin as the VT-d maintainer
> >> agreed with the change back then.
> > 
> > I'm OK with this change.
> 
> Hmm, would you mind explaining? You were also okay with the
> change in the opposite direction back then, and we've had no
> reports of problems.
> 

I haven't seen any issues of the current 64-bit write on recent Intel
Haswell, Broadwell and Skylake Xeon platforms, so I guess the hardware
can properly handle the 64-bits write to contiguous 32-bit registers.

I actually encountered errors when running Xen on KVM/QEMU with QEMU
vIOMMU enabled, which (QEMU) disallows 64-bit writes to 32-bit
registers and aborts if such writes happen.

If this patch is considered senseless (as it does not fix any errors
on real hardware), I'm fine to fix the above abort on QEMU side (i.e.,
let vIOMMU in QEMU follow the behavior of real hardware).


Haozhong






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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Roger Pau Monné]

On Mon, Sep 18, 2017 at 05:05:18PM +0800, Haozhong Zhang wrote:
> On 09/18/17 02:30 -0600, Jan Beulich wrote:
> > >>> On 18.09.17 at 10:18, <kevin.tian@intel.com> wrote:
> > >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> Sent: Monday, September 11, 2017 6:03 PM
> > >> 
> > >> >>> On 11.09.17 at 08:00, <haozhong.zhang@intel.com> wrote:
> > >> > The 64-bit DMAR fault address is composed of two 32 bits registers
> > >> > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> > >> > "Software is expected to access 32-bit registers as aligned doublewords",
> > >> > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> > >> > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
> > >> address,
> > >> > rather than a 64-bit write to DMAR_FEADDR_REG.
> > >> >
> > >> > Though I haven't seen any errors caused by such one 64-bit write on
> > >> > real machines, it's still better to follow the specification.
> > >> 
> > >> Any sane chipset should split qword accesses into dword ones if
> > >> they can't be handled at some layer. Also if you undo something
> > >> explicitly done by an earlier commit, please quote that commit
> > >> and say what was wrong. After all Kevin as the VT-d maintainer
> > >> agreed with the change back then.
> > > 
> > > I'm OK with this change.
> > 
> > Hmm, would you mind explaining? You were also okay with the
> > change in the opposite direction back then, and we've had no
> > reports of problems.
> > 
> 
> I haven't seen any issues of the current 64-bit write on recent Intel
> Haswell, Broadwell and Skylake Xeon platforms, so I guess the hardware
> can properly handle the 64-bits write to contiguous 32-bit registers.
> 
> I actually encountered errors when running Xen on KVM/QEMU with QEMU
> vIOMMU enabled, which (QEMU) disallows 64-bit writes to 32-bit
> registers and aborts if such writes happen.
> 
> If this patch is considered senseless (as it does not fix any errors
> on real hardware), I'm fine to fix the above abort on QEMU side (i.e.,
> let vIOMMU in QEMU follow the behavior of real hardware).

I think that either the spec is changed to mention that quad-word
accesses are allowed, or this patch is applied.

There's nothing wrong with the QEMU implementation, it adheres to the
spec. So unless the spec is changed, we might see issues with other
emulated DMAR units.

Roger.

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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Tian, Kevin]

> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> Sent: Monday, September 18, 2017 5:10 PM
> 
> On Mon, Sep 18, 2017 at 05:05:18PM +0800, Haozhong Zhang wrote:
> > On 09/18/17 02:30 -0600, Jan Beulich wrote:
> > > >>> On 18.09.17 at 10:18, <kevin.tian@intel.com> wrote:
> > > >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> > > >> Sent: Monday, September 11, 2017 6:03 PM
> > > >>
> > > >> >>> On 11.09.17 at 08:00, <haozhong.zhang@intel.com> wrote:
> > > >> > The 64-bit DMAR fault address is composed of two 32 bits registers
> > > >> > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d
> spec:
> > > >> > "Software is expected to access 32-bit registers as aligned
> doublewords",
> > > >> > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG
> and
> > > >> > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
> > > >> address,
> > > >> > rather than a 64-bit write to DMAR_FEADDR_REG.
> > > >> >
> > > >> > Though I haven't seen any errors caused by such one 64-bit write
> on
> > > >> > real machines, it's still better to follow the specification.
> > > >>
> > > >> Any sane chipset should split qword accesses into dword ones if
> > > >> they can't be handled at some layer. Also if you undo something
> > > >> explicitly done by an earlier commit, please quote that commit
> > > >> and say what was wrong. After all Kevin as the VT-d maintainer
> > > >> agreed with the change back then.
> > > >
> > > > I'm OK with this change.
> > >
> > > Hmm, would you mind explaining? You were also okay with the
> > > change in the opposite direction back then, and we've had no
> > > reports of problems.
> > >
> >
> > I haven't seen any issues of the current 64-bit write on recent Intel
> > Haswell, Broadwell and Skylake Xeon platforms, so I guess the hardware
> > can properly handle the 64-bits write to contiguous 32-bit registers.
> >
> > I actually encountered errors when running Xen on KVM/QEMU with
> QEMU
> > vIOMMU enabled, which (QEMU) disallows 64-bit writes to 32-bit
> > registers and aborts if such writes happen.
> >
> > If this patch is considered senseless (as it does not fix any errors
> > on real hardware), I'm fine to fix the above abort on QEMU side (i.e.,
> > let vIOMMU in QEMU follow the behavior of real hardware).
> 
> I think that either the spec is changed to mention that quad-word
> accesses are allowed, or this patch is applied.
> 
> There's nothing wrong with the QEMU implementation, it adheres to the
> spec. So unless the spec is changed, we might see issues with other
> emulated DMAR units.
> 

I checked with our hardware guy. It' recommended to strictly
follow what spec says. there is no hardware-level guarantee 
that a 64b write will touch both FEADDR and FEUADDR. So 
we should fix Xen side.

Thanks
Kevin

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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Roger Pau Monné]

On Mon, Sep 11, 2017 at 02:00:48PM +0800, Haozhong Zhang wrote:
> The 64-bit DMAR fault address is composed of two 32 bits registers
> DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> "Software is expected to access 32-bit registers as aligned doublewords",
> a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> DMAR_FEUADDR_REG separately in order to update a 64-bit fault address,
> rather than a 64-bit write to DMAR_FEADDR_REG.

I would add:

"Note that when x2APIC is disabled DMAR_FEUADDR_REG is reserved and it's not
necessary to update it."

> Though I haven't seen any errors caused by such one 64-bit write on
> real machines, it's still better to follow the specification.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Given the reply from Kevin:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Tian, Kevin]

> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> Sent: Wednesday, September 20, 2017 4:31 PM
> 
> On Mon, Sep 11, 2017 at 02:00:48PM +0800, Haozhong Zhang wrote:
> > The 64-bit DMAR fault address is composed of two 32 bits registers
> > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> > "Software is expected to access 32-bit registers as aligned doublewords",
> > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
> address,
> > rather than a 64-bit write to DMAR_FEADDR_REG.
> 
> I would add:
> 
> "Note that when x2APIC is disabled DMAR_FEUADDR_REG is reserved and
> it's not
> necessary to update it."
> 
> > Though I haven't seen any errors caused by such one 64-bit write on
> > real machines, it's still better to follow the specification.
> >
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> Given the reply from Kevin:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Haozhong, can you resend a new version with patch description
updated?

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

Re: [PATCH] vt-d: use two 32-bit writes to update DMAR fault address registers

[Zhang, Haozhong]

On 10/10/17 13:36 +0800, Tian, Kevin wrote:
> > From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> > Sent: Wednesday, September 20, 2017 4:31 PM
> > 
> > On Mon, Sep 11, 2017 at 02:00:48PM +0800, Haozhong Zhang wrote:
> > > The 64-bit DMAR fault address is composed of two 32 bits registers
> > > DMAR_FEADDR_REG and DMAR_FEUADDR_REG. According to VT-d spec:
> > > "Software is expected to access 32-bit registers as aligned doublewords",
> > > a hypervisor should use two 32-bit writes to DMAR_FEADDR_REG and
> > > DMAR_FEUADDR_REG separately in order to update a 64-bit fault
> > address,
> > > rather than a 64-bit write to DMAR_FEADDR_REG.
> > 
> > I would add:
> > 
> > "Note that when x2APIC is disabled DMAR_FEUADDR_REG is reserved and
> > it's not
> > necessary to update it."
> > 
> > > Though I haven't seen any errors caused by such one 64-bit write on
> > > real machines, it's still better to follow the specification.
> > >
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > 
> > Given the reply from Kevin:
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> 
> Haozhong, can you resend a new version with patch description
> updated?

Sorry, I forgot it and will send.

Thanks,
Haozhong

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