[5/5] x86/HVM: don't setup an intercept handler for IO port 0xcf8 unconditionally
diff mbox

Message ID 1452688338-70075-6-git-send-email-roger.pau@citrix.com
State New, archived
Headers show

Commit Message

Roger Pau Monné Jan. 13, 2016, 12:32 p.m. UTC
Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
server, otherwise it makes no sense since the only code that uses the value
stored by hvm_access_cf8 is the IOREQ server.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Jan Beulich Jan. 13, 2016, 4:38 p.m. UTC | #1
>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
> server, otherwise it makes no sense since the only code that uses the value
> stored by hvm_access_cf8 is the IOREQ server.

Afaict an ioreq server could also attach subsequently - Paul?

Jan
Andrew Cooper Jan. 13, 2016, 4:45 p.m. UTC | #2
On 13/01/16 16:38, Jan Beulich wrote:
>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>> server, otherwise it makes no sense since the only code that uses the value
>> stored by hvm_access_cf8 is the IOREQ server.
> Afaict an ioreq server could also attach subsequently - Paul?

Indeed it can.

The last-latched cfc/cf8 values should be accurate at that point.  The
intercept needs to stay.

~Andrew
Paul Durrant Jan. 13, 2016, 4:48 p.m. UTC | #3
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 January 2016 16:39
> To: Paul Durrant; Roger Pau Monne
> Cc: Andrew Cooper; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
> port 0xcf8 unconditionally
> 
> >>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
> > Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
> > server, otherwise it makes no sense since the only code that uses the value
> > stored by hvm_access_cf8 is the IOREQ server.
> 
> Afaict an ioreq server could also attach subsequently - Paul?
> 

Indeed, ioreq servers can come and go at any time.

  Paul

> Jan
Roger Pau Monné Jan. 14, 2016, 8:35 a.m. UTC | #4
El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 January 2016 16:39
>> To: Paul Durrant; Roger Pau Monne
>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>> port 0xcf8 unconditionally
>>
>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>> server, otherwise it makes no sense since the only code that uses the value
>>> stored by hvm_access_cf8 is the IOREQ server.
>>
>> Afaict an ioreq server could also attach subsequently - Paul?
>>
> 
> Indeed, ioreq servers can come and go at any time.

Right, then I will have to prevent hvm_access_cf8 from being added if
the domain is the hardware domain, otherwise it overlaps with the Dom0
passthrough intercept (handle_pvh_io).

Roger.
Jan Beulich Jan. 14, 2016, 9:12 a.m. UTC | #5
>>> On 14.01.16 at 09:35, <roger.pau@citrix.com> wrote:
> El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>>> -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 13 January 2016 16:39
>>> To: Paul Durrant; Roger Pau Monne
>>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org 
>>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>>> port 0xcf8 unconditionally
>>>
>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>>> server, otherwise it makes no sense since the only code that uses the value
>>>> stored by hvm_access_cf8 is the IOREQ server.
>>>
>>> Afaict an ioreq server could also attach subsequently - Paul?
>>>
>> 
>> Indeed, ioreq servers can come and go at any time.
> 
> Right, then I will have to prevent hvm_access_cf8 from being added if
> the domain is the hardware domain, otherwise it overlaps with the Dom0
> passthrough intercept (handle_pvh_io).

Yes, that indeed makes sense.

Jan
Andrew Cooper Jan. 14, 2016, 10:50 a.m. UTC | #6
On 14/01/16 09:12, Jan Beulich wrote:
>>>> On 14.01.16 at 09:35, <roger.pau@citrix.com> wrote:
>> El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>>>> -----Original Message-----
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>> Sent: 13 January 2016 16:39
>>>> To: Paul Durrant; Roger Pau Monne
>>>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org 
>>>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>>>> port 0xcf8 unconditionally
>>>>
>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>>>> server, otherwise it makes no sense since the only code that uses the value
>>>>> stored by hvm_access_cf8 is the IOREQ server.
>>>> Afaict an ioreq server could also attach subsequently - Paul?
>>>>
>>> Indeed, ioreq servers can come and go at any time.
>> Right, then I will have to prevent hvm_access_cf8 from being added if
>> the domain is the hardware domain, otherwise it overlaps with the Dom0
>> passthrough intercept (handle_pvh_io).
> Yes, that indeed makes sense.

Even for the hardware domain, cf8 needs trapping and emulating (although
on a different path).  Being an indirect port pair shared by Xen and
dom0, dom0 cannot use it directly of its own accord.

~Andrew
Roger Pau Monné Jan. 14, 2016, 11 a.m. UTC | #7
El 14/01/16 a les 11.50, Andrew Cooper ha escrit:
> On 14/01/16 09:12, Jan Beulich wrote:
>>>>> On 14.01.16 at 09:35, <roger.pau@citrix.com> wrote:
>>> El 13/01/16 a les 17.48, Paul Durrant ha escrit:
>>>>> -----Original Message-----
>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>> Sent: 13 January 2016 16:39
>>>>> To: Paul Durrant; Roger Pau Monne
>>>>> Cc: Andrew Cooper; xen-devel@lists.xenproject.org 
>>>>> Subject: Re: [PATCH 5/5] x86/HVM: don't setup an intercept handler for IO
>>>>> port 0xcf8 unconditionally
>>>>>
>>>>>>>> On 13.01.16 at 13:32, <roger.pau@citrix.com> wrote:
>>>>>> Only intercept accesses to IO port 0xcf8 if there's at least one IOREQ
>>>>>> server, otherwise it makes no sense since the only code that uses the value
>>>>>> stored by hvm_access_cf8 is the IOREQ server.
>>>>> Afaict an ioreq server could also attach subsequently - Paul?
>>>>>
>>>> Indeed, ioreq servers can come and go at any time.
>>> Right, then I will have to prevent hvm_access_cf8 from being added if
>>> the domain is the hardware domain, otherwise it overlaps with the Dom0
>>> passthrough intercept (handle_pvh_io).
>> Yes, that indeed makes sense.
> 
> Even for the hardware domain, cf8 needs trapping and emulating (although
> on a different path).  Being an indirect port pair shared by Xen and
> dom0, dom0 cannot use it directly of its own accord.

Sure, but it has to be handled by handle_pvh_io (which I plan to rename
in due time) and not hvm_access_cf8.

Roger.

Patch
diff mbox

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 28c6cd9..24c3d46 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1141,6 +1141,13 @@  static int hvm_create_ioreq_server(struct domain *d, domid_t domid,
     if ( rc )
         goto fail3;
 
+    /*
+     * We cannot fail after this point, or we risk registering the handler
+     * multiple times (there's no unregister function yet).
+     */
+    if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) )
+        register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
+
     list_add(&s->list_entry,
              &d->arch.hvm_domain.ioreq_server.list);
 
@@ -1625,7 +1632,6 @@  int hvm_domain_initialise(struct domain *d)
     pit_init(d, cpu_khz);
 
     register_portio_handler(d, 0xe9, 1, hvm_print_line);
-    register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
 
     rc = hvm_funcs.domain_initialise(d);
     if ( rc != 0 )