Re: [PATCH v16 4/5] xen/arm: translate virtual PCI bus topology for guests

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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_andrushchenko@xxxxxxxx>
>>
>> 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@xxxxxxxx>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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.