xen/arch/x86/pv/emul-gate-op.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
__{put,get}_guest returns -EFAULT on access faults which causes
the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
incorrect.
Fix the computation by relying on copy_{from,to}_guest_pv which
reports the number of remaining bytes instead of a negative errno,
such that we can compute the offset properly.
Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/arch/x86/pv/emul-gate-op.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
index c2c699fbff..cacc171115 100644
--- a/xen/arch/x86/pv/emul-gate-op.c
+++ b/xen/arch/x86/pv/emul-gate-op.c
@@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
int rc;
#define push(item) do \
{ \
+ unsigned int __value = item; \
--stkp; \
esp -= 4; \
- rc = __put_guest(item, stkp); \
+ rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
if ( rc ) \
{ \
pv_inject_page_fault(PFEC_write_access, \
@@ -359,7 +360,7 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
unsigned int parm;
--ustkp;
- rc = __get_guest(parm, ustkp);
+ rc = copy_from_guest_pv(&parm, ustkp, sizeof(parm));
if ( rc )
{
pv_inject_page_fault(0, (unsigned long)(ustkp + 1) - rc);
--
2.52.0
--
| Vates
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.techOn 20/05/2026 4:51 pm, Teddy Astie wrote:
> __{put,get}_guest returns -EFAULT on access faults which causes
> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
> incorrect.
>
> Fix the computation by relying on copy_{from,to}_guest_pv which
> reports the number of remaining bytes instead of a negative errno,
> such that we can compute the offset properly.
>
> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
> index c2c699fbff..cacc171115 100644
> --- a/xen/arch/x86/pv/emul-gate-op.c
> +++ b/xen/arch/x86/pv/emul-gate-op.c
> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
> int rc;
> #define push(item) do \
> { \
> + unsigned int __value = item; \
> --stkp; \
> esp -= 4; \
> - rc = __put_guest(item, stkp); \
> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
Oh, this probably violates MISRA, but you don't need to use a separate
variable because sizeof() has no side effects.
Given that the expression is now &item, I think it needs to be &(item).
Can also be fixed on commit.
~Andrew
On 20.05.2026 18:34, Andrew Cooper wrote:
> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>> __{put,get}_guest returns -EFAULT on access faults which causes
>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>> incorrect.
>>
>> Fix the computation by relying on copy_{from,to}_guest_pv which
>> reports the number of remaining bytes instead of a negative errno,
>> such that we can compute the offset properly.
>>
>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
>> index c2c699fbff..cacc171115 100644
>> --- a/xen/arch/x86/pv/emul-gate-op.c
>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>> int rc;
>> #define push(item) do \
>> { \
>> + unsigned int __value = item; \
As per other comments, this wants to use uint32_t as type.
Given the number of comments, including some back and forth, I think a v2
really needs submitting (rather than one of us doing on-commit edits).
Jan
>> --stkp; \
>> esp -= 4; \
>> - rc = __put_guest(item, stkp); \
>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>
> Oh, this probably violates MISRA, but you don't need to use a separate
> variable because sizeof() has no side effects.
>
> Given that the expression is now &item, I think it needs to be &(item).
>
> Can also be fixed on commit.
>
> ~Andrew
Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>> __{put,get}_guest returns -EFAULT on access faults which causes
>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>> incorrect.
>>
>> Fix the computation by relying on copy_{from,to}_guest_pv which
>> reports the number of remaining bytes instead of a negative errno,
>> such that we can compute the offset properly.
>>
>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
>> index c2c699fbff..cacc171115 100644
>> --- a/xen/arch/x86/pv/emul-gate-op.c
>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>> int rc;
>> #define push(item) do \
>> { \
>> + unsigned int __value = item; \
>> --stkp; \
>> esp -= 4; \
>> - rc = __put_guest(item, stkp); \
>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>
> Oh, this probably violates MISRA, but you don't need to use a separate
> variable because sizeof() has no side effects.
>
> Given that the expression is now &item, I think it needs to be &(item).
>
I tried something like that, but it looked a bit weird and clang wasn't
happy (at least in language server) because of the &(x + y).
We also need to ensure that we're actually copying 32-bits scalars (and
not 16-bits or 64-bits ones) like the previous behavior.
That diff seems to work though
diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
index cacc171115..b72a3058dd 100644
--- a/xen/arch/x86/pv/emul-gate-op.c
+++ b/xen/arch/x86/pv/emul-gate-op.c
@@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
int rc;
#define push(item) do \
{ \
- unsigned int __value = item; \
--stkp; \
esp -= 4; \
- rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
+ rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
sizeof(uint32_t)); \
if ( rc ) \
{ \
pv_inject_page_fault(PFEC_write_access, \
> Can also be fixed on commit.
>
> ~Andrew
Teddy
On 20.05.2026 18:48, Teddy Astie wrote:
> Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
>> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>>> __{put,get}_guest returns -EFAULT on access faults which causes
>>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>>> incorrect.
>>>
>>> Fix the computation by relying on copy_{from,to}_guest_pv which
>>> reports the number of remaining bytes instead of a negative errno,
>>> such that we can compute the offset properly.
>>>
>>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>> ---
>>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
>>> index c2c699fbff..cacc171115 100644
>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>>> int rc;
>>> #define push(item) do \
>>> { \
>>> + unsigned int __value = item; \
>>> --stkp; \
>>> esp -= 4; \
>>> - rc = __put_guest(item, stkp); \
>>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>
>> Oh, this probably violates MISRA, but you don't need to use a separate
>> variable because sizeof() has no side effects.
>>
>> Given that the expression is now &item, I think it needs to be &(item).
>>
>
> I tried something like that, but it looked a bit weird and clang wasn't
> happy (at least in language server) because of the &(x + y).
>
> We also need to ensure that we're actually copying 32-bits scalars (and
> not 16-bits or 64-bits ones) like the previous behavior.
>
> That diff seems to work though
>
> diff --git a/xen/arch/x86/pv/emul-gate-op.c b/xen/arch/x86/pv/emul-gate-op.c
> index cacc171115..b72a3058dd 100644
> --- a/xen/arch/x86/pv/emul-gate-op.c
> +++ b/xen/arch/x86/pv/emul-gate-op.c
> @@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
> int rc;
> #define push(item) do \
> { \
> - unsigned int __value = item; \
> --stkp; \
> esp -= 4; \
> - rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
> + rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
> sizeof(uint32_t)); \
But a cast expression isn't an lvalue, so & cannot be applied to it (much
like it can't be applied to (x + y) as you mentioned above).
Jan
> if ( rc ) \
> { \
> pv_inject_page_fault(PFEC_write_access, \
>
>
>> Can also be fixed on commit.
>>
>> ~Andrew
>
> Teddy
On 20/05/2026 5:48 pm, Teddy Astie wrote:
> Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
>> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>>> __{put,get}_guest returns -EFAULT on access faults which causes
>>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>>> incorrect.
>>>
>>> Fix the computation by relying on copy_{from,to}_guest_pv which
>>> reports the number of remaining bytes instead of a negative errno,
>>> such that we can compute the offset properly.
>>>
>>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>> ---
>>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>> b/xen/arch/x86/pv/emul-gate-op.c
>>> index c2c699fbff..cacc171115 100644
>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs
>>> *regs)
>>> int rc;
>>> #define push(item) do \
>>> { \
>>> + unsigned int __value = item; \
>>> --stkp; \
>>> esp -= 4; \
>>> - rc = __put_guest(item, stkp); \
>>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>
>> Oh, this probably violates MISRA, but you don't need to use a separate
>> variable because sizeof() has no side effects.
>>
>> Given that the expression is now &item, I think it needs to be &(item).
>>
>
> I tried something like that, but it looked a bit weird and clang
> wasn't happy (at least in language server) because of the &(x + y).
>
> We also need to ensure that we're actually copying 32-bits scalars
> (and not 16-bits or 64-bits ones) like the previous behavior.
>
> That diff seems to work though
>
> diff --git a/xen/arch/x86/pv/emul-gate-op.c
> b/xen/arch/x86/pv/emul-gate-op.c
> index cacc171115..b72a3058dd 100644
> --- a/xen/arch/x86/pv/emul-gate-op.c
> +++ b/xen/arch/x86/pv/emul-gate-op.c
> @@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
> int rc;
> #define push(item) do \
> { \
> - unsigned int __value = item; \
> --stkp; \
> esp -= 4; \
> - rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
> + rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
> sizeof(uint32_t)); \
> if ( rc ) \
> { \
> pv_inject_page_fault(PFEC_write_access, \
Oh, that's a second bug you're fixing then.
Pushes of ss/cs need to be done with 4-byte writes and zero extended.
I've added:
The use of a local variable in push() also fixes a second bug. On all
but the earliest 32bit CPUs, segment selectors pushes are
zero-extended 32bit stores. Xen was not doing this for %ss and %cs.
to the commit message.
~Andrew
On 20.05.2026 19:21, Andrew Cooper wrote:
> On 20/05/2026 5:48 pm, Teddy Astie wrote:
>> Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
>>> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>>>> __{put,get}_guest returns -EFAULT on access faults which causes
>>>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>>>> incorrect.
>>>>
>>>> Fix the computation by relying on copy_{from,to}_guest_pv which
>>>> reports the number of remaining bytes instead of a negative errno,
>>>> such that we can compute the offset properly.
>>>>
>>>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>>> ---
>>>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>>> b/xen/arch/x86/pv/emul-gate-op.c
>>>> index c2c699fbff..cacc171115 100644
>>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs
>>>> *regs)
>>>> int rc;
>>>> #define push(item) do \
>>>> { \
>>>> + unsigned int __value = item; \
>>>> --stkp; \
>>>> esp -= 4; \
>>>> - rc = __put_guest(item, stkp); \
>>>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>>
>>> Oh, this probably violates MISRA, but you don't need to use a separate
>>> variable because sizeof() has no side effects.
>>>
>>> Given that the expression is now &item, I think it needs to be &(item).
>>>
>>
>> I tried something like that, but it looked a bit weird and clang
>> wasn't happy (at least in language server) because of the &(x + y).
>>
>> We also need to ensure that we're actually copying 32-bits scalars
>> (and not 16-bits or 64-bits ones) like the previous behavior.
>>
>> That diff seems to work though
>>
>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>> b/xen/arch/x86/pv/emul-gate-op.c
>> index cacc171115..b72a3058dd 100644
>> --- a/xen/arch/x86/pv/emul-gate-op.c
>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>> @@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>> int rc;
>> #define push(item) do \
>> { \
>> - unsigned int __value = item; \
>> --stkp; \
>> esp -= 4; \
>> - rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>> + rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
>> sizeof(uint32_t)); \
>> if ( rc ) \
>> { \
>> pv_inject_page_fault(PFEC_write_access, \
>
> Oh, that's a second bug you're fixing then.
>
> Pushes of ss/cs need to be done with 4-byte writes and zero extended.
And they are: Access size is derived from the pointer passed, not from the
item.
Jan
> I've added:
>
> The use of a local variable in push() also fixes a second bug. On all
> but the earliest 32bit CPUs, segment selectors pushes are
> zero-extended 32bit stores. Xen was not doing this for %ss and %cs.
>
> to the commit message.
>
> ~Andrew
On 21.05.2026 08:33, Jan Beulich wrote:
> On 20.05.2026 19:21, Andrew Cooper wrote:
>> On 20/05/2026 5:48 pm, Teddy Astie wrote:
>>> Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
>>>> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>>>>> __{put,get}_guest returns -EFAULT on access faults which causes
>>>>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>>>>> incorrect.
>>>>>
>>>>> Fix the computation by relying on copy_{from,to}_guest_pv which
>>>>> reports the number of remaining bytes instead of a negative errno,
>>>>> such that we can compute the offset properly.
>>>>>
>>>>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>>>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>>>> ---
>>>>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>>>> b/xen/arch/x86/pv/emul-gate-op.c
>>>>> index c2c699fbff..cacc171115 100644
>>>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>>>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs
>>>>> *regs)
>>>>> int rc;
>>>>> #define push(item) do \
>>>>> { \
>>>>> + unsigned int __value = item; \
>>>>> --stkp; \
>>>>> esp -= 4; \
>>>>> - rc = __put_guest(item, stkp); \
>>>>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>>>
>>>> Oh, this probably violates MISRA, but you don't need to use a separate
>>>> variable because sizeof() has no side effects.
>>>>
>>>> Given that the expression is now &item, I think it needs to be &(item).
>>>>
>>>
>>> I tried something like that, but it looked a bit weird and clang
>>> wasn't happy (at least in language server) because of the &(x + y).
>>>
>>> We also need to ensure that we're actually copying 32-bits scalars
>>> (and not 16-bits or 64-bits ones) like the previous behavior.
>>>
>>> That diff seems to work though
>>>
>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>> b/xen/arch/x86/pv/emul-gate-op.c
>>> index cacc171115..b72a3058dd 100644
>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>> @@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>>> int rc;
>>> #define push(item) do \
>>> { \
>>> - unsigned int __value = item; \
>>> --stkp; \
>>> esp -= 4; \
>>> - rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>> + rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
>>> sizeof(uint32_t)); \
>>> if ( rc ) \
>>> { \
>>> pv_inject_page_fault(PFEC_write_access, \
>>
>> Oh, that's a second bug you're fixing then.
>>
>> Pushes of ss/cs need to be done with 4-byte writes and zero extended.
>
> And they are: Access size is derived from the pointer passed, not from the
> item.
Oh, while access size has always been correct, ....
>> I've added:
>>
>> The use of a local variable in push() also fixes a second bug. On all
>> but the earliest 32bit CPUs, segment selectors pushes are
>> zero-extended 32bit stores. Xen was not doing this for %ss and %cs.
... zero-extension was lost with the FRED work, so a 2nd Fixes: tag is
going to be necessary: cb29eed2dae7 ("x86/traps: Extend struct
cpu_user_regs/cpu_info with FRED fields").
Jan
>> to the commit message.
>>
>> ~Andrew
>
On 21/05/2026 8:00 am, Jan Beulich wrote:
> On 21.05.2026 08:33, Jan Beulich wrote:
>> On 20.05.2026 19:21, Andrew Cooper wrote:
>>> On 20/05/2026 5:48 pm, Teddy Astie wrote:
>>>> Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
>>>>> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>>>>>> __{put,get}_guest returns -EFAULT on access faults which causes
>>>>>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>>>>>> incorrect.
>>>>>>
>>>>>> Fix the computation by relying on copy_{from,to}_guest_pv which
>>>>>> reports the number of remaining bytes instead of a negative errno,
>>>>>> such that we can compute the offset properly.
>>>>>>
>>>>>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>>>>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>>>>> ---
>>>>>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>>>>> b/xen/arch/x86/pv/emul-gate-op.c
>>>>>> index c2c699fbff..cacc171115 100644
>>>>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>>>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>>>>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs
>>>>>> *regs)
>>>>>> int rc;
>>>>>> #define push(item) do \
>>>>>> { \
>>>>>> + unsigned int __value = item; \
>>>>>> --stkp; \
>>>>>> esp -= 4; \
>>>>>> - rc = __put_guest(item, stkp); \
>>>>>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>>>> Oh, this probably violates MISRA, but you don't need to use a separate
>>>>> variable because sizeof() has no side effects.
>>>>>
>>>>> Given that the expression is now &item, I think it needs to be &(item).
>>>>>
>>>> I tried something like that, but it looked a bit weird and clang
>>>> wasn't happy (at least in language server) because of the &(x + y).
>>>>
>>>> We also need to ensure that we're actually copying 32-bits scalars
>>>> (and not 16-bits or 64-bits ones) like the previous behavior.
>>>>
>>>> That diff seems to work though
>>>>
>>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>>> b/xen/arch/x86/pv/emul-gate-op.c
>>>> index cacc171115..b72a3058dd 100644
>>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>>> @@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>>>> int rc;
>>>> #define push(item) do \
>>>> { \
>>>> - unsigned int __value = item; \
>>>> --stkp; \
>>>> esp -= 4; \
>>>> - rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>>> + rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
>>>> sizeof(uint32_t)); \
>>>> if ( rc ) \
>>>> { \
>>>> pv_inject_page_fault(PFEC_write_access, \
>>> Oh, that's a second bug you're fixing then.
>>>
>>> Pushes of ss/cs need to be done with 4-byte writes and zero extended.
>> And they are: Access size is derived from the pointer passed, not from the
>> item.
> Oh, while access size has always been correct, ....
>
>>> I've added:
>>>
>>> The use of a local variable in push() also fixes a second bug. On all
>>> but the earliest 32bit CPUs, segment selectors pushes are
>>> zero-extended 32bit stores. Xen was not doing this for %ss and %cs.
> ... zero-extension was lost with the FRED work, so a 2nd Fixes: tag is
> going to be necessary: cb29eed2dae7 ("x86/traps: Extend struct
> cpu_user_regs/cpu_info with FRED fields").
I don't understand this comment.
The FRED work added extra fields into %cs/%ss with unions, but the
fields named cs and ss are still uint16_t. That aspect didn't change.
~Andrew
On 21.05.2026 12:56, Andrew Cooper wrote:
> On 21/05/2026 8:00 am, Jan Beulich wrote:
>> On 21.05.2026 08:33, Jan Beulich wrote:
>>> On 20.05.2026 19:21, Andrew Cooper wrote:
>>>> On 20/05/2026 5:48 pm, Teddy Astie wrote:
>>>>> Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
>>>>>> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>>>>>>> __{put,get}_guest returns -EFAULT on access faults which causes
>>>>>>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>>>>>>> incorrect.
>>>>>>>
>>>>>>> Fix the computation by relying on copy_{from,to}_guest_pv which
>>>>>>> reports the number of remaining bytes instead of a negative errno,
>>>>>>> such that we can compute the offset properly.
>>>>>>>
>>>>>>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>>>>>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>>>>>> ---
>>>>>>> xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>>>>>> b/xen/arch/x86/pv/emul-gate-op.c
>>>>>>> index c2c699fbff..cacc171115 100644
>>>>>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>>>>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>>>>>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs
>>>>>>> *regs)
>>>>>>> int rc;
>>>>>>> #define push(item) do \
>>>>>>> { \
>>>>>>> + unsigned int __value = item; \
>>>>>>> --stkp; \
>>>>>>> esp -= 4; \
>>>>>>> - rc = __put_guest(item, stkp); \
>>>>>>> + rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>>>>> Oh, this probably violates MISRA, but you don't need to use a separate
>>>>>> variable because sizeof() has no side effects.
>>>>>>
>>>>>> Given that the expression is now &item, I think it needs to be &(item).
>>>>>>
>>>>> I tried something like that, but it looked a bit weird and clang
>>>>> wasn't happy (at least in language server) because of the &(x + y).
>>>>>
>>>>> We also need to ensure that we're actually copying 32-bits scalars
>>>>> (and not 16-bits or 64-bits ones) like the previous behavior.
>>>>>
>>>>> That diff seems to work though
>>>>>
>>>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>>>> b/xen/arch/x86/pv/emul-gate-op.c
>>>>> index cacc171115..b72a3058dd 100644
>>>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>>>> @@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>>>>> int rc;
>>>>> #define push(item) do \
>>>>> { \
>>>>> - unsigned int __value = item; \
>>>>> --stkp; \
>>>>> esp -= 4; \
>>>>> - rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>>>> + rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
>>>>> sizeof(uint32_t)); \
>>>>> if ( rc ) \
>>>>> { \
>>>>> pv_inject_page_fault(PFEC_write_access, \
>>>> Oh, that's a second bug you're fixing then.
>>>>
>>>> Pushes of ss/cs need to be done with 4-byte writes and zero extended.
>>> And they are: Access size is derived from the pointer passed, not from the
>>> item.
>> Oh, while access size has always been correct, ....
>>
>>>> I've added:
>>>>
>>>> The use of a local variable in push() also fixes a second bug. On all
>>>> but the earliest 32bit CPUs, segment selectors pushes are
>>>> zero-extended 32bit stores. Xen was not doing this for %ss and %cs.
>> ... zero-extension was lost with the FRED work, so a 2nd Fixes: tag is
>> going to be necessary: cb29eed2dae7 ("x86/traps: Extend struct
>> cpu_user_regs/cpu_info with FRED fields").
>
> I don't understand this comment.
>
> The FRED work added extra fields into %cs/%ss with unions, but the
> fields named cs and ss are still uint16_t. That aspect didn't change.
__{get,put}_guest() determine the amount of data to copy from the pointer
they're passed. That being unsigned int *, 32 bits will be copied
regardless of field type. Actually: __put_guest() casts the incoming
value to the type the pointer argument points to. So FRED work didn't
break anything, and I was wrong to ask for a 2nd Fixes: tag. Zero-
extension was and is there.
For __get_guest() et al there looks to be potential of data corruption, if
variable type is less wide than pointed-to type.
Jan
On 20/05/2026 4:51 pm, Teddy Astie wrote:
> __{put,get}_guest returns -EFAULT on access faults which causes
> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
> incorrect.
>
> Fix the computation by relying on copy_{from,to}_guest_pv which
> reports the number of remaining bytes instead of a negative errno,
> such that we can compute the offset properly.
>
> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
Given it was __*_guest() before, I think we can use the
__copy_*_guest_pv() variants.
I can fix on commit if you're happy? Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>
Jan, this wants committing ahead of your MISRA change, as it needs
backporting.
Strictly speaking, gate emulation is all PV right now. It does want
moving behind CONFIG_PV32.
Also, with these two callers dropped, all remaining users of
__{get,put}_guest() only care about success/failure rather than the
precise value, so there is probably some cleanup which can be done.
~Andrew
On 20.05.2026 18:27, Andrew Cooper wrote:
> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>> __{put,get}_guest returns -EFAULT on access faults which causes
>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>> incorrect.
>>
>> Fix the computation by relying on copy_{from,to}_guest_pv which
>> reports the number of remaining bytes instead of a negative errno,
>> such that we can compute the offset properly.
>>
>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>
> Given it was __*_guest() before, I think we can use the
> __copy_*_guest_pv() variants.
>
> I can fix on commit if you're happy? Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
>
> Jan, this wants committing ahead of your MISRA change, as it needs
> backporting.
It would be slightly easier in that order, but adjusting the backport
wouldn't be overly difficult if the Misra change went in first. Yet
that's still lacking an ack anyway.
Jan
Le 20/05/2026 à 18:27, Andrew Cooper a écrit :
> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>> __{put,get}_guest returns -EFAULT on access faults which causes
>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>> incorrect.
>>
>> Fix the computation by relying on copy_{from,to}_guest_pv which
>> reports the number of remaining bytes instead of a negative errno,
>> such that we can compute the offset properly.
>>
>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>
> Given it was __*_guest() before, I think we can use the
> __copy_*_guest_pv() variants.
>
> I can fix on commit if you're happy? Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
>
Looks good to me.
> Jan, this wants committing ahead of your MISRA change, as it needs
> backporting.
>
> Strictly speaking, gate emulation is all PV right now. It does want
> moving behind CONFIG_PV32.
>
This file is already gated behind CONFIG_PV32, so I think it's already
the case.
> Also, with these two callers dropped, all remaining users of
> __{get,put}_guest() only care about success/failure rather than the
> precise value, so there is probably some cleanup which can be done.
>
> ~Andrew
Teddy
© 2016 - 2026 Red Hat, Inc.