xen/drivers/vpci/vpci.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
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(-)
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;
}
--
2.22.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.