xen/arch/x86/hvm/svm/svm.c | 11 +++++------ xen/arch/x86/include/asm/hvm/svm/vmcb.h | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-)
This removes raw number manipulation, and makes the logic easier to follow.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
xen/arch/x86/hvm/svm/svm.c | 11 +++++------
xen/arch/x86/include/asm/hvm/svm/vmcb.h | 14 ++++++++++++++
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index a43bcf2e92f0..bfe03316def6 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2939,13 +2939,12 @@ void svm_vmexit_handler(void)
break;
case VMEXIT_IOIO:
- if ( (vmcb->exitinfo1 & (1u<<2)) == 0 )
+ if ( !vmcb->ei.io.str )
{
- uint16_t port = (vmcb->exitinfo1 >> 16) & 0xFFFF;
- int bytes = ((vmcb->exitinfo1 >> 4) & 0x07);
- int dir = (vmcb->exitinfo1 & 1) ? IOREQ_READ : IOREQ_WRITE;
- if ( handle_pio(port, bytes, dir) )
- __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
+ if ( handle_pio(vmcb->ei.io.port,
+ vmcb->ei.io.bytes,
+ vmcb->ei.io.in ? IOREQ_READ : IOREQ_WRITE) )
+ __update_guest_eip(regs, vmcb->ei.io.nrip - vmcb->rip);
}
else if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
hvm_inject_hw_exception(TRAP_gp_fault, 0);
diff --git a/xen/arch/x86/include/asm/hvm/svm/vmcb.h b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
index e87728fa81cd..2981a303746a 100644
--- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
+++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
@@ -436,6 +436,20 @@ struct vmcb_struct {
uint64_t exitinfo2; /* offset 0x80 */
};
union {
+ struct {
+ bool in:1;
+ bool :1;
+ bool str:1;
+ bool /* rep */:1;
+ uint16_t bytes:3;
+ uint16_t /* asz */:3;
+ uint16_t /* seg */:3;
+ uint16_t :3;
+ uint16_t port;
+ uint32_t :32;
+
+ uint64_t nrip;
+ } io;
struct {
uint16_t sel;
uint64_t :48;
--
2.30.2
On 15.03.2023 23:10, Andrew Cooper wrote:
> This removes raw number manipulation, and makes the logic easier to follow.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
But I have a question:
> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> @@ -436,6 +436,20 @@ struct vmcb_struct {
> uint64_t exitinfo2; /* offset 0x80 */
> };
> union {
> + struct {
> + bool in:1;
> + bool :1;
> + bool str:1;
> + bool /* rep */:1;
> + uint16_t bytes:3;
> + uint16_t /* asz */:3;
> + uint16_t /* seg */:3;
Is there a particular reason you comment out some of the field names? I
can see that "asz" might be a little odd to use, but both "rep" and "seg"
are imo fine to have a name even if currently there's nothing accessing
these fields.
Jan
On 16/03/2023 10:19 am, Jan Beulich wrote:
> On 15.03.2023 23:10, Andrew Cooper wrote:
>> This removes raw number manipulation, and makes the logic easier to follow.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
Although looking through the emails on list, apparently the vmx side of
this got missed.
They're both to simplify the current monitor patch.
>
> But I have a question:
>
>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>> @@ -436,6 +436,20 @@ struct vmcb_struct {
>> uint64_t exitinfo2; /* offset 0x80 */
>> };
>> union {
>> + struct {
>> + bool in:1;
>> + bool :1;
>> + bool str:1;
>> + bool /* rep */:1;
>> + uint16_t bytes:3;
>> + uint16_t /* asz */:3;
>> + uint16_t /* seg */:3;
> Is there a particular reason you comment out some of the field names? I
> can see that "asz" might be a little odd to use, but both "rep" and "seg"
> are imo fine to have a name even if currently there's nothing accessing
> these fields.
There's not currently used, and in particular asz looks hard to use as
it doesn't appear to translate nicely. Also, I don't that seg matches
Xen's x86_segment_* encoding.
I can uncomment them if you'd prefer, but I thought this was marginally
safer. I suppose it doesn't really matter.
As for asz, I previously had osz to match before renaming to bytes.
Suggestions welcome.
~Andrew
On 16.03.2023 11:27, Andrew Cooper wrote:
> On 16/03/2023 10:19 am, Jan Beulich wrote:
>> On 15.03.2023 23:10, Andrew Cooper wrote:
>>> This removes raw number manipulation, and makes the logic easier to follow.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Thanks.
>
> Although looking through the emails on list, apparently the vmx side of
> this got missed.
I was wondering, but then expected it would be a separate patch at some
other time.
>>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>> @@ -436,6 +436,20 @@ struct vmcb_struct {
>>> uint64_t exitinfo2; /* offset 0x80 */
>>> };
>>> union {
>>> + struct {
>>> + bool in:1;
>>> + bool :1;
>>> + bool str:1;
>>> + bool /* rep */:1;
>>> + uint16_t bytes:3;
>>> + uint16_t /* asz */:3;
>>> + uint16_t /* seg */:3;
>> Is there a particular reason you comment out some of the field names? I
>> can see that "asz" might be a little odd to use, but both "rep" and "seg"
>> are imo fine to have a name even if currently there's nothing accessing
>> these fields.
>
> There's not currently used, and in particular asz looks hard to use as
> it doesn't appear to translate nicely. Also, I don't that seg matches
> Xen's x86_segment_* encoding.
>
> I can uncomment them if you'd prefer, but I thought this was marginally
> safer. I suppose it doesn't really matter.
I would appreciate if you uncommented rep and seg; leaving asz commented
is ...
> As for asz, I previously had osz to match before renaming to bytes.
> Suggestions welcome.
... perhaps better, as the name would be misleading, and half_asz isn't
very nice either.
Jan
© 2016 - 2023 Red Hat, Inc.