* [PATCH v2 00/11] PCI devices passthrough on Arm, part 3
@ 2021-09-23 12:54 Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 01/11] vpci: Make vpci registers removal a dedicated function Oleksandr Andrushchenko
` (10 more replies)
0 siblings, 11 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Hi, all!
This patch series is focusing on vPCI and adds support for non-identity
PCI BAR mappings which is required while passing through a PCI device to
a guest. The highlights are:
- Add relevant vpci register handlers when assigning PCI device to a domain
and remove those when de-assigning. This allows having different
handlers for different domains, e.g. hwdom and other guests.
- Emulate guest BAR register values based on physical BAR values.
This allows creating a guest view of the registers and emulates
size and properties probe as it is done during PCI device enumeration by
the guest.
- Instead of handling a single range set, that contains all the memory
regions of all the BARs and ROM, have them per BAR.
- Take into account guest's BAR view and program its p2m accordingly:
gfn is guest's view of the BAR and mfn is the physical BAR value as set
up by the host bridge in the hardware domain.
This way hardware doamin sees physical BAR values and guest sees
emulated ones.
The series also adds support for virtual PCI bus topology for guests:
- We emulate a single host bridge for the guest, so segment is always 0.
- The implementation is limited to 32 devices which are allowed on
a single PCI bus.
- The virtual bus number is set to 0, so virtual devices are seen
as embedded endpoints behind the root complex.
The series was also tested on x86 PVH Dom0 and doesn't break it.
Thank you,
Oleksandr
Oleksandr Andrushchenko (11):
vpci: Make vpci registers removal a dedicated function
vpci: Add hooks for PCI device assign/de-assign
vpci/header: Move register assignments from init_bars
vpci/header: Add and remove register handlers dynamically
vpci/header: Implement guest BAR register handlers
vpci/header: Handle p2m range sets per BAR
vpci/header: program p2m with guest BAR view
vpci/header: Emulate PCI_COMMAND register for guests
vpci/header: Reset the command register when adding devices
vpci: Add initial support for virtual PCI bus topology
xen/arm: Translate virtual PCI bus topology for guests
xen/arch/arm/domain.c | 1 +
xen/arch/arm/vpci.c | 87 ++++++-
xen/arch/arm/vpci.h | 3 +
xen/common/domain.c | 1 +
xen/drivers/Kconfig | 4 +
xen/drivers/passthrough/pci.c | 91 ++++++++
xen/drivers/vpci/header.c | 412 +++++++++++++++++++++++++++-------
xen/drivers/vpci/vpci.c | 48 +++-
xen/include/asm-arm/pci.h | 1 +
xen/include/xen/pci.h | 19 ++
xen/include/xen/sched.h | 8 +
xen/include/xen/vpci.h | 34 ++-
12 files changed, 617 insertions(+), 92 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2 01/11] vpci: Make vpci registers removal a dedicated function
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-28 11:15 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 02/11] vpci: Add hooks for PCI device assign/de-assign Oleksandr Andrushchenko
` (9 subsequent siblings)
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
This is in preparation for dynamic assignment of the vpci register
handlers depending on the domain: hwdom or guest.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v1:
- constify struct pci_dev where possible
---
xen/drivers/vpci/vpci.c | 7 ++++++-
xen/include/xen/vpci.h | 2 ++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index cbd1bac7fc33..1666402d55b8 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -35,7 +35,7 @@ extern vpci_register_init_t *const __start_vpci_array[];
extern vpci_register_init_t *const __end_vpci_array[];
#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
-void vpci_remove_device(struct pci_dev *pdev)
+void vpci_remove_device_registers(const struct pci_dev *pdev)
{
spin_lock(&pdev->vpci->lock);
while ( !list_empty(&pdev->vpci->handlers) )
@@ -48,6 +48,11 @@ void vpci_remove_device(struct pci_dev *pdev)
xfree(r);
}
spin_unlock(&pdev->vpci->lock);
+}
+
+void vpci_remove_device(struct pci_dev *pdev)
+{
+ vpci_remove_device_registers(pdev);
xfree(pdev->vpci->msix);
xfree(pdev->vpci->msi);
xfree(pdev->vpci);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 9f5b5d52e159..2e910d0b1f90 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -28,6 +28,8 @@ int __must_check vpci_add_handlers(struct pci_dev *dev);
/* Remove all handlers and free vpci related structures. */
void vpci_remove_device(struct pci_dev *pdev);
+/* Remove all handlers for the device given. */
+void vpci_remove_device_registers(const struct pci_dev *pdev);
/* Add/remove a register handler. */
int __must_check vpci_add_register(struct vpci *vpci,
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 02/11] vpci: Add hooks for PCI device assign/de-assign
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 01/11] vpci: Make vpci registers removal a dedicated function Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-29 9:35 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 03/11] vpci/header: Move register assignments from init_bars Oleksandr Andrushchenko
` (8 subsequent siblings)
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
When a PCI device gets assigned/de-assigned some work on vPCI side needs
to be done for that device. Introduce a pair of hooks so vPCI can handle
that.
Please note, that in the current design the error path is handled by
the toolstack via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device,
so this is why it is acceptable not to de-assign devices if vPCI's
assign fails, e.g. the roll back will be handled on deassign_device when
it is called by the toolstack.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v1:
- constify struct pci_dev where possible
- do not open code is_system_domain()
- extended the commit message
---
xen/drivers/passthrough/pci.c | 9 +++++++++
xen/drivers/vpci/vpci.c | 21 +++++++++++++++++++++
xen/include/xen/vpci.h | 18 ++++++++++++++++++
3 files changed, 48 insertions(+)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index fc3469bc12dc..e1da283d73ad 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -872,6 +872,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
if ( ret )
goto out;
+ ret = vpci_deassign_device(d, pdev);
+ if ( ret )
+ goto out;
+
if ( pdev->domain == hardware_domain )
pdev->quarantine = false;
@@ -1431,6 +1435,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag);
}
+ if ( rc )
+ goto done;
+
+ rc = vpci_assign_device(d, pdev);
+
done:
if ( rc )
printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1666402d55b8..a8fed3d2c42e 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -86,6 +86,27 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
return rc;
}
+
+/* Notify vPCI that device is assigned to guest. */
+int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
+{
+ /* It only makes sense to assign for hwdom or guest domain. */
+ if ( is_system_domain(d) || !has_vpci(d) )
+ return 0;
+
+ return 0;
+}
+
+/* Notify vPCI that device is de-assigned from guest. */
+int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
+{
+ /* It only makes sense to de-assign from hwdom or guest domain. */
+ if ( is_system_domain(d) || !has_vpci(d) )
+ return 0;
+
+ return 0;
+}
+
#endif /* __XEN__ */
static int vpci_register_cmp(const struct vpci_register *r1,
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 2e910d0b1f90..b9485b2aea1b 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -26,6 +26,12 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
/* Add vPCI handlers to device. */
int __must_check vpci_add_handlers(struct pci_dev *dev);
+/* Notify vPCI that device is assigned/de-assigned to/from guest. */
+int __must_check vpci_assign_device(struct domain *d,
+ const struct pci_dev *dev);
+int __must_check vpci_deassign_device(struct domain *d,
+ const struct pci_dev *dev);
+
/* Remove all handlers and free vpci related structures. */
void vpci_remove_device(struct pci_dev *pdev);
/* Remove all handlers for the device given. */
@@ -220,6 +226,18 @@ static inline int vpci_add_handlers(struct pci_dev *pdev)
return 0;
}
+static inline int vpci_assign_device(struct domain *d,
+ const struct pci_dev *dev)
+{
+ return 0;
+};
+
+static inline int vpci_deassign_device(struct domain *d,
+ const struct pci_dev *dev)
+{
+ return 0;
+};
+
static inline void vpci_dump_msi(void) { }
static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 03/11] vpci/header: Move register assignments from init_bars
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 01/11] vpci: Make vpci registers removal a dedicated function Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 02/11] vpci: Add hooks for PCI device assign/de-assign Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 04/11] vpci/header: Add and remove register handlers dynamically Oleksandr Andrushchenko
` (7 subsequent siblings)
10 siblings, 0 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
This is in preparation for dynamic assignment of the vPCI register
handlers depending on the domain: hwdom or guest.
The need for this step is that it is easier to have all related functionality
put at one place. When the subsequent patches add decisions on which
handlers to install, e.g. hwdom or guest handlers, then this is easily
achievable.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v1:
- constify struct pci_dev where possible
- extend patch description
---
xen/drivers/vpci/header.c | 83 ++++++++++++++++++++++++++-------------
1 file changed, 56 insertions(+), 27 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index f8cd55e7c024..3d571356397a 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -445,6 +445,55 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
rom->addr = val & PCI_ROM_ADDRESS_MASK;
}
+static int add_bar_handlers(const struct pci_dev *pdev)
+{
+ unsigned int i;
+ struct vpci_header *header = &pdev->vpci->header;
+ struct vpci_bar *bars = header->bars;
+ int rc;
+
+ /* Setup a handler for the command register. */
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
+ 2, header);
+ if ( rc )
+ return rc;
+
+ if ( pdev->ignore_bars )
+ return 0;
+
+ for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS + 1; i++ )
+ {
+ if ( (bars[i].type == VPCI_BAR_IO) || (bars[i].type == VPCI_BAR_EMPTY) )
+ continue;
+
+ if ( bars[i].type == VPCI_BAR_ROM )
+ {
+ unsigned int rom_reg;
+ uint8_t header_type = pci_conf_read8(pdev->sbdf,
+ PCI_HEADER_TYPE) & 0x7f;
+ if ( header_type == PCI_HEADER_TYPE_NORMAL )
+ rom_reg = PCI_ROM_ADDRESS;
+ else
+ rom_reg = PCI_ROM_ADDRESS1;
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
+ rom_reg, 4, &bars[i]);
+ if ( rc )
+ return rc;
+ }
+ else
+ {
+ uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
+
+ /* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
+ 4, &bars[i]);
+ if ( rc )
+ return rc;
+ }
+ }
+ return 0;
+}
+
static int init_bars(struct pci_dev *pdev)
{
uint16_t cmd;
@@ -470,14 +519,8 @@ static int init_bars(struct pci_dev *pdev)
return -EOPNOTSUPP;
}
- /* Setup a handler for the command register. */
- rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
- 2, header);
- if ( rc )
- return rc;
-
if ( pdev->ignore_bars )
- return 0;
+ return add_bar_handlers(pdev);
/* Disable memory decoding before sizing. */
cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
@@ -492,14 +535,6 @@ static int init_bars(struct pci_dev *pdev)
if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO )
{
bars[i].type = VPCI_BAR_MEM64_HI;
- rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
- 4, &bars[i]);
- if ( rc )
- {
- pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
- return rc;
- }
-
continue;
}
@@ -532,14 +567,6 @@ static int init_bars(struct pci_dev *pdev)
bars[i].addr = addr;
bars[i].size = size;
bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
-
- rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, 4,
- &bars[i]);
- if ( rc )
- {
- pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
- return rc;
- }
}
/* Check expansion ROM. */
@@ -553,11 +580,13 @@ static int init_bars(struct pci_dev *pdev)
rom->addr = addr;
header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) &
PCI_ROM_ADDRESS_ENABLE;
+ }
- rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, rom_reg,
- 4, rom);
- if ( rc )
- rom->type = VPCI_BAR_EMPTY;
+ rc = add_bar_handlers(pdev);
+ if ( rc )
+ {
+ pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
+ return rc;
}
return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 04/11] vpci/header: Add and remove register handlers dynamically
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (2 preceding siblings ...)
2021-09-23 12:54 ` [PATCH v2 03/11] vpci/header: Move register assignments from init_bars Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 05/11] vpci/header: Implement guest BAR register handlers Oleksandr Andrushchenko
` (6 subsequent siblings)
10 siblings, 0 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Add relevant vpci register handlers when assigning PCI device to a domain
and remove those when de-assigning. This allows having different
handlers for different domains, e.g. hwdom and other guests.
Use stubs for guest domains for now.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v1:
- constify struct pci_dev where possible
- do not open code is_system_domain()
- simplify some code3. simplify
- use gdprintk + error code instead of gprintk
- gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT,
so these do not get compiled for x86
- removed unneeded is_system_domain check
---
xen/drivers/Kconfig | 4 +++
xen/drivers/vpci/header.c | 72 ++++++++++++++++++++++++++++++++++-----
xen/drivers/vpci/vpci.c | 8 +++++
xen/include/xen/vpci.h | 8 +++++
4 files changed, 84 insertions(+), 8 deletions(-)
diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
index db94393f47a6..f3e3a05a4544 100644
--- a/xen/drivers/Kconfig
+++ b/xen/drivers/Kconfig
@@ -15,4 +15,8 @@ source "drivers/video/Kconfig"
config HAS_VPCI
bool
+config HAS_VPCI_GUEST_SUPPORT
+ bool
+ depends on HAS_PCI
+
endmenu
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 3d571356397a..1ce98795fcca 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -397,6 +397,17 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg,
pci_conf_write32(pdev->sbdf, reg, val);
}
+static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
+ uint32_t val, void *data)
+{
+}
+
+static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
+ void *data)
+{
+ return 0xffffffff;
+}
+
static void rom_write(const struct pci_dev *pdev, unsigned int reg,
uint32_t val, void *data)
{
@@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
rom->addr = val & PCI_ROM_ADDRESS_MASK;
}
-static int add_bar_handlers(const struct pci_dev *pdev)
+static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
+ uint32_t val, void *data)
+{
+}
+
+static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
+ void *data)
+{
+ return 0xffffffff;
+}
+
+static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
{
unsigned int i;
struct vpci_header *header = &pdev->vpci->header;
struct vpci_bar *bars = header->bars;
int rc;
- /* Setup a handler for the command register. */
+ /* Setup a handler for the command register: same for hwdom and guests. */
rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
2, header);
if ( rc )
@@ -475,8 +497,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
rom_reg = PCI_ROM_ADDRESS;
else
rom_reg = PCI_ROM_ADDRESS1;
- rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
- rom_reg, 4, &bars[i]);
+ if ( is_hwdom )
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
+ rom_reg, 4, &bars[i]);
+ else
+ rc = vpci_add_register(pdev->vpci,
+ guest_rom_read, guest_rom_write,
+ rom_reg, 4, &bars[i]);
if ( rc )
return rc;
}
@@ -485,8 +512,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
/* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
- rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
- 4, &bars[i]);
+ if ( is_hwdom )
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
+ reg, 4, &bars[i]);
+ else
+ rc = vpci_add_register(pdev->vpci,
+ guest_bar_read, guest_bar_write,
+ reg, 4, &bars[i]);
if ( rc )
return rc;
}
@@ -520,7 +552,7 @@ static int init_bars(struct pci_dev *pdev)
}
if ( pdev->ignore_bars )
- return add_bar_handlers(pdev);
+ return add_bar_handlers(pdev, true);
/* Disable memory decoding before sizing. */
cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
@@ -582,7 +614,7 @@ static int init_bars(struct pci_dev *pdev)
PCI_ROM_ADDRESS_ENABLE;
}
- rc = add_bar_handlers(pdev);
+ rc = add_bar_handlers(pdev, true);
if ( rc )
{
pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
@@ -593,6 +625,30 @@ static int init_bars(struct pci_dev *pdev)
}
REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
+{
+ int rc;
+
+ /* Remove previously added registers. */
+ vpci_remove_device_registers(pdev);
+
+ rc = add_bar_handlers(pdev, is_hardware_domain(d));
+ if ( rc )
+ gdprintk(XENLOG_ERR,
+ "%pp: failed to add BAR handlers for dom%pd: %d\n",
+ &pdev->sbdf, d, rc);
+ return rc;
+}
+
+int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev *pdev)
+{
+ /* Remove previously added registers. */
+ vpci_remove_device_registers(pdev);
+ return 0;
+}
+#endif
+
/*
* Local variables:
* mode: C
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index a8fed3d2c42e..0cdc0c3f75c4 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -94,7 +94,11 @@ int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
if ( is_system_domain(d) || !has_vpci(d) )
return 0;
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+ return vpci_bar_add_handlers(d, dev);
+#else
return 0;
+#endif
}
/* Notify vPCI that device is de-assigned from guest. */
@@ -104,7 +108,11 @@ int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
if ( is_system_domain(d) || !has_vpci(d) )
return 0;
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+ return vpci_bar_remove_handlers(d, dev);
+#else
return 0;
+#endif
}
#endif /* __XEN__ */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index b9485b2aea1b..912cbc6aa7ad 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -63,6 +63,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
*/
bool __must_check vpci_process_pending(struct vcpu *v);
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+/* Add/remove BAR handlers for a domain. */
+int vpci_bar_add_handlers(const struct domain *d,
+ const struct pci_dev *pdev);
+int vpci_bar_remove_handlers(const struct domain *d,
+ const struct pci_dev *pdev);
+#endif
+
struct vpci {
/* List of vPCI handlers for a device. */
struct list_head handlers;
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 05/11] vpci/header: Implement guest BAR register handlers
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (3 preceding siblings ...)
2021-09-23 12:54 ` [PATCH v2 04/11] vpci/header: Add and remove register handlers dynamically Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-29 9:27 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 06/11] vpci/header: Handle p2m range sets per BAR Oleksandr Andrushchenko
` (5 subsequent siblings)
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Emulate guest BAR register values: this allows creating a guest view
of the registers and emulates size and properties probe as it is done
during PCI device enumeration by the guest.
ROM BAR is only handled for the hardware domain and for guest domains
there is a stub: at the moment PCI expansion ROM is x86 only, so it
might not be used by other architectures without emulating x86. Other
use-cases may include using that expansion ROM before Xen boots, hence
no emulation is needed in Xen itself. Or when a guest wants to use the
ROM code which seems to be rare.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v1:
- re-work guest read/write to be much simpler and do more work on write
than read which is expected to be called more frequently
- removed one too obvious comment
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
xen/drivers/vpci/header.c | 30 +++++++++++++++++++++++++++++-
xen/include/xen/vpci.h | 3 +++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 1ce98795fcca..ec4d215f36ff 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -400,12 +400,38 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg,
static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
uint32_t val, void *data)
{
+ struct vpci_bar *bar = data;
+ bool hi = false;
+
+ if ( bar->type == VPCI_BAR_MEM64_HI )
+ {
+ ASSERT(reg > PCI_BASE_ADDRESS_0);
+ bar--;
+ hi = true;
+ }
+ else
+ {
+ val &= PCI_BASE_ADDRESS_MEM_MASK;
+ val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32
+ : PCI_BASE_ADDRESS_MEM_TYPE_64;
+ val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
+ }
+
+ bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
+ bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
+
+ bar->guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK;
}
static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
void *data)
{
- return 0xffffffff;
+ const struct vpci_bar *bar = data;
+
+ if ( bar->type == VPCI_BAR_MEM64_HI )
+ return bar->guest_addr >> 32;
+
+ return bar->guest_addr;
}
static void rom_write(const struct pci_dev *pdev, unsigned int reg,
@@ -522,6 +548,8 @@ static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
if ( rc )
return rc;
}
+
+ bars[i].guest_addr = 0;
}
return 0;
}
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 912cbc6aa7ad..9eaf99f356fe 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -81,7 +81,10 @@ struct vpci {
struct vpci_header {
/* Information about the PCI BARs of this device. */
struct vpci_bar {
+ /* Physical view of the BAR. */
uint64_t addr;
+ /* Guest view of the BAR. */
+ uint64_t guest_addr;
uint64_t size;
enum {
VPCI_BAR_EMPTY,
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 06/11] vpci/header: Handle p2m range sets per BAR
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (4 preceding siblings ...)
2021-09-23 12:54 ` [PATCH v2 05/11] vpci/header: Implement guest BAR register handlers Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 07/11] vpci/header: program p2m with guest BAR view Oleksandr Andrushchenko
` (4 subsequent siblings)
10 siblings, 0 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Instead of handling a single range set, that contains all the memory
regions of all the BARs and ROM, have them per BAR.
This is in preparation of making non-identity mappings in p2m for the
MMIOs/ROM.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
xen/drivers/vpci/header.c | 172 ++++++++++++++++++++++++++------------
xen/include/xen/vpci.h | 3 +-
2 files changed, 122 insertions(+), 53 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index ec4d215f36ff..9c603d26d302 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -131,49 +131,75 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
bool vpci_process_pending(struct vcpu *v)
{
- if ( v->vpci.mem )
+ if ( v->vpci.num_mem_ranges )
{
struct map_data data = {
.d = v->domain,
.map = v->vpci.cmd & PCI_COMMAND_MEMORY,
};
- int rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
+ struct pci_dev *pdev = v->vpci.pdev;
+ struct vpci_header *header = &pdev->vpci->header;
+ unsigned int i;
- if ( rc == -ERESTART )
- return true;
+ for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+ {
+ struct vpci_bar *bar = &header->bars[i];
+ int rc;
- spin_lock(&v->vpci.pdev->vpci->lock);
- /* Disable memory decoding unconditionally on failure. */
- modify_decoding(v->vpci.pdev,
- rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
- !rc && v->vpci.rom_only);
- spin_unlock(&v->vpci.pdev->vpci->lock);
+ if ( !bar->mem )
+ continue;
- rangeset_destroy(v->vpci.mem);
- v->vpci.mem = NULL;
- if ( rc )
- /*
- * FIXME: in case of failure remove the device from the domain.
- * Note that there might still be leftover mappings. While this is
- * safe for Dom0, for DomUs the domain will likely need to be
- * killed in order to avoid leaking stale p2m mappings on
- * failure.
- */
- vpci_remove_device(v->vpci.pdev);
+ rc = rangeset_consume_ranges(bar->mem, map_range, &data);
+
+ if ( rc == -ERESTART )
+ return true;
+
+ spin_lock(&pdev->vpci->lock);
+ /* Disable memory decoding unconditionally on failure. */
+ modify_decoding(pdev,
+ rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
+ !rc && v->vpci.rom_only);
+ spin_unlock(&pdev->vpci->lock);
+
+ rangeset_destroy(bar->mem);
+ bar->mem = NULL;
+ v->vpci.num_mem_ranges--;
+ if ( rc )
+ /*
+ * FIXME: in case of failure remove the device from the domain.
+ * Note that there might still be leftover mappings. While this is
+ * safe for Dom0, for DomUs the domain will likely need to be
+ * killed in order to avoid leaking stale p2m mappings on
+ * failure.
+ */
+ vpci_remove_device(pdev);
+ }
}
return false;
}
static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
- struct rangeset *mem, uint16_t cmd)
+ uint16_t cmd)
{
struct map_data data = { .d = d, .map = true };
- int rc;
+ struct vpci_header *header = &pdev->vpci->header;
+ int rc = 0;
+ unsigned int i;
+
+ for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+ {
+ struct vpci_bar *bar = &header->bars[i];
- while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) == -ERESTART )
- process_pending_softirqs();
- rangeset_destroy(mem);
+ if ( !bar->mem )
+ continue;
+
+ while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
+ &data)) == -ERESTART )
+ process_pending_softirqs();
+ rangeset_destroy(bar->mem);
+ bar->mem = NULL;
+ }
if ( !rc )
modify_decoding(pdev, cmd, false);
@@ -181,7 +207,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
}
static void defer_map(struct domain *d, struct pci_dev *pdev,
- struct rangeset *mem, uint16_t cmd, bool rom_only)
+ uint16_t cmd, bool rom_only, uint8_t num_mem_ranges)
{
struct vcpu *curr = current;
@@ -192,9 +218,9 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
* started for the same device if the domain is not well-behaved.
*/
curr->vpci.pdev = pdev;
- curr->vpci.mem = mem;
curr->vpci.cmd = cmd;
curr->vpci.rom_only = rom_only;
+ curr->vpci.num_mem_ranges = num_mem_ranges;
/*
* Raise a scheduler softirq in order to prevent the guest from resuming
* execution with pending mapping operations, to trigger the invocation
@@ -206,42 +232,47 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
{
struct vpci_header *header = &pdev->vpci->header;
- struct rangeset *mem = rangeset_new(NULL, NULL, 0);
struct pci_dev *tmp, *dev = NULL;
const struct vpci_msix *msix = pdev->vpci->msix;
- unsigned int i;
+ unsigned int i, j;
int rc;
-
- if ( !mem )
- return -ENOMEM;
+ uint8_t num_mem_ranges;
/*
- * Create a rangeset that represents the current device BARs memory region
+ * Create a rangeset per BAR that represents the current device memory region
* and compare it against all the currently active BAR memory regions. If
* an overlap is found, subtract it from the region to be mapped/unmapped.
*
- * First fill the rangeset with all the BARs of this device or with the ROM
+ * First fill the rangesets with all the BARs of this device or with the ROM
* BAR only, depending on whether the guest is toggling the memory decode
* bit of the command register, or the enable bit of the ROM BAR register.
*/
for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
{
- const struct vpci_bar *bar = &header->bars[i];
+ struct vpci_bar *bar = &header->bars[i];
unsigned long start = PFN_DOWN(bar->addr);
unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
+ bar->mem = NULL;
+
if ( !MAPPABLE_BAR(bar) ||
(rom_only ? bar->type != VPCI_BAR_ROM
: (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) )
continue;
- rc = rangeset_add_range(mem, start, end);
+ bar->mem = rangeset_new(NULL, NULL, 0);
+ if ( !bar->mem )
+ {
+ rc = -ENOMEM;
+ goto fail;
+ }
+
+ rc = rangeset_add_range(bar->mem, start, end);
if ( rc )
{
printk(XENLOG_G_WARNING "Failed to add [%lx, %lx]: %d\n",
start, end, rc);
- rangeset_destroy(mem);
- return rc;
+ goto fail;
}
}
@@ -252,14 +283,21 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
vmsix_table_size(pdev->vpci, i) - 1);
- rc = rangeset_remove_range(mem, start, end);
- if ( rc )
+ for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
{
- printk(XENLOG_G_WARNING
- "Failed to remove MSIX table [%lx, %lx]: %d\n",
- start, end, rc);
- rangeset_destroy(mem);
- return rc;
+ const struct vpci_bar *bar = &header->bars[j];
+
+ if ( !bar->mem )
+ continue;
+
+ rc = rangeset_remove_range(bar->mem, start, end);
+ if ( rc )
+ {
+ printk(XENLOG_G_WARNING
+ "Failed to remove MSIX table [%lx, %lx]: %d\n",
+ start, end, rc);
+ goto fail;
+ }
}
}
@@ -291,7 +329,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
unsigned long start = PFN_DOWN(bar->addr);
unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
- if ( !bar->enabled || !rangeset_overlaps_range(mem, start, end) ||
+ if ( !bar->enabled ||
+ !rangeset_overlaps_range(bar->mem, start, end) ||
/*
* If only the ROM enable bit is toggled check against other
* BARs in the same device for overlaps, but not against the
@@ -300,13 +339,12 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
(rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
continue;
- rc = rangeset_remove_range(mem, start, end);
+ rc = rangeset_remove_range(bar->mem, start, end);
if ( rc )
{
printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
start, end, rc);
- rangeset_destroy(mem);
- return rc;
+ goto fail;
}
}
}
@@ -324,12 +362,42 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
* will always be to establish mappings and process all the BARs.
*/
ASSERT((cmd & PCI_COMMAND_MEMORY) && !rom_only);
- return apply_map(pdev->domain, pdev, mem, cmd);
+ return apply_map(pdev->domain, pdev, cmd);
}
- defer_map(dev->domain, dev, mem, cmd, rom_only);
+ /* Find out how many memory ranges has left after MSI and overlaps. */
+ num_mem_ranges = 0;
+ for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+ {
+ struct vpci_bar *bar = &header->bars[i];
+
+ if ( !rangeset_is_empty(bar->mem) )
+ num_mem_ranges++;
+ }
+
+ /*
+ * There are cases when PCI device, root port for example, has neither
+ * memory space nor IO. In this case PCI command register write is
+ * missed resulting in the underlying PCI device not functional, so:
+ * - if there are no regions write the command register now
+ * - if there are regions then defer work and write later on
+ */
+ if ( !num_mem_ranges )
+ pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
+ else
+ defer_map(dev->domain, dev, cmd, rom_only, num_mem_ranges);
return 0;
+
+fail:
+ for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+ {
+ struct vpci_bar *bar = &header->bars[i];
+
+ rangeset_destroy(bar->mem);
+ bar->mem = NULL;
+ }
+ return rc;
}
static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 9eaf99f356fe..3696c73a4237 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -86,6 +86,7 @@ struct vpci {
/* Guest view of the BAR. */
uint64_t guest_addr;
uint64_t size;
+ struct rangeset *mem;
enum {
VPCI_BAR_EMPTY,
VPCI_BAR_IO,
@@ -160,9 +161,9 @@ struct vpci {
struct vpci_vcpu {
/* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
- struct rangeset *mem;
struct pci_dev *pdev;
uint16_t cmd;
+ uint8_t num_mem_ranges;
bool rom_only : 1;
};
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 07/11] vpci/header: program p2m with guest BAR view
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (5 preceding siblings ...)
2021-09-23 12:54 ` [PATCH v2 06/11] vpci/header: Handle p2m range sets per BAR Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-29 8:13 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 08/11] vpci/header: Emulate PCI_COMMAND register for guests Oleksandr Andrushchenko
` (3 subsequent siblings)
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Take into account guest's BAR view and program its p2m accordingly:
gfn is guest's view of the BAR and mfn is the physical BAR value as set
up by the host bridge in the hardware domain.
This way hardware doamin sees physical BAR values and guest sees
emulated ones.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v1:
- s/MSI/MSI-X in comments
---
xen/drivers/vpci/header.c | 35 +++++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 9c603d26d302..bdd18599b205 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -30,6 +30,10 @@
struct map_data {
struct domain *d;
+ /* Start address of the BAR as seen by the guest. */
+ gfn_t start_gfn;
+ /* Physical start address of the BAR. */
+ mfn_t start_mfn;
bool map;
};
@@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data,
unsigned long *c)
{
const struct map_data *map = data;
+ gfn_t start_gfn;
int rc;
for ( ; ; )
{
unsigned long size = e - s + 1;
+ /*
+ * Any BAR may have holes in its memory we want to map, e.g.
+ * we don't want to map MSI-X regions which may be a part of that BAR,
+ * e.g. when a single BAR is used for both MMIO and MSI-X.
+ * In this case MSI-X regions are subtracted from the mapping, but
+ * map->start_gfn still points to the very beginning of the BAR.
+ * So if there is a hole present then we need to adjust start_gfn
+ * to reflect the fact of that substraction.
+ */
+ start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn));
+
+ printk(XENLOG_G_DEBUG
+ "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n",
+ map->map ? "" : "un", s, e, gfn_x(start_gfn),
+ map->d->domain_id);
/*
* ARM TODOs:
* - On ARM whether the memory is prefetchable or not should be passed
@@ -52,8 +72,10 @@ static int map_range(unsigned long s, unsigned long e, void *data,
* - {un}map_mmio_regions doesn't support preemption.
*/
- rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
- : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
+ rc = map->map ? map_mmio_regions(map->d, start_gfn,
+ size, _mfn(s))
+ : unmap_mmio_regions(map->d, start_gfn,
+ size, _mfn(s));
if ( rc == 0 )
{
*c += size;
@@ -69,6 +91,7 @@ static int map_range(unsigned long s, unsigned long e, void *data,
ASSERT(rc < size);
*c += rc;
s += rc;
+ gfn_add(map->start_gfn, rc);
if ( general_preempt_check() )
return -ERESTART;
}
@@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
if ( !bar->mem )
continue;
+ data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
+ _gfn(PFN_DOWN(bar->addr)) :
+ _gfn(PFN_DOWN(bar->guest_addr));
+ data.start_mfn = _mfn(PFN_DOWN(bar->addr));
rc = rangeset_consume_ranges(bar->mem, map_range, &data);
if ( rc == -ERESTART )
@@ -194,6 +221,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
if ( !bar->mem )
continue;
+ data.start_gfn = is_hardware_domain(d) ?
+ _gfn(PFN_DOWN(bar->addr)) :
+ _gfn(PFN_DOWN(bar->guest_addr));
+ data.start_mfn = _mfn(PFN_DOWN(bar->addr));
while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
&data)) == -ERESTART )
process_pending_softirqs();
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 08/11] vpci/header: Emulate PCI_COMMAND register for guests
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (6 preceding siblings ...)
2021-09-23 12:54 ` [PATCH v2 07/11] vpci/header: program p2m with guest BAR view Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-28 7:34 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 09/11] vpci/header: Reset the command register when adding devices Oleksandr Andrushchenko
` (2 subsequent siblings)
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Add basic emulation support for guests. At the moment only emulate
PCI_COMMAND_INTX_DISABLE bit, the rest is not emulated yet and left
as TODO.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
New in v2
---
xen/drivers/vpci/header.c | 35 ++++++++++++++++++++++++++++++++---
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index bdd18599b205..99f9c37dfb00 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -452,6 +452,32 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
pci_conf_write16(pdev->sbdf, reg, cmd);
}
+static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
+ uint32_t cmd, void *data)
+{
+ /* TODO: Add proper emulation for all bits of the command register. */
+
+ if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 )
+ {
+ /*
+ * Guest wants to enable INTx. It can't be enabled if:
+ * - host has INTx disabled
+ * - MSI/MSI-X enabled
+ */
+ if ( pdev->vpci->msi->enabled )
+ cmd |= PCI_COMMAND_INTX_DISABLE;
+ else
+ {
+ uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
+
+ if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
+ cmd |= PCI_COMMAND_INTX_DISABLE;
+ }
+ }
+
+ cmd_write(pdev, reg, cmd, data);
+}
+
static void bar_write(const struct pci_dev *pdev, unsigned int reg,
uint32_t val, void *data)
{
@@ -599,9 +625,12 @@ static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
struct vpci_bar *bars = header->bars;
int rc;
- /* Setup a handler for the command register: same for hwdom and guests. */
- rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
- 2, header);
+ if ( is_hwdom )
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write,
+ PCI_COMMAND, 2, header);
+ else
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write,
+ PCI_COMMAND, 2, header);
if ( rc )
return rc;
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 09/11] vpci/header: Reset the command register when adding devices
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (7 preceding siblings ...)
2021-09-23 12:54 ` [PATCH v2 08/11] vpci/header: Emulate PCI_COMMAND register for guests Oleksandr Andrushchenko
@ 2021-09-23 12:54 ` Oleksandr Andrushchenko
2021-09-28 7:38 ` Michal Orzel
2021-09-23 12:55 ` [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology Oleksandr Andrushchenko
2021-09-23 12:55 ` [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests Oleksandr Andrushchenko
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:54 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reset the command register when passing through a PCI device:
it is possible that when passing through a PCI device its memory
decoding bits in the command register are already set. Thus, a
guest OS may not write to the command register to update memory
decoding, so guest mappings (guest's view of the BARs) are
left not updated.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
Since v1:
- do not write 0 to the command register, but respect host settings.
---
xen/drivers/vpci/header.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 99f9c37dfb00..b2829b9d206b 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -452,8 +452,7 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
pci_conf_write16(pdev->sbdf, reg, cmd);
}
-static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
- uint32_t cmd, void *data)
+static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd)
{
/* TODO: Add proper emulation for all bits of the command register. */
@@ -468,14 +467,20 @@ static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
cmd |= PCI_COMMAND_INTX_DISABLE;
else
{
- uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
+ uint16_t current_cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
cmd |= PCI_COMMAND_INTX_DISABLE;
}
}
- cmd_write(pdev, reg, cmd, data);
+ return cmd;
+}
+
+static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
+ uint32_t cmd, void *data)
+{
+ cmd_write(pdev, reg, emulate_cmd_reg(pdev, cmd), data);
}
static void bar_write(const struct pci_dev *pdev, unsigned int reg,
@@ -794,6 +799,10 @@ int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
gdprintk(XENLOG_ERR,
"%pp: failed to add BAR handlers for dom%pd: %d\n",
&pdev->sbdf, d, rc);
+
+ /* Reset the command register with respect to host settings. */
+ pci_conf_write16(pdev->sbdf, PCI_COMMAND, emulate_cmd_reg(pdev, 0));
+
return rc;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (8 preceding siblings ...)
2021-09-23 12:54 ` [PATCH v2 09/11] vpci/header: Reset the command register when adding devices Oleksandr Andrushchenko
@ 2021-09-23 12:55 ` Oleksandr Andrushchenko
2021-09-28 7:48 ` Michal Orzel
2021-09-23 12:55 ` [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests Oleksandr Andrushchenko
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:55 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Assign SBDF to the PCI devices being passed through with bus 0.
The resulting topology is where PCIe devices reside on the bus 0 of the
root complex itself (embedded endpoints).
This implementation is limited to 32 devices which are allowed on
a single PCI bus.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
New in v2
---
xen/common/domain.c | 1 +
xen/drivers/passthrough/pci.c | 57 +++++++++++++++++++++++++++++++++++
xen/drivers/vpci/vpci.c | 18 +++++++++--
xen/include/xen/pci.h | 18 +++++++++++
xen/include/xen/sched.h | 6 ++++
5 files changed, 97 insertions(+), 3 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 40d67ec34232..b80ff2e5e2e6 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -601,6 +601,7 @@ struct domain *domain_create(domid_t domid,
#ifdef CONFIG_HAS_PCI
INIT_LIST_HEAD(&d->pdev_list);
+ INIT_LIST_HEAD(&d->vdev_list);
#endif
/* All error paths can depend on the above setup. */
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index e1da283d73ad..4552ace855e0 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -833,6 +833,63 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
return ret;
}
+static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
+ const struct pci_dev *pdev)
+{
+ struct vpci_dev *vdev;
+
+ list_for_each_entry ( vdev, &d->vdev_list, list )
+ if ( vdev->pdev == pdev )
+ return vdev;
+ return NULL;
+}
+
+int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
+{
+ struct vpci_dev *vdev;
+
+ ASSERT(!pci_find_virtual_device(d, pdev));
+
+ /* Each PCI bus supports 32 devices/slots at max. */
+ if ( d->vpci_dev_next > 31 )
+ return -ENOSPC;
+
+ vdev = xzalloc(struct vpci_dev);
+ if ( !vdev )
+ return -ENOMEM;
+
+ /* We emulate a single host bridge for the guest, so segment is always 0. */
+ *(u16*) &vdev->seg = 0;
+ /*
+ * The bus number is set to 0, so virtual devices are seen
+ * as embedded endpoints behind the root complex.
+ */
+ *((u8*) &vdev->bus) = 0;
+ *((u8*) &vdev->devfn) = PCI_DEVFN(d->vpci_dev_next++, 0);
+
+ vdev->pdev = pdev;
+ vdev->domain = d;
+
+ pcidevs_lock();
+ list_add_tail(&vdev->list, &d->vdev_list);
+ pcidevs_unlock();
+
+ return 0;
+}
+
+int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
+{
+ struct vpci_dev *vdev;
+
+ pcidevs_lock();
+ vdev = pci_find_virtual_device(d, pdev);
+ if ( vdev )
+ list_del(&vdev->list);
+ pcidevs_unlock();
+ xfree(vdev);
+ return 0;
+}
+
/* Caller should hold the pcidevs_lock */
static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
uint8_t devfn)
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 0cdc0c3f75c4..a40a138a14f7 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -90,24 +90,36 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
/* Notify vPCI that device is assigned to guest. */
int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
{
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+ int rc;
+#endif
+
/* It only makes sense to assign for hwdom or guest domain. */
if ( is_system_domain(d) || !has_vpci(d) )
return 0;
#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
- return vpci_bar_add_handlers(d, dev);
-#else
- return 0;
+ rc = vpci_bar_add_handlers(d, dev);
+ if ( rc )
+ return rc;
#endif
+
+ return pci_add_virtual_device(d, dev);
}
/* Notify vPCI that device is de-assigned from guest. */
int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
{
+ int rc;
+
/* It only makes sense to de-assign from hwdom or guest domain. */
if ( is_system_domain(d) || !has_vpci(d) )
return 0;
+ rc = pci_remove_virtual_device(d, dev);
+ if ( rc )
+ return rc;
+
#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
return vpci_bar_remove_handlers(d, dev);
#else
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 2b2dfb6f1b49..35ae1d093921 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -136,6 +136,22 @@ struct pci_dev {
struct vpci *vpci;
};
+struct vpci_dev {
+ struct list_head list;
+ /* Physical PCI device this virtual device is connected to. */
+ const struct pci_dev *pdev;
+ /* Virtual SBDF of the device. */
+ const union {
+ struct {
+ uint8_t devfn;
+ uint8_t bus;
+ uint16_t seg;
+ };
+ pci_sbdf_t sbdf;
+ };
+ struct domain *domain;
+};
+
#define for_each_pdev(domain, pdev) \
list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
@@ -165,7 +181,9 @@ int pci_add_segment(u16 seg);
const unsigned long *pci_get_ro_map(u16 seg);
int pci_add_device(u16 seg, u8 bus, u8 devfn,
const struct pci_dev_info *, nodeid_t node);
+int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev);
int pci_remove_device(u16 seg, u8 bus, u8 devfn);
+int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev);
int pci_ro_device(int seg, int bus, int devfn);
int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 28146ee404e6..d304c7ebe766 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -444,6 +444,12 @@ struct domain
#ifdef CONFIG_HAS_PCI
struct list_head pdev_list;
+ struct list_head vdev_list;
+ /*
+ * Current device number used by the virtual PCI bus topology
+ * to assign a unique SBDF to a passed through virtual PCI device.
+ */
+ int vpci_dev_next;
#endif
#ifdef CONFIG_HAS_PASSTHROUGH
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
` (9 preceding siblings ...)
2021-09-23 12:55 ` [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology Oleksandr Andrushchenko
@ 2021-09-23 12:55 ` Oleksandr Andrushchenko
2021-09-27 11:31 ` Jan Beulich
10 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-23 12:55 UTC (permalink / raw)
To: xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
There are three originators for the PCI configuration space access:
1. The domain that owns physical host bridge: MMIO handlers are
there so we can update vPCI register handlers with the values
written by the hardware domain, e.g. physical view of the registers
vs guest's view on the configuration space.
2. Guest access to the passed through PCI devices: we need to properly
map virtual bus topology to the physical one, e.g. pass the configuration
space access to the corresponding physical devices.
3. Emulated host PCI bridge access. It doesn't exist in the physical
topology, e.g. it can't be mapped to some physical host bridge.
So, all access to the host bridge itself needs to be trapped and
emulated.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
New in v2
---
xen/arch/arm/domain.c | 1 +
xen/arch/arm/vpci.c | 87 +++++++++++++++++++++++++++++++----
xen/arch/arm/vpci.h | 3 ++
xen/drivers/passthrough/pci.c | 25 ++++++++++
xen/include/asm-arm/pci.h | 1 +
xen/include/xen/pci.h | 1 +
xen/include/xen/sched.h | 2 +
7 files changed, 111 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index c7b25bc70439..c0ad6ad682d2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -797,6 +797,7 @@ void arch_domain_destroy(struct domain *d)
get_order_from_bytes(d->arch.efi_acpi_len));
#endif
domain_io_free(d);
+ domain_vpci_free(d);
}
void arch_domain_shutdown(struct domain *d)
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index 14947e975d69..012f958960d1 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -17,6 +17,14 @@
#define REGISTER_OFFSET(addr) ( (addr) & 0x00000fff)
+struct vpci_mmio_priv {
+ /*
+ * Set to true if the MMIO handlers were set up for the emulated
+ * ECAM host PCI bridge.
+ */
+ bool is_virt_ecam;
+};
+
/* Do some sanity checks. */
static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
{
@@ -38,6 +46,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
pci_sbdf_t sbdf;
unsigned long data = ~0UL;
unsigned int size = 1U << info->dabt.size;
+ struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
sbdf.sbdf = MMCFG_BDF(info->gpa);
reg = REGISTER_OFFSET(info->gpa);
@@ -45,6 +54,13 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
if ( !vpci_mmio_access_allowed(reg, size) )
return 0;
+ /*
+ * For the passed through devices we need to map their virtual SBDF
+ * to the physical PCI device being passed through.
+ */
+ if ( priv->is_virt_ecam && !pci_translate_virtual_device(v, &sbdf) )
+ return 1;
+
data = vpci_read(sbdf, reg, min(4u, size));
if ( size == 8 )
data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
@@ -61,6 +77,7 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
pci_sbdf_t sbdf;
unsigned long data = r;
unsigned int size = 1U << info->dabt.size;
+ struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
sbdf.sbdf = MMCFG_BDF(info->gpa);
reg = REGISTER_OFFSET(info->gpa);
@@ -68,6 +85,13 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
if ( !vpci_mmio_access_allowed(reg, size) )
return 0;
+ /*
+ * For the passed through devices we need to map their virtual SBDF
+ * to the physical PCI device being passed through.
+ */
+ if ( priv->is_virt_ecam && !pci_translate_virtual_device(v, &sbdf) )
+ return 1;
+
vpci_write(sbdf, reg, min(4u, size), data);
if ( size == 8 )
vpci_write(sbdf, reg + 4, 4, data >> 32);
@@ -80,13 +104,48 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
.write = vpci_mmio_write,
};
+/*
+ * There are three originators for the PCI configuration space access:
+ * 1. The domain that owns physical host bridge: MMIO handlers are
+ * there so we can update vPCI register handlers with the values
+ * written by the hardware domain, e.g. physical view of the registers/
+ * configuration space.
+ * 2. Guest access to the passed through PCI devices: we need to properly
+ * map virtual bus topology to the physical one, e.g. pass the configuration
+ * space access to the corresponding physical devices.
+ * 3. Emulated host PCI bridge access. It doesn't exist in the physical
+ * topology, e.g. it can't be mapped to some physical host bridge.
+ * So, all access to the host bridge itself needs to be trapped and
+ * emulated.
+ */
static int vpci_setup_mmio_handler(struct domain *d,
struct pci_host_bridge *bridge)
{
- struct pci_config_window *cfg = bridge->cfg;
+ struct vpci_mmio_priv *priv;
+
+ priv = xzalloc(struct vpci_mmio_priv);
+ if ( !priv )
+ return -ENOMEM;
+
+ priv->is_virt_ecam = !is_hardware_domain(d);
- register_mmio_handler(d, &vpci_mmio_handler,
- cfg->phys_addr, cfg->size, NULL);
+ if ( is_hardware_domain(d) )
+ {
+ struct pci_config_window *cfg = bridge->cfg;
+
+ bridge->mmio_priv = priv;
+ register_mmio_handler(d, &vpci_mmio_handler,
+ cfg->phys_addr, cfg->size,
+ priv);
+ }
+ else
+ {
+ d->vpci_mmio_priv = priv;
+ /* Guest domains use what is programmed in their device tree. */
+ register_mmio_handler(d, &vpci_mmio_handler,
+ GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE,
+ priv);
+ }
return 0;
}
@@ -95,13 +154,16 @@ int domain_vpci_init(struct domain *d)
if ( !has_vpci(d) )
return 0;
- if ( is_hardware_domain(d) )
- return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
-
- /* Guest domains use what is programmed in their device tree. */
- register_mmio_handler(d, &vpci_mmio_handler,
- GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
+ return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
+}
+static int domain_vpci_free_cb(struct domain *d,
+ struct pci_host_bridge *bridge)
+{
+ if ( is_hardware_domain(d) )
+ XFREE(bridge->mmio_priv);
+ else
+ XFREE(d->vpci_mmio_priv);
return 0;
}
@@ -124,6 +186,13 @@ int domain_vpci_get_num_mmio_handlers(struct domain *d)
return count;
}
+void domain_vpci_free(struct domain *d)
+{
+ if ( !has_vpci(d) )
+ return;
+
+ pci_host_iterate_bridges(d, domain_vpci_free_cb);
+}
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
index 27a2b069abd2..38e5a28c0d95 100644
--- a/xen/arch/arm/vpci.h
+++ b/xen/arch/arm/vpci.h
@@ -18,6 +18,7 @@
#ifdef CONFIG_HAS_VPCI
int domain_vpci_init(struct domain *d);
int domain_vpci_get_num_mmio_handlers(struct domain *d);
+void domain_vpci_free(struct domain *d);
#else
static inline int domain_vpci_init(struct domain *d)
{
@@ -28,6 +29,8 @@ static inline int domain_vpci_get_num_mmio_handlers(struct domain *d)
{
return 0;
}
+
+static inline void domain_vpci_free(struct domain *d) { }
#endif
#endif /* __ARCH_ARM_VPCI_H__ */
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 4552ace855e0..579c6947cc35 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
return 0;
}
+/*
+ * Find the physical device which is mapped to the virtual device
+ * and translate virtual SBDF to the physical one.
+ */
+bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
+{
+ struct domain *d = v->domain;
+ struct vpci_dev *vdev;
+ bool found = false;
+
+ pcidevs_lock();
+ list_for_each_entry ( vdev, &d->vdev_list, list )
+ {
+ if ( vdev->sbdf.sbdf == sbdf->sbdf )
+ {
+ /* Replace virtual SBDF with the physical one. */
+ *sbdf = vdev->pdev->sbdf;
+ found = true;
+ break;
+ }
+ }
+ pcidevs_unlock();
+ return found;
+}
+
/* Caller should hold the pcidevs_lock */
static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
uint8_t devfn)
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index b81f66e813ef..983a63be7572 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -72,6 +72,7 @@ struct pci_host_bridge {
struct pci_config_window* cfg; /* Pointer to the bridge config window */
void *sysdata; /* Pointer to the config space window*/
const struct pci_ops *ops;
+ void *mmio_priv; /* MMIO handler's private data. */
};
struct pci_ops {
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 35ae1d093921..0017de19e7c8 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -191,6 +191,7 @@ struct pci_dev *pci_get_real_pdev(int seg, int bus, int devfn);
struct pci_dev *pci_get_pdev_by_domain(const struct domain *, int seg,
int bus, int devfn);
void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
+bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf);
uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg);
uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index d304c7ebe766..0626820545cc 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -450,6 +450,8 @@ struct domain
* to assign a unique SBDF to a passed through virtual PCI device.
*/
int vpci_dev_next;
+ /* Virtual PCI MMIO handler's private data. */
+ void *vpci_mmio_priv;
#endif
#ifdef CONFIG_HAS_PASSTHROUGH
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-23 12:55 ` [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests Oleksandr Andrushchenko
@ 2021-09-27 11:31 ` Jan Beulich
2021-09-27 12:08 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-27 11:31 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, bertrand.marquis, rahul.singh,
Oleksandr Andrushchenko, xen-devel
On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> There are three originators for the PCI configuration space access:
> 1. The domain that owns physical host bridge: MMIO handlers are
> there so we can update vPCI register handlers with the values
> written by the hardware domain, e.g. physical view of the registers
> vs guest's view on the configuration space.
> 2. Guest access to the passed through PCI devices: we need to properly
> map virtual bus topology to the physical one, e.g. pass the configuration
> space access to the corresponding physical devices.
> 3. Emulated host PCI bridge access. It doesn't exist in the physical
> topology, e.g. it can't be mapped to some physical host bridge.
> So, all access to the host bridge itself needs to be trapped and
> emulated.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> New in v2
> ---
> xen/arch/arm/domain.c | 1 +
> xen/arch/arm/vpci.c | 87 +++++++++++++++++++++++++++++++----
> xen/arch/arm/vpci.h | 3 ++
> xen/drivers/passthrough/pci.c | 25 ++++++++++
> xen/include/asm-arm/pci.h | 1 +
> xen/include/xen/pci.h | 1 +
> xen/include/xen/sched.h | 2 +
> 7 files changed, 111 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index c7b25bc70439..c0ad6ad682d2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -797,6 +797,7 @@ void arch_domain_destroy(struct domain *d)
> get_order_from_bytes(d->arch.efi_acpi_len));
> #endif
> domain_io_free(d);
> + domain_vpci_free(d);
> }
>
> void arch_domain_shutdown(struct domain *d)
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 14947e975d69..012f958960d1 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -17,6 +17,14 @@
>
> #define REGISTER_OFFSET(addr) ( (addr) & 0x00000fff)
>
> +struct vpci_mmio_priv {
> + /*
> + * Set to true if the MMIO handlers were set up for the emulated
> + * ECAM host PCI bridge.
> + */
> + bool is_virt_ecam;
> +};
> +
> /* Do some sanity checks. */
> static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
> {
> @@ -38,6 +46,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> pci_sbdf_t sbdf;
> unsigned long data = ~0UL;
> unsigned int size = 1U << info->dabt.size;
> + struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
>
> sbdf.sbdf = MMCFG_BDF(info->gpa);
> reg = REGISTER_OFFSET(info->gpa);
> @@ -45,6 +54,13 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> if ( !vpci_mmio_access_allowed(reg, size) )
> return 0;
>
> + /*
> + * For the passed through devices we need to map their virtual SBDF
> + * to the physical PCI device being passed through.
> + */
> + if ( priv->is_virt_ecam && !pci_translate_virtual_device(v, &sbdf) )
> + return 1;
> +
> data = vpci_read(sbdf, reg, min(4u, size));
> if ( size == 8 )
> data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
> @@ -61,6 +77,7 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
> pci_sbdf_t sbdf;
> unsigned long data = r;
> unsigned int size = 1U << info->dabt.size;
> + struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
>
> sbdf.sbdf = MMCFG_BDF(info->gpa);
> reg = REGISTER_OFFSET(info->gpa);
> @@ -68,6 +85,13 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
> if ( !vpci_mmio_access_allowed(reg, size) )
> return 0;
>
> + /*
> + * For the passed through devices we need to map their virtual SBDF
> + * to the physical PCI device being passed through.
> + */
> + if ( priv->is_virt_ecam && !pci_translate_virtual_device(v, &sbdf) )
> + return 1;
> +
> vpci_write(sbdf, reg, min(4u, size), data);
> if ( size == 8 )
> vpci_write(sbdf, reg + 4, 4, data >> 32);
> @@ -80,13 +104,48 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
> .write = vpci_mmio_write,
> };
>
> +/*
> + * There are three originators for the PCI configuration space access:
> + * 1. The domain that owns physical host bridge: MMIO handlers are
> + * there so we can update vPCI register handlers with the values
> + * written by the hardware domain, e.g. physical view of the registers/
> + * configuration space.
> + * 2. Guest access to the passed through PCI devices: we need to properly
> + * map virtual bus topology to the physical one, e.g. pass the configuration
> + * space access to the corresponding physical devices.
> + * 3. Emulated host PCI bridge access. It doesn't exist in the physical
> + * topology, e.g. it can't be mapped to some physical host bridge.
> + * So, all access to the host bridge itself needs to be trapped and
> + * emulated.
> + */
> static int vpci_setup_mmio_handler(struct domain *d,
> struct pci_host_bridge *bridge)
> {
> - struct pci_config_window *cfg = bridge->cfg;
> + struct vpci_mmio_priv *priv;
> +
> + priv = xzalloc(struct vpci_mmio_priv);
> + if ( !priv )
> + return -ENOMEM;
> +
> + priv->is_virt_ecam = !is_hardware_domain(d);
>
> - register_mmio_handler(d, &vpci_mmio_handler,
> - cfg->phys_addr, cfg->size, NULL);
> + if ( is_hardware_domain(d) )
> + {
> + struct pci_config_window *cfg = bridge->cfg;
> +
> + bridge->mmio_priv = priv;
> + register_mmio_handler(d, &vpci_mmio_handler,
> + cfg->phys_addr, cfg->size,
> + priv);
> + }
> + else
> + {
> + d->vpci_mmio_priv = priv;
> + /* Guest domains use what is programmed in their device tree. */
> + register_mmio_handler(d, &vpci_mmio_handler,
> + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE,
> + priv);
> + }
> return 0;
> }
>
> @@ -95,13 +154,16 @@ int domain_vpci_init(struct domain *d)
> if ( !has_vpci(d) )
> return 0;
>
> - if ( is_hardware_domain(d) )
> - return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
> -
> - /* Guest domains use what is programmed in their device tree. */
> - register_mmio_handler(d, &vpci_mmio_handler,
> - GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
> + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
> +}
>
> +static int domain_vpci_free_cb(struct domain *d,
> + struct pci_host_bridge *bridge)
> +{
> + if ( is_hardware_domain(d) )
> + XFREE(bridge->mmio_priv);
> + else
> + XFREE(d->vpci_mmio_priv);
> return 0;
> }
>
> @@ -124,6 +186,13 @@ int domain_vpci_get_num_mmio_handlers(struct domain *d)
> return count;
> }
>
> +void domain_vpci_free(struct domain *d)
> +{
> + if ( !has_vpci(d) )
> + return;
> +
> + pci_host_iterate_bridges(d, domain_vpci_free_cb);
> +}
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
> index 27a2b069abd2..38e5a28c0d95 100644
> --- a/xen/arch/arm/vpci.h
> +++ b/xen/arch/arm/vpci.h
> @@ -18,6 +18,7 @@
> #ifdef CONFIG_HAS_VPCI
> int domain_vpci_init(struct domain *d);
> int domain_vpci_get_num_mmio_handlers(struct domain *d);
> +void domain_vpci_free(struct domain *d);
> #else
> static inline int domain_vpci_init(struct domain *d)
> {
> @@ -28,6 +29,8 @@ static inline int domain_vpci_get_num_mmio_handlers(struct domain *d)
> {
> return 0;
> }
> +
> +static inline void domain_vpci_free(struct domain *d) { }
> #endif
>
> #endif /* __ARCH_ARM_VPCI_H__ */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 4552ace855e0..579c6947cc35 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
> return 0;
> }
>
> +/*
> + * Find the physical device which is mapped to the virtual device
> + * and translate virtual SBDF to the physical one.
> + */
> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
Why struct vcpu, when you only need ...
> +{
> + struct domain *d = v->domain;
... this? It's also not really logical for this function to take a
struct vcpu, as the translation should be uniform within a domain.
Also - const please (as said elsewhere before, ideally wherever possible
and sensible).
> + struct vpci_dev *vdev;
> + bool found = false;
> +
> + pcidevs_lock();
> + list_for_each_entry ( vdev, &d->vdev_list, list )
> + {
> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
> + {
> + /* Replace virtual SBDF with the physical one. */
> + *sbdf = vdev->pdev->sbdf;
> + found = true;
> + break;
> + }
> + }
For a DomU with just one or at most a couple of devices, such a brute
force lookup may be fine. What about Dom0 though? The physical topology
gets split at the segment level, so maybe this would by a reasonable
granularity here as well?
> + pcidevs_unlock();
> + return found;
Nit: Blank line please ahead of the main "return" of a function.
> +}
> +
> /* Caller should hold the pcidevs_lock */
> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
> uint8_t devfn)
Seeing this function in context (which patch 2 adds without any #ifdef
around it afaics), will this new function needlessly be built on x86 as
well? (I didn't look at other intermediate patches yet, so please
forgive if I've missed the addition of an #ifdef.)
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-27 11:31 ` Jan Beulich
@ 2021-09-27 12:08 ` Oleksandr Andrushchenko
2021-09-27 13:34 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-27 12:08 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Oleksandr Andrushchenko
On 27.09.21 14:31, Jan Beulich wrote:
> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> There are three originators for the PCI configuration space access:
>> 1. The domain that owns physical host bridge: MMIO handlers are
>> there so we can update vPCI register handlers with the values
>> written by the hardware domain, e.g. physical view of the registers
>> vs guest's view on the configuration space.
>> 2. Guest access to the passed through PCI devices: we need to properly
>> map virtual bus topology to the physical one, e.g. pass the configuration
>> space access to the corresponding physical devices.
>> 3. Emulated host PCI bridge access. It doesn't exist in the physical
>> topology, e.g. it can't be mapped to some physical host bridge.
>> So, all access to the host bridge itself needs to be trapped and
>> emulated.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> ---
>> New in v2
>> ---
>> xen/arch/arm/domain.c | 1 +
>> xen/arch/arm/vpci.c | 87 +++++++++++++++++++++++++++++++----
>> xen/arch/arm/vpci.h | 3 ++
>> xen/drivers/passthrough/pci.c | 25 ++++++++++
>> xen/include/asm-arm/pci.h | 1 +
>> xen/include/xen/pci.h | 1 +
>> xen/include/xen/sched.h | 2 +
>> 7 files changed, 111 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index c7b25bc70439..c0ad6ad682d2 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -797,6 +797,7 @@ void arch_domain_destroy(struct domain *d)
>> get_order_from_bytes(d->arch.efi_acpi_len));
>> #endif
>> domain_io_free(d);
>> + domain_vpci_free(d);
>> }
>>
>> void arch_domain_shutdown(struct domain *d)
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index 14947e975d69..012f958960d1 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -17,6 +17,14 @@
>>
>> #define REGISTER_OFFSET(addr) ( (addr) & 0x00000fff)
>>
>> +struct vpci_mmio_priv {
>> + /*
>> + * Set to true if the MMIO handlers were set up for the emulated
>> + * ECAM host PCI bridge.
>> + */
>> + bool is_virt_ecam;
>> +};
>> +
>> /* Do some sanity checks. */
>> static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
>> {
>> @@ -38,6 +46,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> pci_sbdf_t sbdf;
>> unsigned long data = ~0UL;
>> unsigned int size = 1U << info->dabt.size;
>> + struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
>>
>> sbdf.sbdf = MMCFG_BDF(info->gpa);
>> reg = REGISTER_OFFSET(info->gpa);
>> @@ -45,6 +54,13 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> if ( !vpci_mmio_access_allowed(reg, size) )
>> return 0;
>>
>> + /*
>> + * For the passed through devices we need to map their virtual SBDF
>> + * to the physical PCI device being passed through.
>> + */
>> + if ( priv->is_virt_ecam && !pci_translate_virtual_device(v, &sbdf) )
>> + return 1;
>> +
>> data = vpci_read(sbdf, reg, min(4u, size));
>> if ( size == 8 )
>> data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
>> @@ -61,6 +77,7 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>> pci_sbdf_t sbdf;
>> unsigned long data = r;
>> unsigned int size = 1U << info->dabt.size;
>> + struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
>>
>> sbdf.sbdf = MMCFG_BDF(info->gpa);
>> reg = REGISTER_OFFSET(info->gpa);
>> @@ -68,6 +85,13 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>> if ( !vpci_mmio_access_allowed(reg, size) )
>> return 0;
>>
>> + /*
>> + * For the passed through devices we need to map their virtual SBDF
>> + * to the physical PCI device being passed through.
>> + */
>> + if ( priv->is_virt_ecam && !pci_translate_virtual_device(v, &sbdf) )
>> + return 1;
>> +
>> vpci_write(sbdf, reg, min(4u, size), data);
>> if ( size == 8 )
>> vpci_write(sbdf, reg + 4, 4, data >> 32);
>> @@ -80,13 +104,48 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
>> .write = vpci_mmio_write,
>> };
>>
>> +/*
>> + * There are three originators for the PCI configuration space access:
>> + * 1. The domain that owns physical host bridge: MMIO handlers are
>> + * there so we can update vPCI register handlers with the values
>> + * written by the hardware domain, e.g. physical view of the registers/
>> + * configuration space.
>> + * 2. Guest access to the passed through PCI devices: we need to properly
>> + * map virtual bus topology to the physical one, e.g. pass the configuration
>> + * space access to the corresponding physical devices.
>> + * 3. Emulated host PCI bridge access. It doesn't exist in the physical
>> + * topology, e.g. it can't be mapped to some physical host bridge.
>> + * So, all access to the host bridge itself needs to be trapped and
>> + * emulated.
>> + */
>> static int vpci_setup_mmio_handler(struct domain *d,
>> struct pci_host_bridge *bridge)
>> {
>> - struct pci_config_window *cfg = bridge->cfg;
>> + struct vpci_mmio_priv *priv;
>> +
>> + priv = xzalloc(struct vpci_mmio_priv);
>> + if ( !priv )
>> + return -ENOMEM;
>> +
>> + priv->is_virt_ecam = !is_hardware_domain(d);
>>
>> - register_mmio_handler(d, &vpci_mmio_handler,
>> - cfg->phys_addr, cfg->size, NULL);
>> + if ( is_hardware_domain(d) )
>> + {
>> + struct pci_config_window *cfg = bridge->cfg;
>> +
>> + bridge->mmio_priv = priv;
>> + register_mmio_handler(d, &vpci_mmio_handler,
>> + cfg->phys_addr, cfg->size,
>> + priv);
>> + }
>> + else
>> + {
>> + d->vpci_mmio_priv = priv;
>> + /* Guest domains use what is programmed in their device tree. */
>> + register_mmio_handler(d, &vpci_mmio_handler,
>> + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE,
>> + priv);
>> + }
>> return 0;
>> }
>>
>> @@ -95,13 +154,16 @@ int domain_vpci_init(struct domain *d)
>> if ( !has_vpci(d) )
>> return 0;
>>
>> - if ( is_hardware_domain(d) )
>> - return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
>> -
>> - /* Guest domains use what is programmed in their device tree. */
>> - register_mmio_handler(d, &vpci_mmio_handler,
>> - GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>> + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
>> +}
>>
>> +static int domain_vpci_free_cb(struct domain *d,
>> + struct pci_host_bridge *bridge)
>> +{
>> + if ( is_hardware_domain(d) )
>> + XFREE(bridge->mmio_priv);
>> + else
>> + XFREE(d->vpci_mmio_priv);
>> return 0;
>> }
>>
>> @@ -124,6 +186,13 @@ int domain_vpci_get_num_mmio_handlers(struct domain *d)
>> return count;
>> }
>>
>> +void domain_vpci_free(struct domain *d)
>> +{
>> + if ( !has_vpci(d) )
>> + return;
>> +
>> + pci_host_iterate_bridges(d, domain_vpci_free_cb);
>> +}
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
>> index 27a2b069abd2..38e5a28c0d95 100644
>> --- a/xen/arch/arm/vpci.h
>> +++ b/xen/arch/arm/vpci.h
>> @@ -18,6 +18,7 @@
>> #ifdef CONFIG_HAS_VPCI
>> int domain_vpci_init(struct domain *d);
>> int domain_vpci_get_num_mmio_handlers(struct domain *d);
>> +void domain_vpci_free(struct domain *d);
>> #else
>> static inline int domain_vpci_init(struct domain *d)
>> {
>> @@ -28,6 +29,8 @@ static inline int domain_vpci_get_num_mmio_handlers(struct domain *d)
>> {
>> return 0;
>> }
>> +
>> +static inline void domain_vpci_free(struct domain *d) { }
>> #endif
>>
>> #endif /* __ARCH_ARM_VPCI_H__ */
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 4552ace855e0..579c6947cc35 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
>> return 0;
>> }
>>
>> +/*
>> + * Find the physical device which is mapped to the virtual device
>> + * and translate virtual SBDF to the physical one.
>> + */
>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
> Why struct vcpu, when you only need ...
>
>> +{
>> + struct domain *d = v->domain;
> ... this? It's also not really logical for this function to take a
> struct vcpu, as the translation should be uniform within a domain.
Agree, struct domain is just enough
>
> Also - const please (as said elsewhere before, ideally wherever possible
> and sensible).
Ok
>
>> + struct vpci_dev *vdev;
>> + bool found = false;
>> +
>> + pcidevs_lock();
>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>> + {
>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>> + {
>> + /* Replace virtual SBDF with the physical one. */
>> + *sbdf = vdev->pdev->sbdf;
>> + found = true;
>> + break;
>> + }
>> + }
> For a DomU with just one or at most a couple of devices, such a brute
> force lookup may be fine. What about Dom0 though? The physical topology
> gets split at the segment level, so maybe this would by a reasonable
> granularity here as well?
Not sure I am following why topology matters here. We are just trying to
match one SBDF (as seen by the guest) to other SBDF (physical,
as seen by Dom0), so we can proxy DomU's configuration space access
to the proper device in Dom0.
>
>> + pcidevs_unlock();
>> + return found;
> Nit: Blank line please ahead of the main "return" of a function.
Sure
>
>> +}
>> +
>> /* Caller should hold the pcidevs_lock */
>> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>> uint8_t devfn)
> Seeing this function in context (which patch 2 adds without any #ifdef
> around it afaics),
I believe you are talking about vpci_deassign_device here
vpci_{assign|deassign}_device seem to be not called on x86 PVH as of now,
this is true.
> will this new function needlessly be built on x86 as
> well?
It will at the moment. But in order to avoid ifdefery I would like
to still implement it as an empty function for x86.
> (I didn't look at other intermediate patches yet, so please
> forgive if I've missed the addition of an #ifdef.)
So, I can gate this with HAS_VPCI_GUEST_SUPPORT in patch 2
(HAS_VPCI_GUEST_SUPPORT is introduced in patch 4, so I'll move it to 2)
Does this sound good?
>
> Jan
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-27 12:08 ` Oleksandr Andrushchenko
@ 2021-09-27 13:34 ` Jan Beulich
2021-09-27 13:43 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-27 13:34 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel
On 27.09.2021 14:08, Oleksandr Andrushchenko wrote:
> On 27.09.21 14:31, Jan Beulich wrote:
>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>> return 0;
>>> }
>>>
>>> +/*
>>> + * Find the physical device which is mapped to the virtual device
>>> + * and translate virtual SBDF to the physical one.
>>> + */
>>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
>> Why struct vcpu, when you only need ...
>>
>>> +{
>>> + struct domain *d = v->domain;
>> ... this? It's also not really logical for this function to take a
>> struct vcpu, as the translation should be uniform within a domain.
> Agree, struct domain is just enough
>>
>> Also - const please (as said elsewhere before, ideally wherever possible
>> and sensible).
> Ok
>>
>>> + struct vpci_dev *vdev;
>>> + bool found = false;
>>> +
>>> + pcidevs_lock();
>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>> + {
>>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>>> + {
>>> + /* Replace virtual SBDF with the physical one. */
>>> + *sbdf = vdev->pdev->sbdf;
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>> For a DomU with just one or at most a couple of devices, such a brute
>> force lookup may be fine. What about Dom0 though? The physical topology
>> gets split at the segment level, so maybe this would by a reasonable
>> granularity here as well?
>
> Not sure I am following why topology matters here. We are just trying to
> match one SBDF (as seen by the guest) to other SBDF (physical,
> as seen by Dom0), so we can proxy DomU's configuration space access
> to the proper device in Dom0.
Topology here matters only in so far as I've suggested to have separate
lists per segment, to reduce look times. Other methods of avoiding a
fully linear search are of course possible as well.
>>> + pcidevs_unlock();
>>> + return found;
>> Nit: Blank line please ahead of the main "return" of a function.
> Sure
>>
>>> +}
>>> +
>>> /* Caller should hold the pcidevs_lock */
>>> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>>> uint8_t devfn)
>> Seeing this function in context (which patch 2 adds without any #ifdef
>> around it afaics),
>
> I believe you are talking about vpci_deassign_device here
> vpci_{assign|deassign}_device seem to be not called on x86 PVH as of now,
> this is true.
>
>> will this new function needlessly be built on x86 as
>> well?
>
> It will at the moment. But in order to avoid ifdefery I would like
> to still implement it as an empty function for x86.
>
>> (I didn't look at other intermediate patches yet, so please
>> forgive if I've missed the addition of an #ifdef.)
>
> So, I can gate this with HAS_VPCI_GUEST_SUPPORT in patch 2
> (HAS_VPCI_GUEST_SUPPORT is introduced in patch 4, so I'll move it to 2)
> Does this sound good?
Yes.
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-27 13:34 ` Jan Beulich
@ 2021-09-27 13:43 ` Oleksandr Andrushchenko
2021-09-27 13:51 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-27 13:43 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Oleksandr Andrushchenko
On 27.09.21 16:34, Jan Beulich wrote:
> On 27.09.2021 14:08, Oleksandr Andrushchenko wrote:
>> On 27.09.21 14:31, Jan Beulich wrote:
>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>> return 0;
>>>> }
>>>>
>>>> +/*
>>>> + * Find the physical device which is mapped to the virtual device
>>>> + * and translate virtual SBDF to the physical one.
>>>> + */
>>>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
>>> Why struct vcpu, when you only need ...
>>>
>>>> +{
>>>> + struct domain *d = v->domain;
>>> ... this? It's also not really logical for this function to take a
>>> struct vcpu, as the translation should be uniform within a domain.
>> Agree, struct domain is just enough
>>> Also - const please (as said elsewhere before, ideally wherever possible
>>> and sensible).
>> Ok
>>>> + struct vpci_dev *vdev;
>>>> + bool found = false;
>>>> +
>>>> + pcidevs_lock();
>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>> + {
>>>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>>>> + {
>>>> + /* Replace virtual SBDF with the physical one. */
>>>> + *sbdf = vdev->pdev->sbdf;
>>>> + found = true;
>>>> + break;
>>>> + }
>>>> + }
>>> For a DomU with just one or at most a couple of devices, such a brute
>>> force lookup may be fine. What about Dom0 though? The physical topology
>>> gets split at the segment level, so maybe this would by a reasonable
>>> granularity here as well?
>> Not sure I am following why topology matters here. We are just trying to
>> match one SBDF (as seen by the guest) to other SBDF (physical,
>> as seen by Dom0), so we can proxy DomU's configuration space access
>> to the proper device in Dom0.
> Topology here matters only in so far as I've suggested to have separate
> lists per segment, to reduce look times. Other methods of avoiding a
> fully linear search are of course possible as well.
Ah, with that that respect then of course. But let's be realistic.
How many PCI devices are normally passed through to a guest?
I can assume this is probably less than 10 most of the time.
By assuming that the number of devices is small I see no profit,
but unneeded complexity in accounting virtual devices per segment
and performing the relevant lookup. So, I would go with a single list
and "brute force lookup" unless it is clearly seen that this needs to be
optimized.
>
>>>> + pcidevs_unlock();
>>>> + return found;
>>> Nit: Blank line please ahead of the main "return" of a function.
>> Sure
>>>> +}
>>>> +
>>>> /* Caller should hold the pcidevs_lock */
>>>> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>>>> uint8_t devfn)
>>> Seeing this function in context (which patch 2 adds without any #ifdef
>>> around it afaics),
>> I believe you are talking about vpci_deassign_device here
>> vpci_{assign|deassign}_device seem to be not called on x86 PVH as of now,
>> this is true.
>>
>>> will this new function needlessly be built on x86 as
>>> well?
>> It will at the moment. But in order to avoid ifdefery I would like
>> to still implement it as an empty function for x86.
>>
>>> (I didn't look at other intermediate patches yet, so please
>>> forgive if I've missed the addition of an #ifdef.)
>> So, I can gate this with HAS_VPCI_GUEST_SUPPORT in patch 2
>> (HAS_VPCI_GUEST_SUPPORT is introduced in patch 4, so I'll move it to 2)
>> Does this sound good?
> Yes.
I'll see how I can get rid of the code that x86 doesn't use
> Jan
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-27 13:43 ` Oleksandr Andrushchenko
@ 2021-09-27 13:51 ` Jan Beulich
2021-09-27 14:04 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-27 13:51 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel
On 27.09.2021 15:43, Oleksandr Andrushchenko wrote:
>
> On 27.09.21 16:34, Jan Beulich wrote:
>> On 27.09.2021 14:08, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 14:31, Jan Beulich wrote:
>>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +/*
>>>>> + * Find the physical device which is mapped to the virtual device
>>>>> + * and translate virtual SBDF to the physical one.
>>>>> + */
>>>>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
>>>> Why struct vcpu, when you only need ...
>>>>
>>>>> +{
>>>>> + struct domain *d = v->domain;
>>>> ... this? It's also not really logical for this function to take a
>>>> struct vcpu, as the translation should be uniform within a domain.
>>> Agree, struct domain is just enough
>>>> Also - const please (as said elsewhere before, ideally wherever possible
>>>> and sensible).
>>> Ok
>>>>> + struct vpci_dev *vdev;
>>>>> + bool found = false;
>>>>> +
>>>>> + pcidevs_lock();
>>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>>> + {
>>>>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>>>>> + {
>>>>> + /* Replace virtual SBDF with the physical one. */
>>>>> + *sbdf = vdev->pdev->sbdf;
>>>>> + found = true;
>>>>> + break;
>>>>> + }
>>>>> + }
>>>> For a DomU with just one or at most a couple of devices, such a brute
>>>> force lookup may be fine. What about Dom0 though? The physical topology
>>>> gets split at the segment level, so maybe this would by a reasonable
>>>> granularity here as well?
>>> Not sure I am following why topology matters here. We are just trying to
>>> match one SBDF (as seen by the guest) to other SBDF (physical,
>>> as seen by Dom0), so we can proxy DomU's configuration space access
>>> to the proper device in Dom0.
>> Topology here matters only in so far as I've suggested to have separate
>> lists per segment, to reduce look times. Other methods of avoiding a
>> fully linear search are of course possible as well.
>
> Ah, with that that respect then of course. But let's be realistic.
> How many PCI devices are normally passed through to a guest?
> I can assume this is probably less than 10 most of the time.
> By assuming that the number of devices is small I see no profit,
> but unneeded complexity in accounting virtual devices per segment
> and performing the relevant lookup. So, I would go with a single list
> and "brute force lookup" unless it is clearly seen that this needs to be
> optimized.
Just to repeat my initial reply: "For a DomU with just one or at most
a couple of devices, such a brute force lookup may be fine. What about
Dom0 though?" If the code uses the simpler form because it's only
going to be used for DomU, then that's fine for now. But such latent
issues will want recording - e.g. by TODO comments or at the very
least suitable pointing out in the description.
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-27 13:51 ` Jan Beulich
@ 2021-09-27 14:04 ` Oleksandr Andrushchenko
2021-09-27 14:16 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-27 14:04 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Oleksandr Andrushchenko
On 27.09.21 16:51, Jan Beulich wrote:
> On 27.09.2021 15:43, Oleksandr Andrushchenko wrote:
>> On 27.09.21 16:34, Jan Beulich wrote:
>>> On 27.09.2021 14:08, Oleksandr Andrushchenko wrote:
>>>> On 27.09.21 14:31, Jan Beulich wrote:
>>>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +/*
>>>>>> + * Find the physical device which is mapped to the virtual device
>>>>>> + * and translate virtual SBDF to the physical one.
>>>>>> + */
>>>>>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
>>>>> Why struct vcpu, when you only need ...
>>>>>
>>>>>> +{
>>>>>> + struct domain *d = v->domain;
>>>>> ... this? It's also not really logical for this function to take a
>>>>> struct vcpu, as the translation should be uniform within a domain.
>>>> Agree, struct domain is just enough
>>>>> Also - const please (as said elsewhere before, ideally wherever possible
>>>>> and sensible).
>>>> Ok
>>>>>> + struct vpci_dev *vdev;
>>>>>> + bool found = false;
>>>>>> +
>>>>>> + pcidevs_lock();
>>>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>>>> + {
>>>>>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>>>>>> + {
>>>>>> + /* Replace virtual SBDF with the physical one. */
>>>>>> + *sbdf = vdev->pdev->sbdf;
>>>>>> + found = true;
>>>>>> + break;
>>>>>> + }
>>>>>> + }
>>>>> For a DomU with just one or at most a couple of devices, such a brute
>>>>> force lookup may be fine. What about Dom0 though? The physical topology
>>>>> gets split at the segment level, so maybe this would by a reasonable
>>>>> granularity here as well?
>>>> Not sure I am following why topology matters here. We are just trying to
>>>> match one SBDF (as seen by the guest) to other SBDF (physical,
>>>> as seen by Dom0), so we can proxy DomU's configuration space access
>>>> to the proper device in Dom0.
>>> Topology here matters only in so far as I've suggested to have separate
>>> lists per segment, to reduce look times. Other methods of avoiding a
>>> fully linear search are of course possible as well.
>> Ah, with that that respect then of course. But let's be realistic.
>> How many PCI devices are normally passed through to a guest?
>> I can assume this is probably less than 10 most of the time.
>> By assuming that the number of devices is small I see no profit,
>> but unneeded complexity in accounting virtual devices per segment
>> and performing the relevant lookup. So, I would go with a single list
>> and "brute force lookup" unless it is clearly seen that this needs to be
>> optimized.
>
> Just to repeat my initial reply: "For a DomU with just one or at most
> a couple of devices, such a brute force lookup may be fine. What about
> Dom0 though?" If the code uses the simpler form because it's only
> going to be used for DomU, then that's fine for now. But such latent
> issues will want recording - e.g. by TODO comments or at the very
> least suitable pointing out in the description.
As we do not emulate virtual bus topology for Dom0 then it is
clearly seen that the code may only have impact on DomUs.
But anyways, virtual bus topology for DomUs is emulated with
a single segment 0. We have a single list of virtual SBDFs,
again, for virtual segment 0, which maps those virtual SBDFs
to physical SBDFs. So, we go over the list of virtual devices
assigned to that guest and match the virtual SBDF in question to
its counterpart in Dom0. I can't see how this can be optimized or
needs that optimization because of the fact that Dom0 may have
multiple segments...
So, how would that comment look like?
>
> Jan
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-27 14:04 ` Oleksandr Andrushchenko
@ 2021-09-27 14:16 ` Jan Beulich
2021-09-27 14:20 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-27 14:16 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel
On 27.09.2021 16:04, Oleksandr Andrushchenko wrote:
>
> On 27.09.21 16:51, Jan Beulich wrote:
>> On 27.09.2021 15:43, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 16:34, Jan Beulich wrote:
>>>> On 27.09.2021 14:08, Oleksandr Andrushchenko wrote:
>>>>> On 27.09.21 14:31, Jan Beulich wrote:
>>>>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> +/*
>>>>>>> + * Find the physical device which is mapped to the virtual device
>>>>>>> + * and translate virtual SBDF to the physical one.
>>>>>>> + */
>>>>>>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
>>>>>> Why struct vcpu, when you only need ...
>>>>>>
>>>>>>> +{
>>>>>>> + struct domain *d = v->domain;
>>>>>> ... this? It's also not really logical for this function to take a
>>>>>> struct vcpu, as the translation should be uniform within a domain.
>>>>> Agree, struct domain is just enough
>>>>>> Also - const please (as said elsewhere before, ideally wherever possible
>>>>>> and sensible).
>>>>> Ok
>>>>>>> + struct vpci_dev *vdev;
>>>>>>> + bool found = false;
>>>>>>> +
>>>>>>> + pcidevs_lock();
>>>>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>>>>> + {
>>>>>>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>>>>>>> + {
>>>>>>> + /* Replace virtual SBDF with the physical one. */
>>>>>>> + *sbdf = vdev->pdev->sbdf;
>>>>>>> + found = true;
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + }
>>>>>> For a DomU with just one or at most a couple of devices, such a brute
>>>>>> force lookup may be fine. What about Dom0 though? The physical topology
>>>>>> gets split at the segment level, so maybe this would by a reasonable
>>>>>> granularity here as well?
>>>>> Not sure I am following why topology matters here. We are just trying to
>>>>> match one SBDF (as seen by the guest) to other SBDF (physical,
>>>>> as seen by Dom0), so we can proxy DomU's configuration space access
>>>>> to the proper device in Dom0.
>>>> Topology here matters only in so far as I've suggested to have separate
>>>> lists per segment, to reduce look times. Other methods of avoiding a
>>>> fully linear search are of course possible as well.
>>> Ah, with that that respect then of course. But let's be realistic.
>>> How many PCI devices are normally passed through to a guest?
>>> I can assume this is probably less than 10 most of the time.
>>> By assuming that the number of devices is small I see no profit,
>>> but unneeded complexity in accounting virtual devices per segment
>>> and performing the relevant lookup. So, I would go with a single list
>>> and "brute force lookup" unless it is clearly seen that this needs to be
>>> optimized.
>>
>> Just to repeat my initial reply: "For a DomU with just one or at most
>> a couple of devices, such a brute force lookup may be fine. What about
>> Dom0 though?" If the code uses the simpler form because it's only
>> going to be used for DomU, then that's fine for now. But such latent
>> issues will want recording - e.g. by TODO comments or at the very
>> least suitable pointing out in the description.
>
> As we do not emulate virtual bus topology for Dom0 then it is
>
> clearly seen that the code may only have impact on DomUs.
>
> But anyways, virtual bus topology for DomUs is emulated with
>
> a single segment 0. We have a single list of virtual SBDFs,
>
> again, for virtual segment 0, which maps those virtual SBDFs
>
> to physical SBDFs. So, we go over the list of virtual devices
>
> assigned to that guest and match the virtual SBDF in question to
>
> its counterpart in Dom0. I can't see how this can be optimized or
>
> needs that optimization because of the fact that Dom0 may have
>
> multiple segments...
>
> So, how would that comment look like?
Well - if the plan is that this code would never be used for Dom0,
then all is fine as is, I guess. But as you can see the Dom0 plans
on Arm wrt vPCI weren't clear to me at all.
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests
2021-09-27 14:16 ` Jan Beulich
@ 2021-09-27 14:20 ` Oleksandr Andrushchenko
0 siblings, 0 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-27 14:20 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Oleksandr Andrushchenko
On 27.09.21 17:16, Jan Beulich wrote:
> On 27.09.2021 16:04, Oleksandr Andrushchenko wrote:
>> On 27.09.21 16:51, Jan Beulich wrote:
>>> On 27.09.2021 15:43, Oleksandr Andrushchenko wrote:
>>>> On 27.09.21 16:34, Jan Beulich wrote:
>>>>> On 27.09.2021 14:08, Oleksandr Andrushchenko wrote:
>>>>>> On 27.09.21 14:31, Jan Beulich wrote:
>>>>>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +/*
>>>>>>>> + * Find the physical device which is mapped to the virtual device
>>>>>>>> + * and translate virtual SBDF to the physical one.
>>>>>>>> + */
>>>>>>>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
>>>>>>> Why struct vcpu, when you only need ...
>>>>>>>
>>>>>>>> +{
>>>>>>>> + struct domain *d = v->domain;
>>>>>>> ... this? It's also not really logical for this function to take a
>>>>>>> struct vcpu, as the translation should be uniform within a domain.
>>>>>> Agree, struct domain is just enough
>>>>>>> Also - const please (as said elsewhere before, ideally wherever possible
>>>>>>> and sensible).
>>>>>> Ok
>>>>>>>> + struct vpci_dev *vdev;
>>>>>>>> + bool found = false;
>>>>>>>> +
>>>>>>>> + pcidevs_lock();
>>>>>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>>>>>> + {
>>>>>>>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>>>>>>>> + {
>>>>>>>> + /* Replace virtual SBDF with the physical one. */
>>>>>>>> + *sbdf = vdev->pdev->sbdf;
>>>>>>>> + found = true;
>>>>>>>> + break;
>>>>>>>> + }
>>>>>>>> + }
>>>>>>> For a DomU with just one or at most a couple of devices, such a brute
>>>>>>> force lookup may be fine. What about Dom0 though? The physical topology
>>>>>>> gets split at the segment level, so maybe this would by a reasonable
>>>>>>> granularity here as well?
>>>>>> Not sure I am following why topology matters here. We are just trying to
>>>>>> match one SBDF (as seen by the guest) to other SBDF (physical,
>>>>>> as seen by Dom0), so we can proxy DomU's configuration space access
>>>>>> to the proper device in Dom0.
>>>>> Topology here matters only in so far as I've suggested to have separate
>>>>> lists per segment, to reduce look times. Other methods of avoiding a
>>>>> fully linear search are of course possible as well.
>>>> Ah, with that that respect then of course. But let's be realistic.
>>>> How many PCI devices are normally passed through to a guest?
>>>> I can assume this is probably less than 10 most of the time.
>>>> By assuming that the number of devices is small I see no profit,
>>>> but unneeded complexity in accounting virtual devices per segment
>>>> and performing the relevant lookup. So, I would go with a single list
>>>> and "brute force lookup" unless it is clearly seen that this needs to be
>>>> optimized.
>>> Just to repeat my initial reply: "For a DomU with just one or at most
>>> a couple of devices, such a brute force lookup may be fine. What about
>>> Dom0 though?" If the code uses the simpler form because it's only
>>> going to be used for DomU, then that's fine for now. But such latent
>>> issues will want recording - e.g. by TODO comments or at the very
>>> least suitable pointing out in the description.
>> As we do not emulate virtual bus topology for Dom0 then it is
>>
>> clearly seen that the code may only have impact on DomUs.
>>
>> But anyways, virtual bus topology for DomUs is emulated with
>>
>> a single segment 0. We have a single list of virtual SBDFs,
>>
>> again, for virtual segment 0, which maps those virtual SBDFs
>>
>> to physical SBDFs. So, we go over the list of virtual devices
>>
>> assigned to that guest and match the virtual SBDF in question to
>>
>> its counterpart in Dom0. I can't see how this can be optimized or
>>
>> needs that optimization because of the fact that Dom0 may have
>>
>> multiple segments...
>>
>> So, how would that comment look like?
> Well - if the plan is that this code would never be used for Dom0,
> then all is fine as is, I guess. But as you can see the Dom0 plans
> on Arm wrt vPCI weren't clear to me at all.
It is all new to all of us ;) No problem.
> Jan
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 08/11] vpci/header: Emulate PCI_COMMAND register for guests
2021-09-23 12:54 ` [PATCH v2 08/11] vpci/header: Emulate PCI_COMMAND register for guests Oleksandr Andrushchenko
@ 2021-09-28 7:34 ` Michal Orzel
0 siblings, 0 replies; 44+ messages in thread
From: Michal Orzel @ 2021-09-28 7:34 UTC (permalink / raw)
To: Oleksandr Andrushchenko, xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Add basic emulation support for guests. At the moment only emulate
> PCI_COMMAND_INTX_DISABLE bit, the rest is not emulated yet and left
> as TODO.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> New in v2
> ---
> xen/drivers/vpci/header.c | 35 ++++++++++++++++++++++++++++++++---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 09/11] vpci/header: Reset the command register when adding devices
2021-09-23 12:54 ` [PATCH v2 09/11] vpci/header: Reset the command register when adding devices Oleksandr Andrushchenko
@ 2021-09-28 7:38 ` Michal Orzel
0 siblings, 0 replies; 44+ messages in thread
From: Michal Orzel @ 2021-09-28 7:38 UTC (permalink / raw)
To: Oleksandr Andrushchenko, xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Reset the command register when passing through a PCI device:
> it is possible that when passing through a PCI device its memory
> decoding bits in the command register are already set. Thus, a
> guest OS may not write to the command register to update memory
> decoding, so guest mappings (guest's view of the BARs) are
> left not updated.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> Since v1:
> - do not write 0 to the command register, but respect host settings.
> ---
> xen/drivers/vpci/header.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-23 12:55 ` [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology Oleksandr Andrushchenko
@ 2021-09-28 7:48 ` Michal Orzel
2021-09-28 7:59 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Michal Orzel @ 2021-09-28 7:48 UTC (permalink / raw)
To: Oleksandr Andrushchenko, xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
Hi Oleksandr,
On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Assign SBDF to the PCI devices being passed through with bus 0.
> The resulting topology is where PCIe devices reside on the bus 0 of the
> root complex itself (embedded endpoints).
> This implementation is limited to 32 devices which are allowed on
> a single PCI bus.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> New in v2
> ---
> xen/common/domain.c | 1 +
> xen/drivers/passthrough/pci.c | 57 +++++++++++++++++++++++++++++++++++
> xen/drivers/vpci/vpci.c | 18 +++++++++--
> xen/include/xen/pci.h | 18 +++++++++++
> xen/include/xen/sched.h | 6 ++++
> 5 files changed, 97 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 40d67ec34232..b80ff2e5e2e6 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -601,6 +601,7 @@ struct domain *domain_create(domid_t domid,
>
> #ifdef CONFIG_HAS_PCI
> INIT_LIST_HEAD(&d->pdev_list);
> + INIT_LIST_HEAD(&d->vdev_list);
> #endif
>
> /* All error paths can depend on the above setup. */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index e1da283d73ad..4552ace855e0 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -833,6 +833,63 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> return ret;
> }
>
> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
> + const struct pci_dev *pdev)
> +{
> + struct vpci_dev *vdev;
> +
> + list_for_each_entry ( vdev, &d->vdev_list, list )
> + if ( vdev->pdev == pdev )
> + return vdev;
> + return NULL;
> +}
> +
> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
> +{
> + struct vpci_dev *vdev;
> +
> + ASSERT(!pci_find_virtual_device(d, pdev));
> +
> + /* Each PCI bus supports 32 devices/slots at max. */
> + if ( d->vpci_dev_next > 31 )
> + return -ENOSPC;
> +
> + vdev = xzalloc(struct vpci_dev);
> + if ( !vdev )
> + return -ENOMEM;
> +
> + /* We emulate a single host bridge for the guest, so segment is always 0. */
> + *(u16*) &vdev->seg = 0;
Empty line hear would improve readability due to the asterisks being so close to each other.
Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
> + /*
> + * The bus number is set to 0, so virtual devices are seen
> + * as embedded endpoints behind the root complex.
> + */
> + *((u8*) &vdev->bus) = 0;
> + *((u8*) &vdev->devfn) = PCI_DEVFN(d->vpci_dev_next++, 0);
> +
> + vdev->pdev = pdev;
> + vdev->domain = d;
> +
> + pcidevs_lock();
> + list_add_tail(&vdev->list, &d->vdev_list);
> + pcidevs_unlock();
> +
> + return 0;
> +}
> +
> +int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
> +{
> + struct vpci_dev *vdev;
> +
> + pcidevs_lock();
> + vdev = pci_find_virtual_device(d, pdev);
> + if ( vdev )
> + list_del(&vdev->list);
> + pcidevs_unlock();
> + xfree(vdev);
> + return 0;
> +}
> +
> /* Caller should hold the pcidevs_lock */
> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
> uint8_t devfn)
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 0cdc0c3f75c4..a40a138a14f7 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -90,24 +90,36 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> /* Notify vPCI that device is assigned to guest. */
> int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
> {
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> + int rc;
> +#endif
> +
> /* It only makes sense to assign for hwdom or guest domain. */
> if ( is_system_domain(d) || !has_vpci(d) )
> return 0;
>
> #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> - return vpci_bar_add_handlers(d, dev);
> -#else
> - return 0;
> + rc = vpci_bar_add_handlers(d, dev);
> + if ( rc )
> + return rc;
> #endif
> +
> + return pci_add_virtual_device(d, dev);
> }
>
> /* Notify vPCI that device is de-assigned from guest. */
> int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
> {
> + int rc;
> +
> /* It only makes sense to de-assign from hwdom or guest domain. */
> if ( is_system_domain(d) || !has_vpci(d) )
> return 0;
>
> + rc = pci_remove_virtual_device(d, dev);
> + if ( rc )
> + return rc;
> +
> #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> return vpci_bar_remove_handlers(d, dev);
> #else
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 2b2dfb6f1b49..35ae1d093921 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -136,6 +136,22 @@ struct pci_dev {
> struct vpci *vpci;
> };
>
> +struct vpci_dev {
> + struct list_head list;
> + /* Physical PCI device this virtual device is connected to. */
> + const struct pci_dev *pdev;
> + /* Virtual SBDF of the device. */
> + const union {
> + struct {
> + uint8_t devfn;
> + uint8_t bus;
> + uint16_t seg;
> + };
> + pci_sbdf_t sbdf;
> + };
> + struct domain *domain;
> +};
> +
> #define for_each_pdev(domain, pdev) \
> list_for_each_entry(pdev, &(domain)->pdev_list, domain_list)
>
> @@ -165,7 +181,9 @@ int pci_add_segment(u16 seg);
> const unsigned long *pci_get_ro_map(u16 seg);
> int pci_add_device(u16 seg, u8 bus, u8 devfn,
> const struct pci_dev_info *, nodeid_t node);
> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev);
> int pci_remove_device(u16 seg, u8 bus, u8 devfn);
> +int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev);
> int pci_ro_device(int seg, int bus, int devfn);
> int pci_hide_device(unsigned int seg, unsigned int bus, unsigned int devfn);
> struct pci_dev *pci_get_pdev(int seg, int bus, int devfn);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 28146ee404e6..d304c7ebe766 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -444,6 +444,12 @@ struct domain
>
> #ifdef CONFIG_HAS_PCI
> struct list_head pdev_list;
> + struct list_head vdev_list;
> + /*
> + * Current device number used by the virtual PCI bus topology
> + * to assign a unique SBDF to a passed through virtual PCI device.
> + */
> + int vpci_dev_next;
> #endif
>
> #ifdef CONFIG_HAS_PASSTHROUGH
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-28 7:48 ` Michal Orzel
@ 2021-09-28 7:59 ` Jan Beulich
2021-09-28 8:17 ` Michal Orzel
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-28 7:59 UTC (permalink / raw)
To: Michal Orzel, Oleksandr Andrushchenko
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, bertrand.marquis, rahul.singh,
Oleksandr Andrushchenko, xen-devel
On 28.09.2021 09:48, Michal Orzel wrote:
> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -833,6 +833,63 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>> return ret;
>> }
>>
>> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
>> + const struct pci_dev *pdev)
>> +{
>> + struct vpci_dev *vdev;
>> +
>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>> + if ( vdev->pdev == pdev )
>> + return vdev;
>> + return NULL;
>> +}
>> +
>> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
>> +{
>> + struct vpci_dev *vdev;
>> +
>> + ASSERT(!pci_find_virtual_device(d, pdev));
>> +
>> + /* Each PCI bus supports 32 devices/slots at max. */
>> + if ( d->vpci_dev_next > 31 )
>> + return -ENOSPC;
>> +
>> + vdev = xzalloc(struct vpci_dev);
>> + if ( !vdev )
>> + return -ENOMEM;
>> +
>> + /* We emulate a single host bridge for the guest, so segment is always 0. */
>> + *(u16*) &vdev->seg = 0;
> Empty line hear would improve readability due to the asterisks being so close to each other.
> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@arm.com>
>> + /*
>> + * The bus number is set to 0, so virtual devices are seen
>> + * as embedded endpoints behind the root complex.
>> + */
>> + *((u8*) &vdev->bus) = 0;
>> + *((u8*) &vdev->devfn) = PCI_DEVFN(d->vpci_dev_next++, 0);
All of these casts are (a) malformed and (b) unnecessary in the first
place, afaics at least.
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-28 7:59 ` Jan Beulich
@ 2021-09-28 8:17 ` Michal Orzel
2021-09-28 12:58 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Michal Orzel @ 2021-09-28 8:17 UTC (permalink / raw)
To: Jan Beulich, Oleksandr Andrushchenko
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, bertrand.marquis, rahul.singh,
Oleksandr Andrushchenko, xen-devel
On 28.09.2021 09:59, Jan Beulich wrote:
> On 28.09.2021 09:48, Michal Orzel wrote:
>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -833,6 +833,63 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>> return ret;
>>> }
>>>
>>> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
>>> + const struct pci_dev *pdev)
>>> +{
>>> + struct vpci_dev *vdev;
>>> +
>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>> + if ( vdev->pdev == pdev )
>>> + return vdev;
>>> + return NULL;
>>> +}
>>> +
>>> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>> +{
>>> + struct vpci_dev *vdev;
>>> +
>>> + ASSERT(!pci_find_virtual_device(d, pdev));
>>> +
>>> + /* Each PCI bus supports 32 devices/slots at max. */
>>> + if ( d->vpci_dev_next > 31 )
>>> + return -ENOSPC;
>>> +
>>> + vdev = xzalloc(struct vpci_dev);
>>> + if ( !vdev )
>>> + return -ENOMEM;
>>> +
>>> + /* We emulate a single host bridge for the guest, so segment is always 0. */
>>> + *(u16*) &vdev->seg = 0;
>> Empty line hear would improve readability due to the asterisks being so close to each other.
>> Apart from that:
>> Reviewed-by: Michal Orzel <michal.orzel@arm.com>
>>> + /*
>>> + * The bus number is set to 0, so virtual devices are seen
>>> + * as embedded endpoints behind the root complex.
>>> + */
>>> + *((u8*) &vdev->bus) = 0;
>>> + *((u8*) &vdev->devfn) = PCI_DEVFN(d->vpci_dev_next++, 0);
>
> All of these casts are (a) malformed and (b) unnecessary in the first
> place, afaics at least.
>
Agree.
*((u8*) &vdev->bus) = 0;
is the same as:
vdev->bus = 0;
> Jan
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 01/11] vpci: Make vpci registers removal a dedicated function
2021-09-23 12:54 ` [PATCH v2 01/11] vpci: Make vpci registers removal a dedicated function Oleksandr Andrushchenko
@ 2021-09-28 11:15 ` Michal Orzel
0 siblings, 0 replies; 44+ messages in thread
From: Michal Orzel @ 2021-09-28 11:15 UTC (permalink / raw)
To: Oleksandr Andrushchenko, xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> This is in preparation for dynamic assignment of the vpci register
> handlers depending on the domain: hwdom or guest.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> Since v1:
> - constify struct pci_dev where possible
> ---
> xen/drivers/vpci/vpci.c | 7 ++++++-
> xen/include/xen/vpci.h | 2 ++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-28 8:17 ` Michal Orzel
@ 2021-09-28 12:58 ` Oleksandr Andrushchenko
2021-09-29 9:03 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-28 12:58 UTC (permalink / raw)
To: Michal Orzel, Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
Oleksandr Andrushchenko, xen-devel
On 28.09.21 11:17, Michal Orzel wrote:
>
> On 28.09.2021 09:59, Jan Beulich wrote:
>> On 28.09.2021 09:48, Michal Orzel wrote:
>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -833,6 +833,63 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>>> return ret;
>>>> }
>>>>
>>>> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
>>>> + const struct pci_dev *pdev)
>>>> +{
>>>> + struct vpci_dev *vdev;
>>>> +
>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>> + if ( vdev->pdev == pdev )
>>>> + return vdev;
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>> +{
>>>> + struct vpci_dev *vdev;
>>>> +
>>>> + ASSERT(!pci_find_virtual_device(d, pdev));
>>>> +
>>>> + /* Each PCI bus supports 32 devices/slots at max. */
>>>> + if ( d->vpci_dev_next > 31 )
>>>> + return -ENOSPC;
>>>> +
>>>> + vdev = xzalloc(struct vpci_dev);
>>>> + if ( !vdev )
>>>> + return -ENOMEM;
>>>> +
>>>> + /* We emulate a single host bridge for the guest, so segment is always 0. */
>>>> + *(u16*) &vdev->seg = 0;
>>> Empty line hear would improve readability due to the asterisks being so close to each other.
Will add
>>> Apart from that:
>>> Reviewed-by: Michal Orzel <michal.orzel@arm.com>
>>>> + /*
>>>> + * The bus number is set to 0, so virtual devices are seen
>>>> + * as embedded endpoints behind the root complex.
>>>> + */
>>>> + *((u8*) &vdev->bus) = 0;
>>>> + *((u8*) &vdev->devfn) = PCI_DEVFN(d->vpci_dev_next++, 0);
>> All of these casts are (a) malformed and (b) unnecessary in the first
>> place, afaics at least.
>>
> Agree.
> *((u8*) &vdev->bus) = 0;
> is the same as:
> vdev->bus = 0;
Overengineering at its best ;)
Will fix that
>> Jan
>>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view
2021-09-23 12:54 ` [PATCH v2 07/11] vpci/header: program p2m with guest BAR view Oleksandr Andrushchenko
@ 2021-09-29 8:13 ` Michal Orzel
2021-09-29 8:16 ` Jan Beulich
2021-09-29 8:16 ` Oleksandr Andrushchenko
0 siblings, 2 replies; 44+ messages in thread
From: Michal Orzel @ 2021-09-29 8:13 UTC (permalink / raw)
To: Oleksandr Andrushchenko, xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
Hi Oleksandr,
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Take into account guest's BAR view and program its p2m accordingly:
> gfn is guest's view of the BAR and mfn is the physical BAR value as set
> up by the host bridge in the hardware domain.
> This way hardware doamin sees physical BAR values and guest sees
> emulated ones.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> Since v1:
> - s/MSI/MSI-X in comments
> ---
> xen/drivers/vpci/header.c | 35 +++++++++++++++++++++++++++++++++--
> 1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 9c603d26d302..bdd18599b205 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -30,6 +30,10 @@
>
> struct map_data {
> struct domain *d;
> + /* Start address of the BAR as seen by the guest. */
> + gfn_t start_gfn;
> + /* Physical start address of the BAR. */
> + mfn_t start_mfn;
> bool map;
> };
>
> @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data,
> unsigned long *c)
> {
> const struct map_data *map = data;
> + gfn_t start_gfn;
> int rc;
>
> for ( ; ; )
> {
> unsigned long size = e - s + 1;
>
> + /*
> + * Any BAR may have holes in its memory we want to map, e.g.
> + * we don't want to map MSI-X regions which may be a part of that BAR,
> + * e.g. when a single BAR is used for both MMIO and MSI-X.
> + * In this case MSI-X regions are subtracted from the mapping, but
> + * map->start_gfn still points to the very beginning of the BAR.
> + * So if there is a hole present then we need to adjust start_gfn
> + * to reflect the fact of that substraction.
> + */
> + start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn));
> +
> + printk(XENLOG_G_DEBUG
> + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n",
> + map->map ? "" : "un", s, e, gfn_x(start_gfn),
> + map->d->domain_id);
> /*
> * ARM TODOs:
> * - On ARM whether the memory is prefetchable or not should be passed
> @@ -52,8 +72,10 @@ static int map_range(unsigned long s, unsigned long e, void *data,
> * - {un}map_mmio_regions doesn't support preemption.
> */
>
> - rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
> - : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
> + rc = map->map ? map_mmio_regions(map->d, start_gfn,
> + size, _mfn(s))
> + : unmap_mmio_regions(map->d, start_gfn,
> + size, _mfn(s));
> if ( rc == 0 )
> {
> *c += size;
> @@ -69,6 +91,7 @@ static int map_range(unsigned long s, unsigned long e, void *data,
> ASSERT(rc < size);
> *c += rc;
> s += rc;
> + gfn_add(map->start_gfn, rc);
> if ( general_preempt_check() )
> return -ERESTART;
> }
> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
> if ( !bar->mem )
> continue;
>
> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
> + _gfn(PFN_DOWN(bar->addr)) :
> + _gfn(PFN_DOWN(bar->guest_addr));
I believe this would look better with the following alignment:
data.start_gfn = is_hardware_domain(v->vpci.pdev->domain)
? _gfn(PFN_DOWN(bar->addr))
: _gfn(PFN_DOWN(bar->guest_addr));
> + data.start_mfn = _mfn(PFN_DOWN(bar->addr));
> rc = rangeset_consume_ranges(bar->mem, map_range, &data);
>
> if ( rc == -ERESTART )
> @@ -194,6 +221,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
> if ( !bar->mem )
> continue;
>
> + data.start_gfn = is_hardware_domain(d) ?
> + _gfn(PFN_DOWN(bar->addr)) :
> + _gfn(PFN_DOWN(bar->guest_addr));
This one also.
> + data.start_mfn = _mfn(PFN_DOWN(bar->addr));
> while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
> &data)) == -ERESTART )
> process_pending_softirqs();
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view
2021-09-29 8:13 ` Michal Orzel
@ 2021-09-29 8:16 ` Jan Beulich
2021-09-29 8:24 ` Oleksandr Andrushchenko
2021-09-29 8:16 ` Oleksandr Andrushchenko
1 sibling, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-29 8:16 UTC (permalink / raw)
To: Michal Orzel, Oleksandr Andrushchenko
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, bertrand.marquis, rahul.singh,
Oleksandr Andrushchenko, xen-devel
On 29.09.2021 10:13, Michal Orzel wrote:
> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>> if ( !bar->mem )
>> continue;
>>
>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
>> + _gfn(PFN_DOWN(bar->addr)) :
>> + _gfn(PFN_DOWN(bar->guest_addr));
> I believe this would look better with the following alignment:
> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain)
> ? _gfn(PFN_DOWN(bar->addr))
> : _gfn(PFN_DOWN(bar->guest_addr));
FWIW I agree, yet personally I think the conditional operator here
even wants to move inside the _gfn(PFN_DOWN()).
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view
2021-09-29 8:13 ` Michal Orzel
2021-09-29 8:16 ` Jan Beulich
@ 2021-09-29 8:16 ` Oleksandr Andrushchenko
1 sibling, 0 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 8:16 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, jbeulich, Bertrand Marquis,
Rahul Singh, Oleksandr Andrushchenko
Hi, Michal!
On 29.09.21 11:13, Michal Orzel wrote:
> Hi Oleksandr,
>
> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Take into account guest's BAR view and program its p2m accordingly:
>> gfn is guest's view of the BAR and mfn is the physical BAR value as set
>> up by the host bridge in the hardware domain.
>> This way hardware doamin sees physical BAR values and guest sees
>> emulated ones.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> ---
>> Since v1:
>> - s/MSI/MSI-X in comments
>> ---
>> xen/drivers/vpci/header.c | 35 +++++++++++++++++++++++++++++++++--
>> 1 file changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 9c603d26d302..bdd18599b205 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -30,6 +30,10 @@
>>
>> struct map_data {
>> struct domain *d;
>> + /* Start address of the BAR as seen by the guest. */
>> + gfn_t start_gfn;
>> + /* Physical start address of the BAR. */
>> + mfn_t start_mfn;
>> bool map;
>> };
>>
>> @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, void *data,
>> unsigned long *c)
>> {
>> const struct map_data *map = data;
>> + gfn_t start_gfn;
>> int rc;
>>
>> for ( ; ; )
>> {
>> unsigned long size = e - s + 1;
>>
>> + /*
>> + * Any BAR may have holes in its memory we want to map, e.g.
>> + * we don't want to map MSI-X regions which may be a part of that BAR,
>> + * e.g. when a single BAR is used for both MMIO and MSI-X.
>> + * In this case MSI-X regions are subtracted from the mapping, but
>> + * map->start_gfn still points to the very beginning of the BAR.
>> + * So if there is a hole present then we need to adjust start_gfn
>> + * to reflect the fact of that substraction.
>> + */
>> + start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn));
>> +
>> + printk(XENLOG_G_DEBUG
>> + "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n",
>> + map->map ? "" : "un", s, e, gfn_x(start_gfn),
>> + map->d->domain_id);
>> /*
>> * ARM TODOs:
>> * - On ARM whether the memory is prefetchable or not should be passed
>> @@ -52,8 +72,10 @@ static int map_range(unsigned long s, unsigned long e, void *data,
>> * - {un}map_mmio_regions doesn't support preemption.
>> */
>>
>> - rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
>> - : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
>> + rc = map->map ? map_mmio_regions(map->d, start_gfn,
>> + size, _mfn(s))
>> + : unmap_mmio_regions(map->d, start_gfn,
>> + size, _mfn(s));
>> if ( rc == 0 )
>> {
>> *c += size;
>> @@ -69,6 +91,7 @@ static int map_range(unsigned long s, unsigned long e, void *data,
>> ASSERT(rc < size);
>> *c += rc;
>> s += rc;
>> + gfn_add(map->start_gfn, rc);
>> if ( general_preempt_check() )
>> return -ERESTART;
>> }
>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>> if ( !bar->mem )
>> continue;
>>
>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
>> + _gfn(PFN_DOWN(bar->addr)) :
>> + _gfn(PFN_DOWN(bar->guest_addr));
> I believe this would look better with the following alignment:
> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain)
> ? _gfn(PFN_DOWN(bar->addr))
> : _gfn(PFN_DOWN(bar->guest_addr));
I can change that if it improves readability
>> + data.start_mfn = _mfn(PFN_DOWN(bar->addr));
>> rc = rangeset_consume_ranges(bar->mem, map_range, &data);
>>
>> if ( rc == -ERESTART )
>> @@ -194,6 +221,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
>> if ( !bar->mem )
>> continue;
>>
>> + data.start_gfn = is_hardware_domain(d) ?
>> + _gfn(PFN_DOWN(bar->addr)) :
>> + _gfn(PFN_DOWN(bar->guest_addr));
> This one also.
ditto
>> + data.start_mfn = _mfn(PFN_DOWN(bar->addr));
>> while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
>> &data)) == -ERESTART )
>> process_pending_softirqs();
>>
> Cheers,
> Michal
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view
2021-09-29 8:16 ` Jan Beulich
@ 2021-09-29 8:24 ` Oleksandr Andrushchenko
2021-09-29 8:36 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 8:24 UTC (permalink / raw)
To: Jan Beulich, Michal Orzel
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
Oleksandr Andrushchenko, xen-devel
On 29.09.21 11:16, Jan Beulich wrote:
> On 29.09.2021 10:13, Michal Orzel wrote:
>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>>> if ( !bar->mem )
>>> continue;
>>>
>>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
>>> + _gfn(PFN_DOWN(bar->addr)) :
>>> + _gfn(PFN_DOWN(bar->guest_addr));
>> I believe this would look better with the following alignment:
>> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain)
>> ? _gfn(PFN_DOWN(bar->addr))
>> : _gfn(PFN_DOWN(bar->guest_addr));
> FWIW I agree, yet personally I think the conditional operator here
> even wants to move inside the _gfn(PFN_DOWN()).
I can do it as well:
data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr))
But, help me please breaking it into multiline with 80 chars respected
>
> Jan
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view
2021-09-29 8:24 ` Oleksandr Andrushchenko
@ 2021-09-29 8:36 ` Jan Beulich
2021-09-29 8:58 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-29 8:36 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel
On 29.09.2021 10:24, Oleksandr Andrushchenko wrote:
>
> On 29.09.21 11:16, Jan Beulich wrote:
>> On 29.09.2021 10:13, Michal Orzel wrote:
>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>>>> if ( !bar->mem )
>>>> continue;
>>>>
>>>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
>>>> + _gfn(PFN_DOWN(bar->addr)) :
>>>> + _gfn(PFN_DOWN(bar->guest_addr));
>>> I believe this would look better with the following alignment:
>>> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain)
>>> ? _gfn(PFN_DOWN(bar->addr))
>>> : _gfn(PFN_DOWN(bar->guest_addr));
>> FWIW I agree, yet personally I think the conditional operator here
>> even wants to move inside the _gfn(PFN_DOWN()).
>
> I can do it as well:
>
> data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr))
> But, help me please breaking it into multiline with 80 chars respected
Besides the option of latching v->vpci.pdev->domain or
is_hardware_domain(v->vpci.pdev->domain) into a helper variable,
data.start_gfn =
_gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
? bar->addr : bar->guest_addr));
or
data.start_gfn =
_gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
? bar->addr
: bar->guest_addr));
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view
2021-09-29 8:36 ` Jan Beulich
@ 2021-09-29 8:58 ` Oleksandr Andrushchenko
0 siblings, 0 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 8:58 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel
On 29.09.21 11:36, Jan Beulich wrote:
> On 29.09.2021 10:24, Oleksandr Andrushchenko wrote:
>> On 29.09.21 11:16, Jan Beulich wrote:
>>> On 29.09.2021 10:13, Michal Orzel wrote:
>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>>>>> if ( !bar->mem )
>>>>> continue;
>>>>>
>>>>> + data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
>>>>> + _gfn(PFN_DOWN(bar->addr)) :
>>>>> + _gfn(PFN_DOWN(bar->guest_addr));
>>>> I believe this would look better with the following alignment:
>>>> data.start_gfn = is_hardware_domain(v->vpci.pdev->domain)
>>>> ? _gfn(PFN_DOWN(bar->addr))
>>>> : _gfn(PFN_DOWN(bar->guest_addr));
>>> FWIW I agree, yet personally I think the conditional operator here
>>> even wants to move inside the _gfn(PFN_DOWN()).
>> I can do it as well:
>>
>> data.start_gfn = _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain) ? bar->addr : bar->guest_addr))
>> But, help me please breaking it into multiline with 80 chars respected
> Besides the option of latching v->vpci.pdev->domain or
> is_hardware_domain(v->vpci.pdev->domain) into a helper variable,
>
> data.start_gfn =
> _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
> ? bar->addr : bar->guest_addr));
I'll go with this one, thank you
>
> or
>
> data.start_gfn =
> _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)
> ? bar->addr
> : bar->guest_addr));
>
> Jan
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-28 12:58 ` Oleksandr Andrushchenko
@ 2021-09-29 9:03 ` Oleksandr Andrushchenko
2021-09-29 9:09 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 9:03 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel, Oleksandr Andrushchenko
Hi, Jan!
Sorry for top posting, but this is a general question on this patch/functionality.
Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
as this renders in somewhat dead code for x86 for now? I do think this still
needs to be in the common code though.
Thank you in advance,
Oleksandr
On 28.09.21 15:58, Oleksandr Andrushchenko wrote:
> On 28.09.21 11:17, Michal Orzel wrote:
>> On 28.09.2021 09:59, Jan Beulich wrote:
>>> On 28.09.2021 09:48, Michal Orzel wrote:
>>>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -833,6 +833,63 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>>>> return ret;
>>>>> }
>>>>>
>>>>> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
>>>>> + const struct pci_dev *pdev)
>>>>> +{
>>>>> + struct vpci_dev *vdev;
>>>>> +
>>>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>>>> + if ( vdev->pdev == pdev )
>>>>> + return vdev;
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>>>> +{
>>>>> + struct vpci_dev *vdev;
>>>>> +
>>>>> + ASSERT(!pci_find_virtual_device(d, pdev));
>>>>> +
>>>>> + /* Each PCI bus supports 32 devices/slots at max. */
>>>>> + if ( d->vpci_dev_next > 31 )
>>>>> + return -ENOSPC;
>>>>> +
>>>>> + vdev = xzalloc(struct vpci_dev);
>>>>> + if ( !vdev )
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + /* We emulate a single host bridge for the guest, so segment is always 0. */
>>>>> + *(u16*) &vdev->seg = 0;
>>>> Empty line hear would improve readability due to the asterisks being so close to each other.
> Will add
>>>> Apart from that:
>>>> Reviewed-by: Michal Orzel <michal.orzel@arm.com>
>>>>> + /*
>>>>> + * The bus number is set to 0, so virtual devices are seen
>>>>> + * as embedded endpoints behind the root complex.
>>>>> + */
>>>>> + *((u8*) &vdev->bus) = 0;
>>>>> + *((u8*) &vdev->devfn) = PCI_DEVFN(d->vpci_dev_next++, 0);
>>> All of these casts are (a) malformed and (b) unnecessary in the first
>>> place, afaics at least.
>>>
>> Agree.
>> *((u8*) &vdev->bus) = 0;
>> is the same as:
>> vdev->bus = 0;
> Overengineering at its best ;)
>
> Will fix that
>
>>> Jan
>>>
> Thank you,
>
> Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 9:03 ` Oleksandr Andrushchenko
@ 2021-09-29 9:09 ` Jan Beulich
2021-09-29 11:56 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-29 9:09 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel
On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
> Sorry for top posting, but this is a general question on this patch/functionality.
>
> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
> as this renders in somewhat dead code for x86 for now? I do think this still
> needs to be in the common code though.
I agree it wants to live in common code, but I'd still like the code to
not bloat x86 binaries. Hence yes, I think there want to be
"if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
possible without breaking the build, respective #ifdef-s.
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 05/11] vpci/header: Implement guest BAR register handlers
2021-09-23 12:54 ` [PATCH v2 05/11] vpci/header: Implement guest BAR register handlers Oleksandr Andrushchenko
@ 2021-09-29 9:27 ` Michal Orzel
0 siblings, 0 replies; 44+ messages in thread
From: Michal Orzel @ 2021-09-29 9:27 UTC (permalink / raw)
To: Oleksandr Andrushchenko, xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Emulate guest BAR register values: this allows creating a guest view
> of the registers and emulates size and properties probe as it is done
> during PCI device enumeration by the guest.
>
> ROM BAR is only handled for the hardware domain and for guest domains
> there is a stub: at the moment PCI expansion ROM is x86 only, so it
> might not be used by other architectures without emulating x86. Other
> use-cases may include using that expansion ROM before Xen boots, hence
> no emulation is needed in Xen itself. Or when a guest wants to use the
> ROM code which seems to be rare.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> Since v1:
> - re-work guest read/write to be much simpler and do more work on write
> than read which is expected to be called more frequently
> - removed one too obvious comment
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> xen/drivers/vpci/header.c | 30 +++++++++++++++++++++++++++++-
> xen/include/xen/vpci.h | 3 +++
> 2 files changed, 32 insertions(+), 1 deletion(-)
>
FWICS you addressed all Jan's comments from v1 so:
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 02/11] vpci: Add hooks for PCI device assign/de-assign
2021-09-23 12:54 ` [PATCH v2 02/11] vpci: Add hooks for PCI device assign/de-assign Oleksandr Andrushchenko
@ 2021-09-29 9:35 ` Michal Orzel
0 siblings, 0 replies; 44+ messages in thread
From: Michal Orzel @ 2021-09-29 9:35 UTC (permalink / raw)
To: Oleksandr Andrushchenko, xen-devel
Cc: julien, sstabellini, oleksandr_tyshchenko, volodymyr_babchuk,
Artem_Mygaiev, roger.pau, jbeulich, bertrand.marquis,
rahul.singh, Oleksandr Andrushchenko
On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> When a PCI device gets assigned/de-assigned some work on vPCI side needs
> to be done for that device. Introduce a pair of hooks so vPCI can handle
> that.
>
> Please note, that in the current design the error path is handled by
> the toolstack via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device,
> so this is why it is acceptable not to de-assign devices if vPCI's
> assign fails, e.g. the roll back will be handled on deassign_device when
> it is called by the toolstack.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> ---
> Since v1:
> - constify struct pci_dev where possible
> - do not open code is_system_domain()
> - extended the commit message
> ---
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 9:09 ` Jan Beulich
@ 2021-09-29 11:56 ` Oleksandr Andrushchenko
2021-09-29 12:54 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 11:56 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Oleksandr Andrushchenko, Michal Orzel
On 29.09.21 12:09, Jan Beulich wrote:
> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>> Sorry for top posting, but this is a general question on this patch/functionality.
>>
>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>> as this renders in somewhat dead code for x86 for now? I do think this still
>> needs to be in the common code though.
> I agree it wants to live in common code, but I'd still like the code to
> not bloat x86 binaries. Hence yes, I think there want to be
> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
> possible without breaking the build, respective #ifdef-s.
Then it needs to be defined as (xen/drivers/Kconfig):
config HAS_VPCI_GUEST_SUPPORT
# vPCI guest support is only enabled for Arm now
def_bool y if ARM
depends on HAS_VPCI
Because it needs to be defined as "y" for Arm with vPCI support.
Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
but the resulting binary behaves wrong.
Do you see this as an acceptable solution?
>
> Jan
>
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 11:56 ` Oleksandr Andrushchenko
@ 2021-09-29 12:54 ` Jan Beulich
2021-09-29 13:16 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-29 12:54 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel
On 29.09.2021 13:56, Oleksandr Andrushchenko wrote:
>
> On 29.09.21 12:09, Jan Beulich wrote:
>> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>>> Sorry for top posting, but this is a general question on this patch/functionality.
>>>
>>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>>> as this renders in somewhat dead code for x86 for now? I do think this still
>>> needs to be in the common code though.
>> I agree it wants to live in common code, but I'd still like the code to
>> not bloat x86 binaries. Hence yes, I think there want to be
>> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
>> possible without breaking the build, respective #ifdef-s.
>
> Then it needs to be defined as (xen/drivers/Kconfig):
>
> config HAS_VPCI_GUEST_SUPPORT
> # vPCI guest support is only enabled for Arm now
> def_bool y if ARM
> depends on HAS_VPCI
>
> Because it needs to be defined as "y" for Arm with vPCI support.
>
> Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
>
> but the resulting binary behaves wrong.
>
> Do you see this as an acceptable solution?
Like all (or at least the majority) of other HAS_*, it ought to be
"select"-ed (by arm/Kconfig). Is there a reason this isn't possible?
(I don't mind the "depends on", as long as it's clear that it exists
solely to allow kconfig to warn about bogus "select"s.)
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 12:54 ` Jan Beulich
@ 2021-09-29 13:16 ` Oleksandr Andrushchenko
2021-09-29 13:23 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 13:16 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel, Oleksandr Andrushchenko
On 29.09.21 15:54, Jan Beulich wrote:
> On 29.09.2021 13:56, Oleksandr Andrushchenko wrote:
>> On 29.09.21 12:09, Jan Beulich wrote:
>>> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>>>> Sorry for top posting, but this is a general question on this patch/functionality.
>>>>
>>>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>> as this renders in somewhat dead code for x86 for now? I do think this still
>>>> needs to be in the common code though.
>>> I agree it wants to live in common code, but I'd still like the code to
>>> not bloat x86 binaries. Hence yes, I think there want to be
>>> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
>>> possible without breaking the build, respective #ifdef-s.
>> Then it needs to be defined as (xen/drivers/Kconfig):
>>
>> config HAS_VPCI_GUEST_SUPPORT
>> # vPCI guest support is only enabled for Arm now
>> def_bool y if ARM
>> depends on HAS_VPCI
>>
>> Because it needs to be defined as "y" for Arm with vPCI support.
>>
>> Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
>>
>> but the resulting binary behaves wrong.
>>
>> Do you see this as an acceptable solution?
> Like all (or at least the majority) of other HAS_*, it ought to be
> "select"-ed (by arm/Kconfig). Is there a reason this isn't possible?
> (I don't mind the "depends on", as long as it's clear that it exists
> solely to allow kconfig to warn about bogus "select"s.)
There is yet no Kconfig exists (even for Arm) that enables HAS_VPCI,
thus I can't do that at the moment even if I want to. Yes, this can be deferred
to the later stage when we enable VPCI for Arm, bit this config option is still
considered as "common code" as the functionality being added
to the common code is just gated with CONFIG_HAS_VPCI_GUEST_SUPPORT.
With this defined now and not later the code seems to be complete and more clean
as it is seen what is this configuration option and how it is enabled and used in the
code.
So, I see no problem if it is defined in this Kconfig and evaluates to "n" for x86
and eventually will be "y" for Arm when it enables HAS_VPCI.
>
> Jan
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 13:16 ` Oleksandr Andrushchenko
@ 2021-09-29 13:23 ` Jan Beulich
2021-09-29 13:49 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-29 13:23 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel
On 29.09.2021 15:16, Oleksandr Andrushchenko wrote:
>
> On 29.09.21 15:54, Jan Beulich wrote:
>> On 29.09.2021 13:56, Oleksandr Andrushchenko wrote:
>>> On 29.09.21 12:09, Jan Beulich wrote:
>>>> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>>>>> Sorry for top posting, but this is a general question on this patch/functionality.
>>>>>
>>>>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>>> as this renders in somewhat dead code for x86 for now? I do think this still
>>>>> needs to be in the common code though.
>>>> I agree it wants to live in common code, but I'd still like the code to
>>>> not bloat x86 binaries. Hence yes, I think there want to be
>>>> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
>>>> possible without breaking the build, respective #ifdef-s.
>>> Then it needs to be defined as (xen/drivers/Kconfig):
>>>
>>> config HAS_VPCI_GUEST_SUPPORT
>>> # vPCI guest support is only enabled for Arm now
>>> def_bool y if ARM
>>> depends on HAS_VPCI
>>>
>>> Because it needs to be defined as "y" for Arm with vPCI support.
>>>
>>> Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
>>>
>>> but the resulting binary behaves wrong.
>>>
>>> Do you see this as an acceptable solution?
>> Like all (or at least the majority) of other HAS_*, it ought to be
>> "select"-ed (by arm/Kconfig). Is there a reason this isn't possible?
>> (I don't mind the "depends on", as long as it's clear that it exists
>> solely to allow kconfig to warn about bogus "select"s.)
>
> There is yet no Kconfig exists (even for Arm) that enables HAS_VPCI,
>
> thus I can't do that at the moment even if I want to. Yes, this can be deferred
>
> to the later stage when we enable VPCI for Arm, bit this config option is still
>
> considered as "common code" as the functionality being added
>
> to the common code is just gated with CONFIG_HAS_VPCI_GUEST_SUPPORT.
>
> With this defined now and not later the code seems to be complete and more clean
>
> as it is seen what is this configuration option and how it is enabled and used in the
>
> code.
>
> So, I see no problem if it is defined in this Kconfig and evaluates to "n" for x86
>
> and eventually will be "y" for Arm when it enables HAS_VPCI.
I'm afraid I don't view this as a reply reflecting that you have
understood what I'm asking for. The primary request is for there to
not be "def_bool y" but just "bool" accompanied by a "select" in
Arm's Kconfig. If HAS_VPCI doesn't exist as an option yet, simply
omit the (questionable) "depends on".
Jan
PS: The more replies I get from you, the more annoying I find the
insertion of blank lines between every two lines of text. Blank
lines are usually used to separate paragraphs. If it is your mail
program which inserts these, can you please try to do something
about this? Thanks.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 13:23 ` Jan Beulich
@ 2021-09-29 13:49 ` Oleksandr Andrushchenko
2021-09-29 14:07 ` Jan Beulich
0 siblings, 1 reply; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 13:49 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel, Oleksandr Andrushchenko
On 29.09.21 16:23, Jan Beulich wrote:
> On 29.09.2021 15:16, Oleksandr Andrushchenko wrote:
>> On 29.09.21 15:54, Jan Beulich wrote:
>>> On 29.09.2021 13:56, Oleksandr Andrushchenko wrote:
>>>> On 29.09.21 12:09, Jan Beulich wrote:
>>>>> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>>>>>> Sorry for top posting, but this is a general question on this patch/functionality.
>>>>>>
>>>>>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>>>> as this renders in somewhat dead code for x86 for now? I do think this still
>>>>>> needs to be in the common code though.
>>>>> I agree it wants to live in common code, but I'd still like the code to
>>>>> not bloat x86 binaries. Hence yes, I think there want to be
>>>>> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
>>>>> possible without breaking the build, respective #ifdef-s.
>>>> Then it needs to be defined as (xen/drivers/Kconfig):
>>>>
>>>> config HAS_VPCI_GUEST_SUPPORT
>>>> # vPCI guest support is only enabled for Arm now
>>>> def_bool y if ARM
>>>> depends on HAS_VPCI
>>>>
>>>> Because it needs to be defined as "y" for Arm with vPCI support.
>>>>
>>>> Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
>>>>
>>>> but the resulting binary behaves wrong.
>>>>
>>>> Do you see this as an acceptable solution?
>>> Like all (or at least the majority) of other HAS_*, it ought to be
>>> "select"-ed (by arm/Kconfig). Is there a reason this isn't possible?
>>> (I don't mind the "depends on", as long as it's clear that it exists
>>> solely to allow kconfig to warn about bogus "select"s.)
>> There is yet no Kconfig exists (even for Arm) that enables HAS_VPCI,
>>
>> thus I can't do that at the moment even if I want to. Yes, this can be deferred
>>
>> to the later stage when we enable VPCI for Arm, bit this config option is still
>>
>> considered as "common code" as the functionality being added
>>
>> to the common code is just gated with CONFIG_HAS_VPCI_GUEST_SUPPORT.
>>
>> With this defined now and not later the code seems to be complete and more clean
>>
>> as it is seen what is this configuration option and how it is enabled and used in the
>>
>> code.
>>
>> So, I see no problem if it is defined in this Kconfig and evaluates to "n" for x86
>>
>> and eventually will be "y" for Arm when it enables HAS_VPCI.
> I'm afraid I don't view this as a reply reflecting that you have
> understood what I'm asking for. The primary request is for there to
> not be "def_bool y" but just "bool" accompanied by a "select" in
> Arm's Kconfig. If HAS_VPCI doesn't exist as an option yet, simply
> omit the (questionable) "depends on".
I understood that, but was trying to make sure we don't miss
this option while enabling vPCI on Arm, but ok, I'll have the following:
config HAS_VPCI
bool
config HAS_VPCI_GUEST_SUPPORT
bool
depends on HAS_VPCI
and select it for Arm xen/arch/arm/Kconfig
>
> Jan
> PS: The more replies I get from you, the more annoying I find the
> insertion of blank lines between every two lines of text. Blank
> lines are usually used to separate paragraphs. If it is your mail
> program which inserts these, can you please try to do something
> about this? Thanks.
>
I first thought that this is how Thunderbird started showing
my replies and was also curious about the distance between the lines
which seemed to be as double-line, but I couldn't delete or edit
those. I thought this is only visible to me...
It came out that this was because of some Thunderbird settings which
made my replies with those double-liners. Hope it is fixed now.
I am really sorry about that and do understand how annoying it was.
Best regards,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 13:49 ` Oleksandr Andrushchenko
@ 2021-09-29 14:07 ` Jan Beulich
2021-09-29 14:16 ` Oleksandr Andrushchenko
0 siblings, 1 reply; 44+ messages in thread
From: Jan Beulich @ 2021-09-29 14:07 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
xen-devel, Michal Orzel
On 29.09.2021 15:49, Oleksandr Andrushchenko wrote:
>
>
> On 29.09.21 16:23, Jan Beulich wrote:
>> On 29.09.2021 15:16, Oleksandr Andrushchenko wrote:
>>> On 29.09.21 15:54, Jan Beulich wrote:
>>>> On 29.09.2021 13:56, Oleksandr Andrushchenko wrote:
>>>>> On 29.09.21 12:09, Jan Beulich wrote:
>>>>>> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>>>>>>> Sorry for top posting, but this is a general question on this patch/functionality.
>>>>>>>
>>>>>>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>>>>> as this renders in somewhat dead code for x86 for now? I do think this still
>>>>>>> needs to be in the common code though.
>>>>>> I agree it wants to live in common code, but I'd still like the code to
>>>>>> not bloat x86 binaries. Hence yes, I think there want to be
>>>>>> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
>>>>>> possible without breaking the build, respective #ifdef-s.
>>>>> Then it needs to be defined as (xen/drivers/Kconfig):
>>>>>
>>>>> config HAS_VPCI_GUEST_SUPPORT
>>>>> # vPCI guest support is only enabled for Arm now
>>>>> def_bool y if ARM
>>>>> depends on HAS_VPCI
>>>>>
>>>>> Because it needs to be defined as "y" for Arm with vPCI support.
>>>>>
>>>>> Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
>>>>>
>>>>> but the resulting binary behaves wrong.
>>>>>
>>>>> Do you see this as an acceptable solution?
>>>> Like all (or at least the majority) of other HAS_*, it ought to be
>>>> "select"-ed (by arm/Kconfig). Is there a reason this isn't possible?
>>>> (I don't mind the "depends on", as long as it's clear that it exists
>>>> solely to allow kconfig to warn about bogus "select"s.)
>>> There is yet no Kconfig exists (even for Arm) that enables HAS_VPCI,
>>>
>>> thus I can't do that at the moment even if I want to. Yes, this can be deferred
>>>
>>> to the later stage when we enable VPCI for Arm, bit this config option is still
>>>
>>> considered as "common code" as the functionality being added
>>>
>>> to the common code is just gated with CONFIG_HAS_VPCI_GUEST_SUPPORT.
>>>
>>> With this defined now and not later the code seems to be complete and more clean
>>>
>>> as it is seen what is this configuration option and how it is enabled and used in the
>>>
>>> code.
>>>
>>> So, I see no problem if it is defined in this Kconfig and evaluates to "n" for x86
>>>
>>> and eventually will be "y" for Arm when it enables HAS_VPCI.
>> I'm afraid I don't view this as a reply reflecting that you have
>> understood what I'm asking for. The primary request is for there to
>> not be "def_bool y" but just "bool" accompanied by a "select" in
>> Arm's Kconfig. If HAS_VPCI doesn't exist as an option yet, simply
>> omit the (questionable) "depends on".
> I understood that, but was trying to make sure we don't miss
> this option while enabling vPCI on Arm, but ok, I'll have the following:
> config HAS_VPCI
> bool
>
> config HAS_VPCI_GUEST_SUPPORT
> bool
> depends on HAS_VPCI
> and select it for Arm xen/arch/arm/Kconfig
Btw you could also have
config HAS_VPCI
bool
config HAS_VPCI_GUEST_SUPPORT
bool
select HAS_VPCI
which would require arm/Kconfig to only select the latter, while
x86/Kconfig would only select the former.
>> PS: The more replies I get from you, the more annoying I find the
>> insertion of blank lines between every two lines of text. Blank
>> lines are usually used to separate paragraphs. If it is your mail
>> program which inserts these, can you please try to do something
>> about this? Thanks.
>>
> I first thought that this is how Thunderbird started showing
> my replies and was also curious about the distance between the lines
> which seemed to be as double-line, but I couldn't delete or edit
> those. I thought this is only visible to me...
> It came out that this was because of some Thunderbird settings which
> made my replies with those double-liners. Hope it is fixed now.
Indeed, thanks - I did not remove any blank lines from context above.
Jan
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology
2021-09-29 14:07 ` Jan Beulich
@ 2021-09-29 14:16 ` Oleksandr Andrushchenko
0 siblings, 0 replies; 44+ messages in thread
From: Oleksandr Andrushchenko @ 2021-09-29 14:16 UTC (permalink / raw)
To: Jan Beulich
Cc: julien, sstabellini, Oleksandr Tyshchenko, Volodymyr Babchuk,
Artem Mygaiev, roger.pau, Bertrand Marquis, Rahul Singh,
Oleksandr Andrushchenko, xen-devel, Michal Orzel
On 29.09.21 17:07, Jan Beulich wrote:
> On 29.09.2021 15:49, Oleksandr Andrushchenko wrote:
>>
>> On 29.09.21 16:23, Jan Beulich wrote:
>>> On 29.09.2021 15:16, Oleksandr Andrushchenko wrote:
>>>> On 29.09.21 15:54, Jan Beulich wrote:
>>>>> On 29.09.2021 13:56, Oleksandr Andrushchenko wrote:
>>>>>> On 29.09.21 12:09, Jan Beulich wrote:
>>>>>>> On 29.09.2021 11:03, Oleksandr Andrushchenko wrote:
>>>>>>>> Sorry for top posting, but this is a general question on this patch/functionality.
>>>>>>>>
>>>>>>>> Do you see we need to gate all this with CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>>>>>> as this renders in somewhat dead code for x86 for now? I do think this still
>>>>>>>> needs to be in the common code though.
>>>>>>> I agree it wants to live in common code, but I'd still like the code to
>>>>>>> not bloat x86 binaries. Hence yes, I think there want to be
>>>>>>> "if ( !IS_ENABLED() )" early bailout paths or, whenever this isn't
>>>>>>> possible without breaking the build, respective #ifdef-s.
>>>>>> Then it needs to be defined as (xen/drivers/Kconfig):
>>>>>>
>>>>>> config HAS_VPCI_GUEST_SUPPORT
>>>>>> # vPCI guest support is only enabled for Arm now
>>>>>> def_bool y if ARM
>>>>>> depends on HAS_VPCI
>>>>>>
>>>>>> Because it needs to be defined as "y" for Arm with vPCI support.
>>>>>>
>>>>>> Otherwise it breaks the PCI passthrough feature, e.g. it compiles,
>>>>>>
>>>>>> but the resulting binary behaves wrong.
>>>>>>
>>>>>> Do you see this as an acceptable solution?
>>>>> Like all (or at least the majority) of other HAS_*, it ought to be
>>>>> "select"-ed (by arm/Kconfig). Is there a reason this isn't possible?
>>>>> (I don't mind the "depends on", as long as it's clear that it exists
>>>>> solely to allow kconfig to warn about bogus "select"s.)
>>>> There is yet no Kconfig exists (even for Arm) that enables HAS_VPCI,
>>>>
>>>> thus I can't do that at the moment even if I want to. Yes, this can be deferred
>>>>
>>>> to the later stage when we enable VPCI for Arm, bit this config option is still
>>>>
>>>> considered as "common code" as the functionality being added
>>>>
>>>> to the common code is just gated with CONFIG_HAS_VPCI_GUEST_SUPPORT.
>>>>
>>>> With this defined now and not later the code seems to be complete and more clean
>>>>
>>>> as it is seen what is this configuration option and how it is enabled and used in the
>>>>
>>>> code.
>>>>
>>>> So, I see no problem if it is defined in this Kconfig and evaluates to "n" for x86
>>>>
>>>> and eventually will be "y" for Arm when it enables HAS_VPCI.
>>> I'm afraid I don't view this as a reply reflecting that you have
>>> understood what I'm asking for. The primary request is for there to
>>> not be "def_bool y" but just "bool" accompanied by a "select" in
>>> Arm's Kconfig. If HAS_VPCI doesn't exist as an option yet, simply
>>> omit the (questionable) "depends on".
>> I understood that, but was trying to make sure we don't miss
>> this option while enabling vPCI on Arm, but ok, I'll have the following:
>> config HAS_VPCI
>> bool
>>
>> config HAS_VPCI_GUEST_SUPPORT
>> bool
>> depends on HAS_VPCI
>> and select it for Arm xen/arch/arm/Kconfig
> Btw you could also have
>
> config HAS_VPCI
> bool
>
> config HAS_VPCI_GUEST_SUPPORT
> bool
> select HAS_VPCI
>
> which would require arm/Kconfig to only select the latter, while
> x86/Kconfig would only select the former.
I'll probably leave it as I wrote before, because it then looks like
a sub-feature enables the parent feature and doesn't seem right
Although it may still look right...
>
>>> PS: The more replies I get from you, the more annoying I find the
>>> insertion of blank lines between every two lines of text. Blank
>>> lines are usually used to separate paragraphs. If it is your mail
>>> program which inserts these, can you please try to do something
>>> about this? Thanks.
>>>
>> I first thought that this is how Thunderbird started showing
>> my replies and was also curious about the distance between the lines
>> which seemed to be as double-line, but I couldn't delete or edit
>> those. I thought this is only visible to me...
>> It came out that this was because of some Thunderbird settings which
>> made my replies with those double-liners. Hope it is fixed now.
> Indeed, thanks - I did not remove any blank lines from context above.
>
> Jan
>
>
Thank you,
Oleksandr
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2021-09-29 14:16 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-23 12:54 [PATCH v2 00/11] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 01/11] vpci: Make vpci registers removal a dedicated function Oleksandr Andrushchenko
2021-09-28 11:15 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 02/11] vpci: Add hooks for PCI device assign/de-assign Oleksandr Andrushchenko
2021-09-29 9:35 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 03/11] vpci/header: Move register assignments from init_bars Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 04/11] vpci/header: Add and remove register handlers dynamically Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 05/11] vpci/header: Implement guest BAR register handlers Oleksandr Andrushchenko
2021-09-29 9:27 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 06/11] vpci/header: Handle p2m range sets per BAR Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 07/11] vpci/header: program p2m with guest BAR view Oleksandr Andrushchenko
2021-09-29 8:13 ` Michal Orzel
2021-09-29 8:16 ` Jan Beulich
2021-09-29 8:24 ` Oleksandr Andrushchenko
2021-09-29 8:36 ` Jan Beulich
2021-09-29 8:58 ` Oleksandr Andrushchenko
2021-09-29 8:16 ` Oleksandr Andrushchenko
2021-09-23 12:54 ` [PATCH v2 08/11] vpci/header: Emulate PCI_COMMAND register for guests Oleksandr Andrushchenko
2021-09-28 7:34 ` Michal Orzel
2021-09-23 12:54 ` [PATCH v2 09/11] vpci/header: Reset the command register when adding devices Oleksandr Andrushchenko
2021-09-28 7:38 ` Michal Orzel
2021-09-23 12:55 ` [PATCH v2 10/11] vpci: Add initial support for virtual PCI bus topology Oleksandr Andrushchenko
2021-09-28 7:48 ` Michal Orzel
2021-09-28 7:59 ` Jan Beulich
2021-09-28 8:17 ` Michal Orzel
2021-09-28 12:58 ` Oleksandr Andrushchenko
2021-09-29 9:03 ` Oleksandr Andrushchenko
2021-09-29 9:09 ` Jan Beulich
2021-09-29 11:56 ` Oleksandr Andrushchenko
2021-09-29 12:54 ` Jan Beulich
2021-09-29 13:16 ` Oleksandr Andrushchenko
2021-09-29 13:23 ` Jan Beulich
2021-09-29 13:49 ` Oleksandr Andrushchenko
2021-09-29 14:07 ` Jan Beulich
2021-09-29 14:16 ` Oleksandr Andrushchenko
2021-09-23 12:55 ` [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests Oleksandr Andrushchenko
2021-09-27 11:31 ` Jan Beulich
2021-09-27 12:08 ` Oleksandr Andrushchenko
2021-09-27 13:34 ` Jan Beulich
2021-09-27 13:43 ` Oleksandr Andrushchenko
2021-09-27 13:51 ` Jan Beulich
2021-09-27 14:04 ` Oleksandr Andrushchenko
2021-09-27 14:16 ` Jan Beulich
2021-09-27 14:20 ` Oleksandr Andrushchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.