Re: [PATCH v4 05/11] vpci/header: implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 19.11.21 14:37, Jan Beulich wrote:
> On 19.11.2021 13:10, Oleksandr Andrushchenko wrote:
>> On 19.11.21 13:58, Jan Beulich wrote:
>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>
>>>> 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: 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.
>>> At least in the initial days of EFI there was the concept of EFI byte
>>> code, for ROM code to be compiled to such that it would be arch-
>>> independent. While I don't mean this to be an argument against leaving
>>> out ROM BAR handling for now, this may want mentioning here to avoid
>>> giving the impression that it's only x86 which might be affected by
>>> this deliberate omission.
>> I can put:
>> at the moment PCI expansion ROM handling is supported for x86 only
>> and it might not be used by other architectures without emulating x86.
> Sounds at least somewhat better to me.
>
>>>> --- a/xen/drivers/vpci/header.c
>>>> +++ b/xen/drivers/vpci/header.c
>>>> @@ -408,6 +408,48 @@ 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)
>>>> +{
>>>> +    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)
>>>> +{
>>>> +    const 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);
>>> I'm afraid "guest_addr" then isn't the best name; maybe "guest_val"?
>>> This would make more obvious that there is a meaningful difference
>>> from "addr" besides the guest vs host aspect.
>> I am not sure I can agree here:
>> bar->addr and bar->guest_addr make it clear what are these while
>> bar->addr and bar->guest_val would make someone go look for
>> additional information about what that val is for.
> Feel free to replace "val" with something more suitable. "guest_bar"
> maybe? The value definitely is not an address, so "addr" seems
> inappropriate / misleading to me.
This is a guest's view on the BAR's address. So to me it is still guest_addr
>
> Jan
>
Thank you,
Oleksandr