| Message ID | 20190903161428.7159-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ioreq: add support for internal servers | expand |
On 03/09/2019 17:14, Roger Pau Monne wrote: > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > index 692b710b02..69652e1080 100644 > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, > switch ( type ) > { > case XEN_DMOP_IO_RANGE_PORT: > + rc = -EINVAL; > + /* PCI config space accesses are handled internally. */ > + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) > + goto out; > + else > + /* fallthrough. */ You need to drop the else, or it may still trigger warnings. Furthermore, qemu registers cf8-cff so I think you need some fix-ups there first before throwing errors back here. Finally, this prohibits registering cf9 which may legitimately not be terminated in Xen. ~Andrew
On Tue, Sep 03, 2019 at 06:13:59PM +0100, Andrew Cooper wrote: > On 03/09/2019 17:14, Roger Pau Monne wrote: > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > > index 692b710b02..69652e1080 100644 > > --- a/xen/arch/x86/hvm/ioreq.c > > +++ b/xen/arch/x86/hvm/ioreq.c > > @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, > > switch ( type ) > > { > > case XEN_DMOP_IO_RANGE_PORT: > > + rc = -EINVAL; > > + /* PCI config space accesses are handled internally. */ > > + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) > > + goto out; > > + else > > + /* fallthrough. */ > > You need to drop the else, or it may still trigger warnings. Yes, my mistake. The else branch is not needed. > Furthermore, qemu registers cf8-cff so I think you need some fix-ups > there first before throwing errors back here. The version of QEMU I have doesn't seem to register 0xcf8 or 0xcfc, there are no errors on the log and QEMU seems to work just fine. I always assumed QEMU was getting accesses to cf8/cfc forwarded because it was the default device model, and everything not trapped by Xen would be forwarded to it. This default device model behaviour was removed by Paul some time ago, and now QEMU registers explicitly which IO accesses it wants to trap. > Finally, this prohibits registering cf9 which may legitimately not be > terminated in Xen. Yes, that should be cf8 - 7 not 8, thanks for catching it! Will update on the next version. Roger.
On Wed, Sep 04, 2019 at 09:49:23AM +0200, Roger Pau Monné wrote: > On Tue, Sep 03, 2019 at 06:13:59PM +0100, Andrew Cooper wrote: > > On 03/09/2019 17:14, Roger Pau Monne wrote: > > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > > > index 692b710b02..69652e1080 100644 > > > --- a/xen/arch/x86/hvm/ioreq.c > > > +++ b/xen/arch/x86/hvm/ioreq.c > > > @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, > > > switch ( type ) > > > { > > > case XEN_DMOP_IO_RANGE_PORT: > > > + rc = -EINVAL; > > > + /* PCI config space accesses are handled internally. */ > > > + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) > > > + goto out; > > > + else > > > + /* fallthrough. */ > > Finally, this prohibits registering cf9 which may legitimately not be > > terminated in Xen. > > Yes, that should be cf8 - 7 not 8, thanks for catching it! Will update > on the next version. ... or just use < instead of <=. Roger.
On 04.09.2019 09:49, Roger Pau Monné wrote: > On Tue, Sep 03, 2019 at 06:13:59PM +0100, Andrew Cooper wrote: >> On 03/09/2019 17:14, Roger Pau Monne wrote: >>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c >>> index 692b710b02..69652e1080 100644 >>> --- a/xen/arch/x86/hvm/ioreq.c >>> +++ b/xen/arch/x86/hvm/ioreq.c >>> @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, >>> switch ( type ) >>> { >>> case XEN_DMOP_IO_RANGE_PORT: >>> + rc = -EINVAL; >>> + /* PCI config space accesses are handled internally. */ >>> + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) >>> + goto out; >>> + else >>> + /* fallthrough. */ >> >> Finally, this prohibits registering cf9 which may legitimately not be >> terminated in Xen. > > Yes, that should be cf8 - 7 not 8, thanks for catching it! Will update > on the next version. Well, assuming you mean + instead of - , then yes, this needs fixing. But doing so won't take care of Andrew's comment. Jan
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 04 September 2019 08:49 > To: Andrew Cooper <Andrew.Cooper3@citrix.com> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > <jbeulich@suse.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v2 02/11] ioreq: terminate cf8 handling at hypervisor level > > On Tue, Sep 03, 2019 at 06:13:59PM +0100, Andrew Cooper wrote: > > On 03/09/2019 17:14, Roger Pau Monne wrote: > > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > > > index 692b710b02..69652e1080 100644 > > > --- a/xen/arch/x86/hvm/ioreq.c > > > +++ b/xen/arch/x86/hvm/ioreq.c > > > @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, > > > switch ( type ) > > > { > > > case XEN_DMOP_IO_RANGE_PORT: > > > + rc = -EINVAL; > > > + /* PCI config space accesses are handled internally. */ > > > + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) > > > + goto out; > > > + else > > > + /* fallthrough. */ > > > > You need to drop the else, or it may still trigger warnings. > > Yes, my mistake. The else branch is not needed. > > > Furthermore, qemu registers cf8-cff so I think you need some fix-ups > > there first before throwing errors back here. > > The version of QEMU I have doesn't seem to register 0xcf8 or 0xcfc, > there are no errors on the log and QEMU seems to work just fine. > > I always assumed QEMU was getting accesses to cf8/cfc forwarded > because it was the default device model, and everything not trapped by > Xen would be forwarded to it. This default device model behaviour was > removed by Paul some time ago, and now QEMU registers explicitly which > IO accesses it wants to trap. Yes, it used to need them to work correctly as a default emulator. However, we don't generally stop an external emulator from registering ranges that are handled by emulation directly in Xen (e.g. pmtimer) so I don't think you need special-case these ports. Paul > > > Finally, this prohibits registering cf9 which may legitimately not be > > terminated in Xen. > > Yes, that should be cf8 - 7 not 8, thanks for catching it! Will update > on the next version. > > Roger.
On Wed, Sep 04, 2019 at 11:46:24AM +0200, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: 04 September 2019 08:49 > > To: Andrew Cooper <Andrew.Cooper3@citrix.com> > > Cc: xen-devel@lists.xenproject.org; Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > > <jbeulich@suse.com>; Wei Liu <wl@xen.org> > > Subject: Re: [PATCH v2 02/11] ioreq: terminate cf8 handling at hypervisor level > > > > On Tue, Sep 03, 2019 at 06:13:59PM +0100, Andrew Cooper wrote: > > > On 03/09/2019 17:14, Roger Pau Monne wrote: > > > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > > > > index 692b710b02..69652e1080 100644 > > > > --- a/xen/arch/x86/hvm/ioreq.c > > > > +++ b/xen/arch/x86/hvm/ioreq.c > > > > @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, > > > > switch ( type ) > > > > { > > > > case XEN_DMOP_IO_RANGE_PORT: > > > > + rc = -EINVAL; > > > > + /* PCI config space accesses are handled internally. */ > > > > + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) > > > > + goto out; > > > > + else > > > > + /* fallthrough. */ > > > > > > You need to drop the else, or it may still trigger warnings. > > > > Yes, my mistake. The else branch is not needed. > > > > > Furthermore, qemu registers cf8-cff so I think you need some fix-ups > > > there first before throwing errors back here. > > > > The version of QEMU I have doesn't seem to register 0xcf8 or 0xcfc, > > there are no errors on the log and QEMU seems to work just fine. > > > > I always assumed QEMU was getting accesses to cf8/cfc forwarded > > because it was the default device model, and everything not trapped by > > Xen would be forwarded to it. This default device model behaviour was > > removed by Paul some time ago, and now QEMU registers explicitly which > > IO accesses it wants to trap. > > Yes, it used to need them to work correctly as a default emulator. However, we don't generally stop an external emulator from registering ranges that are handled by emulation directly in Xen (e.g. pmtimer) so I don't think you need special-case these ports. That's right, so I guess I would just remove that check (and the one introduced for MCFG regions). We also don't check whether any other ioreq server has already registered a range. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 04 September 2019 14:40 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-devel@lists.xenproject.org; Jan Beulich > <jbeulich@suse.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v2 02/11] ioreq: terminate cf8 handling at hypervisor level > > On Wed, Sep 04, 2019 at 11:46:24AM +0200, Paul Durrant wrote: > > > -----Original Message----- > > > From: Roger Pau Monne <roger.pau@citrix.com> > > > Sent: 04 September 2019 08:49 > > > To: Andrew Cooper <Andrew.Cooper3@citrix.com> > > > Cc: xen-devel@lists.xenproject.org; Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > > > <jbeulich@suse.com>; Wei Liu <wl@xen.org> > > > Subject: Re: [PATCH v2 02/11] ioreq: terminate cf8 handling at hypervisor level > > > > > > On Tue, Sep 03, 2019 at 06:13:59PM +0100, Andrew Cooper wrote: > > > > On 03/09/2019 17:14, Roger Pau Monne wrote: > > > > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > > > > > index 692b710b02..69652e1080 100644 > > > > > --- a/xen/arch/x86/hvm/ioreq.c > > > > > +++ b/xen/arch/x86/hvm/ioreq.c > > > > > @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, > > > > > switch ( type ) > > > > > { > > > > > case XEN_DMOP_IO_RANGE_PORT: > > > > > + rc = -EINVAL; > > > > > + /* PCI config space accesses are handled internally. */ > > > > > + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) > > > > > + goto out; > > > > > + else > > > > > + /* fallthrough. */ > > > > > > > > You need to drop the else, or it may still trigger warnings. > > > > > > Yes, my mistake. The else branch is not needed. > > > > > > > Furthermore, qemu registers cf8-cff so I think you need some fix-ups > > > > there first before throwing errors back here. > > > > > > The version of QEMU I have doesn't seem to register 0xcf8 or 0xcfc, > > > there are no errors on the log and QEMU seems to work just fine. > > > > > > I always assumed QEMU was getting accesses to cf8/cfc forwarded > > > because it was the default device model, and everything not trapped by > > > Xen would be forwarded to it. This default device model behaviour was > > > removed by Paul some time ago, and now QEMU registers explicitly which > > > IO accesses it wants to trap. > > > > Yes, it used to need them to work correctly as a default emulator. However, we don't generally stop > an external emulator from registering ranges that are handled by emulation directly in Xen (e.g. > pmtimer) so I don't think you need special-case these ports. > > That's right, so I guess I would just remove that check (and the one > introduced for MCFG regions). We also don't check whether any other > ioreq server has already registered a range. That's right... it's a last-one-wins game. We could decide to change this in future, but it is quite convenient for testing purposes. Paul > > Thanks, Roger.
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 692b710b02..69652e1080 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -1015,6 +1015,12 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id, switch ( type ) { case XEN_DMOP_IO_RANGE_PORT: + rc = -EINVAL; + /* PCI config space accesses are handled internally. */ + if ( start <= 0xcf8 + 8 && 0xcf8 <= end ) + goto out; + else + /* fallthrough. */ case XEN_DMOP_IO_RANGE_MEMORY: case XEN_DMOP_IO_RANGE_PCI: r = s->range[type]; @@ -1518,11 +1524,15 @@ static int hvm_access_cf8( { struct domain *d = current->domain; - if ( dir == IOREQ_WRITE && bytes == 4 ) + if ( bytes != 4 ) + return X86EMUL_OKAY; + + if ( dir == IOREQ_WRITE ) d->arch.hvm.pci_cf8 = *val; + else + *val = d->arch.hvm.pci_cf8; - /* We always need to fall through to the catch all emulator */ - return X86EMUL_UNHANDLEABLE; + return X86EMUL_OKAY; } void hvm_ioreq_init(struct domain *d)
Do not forward accesses to cf8 to external emulators, decoding of PCI accesses is handled by Xen, and emulators can request handling of config space accesses of devices using the provided ioreq interface. Fully terminate cf8 accesses at the hypervisor level, by improving the existing hvm_access_cf8 helper to also handle register reads, and always return X86EMUL_OKAY in order to terminate the emulation. Also return an error to ioreq servers attempting to map PCI IO ports (0xcf8-cfc), as those are handled by Xen. Note that without this change in the absence of some external emulator that catches accesses to cf8 read requests to the register would misbehave, as the ioreq internal handler did not handle those. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/hvm/ioreq.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)