On 5/23/24 03:48, Roger Pau Monné wrote:
> On Wed, May 22, 2024 at 06:59:23PM -0400, Stewart Hildebrand wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@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_andrushche...@epam.com>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com>
>
> Acked-by: Roger Pau Monné <roger....@citrix.com>
>
> One unrelated question below.
>
>> ---
>> In v15:
>> - base on top of ("arm/vpci: honor access size when returning an error")
>> In v11:
>> - Fixed format issues
>> - Added ASSERT_UNREACHABLE() to the dummy implementation of
>> vpci_translate_virtual_device()
>> - Moved variable in vpci_sbdf_from_gpa(), now it is easier to follow
>> the logic in the function
>> Since v9:
>> - Commend about required lock replaced with ASSERT()
>> - Style fixes
>> - call to vpci_translate_virtual_device folded into vpci_sbdf_from_gpa
>> Since v8:
>> - locks moved out of vpci_translate_virtual_device()
>> Since v6:
>> - add pcidevs locking to vpci_translate_virtual_device
>> - update wrt to the new locking scheme
>> Since v5:
>> - add vpci_translate_virtual_device for #ifndef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> case to simplify ifdefery
>> - add ASSERT(!is_hardware_domain(d)); to vpci_translate_virtual_device
>> - reset output register on failed virtual SBDF translation
>> Since v4:
>> - indentation fixes
>> - constify struct domain
>> - updated commit message
>> - updates to the new locking scheme (pdev->vpci_lock)
>> Since v3:
>> - revisit locking
>> - move code to vpci.c
>> Since v2:
>> - pass struct domain instead of struct vcpu
>> - constify arguments where possible
>> - gate relevant code with CONFIG_HAS_VPCI_GUEST_SUPPORT
>> New in v2
>> ---
>> xen/arch/arm/vpci.c | 45 ++++++++++++++++++++++++++++++++---------
>> xen/drivers/vpci/vpci.c | 24 ++++++++++++++++++++++
>> xen/include/xen/vpci.h | 12 +++++++++++
>> 3 files changed, 71 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index b63a356bb4a8..516933bebfb3 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -7,33 +7,53 @@
>>
>> #include <asm/mmio.h>
>>
>> -static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge,
>> - paddr_t gpa)
>> +static bool vpci_sbdf_from_gpa(struct domain *d,
>> + const struct pci_host_bridge *bridge,
>> + paddr_t gpa, pci_sbdf_t *sbdf)
>> {
>> - pci_sbdf_t sbdf;
>> + bool translated = true;
>> +
>> + ASSERT(sbdf);
>>
>> if ( bridge )
>> {
>> - sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr);
>> - sbdf.seg = bridge->segment;
>> - sbdf.bus += bridge->cfg->busn_start;
>> + sbdf->sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr);
>> + sbdf->seg = bridge->segment;
>> + sbdf->bus += bridge->cfg->busn_start;
>> }
>> else
>> - sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE);
>> + {
>> + /*
>> + * For the passed through devices we need to map their virtual SBDF
>> + * to the physical PCI device being passed through.
>> + */
>> + sbdf->sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE);
>> + read_lock(&d->pci_lock);
>> + translated = vpci_translate_virtual_device(d, sbdf);
>> + read_unlock(&d->pci_lock);
>
> I would consider moving the read_{,un}lock() calls inside
> vpci_translate_virtual_device(), if that's the only caller of
> vpci_translate_virtual_device().
This is a good idea.
> Maybe further patches add other
> instances that call from an already locked context.
In a downstream, we're calling vpci_translate_virtual_device() to
enable PCI passthrough for PVH domUs on x86. In that case, moving the
lock would be equally beneficial.
