* [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space
@ 2013-08-09 15:09 Alexey Kardashevskiy
2013-08-11 13:58 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-09 15:09 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Michael S . Tsirkin, Alexey Kardashevskiy,
Alex Williamson, Paolo Bonzini, David Gibson
A PCI device's DMA address space (possibly an IOMMU) is returned by a
method on the PCIBus. At the moment that only has one caller, so the
method is simply open coded. We'll need another caller for VFIO, so
this patch introduces a helper/wrapper function.
If IOMMU is not set, the pci_device_iommu_address_space() function
returns the parent's IOMMU skipping the "bus master" address space as
otherwise proper emulation would require more effort for no benefit.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[aik: added inheritance from parent if iommu is not set for the current bus]
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* added comment about ignoring bus master address space
v2:
* added inheritance, needed for a pci-bridge on spapr-ppc64
* pci_iommu_as renamed to pci_device_iommu_address_space
---
hw/pci/pci.c | 24 ++++++++++++++++++------
include/hw/pci/pci.h | 1 +
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4c004f5..dbfa395 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
}
pci_dev->bus = bus;
- if (bus->iommu_fn) {
- dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
- } else {
- /* FIXME: inherit memory region from bus creator */
- dma_as = &address_space_memory;
- }
+ dma_as = pci_device_iommu_address_space(pci_dev);
memory_region_init_alias(&pci_dev->bus_master_enable_region,
OBJECT(pci_dev), "bus master",
@@ -2239,6 +2234,23 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
k->props = pci_props;
}
+AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
+{
+ PCIBus *bus = PCI_BUS(dev->bus);
+
+ if (bus->iommu_fn) {
+ return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
+ }
+
+ if (bus->parent_dev) {
+ /** We are ignoring the bus master DMA bit of the bridge
+ * as it would complicate things such as VFIO for no good reason */
+ return pci_device_iommu_address_space(bus->parent_dev);
+ }
+
+ return &address_space_memory;
+}
+
void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
{
bus->iommu_fn = fn;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index ccec2ba..2374aa9 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -405,6 +405,7 @@ void pci_device_deassert_intx(PCIDevice *dev);
typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
+AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
static inline void
--
1.8.3.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space
2013-08-09 15:09 [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space Alexey Kardashevskiy
@ 2013-08-11 13:58 ` Michael S. Tsirkin
2013-08-11 14:36 ` Alexey Kardashevskiy
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-08-11 13:58 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, Alex Williamson, Paolo Bonzini,
David Gibson
On Sat, Aug 10, 2013 at 01:09:08AM +1000, Alexey Kardashevskiy wrote:
> A PCI device's DMA address space (possibly an IOMMU) is returned by a
> method on the PCIBus. At the moment that only has one caller, so the
> method is simply open coded. We'll need another caller for VFIO, so
> this patch introduces a helper/wrapper function.
>
> If IOMMU is not set, the pci_device_iommu_address_space() function
> returns the parent's IOMMU skipping the "bus master" address space as
> otherwise proper emulation would require more effort for no benefit.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [aik: added inheritance from parent if iommu is not set for the current bus]
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> ---
> Changes:
> v3:
> * added comment about ignoring bus master address space
>
> v2:
> * added inheritance, needed for a pci-bridge on spapr-ppc64
> * pci_iommu_as renamed to pci_device_iommu_address_space
> ---
> hw/pci/pci.c | 24 ++++++++++++++++++------
> include/hw/pci/pci.h | 1 +
> 2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4c004f5..dbfa395 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> }
>
> pci_dev->bus = bus;
> - if (bus->iommu_fn) {
> - dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
> - } else {
> - /* FIXME: inherit memory region from bus creator */
> - dma_as = &address_space_memory;
> - }
> + dma_as = pci_device_iommu_address_space(pci_dev);
>
> memory_region_init_alias(&pci_dev->bus_master_enable_region,
> OBJECT(pci_dev), "bus master",
> @@ -2239,6 +2234,23 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
> k->props = pci_props;
> }
>
> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +{
> + PCIBus *bus = PCI_BUS(dev->bus);
> +
> + if (bus->iommu_fn) {
> + return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
> + }
> +
> + if (bus->parent_dev) {
> + /** We are ignoring the bus master DMA bit of the bridge
> + * as it would complicate things such as VFIO for no good reason */
/*
* Always
* like
* this
*/
/** Never
* like this */
The comment should be improved I think.
I would put it like this:
/*
* Note: this does not check bus master enable bit on device or
* any of the pci to pci bridges above it, it's up to the caller to
* check that before initiating the transaction.
*
* TODO: design a mechanism for callers to do this without
* doing bus scans on data path.
*/
Would you like me to queue this on the pci tree? If yes I can
tweak the comment myself, no need to repost.
> + return pci_device_iommu_address_space(bus->parent_dev);
> + }
> +
> + return &address_space_memory;
> +}
> +
> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
> {
> bus->iommu_fn = fn;
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index ccec2ba..2374aa9 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -405,6 +405,7 @@ void pci_device_deassert_intx(PCIDevice *dev);
>
> typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
>
> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
>
> static inline void
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space
2013-08-11 13:58 ` Michael S. Tsirkin
@ 2013-08-11 14:36 ` Alexey Kardashevskiy
2013-08-12 9:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-11 14:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, Alex Williamson, Paolo Bonzini,
David Gibson
On 08/11/2013 11:58 PM, Michael S. Tsirkin wrote:
> On Sat, Aug 10, 2013 at 01:09:08AM +1000, Alexey Kardashevskiy wrote:
>> A PCI device's DMA address space (possibly an IOMMU) is returned by a
>> method on the PCIBus. At the moment that only has one caller, so the
>> method is simply open coded. We'll need another caller for VFIO, so
>> this patch introduces a helper/wrapper function.
>>
>> If IOMMU is not set, the pci_device_iommu_address_space() function
>> returns the parent's IOMMU skipping the "bus master" address space as
>> otherwise proper emulation would require more effort for no benefit.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> [aik: added inheritance from parent if iommu is not set for the current bus]
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> ---
>> Changes:
>> v3:
>> * added comment about ignoring bus master address space
>>
>> v2:
>> * added inheritance, needed for a pci-bridge on spapr-ppc64
>> * pci_iommu_as renamed to pci_device_iommu_address_space
>> ---
>> hw/pci/pci.c | 24 ++++++++++++++++++------
>> include/hw/pci/pci.h | 1 +
>> 2 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 4c004f5..dbfa395 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>> }
>>
>> pci_dev->bus = bus;
>> - if (bus->iommu_fn) {
>> - dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
>> - } else {
>> - /* FIXME: inherit memory region from bus creator */
>> - dma_as = &address_space_memory;
>> - }
>> + dma_as = pci_device_iommu_address_space(pci_dev);
>>
>> memory_region_init_alias(&pci_dev->bus_master_enable_region,
>> OBJECT(pci_dev), "bus master",
>> @@ -2239,6 +2234,23 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
>> k->props = pci_props;
>> }
>>
>> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>> +{
>> + PCIBus *bus = PCI_BUS(dev->bus);
>> +
>> + if (bus->iommu_fn) {
>> + return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
>> + }
>> +
>> + if (bus->parent_dev) {
>> + /** We are ignoring the bus master DMA bit of the bridge
>> + * as it would complicate things such as VFIO for no good reason */
>
> /*
> * Always
> * like
> * this
> */
>
> /** Never
> * like this */
Hm. I thought I saw a lot of those but it was the kernel :)
btw may comments start with "/**" (with no text in that line but still) -
what is the difference to "/*"?
> The comment should be improved I think.
> I would put it like this:
> /*
> * Note: this does not check bus master enable bit on device or
> * any of the pci to pci bridges above it, it's up to the caller to
> * check that before initiating the transaction.
> *
> * TODO: design a mechanism for callers to do this without
> * doing bus scans on data path.
> */
What exactly do you call here "bus scans"?
> Would you like me to queue this on the pci tree? If yes I can
> tweak the comment myself, no need to repost.
Yes, please. Your tree is fine. Thanks!
>> + return pci_device_iommu_address_space(bus->parent_dev);
>> + }
>> +
>> + return &address_space_memory;
>> +}
>> +
>> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
>> {
>> bus->iommu_fn = fn;
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index ccec2ba..2374aa9 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -405,6 +405,7 @@ void pci_device_deassert_intx(PCIDevice *dev);
>>
>> typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
>>
>> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
>> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
>>
>> static inline void
--
Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space
2013-08-11 14:36 ` Alexey Kardashevskiy
@ 2013-08-12 9:02 ` Michael S. Tsirkin
2013-08-27 5:01 ` Alexey Kardashevskiy
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-08-12 9:02 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, Alex Williamson, Paolo Bonzini,
David Gibson
On Mon, Aug 12, 2013 at 12:36:57AM +1000, Alexey Kardashevskiy wrote:
> On 08/11/2013 11:58 PM, Michael S. Tsirkin wrote:
> > On Sat, Aug 10, 2013 at 01:09:08AM +1000, Alexey Kardashevskiy wrote:
> >> A PCI device's DMA address space (possibly an IOMMU) is returned by a
> >> method on the PCIBus. At the moment that only has one caller, so the
> >> method is simply open coded. We'll need another caller for VFIO, so
> >> this patch introduces a helper/wrapper function.
> >>
> >> If IOMMU is not set, the pci_device_iommu_address_space() function
> >> returns the parent's IOMMU skipping the "bus master" address space as
> >> otherwise proper emulation would require more effort for no benefit.
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> [aik: added inheritance from parent if iommu is not set for the current bus]
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>
> >> ---
> >> Changes:
> >> v3:
> >> * added comment about ignoring bus master address space
> >>
> >> v2:
> >> * added inheritance, needed for a pci-bridge on spapr-ppc64
> >> * pci_iommu_as renamed to pci_device_iommu_address_space
> >> ---
> >> hw/pci/pci.c | 24 ++++++++++++++++++------
> >> include/hw/pci/pci.h | 1 +
> >> 2 files changed, 19 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index 4c004f5..dbfa395 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >> }
> >>
> >> pci_dev->bus = bus;
> >> - if (bus->iommu_fn) {
> >> - dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
> >> - } else {
> >> - /* FIXME: inherit memory region from bus creator */
> >> - dma_as = &address_space_memory;
> >> - }
> >> + dma_as = pci_device_iommu_address_space(pci_dev);
> >>
> >> memory_region_init_alias(&pci_dev->bus_master_enable_region,
> >> OBJECT(pci_dev), "bus master",
> >> @@ -2239,6 +2234,23 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
> >> k->props = pci_props;
> >> }
> >>
> >> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> >> +{
> >> + PCIBus *bus = PCI_BUS(dev->bus);
> >> +
> >> + if (bus->iommu_fn) {
> >> + return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
> >> + }
> >> +
> >> + if (bus->parent_dev) {
> >> + /** We are ignoring the bus master DMA bit of the bridge
> >> + * as it would complicate things such as VFIO for no good reason */
> >
> > /*
> > * Always
> > * like
> > * this
> > */
> >
> > /** Never
> > * like this */
>
>
> Hm. I thought I saw a lot of those but it was the kernel :)
> btw may comments start with "/**" (with no text in that line but still) -
> what is the difference to "/*"?
/** are normally for automated generation of docbook from code.
I don't think we do that for QEMU, but in any case, this
would be only any good for properly formatted comments
at top of a function.
>
> > The comment should be improved I think.
> > I would put it like this:
> > /*
> > * Note: this does not check bus master enable bit on device or
> > * any of the pci to pci bridges above it, it's up to the caller to
> > * check that before initiating the transaction.
> > *
> > * TODO: design a mechanism for callers to do this without
> > * doing bus scans on data path.
> > */
>
> What exactly do you call here "bus scans"?
Probably better as 'PCI hierarchy walk'.
>
> > Would you like me to queue this on the pci tree? If yes I can
> > tweak the comment myself, no need to repost.
>
> Yes, please. Your tree is fine. Thanks!
>
OK, I'll apply this.
Thanks!
> >> + return pci_device_iommu_address_space(bus->parent_dev);
> >> + }
> >> +
> >> + return &address_space_memory;
> >> +}
> >> +
> >> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
> >> {
> >> bus->iommu_fn = fn;
> >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> >> index ccec2ba..2374aa9 100644
> >> --- a/include/hw/pci/pci.h
> >> +++ b/include/hw/pci/pci.h
> >> @@ -405,6 +405,7 @@ void pci_device_deassert_intx(PCIDevice *dev);
> >>
> >> typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
> >>
> >> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> >> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
> >>
> >> static inline void
>
>
>
> --
> Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space
2013-08-12 9:02 ` Michael S. Tsirkin
@ 2013-08-27 5:01 ` Alexey Kardashevskiy
2013-08-27 5:32 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-27 5:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, qemu-devel, Alex Williamson, Paolo Bonzini,
David Gibson
On 08/12/2013 07:02 PM, Michael S. Tsirkin wrote:
> On Mon, Aug 12, 2013 at 12:36:57AM +1000, Alexey Kardashevskiy wrote:
>> On 08/11/2013 11:58 PM, Michael S. Tsirkin wrote:
>>> On Sat, Aug 10, 2013 at 01:09:08AM +1000, Alexey Kardashevskiy wrote:
>>>> A PCI device's DMA address space (possibly an IOMMU) is returned by a
>>>> method on the PCIBus. At the moment that only has one caller, so the
>>>> method is simply open coded. We'll need another caller for VFIO, so
>>>> this patch introduces a helper/wrapper function.
>>>>
>>>> If IOMMU is not set, the pci_device_iommu_address_space() function
>>>> returns the parent's IOMMU skipping the "bus master" address space as
>>>> otherwise proper emulation would require more effort for no benefit.
>>>>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> [aik: added inheritance from parent if iommu is not set for the current bus]
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>> ---
>>>> Changes:
>>>> v3:
>>>> * added comment about ignoring bus master address space
>>>>
>>>> v2:
>>>> * added inheritance, needed for a pci-bridge on spapr-ppc64
>>>> * pci_iommu_as renamed to pci_device_iommu_address_space
>>>> ---
>>>> hw/pci/pci.c | 24 ++++++++++++++++++------
>>>> include/hw/pci/pci.h | 1 +
>>>> 2 files changed, 19 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index 4c004f5..dbfa395 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>>> }
>>>>
>>>> pci_dev->bus = bus;
>>>> - if (bus->iommu_fn) {
>>>> - dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
>>>> - } else {
>>>> - /* FIXME: inherit memory region from bus creator */
>>>> - dma_as = &address_space_memory;
>>>> - }
>>>> + dma_as = pci_device_iommu_address_space(pci_dev);
>>>>
>>>> memory_region_init_alias(&pci_dev->bus_master_enable_region,
>>>> OBJECT(pci_dev), "bus master",
>>>> @@ -2239,6 +2234,23 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
>>>> k->props = pci_props;
>>>> }
>>>>
>>>> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>>>> +{
>>>> + PCIBus *bus = PCI_BUS(dev->bus);
>>>> +
>>>> + if (bus->iommu_fn) {
>>>> + return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
>>>> + }
>>>> +
>>>> + if (bus->parent_dev) {
>>>> + /** We are ignoring the bus master DMA bit of the bridge
>>>> + * as it would complicate things such as VFIO for no good reason */
>>>
>>> /*
>>> * Always
>>> * like
>>> * this
>>> */
>>>
>>> /** Never
>>> * like this */
>>
>>
>> Hm. I thought I saw a lot of those but it was the kernel :)
>> btw may comments start with "/**" (with no text in that line but still) -
>> what is the difference to "/*"?
>
> /** are normally for automated generation of docbook from code.
> I don't think we do that for QEMU, but in any case, this
> would be only any good for properly formatted comments
> at top of a function.
>
>>
>>> The comment should be improved I think.
>>> I would put it like this:
>>> /*
>>> * Note: this does not check bus master enable bit on device or
>>> * any of the pci to pci bridges above it, it's up to the caller to
>>> * check that before initiating the transaction.
>>> *
>>> * TODO: design a mechanism for callers to do this without
>>> * doing bus scans on data path.
>>> */
>>
>> What exactly do you call here "bus scans"?
>
> Probably better as 'PCI hierarchy walk'.
>
>>
>>> Would you like me to queue this on the pci tree? If yes I can
>>> tweak the comment myself, no need to repost.
>>
>> Yes, please. Your tree is fine. Thanks!
>>
>
> OK, I'll apply this.
> Thanks!
I could not see this patch in the pull request you send today. Something
happened to the patch what I should fix? Thanks!
--
Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space
2013-08-27 5:01 ` Alexey Kardashevskiy
@ 2013-08-27 5:32 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2013-08-27 5:32 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, Alex Williamson, Paolo Bonzini,
David Gibson
On Tue, Aug 27, 2013 at 03:01:03PM +1000, Alexey Kardashevskiy wrote:
> On 08/12/2013 07:02 PM, Michael S. Tsirkin wrote:
> > On Mon, Aug 12, 2013 at 12:36:57AM +1000, Alexey Kardashevskiy wrote:
> >> On 08/11/2013 11:58 PM, Michael S. Tsirkin wrote:
> >>> On Sat, Aug 10, 2013 at 01:09:08AM +1000, Alexey Kardashevskiy wrote:
> >>>> A PCI device's DMA address space (possibly an IOMMU) is returned by a
> >>>> method on the PCIBus. At the moment that only has one caller, so the
> >>>> method is simply open coded. We'll need another caller for VFIO, so
> >>>> this patch introduces a helper/wrapper function.
> >>>>
> >>>> If IOMMU is not set, the pci_device_iommu_address_space() function
> >>>> returns the parent's IOMMU skipping the "bus master" address space as
> >>>> otherwise proper emulation would require more effort for no benefit.
> >>>>
> >>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>>> [aik: added inheritance from parent if iommu is not set for the current bus]
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>
> >>>> ---
> >>>> Changes:
> >>>> v3:
> >>>> * added comment about ignoring bus master address space
> >>>>
> >>>> v2:
> >>>> * added inheritance, needed for a pci-bridge on spapr-ppc64
> >>>> * pci_iommu_as renamed to pci_device_iommu_address_space
> >>>> ---
> >>>> hw/pci/pci.c | 24 ++++++++++++++++++------
> >>>> include/hw/pci/pci.h | 1 +
> >>>> 2 files changed, 19 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>> index 4c004f5..dbfa395 100644
> >>>> --- a/hw/pci/pci.c
> >>>> +++ b/hw/pci/pci.c
> >>>> @@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >>>> }
> >>>>
> >>>> pci_dev->bus = bus;
> >>>> - if (bus->iommu_fn) {
> >>>> - dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
> >>>> - } else {
> >>>> - /* FIXME: inherit memory region from bus creator */
> >>>> - dma_as = &address_space_memory;
> >>>> - }
> >>>> + dma_as = pci_device_iommu_address_space(pci_dev);
> >>>>
> >>>> memory_region_init_alias(&pci_dev->bus_master_enable_region,
> >>>> OBJECT(pci_dev), "bus master",
> >>>> @@ -2239,6 +2234,23 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
> >>>> k->props = pci_props;
> >>>> }
> >>>>
> >>>> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> >>>> +{
> >>>> + PCIBus *bus = PCI_BUS(dev->bus);
> >>>> +
> >>>> + if (bus->iommu_fn) {
> >>>> + return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
> >>>> + }
> >>>> +
> >>>> + if (bus->parent_dev) {
> >>>> + /** We are ignoring the bus master DMA bit of the bridge
> >>>> + * as it would complicate things such as VFIO for no good reason */
> >>>
> >>> /*
> >>> * Always
> >>> * like
> >>> * this
> >>> */
> >>>
> >>> /** Never
> >>> * like this */
> >>
> >>
> >> Hm. I thought I saw a lot of those but it was the kernel :)
> >> btw may comments start with "/**" (with no text in that line but still) -
> >> what is the difference to "/*"?
> >
> > /** are normally for automated generation of docbook from code.
> > I don't think we do that for QEMU, but in any case, this
> > would be only any good for properly formatted comments
> > at top of a function.
> >
> >>
> >>> The comment should be improved I think.
> >>> I would put it like this:
> >>> /*
> >>> * Note: this does not check bus master enable bit on device or
> >>> * any of the pci to pci bridges above it, it's up to the caller to
> >>> * check that before initiating the transaction.
> >>> *
> >>> * TODO: design a mechanism for callers to do this without
> >>> * doing bus scans on data path.
> >>> */
> >>
> >> What exactly do you call here "bus scans"?
> >
> > Probably better as 'PCI hierarchy walk'.
> >
> >>
> >>> Would you like me to queue this on the pci tree? If yes I can
> >>> tweak the comment myself, no need to repost.
> >>
> >> Yes, please. Your tree is fine. Thanks!
> >>
> >
> > OK, I'll apply this.
> > Thanks!
>
>
> I could not see this patch in the pull request you send today. Something
> happened to the patch what I should fix? Thanks!
>
Nope, I did rebases and moved this out to the not-ready part
by mistake.
But it's still on my tree so it will get there in the next pull,
I plan it about next Sunday.
>
>
> --
> Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-27 5:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-09 15:09 [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space Alexey Kardashevskiy
2013-08-11 13:58 ` Michael S. Tsirkin
2013-08-11 14:36 ` Alexey Kardashevskiy
2013-08-12 9:02 ` Michael S. Tsirkin
2013-08-27 5:01 ` Alexey Kardashevskiy
2013-08-27 5:32 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).