| Message ID | 20190902153037.99845-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] vpci: honor read-only devices | expand |
On 02.09.2019 17:30, Roger Pau Monne wrote: > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, > return; > } > > - /* > - * Find the PCI dev matching the address. > - * Passthrough everything that's not trapped. > - */ > + /* Find the PCI dev matching the address. */ > pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); > if ( !pdev ) > { > + const unsigned long *ro_map = pci_get_ro_map(sbdf.seg); > + > + if ( ro_map && test_bit(sbdf.bdf, ro_map) ) > + /* Ignore writes to read-only devices. */ > + return; > + > + /* > + * Let the hardware domain access config space regions for non-existent > + * devices. > + * TODO: revisit for domU support. > + */ > vpci_write_hw(sbdf, reg, size, data); > return; > } > In principle I'm okay with the change, but I have two more things to be considered: 1) I'd prefer if the check was independent of the return value of pci_get_pdev_by_domain(), to be more robust against the r/o map having got updated but the owner still being hwdom. 2) Wouldn't it be better to move the check into the callers of vpci_write(), to avoid the duplicate lookup in the qword-MCFG- write case? The main questionable point here is where, for DomU support, the SBDF translation is going to live. Jan
On Tue, Sep 03, 2019 at 11:09:09AM +0200, Jan Beulich wrote: > On 02.09.2019 17:30, Roger Pau Monne wrote: > > --- a/xen/drivers/vpci/vpci.c > > +++ b/xen/drivers/vpci/vpci.c > > @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, > > return; > > } > > > > - /* > > - * Find the PCI dev matching the address. > > - * Passthrough everything that's not trapped. > > - */ > > + /* Find the PCI dev matching the address. */ > > pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); > > if ( !pdev ) > > { > > + const unsigned long *ro_map = pci_get_ro_map(sbdf.seg); > > + > > + if ( ro_map && test_bit(sbdf.bdf, ro_map) ) > > + /* Ignore writes to read-only devices. */ > > + return; > > + > > + /* > > + * Let the hardware domain access config space regions for non-existent > > + * devices. > > + * TODO: revisit for domU support. > > + */ > > vpci_write_hw(sbdf, reg, size, data); > > return; > > } > > > > In principle I'm okay with the change, but I have two more things > to be considered: > > 1) I'd prefer if the check was independent of the return value of > pci_get_pdev_by_domain(), to be more robust against the r/o map > having got updated but the owner still being hwdom. So the RO check would be done ahead of the owner check? I can do that, but it seems like a bodge for the locking issues (or lack of it) we have in the handling of PCI devices. I assume having a RO device assigned to a domain different than dom_xen is not possible. > 2) Wouldn't it be better to move the check into the callers of > vpci_write(), to avoid the duplicate lookup in the qword-MCFG- > write case? The main questionable point here is where, for DomU > support, the SBDF translation is going to live. So I have a series I'm going to send quite soon in order to integrate vPCI with ioreq, as a first step in order to make it available to domUs. The SBDF translation there is going to be performed by the ioreq code (ie: hvm_select_ioreq_server), but checking against the RO map there would be wrong, as ioreq doesn't know whether the underlying handler is for an emulated device or for a passthrough one. I think the RO check needs to be in the vPCI code itself. Thanks, Roger.
On 03.09.2019 11:28, Roger Pau Monné wrote: > On Tue, Sep 03, 2019 at 11:09:09AM +0200, Jan Beulich wrote: >> On 02.09.2019 17:30, Roger Pau Monne wrote: >>> --- a/xen/drivers/vpci/vpci.c >>> +++ b/xen/drivers/vpci/vpci.c >>> @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, >>> return; >>> } >>> >>> - /* >>> - * Find the PCI dev matching the address. >>> - * Passthrough everything that's not trapped. >>> - */ >>> + /* Find the PCI dev matching the address. */ >>> pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); >>> if ( !pdev ) >>> { >>> + const unsigned long *ro_map = pci_get_ro_map(sbdf.seg); >>> + >>> + if ( ro_map && test_bit(sbdf.bdf, ro_map) ) >>> + /* Ignore writes to read-only devices. */ >>> + return; >>> + >>> + /* >>> + * Let the hardware domain access config space regions for non-existent >>> + * devices. >>> + * TODO: revisit for domU support. >>> + */ >>> vpci_write_hw(sbdf, reg, size, data); >>> return; >>> } >>> >> >> In principle I'm okay with the change, but I have two more things >> to be considered: >> >> 1) I'd prefer if the check was independent of the return value of >> pci_get_pdev_by_domain(), to be more robust against the r/o map >> having got updated but the owner still being hwdom. > > So the RO check would be done ahead of the owner check? > > I can do that, but it seems like a bodge for the locking issues (or > lack of it) we have in the handling of PCI devices. I assume having a > RO device assigned to a domain different than dom_xen is not possible. It ought not be possible. Hence me saying "more robust" (i.e. in case the "ought not" somehow gets broken). And no, the comment wasn't really related to the (lack of) locking here - that's an orthogonal issue. >> 2) Wouldn't it be better to move the check into the callers of >> vpci_write(), to avoid the duplicate lookup in the qword-MCFG- >> write case? The main questionable point here is where, for DomU >> support, the SBDF translation is going to live. > > So I have a series I'm going to send quite soon in order to integrate > vPCI with ioreq, as a first step in order to make it available to > domUs. > > The SBDF translation there is going to be performed by the ioreq code > (ie: hvm_select_ioreq_server), but checking against the RO map there > would be wrong, as ioreq doesn't know whether the underlying handler > is for an emulated device or for a passthrough one. I think the RO > check needs to be in the vPCI code itself. Oh, sure. The question then simply converts to "Where can it be done the earliest?" I.e. when/where do we have the physical SBDF in our hands? Jan
On Tue, Sep 03, 2019 at 11:48:19AM +0200, Jan Beulich wrote: > On 03.09.2019 11:28, Roger Pau Monné wrote: > > On Tue, Sep 03, 2019 at 11:09:09AM +0200, Jan Beulich wrote: > >> On 02.09.2019 17:30, Roger Pau Monne wrote: > >>> --- a/xen/drivers/vpci/vpci.c > >>> +++ b/xen/drivers/vpci/vpci.c > >>> @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, > >>> return; > >>> } > >>> > >>> - /* > >>> - * Find the PCI dev matching the address. > >>> - * Passthrough everything that's not trapped. > >>> - */ > >>> + /* Find the PCI dev matching the address. */ > >>> pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); > >>> if ( !pdev ) > >>> { > >>> + const unsigned long *ro_map = pci_get_ro_map(sbdf.seg); > >>> + > >>> + if ( ro_map && test_bit(sbdf.bdf, ro_map) ) > >>> + /* Ignore writes to read-only devices. */ > >>> + return; > >>> + > >>> + /* > >>> + * Let the hardware domain access config space regions for non-existent > >>> + * devices. > >>> + * TODO: revisit for domU support. > >>> + */ > >>> vpci_write_hw(sbdf, reg, size, data); > >>> return; > >>> } > >>> > >> > >> In principle I'm okay with the change, but I have two more things > >> to be considered: > >> > >> 1) I'd prefer if the check was independent of the return value of > >> pci_get_pdev_by_domain(), to be more robust against the r/o map > >> having got updated but the owner still being hwdom. > > > > So the RO check would be done ahead of the owner check? > > > > I can do that, but it seems like a bodge for the locking issues (or > > lack of it) we have in the handling of PCI devices. I assume having a > > RO device assigned to a domain different than dom_xen is not possible. > > It ought not be possible. Hence me saying "more robust" (i.e. in > case the "ought not" somehow gets broken). And no, the comment > wasn't really related to the (lack of) locking here - that's an > orthogonal issue. Ack, I have to send v3 anyway since I was missing some changes to the vPCI test harness anyway. > >> 2) Wouldn't it be better to move the check into the callers of > >> vpci_write(), to avoid the duplicate lookup in the qword-MCFG- > >> write case? The main questionable point here is where, for DomU > >> support, the SBDF translation is going to live. > > > > So I have a series I'm going to send quite soon in order to integrate > > vPCI with ioreq, as a first step in order to make it available to > > domUs. > > > > The SBDF translation there is going to be performed by the ioreq code > > (ie: hvm_select_ioreq_server), but checking against the RO map there > > would be wrong, as ioreq doesn't know whether the underlying handler > > is for an emulated device or for a passthrough one. I think the RO > > check needs to be in the vPCI code itself. > > Oh, sure. The question then simply converts to "Where can it be done > the earliest?" I.e. when/where do we have the physical SBDF in our > hands? I'm going to introduce a vPCI ioreq handler that will forward accesses to vpci_{read/write}, but that's not here yet, and in any case it's not going to make much of a difference IMO. I think at least ATM the best place to put the check is in vpci_write, later on we can see about moving it, there's a TODO next to it which will help identify this. Thanks, Roger.
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 758d9420e7..fc5feeb627 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, return; } - /* - * Find the PCI dev matching the address. - * Passthrough everything that's not trapped. - */ + /* Find the PCI dev matching the address. */ pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); if ( !pdev ) { + const unsigned long *ro_map = pci_get_ro_map(sbdf.seg); + + if ( ro_map && test_bit(sbdf.bdf, ro_map) ) + /* Ignore writes to read-only devices. */ + return; + + /* + * Let the hardware domain access config space regions for non-existent + * devices. + * TODO: revisit for domU support. + */ vpci_write_hw(sbdf, reg, size, data); return; }
Don't allow the hardware domain write access the PCI config space of devices marked as read-only. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Change the approach and allow full read access, while limiting write access to devices marked RO. --- xen/drivers/vpci/vpci.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)