It seems MOVSXD which opcode is 0x63 is not handled, support
to decode it in insn_decode_mmio().
Signed-off-by: Wu Zongyong <[email protected]>
---
arch/x86/lib/insn-eval.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 558a605929db..db6f93bad219 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -1607,6 +1607,10 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
return INSN_MMIO_DECODE_FAILED;
switch (insn->opcode.bytes[0]) {
+ case 0x63: /* MOVSXD r64, m32 */
+ *bytes = 4;
+ type = INSN_MMIO_READ_SIGN_EXTEND;
+ break;
case 0x88: /* MOV m8,r8 */
*bytes = 1;
fallthrough;
--
2.34.3
On 3/28/23 21:59, Wu Zongyong wrote:
> It seems MOVSXD which opcode is 0x63 is not handled, support
> to decode it in insn_decode_mmio().
Aren't there some caveats to worry about with this instruction based on
the presence of the REX prefix 64-bit operand size bit? Sometimes it can
be a sign extended and sometimes it can be a zero extended.
Thanks,
Tom
>
> Signed-off-by: Wu Zongyong <[email protected]>
> ---
> arch/x86/lib/insn-eval.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 558a605929db..db6f93bad219 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -1607,6 +1607,10 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
> return INSN_MMIO_DECODE_FAILED;
>
> switch (insn->opcode.bytes[0]) {
> + case 0x63: /* MOVSXD r64, m32 */
> + *bytes = 4;
> + type = INSN_MMIO_READ_SIGN_EXTEND;
> + break;
> case 0x88: /* MOV m8,r8 */
> *bytes = 1;
> fallthrough;
On Wed, Mar 29, 2023 at 08:49:24AM -0500, Tom Lendacky wrote:
> On 3/28/23 21:59, Wu Zongyong wrote:
> > It seems MOVSXD which opcode is 0x63 is not handled, support
> > to decode it in insn_decode_mmio().
>
> Aren't there some caveats to worry about with this instruction based on the
> presence of the REX prefix 64-bit operand size bit? Sometimes it can be a
> sign extended and sometimes it can be a zero extended.
If I undertand right, the patch should like that?
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 558a605929db..a1272f1be35d 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -1607,6 +1607,13 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
return INSN_MMIO_DECODE_FAILED;
switch (insn->opcode.bytes[0]) {
+ case 0x63:
+ *bytes = 4;
+ if (X86_REX_W(insn->rex_prefix.value))
+ type = INSN_MMIO_READ_SIGN_EXTEND;
+ else
+ type = INSN_MMIO_READ_ZERO_EXTEND;
+ break;
case 0x88: /* MOV m8,r8 */
*bytes = 1;
fallthrough;
>
> Thanks,
> Tom
>
> >
> > Signed-off-by: Wu Zongyong <[email protected]>
> > ---
> > arch/x86/lib/insn-eval.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index 558a605929db..db6f93bad219 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -1607,6 +1607,10 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
> > return INSN_MMIO_DECODE_FAILED;
> > switch (insn->opcode.bytes[0]) {
> > + case 0x63: /* MOVSXD r64, m32 */
> > + *bytes = 4;
> > + type = INSN_MMIO_READ_SIGN_EXTEND;
> > + break;
> > case 0x88: /* MOV m8,r8 */
> > *bytes = 1;
> > fallthrough;
On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
> It seems MOVSXD which opcode is 0x63 is not handled, support
> to decode it in insn_decode_mmio().
Do you have a particular user in mind?
--
Kiryl Shutsemau / Kirill A. Shutemov
On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
> On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
> > It seems MOVSXD which opcode is 0x63 is not handled, support
> > to decode it in insn_decode_mmio().
>
> Do you have a particular user in mind?
To be honest, I don't find a specific user which uses the MOVSXD.
But both Intel and AMD's instructions reference contains MOVSXD and lots
of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
may be useful to support it in insn_decode_mmio().
Are there some special consideration about this instruction?
>
> --
> Kiryl Shutsemau / Kirill A. Shutemov
From: Wu Zongyong
> Sent: 31 March 2023 03:24
>
> On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
> > On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
> > > It seems MOVSXD which opcode is 0x63 is not handled, support
> > > to decode it in insn_decode_mmio().
> >
> > Do you have a particular user in mind?
> To be honest, I don't find a specific user which uses the MOVSXD.
>
> But both Intel and AMD's instructions reference contains MOVSXD and lots
> of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
> may be useful to support it in insn_decode_mmio().
>
> Are there some special consideration about this instruction?
It is a sign-extending memory read (32bit to 64bit).
You pretty much never want to do that to a device register.
Also kernel code should be using readl() (etc) which do
unsigned reads.
So they should never happen for mmio.
Of course, if you mmap() PCIe space directly into a program's
address space anything might happen ...
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Mar 31, 2023 at 08:49:48AM +0000, David Laight wrote:
> From: Wu Zongyong
> > Sent: 31 March 2023 03:24
> >
> > On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
> > > On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
> > > > It seems MOVSXD which opcode is 0x63 is not handled, support
> > > > to decode it in insn_decode_mmio().
> > >
> > > Do you have a particular user in mind?
> > To be honest, I don't find a specific user which uses the MOVSXD.
> >
> > But both Intel and AMD's instructions reference contains MOVSXD and lots
> > of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
> > may be useful to support it in insn_decode_mmio().
> >
> > Are there some special consideration about this instruction?
>
> It is a sign-extending memory read (32bit to 64bit).
> You pretty much never want to do that to a device register.
> Also kernel code should be using readl() (etc) which do
> unsigned reads.
> So they should never happen for mmio.
>
> Of course, if you mmap() PCIe space directly into a program's
> address space anything might happen ...
There are two users of the interface: TDX and SEV. TDX doesn't allow
userspace MMIO. SEV *seems* allows it, but I am not sure how it is safe.
Tom?
--
Kiryl Shutsemau / Kirill A. Shutemov
On 3/31/23 05:06, Kirill A. Shutemov wrote:
> On Fri, Mar 31, 2023 at 08:49:48AM +0000, David Laight wrote:
>> From: Wu Zongyong
>>> Sent: 31 March 2023 03:24
>>>
>>> On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
>>>> On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
>>>>> It seems MOVSXD which opcode is 0x63 is not handled, support
>>>>> to decode it in insn_decode_mmio().
>>>>
>>>> Do you have a particular user in mind?
>>> To be honest, I don't find a specific user which uses the MOVSXD.
>>>
>>> But both Intel and AMD's instructions reference contains MOVSXD and lots
>>> of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
>>> may be useful to support it in insn_decode_mmio().
>>>
>>> Are there some special consideration about this instruction?
>>
>> It is a sign-extending memory read (32bit to 64bit).
>> You pretty much never want to do that to a device register.
>> Also kernel code should be using readl() (etc) which do
>> unsigned reads.
>> So they should never happen for mmio.
>>
>> Of course, if you mmap() PCIe space directly into a program's
>> address space anything might happen ...
>
> There are two users of the interface: TDX and SEV. TDX doesn't allow
> userspace MMIO. SEV *seems* allows it, but I am not sure how it is safe.
>
> Tom?
The insn_decode_mmio() function is only called by the SEV/TDX related code
and is specifically MMIO oriented. As David said, this instruction is
likely not being used for that in the kernel. If we come across a case
where this is used, we can look at how it is being used in that situation
and it can be addressed then.
Thanks,
Tom
>
On Fri, Mar 31, 2023 at 08:40:30AM -0500, Tom Lendacky wrote:
> On 3/31/23 05:06, Kirill A. Shutemov wrote:
> > On Fri, Mar 31, 2023 at 08:49:48AM +0000, David Laight wrote:
> > > From: Wu Zongyong
> > > > Sent: 31 March 2023 03:24
> > > >
> > > > On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
> > > > > On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
> > > > > > It seems MOVSXD which opcode is 0x63 is not handled, support
> > > > > > to decode it in insn_decode_mmio().
> > > > >
> > > > > Do you have a particular user in mind?
> > > > To be honest, I don't find a specific user which uses the MOVSXD.
> > > >
> > > > But both Intel and AMD's instructions reference contains MOVSXD and lots
> > > > of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
> > > > may be useful to support it in insn_decode_mmio().
> > > >
> > > > Are there some special consideration about this instruction?
> > >
> > > It is a sign-extending memory read (32bit to 64bit).
> > > You pretty much never want to do that to a device register.
> > > Also kernel code should be using readl() (etc) which do
> > > unsigned reads.
> > > So they should never happen for mmio.
> > >
> > > Of course, if you mmap() PCIe space directly into a program's
> > > address space anything might happen ...
> >
> > There are two users of the interface: TDX and SEV. TDX doesn't allow
> > userspace MMIO. SEV *seems* allows it, but I am not sure how it is safe.
> >
> > Tom?
>
> The insn_decode_mmio() function is only called by the SEV/TDX related code
> and is specifically MMIO oriented. As David said, this instruction is likely
> not being used for that in the kernel. If we come across a case where this
> is used, we can look at how it is being used in that situation and it can be
> addressed then.
I was asking if SEV supports userspace MMIO. And if yes, how do you make
it safe?
--
Kiryl Shutsemau / Kirill A. Shutemov
On 3/31/23 09:09, Kirill A. Shutemov wrote:
> On Fri, Mar 31, 2023 at 08:40:30AM -0500, Tom Lendacky wrote:
>> On 3/31/23 05:06, Kirill A. Shutemov wrote:
>>> On Fri, Mar 31, 2023 at 08:49:48AM +0000, David Laight wrote:
>>>> From: Wu Zongyong
>>>>> Sent: 31 March 2023 03:24
>>>>>
>>>>> On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
>>>>>> On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
>>>>>>> It seems MOVSXD which opcode is 0x63 is not handled, support
>>>>>>> to decode it in insn_decode_mmio().
>>>>>>
>>>>>> Do you have a particular user in mind?
>>>>> To be honest, I don't find a specific user which uses the MOVSXD.
>>>>>
>>>>> But both Intel and AMD's instructions reference contains MOVSXD and lots
>>>>> of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
>>>>> may be useful to support it in insn_decode_mmio().
>>>>>
>>>>> Are there some special consideration about this instruction?
>>>>
>>>> It is a sign-extending memory read (32bit to 64bit).
>>>> You pretty much never want to do that to a device register.
>>>> Also kernel code should be using readl() (etc) which do
>>>> unsigned reads.
>>>> So they should never happen for mmio.
>>>>
>>>> Of course, if you mmap() PCIe space directly into a program's
>>>> address space anything might happen ...
>>>
>>> There are two users of the interface: TDX and SEV. TDX doesn't allow
>>> userspace MMIO. SEV *seems* allows it, but I am not sure how it is safe.
>>>
>>> Tom?
>>
>> The insn_decode_mmio() function is only called by the SEV/TDX related code
>> and is specifically MMIO oriented. As David said, this instruction is likely
>> not being used for that in the kernel. If we come across a case where this
>> is used, we can look at how it is being used in that situation and it can be
>> addressed then.
>
> I was asking if SEV supports userspace MMIO. And if yes, how do you make
> it safe?
>
No, SEV doesn't support userspace MMIO.
Thanks,
Tom
On Fri, Mar 31, 2023 at 09:33:31AM -0500, Tom Lendacky wrote:
>
>
> On 3/31/23 09:09, Kirill A. Shutemov wrote:
> > On Fri, Mar 31, 2023 at 08:40:30AM -0500, Tom Lendacky wrote:
> > > On 3/31/23 05:06, Kirill A. Shutemov wrote:
> > > > On Fri, Mar 31, 2023 at 08:49:48AM +0000, David Laight wrote:
> > > > > From: Wu Zongyong
> > > > > > Sent: 31 March 2023 03:24
> > > > > >
> > > > > > On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
> > > > > > > On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
> > > > > > > > It seems MOVSXD which opcode is 0x63 is not handled, support
> > > > > > > > to decode it in insn_decode_mmio().
> > > > > > >
> > > > > > > Do you have a particular user in mind?
> > > > > > To be honest, I don't find a specific user which uses the MOVSXD.
> > > > > >
> > > > > > But both Intel and AMD's instructions reference contains MOVSXD and lots
> > > > > > of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
> > > > > > may be useful to support it in insn_decode_mmio().
> > > > > >
> > > > > > Are there some special consideration about this instruction?
> > > > >
> > > > > It is a sign-extending memory read (32bit to 64bit).
> > > > > You pretty much never want to do that to a device register.
> > > > > Also kernel code should be using readl() (etc) which do
> > > > > unsigned reads.
> > > > > So they should never happen for mmio.
> > > > >
> > > > > Of course, if you mmap() PCIe space directly into a program's
> > > > > address space anything might happen ...
> > > >
> > > > There are two users of the interface: TDX and SEV. TDX doesn't allow
> > > > userspace MMIO. SEV *seems* allows it, but I am not sure how it is safe.
> > > >
> > > > Tom?
> > >
> > > The insn_decode_mmio() function is only called by the SEV/TDX related code
> > > and is specifically MMIO oriented. As David said, this instruction is likely
> > > not being used for that in the kernel. If we come across a case where this
> > > is used, we can look at how it is being used in that situation and it can be
> > > addressed then.
> >
> > I was asking if SEV supports userspace MMIO. And if yes, how do you make
> > it safe?
> >
>
> No, SEV doesn't support userspace MMIO.
But where do you filter out userspace MMIO? AFAICS, it goes straight from
from #VC to insn_decode_mmio(). Hm?
--
Kiryl Shutsemau / Kirill A. Shutemov
On 3/28/23 19:59, Wu Zongyong wrote:
> It seems MOVSXD which opcode is 0x63 is not handled, support
> to decode it in insn_decode_mmio().
...
> switch (insn->opcode.bytes[0]) {
> + case 0x63: /* MOVSXD r64, m32 */
> + *bytes = 4;
> + type = INSN_MMIO_READ_SIGN_EXTEND;
> + break;
The kernel does not support _arbitrary_ memory access instructions
messing with MMIO.
Before even considering this, I'd want to see a very concrete
explanation for why _this_ instruction in particular is required. I'd
also want to make sure this doesn't set us off down a slippery slope
trying to make the MMIO decoder more expansive.
On 3/31/23 10:25, Kirill A. Shutemov wrote:
> On Fri, Mar 31, 2023 at 09:33:31AM -0500, Tom Lendacky wrote:
>>
>>
>> On 3/31/23 09:09, Kirill A. Shutemov wrote:
>>> On Fri, Mar 31, 2023 at 08:40:30AM -0500, Tom Lendacky wrote:
>>>> On 3/31/23 05:06, Kirill A. Shutemov wrote:
>>>>> On Fri, Mar 31, 2023 at 08:49:48AM +0000, David Laight wrote:
>>>>>> From: Wu Zongyong
>>>>>>> Sent: 31 March 2023 03:24
>>>>>>>
>>>>>>> On Thu, Mar 30, 2023 at 03:39:51PM +0300, [email protected] wrote:
>>>>>>>> On Wed, Mar 29, 2023 at 10:59:37AM +0800, Wu Zongyong wrote:
>>>>>>>>> It seems MOVSXD which opcode is 0x63 is not handled, support
>>>>>>>>> to decode it in insn_decode_mmio().
>>>>>>>>
>>>>>>>> Do you have a particular user in mind?
>>>>>>> To be honest, I don't find a specific user which uses the MOVSXD.
>>>>>>>
>>>>>>> But both Intel and AMD's instructions reference contains MOVSXD and lots
>>>>>>> of MOVSXD instructions occur when I "objdump -S vmlinux", so I think it
>>>>>>> may be useful to support it in insn_decode_mmio().
>>>>>>>
>>>>>>> Are there some special consideration about this instruction?
>>>>>>
>>>>>> It is a sign-extending memory read (32bit to 64bit).
>>>>>> You pretty much never want to do that to a device register.
>>>>>> Also kernel code should be using readl() (etc) which do
>>>>>> unsigned reads.
>>>>>> So they should never happen for mmio.
>>>>>>
>>>>>> Of course, if you mmap() PCIe space directly into a program's
>>>>>> address space anything might happen ...
>>>>>
>>>>> There are two users of the interface: TDX and SEV. TDX doesn't allow
>>>>> userspace MMIO. SEV *seems* allows it, but I am not sure how it is safe.
>>>>>
>>>>> Tom?
>>>>
>>>> The insn_decode_mmio() function is only called by the SEV/TDX related code
>>>> and is specifically MMIO oriented. As David said, this instruction is likely
>>>> not being used for that in the kernel. If we come across a case where this
>>>> is used, we can look at how it is being used in that situation and it can be
>>>> addressed then.
>>>
>>> I was asking if SEV supports userspace MMIO. And if yes, how do you make
>>> it safe?
>>>
>>
>> No, SEV doesn't support userspace MMIO.
>
> But where do you filter out userspace MMIO? AFAICS, it goes straight from
> from #VC to insn_decode_mmio(). Hm?
The userspace mapping would have the encryption bit set and MMIO to
encrypted memory is detected and not allowed.
Thanks,
Tom
>
From: Kirill A. Shutemov
> Sent: 31 March 2023 16:25
...
> > No, SEV doesn't support userspace MMIO.
>
> But where do you filter out userspace MMIO? AFAICS, it goes straight from
> from #VC to insn_decode_mmio(). Hm?
Probably by making vm_iomap_memory() fail.
Otherwise MOVSXD is the least of your problems.
You'd need to worry about all the AVX opcodes as well.
Although you might even find kernel code that is using
kernel_fpu_begin/end() to wrap mmio copies that use the
big AVX512 registers.
When each PCIe read takes about 1us (measured into our fpga)
increasing the TLP to 64 bytes (from 8) makes a massive
difference to buffer reads.
(Mostly we try to get the fpga to do writes instead.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)