It seems MOVSXD which opcode is 0x63 is not handled, support
to decode it in insn_decode_mmio().

Signed-off-by: Wu Zongyong <wuzongyong@linux.alibaba.com>
---
 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 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 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, kirill.shutemov@linux.intel.com 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, kirill.shutemov@linux.intel.com 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, kirill.shutemov@linux.intel.com 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, kirill.shutemov@linux.intel.com 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, kirill.shutemov@linux.intel.com 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, kirill.shutemov@linux.intel.com 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, kirill.shutemov@linux.intel.com 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

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)

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, kirill.shutemov@linux.intel.com 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

>

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 <wuzongyong@linux.alibaba.com>
> ---
>   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 <wuzongyong@linux.alibaba.com>
> > ---
> >   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;