Re: [PATCH v3 05/11] vpci/header: Implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 26.10.21 10:50, Roger Pau Monné wrote:
> On Thu, Sep 30, 2021 at 10:52:17AM +0300, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> 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@xxxxxxxx>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> 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@xxxxxxxx>
>> ---
>>   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;
> I think this is missing a check for whether the BAR is the high part
> of a 64bit one? Ie:
>
> struct vpci_bar *bar = data;
> bool hi = false;
>
> if ( bar->type == VPCI_BAR_MEM64_HI )
> {
>      ASSERT(reg > PCI_BASE_ADDRESS_0);
>      bar--;
>      hi = true;
> }
>
> return bar->guest_addr >> (hi ? 32 : 0);
>
> Or else when accessing the high part of a 64bit BAR you will always
> return 0s as it hasn't been setup by guest_bar_write.
Yes, you are right
> Thanks, Roger.
Thank you,
Oleksandr